feat(agent): add connection reporting for SSH and reconnecing PTY #16652
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
1b8c15c to
cd31d7e
Compare
cd31d7e to
58463c6
Compare
4f934c8 to
a77ceac
Compare
| a.reportConnectionsMu.Lock() | ||
| if len(a.reportConnections) == 0 { | ||
| a.reportConnectionsMu.Unlock() | ||
| break |
There was a problem hiding this comment.
Do we need a label here for clarity? break will always break the innermost loop but I always have trouble remembering that personally.
There was a problem hiding this comment.
If you review in VS Code, the syntax highlighting can be helpful here!
There was a problem hiding this comment.
I only use labels when I need to. I don't think we have examples in our code for labels breaking the inner loop. Do we?
There was a problem hiding this comment.
You wrote one a while back ;-) https://github.com/coder/coder/pull/14578/files#diff-f8b1ec0d615f1374c7f54e81c7871b337f92f8749e5608a155227c71160fafc8R48
| } | ||
|
|
||
| func (s *sessionCloseTracker) Close() error { | ||
| s.track(1) |
There was a problem hiding this comment.
What does 1 mean in this instance?
There was a problem hiding this comment.
Same as a shell script exiting with generic error (1). Calling Close instead of Exit on the session indicates an "error" state (not error as in log, but error as in session didn't go right).
| { | ||
| Flag: "experimental-connection-reports-enable", | ||
| Hidden: true, | ||
| Default: "false", | ||
| Env: "CODER_AGENT_EXPERIMENTAL_CONNECTION_REPORTS_ENABLE", | ||
| Description: "Enable experimental connection reports.", | ||
| Value: serpent.BoolOf(&experimentalConnectionReports), | ||
| }, |
There was a problem hiding this comment.
Should this be something that gets set as a deployment-wide experiment and is automatically set in the agent manifest?
There was a problem hiding this comment.
The reason this is an (temporary) experiment is simply to not mess up the audit log UI until it is updated. I'm not aware that this needs to be placed behind an experiment deployment wide.
There was a problem hiding this comment.
Just an alternative suggestion!
| a.reportConnectionsMu.Lock() | ||
| if len(a.reportConnections) == 0 { | ||
| a.reportConnectionsMu.Unlock() | ||
| break |
There was a problem hiding this comment.
I only use labels when I need to. I don't think we have examples in our code for labels breaking the inner loop. Do we?
|
|
||
| // Remove the payload we sent. | ||
| a.reportConnectionsMu.Lock() | ||
| a.reportConnections = a.reportConnections[1:] |
There was a problem hiding this comment.
Why not just make reportConnections channel? If it's an append only slice that is only read in this function.
This slice behavior is correct, just feels like a weaker implementation of a channel.
There was a problem hiding this comment.
I guess it could be a channel, but how big to make it? How many in-flight reports is "too many"?
There was a problem hiding this comment.
That's fair, a channel would not be a terrible option here. It requires upfront allocation which can either be a good or a bad thing in memory constrained systems. For now I've limited this to 2048 reports pending, or about 300KB. We can revisit this later if needed.
agent/agent.go
Outdated
| a.reportConnections = append(a.reportConnections, &proto.ReportConnectionRequest{ | ||
| Connection: &proto.Connection{ | ||
| Id: id[:], | ||
| Action: proto.Connection_CONNECT, | ||
| Type: connectionType, | ||
| Timestamp: timestamppb.New(time.Now()), | ||
| Ip: ip, | ||
| StatusCode: 0, | ||
| Reason: nil, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Do we have to worry about the size of this slice?
There was a problem hiding this comment.
Typically no. The only way these accumulate is if we have lost connection to coders. I could add some safe-guards around this to drop messages, but since it's part of auditing that feels a bit wrong. WDYT?
There was a problem hiding this comment.
I'll defer to your judegment. If you feel it is relatively bounded, then it's good with me. Maybe leave a comment of the assumptions?
I did not spend that long understanding the full context of the code.
c77b24e to
0318212
Compare
Updates #15139
To allow merging, I've placed the reporting behind an experimental flag on the agent (
--experimental-connection-reports-enable).See #15139 for how these are represented in the UI (and plans for updating the UI).