Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: add agentapi endpoint to report connections for audit#16507

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
mafredri merged 4 commits intomainfrommafredri/feat-add-agentapi-for-connect-audit
Feb 20, 2025

Conversation

mafredri
Copy link
Member

@mafredrimafredri commentedFeb 10, 2025•
edited
Loading

This change adds a newReportConnection endpoint to theagentapi 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
Depends on#16493

Copy link
Member

@EmyrkEmyrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Overall this makes sense to me.

To me it just comes down to usingworkspace_id or theagent_id as the audit log's resource id.

I really think usingworkspace_id makes the most sense, as if you search for a workspace, we should showall audit logs related to said workspace.

Saying that though, thecorrect thing is the agent id....

Random thought. We haveAdditionalFields. Could we just add a custom query option toGetAuditLogsOffset 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 aworkspace_id for audit logs?

mafredri reacted with thumbs up emoji
@mafredrimafredriforce-pushed themafredri/feat-add-audit-types-for-connect-open-agent-app branch from8128b7e tode017dcCompareFebruary 11, 2025 12:03
@mafredrimafredriforce-pushed themafredri/feat-add-agentapi-for-connect-audit branch from841b6ca to724efffCompareFebruary 11, 2025 12:04
@mafredrimafredriforce-pushed themafredri/feat-add-audit-types-for-connect-open-agent-app branch fromde017dc to640a0c3CompareFebruary 11, 2025 12:10
@mafredrimafredriforce-pushed themafredri/feat-add-agentapi-for-connect-audit branch 2 times, most recently fromac60d34 to761ba0aCompareFebruary 11, 2025 12:13
@mafredrimafredriforce-pushed themafredri/feat-add-agentapi-for-connect-audit branch 4 times, most recently fromdcdf3bd to88c0862CompareFebruary 12, 2025 14:21
@mafredrimafredriforce-pushed themafredri/feat-add-audit-types-for-connect-open-agent-app branch fromdebcf9c to58f1d2aCompareFebruary 12, 2025 14:22
@mafredrimafredriforce-pushed themafredri/feat-add-agentapi-for-connect-audit branch 2 times, most recently fromcc57f22 to3edbd1aCompareFebruary 12, 2025 14:28
@mafredrimafredriforce-pushed themafredri/feat-add-audit-types-for-connect-open-agent-app branch froma9b4adf to88f22b7CompareFebruary 12, 2025 15:04
@mafredrimafredriforce-pushed themafredri/feat-add-agentapi-for-connect-audit branch from3edbd1a toab51feaCompareFebruary 12, 2025 15:04
@mafredrimafredri marked this pull request as ready for reviewFebruary 12, 2025 17:11
@mafredrimafredriforce-pushed themafredri/feat-add-audit-types-for-connect-open-agent-app branch from88f22b7 to469e2cdCompareFebruary 12, 2025 18:28
@mafredrimafredriforce-pushed themafredri/feat-add-agentapi-for-connect-audit branch fromccfd326 tof9c6b31CompareFebruary 12, 2025 18:31
@spikecurtisGraphite App
Copy link
Contributor

#16438 also changes the Agent API by adding some new RPCs. If these both land prior to the next release, then we only need to bump the minor version once to 2.4. If one lands but not the other, the latter will have to bump to 2.5. cc@defelmnq

@mafredri
Copy link
MemberAuthor

mafredri commentedFeb 13, 2025•
edited
Loading

#16438 also changes the Agent API by adding some new RPCs. If these both land prior to the next release, then we only need to bump the minor version once to 2.4. If one lands but not the other, the latter will have to bump to 2.5. cc@defelmnq

@spikecurtis thanks for the shoutout. I thought about this yesterday and I think there is value in bumping the version onmain for unreleased changes as well. It would make release patching/cherry-picking safer. (In this case I think it's unlikely we'll run into a conflict due to that, but sticking to a system helps avoids unforeseen situations down the line.)

Edit: Well as I posted that, I just realized that if they're cherry-picked out of order that's no good 😅

@spikecurtisGraphite App
Copy link
Contributor

We should bump onmain for unreleased changes! Just no need to bump more than once per release.

mafredri reacted with thumbs up emoji

google.protobuf.Timestamp timestamp = 4;
string ip = 5;
int32 status_code = 6;
optional string reason = 7;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm unclear on what goes in this field in practice.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Base automatically changed frommafredri/feat-add-audit-types-for-connect-open-agent-app tomainFebruary 17, 2025 13:02
This change adds a new `ReportConnection` endpoint to the `agentapi` andbumps the protocol version.This allows the agent to report connection events, for example when theuser connects to the workspace via SSH or VS Code.Updates#15139
@mafredrimafredriforce-pushed themafredri/feat-add-agentapi-for-connect-audit branch fromf9c6b31 to632bf98CompareFebruary 17, 2025 13:32
Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Changes LGTM as these are going in with OOM/OOD resource monitors stuff.

}

// Fetch contextual data for this audit event.
workspaceAgent, err := a.AgentFn(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

suggestion: check thatAgentFn is not nil; this could panic otherwise.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's a good point, but one that should be solved for the entire package. Each sub-API onagentapi.API has this same potential edge-case. The assumption seems to be thatNew 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.

johnstcn reacted with thumbs up emoji
Comment on lines +51 to +58
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)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

non-blocking: It's unfortunate thatGetWorkspaceByAgentID ends up doing a join onworkspace_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.

mafredri reacted with confused emoji
@@ -43,6 +43,8 @@ import (
// - Shipped in Coder v2.{{placeholder}} // TODO Vincent: Replace with the correct version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@mafredrimafredri merged commitb07b33e intomainFeb 20, 2025
32 checks passed
@mafredrimafredri deleted the mafredri/feat-add-agentapi-for-connect-audit branchFebruary 20, 2025 12:52
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsFeb 20, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@bpmctbpmctbpmct left review comments

@johnstcnjohnstcnjohnstcn approved these changes

@EmyrkEmyrkAwaiting requested review from Emyrk

@spikecurtisspikecurtisAwaiting requested review from spikecurtisspikecurtis is a code owner

Assignees

@mafredrimafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@mafredri@spikecurtis@johnstcn@Emyrk@bpmct
[8]ページ先頭
©2009-2025 Movatter.jp