feat: add agentapi endpoint to report connections for audit #16507
Conversation
There was a problem hiding this comment.
Overall this makes sense to me.
To me it just comes down to using workspace_id or the agent_id as the audit log's resource id.
I really think using workspace_id makes the most sense, as if you search for a workspace, we should show all audit logs related to said workspace.
Saying that though, the correct thing is the agent id....
Random thought. We have AdditionalFields. Could we just add a custom query option to GetAuditLogsOffset that searches audit logs by their json body payload for the workspace id? Would that be too slow? Should we just add another column and allow a workspace_id for audit logs?
8128b7e to
de017dc
Compare
841b6ca to
724efff
Compare
de017dc to
640a0c3
Compare
ac60d34 to
761ba0a
Compare
dcdf3bd to
88c0862
Compare
debcf9c to
58f1d2a
Compare
cc57f22 to
3edbd1a
Compare
a9b4adf to
88f22b7
Compare
3edbd1a to
ab51fea
Compare
88f22b7 to
469e2cd
Compare
ccfd326 to
f9c6b31
Compare
@spikecurtis thanks for the shoutout. I thought about this yesterday and I think there is value in bumping the version on Edit: Well as I posted that, I just realized that if they're cherry-picked out of order that's no good 😅 |
|
We should bump on |
| google.protobuf.Timestamp timestamp = 4; | ||
| string ip = 5; | ||
| int32 status_code = 6; | ||
| optional string reason = 7; |
There was a problem hiding this comment.
I'm unclear on what goes in this field in practice.
There was a problem hiding this comment.
What change would you like to see? It's a free-text reason as to why this event happened. Typically it will be used on disconnect to report why the connection ended, if we know. It could also be reported on connect if e.g. file transfer is blocked (i.e. failed connect, although it could be argued that it should be a connect+immediate disconnect). I can rename it to disconnect reason if you prefer it more explicit, should be fine even if more restrictive.
This change adds a new `ReportConnection` endpoint to the `agentapi` and bumps the protocol version. This allows the agent to report connection events, for example when the user connects to the workspace via SSH or VS Code. Updates #15139
f9c6b31 to
632bf98
Compare
| } | ||
|
|
||
| // Fetch contextual data for this audit event. | ||
| workspaceAgent, err := a.AgentFn(ctx) |
There was a problem hiding this comment.
suggestion: check that AgentFn is not nil; this could panic otherwise.
There was a problem hiding this comment.
It's a good point, but one that should be solved for the entire package. Each sub-API on agentapi.API has this same potential edge-case. The assumption seems to be that New will correctly assign it, otherwise it's a developer error.
Testing for that in a future proof way is of course an issue. As is allowing Coder to start at all if it's missing.
| workspace, err := a.Database.GetWorkspaceByAgentID(ctx, workspaceAgent.ID) | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("get workspace by agent id: %w", err) | ||
| } | ||
| build, err := a.Database.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspace.ID) | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("get latest workspace build by workspace id: %w", err) | ||
| } |
There was a problem hiding this comment.
non-blocking: It's unfortunate that GetWorkspaceByAgentID ends up doing a join on workspace_builds but disregarding the related row! I had a quick check and didn't see any other places that would benefit from returning this additional information though.
| @@ -43,6 +43,8 @@ import ( | |||
| // - Shipped in Coder v2.{{placeholder}} // TODO Vincent: Replace with the correct version | |||
This change adds a new
ReportConnectionendpoint to theagentapiandbumps the protocol version.
This allows the agent to report connection events, for example when the
user connects to the workspace via SSH or VS Code.
Updates #15139
Depends on #16493