-
Notifications
You must be signed in to change notification settings - Fork 943
chore!: route connection logs to new table #18340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Requesting a draft review just to confirm we're happy with the DB schema, the RBAC setup, and the overall direction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I think this looks great in general, but the upsert part of the logic here doesn't fully make sense to me (see related comment). Could you elaborate on the intent?
947ccd1
to
5231bef
Compare
e4e1a04
to
1fcd8d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nits and suggestions but largely looks alright to me, nice work! 👍🏻
Also, obligatory: ./coderd/database/migrations/fix_migration_numbers.sh
.
f47a3f7
to
0346609
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a dedicated connection_logs
table and replaces audit-based connection events with a new ConnectionLogger
abstraction, ensuring SSH and Workspace App connections are recorded in their own table and UI. Key changes include:
- Schema and SQL migrations for
connection_logs
, plus generated Go query methods and RBAC filters. - New
ConnectionLogger
abstraction in both enterprise and open core, replacing audit calls for connection events. - Updates across CLI, API server, agent API, and tests to route connection events through
ConnectionLogger
.
Reviewed Changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
enterprise/coderd/connectionlog/connectionlog.go | Implements Backend and ConnectionLogger types |
coderd/database/migrations/000349_connection_logs.up.sql | Creates connection_logs table and types |
coderd/database/queries/connectionlogs.sql | SQL for reading and upserting connection logs |
coderd/workspaceapps/db.go | Replaces audit logic with connection logging |
coderd/agentapi/connectionlog.go | New agent API endpoint for reporting connections |
coderd/database/types.go | Adds ParseIP helper for inet type |
coderd/database/queries.sql.go | Adds generated GetConnectionLogsOffset and UpsertConnectionLog methods |
Comments suppressed due to low confidence (2)
coderd/workspaceapps/db.go:97
- [nitpick] The variable name
commitAudit
is misleading since this sets up connection logging, not audit logging. Consider renaming it tocommitConnLog
orcommitConnectionLog
for clarity.
aReq, commitAudit := p.connLogInitRequest(ctx, rw, r)
coderd/workspaceapps/db.go:394
- [nitpick] The comment uses “connect log” where “connection log” would match the terminology used elsewhere. Consider updating for consistency.
// connLogInitRequest creates a new connection log session and connect log for the
7d13236
to
bea35a3
Compare
bea35a3
to
3164ea2
Compare
Merge activity
|
Breaking Change (changelog note):
Context
This is the first PR of a few for moving connection events out of the audit log, and into a new database table and web UI page called the 'Connection Log'.
This PR:
ConnectionLogger
abstraction to replace theAuditor
abstraction for these logs. (No-op'd in AGPL, like theAuditor
)ConnectionLogger
ConnectionLogger
instead of theAuditor
.Future PRs:
Note
The PRs in this stack obviously won't be (completely) atomic. Whilst they'll each pass CI, the stack is designed to be merged all at once. I'm splitting them up for the sake of those reviewing, and so changes can be reviewed as early as possible. Despite this, it's really hard to make this PR any smaller than it already is. I'll be keeping it in draft until it's actually ready to merge.