NotificationsYou must be signed in to change notification settings
Fork917
Star10.1k

feat(cli): allow SSH command to connect to running container#16726

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

johnstcn merged 4 commits intomainfromcj/cli-ssh-container

Feb 28, 2025

Merged

feat(cli): allow SSH command to connect to running container#16726

johnstcn merged 4 commits intomainfromcj/cli-ssh-container

Feb 28, 2025

Conversation

Copy link

Member

johnstcn commentedFeb 26, 2025•
edited
Loading

Fixes#16709 and#16420

Notes:

SFTP is currently not supported
Haven't tested X11 container forwarding
Haven't tested agent forwarding

johnstcn self-assigned this

Feb 26, 2025

johnstcn changed the title~~feat(cli): allow SSH command to connect to running container~~[ci skip] feat(cli): allow SSH command to connect to running container

Feb 27, 2025

johnstcn force-pushed thecj/cli-ssh-container branch from3692436 toe599cefCompare

February 27, 2025 09:16

johnstcn changed the title~~[ci skip] feat(cli): allow SSH command to connect to running container~~feat(cli): allow SSH command to connect to running container

Feb 27, 2025

johnstcn force-pushed thecj/cli-ssh-container branch from9780c67 toeec7378Compare

February 27, 2025 12:09

feat(cli): add capability for SSH command to connect to a running con…

3dc994a

…tainer

johnstcn force-pushed thecj/cli-ssh-container branch fromeec7378 to3dc994aCompare

February 27, 2025 12:19

johnstcn marked this pull request as ready for review

February 27, 2025 13:23

johnstcn requested review frommafredri andmtojek

February 27, 2025 13:23

johnstcn commented

Feb 27, 2025

View reviewed changes

agent/agentssh/agentssh.go OutdatedShow resolvedHide resolved

johnstcn commented

Feb 27, 2025

View reviewed changes

