feat(cli): allow SSH command to connect to running container #16726
Conversation
3692436 to
e599cef
Compare
9780c67 to
eec7378
Compare
eec7378 to
3dc994a
Compare
| @@ -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) { | |||
There was a problem hiding this comment.
review: think it might be time to consider an opts struct?
There was a problem hiding this comment.
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 in session.go or some such. But that's a story for another time.
agent/agentssh/agentssh.go
Outdated
| 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=") | ||
| }) |
There was a problem hiding this comment.
review: inspired by the magic session stuff, this seems to be the only real way to smuggle info betweeen the client and the server.
| // 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" |
There was a problem hiding this comment.
Should we be more specific about the container here? Is it only Devcontainer?
There was a problem hiding this comment.
I'm not forcing it to be :) It can be any container but the UI and coder show should only show devcontainers.
There was a problem hiding this comment.
ok, so in theory a smart Coder user can run any container this way?
There was a problem hiding this comment.
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.
| @@ -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) { | |||
There was a problem hiding this comment.
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 in session.go or some such. But that's a story for another time.
| // ContainerUserEnvironmentVariable is used to specify the container user for | ||
| // an SSH connection. | ||
| // Only available if CODER_AGENT_DEVCONTAINERS_ENABLE=true. | ||
| ContainerUserEnvironmentVariable = "CODER_CONTAINER_USER" |
There was a problem hiding this comment.
No need to change CODER_CONTAINER or CODER_CONTAINER_USER values, but should we prefix the varsity MagicSession like the other?
There was a problem hiding this comment.
I personally dislike the MAGIC_ prefix as it doesn't really impart any information to the reader (except perhaps that it's mysterious and whimsical).
Fixes #16709 and #16420
Notes: