Proof of concept for reverse tunnel#456
Conversation
0bb3c83 to
4792f27
Compare
There was a problem hiding this comment.
Pull request overview
Proof-of-concept implementation of a reverse tunnel from each remote cluster back to the cluster manager, plus a management-API proxy endpoint intended to serve the LXD-UI through that tunnel. This builds on the earlier spike work in #207 by introducing a shared tunnel store, websocket handling, and some auth/token persistence needed for proxying.
Changes:
- Add reverse-tunnel plumbing: websocket endpoint on the cluster-connector, shared tunnel connection store, and HTTP-to-tunnel request/response forwarding.
- Add management-API tunnel proxy endpoint and expose tunnel connectivity status via the REST model to the UI.
- Add storage for OIDC access tokens (DB + encryption helpers) and a
user_secretcookie to support per-user encryption.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/types/cluster.d.ts | Adds tunnel_connected flag to cluster type for UI display. |
| ui/src/pages/clusters/ClusterDetail.tsx | Displays tunnel connectivity notification in cluster detail UI. |
| internal/pkg/types/rest.go | Introduces TunnelStore/Tunnel structs and tunnel request/response types. |
| internal/pkg/middleware/request.go | Adjusts request logging to better support websocket upgrades and hijacking. |
| internal/pkg/database/store/user_access_tokens.go | Adds DB access helpers for storing/retrieving per-user access tokens. |
| internal/pkg/database/store/remote_cluster_detail.go | Adds tunnel_member_url persistence to track which member holds a tunnel. |
| internal/pkg/database/schema/migrations/00001_db.sql | Adds tunnel_member_url column to remote_cluster_details. |
| internal/pkg/database/schema/migrations/00002_db.sql | Adds user_access_tokens table for access token storage. |
| internal/pkg/config/cypher.go | Adds helper functions to create secrets and encrypt/decrypt user values. |
| internal/pkg/api/models/v1/remote_cluster.go | Exposes tunnel_connected in the API model. |
| internal/pkg/api/api.go | Initializes and wires a shared TunnelStore into route config. |
| internal/app/management-api/core/auth/oidc.go | Adds user_secret cookie handling and stores encrypted access token in DB. |
| internal/app/management-api/api/v1/remote_cluster.go | Adds management-API tunnel proxy endpoint and sets tunnel_connected. |
| internal/app/management-api/api/v1/auth.go | Passes route config into OIDC callback handler. |
| internal/app/cluster-connector/api/v1/remote_cluster.go | Adds websocket tunnel endpoint and HTTP forwarding endpoints in cluster-connector. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
89d075a to
09bb67e
Compare
cd509d6 to
fb42e7a
Compare
fc7dea7 to
32133a5
Compare
7bde0ff to
cde8890
Compare
d1c1b43 to
4941268
Compare
6cec051 to
8845bd0
Compare
omarelkashef
left a comment
There was a problem hiding this comment.
I did an initial review. Very good PR! A few comments below
| MemberStatuses json.RawMessage `db:"member_statuses"` // JSON array of member statuses | ||
| StoragePoolUsages json.RawMessage `db:"storage_pool_usages"` // JSON array of storage pool usages | ||
| UIURL string `db:"ui_url"` // UI URL | ||
| TunnelMemberURL string `db:"tunnel_member_url"` // Cluster manager member url that holds the tunnel |
There was a problem hiding this comment.
The name made me think the variable refers to a member in the cluster but the comment implies a member in cluster manager.
There was a problem hiding this comment.
Any more specific terminology I can think of become very bulky, hence I went with this one. Do you have a suggestion?
There was a problem hiding this comment.
What do you think of TunnelManagerMemberURL or TunnelHostMemberURL?
| InstanceStatuses: instanceStatuses, | ||
| StoragePoolUsages: storagePoolUsages, | ||
| UIURL: e.UIURL, | ||
| TunnelConnected: e.TunnelMemberURL != "", |
There was a problem hiding this comment.
Should this be a more thorough check to make sure the member is reachable?
There was a problem hiding this comment.
I would trust the database state here.
There was a problem hiding this comment.
I would trust the database state here.
The database state is correct, but it does not guarantee reachability. That would be fine if we do not want the overhead of testing reachability and we will return the error to the caller.
There was a problem hiding this comment.
If we want to keep it this way, we can use "established" or "registered" (at least in the UI) instead of "connected" because I also noticed in the UI we inform the user that the tunnel is connected which implies that they can use it.
Signed-off-by: David Edler <david.edler@canonical.com>
|
Thanks for the review @omarelkashef please have another pass at the changes. |
omarelkashef
left a comment
There was a problem hiding this comment.
I did another round of review. Logically, it looks good. Only a few suggestions.
| UserSessions map[string]string | ||
| } | ||
|
|
||
| // ClusterManagerTunnelRequest represents the request received through the tunnel. |
There was a problem hiding this comment.
These comments are confusing. How about "request forwarded over reverse tunnel" here and "response received from to the reverse tunnel" below?
| InstanceStatuses: instanceStatuses, | ||
| StoragePoolUsages: storagePoolUsages, | ||
| UIURL: e.UIURL, | ||
| TunnelConnected: e.TunnelMemberURL != "", |
There was a problem hiding this comment.
I would trust the database state here.
The database state is correct, but it does not guarantee reachability. That would be fine if we do not want the overhead of testing reachability and we will return the error to the caller.
| type Tunnel struct { | ||
| Mu sync.RWMutex | ||
| WsConn *websocket.Conn | ||
| PendingCalls map[string]chan ClusterManagerTunnelResponse |
There was a problem hiding this comment.
PendingCalls sounds like “pending send” rather than “waiting response”. How about PendingResponses, InFlightResponseCh, ResponseChByRequestID, etc.?
| return | ||
| } | ||
|
|
||
| tunnel.WsConn = nil |
There was a problem hiding this comment.
Should we also clear UserSessions and PendingCalls (the whole tunnel entry in TunnelByCluster) here to avoid memory leakage or leave UserSessions for future connection? We can also do the cleanup on remote cluster delete.
| InstanceStatuses: instanceStatuses, | ||
| StoragePoolUsages: storagePoolUsages, | ||
| UIURL: e.UIURL, | ||
| TunnelConnected: e.TunnelMemberURL != "", |
There was a problem hiding this comment.
If we want to keep it this way, we can use "established" or "registered" (at least in the UI) instead of "connected" because I also noticed in the UI we inform the user that the tunnel is connected which implies that they can use it.
| tunnel.Mu.Unlock() | ||
| return response.SmartError(errors.New("Tunnel not connected")).Render(w, r) | ||
| } | ||
| err = wsConn.WriteJSON(req) |
There was a problem hiding this comment.
I am not sure that this part where we send the request here and wait for the channel response coming from remoteClusterWsGet is easily understood on the first try, but I can not think of a better way to do it.
Done
Draft because there are open issues
Follow up of #207