feat: log tailnet tunnels to the connection log - #27423
Conversation
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
565ac81 to
761271b
Compare
Documentation CheckUpdates Needed
No new pages are needed: the Automated review via Coder Agents |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 761271b49c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Agent-reported SSH/VSCode/JetBrains connection_log rows have no user_id
because the agent does not know which Coder user connected (see the
comment in coderd/agentapi/connectionlog.go). This makes it impossible
to attribute SSH/IDE sessions to a Coder user from the connection log.
Every such session is carried over a tailnet tunnel that the client
opens via /api/v2/workspaceagents/{id}/coordinate using the user's API
key, so coderd knows the user at that point.
Add a new connection_type 'tailnet' and write one connection_log row
from workspaceAgentClientCoordinate whenever an authenticated user
successfully upgrades the coordinate WebSocket. The row carries
user_id, ip, user_agent, workspace_id and agent_name, and is rendered
via WebInfo alongside workspace_app / port_forwarding.
Address review feedback on the original changeset: - Rebase onto main and renumber the migration to 000551 to resolve the collision with 000539_ai_provider_icons. - Rename the connection_type value tailnet to tunnel across the migration, codersdk, enterprise converter, and frontend, since the enum value is permanent API surface and should name the event rather than the transport. - Implement a real down migration that recreates connection_type without the tunnel value, following the pattern in 000533_nats_ca_crypto_key_feature.down.sql. - Update the remaining connection_logs column comments (slug_or_port, disconnect_time, disconnect_reason) to the agent-reported vs coderd-reported taxonomy. - Extract the connection log write into api.logTunnelConnection and bound it with a 3s timeout so connection log backpressure cannot stall tunnel establishment. - Cover the status-filter exclusion with a tunnel fixture in TestConnectionLogsOffsetFilters and the WebInfo mapping with a WebInfoTunnel subtest. - Harden the coordinate test: testutil.Context, AwaitReachable assertion, Code/ConnectionStatus expectations, and a second dial asserting each handshake produces its own row. - Document tunnel connections and their semantics in docs/admin/monitoring/connection-logs.md. - Clarify the connectionTypeIsWeb helper and replace the unreachable Unauthenticated user copy in the tunnel description branch. Co-authored-by: Chris DiGiamo <cdigiamo@anthropic.com>
Reuse the workspace_app_audit_sessions mechanism to collapse tunnel reconnections into one connection log row per active session. Clients automatically re-dial the coordinate endpoint after network blips, load balancer timeouts, and coderd restarts, so logging every handshake would flood the connection log with reconnect noise that is indistinguishable from genuine user activity. Tunnel sessions are keyed on (agent, user, IP, user agent) with app_id = uuid.Nil and an empty slug, matching how port forwarding uses the table; status code 101 keeps them from colliding with app sessions. A new row is logged only when no session exists for the key or the session has been idle past the stale interval (1 hour by default, sliding). On dedup failure the log write is skipped, matching the workspace app precedent of not spamming the connection log during database problems.
Removing an enum value requires recreating connection_type and rewriting the connection_logs type column, which takes an exclusive lock on the table and would have to delete all tunnel rows because they cannot exist in the old type. Leaving the value in place is harmless and matches the precedent of other enum-value additions.
Explain how the status code keeps tunnel audit sessions from colliding with app and port forwarding sessions: the status code is part of the session unique key, and app sessions record the token authorization status (200, 4xx) while tunnel sessions always record 101, so the keys can never conflict. Drop the redundant 'failed' suffix from the error log messages; the Error level already implies failure.
…dline Renumber the tunnel enum migration from 000551 to 000556 after rebasing onto main, which gained migrations 000551 through 000555. Give the connection log enqueue its own 3s deadline detached from the audit session upsert's budget. Sharing one deadline meant a slow session upsert could exhaust it before the enqueue ran, committing the session row with no log row written; the active session would then suppress retries for the entire stale interval, dropping the tunnel from the audit log for up to an hour after the database recovered.
…tion Keep the migration to the enum addition only. The prior comment rewording was cosmetic and added churn to both the up and down migrations.
Use one named writeCtx for both the audit session upsert and the connection log enqueue instead of a shadowed ctx plus a separate enqueue deadline, inline the system-restricted context, and rename newOrStale to newSession. The shared-budget tradeoff (a slow session upsert can drop the log row until the session goes stale) is documented at the timeout.
Main gained 000556_user_secrets_enabled.
79d9c3e to
14bf16c
Compare
Emyrk
left a comment
There was a problem hiding this comment.
Comments inline. The mechanism is sound — dedupe key verified collision-free against every existing session writer, status-filter exclusion is the only consistent choice, and the docs' interpretation caveats are unusually good. Main open question is the attribution gaps (workspace proxies, Coder Desktop).
Coder Agents on behalf of @Emyrk.
| // workspace-proxy-authenticated requests carry no API key and are | ||
| // skipped. | ||
| func (api *API) logTunnelConnection(ctx context.Context, r *http.Request, waws database.GetWorkspaceAgentAndWorkspaceByIDRow) { | ||
| apiKey, ok := httpmw.APIKeyOptional(r) |
There was a problem hiding this comment.
Checked the proxy path: the skip here is correct by design. Proxy-authenticated coordinates (RequireAPIKeyOrWorkspaceProxyAuth) are shared proxy↔agent tunnels carrying many users' traffic, and per-user attribution for proxied activity already happens at the app layer (the wsproxy's workspaceapps provider writes workspace_app/port_forwarding/terminal rows with user identity). Logging them as tunnel rows would just be unattributed noise.
That leaves Coder Desktop / the user-scoped tailnet API as the one real attribution gap — is that tracked as a follow-up?
Coder Agents on behalf of @Emyrk.
Emyrk
left a comment
There was a problem hiding this comment.
The empty-slug reservation is now programmatically enforced in workspaceapps/db.go (8c22808) — exactly what I asked for. Verified the dedupe key against every session writer, and the workspace-proxy skip is correct by design (proxied activity is attributed at the app layer). The Coder Desktop / user-scoped tailnet API question stands as a non-blocking follow-up.
Coder Agents on behalf of @Emyrk.
Agent-reported SSH/IDE connection log rows can't identify the connecting user, but the tailnet tunnel carrying those sessions is opened via the coordinate endpoint with the user's API key. This PR adds a
tunnelconnection type and logs a row there with the user's identity, IP, and user agent. Events are deduplicated per (user, agent, IP, client) through the existingworkspace_app_audit_sessionsmechanism, so client reconnect churn doesn't flood the log.Closes #27006
Supersedes #27005