From 8c357b4400e665174b702494dc8a8f7ce0119b32 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 19 Feb 2025 16:08:28 +0000 Subject: [PATCH 1/9] feat(agent/agentcontainers): add ContainerEnvInfoer --- agent/agentcontainers/containers.go | 2 + agent/agentcontainers/containers_dockercli.go | 168 +++++++++++++ .../containers_internal_test.go | 231 +++++++++++++++++- 3 files changed, 389 insertions(+), 12 deletions(-) diff --git a/agent/agentcontainers/containers.go b/agent/agentcontainers/containers.go index 4f03f35ed5710..031d3c7208424 100644 --- a/agent/agentcontainers/containers.go +++ b/agent/agentcontainers/containers.go @@ -144,6 +144,8 @@ type Lister interface { // NoopLister is a Lister interface that never returns any containers. type NoopLister struct{} +var _ Lister = NoopLister{} + func (NoopLister) List(_ context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) { return codersdk.WorkspaceAgentListContainersResponse{}, nil } diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index 3842735116329..291a55dc3280f 100644 --- a/agent/agentcontainers/containers_dockercli.go +++ b/agent/agentcontainers/containers_dockercli.go @@ -6,6 +6,8 @@ import ( "context" "encoding/json" "fmt" + "os/user" + "slices" "sort" "strconv" "strings" @@ -31,6 +33,172 @@ func NewDocker(execer agentexec.Execer) Lister { } } +// ContainerEnvInfoer is an implementation of agentssh.EnvInfoer that returns +// information about a container. +type ContainerEnvInfoer struct { + container string + user *user.User + userShell string + env []string +} + +// EnvInfo returns information about the environment of a container. +func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerUser string) (*ContainerEnvInfoer, error) { + var dei ContainerEnvInfoer + dei.container = container + + var stdoutBuf, stderrBuf bytes.Buffer + if containerUser == "" { + // Get the "default" user of the container if no user is specified. + // TODO: handle different container runtimes. + cmd, args := WrapDockerExec(container, "")("whoami") + execCmd := execer.CommandContext(ctx, cmd, args...) + execCmd.Stdout = &stdoutBuf + execCmd.Stderr = &stderrBuf + if err := execCmd.Run(); err != nil { + return nil, xerrors.Errorf("get container user: run whoami: %w: stderr: %q", err, strings.TrimSpace(stderrBuf.String())) + } + out := strings.TrimSpace(stdoutBuf.String()) + if len(out) == 0 { + return nil, xerrors.Errorf("get container user: run whoami: empty output: stderr: %q", strings.TrimSpace(stderrBuf.String())) + } + containerUser = out + stdoutBuf.Reset() + stderrBuf.Reset() + } + // Now that we know the username, get the required info from the container. + // We can't assume the presence of `getent` so we'll just have to sniff /etc/passwd. + cmd, args := WrapDockerExec(container, containerUser)("cat", "/etc/passwd") + execCmd := execer.CommandContext(ctx, cmd, args...) + execCmd.Stdout = &stdoutBuf + execCmd.Stderr = &stderrBuf + if err := execCmd.Run(); err != nil { + return nil, xerrors.Errorf("get container user: read /etc/passwd: %w stderr: %q", err, strings.TrimSpace(stderrBuf.String())) + } + + scanner := bufio.NewScanner(&stdoutBuf) + var foundLine string + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if len(line) == 0 { + continue + } + if !strings.HasPrefix(line, containerUser+":") { + continue + } + foundLine = line + } + if err := scanner.Err(); err != nil { + return nil, xerrors.Errorf("get container user: scan /etc/passwd: %w", err) + } + if foundLine == "" { + return nil, xerrors.Errorf("get container user: no matching entry for %q found in /etc/passwd", containerUser) + } + + // Parse the output of /etc/passwd. It looks like this: + // postgres:x:999:999::/var/lib/postgresql:/bin/bash + passwdFields := strings.Split(foundLine, ":") + if len(passwdFields) < 7 { + return nil, xerrors.Errorf("get container user: invalid line in /etc/passwd: %q", foundLine) + } + + // The fourth entry in /etc/passwd contains GECOS information, which is a + // comma-separated list of fields. The first field is the user's full name. + gecos := strings.Split(passwdFields[4], ",") + fullName := "" + if len(gecos) > 1 { + fullName = gecos[0] + } + + dei.user = &user.User{ + Gid: passwdFields[3], + HomeDir: passwdFields[5], + Name: fullName, + Uid: passwdFields[2], + Username: containerUser, + } + dei.userShell = passwdFields[6] + + // Finally, get the environment of the container. + stdoutBuf.Reset() + stderrBuf.Reset() + cmd, args = WrapDockerExec(container, containerUser)("env") + execCmd = execer.CommandContext(ctx, cmd, args...) + execCmd.Stdout = &stdoutBuf + execCmd.Stderr = &stderrBuf + if err := execCmd.Run(); err != nil { + return nil, xerrors.Errorf("get container environment: run env: %w stderr: %q", err, strings.TrimSpace(stderrBuf.String())) + } + + scanner = bufio.NewScanner(&stdoutBuf) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + dei.env = append(dei.env, line) + } + if err := scanner.Err(); err != nil { + return nil, xerrors.Errorf("get container environment: scan env output: %w", err) + } + + return &dei, nil +} + +func (dei *ContainerEnvInfoer) CurrentUser() (*user.User, error) { + // Clone the user so that the caller can't modify it + u := &user.User{ + Gid: dei.user.Gid, + HomeDir: dei.user.HomeDir, + Name: dei.user.Name, + Uid: dei.user.Uid, + Username: dei.user.Username, + } + return u, nil +} + +func (dei *ContainerEnvInfoer) Environ() []string { + // Return a clone of the environment so that the caller can't modify it + return slices.Clone(dei.env) +} + +func (dei *ContainerEnvInfoer) UserHomeDir() (string, error) { + return dei.user.HomeDir, nil +} + +func (dei *ContainerEnvInfoer) UserShell(string) (string, error) { + return dei.userShell, nil +} + +func (dei *ContainerEnvInfoer) ModifyCommand(cmd string, args ...string) (string, []string) { + return WrapDockerExecPTY(dei.container, dei.user.Username)(cmd, args...) +} + +// WrapFn is a function that wraps a command and its arguments with another command and arguments. +type WrapFn func(cmd string, args ...string) (string, []string) + +// WrapDockerExec returns a WrapFn that wraps the given command and arguments +// with a docker exec command that runs as the given user in the given container. +func WrapDockerExec(containerName, userName string) WrapFn { + return func(cmd string, args ...string) (string, []string) { + dockerArgs := []string{"exec", "--interactive"} + if userName != "" { + dockerArgs = append(dockerArgs, "--user", userName) + } + dockerArgs = append(dockerArgs, containerName, cmd) + return "docker", append(dockerArgs, args...) + } +} + +// WrapDockerExecPTY is similar to WrapDockerExec but also allocates a PTY. +func WrapDockerExecPTY(containerName, userName string) WrapFn { + return func(cmd string, args ...string) (string, []string) { + dockerArgs := []string{"exec", "--interactive", "--tty"} + if userName != "" { + dockerArgs = append(dockerArgs, "--user", userName) + } + dockerArgs = append(dockerArgs, containerName, cmd) + return "docker", append(dockerArgs, args...) + } +} + func (dcl *DockerCLILister) List(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) { var stdoutBuf, stderrBuf bytes.Buffer // List all container IDs, one per line, with no truncation diff --git a/agent/agentcontainers/containers_internal_test.go b/agent/agentcontainers/containers_internal_test.go index e15deae54c2bd..01043ac3b690e 100644 --- a/agent/agentcontainers/containers_internal_test.go +++ b/agent/agentcontainers/containers_internal_test.go @@ -2,38 +2,37 @@ package agentcontainers import ( "fmt" - "os" + "runtime" "strconv" "strings" "testing" "time" + "go.uber.org/mock/gomock" + "github.com/google/uuid" "github.com/ory/dockertest/v3" "github.com/ory/dockertest/v3/docker" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" "github.com/coder/coder/v2/agent/agentcontainers/acmock" "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/pty" "github.com/coder/coder/v2/testutil" "github.com/coder/quartz" ) -// TestDockerCLIContainerLister tests the happy path of the -// dockerCLIContainerLister.List method. It starts a container with a known +// TestIntegrationDocker tests agentcontainers functionality using a real +// Docker container. It starts a container with a known // label, lists the containers, and verifies that the expected container is -// returned. The container is deleted after the test is complete. -// As this test creates containers, it is skipped by default. -// It can be run manually as follows: -// -// CODER_TEST_USE_DOCKER=1 go test ./agent/agentcontainers -run TestDockerCLIContainerLister -func TestDockerCLIContainerLister(t *testing.T) { +// returned. It also executes a sample command inside the container. +// The container is deleted after the test is complete. +func TestIntegrationDocker(t *testing.T) { t.Parallel() - if ctud, ok := os.LookupEnv("CODER_TEST_USE_DOCKER"); !ok || ctud != "1" { - t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") + if runtime.GOOS != "linux" { + t.Skip("This test creates containers, which can be flaky in non-Linux CI environments.") } pool, err := dockertest.NewPool("") @@ -68,8 +67,15 @@ func TestDockerCLIContainerLister(t *testing.T) { assert.NoError(t, pool.Purge(ct), "Could not purge resource %q", ct.Container.Name) t.Logf("Purged container %q", ct.Container.Name) }) + // 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") dcl := NewDocker(agentexec.DefaultExecer) + wex := WrapDockerExec(ct.Container.Name, "") + wexpty := WrapDockerExecPTY(ct.Container.Name, "") ctx := testutil.Context(t, testutil.WaitShort) actual, err := dcl.List(ctx) require.NoError(t, err, "Could not list containers") @@ -93,12 +99,106 @@ func TestDockerCLIContainerLister(t *testing.T) { if assert.Len(t, foundContainer.Volumes, 1) { assert.Equal(t, testTempDir, foundContainer.Volumes[testTempDir]) } + // Test command execution + wrappedCmd, wrappedArgs := wex("cat", "/etc/hostname") + cmd := agentexec.DefaultExecer.CommandContext(ctx, wrappedCmd, wrappedArgs...) + out, err := cmd.CombinedOutput() + if !assert.NoError(t, err) { + t.Logf("Container %q exited with error: %v", ct.Container.ID, err) + t.Logf("Output:\n%s", string(out)) + t.FailNow() + } + require.Equal(t, ct.Container.Config.Hostname, strings.TrimSpace(string(out))) + + // Test command execution with PTY + ptyWrappedCmd, ptyWrappedArgs := wexpty("/bin/sh", "--norc") + ptyCmd, ptyPs, err := pty.Start(agentexec.DefaultExecer.PTYCommandContext(ctx, ptyWrappedCmd, ptyWrappedArgs...)) + require.NoError(t, err, "failed to start pty command") + t.Cleanup(func() { + _ = ptyPs.Kill() + _ = ptyCmd.Close() + }) + tr := testutil.NewTerminalReader(t, ptyCmd.OutputReader()) + matchPrompt := func(line string) bool { + return strings.HasPrefix(strings.TrimSpace(line), "/ #") + } + matchHostnameCmd := func(line string) bool { + return strings.Contains(strings.TrimSpace(line), "hostname") + } + matchHostnameOuput := func(line string) bool { + return strings.Contains(strings.TrimSpace(line), ct.Container.Config.Hostname) + } + require.NoError(t, tr.ReadUntil(ctx, matchPrompt), "failed to match prompt") + t.Logf("Matched prompt") + _, err = ptyCmd.InputWriter().Write([]byte("hostname\r\n")) + require.NoError(t, err, "failed to write to pty") + t.Logf("Wrote hostname command") + require.NoError(t, tr.ReadUntil(ctx, matchHostnameCmd), "failed to match hostname command") + t.Logf("Matched hostname command") + require.NoError(t, tr.ReadUntil(ctx, matchHostnameOuput), "failed to match hostname output") + t.Logf("Matched hostname output") break } } assert.True(t, found, "Expected to find container with label 'com.coder.test=%s'", testLabelValue) } +func TestWrapDockerExec(t *testing.T) { + t.Parallel() + tests := []struct { + name string + wrapFn WrapFn + cmdArgs []string + wantCmd []string + }{ + { + name: "cmd with no args", + wrapFn: WrapDockerExec("my-container", "my-user"), + cmdArgs: []string{"my-cmd"}, + wantCmd: []string{"docker", "exec", "--interactive", "--user", "my-user", "my-container", "my-cmd"}, + }, + { + name: "cmd with args", + wrapFn: WrapDockerExec("my-container", "my-user"), + cmdArgs: []string{"my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, + wantCmd: []string{"docker", "exec", "--interactive", "--user", "my-user", "my-container", "my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, + }, + { + name: "no user specified", + wrapFn: WrapDockerExec("my-container", ""), + cmdArgs: []string{"my-cmd"}, + wantCmd: []string{"docker", "exec", "--interactive", "my-container", "my-cmd"}, + }, + { + name: "tty cmd with no args", + wrapFn: WrapDockerExecPTY("my-container", "my-user"), + cmdArgs: []string{"my-cmd"}, + wantCmd: []string{"docker", "exec", "--interactive", "--tty", "--user", "my-user", "my-container", "my-cmd"}, + }, + { + name: "cmd with args", + wrapFn: WrapDockerExecPTY("my-container", "my-user"), + cmdArgs: []string{"my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, + wantCmd: []string{"docker", "exec", "--interactive", "--tty", "--user", "my-user", "my-container", "my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, + }, + { + name: "no user specified", + wrapFn: WrapDockerExecPTY("my-container", ""), + cmdArgs: []string{"my-cmd"}, + wantCmd: []string{"docker", "exec", "--interactive", "--tty", "my-container", "my-cmd"}, + }, + } + for _, tt := range tests { + tt := tt // appease the linter even though this isn't needed anymore + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + actualCmd, actualArgs := tt.wrapFn(tt.cmdArgs[0], tt.cmdArgs[1:]...) + assert.Equal(t, tt.wantCmd[0], actualCmd) + assert.Equal(t, tt.wantCmd[1:], actualArgs) + }) + } +} + // TestContainersHandler tests the containersHandler.getContainers method using // a mock implementation. It specifically tests caching behavior. func TestContainersHandler(t *testing.T) { @@ -319,6 +419,113 @@ func TestConvertDockerVolume(t *testing.T) { } } +func TestDockerEnvInfoer(t *testing.T) { + t.Parallel() + if runtime.GOOS != "linux" { + t.Skip("This test creates containers, which can be flaky in non-Linux CI environments.") + } + + pool, err := dockertest.NewPool("") + require.NoError(t, err, "Could not connect to docker") + // nolint:paralleltest // variable recapture no longer required + for idx, tt := range []struct { + image string + env []string + containerUser string + expectedUsername string + expectedUserHomedir string + expectedUserShell string + expectedEnviron []string + }{ + { + image: "busybox:latest", + env: []string{"FOO=bar"}, + expectedUsername: "root", + expectedUserHomedir: "/root", + expectedUserShell: "/bin/sh", + expectedEnviron: []string{"FOO=bar"}, + }, + { + image: "busybox:latest", + env: []string{"FOO=bar"}, + containerUser: "root", + expectedUsername: "root", + expectedUserHomedir: "/root", + expectedUserShell: "/bin/sh", + expectedEnviron: []string{"FOO=bar"}, + }, + { + image: "codercom/enterprise-minimal:ubuntu", + env: []string{"FOO=bar"}, + expectedUsername: "coder", + expectedUserHomedir: "/home/coder", + expectedUserShell: "/bin/bash", + expectedEnviron: []string{"FOO=bar"}, + }, + { + image: "codercom/enterprise-minimal:ubuntu", + env: []string{"FOO=bar"}, + containerUser: "coder", + expectedUsername: "coder", + expectedUserHomedir: "/home/coder", + expectedUserShell: "/bin/bash", + expectedEnviron: []string{"FOO=bar"}, + }, + { + image: "codercom/enterprise-minimal:ubuntu", + env: []string{"FOO=bar"}, + containerUser: "root", + expectedUsername: "root", + expectedUserHomedir: "/root", + expectedUserShell: "/bin/bash", + expectedEnviron: []string{"FOO=bar"}, + }, + } { + t.Run(fmt.Sprintf("#%d", idx), func(t *testing.T) { + t.Parallel() + + // Start a container with the given image + // and environment variables + image := strings.Split(tt.image, ":")[0] + tag := strings.Split(tt.image, ":")[1] + ct, err := pool.RunWithOptions(&dockertest.RunOptions{ + Repository: image, + Tag: tag, + Cmd: []string{"sleep", "infinity"}, + Env: tt.env, + }, func(config *docker.HostConfig) { + config.AutoRemove = true + config.RestartPolicy = docker.RestartPolicy{Name: "no"} + }) + require.NoError(t, err, "Could not start test docker container") + t.Logf("Created container %q", ct.Container.Name) + t.Cleanup(func() { + assert.NoError(t, pool.Purge(ct), "Could not purge resource %q", ct.Container.Name) + t.Logf("Purged container %q", ct.Container.Name) + }) + + ctx := testutil.Context(t, testutil.WaitShort) + dei, err := EnvInfo(ctx, agentexec.DefaultExecer, ct.Container.ID, tt.containerUser) + require.NoError(t, err, "Expected no error from DockerEnvInfo()") + + u, err := dei.CurrentUser() + require.NoError(t, err, "Expected no error from CurrentUser()") + require.Equal(t, tt.expectedUsername, u.Username, "Expected username to match") + + hd, err := dei.UserHomeDir() + require.NoError(t, err, "Expected no error from UserHomeDir()") + require.Equal(t, tt.expectedUserHomedir, hd, "Expected user homedir to match") + + sh, err := dei.UserShell(tt.containerUser) + require.NoError(t, err, "Expected no error from UserShell()") + require.Equal(t, tt.expectedUserShell, sh, "Expected user shell to match") + + environ := dei.Environ() + require.Subset(t, environ, tt.expectedEnviron, "Expected environment to match") + }) + } +} + func fakeContainer(t *testing.T, mut ...func(*codersdk.WorkspaceAgentDevcontainer)) codersdk.WorkspaceAgentDevcontainer { t.Helper() ct := codersdk.WorkspaceAgentDevcontainer{ From e76af04928e33e706d79563739ac2dd0cae78a4f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 19 Feb 2025 16:32:40 +0000 Subject: [PATCH 2/9] ctud --- .../containers_internal_test.go | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/agent/agentcontainers/containers_internal_test.go b/agent/agentcontainers/containers_internal_test.go index 01043ac3b690e..18872c7c549e9 100644 --- a/agent/agentcontainers/containers_internal_test.go +++ b/agent/agentcontainers/containers_internal_test.go @@ -2,7 +2,7 @@ package agentcontainers import ( "fmt" - "runtime" + "os" "strconv" "strings" "testing" @@ -29,10 +29,14 @@ import ( // label, lists the containers, and verifies that the expected container is // returned. It also executes a sample command inside the container. // The container is deleted after the test is complete. +// As this test creates containers, it is skipped by default. +// It can be run manually as follows: +// +// CODER_TEST_USE_DOCKER=1 go test ./agent/agentcontainers -run TestDockerCLIContainerLister func TestIntegrationDocker(t *testing.T) { t.Parallel() - if runtime.GOOS != "linux" { - t.Skip("This test creates containers, which can be flaky in non-Linux CI environments.") + if ctud, ok := os.LookupEnv("CODER_TEST_USE_DOCKER"); !ok || ctud != "1" { + t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") } pool, err := dockertest.NewPool("") @@ -419,10 +423,16 @@ func TestConvertDockerVolume(t *testing.T) { } } +// TestDockerEnvInfoer tests the ability of EnvInfo to extract information from +// running containers. Containers are deleted after the test is complete. +// As this test creates containers, it is skipped by default. +// It can be run manually as follows: +// +// CODER_TEST_USE_DOCKER=1 go test ./agent/agentcontainers -run TestDockerEnvInfoer func TestDockerEnvInfoer(t *testing.T) { t.Parallel() - if runtime.GOOS != "linux" { - t.Skip("This test creates containers, which can be flaky in non-Linux CI environments.") + if ctud, ok := os.LookupEnv("CODER_TEST_USE_DOCKER"); !ok || ctud != "1" { + t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") } pool, err := dockertest.NewPool("") From 7fd24be9ea2f43b5f518c8acd50e5754f026e405 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 19 Feb 2025 17:57:38 +0000 Subject: [PATCH 3/9] Apply suggestions from code review Co-authored-by: Mathias Fredriksson --- agent/agentcontainers/containers_dockercli.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index 291a55dc3280f..df68fd00efb7b 100644 --- a/agent/agentcontainers/containers_dockercli.go +++ b/agent/agentcontainers/containers_dockercli.go @@ -98,7 +98,7 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU // Parse the output of /etc/passwd. It looks like this: // postgres:x:999:999::/var/lib/postgresql:/bin/bash passwdFields := strings.Split(foundLine, ":") - if len(passwdFields) < 7 { + if len(passwdFields) != 7 { return nil, xerrors.Errorf("get container user: invalid line in /etc/passwd: %q", foundLine) } @@ -144,14 +144,8 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU func (dei *ContainerEnvInfoer) CurrentUser() (*user.User, error) { // Clone the user so that the caller can't modify it - u := &user.User{ - Gid: dei.user.Gid, - HomeDir: dei.user.HomeDir, - Name: dei.user.Name, - Uid: dei.user.Uid, - Username: dei.user.Username, - } - return u, nil + u := *dei.user + return &u, nil } func (dei *ContainerEnvInfoer) Environ() []string { From 5e5703559b084b942d2868d22d40684c72a06997 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 19 Feb 2025 18:34:16 +0000 Subject: [PATCH 4/9] address mafcode review comments --- agent/agentcontainers/containers_dockercli.go | 89 ++++++++++--------- .../containers_internal_test.go | 18 ++-- 2 files changed, 55 insertions(+), 52 deletions(-) diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index df68fd00efb7b..6e89325a71a8d 100644 --- a/agent/agentcontainers/containers_dockercli.go +++ b/agent/agentcontainers/containers_dockercli.go @@ -42,51 +42,56 @@ type ContainerEnvInfoer struct { env []string } +// Helper function to run a command and return its stdout and stderr. +// We want to differentiate stdout and stderr instead of using CombinedOutput. +// We also want to differentiate between a command running successfully with +// output to stderr and a non-zero exit code. +func run(ctx context.Context, execer agentexec.Execer, cmd string, args ...string) (stdout, stderr string, err error) { + var stdoutBuf, stderrBuf bytes.Buffer + execCmd := execer.CommandContext(ctx, cmd, args...) + execCmd.Stdout = &stdoutBuf + execCmd.Stderr = &stderrBuf + err = execCmd.Run() + stdout = strings.TrimSpace(stdoutBuf.String()) + stderr = strings.TrimSpace(stderrBuf.String()) + return stdout, stderr, err +} + // EnvInfo returns information about the environment of a container. func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerUser string) (*ContainerEnvInfoer, error) { - var dei ContainerEnvInfoer - dei.container = container + var cei ContainerEnvInfoer + cei.container = container - var stdoutBuf, stderrBuf bytes.Buffer if containerUser == "" { // Get the "default" user of the container if no user is specified. // TODO: handle different container runtimes. cmd, args := WrapDockerExec(container, "")("whoami") - execCmd := execer.CommandContext(ctx, cmd, args...) - execCmd.Stdout = &stdoutBuf - execCmd.Stderr = &stderrBuf - if err := execCmd.Run(); err != nil { - return nil, xerrors.Errorf("get container user: run whoami: %w: stderr: %q", err, strings.TrimSpace(stderrBuf.String())) + stdout, stderr, err := run(ctx, execer, cmd, args...) + if err != nil { + return nil, xerrors.Errorf("get container user: run whoami: %w: %s", err, stderr) } - out := strings.TrimSpace(stdoutBuf.String()) - if len(out) == 0 { - return nil, xerrors.Errorf("get container user: run whoami: empty output: stderr: %q", strings.TrimSpace(stderrBuf.String())) + if len(stdout) == 0 { + return nil, xerrors.Errorf("get container user: run whoami: empty output") } - containerUser = out - stdoutBuf.Reset() - stderrBuf.Reset() + containerUser = stdout } // Now that we know the username, get the required info from the container. // We can't assume the presence of `getent` so we'll just have to sniff /etc/passwd. cmd, args := WrapDockerExec(container, containerUser)("cat", "/etc/passwd") - execCmd := execer.CommandContext(ctx, cmd, args...) - execCmd.Stdout = &stdoutBuf - execCmd.Stderr = &stderrBuf - if err := execCmd.Run(); err != nil { - return nil, xerrors.Errorf("get container user: read /etc/passwd: %w stderr: %q", err, strings.TrimSpace(stderrBuf.String())) + stdout, stderr, err := run(ctx, execer, cmd, args...) + if err != nil { + return nil, xerrors.Errorf("get container user: read /etc/passwd: %w: %q", err, stderr) } - scanner := bufio.NewScanner(&stdoutBuf) + scanner := bufio.NewScanner(strings.NewReader(stdout)) var foundLine string for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) - if len(line) == 0 { - continue - } if !strings.HasPrefix(line, containerUser+":") { continue } foundLine = line + break } if err := scanner.Err(); err != nil { return nil, xerrors.Errorf("get container user: scan /etc/passwd: %w", err) @@ -110,36 +115,40 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU fullName = gecos[0] } - dei.user = &user.User{ + cei.user = &user.User{ Gid: passwdFields[3], HomeDir: passwdFields[5], Name: fullName, Uid: passwdFields[2], Username: containerUser, } - dei.userShell = passwdFields[6] + cei.userShell = passwdFields[6] // Finally, get the environment of the container. - stdoutBuf.Reset() - stderrBuf.Reset() - cmd, args = WrapDockerExec(container, containerUser)("env") - execCmd = execer.CommandContext(ctx, cmd, args...) - execCmd.Stdout = &stdoutBuf - execCmd.Stderr = &stderrBuf - if err := execCmd.Run(); err != nil { - return nil, xerrors.Errorf("get container environment: run env: %w stderr: %q", err, strings.TrimSpace(stderrBuf.String())) - } - - scanner = bufio.NewScanner(&stdoutBuf) - for scanner.Scan() { - line := strings.TrimSpace(scanner.Text()) - dei.env = append(dei.env, line) + // TODO(cian): for devcontainers we will need to also take into account both + // remoteEnv and userEnvProbe. + // ref: https://code.visualstudio.com/docs/devcontainers/attach-container + cmd, args = WrapDockerExec(container, containerUser)("env", "-0") + stdout, stderr, err = run(ctx, execer, cmd, args...) + if err != nil { + return nil, xerrors.Errorf("get container environment: run env -0: %w: %q", err, stderr) + } + + // Parse the output of `env -0` which is a null-terminated list of environment + // variables. This is important as environment variables can contain newlines. + envs := strings.Split(stdout, "\x00") + for _, env := range envs { + env = strings.TrimSpace(env) + if env == "" { + continue + } + cei.env = append(cei.env, env) } if err := scanner.Err(); err != nil { return nil, xerrors.Errorf("get container environment: scan env output: %w", err) } - return &dei, nil + return &cei, nil } func (dei *ContainerEnvInfoer) CurrentUser() (*user.User, error) { diff --git a/agent/agentcontainers/containers_internal_test.go b/agent/agentcontainers/containers_internal_test.go index 18872c7c549e9..7cd2753c4c5bb 100644 --- a/agent/agentcontainers/containers_internal_test.go +++ b/agent/agentcontainers/containers_internal_test.go @@ -445,50 +445,44 @@ func TestDockerEnvInfoer(t *testing.T) { expectedUsername string expectedUserHomedir string expectedUserShell string - expectedEnviron []string }{ { image: "busybox:latest", - env: []string{"FOO=bar"}, + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, expectedUsername: "root", expectedUserHomedir: "/root", expectedUserShell: "/bin/sh", - expectedEnviron: []string{"FOO=bar"}, }, { image: "busybox:latest", - env: []string{"FOO=bar"}, + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, containerUser: "root", expectedUsername: "root", expectedUserHomedir: "/root", expectedUserShell: "/bin/sh", - expectedEnviron: []string{"FOO=bar"}, }, { image: "codercom/enterprise-minimal:ubuntu", - env: []string{"FOO=bar"}, + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, expectedUsername: "coder", expectedUserHomedir: "/home/coder", expectedUserShell: "/bin/bash", - expectedEnviron: []string{"FOO=bar"}, }, { image: "codercom/enterprise-minimal:ubuntu", - env: []string{"FOO=bar"}, + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, containerUser: "coder", expectedUsername: "coder", expectedUserHomedir: "/home/coder", expectedUserShell: "/bin/bash", - expectedEnviron: []string{"FOO=bar"}, }, { image: "codercom/enterprise-minimal:ubuntu", - env: []string{"FOO=bar"}, + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, containerUser: "root", expectedUsername: "root", expectedUserHomedir: "/root", expectedUserShell: "/bin/bash", - expectedEnviron: []string{"FOO=bar"}, }, } { t.Run(fmt.Sprintf("#%d", idx), func(t *testing.T) { @@ -531,7 +525,7 @@ func TestDockerEnvInfoer(t *testing.T) { require.Equal(t, tt.expectedUserShell, sh, "Expected user shell to match") environ := dei.Environ() - require.Subset(t, environ, tt.expectedEnviron, "Expected environment to match") + require.Subset(t, environ, tt.env, "Expected environment to match") }) } } From d52d0c1e8bc471f27b33e4798d46ebf0075e7c8f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 19 Feb 2025 22:10:22 +0000 Subject: [PATCH 5/9] fix issue with docker homedir and env clobbering real env for cmd --- agent/agentcontainers/containers_dockercli.go | 136 ++++++++--------- .../containers_internal_test.go | 143 +++++++----------- 2 files changed, 118 insertions(+), 161 deletions(-) diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index 6e89325a71a8d..c31521629f97e 100644 --- a/agent/agentcontainers/containers_dockercli.go +++ b/agent/agentcontainers/containers_dockercli.go @@ -6,8 +6,8 @@ import ( "context" "encoding/json" "fmt" + "os" "os/user" - "slices" "sort" "strconv" "strings" @@ -42,21 +42,6 @@ type ContainerEnvInfoer struct { env []string } -// Helper function to run a command and return its stdout and stderr. -// We want to differentiate stdout and stderr instead of using CombinedOutput. -// We also want to differentiate between a command running successfully with -// output to stderr and a non-zero exit code. -func run(ctx context.Context, execer agentexec.Execer, cmd string, args ...string) (stdout, stderr string, err error) { - var stdoutBuf, stderrBuf bytes.Buffer - execCmd := execer.CommandContext(ctx, cmd, args...) - execCmd.Stdout = &stdoutBuf - execCmd.Stderr = &stderrBuf - err = execCmd.Run() - stdout = strings.TrimSpace(stdoutBuf.String()) - stderr = strings.TrimSpace(stderrBuf.String()) - return stdout, stderr, err -} - // EnvInfo returns information about the environment of a container. func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerUser string) (*ContainerEnvInfoer, error) { var cei ContainerEnvInfoer @@ -65,7 +50,7 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU if containerUser == "" { // Get the "default" user of the container if no user is specified. // TODO: handle different container runtimes. - cmd, args := WrapDockerExec(container, "")("whoami") + cmd, args := wrapDockerExec(container, "", "whoami") stdout, stderr, err := run(ctx, execer, cmd, args...) if err != nil { return nil, xerrors.Errorf("get container user: run whoami: %w: %s", err, stderr) @@ -77,7 +62,7 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU } // Now that we know the username, get the required info from the container. // We can't assume the presence of `getent` so we'll just have to sniff /etc/passwd. - cmd, args := WrapDockerExec(container, containerUser)("cat", "/etc/passwd") + cmd, args := wrapDockerExec(container, containerUser, "cat", "/etc/passwd") stdout, stderr, err := run(ctx, execer, cmd, args...) if err != nil { return nil, xerrors.Errorf("get container user: read /etc/passwd: %w: %q", err, stderr) @@ -125,81 +110,86 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU cei.userShell = passwdFields[6] // Finally, get the environment of the container. - // TODO(cian): for devcontainers we will need to also take into account both - // remoteEnv and userEnvProbe. + // TODO: For containers, we can't simply run `env` inside the container. + // We need to inspect the container labels for remoteEnv and append these to + // the resulting docker exec command. // ref: https://code.visualstudio.com/docs/devcontainers/attach-container - cmd, args = WrapDockerExec(container, containerUser)("env", "-0") - stdout, stderr, err = run(ctx, execer, cmd, args...) - if err != nil { - return nil, xerrors.Errorf("get container environment: run env -0: %w: %q", err, stderr) - } - - // Parse the output of `env -0` which is a null-terminated list of environment - // variables. This is important as environment variables can contain newlines. - envs := strings.Split(stdout, "\x00") - for _, env := range envs { - env = strings.TrimSpace(env) - if env == "" { - continue - } - cei.env = append(cei.env, env) - } - if err := scanner.Err(); err != nil { - return nil, xerrors.Errorf("get container environment: scan env output: %w", err) - } + // For now, we're simply returning the host environment. + cei.env = make([]string, 0) return &cei, nil } -func (dei *ContainerEnvInfoer) CurrentUser() (*user.User, error) { +func (cei *ContainerEnvInfoer) CurrentUser() (*user.User, error) { // Clone the user so that the caller can't modify it - u := *dei.user + u := *cei.user return &u, nil } -func (dei *ContainerEnvInfoer) Environ() []string { +func (*ContainerEnvInfoer) Environ() []string { // Return a clone of the environment so that the caller can't modify it - return slices.Clone(dei.env) + return os.Environ() } -func (dei *ContainerEnvInfoer) UserHomeDir() (string, error) { - return dei.user.HomeDir, nil +func (*ContainerEnvInfoer) 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 *ContainerEnvInfoer) UserShell(string) (string, error) { - return dei.userShell, nil +func (cei *ContainerEnvInfoer) UserShell(string) (string, error) { + return cei.userShell, nil } -func (dei *ContainerEnvInfoer) ModifyCommand(cmd string, args ...string) (string, []string) { - return WrapDockerExecPTY(dei.container, dei.user.Username)(cmd, args...) +func (cei *ContainerEnvInfoer) ModifyCommand(cmd string, args ...string) (string, []string) { + // Wrap the command with `docker exec` and run it as the container user. + // There is some additional munging here regarding the container user and environment. + // return WrapDockerExecPTY(dei.container, dei.user.Username)(cmd, args...) + dockerArgs := []string{ + "exec", + // The assumption is that this command will be a shell command, so allocate a PTY. + "--interactive", + "--tty", + // Run the command as the user in the container. + "--user", + cei.user.Username, + // Set the working directory to the user's home directory as a sane default. + "--workdir", + cei.user.HomeDir, + cei.container, + cmd, + } + return "docker", append(dockerArgs, args...) } -// WrapFn is a function that wraps a command and its arguments with another command and arguments. -type WrapFn func(cmd string, args ...string) (string, []string) - -// WrapDockerExec returns a WrapFn that wraps the given command and arguments -// with a docker exec command that runs as the given user in the given container. -func WrapDockerExec(containerName, userName string) WrapFn { - return func(cmd string, args ...string) (string, []string) { - dockerArgs := []string{"exec", "--interactive"} - if userName != "" { - dockerArgs = append(dockerArgs, "--user", userName) - } - dockerArgs = append(dockerArgs, containerName, cmd) - return "docker", append(dockerArgs, args...) - } +// wrapDockerExec is a helper function that wraps the given command and arguments +// with a docker exec command that runs as the given user in the given +// container. This is used to fetch information about a container prior to +// running the actual command. +func wrapDockerExec(containerName, userName, cmd string, args ...string) (string, []string) { + dockerArgs := []string{"exec", "--interactive"} + if userName != "" { + dockerArgs = append(dockerArgs, "--user", userName) + } + dockerArgs = append(dockerArgs, containerName, cmd) + return "docker", append(dockerArgs, args...) } -// WrapDockerExecPTY is similar to WrapDockerExec but also allocates a PTY. -func WrapDockerExecPTY(containerName, userName string) WrapFn { - return func(cmd string, args ...string) (string, []string) { - dockerArgs := []string{"exec", "--interactive", "--tty"} - if userName != "" { - dockerArgs = append(dockerArgs, "--user", userName) - } - dockerArgs = append(dockerArgs, containerName, cmd) - return "docker", append(dockerArgs, args...) - } +// Helper function to run a command and return its stdout and stderr. +// We want to differentiate stdout and stderr instead of using CombinedOutput. +// We also want to differentiate between a command running successfully with +// output to stderr and a non-zero exit code. +func run(ctx context.Context, execer agentexec.Execer, cmd string, args ...string) (stdout, stderr string, err error) { + var stdoutBuf, stderrBuf bytes.Buffer + execCmd := execer.CommandContext(ctx, cmd, args...) + execCmd.Stdout = &stdoutBuf + execCmd.Stderr = &stderrBuf + err = execCmd.Run() + stdout = strings.TrimSpace(stdoutBuf.String()) + stderr = strings.TrimSpace(stderrBuf.String()) + return stdout, stderr, err } func (dcl *DockerCLILister) List(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) { diff --git a/agent/agentcontainers/containers_internal_test.go b/agent/agentcontainers/containers_internal_test.go index 7cd2753c4c5bb..7dfcd145c62e0 100644 --- a/agent/agentcontainers/containers_internal_test.go +++ b/agent/agentcontainers/containers_internal_test.go @@ -78,8 +78,6 @@ func TestIntegrationDocker(t *testing.T) { }, testutil.WaitShort, testutil.IntervalSlow, "Container did not start in time") dcl := NewDocker(agentexec.DefaultExecer) - wex := WrapDockerExec(ct.Container.Name, "") - wexpty := WrapDockerExecPTY(ct.Container.Name, "") ctx := testutil.Context(t, testutil.WaitShort) actual, err := dcl.List(ctx) require.NoError(t, err, "Could not list containers") @@ -103,19 +101,11 @@ func TestIntegrationDocker(t *testing.T) { if assert.Len(t, foundContainer.Volumes, 1) { assert.Equal(t, testTempDir, foundContainer.Volumes[testTempDir]) } - // Test command execution - wrappedCmd, wrappedArgs := wex("cat", "/etc/hostname") - cmd := agentexec.DefaultExecer.CommandContext(ctx, wrappedCmd, wrappedArgs...) - out, err := cmd.CombinedOutput() - if !assert.NoError(t, err) { - t.Logf("Container %q exited with error: %v", ct.Container.ID, err) - t.Logf("Output:\n%s", string(out)) - t.FailNow() - } - require.Equal(t, ct.Container.Config.Hostname, strings.TrimSpace(string(out))) - - // Test command execution with PTY - ptyWrappedCmd, ptyWrappedArgs := wexpty("/bin/sh", "--norc") + // Test that EnvInfo is able to correctly modify a command to be + // executed inside the container. + dei, err := EnvInfo(ctx, agentexec.DefaultExecer, ct.Container.ID, "") + require.NoError(t, err, "Expected no error from DockerEnvInfo()") + ptyWrappedCmd, ptyWrappedArgs := dei.ModifyCommand("/bin/sh", "--norc") ptyCmd, ptyPs, err := pty.Start(agentexec.DefaultExecer.PTYCommandContext(ctx, ptyWrappedCmd, ptyWrappedArgs...)) require.NoError(t, err, "failed to start pty command") t.Cleanup(func() { @@ -124,7 +114,7 @@ func TestIntegrationDocker(t *testing.T) { }) tr := testutil.NewTerminalReader(t, ptyCmd.OutputReader()) matchPrompt := func(line string) bool { - return strings.HasPrefix(strings.TrimSpace(line), "/ #") + return strings.Contains(line, "#") } matchHostnameCmd := func(line string) bool { return strings.Contains(strings.TrimSpace(line), "hostname") @@ -150,53 +140,35 @@ func TestIntegrationDocker(t *testing.T) { func TestWrapDockerExec(t *testing.T) { t.Parallel() tests := []struct { - name string - wrapFn WrapFn - cmdArgs []string - wantCmd []string + name string + containerUser string + cmdArgs []string + wantCmd []string }{ { - name: "cmd with no args", - wrapFn: WrapDockerExec("my-container", "my-user"), - cmdArgs: []string{"my-cmd"}, - wantCmd: []string{"docker", "exec", "--interactive", "--user", "my-user", "my-container", "my-cmd"}, - }, - { - name: "cmd with args", - wrapFn: WrapDockerExec("my-container", "my-user"), - cmdArgs: []string{"my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, - wantCmd: []string{"docker", "exec", "--interactive", "--user", "my-user", "my-container", "my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, - }, - { - name: "no user specified", - wrapFn: WrapDockerExec("my-container", ""), - cmdArgs: []string{"my-cmd"}, - wantCmd: []string{"docker", "exec", "--interactive", "my-container", "my-cmd"}, - }, - { - name: "tty cmd with no args", - wrapFn: WrapDockerExecPTY("my-container", "my-user"), - cmdArgs: []string{"my-cmd"}, - wantCmd: []string{"docker", "exec", "--interactive", "--tty", "--user", "my-user", "my-container", "my-cmd"}, + name: "cmd with no args", + containerUser: "my-user", + cmdArgs: []string{"my-cmd"}, + wantCmd: []string{"docker", "exec", "--interactive", "--user", "my-user", "my-container", "my-cmd"}, }, { - name: "cmd with args", - wrapFn: WrapDockerExecPTY("my-container", "my-user"), - cmdArgs: []string{"my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, - wantCmd: []string{"docker", "exec", "--interactive", "--tty", "--user", "my-user", "my-container", "my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, + name: "cmd with args", + containerUser: "my-user", + cmdArgs: []string{"my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, + wantCmd: []string{"docker", "exec", "--interactive", "--user", "my-user", "my-container", "my-cmd", "arg1", "--arg2", "arg3", "--arg4"}, }, { - name: "no user specified", - wrapFn: WrapDockerExecPTY("my-container", ""), - cmdArgs: []string{"my-cmd"}, - wantCmd: []string{"docker", "exec", "--interactive", "--tty", "my-container", "my-cmd"}, + name: "no user specified", + containerUser: "", + cmdArgs: []string{"my-cmd"}, + wantCmd: []string{"docker", "exec", "--interactive", "my-container", "my-cmd"}, }, } for _, tt := range tests { tt := tt // appease the linter even though this isn't needed anymore t.Run(tt.name, func(t *testing.T) { t.Parallel() - actualCmd, actualArgs := tt.wrapFn(tt.cmdArgs[0], tt.cmdArgs[1:]...) + actualCmd, actualArgs := wrapDockerExec("my-container", tt.containerUser, tt.cmdArgs[0], tt.cmdArgs[1:]...) assert.Equal(t, tt.wantCmd[0], actualCmd) assert.Equal(t, tt.wantCmd[1:], actualArgs) }) @@ -439,50 +411,44 @@ func TestDockerEnvInfoer(t *testing.T) { require.NoError(t, err, "Could not connect to docker") // nolint:paralleltest // variable recapture no longer required for idx, tt := range []struct { - image string - env []string - containerUser string - expectedUsername string - expectedUserHomedir string - expectedUserShell string + image string + env []string + containerUser string + expectedUsername string + expectedUserShell string }{ { - image: "busybox:latest", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, - expectedUsername: "root", - expectedUserHomedir: "/root", - expectedUserShell: "/bin/sh", + image: "busybox:latest", + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + expectedUsername: "root", + expectedUserShell: "/bin/sh", }, { - image: "busybox:latest", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, - containerUser: "root", - expectedUsername: "root", - expectedUserHomedir: "/root", - expectedUserShell: "/bin/sh", + image: "busybox:latest", + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + containerUser: "root", + expectedUsername: "root", + expectedUserShell: "/bin/sh", }, { - image: "codercom/enterprise-minimal:ubuntu", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, - expectedUsername: "coder", - expectedUserHomedir: "/home/coder", - expectedUserShell: "/bin/bash", + image: "codercom/enterprise-minimal:ubuntu", + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + expectedUsername: "coder", + expectedUserShell: "/bin/bash", }, { - image: "codercom/enterprise-minimal:ubuntu", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, - containerUser: "coder", - expectedUsername: "coder", - expectedUserHomedir: "/home/coder", - expectedUserShell: "/bin/bash", + image: "codercom/enterprise-minimal:ubuntu", + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + containerUser: "coder", + expectedUsername: "coder", + expectedUserShell: "/bin/bash", }, { - image: "codercom/enterprise-minimal:ubuntu", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, - containerUser: "root", - expectedUsername: "root", - expectedUserHomedir: "/root", - expectedUserShell: "/bin/bash", + image: "codercom/enterprise-minimal:ubuntu", + env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + containerUser: "root", + expectedUsername: "root", + expectedUserShell: "/bin/bash", }, } { t.Run(fmt.Sprintf("#%d", idx), func(t *testing.T) { @@ -518,14 +484,15 @@ func TestDockerEnvInfoer(t *testing.T) { hd, err := dei.UserHomeDir() require.NoError(t, err, "Expected no error from UserHomeDir()") - require.Equal(t, tt.expectedUserHomedir, hd, "Expected user homedir to match") + require.NotEmpty(t, hd, "Expected user homedir to be non-empty") sh, err := dei.UserShell(tt.containerUser) require.NoError(t, err, "Expected no error from UserShell()") require.Equal(t, tt.expectedUserShell, sh, "Expected user shell to match") - environ := dei.Environ() - require.Subset(t, environ, tt.env, "Expected environment to match") + // TODO: environment from devcontainer labels. + // environ := dei.Environ() + // require.Subset(t, environ, tt.env, "Expected environment to match") }) } } From 467a6e8843d56f6e278c5a8f540f43ff82511f75 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 19 Feb 2025 23:09:37 +0000 Subject: [PATCH 6/9] env from devcontainer metadata --- agent/agentcontainers/containers_dockercli.go | 103 ++++++++++++++---- .../containers_internal_test.go | 76 ++++++++++--- 2 files changed, 137 insertions(+), 42 deletions(-) diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index c31521629f97e..906f8ab89b029 100644 --- a/agent/agentcontainers/containers_dockercli.go +++ b/agent/agentcontainers/containers_dockercli.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "os/user" + "slices" "sort" "strconv" "strings" @@ -109,13 +110,14 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU } cei.userShell = passwdFields[6] - // Finally, get the environment of the container. - // TODO: For containers, we can't simply run `env` inside the container. // We need to inspect the container labels for remoteEnv and append these to // the resulting docker exec command. // ref: https://code.visualstudio.com/docs/devcontainers/attach-container - // For now, we're simply returning the host environment. - cei.env = make([]string, 0) + env, err := devcontainerEnv(ctx, execer, container) + if err != nil { // best effort. + return nil, xerrors.Errorf("read devcontainer remoteEnv: %w", err) + } + cei.env = env return &cei, nil } @@ -158,12 +160,57 @@ func (cei *ContainerEnvInfoer) ModifyCommand(cmd string, args ...string) (string // Set the working directory to the user's home directory as a sane default. "--workdir", cei.user.HomeDir, - cei.container, - cmd, } + + // Append the environment variables from the container. + for _, e := range cei.env { + dockerArgs = append(dockerArgs, "--env", e) + } + + // Append the container name and the command. + dockerArgs = append(dockerArgs, cei.container, cmd) return "docker", append(dockerArgs, args...) } +// devcontainerEnv is a helper function that inspects the container labels to +// find the required environment variables for running a command in the container. +func devcontainerEnv(ctx context.Context, execer agentexec.Execer, container string) ([]string, error) { + ins, stderr, err := runDockerInspect(ctx, execer, container) + if err != nil { + return nil, xerrors.Errorf("inspect container: %w: %q", err, stderr) + } + + if len(ins) != 1 { + return nil, xerrors.Errorf("inspect container: expected 1 container, got %d", len(ins)) + } + + in := ins[0] + if in.Config.Labels == nil { + return nil, nil + } + + // We want to look for the devcontainer metadata, which is in the + // value of the label `devcontainer.metadata`. + rawMeta, ok := in.Config.Labels["devcontainer.metadata"] + if !ok { + return nil, nil + } + meta := struct { + RemoteEnv map[string]string `json:"remoteEnv"` + }{} + if err := json.Unmarshal([]byte(rawMeta), &meta); err != nil { + return nil, xerrors.Errorf("unmarshal devcontainer.metadata: %w", err) + } + + // The environment variables are stored in the `remoteEnv` key. + env := make([]string, 0, len(meta.RemoteEnv)) + for k, v := range meta.RemoteEnv { + env = append(env, fmt.Sprintf("%s=%s", k, v)) + } + slices.Sort(env) + return env, nil +} + // wrapDockerExec is a helper function that wraps the given command and arguments // with a docker exec command that runs as the given user in the given // container. This is used to fetch information about a container prior to @@ -227,30 +274,16 @@ func (dcl *DockerCLILister) List(ctx context.Context) (codersdk.WorkspaceAgentLi } // now we can get the detailed information for each container - // Run `docker inspect` on each container ID - stdoutBuf.Reset() - stderrBuf.Reset() - // nolint: gosec // We are not executing user input, these IDs come from - // `docker ps`. - cmd = dcl.execer.CommandContext(ctx, "docker", append([]string{"inspect"}, ids...)...) - cmd.Stdout = &stdoutBuf - cmd.Stderr = &stderrBuf - if err := cmd.Run(); err != nil { - return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("run docker inspect: %w: %s", err, strings.TrimSpace(stderrBuf.String())) - } - - dockerInspectStderr := strings.TrimSpace(stderrBuf.String()) - + // Run `docker inspect` on each container ID. // NOTE: There is an unavoidable potential race condition where a // container is removed between `docker ps` and `docker inspect`. // In this case, stderr will contain an error message but stdout // will still contain valid JSON. We will just end up missing // information about the removed container. We could potentially // log this error, but I'm not sure it's worth it. - ins := make([]dockerInspect, 0, len(ids)) - if err := json.NewDecoder(&stdoutBuf).Decode(&ins); err != nil { - // However, if we just get invalid JSON, we should absolutely return an error. - return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("decode docker inspect output: %w", err) + ins, dockerInspectStderr, err := runDockerInspect(ctx, dcl.execer, ids...) + if err != nil { + return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("run docker inspect: %w", err) } res := codersdk.WorkspaceAgentListContainersResponse{ @@ -272,6 +305,28 @@ func (dcl *DockerCLILister) List(ctx context.Context) (codersdk.WorkspaceAgentLi return res, nil } +// runDockerInspect is a helper function that runs `docker inspect` on the given +// container IDs and returns the parsed output. +// The stderr output is also returned for logging purposes. +func runDockerInspect(ctx context.Context, execer agentexec.Execer, ids ...string) ([]dockerInspect, string, error) { + var stdoutBuf, stderrBuf bytes.Buffer + cmd := execer.CommandContext(ctx, "docker", append([]string{"inspect"}, ids...)...) + cmd.Stdout = &stdoutBuf + cmd.Stderr = &stderrBuf + err := cmd.Run() + stderr := strings.TrimSpace(stderrBuf.String()) + if err != nil { + return nil, stderr, err + } + + var ins []dockerInspect + if err := json.NewDecoder(&stdoutBuf).Decode(&ins); err != nil { + return nil, stderr, xerrors.Errorf("decode docker inspect output: %w", err) + } + + return ins, stderr, nil +} + // To avoid a direct dependency on the Docker API, we use the docker CLI // to fetch information about containers. type dockerInspect struct { diff --git a/agent/agentcontainers/containers_internal_test.go b/agent/agentcontainers/containers_internal_test.go index 7dfcd145c62e0..d30862a07282f 100644 --- a/agent/agentcontainers/containers_internal_test.go +++ b/agent/agentcontainers/containers_internal_test.go @@ -3,6 +3,7 @@ package agentcontainers import ( "fmt" "os" + "slices" "strconv" "strings" "testing" @@ -47,10 +48,13 @@ func TestIntegrationDocker(t *testing.T) { // Pick a random port to expose for testing port bindings. testRandPort := testutil.RandomPortNoListen(t) ct, err := pool.RunWithOptions(&dockertest.RunOptions{ - Repository: "busybox", - Tag: "latest", - Cmd: []string{"sleep", "infnity"}, - Labels: map[string]string{"com.coder.test": testLabelValue}, + Repository: "busybox", + Tag: "latest", + Cmd: []string{"sleep", "infnity"}, + Labels: map[string]string{ + "com.coder.test": testLabelValue, + "devcontainer.metadata": `{"remoteEnv": {"FOO": "bar", "MULTILINE": "foo\nbar\nbaz"}}`, + }, Mounts: []string{testTempDir + ":" + testTempDir}, ExposedPorts: []string{fmt.Sprintf("%d/tcp", testRandPort)}, PortBindings: map[docker.Port][]docker.PortBinding{ @@ -122,6 +126,12 @@ func TestIntegrationDocker(t *testing.T) { matchHostnameOuput := func(line string) bool { return strings.Contains(strings.TrimSpace(line), ct.Container.Config.Hostname) } + matchEnvCmd := func(line string) bool { + return strings.Contains(strings.TrimSpace(line), "env") + } + matchEnvOutput := func(line string) bool { + return strings.Contains(line, "FOO=bar") || strings.Contains(line, "MULTILINE=foo") + } require.NoError(t, tr.ReadUntil(ctx, matchPrompt), "failed to match prompt") t.Logf("Matched prompt") _, err = ptyCmd.InputWriter().Write([]byte("hostname\r\n")) @@ -131,6 +141,13 @@ func TestIntegrationDocker(t *testing.T) { t.Logf("Matched hostname command") require.NoError(t, tr.ReadUntil(ctx, matchHostnameOuput), "failed to match hostname output") t.Logf("Matched hostname output") + _, err = ptyCmd.InputWriter().Write([]byte("env\r\n")) + require.NoError(t, err, "failed to write to pty") + t.Logf("Wrote env command") + require.NoError(t, tr.ReadUntil(ctx, matchEnvCmd), "failed to match env command") + t.Logf("Matched env command") + require.NoError(t, tr.ReadUntil(ctx, matchEnvOutput), "failed to match env output") + t.Logf("Matched env output") break } } @@ -403,49 +420,56 @@ func TestConvertDockerVolume(t *testing.T) { // CODER_TEST_USE_DOCKER=1 go test ./agent/agentcontainers -run TestDockerEnvInfoer func TestDockerEnvInfoer(t *testing.T) { t.Parallel() - if ctud, ok := os.LookupEnv("CODER_TEST_USE_DOCKER"); !ok || ctud != "1" { - t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") - } + // if ctud, ok := os.LookupEnv("CODER_TEST_USE_DOCKER"); !ok || ctud != "1" { + // t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") + // } pool, err := dockertest.NewPool("") require.NoError(t, err, "Could not connect to docker") // nolint:paralleltest // variable recapture no longer required for idx, tt := range []struct { image string - env []string + labels map[string]string + expectedEnv []string containerUser string expectedUsername string expectedUserShell string }{ { - image: "busybox:latest", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + image: "busybox:latest", + labels: map[string]string{`devcontainer.metadata`: `{"remoteEnv": {"FOO": "bar", "MULTILINE": "foo\nbar\nbaz"}}`}, + + expectedEnv: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, expectedUsername: "root", expectedUserShell: "/bin/sh", }, { image: "busybox:latest", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + labels: map[string]string{`devcontainer.metadata`: `{"remoteEnv": {"FOO": "bar", "MULTILINE": "foo\nbar\nbaz"}}`}, + expectedEnv: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, containerUser: "root", expectedUsername: "root", expectedUserShell: "/bin/sh", }, { image: "codercom/enterprise-minimal:ubuntu", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + labels: map[string]string{`devcontainer.metadata`: `{"remoteEnv": {"FOO": "bar", "MULTILINE": "foo\nbar\nbaz"}}`}, + expectedEnv: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, expectedUsername: "coder", expectedUserShell: "/bin/bash", }, { image: "codercom/enterprise-minimal:ubuntu", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + labels: map[string]string{`devcontainer.metadata`: `{"remoteEnv": {"FOO": "bar", "MULTILINE": "foo\nbar\nbaz"}}`}, + expectedEnv: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, containerUser: "coder", expectedUsername: "coder", expectedUserShell: "/bin/bash", }, { image: "codercom/enterprise-minimal:ubuntu", - env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, + labels: map[string]string{`devcontainer.metadata`: `{"remoteEnv": {"FOO": "bar", "MULTILINE": "foo\nbar\nbaz"}}`}, + expectedEnv: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"}, containerUser: "root", expectedUsername: "root", expectedUserShell: "/bin/bash", @@ -462,7 +486,7 @@ func TestDockerEnvInfoer(t *testing.T) { Repository: image, Tag: tag, Cmd: []string{"sleep", "infinity"}, - Env: tt.env, + Labels: tt.labels, }, func(config *docker.HostConfig) { config.AutoRemove = true config.RestartPolicy = docker.RestartPolicy{Name: "no"} @@ -490,9 +514,25 @@ func TestDockerEnvInfoer(t *testing.T) { require.NoError(t, err, "Expected no error from UserShell()") require.Equal(t, tt.expectedUserShell, sh, "Expected user shell to match") - // TODO: environment from devcontainer labels. - // environ := dei.Environ() - // require.Subset(t, environ, tt.env, "Expected environment to match") + // We don't need to test the actual environment variables here. + environ := dei.Environ() + require.NotEmpty(t, environ, "Expected environ to be non-empty") + + // Test that the environment variables are present in modified command + // output. + envCmd, envArgs := dei.ModifyCommand("env") + for _, env := range tt.expectedEnv { + require.Subset(t, envArgs, []string{"--env", env}) + } + // Run the command in the container and check the output + // HACK: we remove the --tty argument because we're not running in a tty + envArgs = slices.DeleteFunc(envArgs, func(s string) bool { return s == "--tty" }) + stdout, stderr, err := run(ctx, agentexec.DefaultExecer, envCmd, envArgs...) + require.Empty(t, stderr, "Expected no stderr output") + require.NoError(t, err, "Expected no error from running command") + for _, env := range tt.expectedEnv { + require.Contains(t, stdout, env) + } }) } } From 06b9b7ec14521629b109919c2c00b38c30c73e4a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 19 Feb 2025 23:18:35 +0000 Subject: [PATCH 7/9] re-add comment --- agent/agentcontainers/containers_internal_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/agent/agentcontainers/containers_internal_test.go b/agent/agentcontainers/containers_internal_test.go index d30862a07282f..cdda03f9c8200 100644 --- a/agent/agentcontainers/containers_internal_test.go +++ b/agent/agentcontainers/containers_internal_test.go @@ -420,9 +420,9 @@ func TestConvertDockerVolume(t *testing.T) { // CODER_TEST_USE_DOCKER=1 go test ./agent/agentcontainers -run TestDockerEnvInfoer func TestDockerEnvInfoer(t *testing.T) { t.Parallel() - // if ctud, ok := os.LookupEnv("CODER_TEST_USE_DOCKER"); !ok || ctud != "1" { - // t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") - // } + if ctud, ok := os.LookupEnv("CODER_TEST_USE_DOCKER"); !ok || ctud != "1" { + t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") + } pool, err := dockertest.NewPool("") require.NoError(t, err, "Could not connect to docker") From bf101d336118734ef74db633e61b8043bbfdd8c7 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 24 Feb 2025 12:22:54 +0000 Subject: [PATCH 8/9] PR comments again --- agent/agentcontainers/containers_dockercli.go | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index 906f8ab89b029..eeb73f37dca99 100644 --- a/agent/agentcontainers/containers_dockercli.go +++ b/agent/agentcontainers/containers_dockercli.go @@ -34,9 +34,9 @@ func NewDocker(execer agentexec.Execer) Lister { } } -// ContainerEnvInfoer is an implementation of agentssh.EnvInfoer that returns +// DockerEnvInfoer is an implementation of agentssh.EnvInfoer that returns // information about a container. -type ContainerEnvInfoer struct { +type DockerEnvInfoer struct { container string user *user.User userShell string @@ -44,9 +44,9 @@ type ContainerEnvInfoer struct { } // EnvInfo returns information about the environment of a container. -func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerUser string) (*ContainerEnvInfoer, error) { - var cei ContainerEnvInfoer - cei.container = container +func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerUser string) (*DockerEnvInfoer, error) { + var dei DockerEnvInfoer + dei.container = container if containerUser == "" { // Get the "default" user of the container if no user is specified. @@ -93,7 +93,7 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU return nil, xerrors.Errorf("get container user: invalid line in /etc/passwd: %q", foundLine) } - // The fourth entry in /etc/passwd contains GECOS information, which is a + // The fifth entry in /etc/passwd contains GECOS information, which is a // comma-separated list of fields. The first field is the user's full name. gecos := strings.Split(passwdFields[4], ",") fullName := "" @@ -101,14 +101,14 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU fullName = gecos[0] } - cei.user = &user.User{ + dei.user = &user.User{ Gid: passwdFields[3], HomeDir: passwdFields[5], Name: fullName, Uid: passwdFields[2], Username: containerUser, } - cei.userShell = passwdFields[6] + dei.userShell = passwdFields[6] // We need to inspect the container labels for remoteEnv and append these to // the resulting docker exec command. @@ -117,23 +117,23 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU if err != nil { // best effort. return nil, xerrors.Errorf("read devcontainer remoteEnv: %w", err) } - cei.env = env + dei.env = env - return &cei, nil + return &dei, nil } -func (cei *ContainerEnvInfoer) CurrentUser() (*user.User, error) { +func (dei *DockerEnvInfoer) CurrentUser() (*user.User, error) { // Clone the user so that the caller can't modify it - u := *cei.user + u := *dei.user return &u, nil } -func (*ContainerEnvInfoer) Environ() []string { +func (*DockerEnvInfoer) Environ() []string { // Return a clone of the environment so that the caller can't modify it return os.Environ() } -func (*ContainerEnvInfoer) UserHomeDir() (string, error) { +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 @@ -141,14 +141,13 @@ func (*ContainerEnvInfoer) UserHomeDir() (string, error) { return os.UserHomeDir() } -func (cei *ContainerEnvInfoer) UserShell(string) (string, error) { - return cei.userShell, nil +func (dei *DockerEnvInfoer) UserShell(string) (string, error) { + return dei.userShell, nil } -func (cei *ContainerEnvInfoer) ModifyCommand(cmd string, args ...string) (string, []string) { +func (dei *DockerEnvInfoer) ModifyCommand(cmd string, args ...string) (string, []string) { // Wrap the command with `docker exec` and run it as the container user. // There is some additional munging here regarding the container user and environment. - // return WrapDockerExecPTY(dei.container, dei.user.Username)(cmd, args...) dockerArgs := []string{ "exec", // The assumption is that this command will be a shell command, so allocate a PTY. @@ -156,19 +155,19 @@ func (cei *ContainerEnvInfoer) ModifyCommand(cmd string, args ...string) (string "--tty", // Run the command as the user in the container. "--user", - cei.user.Username, + dei.user.Username, // Set the working directory to the user's home directory as a sane default. "--workdir", - cei.user.HomeDir, + dei.user.HomeDir, } // Append the environment variables from the container. - for _, e := range cei.env { + for _, e := range dei.env { dockerArgs = append(dockerArgs, "--env", e) } // Append the container name and the command. - dockerArgs = append(dockerArgs, cei.container, cmd) + dockerArgs = append(dockerArgs, dei.container, cmd) return "docker", append(dockerArgs, args...) } From f06105a7d13434f88e4ce7e1b33e785eaf820fb5 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 24 Feb 2025 14:44:42 +0000 Subject: [PATCH 9/9] strings.Builder --- agent/agentcontainers/containers_dockercli.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index eeb73f37dca99..64f264c1ba730 100644 --- a/agent/agentcontainers/containers_dockercli.go +++ b/agent/agentcontainers/containers_dockercli.go @@ -228,7 +228,7 @@ func wrapDockerExec(containerName, userName, cmd string, args ...string) (string // We also want to differentiate between a command running successfully with // output to stderr and a non-zero exit code. func run(ctx context.Context, execer agentexec.Execer, cmd string, args ...string) (stdout, stderr string, err error) { - var stdoutBuf, stderrBuf bytes.Buffer + var stdoutBuf, stderrBuf strings.Builder execCmd := execer.CommandContext(ctx, cmd, args...) execCmd.Stdout = &stdoutBuf execCmd.Stderr = &stderrBuf