feat(agent): wire up agentssh server to allow exec into container (#1…

…6638) Builds on top of #16623 and wires up the ReconnectingPTY server. This does nothing to wire up the web terminal yet but the added test demonstrates the functionality working. Other changes: * Refactors and moves the `SystemEnvInfo` interface to the `agent/usershell` package to address follow-up from #16623 (comment) * Marks `usershellinfo.Get` as deprecated. Consumers should use the `EnvInfoer` interface instead. --------- Co-authored-by: Mathias Fredriksson <mafredri@gmail.com> Co-authored-by: Danny Kopping <danny@coder.com>
coder · matifali · Feb 26, 2025 · Feb 26, 2025 · Feb 26, 2025 · Feb 26, 2025
commit 172e52317cd053dcdffc2b7d445a1d390ebbe53b
diff --git a/agent/agent.go b/agent/agent.go
@@ -88,6 +88,8 @@ type Options struct {
 	BlockFileTransfer            bool
 	Execer                       agentexec.Execer
    	ContainerLister              agentcontainers.Lister
+
+	ExperimentalContainersEnabled bool
 }
 
 type Client interface {
@@ -188,6 +190,8 @@ func New(options Options) Agent {
 		metrics:            newAgentMetrics(prometheusRegistry),
 		execer:             options.Execer,
 		lister:             options.ContainerLister,
+
+		experimentalDevcontainersEnabled: options.ExperimentalContainersEnabled,
 	}
 	// Initially, we have a closed channel, reflecting the fact that we are not initially connected.
 	// Each time we connect we replace the channel (while holding the closeMutex) with a new one
@@ -258,6 +262,8 @@ type agent struct {
 	metrics *agentMetrics
 	execer  agentexec.Execer
 	lister  agentcontainers.Lister
+
+	experimentalDevcontainersEnabled bool
 }
 
 func (a *agent) TailnetConn() *tailnet.Conn {
@@ -297,6 +303,9 @@ func (a *agent) init() {
 		a.sshServer,
 		a.metrics.connectionsTotal, a.metrics.reconnectingPTYErrors,
 		a.reconnectingPTYTimeout,
+		func(s *reconnectingpty.Server) {
+			s.ExperimentalContainersEnabled = a.experimentalDevcontainersEnabled
+		},
 	)
 	go a.runLoop()
 }

diff --git a/agent/agent_test.go b/agent/agent_test.go
@@ -25,24 +25,28 @@ import (
 	"testing"
 	"time"
 
+	"go.uber.org/goleak"
+	"tailscale.com/net/speedtest"
+	"tailscale.com/tailcfg"
+
 	"github.com/bramvdbogaerde/go-scp"
 	"github.com/google/uuid"
+	"github.com/ory/dockertest/v3"
+	"github.com/ory/dockertest/v3/docker"
 	"github.com/pion/udp"
 	"github.com/pkg/sftp"
 	"github.com/prometheus/client_golang/prometheus"
 	promgo "github.com/prometheus/client_model/go"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"go.uber.org/goleak"
 	"golang.org/x/crypto/ssh"
 	"golang.org/x/exp/slices"
 	"golang.org/x/xerrors"
-	"tailscale.com/net/speedtest"
-	"tailscale.com/tailcfg"
 
 	"cdr.dev/slog"
 	"cdr.dev/slog/sloggers/slogtest"
+
 	"github.com/coder/coder/v2/agent"
 	"github.com/coder/coder/v2/agent/agentssh"
 	"github.com/coder/coder/v2/agent/agenttest"
@@ -1761,6 +1765,74 @@ func TestAgent_ReconnectingPTY(t *testing.T) {
 	}
 }
 
+// This tests end-to-end functionality of connecting to a running container
+// and executing a command. It creates a real Docker container and runs a
+// command. As such, it does not run by default in CI.
+// You can run it manually as follows:
+//
+// CODER_TEST_USE_DOCKER=1 go test -count=1 ./agent -run TestAgent_ReconnectingPTYContainer
+func TestAgent_ReconnectingPTYContainer(t *testing.T) {
+	t.Parallel()
+	if os.Getenv("CODER_TEST_USE_DOCKER") != "1" {
+		t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test")
+	}
+
+	ctx := testutil.Context(t, testutil.WaitLong)
+
+	pool, err := dockertest.NewPool("")
+	require.NoError(t, err, "Could not connect to docker")
+	ct, err := pool.RunWithOptions(&dockertest.RunOptions{
+		Repository: "busybox",
+		Tag:        "latest",
+		Cmd:        []string{"sleep", "infnity"},
+	}, func(config *docker.HostConfig) {
+		config.AutoRemove = true
+		config.RestartPolicy = docker.RestartPolicy{Name: "no"}
+	})
+	require.NoError(t, err, "Could not start container")
+	t.Cleanup(func() {
+		err := pool.Purge(ct)
+		require.NoError(t, err, "Could not stop container")
+	})
+	// Wait for container to start
+	require.Eventually(t, func() bool {
+		ct, ok := pool.ContainerByName(ct.Container.Name)
+		return ok && ct.Container.State.Running
+	}, testutil.WaitShort, testutil.IntervalSlow, "Container did not start in time")
+
+	// nolint: dogsled
+	conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
+		o.ExperimentalContainersEnabled = true
+	})
+	ac, err := conn.ReconnectingPTY(ctx, uuid.New(), 80, 80, "/bin/sh", func(arp *workspacesdk.AgentReconnectingPTYInit) {
+		arp.Container = ct.Container.ID
+	})
+	require.NoError(t, err, "failed to create ReconnectingPTY")
+	defer ac.Close()
+	tr := testutil.NewTerminalReader(t, ac)
+
+	require.NoError(t, tr.ReadUntil(ctx, func(line string) bool {
+		return strings.Contains(line, "#") || strings.Contains(line, "$")
+	}), "find prompt")
+
+	require.NoError(t, json.NewEncoder(ac).Encode(workspacesdk.ReconnectingPTYRequest{
+		Data: "hostname\r",
+	}), "write hostname")
+	require.NoError(t, tr.ReadUntil(ctx, func(line string) bool {
+		return strings.Contains(line, "hostname")
+	}), "find hostname command")
+
+	require.NoError(t, tr.ReadUntil(ctx, func(line string) bool {
+		return strings.Contains(line, ct.Container.Config.Hostname)
+	}), "find hostname output")
+	require.NoError(t, json.NewEncoder(ac).Encode(workspacesdk.ReconnectingPTYRequest{
+		Data: "exit\r",
+	}), "write exit command")
+
+	// Wait for the connection to close.
+	require.ErrorIs(t, tr.ReadUntil(ctx, nil), io.EOF)
+}
+
 func TestAgent_Dial(t *testing.T) {
 	t.Parallel()
 

diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go
@@ -6,7 +6,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"os"
 	"os/user"
 	"slices"
 	"sort"
@@ -15,6 +14,7 @@ import (
 	"time"
 
 	"github.com/coder/coder/v2/agent/agentexec"
+	"github.com/coder/coder/v2/agent/usershell"
 	"github.com/coder/coder/v2/codersdk"
 
 	"golang.org/x/exp/maps"
@@ -37,6 +37,7 @@ func NewDocker(execer agentexec.Execer) Lister {
 // DockerEnvInfoer is an implementation of agentssh.EnvInfoer that returns
 // information about a container.
 type DockerEnvInfoer struct {
+	usershell.SystemEnvInfo
 	container string
 	user      *user.User
 	userShell string
@@ -122,26 +123,13 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU
 	return &dei, nil
 }
 
-func (dei *DockerEnvInfoer) CurrentUser() (*user.User, error) {
+func (dei *DockerEnvInfoer) User() (*user.User, error) {
 	// Clone the user so that the caller can't modify it
 	u := *dei.user
 	return &u, nil
 }
 
-func (*DockerEnvInfoer) Environ() []string {
-	// Return a clone of the environment so that the caller can't modify it
-	return os.Environ()
-}
-
-func (*DockerEnvInfoer) UserHomeDir() (string, error) {
-	// We default the working directory of the command to the user's home
-	// directory. Since this came from inside the container, we cannot guarantee
-	// that this exists on the host. Return the "real" home directory of the user
-	// instead.
-	return os.UserHomeDir()
-}
-
-func (dei *DockerEnvInfoer) UserShell(string) (string, error) {
+func (dei *DockerEnvInfoer) Shell(string) (string, error) {
 	return dei.userShell, nil
 }
 

diff --git a/agent/agentcontainers/containers_internal_test.go b/agent/agentcontainers/containers_internal_test.go
@@ -502,15 +502,15 @@ func TestDockerEnvInfoer(t *testing.T) {
 			dei, err := EnvInfo(ctx, agentexec.DefaultExecer, ct.Container.ID, tt.containerUser)
 			require.NoError(t, err, "Expected no error from DockerEnvInfo()")
 
-			u, err := dei.CurrentUser()
+			u, err := dei.User()
 			require.NoError(t, err, "Expected no error from CurrentUser()")
 			require.Equal(t, tt.expectedUsername, u.Username, "Expected username to match")
 
-			hd, err := dei.UserHomeDir()
+			hd, err := dei.HomeDir()
 			require.NoError(t, err, "Expected no error from UserHomeDir()")
 			require.NotEmpty(t, hd, "Expected user homedir to be non-empty")
 
-			sh, err := dei.UserShell(tt.containerUser)
+			sh, err := dei.Shell(tt.containerUser)
 			require.NoError(t, err, "Expected no error from UserShell()")
 			require.Equal(t, tt.expectedUserShell, sh, "Expected user shell to match")
 

diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go
@@ -698,63 +698,24 @@ func (s *Server) sftpHandler(logger slog.Logger, session ssh.Session) {
 	_ = session.Exit(1)
 }
 
-// EnvInfoer encapsulates external information required by CreateCommand.
-type EnvInfoer interface {
-	// CurrentUser returns the current user.
-	CurrentUser() (*user.User, error)
-	// Environ returns the environment variables of the current process.
-	Environ() []string
-	// UserHomeDir returns the home directory of the current user.
-	UserHomeDir() (string, error)
-	// UserShell returns the shell of the given user.
-	UserShell(username string) (string, error)
-}
-
-type systemEnvInfoer struct{}
-
-var defaultEnvInfoer EnvInfoer = &systemEnvInfoer{}
-
-// DefaultEnvInfoer returns a default implementation of
-// EnvInfoer. This reads information using the default Go
-// implementations.
-func DefaultEnvInfoer() EnvInfoer {
-	return defaultEnvInfoer
-}
-
-func (systemEnvInfoer) CurrentUser() (*user.User, error) {
-	return user.Current()
-}
-
-func (systemEnvInfoer) Environ() []string {
-	return os.Environ()
-}
-
-func (systemEnvInfoer) UserHomeDir() (string, error) {
-	return userHomeDir()
-}
-
-func (systemEnvInfoer) UserShell(username string) (string, error) {
-	return usershell.Get(username)
-}
-
 // CreateCommand processes raw command input with OpenSSH-like behavior.
 // If the script provided is empty, it will default to the users shell.
 // This injects environment variables specified by the user at launch too.
 // The final argument is an interface that allows the caller to provide
 // alternative implementations for the dependencies of CreateCommand.
 // This is useful when creating a command to be run in a separate environment
 // (for example, a Docker container). Pass in nil to use the default.
-func (s *Server) CreateCommand(ctx context.Context, script string, env []string, deps EnvInfoer) (*pty.Cmd, error) {
-	if deps == nil {
-		deps = DefaultEnvInfoer()
+func (s *Server) CreateCommand(ctx context.Context, script string, env []string, ei usershell.EnvInfoer) (*pty.Cmd, error) {
+	if ei == nil {
+		ei = &usershell.SystemEnvInfo{}
 	}
-	currentUser, err := deps.CurrentUser()
+	currentUser, err := ei.User()
 	if err != nil {
 		return nil, xerrors.Errorf("get current user: %w", err)
 	}
 	username := currentUser.Username
 
-	shell, err := deps.UserShell(username)
+	shell, err := ei.Shell(username)
 	if err != nil {
 		return nil, xerrors.Errorf("get user shell: %w", err)
 	}
@@ -802,21 +763,32 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string,
 		}
 	}
 
-	cmd := s.Execer.PTYCommandContext(ctx, name, args...)
+	// Modify command prior to execution. This will usually be a no-op, but not
+	// always. For example, to run a command in a Docker container, we need to
+	// modify the command to be `docker exec -it <container> <command>`.
+	modifiedName, modifiedArgs := ei.ModifyCommand(name, args...)
+	// Log if the command was modified.
+	if modifiedName != name && slices.Compare(modifiedArgs, args) != 0 {
+		s.logger.Debug(ctx, "modified command",
+			slog.F("before", append([]string{name}, args...)),
+			slog.F("after", append([]string{modifiedName}, modifiedArgs...)),
+		)
+	}
+	cmd := s.Execer.PTYCommandContext(ctx, modifiedName, modifiedArgs...)
 	cmd.Dir = s.config.WorkingDirectory()
 
 	// If the metadata directory doesn't exist, we run the command
 	// in the users home directory.
 	_, err = os.Stat(cmd.Dir)
 	if cmd.Dir == "" || err != nil {
 		// Default to user home if a directory is not set.
-		homedir, err := deps.UserHomeDir()
+		homedir, err := ei.HomeDir()
 		if err != nil {
 			return nil, xerrors.Errorf("get home dir: %w", err)
 		}
 		cmd.Dir = homedir
 	}
-	cmd.Env = append(deps.Environ(), env...)
+	cmd.Env = append(ei.Environ(), env...)
 	cmd.Env = append(cmd.Env, fmt.Sprintf("USER=%s", username))
 
 	// Set SSH connection environment variables (these are also set by OpenSSH

diff --git a/agent/agentssh/agentssh_test.go b/agent/agentssh/agentssh_test.go
@@ -124,22 +124,26 @@ type fakeEnvInfoer struct {
 	UserShellFn   func(string) (string, error)
 }
 
-func (f *fakeEnvInfoer) CurrentUser() (u *user.User, err error) {
+func (f *fakeEnvInfoer) User() (u *user.User, err error) {
 	return f.CurrentUserFn()
 }
 
 func (f *fakeEnvInfoer) Environ() []string {
 	return f.EnvironFn()
 }
 
-func (f *fakeEnvInfoer) UserHomeDir() (string, error) {
+func (f *fakeEnvInfoer) HomeDir() (string, error) {
 	return f.UserHomeDirFn()
 }
 
-func (f *fakeEnvInfoer) UserShell(u string) (string, error) {
+func (f *fakeEnvInfoer) Shell(u string) (string, error) {
 	return f.UserShellFn(u)
 }
 
+func (*fakeEnvInfoer) ModifyCommand(cmd string, args ...string) (string, []string) {
+	return cmd, args
+}
+
 func TestNewServer_CloseActiveConnections(t *testing.T) {
 	t.Parallel()