agent/agentssh/agentssh.go

		@@ -495,30 +526,35 @@ func (s *Server) fileTransferBlocked(session ssh.Session) bool {
		return false
		}

		func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, env []string, magicType MagicSessionType) (retErr error) {
		func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, env []string, magicType MagicSessionType, container, containerUser string) (retErr error) {

Copy link

MemberAuthor

johnstcnFeb 27, 2025

Choose a reason for hiding this comment

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

review: think it might be time to consider an opts struct?

Copy link

Member

mafredriFeb 27, 2025

Choose a reason for hiding this comment

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

It could be done, but no need to do that yet IMO. If this grows more then probably.

I'd actually want us to refactor the whole session handling and manage our own*session insession.go or some such. But that's a story for another time.

johnstcn commented

Feb 27, 2025

View reviewed changes

agent/agentssh/agentssh.go OutdatedShow resolvedHide resolved

johnstcn commented

Feb 27, 2025

View reviewed changes

agent/agentssh/agentssh.go Outdated

Comment on lines 332 to 344

		for _, kv := range env {
		if strings.HasPrefix(kv, "CODER_CONTAINER=") {
		container = strings.TrimPrefix(kv, "CODER_CONTAINER=")
		}

		if strings.HasPrefix(kv, "CODER_CONTAINER_USER=") {
		containerUser = strings.TrimPrefix(kv, "CODER_CONTAINER_USER=")
		}
		}

		return container, containerUser, slices.DeleteFunc(env, func(kv string) bool {
		return strings.HasPrefix(kv, "CODER_CONTAINER=") \|\| strings.HasPrefix(kv, "CODER_CONTAINER_USER=")
		})

Copy link

MemberAuthor

johnstcnFeb 27, 2025

Choose a reason for hiding this comment

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

review: inspired by the magic session stuff, this seems to be the only real way to smuggle info betweeen the client and the server.

johnstcn commented

Feb 27, 2025

View reviewed changes

cli/ssh.go OutdatedShow resolvedHide resolved

fixup! feat(cli): add capability for SSH command to connect to a runn…

56538a9

…ing container

mtojek approved these changes

Feb 27, 2025

View reviewed changes

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

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

Looking forward to X11 testing!

agent/agent.go OutdatedShow resolvedHide resolved

agent/agentssh/agentssh.go

		// ContainerEnvironmentVariable is used to specify the target container for an SSH connection.
		// This is stripped from any commands being executed.
		// Only available if CODER_AGENT_DEVCONTAINERS_ENABLE=true.
		ContainerEnvironmentVariable = "CODER_CONTAINER"

Copy link

Member

mtojekFeb 27, 2025

Choose a reason for hiding this comment

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

Should we be more specific about the container here? Is it only Devcontainer?

Copy link

MemberAuthor

johnstcnFeb 27, 2025

Choose a reason for hiding this comment

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

I'm not forcing it to be :) It can be any container but the UI andcoder show should only show devcontainers.

Copy link

Member

mtojekFeb 27, 2025

Choose a reason for hiding this comment

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

ok, so in theory a smart Coder user can run any container this way?

Copy link

MemberAuthor

johnstcnFeb 27, 2025

Choose a reason for hiding this comment

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

Yep, in theory. This is still 'early access' so I'm choosing to not restrict things like this until it makes sense to do so.

agent/agentssh/agentssh.go OutdatedShow resolvedHide resolved

cli/ssh.go OutdatedShow resolvedHide resolved

mafredri approved these changes

Feb 27, 2025

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

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

Wish we could keep the container specific stuff out ofagentssh package, but I don't see a way around it. Glad the invasion is relatively constrained. Nice work!

agent/agentssh/agentssh.goShow resolvedHide resolved

agent/agentssh/agentssh.go OutdatedShow resolvedHide resolved

agent/agentssh/agentssh.go

		@@ -495,30 +526,35 @@ func (s *Server) fileTransferBlocked(session ssh.Session) bool {
		return false
		}

		func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, env []string, magicType MagicSessionType) (retErr error) {
		func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, env []string, magicType MagicSessionType, container, containerUser string) (retErr error) {

Copy link

Member

mafredriFeb 27, 2025

Choose a reason for hiding this comment

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

It could be done, but no need to do that yet IMO. If this grows more then probably.

I'd actually want us to refactor the whole session handling and manage our own*session insession.go or some such. But that's a story for another time.

agent/agentssh/agentssh.go OutdatedShow resolvedHide resolved

cli/ssh.go OutdatedShow resolvedHide resolved

agent/agentssh/agentssh.go

		// ContainerUserEnvironmentVariable is used to specify the container user for
		// an SSH connection.
		// Only available if CODER_AGENT_DEVCONTAINERS_ENABLE=true.
		ContainerUserEnvironmentVariable = "CODER_CONTAINER_USER"

Copy link

Member

mafredriFeb 27, 2025

Choose a reason for hiding this comment

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

No need to changeCODER_CONTAINER orCODER_CONTAINER_USER values, but should we prefix the varsityMagicSession like the other?

Copy link

MemberAuthor

johnstcnFeb 27, 2025

Choose a reason for hiding this comment

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

I personally dislike theMAGIC_ prefix as it doesn't really impart any information to the reader (except perhaps that it's mysterious and whimsical).

johnstcn added2 commits

February 27, 2025 18:50

address PR comments

7437e49

fixup! address PR comments

a1d0d00

johnstcn merged commitec44f06 intomain

Feb 28, 2025

30 checks passed

johnstcn deleted the cj/cli-ssh-container branch

February 28, 2025 09:38

github-actionsbot locked and limited conversation to collaborators

Feb 28, 2025

Labels

None yet

3 participants

Movatterモバイル変換

feat(cli): allow SSH command to connect to running container#16726

feat(cli): allow SSH command to connect to running container#16726

Uh oh!

Conversation

johnstcn commentedFeb 26, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

johnstcn commentedFeb 26, 2025•
edited
Loading