Skip to content

Commit d52d0c1

Browse files
johnstcnjohnstcn
committed
fix issue with docker homedir and env clobbering real env for cmd
1 parent 5e57035 commit d52d0c1

File tree

2 files changed

+118
-161
lines changed

2 files changed

+118
-161
lines changed

‎agent/agentcontainers/containers_dockercli.go

Lines changed: 63 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@ import (
66
"context"
77
"encoding/json"
88
"fmt"
9+
"os"
910
"os/user"
10-
"slices"
1111
"sort"
1212
"strconv"
1313
"strings"
@@ -42,21 +42,6 @@ type ContainerEnvInfoer struct {
4242
env []string
4343
}
4444

45-
// Helper function to run a command and return its stdout and stderr.
46-
// We want to differentiate stdout and stderr instead of using CombinedOutput.
47-
// We also want to differentiate between a command running successfully with
48-
// output to stderr and a non-zero exit code.
49-
func run(ctx context.Context, execer agentexec.Execer, cmd string, args ...string) (stdout, stderr string, err error) {
50-
var stdoutBuf, stderrBuf bytes.Buffer
51-
execCmd := execer.CommandContext(ctx, cmd, args...)
52-
execCmd.Stdout = &stdoutBuf
53-
execCmd.Stderr = &stderrBuf
54-
err = execCmd.Run()
55-
stdout = strings.TrimSpace(stdoutBuf.String())
56-
stderr = strings.TrimSpace(stderrBuf.String())
57-
return stdout, stderr, err
58-
}
59-
6045
// EnvInfo returns information about the environment of a container.
6146
func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerUser string) (*ContainerEnvInfoer, error) {
6247
var cei ContainerEnvInfoer
@@ -65,7 +50,7 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU
6550
if containerUser == "" {
6651
// Get the "default" user of the container if no user is specified.
6752
// TODO: handle different container runtimes.
68-
cmd, args := WrapDockerExec(container, "")("whoami")
53+
cmd, args := wrapDockerExec(container, "", "whoami")
6954
stdout, stderr, err := run(ctx, execer, cmd, args...)
7055
if err != nil {
7156
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
7762
}
7863
// Now that we know the username, get the required info from the container.
7964
// We can't assume the presence of `getent` so we'll just have to sniff /etc/passwd.
80-
cmd, args := WrapDockerExec(container, containerUser)("cat", "/etc/passwd")
65+
cmd, args := wrapDockerExec(container, containerUser, "cat", "/etc/passwd")
8166
stdout, stderr, err := run(ctx, execer, cmd, args...)
8267
if err != nil {
8368
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
125110
cei.userShell = passwdFields[6]
126111

127112
// Finally, get the environment of the container.
128-
// TODO(cian): for devcontainers we will need to also take into account both
129-
// remoteEnv and userEnvProbe.
113+
// TODO: For containers, we can't simply run `env` inside the container.
114+
// We need to inspect the container labels for remoteEnv and append these to
115+
// the resulting docker exec command.
130116
// ref: https://code.visualstudio.com/docs/devcontainers/attach-container
131-
cmd, args = WrapDockerExec(container, containerUser)("env", "-0")
132-
stdout, stderr, err = run(ctx, execer, cmd, args...)
133-
if err != nil {
134-
return nil, xerrors.Errorf("get container environment: run env -0: %w: %q", err, stderr)
135-
}
136-
137-
// Parse the output of `env -0` which is a null-terminated list of environment
138-
// variables. This is important as environment variables can contain newlines.
139-
envs := strings.Split(stdout, "\x00")
140-
for _, env := range envs {
141-
env = strings.TrimSpace(env)
142-
if env == "" {
143-
continue
144-
}
145-
cei.env = append(cei.env, env)
146-
}
147-
if err := scanner.Err(); err != nil {
148-
return nil, xerrors.Errorf("get container environment: scan env output: %w", err)
149-
}
117+
// For now, we're simply returning the host environment.
118+
cei.env = make([]string, 0)
150119

151120
return &cei, nil
152121
}
153122

154-
func (dei *ContainerEnvInfoer) CurrentUser() (*user.User, error) {
123+
func (cei *ContainerEnvInfoer) CurrentUser() (*user.User, error) {
155124
// Clone the user so that the caller can't modify it
156-
u := *dei.user
125+
u := *cei.user
157126
return &u, nil
158127
}
159128

160-
func (dei *ContainerEnvInfoer) Environ() []string {
129+
func (*ContainerEnvInfoer) Environ() []string {
161130
// Return a clone of the environment so that the caller can't modify it
162-
return slices.Clone(dei.env)
131+
return os.Environ()
163132
}
164133

165-
func (dei *ContainerEnvInfoer) UserHomeDir() (string, error) {
166-
return dei.user.HomeDir, nil
134+
func (*ContainerEnvInfoer) UserHomeDir() (string, error) {
135+
// We default the working directory of the command to the user's home
136+
// directory. Since this came from inside the container, we cannot guarantee
137+
// that this exists on the host. Return the "real" home directory of the user
138+
// instead.
139+
return os.UserHomeDir()
167140
}
168141

169-
func (dei *ContainerEnvInfoer) UserShell(string) (string, error) {
170-
return dei.userShell, nil
142+
func (cei *ContainerEnvInfoer) UserShell(string) (string, error) {
143+
return cei.userShell, nil
171144
}
172145

173-
func (dei *ContainerEnvInfoer) ModifyCommand(cmd string, args ...string) (string, []string) {
174-
return WrapDockerExecPTY(dei.container, dei.user.Username)(cmd, args...)
146+
func (cei *ContainerEnvInfoer) ModifyCommand(cmd string, args ...string) (string, []string) {
147+
// Wrap the command with `docker exec` and run it as the container user.
148+
// There is some additional munging here regarding the container user and environment.
149+
// return WrapDockerExecPTY(dei.container, dei.user.Username)(cmd, args...)
150+
dockerArgs := []string{
151+
"exec",
152+
// The assumption is that this command will be a shell command, so allocate a PTY.
153+
"--interactive",
154+
"--tty",
155+
// Run the command as the user in the container.
156+
"--user",
157+
cei.user.Username,
158+
// Set the working directory to the user's home directory as a sane default.
159+
"--workdir",
160+
cei.user.HomeDir,
161+
cei.container,
162+
cmd,
163+
}
164+
return "docker", append(dockerArgs, args...)
175165
}
176166

177-
// WrapFn is a function that wraps a command and its arguments with another command and arguments.
178-
type WrapFn func(cmd string, args ...string) (string, []string)
179-
180-
// WrapDockerExec returns a WrapFn that wraps the given command and arguments
181-
// with a docker exec command that runs as the given user in the given container.
182-
func WrapDockerExec(containerName, userName string) WrapFn {
183-
return func(cmd string, args ...string) (string, []string) {
184-
dockerArgs := []string{"exec", "--interactive"}
185-
if userName != "" {
186-
dockerArgs = append(dockerArgs, "--user", userName)
187-
}
188-
dockerArgs = append(dockerArgs, containerName, cmd)
189-
return "docker", append(dockerArgs, args...)
190-
}
167+
// wrapDockerExec is a helper function that wraps the given command and arguments
168+
// with a docker exec command that runs as the given user in the given
169+
// container. This is used to fetch information about a container prior to
170+
// running the actual command.
171+
func wrapDockerExec(containerName, userName, cmd string, args ...string) (string, []string) {
172+
dockerArgs := []string{"exec", "--interactive"}
173+
if userName != "" {
174+
dockerArgs = append(dockerArgs, "--user", userName)
175+
}
176+
dockerArgs = append(dockerArgs, containerName, cmd)
177+
return "docker", append(dockerArgs, args...)
191178
}
192179

193-
// WrapDockerExecPTY is similar to WrapDockerExec but also allocates a PTY.
194-
func WrapDockerExecPTY(containerName, userName string) WrapFn {
195-
return func(cmd string, args ...string) (string, []string) {
196-
dockerArgs := []string{"exec", "--interactive", "--tty"}
197-
if userName != "" {
198-
dockerArgs = append(dockerArgs, "--user", userName)
199-
}
200-
dockerArgs = append(dockerArgs, containerName, cmd)
201-
return "docker", append(dockerArgs, args...)
202-
}
180+
// Helper function to run a command and return its stdout and stderr.
181+
// We want to differentiate stdout and stderr instead of using CombinedOutput.
182+
// We also want to differentiate between a command running successfully with
183+
// output to stderr and a non-zero exit code.
184+
func run(ctx context.Context, execer agentexec.Execer, cmd string, args ...string) (stdout, stderr string, err error) {
185+
var stdoutBuf, stderrBuf bytes.Buffer
186+
execCmd := execer.CommandContext(ctx, cmd, args...)
187+
execCmd.Stdout = &stdoutBuf
188+
execCmd.Stderr = &stderrBuf
189+
err = execCmd.Run()
190+
stdout = strings.TrimSpace(stdoutBuf.String())
191+
stderr = strings.TrimSpace(stderrBuf.String())
192+
return stdout, stderr, err
203193
}
204194

205195
func (dcl *DockerCLILister) List(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) {

0 commit comments

Comments
 (0)