NotificationsYou must be signed in to change notification settings
Fork1k
Star11.2k

feat: Add workspace agent for SSH#318

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

Jump to bottom

Merged

kylecarbs merged 19 commits intomainfromworkspaceagent

Feb 19, 2022

Merged

feat: Add workspace agent for SSH#318

kylecarbs merged 19 commits intomainfromworkspaceagent

Feb 19, 2022

Conversation

Copy link

Member

kylecarbs commentedFeb 18, 2022

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 requested a review frombryphe-coder

February 18, 2022 00:47

kylecarbs self-assigned this

Feb 18, 2022

kylecarbs force-pushed theworkspaceagent branch from4fe2a63 tocab5f15Compare

February 18, 2022 00:51

Copy link

codecovbot commentedFeb 18, 2022•
edited
Loading

Codecov Report

Merging#318 (516b605) intomain (65de96c) willincrease coverage by0.16%.
The diff coverage is63.73%.

@@            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

Flag	Coverage Δ
unittest-go-macos-latest	`66.49% <65.06%> (+0.47%)`	⬆️
unittest-go-ubuntu-latest	`67.57% <64.77%> (+0.14%)`	⬆️
unittest-go-windows-2022	`65.95% <64.12%> (+0.16%)`	⬆️
unittest-js	`63.61% <ø> (ø)`

Impacted Files	Coverage Δ
pty/pty_other.go	`35.89% <0.00%> (ø)`
pty/start_windows.go	`51.30% <20.00%> (-0.95%)`	⬇️
agent/usershell/usershell_other.go	`60.00% <60.00%> (ø)`
agent/agent.go	`66.08% <66.08%> (ø)`
pty/ptytest/ptytest.go	`90.74% <66.66%> (+0.94%)`	⬆️
pty/start_other.go	`72.00% <66.66%> (+1.16%)`	⬆️
pty/pty_windows.go	`60.31% <80.00%> (+1.30%)`	⬆️
coderd/coderdtest/coderdtest.go	`100.00% <100.00%> (ø)`
pty/start.go	`100.00% <100.00%> (ø)`
... and4 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update65de96c...516b605. Read thecomment docs.

bryphe-coder reviewed

Feb 18, 2022

View reviewed changes

agent/agent.go

		@@ -0,0 +1,293 @@
		package agent

Copy link

Contributor

Choose a reason for hiding this comment

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

Will this agent be run as a standalone executable, or part ofcoderd? If it is the former - maybe it would make sense to beagentd?

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Part of thecoder 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 ascoder agent start, or something of the like.

I initially had this incli/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.

Copy link

Contributor

bryphe-coderFeb 18, 2022

Choose a reason for hiding this comment

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

Thanks for all the helpful details@kylecarbs !

It'll be added ascoder agent start, or something of the like.

That sounds good!

I initially had this incli/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 👍

bryphe-coder reviewed

Feb 18, 2022

View reviewed changes

