feat: Add workspace agent for SSH #318
Conversation
kylecarbs
force-pushed
the
workspaceagent
branch
from
4fe2a63
to
cab5f15
Compare
Codecov Report
@@ Coverage Diff @@
## main #318 +/- ##
==========================================
+ Coverage 67.53% 67.70% +0.16%
==========================================
Files 140 142 +2
Lines 7443 7700 +257
Branches 77 77
==========================================
+ Hits 5027 5213 +186
- Misses 1905 1960 +55
- Partials 511 527 +16
Continue to review full report at Codecov.
|
| @@ -0,0 +1,293 @@ | |||
| package agent | |||
There was a problem hiding this comment.
Will this agent be run as a standalone executable, or part of coderd? If it is the former - maybe it would make sense to be agentd?
There was a problem hiding this comment.
Part of the coder CLI! I was debating the name and structure internally. The purpose is the listening agent inside of a workspace for enabling access. It'll be added as coder agent start, or something of the like.
I initially had this in cli/agent, but felt that unnecessarily nested core business logic. Beyond that, it's likely this package will be used for tests that aren't relevant to the CLI at all. eg. agenttest.New will likely exist.
I'd appreciate your thoughts here. It's more of an agent than a daemon, because it doesn't require system-level privileges, and is active rather than passive.
There was a problem hiding this comment.
Thanks for all the helpful details @kylecarbs !
It'll be added as
coder agent start, or something of the like.
That sounds good!
I initially had this in
cli/agent, but felt that unnecessarily nested core business logic.
Agreed, it seems like our convention here is to have packages at the top-level - don't see a compelling reason to switch that.
It's more of an agent than a daemon, because it doesn't require system-level privileges, and is active rather than passive.
Makes sense to me 👍
agent/agent.go
Outdated
| return nil, err | ||
| } | ||
| sshConn, channels, requests, err := gossh.NewClientConn(netConn, "localhost:22", &gossh.ClientConfig{ | ||
| User: "kyle", |
There was a problem hiding this comment.
Will this user be configurable?
kylecarbs
force-pushed
the
workspaceagent
branch
2 times, most recently
from
7ec58fb
to
2c477aa
Compare
This adds the initial agent that supports TTY and execution over SSH. It functions across MacOS, Windows, and Linux. This does not handle the coderd interaction yet, but does setup a simple path forward.
kylecarbs
force-pushed
the
workspaceagent
branch
from
2c477aa
to
fa7489a
Compare
pty/start_windows.go
Outdated
| return nil, nil, xerrors.Errorf("find process %d: %w", processInfo.ProcessId, err) | ||
| } | ||
| go func() { | ||
| _, _ = process.Wait() |
There was a problem hiding this comment.
It might be worth adding logging here too with the exit code of the process.
I saw this failure in a recent run:
ptytest.go:82:
Error Trace: ptytest.go:82
start_windows_test.go:24
Error: Received unexpected error:
read |0: file already closed
Test: TestStart/Echo
https://github.com/coder/coder/runs/5242887100?check_suite_focus=true#step:7:239
And I suspect this part of the change might be related. Having the logging could help give details here. It could be a race in that the process might exit and the pty is closed before being fully read by the test.
There was a problem hiding this comment.
CI caught a real issue here! Fixed in 42656a4
kylecarbs
force-pushed
the
workspaceagent
branch
2 times, most recently
from
11e5274
to
0c755fb
Compare
kylecarbs
force-pushed
the
workspaceagent
branch
from
0c755fb
to
b54e815
Compare
Closes #317. We depended on the context canceling the yamux connection, but this isn't a sync operation. Explicitly calling close ensures the handler waits for yamux to complete before exit.
This adds the initial agent that supports TTY
and execution over SSH. It functions across MacOS,
Windows, and Linux.
This does not handle the coderd interaction yet,
but does setup a simple path forward.