- Notifications
You must be signed in to change notification settings - Fork920
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
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
3dc994a56538a97437e49a1d0d00File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -29,6 +29,7 @@ import ( | ||
| "cdr.dev/slog" | ||
| "github.com/coder/coder/v2/agent/agentcontainers" | ||
| "github.com/coder/coder/v2/agent/agentexec" | ||
| "github.com/coder/coder/v2/agent/agentrsa" | ||
| "github.com/coder/coder/v2/agent/usershell" | ||
| @@ -60,6 +61,14 @@ const ( | ||
| // MagicSessionTypeEnvironmentVariable is used to track the purpose behind an SSH connection. | ||
| // This is stripped from any commands being executed, and is counted towards connection stats. | ||
| MagicSessionTypeEnvironmentVariable = "CODER_SSH_SESSION_TYPE" | ||
| // 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. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 and There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. No need to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I personally dislike the | ||
| ) | ||
| // MagicSessionType enums. | ||
| @@ -104,6 +113,9 @@ type Config struct { | ||
| BlockFileTransfer bool | ||
| // ReportConnection. | ||
| ReportConnection reportConnectionFunc | ||
| // Experimental: allow connecting to running containers if | ||
| // CODER_AGENT_DEVCONTAINERS_ENABLE=true. | ||
| ExperimentalDevContainersEnabled bool | ||
| } | ||
| type Server struct { | ||
| @@ -324,6 +336,22 @@ func (s *sessionCloseTracker) Close() error { | ||
| return s.Session.Close() | ||
| } | ||
| func extractContainerInfo(env []string) (container, containerUser string, filteredEnv []string) { | ||
| for _, kv := range env { | ||
| if strings.HasPrefix(kv, ContainerEnvironmentVariable+"=") { | ||
| container = strings.TrimPrefix(kv, ContainerEnvironmentVariable+"=") | ||
| } | ||
| if strings.HasPrefix(kv, ContainerUserEnvironmentVariable+"=") { | ||
| containerUser = strings.TrimPrefix(kv, ContainerUserEnvironmentVariable+"=") | ||
| } | ||
| } | ||
| return container, containerUser, slices.DeleteFunc(env, func(kv string) bool { | ||
| return strings.HasPrefix(kv, ContainerEnvironmentVariable+"=") || strings.HasPrefix(kv, ContainerUserEnvironmentVariable+"=") | ||
| }) | ||
| } | ||
| func (s *Server) sessionHandler(session ssh.Session) { | ||
| ctx := session.Context() | ||
| id := uuid.New() | ||
| @@ -353,6 +381,7 @@ func (s *Server) sessionHandler(session ssh.Session) { | ||
| defer s.trackSession(session, false) | ||
| reportSession := true | ||
| switch magicType { | ||
| case MagicSessionTypeVSCode: | ||
| s.connCountVSCode.Add(1) | ||
| @@ -395,9 +424,22 @@ func (s *Server) sessionHandler(session ssh.Session) { | ||
| return | ||
| } | ||
| container, containerUser, env := extractContainerInfo(env) | ||
| if container != "" { | ||
| s.logger.Debug(ctx, "container info", | ||
| slog.F("container", container), | ||
| slog.F("container_user", containerUser), | ||
| ) | ||
| } | ||
| switch ss := session.Subsystem(); ss { | ||
| case "": | ||
| case "sftp": | ||
| if s.config.ExperimentalDevContainersEnabled && container != "" { | ||
| closeCause("sftp not yet supported with containers") | ||
| _ = session.Exit(1) | ||
| return | ||
johnstcn marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| } | ||
| err := s.sftpHandler(logger, session) | ||
| if err != nil { | ||
| closeCause(err.Error()) | ||
| @@ -422,7 +464,7 @@ func (s *Server) sessionHandler(session ssh.Session) { | ||
| env = append(env, fmt.Sprintf("DISPLAY=localhost:%d.%d", display, x11.ScreenNumber)) | ||
| } | ||
| err := s.sessionStart(logger, session, env, magicType, container, containerUser) | ||
| var exitError *exec.ExitError | ||
| if xerrors.As(err, &exitError) { | ||
| code := exitError.ExitCode() | ||
| @@ -495,30 +537,34 @@ func (s *Server) fileTransferBlocked(session ssh.Session) bool { | ||
| return false | ||
| } | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. review: think it might be time to consider an opts struct? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| ctx := session.Context() | ||
| magicTypeLabel := magicTypeMetricLabel(magicType) | ||
| sshPty, windowSize, isPty := session.Pty() | ||
| ptyLabel := "no" | ||
| if isPty { | ||
| ptyLabel = "yes" | ||
| } | ||
| var ei usershell.EnvInfoer | ||
| var err error | ||
| if s.config.ExperimentalDevContainersEnabled && container != "" { | ||
| ei, err = agentcontainers.EnvInfo(ctx, s.Execer, container, containerUser) | ||
| if err != nil { | ||
| s.metrics.sessionErrors.WithLabelValues(magicTypeLabel, ptyLabel, "container_env_info").Add(1) | ||
| return err | ||
| } | ||
| } | ||
| cmd, err := s.CreateCommand(ctx, session.RawCommand(), env, ei) | ||
| if err != nil { | ||
| s.metrics.sessionErrors.WithLabelValues(magicTypeLabel, ptyLabel, "create_command").Add(1) | ||
| return err | ||
| } | ||
| if ssh.AgentRequested(session) { | ||
| l, err := ssh.NewAgentListener() | ||
| if err != nil { | ||
| s.metrics.sessionErrors.WithLabelValues(magicTypeLabel, ptyLabel, "listener").Add(1) | ||
| return xerrors.Errorf("new agent listener: %w", err) | ||
| } | ||
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.