agent/agent.go Outdated

		returnnil,err
		}
		sshConn,channels,requests,err:=gossh.NewClientConn(netConn,"localhost:22",&gossh.ClientConfig{
		User:"kyle",

Copy link

Contributor

bryphe-coderFeb 18, 2022

Choose a reason for hiding this comment

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

Will this user be configurable?

Copy link

MemberAuthor

kylecarbsFeb 18, 2022

Choose a reason for hiding this comment

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

Oops, yes haha!

kylecarbs force-pushed theworkspaceagent branch 2 times, most recently from7ec58fb to2c477aaCompare

February 18, 2022 03:57

bryphe-coder approved these changes

Feb 18, 2022

View reviewed changes

Copy link

Contributor

bryphe-coder left a comment

Choose a reason for hiding this comment

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

Just some questions inline - but overall looks great. Exciting to have theagent piece come together. Hopefully that means we canssh into workspaces soon 😄

agent/agent.go OutdatedShow resolvedHide resolved

agent/agent_test.go OutdatedShow resolvedHide resolved

agent/agent_test.goShow resolvedHide resolved

agent/usershell/usershell_other.goShow resolvedHide resolved

templates/null/main.tfShow resolvedHide resolved

bryphe-coder mentioned this pull request

Feb 18, 2022

refactor: Add minimal terraform project#308

Merged

feat: Add workspace agent for SSH

fa7489a

This adds the initial agent that supports TTYand 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 theworkspaceagent branch from2c477aa tofa7489aCompare

February 18, 2022 04:19

bryphe-coder reviewed

Feb 18, 2022

View reviewed changes

agent/agent_test.goShow resolvedHide resolved

bryphe-coder reviewed

Feb 18, 2022

View reviewed changes

pty/start_windows.go Outdated

		returnnil,nil,xerrors.Errorf("find process %d: %w",processInfo.ProcessId,err)
		}
		gofunc() {
		_,_=process.Wait()

Copy link

Contributor

bryphe-coderFeb 18, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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 thepty is closed before being fully read by the test.

Copy link

MemberAuthor

kylecarbsFeb 18, 2022

Choose a reason for hiding this comment

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

Yup, will add!

Copy link

MemberAuthor

kylecarbsFeb 18, 2022

Choose a reason for hiding this comment

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

CI caught a real issue here! Fixed in42656a4

Merge branch 'main' into workspaceagent

2606fda

kylecarbs force-pushed theworkspaceagent branch 2 times, most recently from11e5274 to0c755fbCompare

February 18, 2022 05:33

Fix pty tests on Windows

b54e815

kylecarbs force-pushed theworkspaceagent branch from0c755fb tob54e815Compare

February 18, 2022 05:39

kylecarbs added11 commits

February 18, 2022 15:01

Fix log race

c484d47

Lock around dial error to fix log output

cc2bcde

Fix context return early

ae36c63

fix: Leaking yamux session after HTTP handler is closed

4932ba7

Closes#317. We depended on the context canceling the yamux connection,but this isn't a sync operation. Explicitly calling close ensures thehandler waits for yamux to complete before exit.

Merge branch 'main' into closeconn

0c84636

Lock around close return

5601b4d

Merge branch 'main' into closeconn

94cf442

Force failure with log

055ce11

Merge branch 'main' into closeconn

94e03a2

Merge branch 'closeconn' into workspaceagent

f8566f3

Fix failed handler

70d3723

kylecarbs added5 commits

February 18, 2022 21:35

Upgrade dep

223540e

Fix defer inside loops

1a48bea

Fix context cancel for HTTP requests

ee0ee70

Merge branch 'main' into workspaceagent

30189aa

Fix resize

516b605

kylecarbs merged commit91bf863 intomain

Feb 19, 2022

kylecarbs deleted the workspaceagent branch

February 19, 2022 05:13

Labels

None yet

Movatterモバイル変換

feat: Add workspace agent for SSH#318

feat: Add workspace agent for SSH#318

Uh oh!

Conversation

kylecarbs commentedFeb 18, 2022

Uh oh!

codecovbot commentedFeb 18, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bryphe-coderFeb 18, 2022

Choose a reason for hiding this comment

Uh oh!

kylecarbsFeb 18, 2022

Choose a reason for hiding this comment

Uh oh!

bryphe-coderFeb 18, 2022

Choose a reason for hiding this comment

Uh oh!

bryphe-coderFeb 18, 2022

Choose a reason for hiding this comment

Uh oh!

kylecarbsFeb 18, 2022

Choose a reason for hiding this comment

Uh oh!

bryphe-coder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryphe-coderFeb 18, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylecarbsFeb 18, 2022

Choose a reason for hiding this comment

Uh oh!

kylecarbsFeb 18, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecovbot commentedFeb 18, 2022•
edited
Loading

bryphe-coderFeb 18, 2022•
edited
Loading