Skip to content

Commit 5e57035

Browse files
johnstcnjohnstcn
committed
address mafcode review comments
1 parent 7fd24be commit 5e57035

File tree

2 files changed

+55
-52
lines changed

2 files changed

+55
-52
lines changed

‎agent/agentcontainers/containers_dockercli.go

Lines changed: 49 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -42,51 +42,56 @@ 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+
4560
// EnvInfo returns information about the environment of a container.
4661
func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerUser string) (*ContainerEnvInfoer, error) {
47-
var dei ContainerEnvInfoer
48-
dei.container = container
62+
var cei ContainerEnvInfoer
63+
cei.container = container
4964

50-
var stdoutBuf, stderrBuf bytes.Buffer
5165
if containerUser == "" {
5266
// Get the "default" user of the container if no user is specified.
5367
// TODO: handle different container runtimes.
5468
cmd, args := WrapDockerExec(container, "")("whoami")
55-
execCmd := execer.CommandContext(ctx, cmd, args...)
56-
execCmd.Stdout = &stdoutBuf
57-
execCmd.Stderr = &stderrBuf
58-
if err := execCmd.Run(); err != nil {
59-
return nil, xerrors.Errorf("get container user: run whoami: %w: stderr: %q", err, strings.TrimSpace(stderrBuf.String()))
69+
stdout, stderr, err := run(ctx, execer, cmd, args...)
70+
if err != nil {
71+
return nil, xerrors.Errorf("get container user: run whoami: %w: %s", err, stderr)
6072
}
61-
out := strings.TrimSpace(stdoutBuf.String())
62-
if len(out) == 0 {
63-
return nil, xerrors.Errorf("get container user: run whoami: empty output: stderr: %q", strings.TrimSpace(stderrBuf.String()))
73+
if len(stdout) == 0 {
74+
return nil, xerrors.Errorf("get container user: run whoami: empty output")
6475
}
65-
containerUser = out
66-
stdoutBuf.Reset()
67-
stderrBuf.Reset()
76+
containerUser = stdout
6877
}
6978
// Now that we know the username, get the required info from the container.
7079
// We can't assume the presence of `getent` so we'll just have to sniff /etc/passwd.
7180
cmd, args := WrapDockerExec(container, containerUser)("cat", "/etc/passwd")
72-
execCmd := execer.CommandContext(ctx, cmd, args...)
73-
execCmd.Stdout = &stdoutBuf
74-
execCmd.Stderr = &stderrBuf
75-
if err := execCmd.Run(); err != nil {
76-
return nil, xerrors.Errorf("get container user: read /etc/passwd: %w stderr: %q", err, strings.TrimSpace(stderrBuf.String()))
81+
stdout, stderr, err := run(ctx, execer, cmd, args...)
82+
if err != nil {
83+
return nil, xerrors.Errorf("get container user: read /etc/passwd: %w: %q", err, stderr)
7784
}
7885

79-
scanner := bufio.NewScanner(&stdoutBuf)
86+
scanner := bufio.NewScanner(strings.NewReader(stdout))
8087
var foundLine string
8188
for scanner.Scan() {
8289
line := strings.TrimSpace(scanner.Text())
83-
if len(line) == 0 {
84-
continue
85-
}
8690
if !strings.HasPrefix(line, containerUser+":") {
8791
continue
8892
}
8993
foundLine = line
94+
break
9095
}
9196
if err := scanner.Err(); err != nil {
9297
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
110115
fullName = gecos[0]
111116
}
112117

113-
dei.user = &user.User{
118+
cei.user = &user.User{
114119
Gid: passwdFields[3],
115120
HomeDir: passwdFields[5],
116121
Name: fullName,
117122
Uid: passwdFields[2],
118123
Username: containerUser,
119124
}
120-
dei.userShell = passwdFields[6]
125+
cei.userShell = passwdFields[6]
121126

122127
// Finally, get the environment of the container.
123-
stdoutBuf.Reset()
124-
stderrBuf.Reset()
125-
cmd, args = WrapDockerExec(container, containerUser)("env")
126-
execCmd = execer.CommandContext(ctx, cmd, args...)
127-
execCmd.Stdout = &stdoutBuf
128-
execCmd.Stderr = &stderrBuf
129-
if err := execCmd.Run(); err != nil {
130-
return nil, xerrors.Errorf("get container environment: run env: %w stderr: %q", err, strings.TrimSpace(stderrBuf.String()))
131-
}
132-
133-
scanner = bufio.NewScanner(&stdoutBuf)
134-
for scanner.Scan() {
135-
line := strings.TrimSpace(scanner.Text())
136-
dei.env = append(dei.env, line)
128+
// TODO(cian): for devcontainers we will need to also take into account both
129+
// remoteEnv and userEnvProbe.
130+
// 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)
137146
}
138147
if err := scanner.Err(); err != nil {
139148
return nil, xerrors.Errorf("get container environment: scan env output: %w", err)
140149
}
141150

142-
return &dei, nil
151+
return &cei, nil
143152
}
144153

145154
func (dei *ContainerEnvInfoer) CurrentUser() (*user.User, error) {

‎agent/agentcontainers/containers_internal_test.go

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -445,50 +445,44 @@ func TestDockerEnvInfoer(t *testing.T) {
445445
expectedUsername string
446446
expectedUserHomedir string
447447
expectedUserShell string
448-
expectedEnviron []string
449448
}{
450449
{
451450
image: "busybox:latest",
452-
env: []string{"FOO=bar"},
451+
env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"},
453452
expectedUsername: "root",
454453
expectedUserHomedir: "/root",
455454
expectedUserShell: "/bin/sh",
456-
expectedEnviron: []string{"FOO=bar"},
457455
},
458456
{
459457
image: "busybox:latest",
460-
env: []string{"FOO=bar"},
458+
env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"},
461459
containerUser: "root",
462460
expectedUsername: "root",
463461
expectedUserHomedir: "/root",
464462
expectedUserShell: "/bin/sh",
465-
expectedEnviron: []string{"FOO=bar"},
466463
},
467464
{
468465
image: "codercom/enterprise-minimal:ubuntu",
469-
env: []string{"FOO=bar"},
466+
env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"},
470467
expectedUsername: "coder",
471468
expectedUserHomedir: "/home/coder",
472469
expectedUserShell: "/bin/bash",
473-
expectedEnviron: []string{"FOO=bar"},
474470
},
475471
{
476472
image: "codercom/enterprise-minimal:ubuntu",
477-
env: []string{"FOO=bar"},
473+
env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"},
478474
containerUser: "coder",
479475
expectedUsername: "coder",
480476
expectedUserHomedir: "/home/coder",
481477
expectedUserShell: "/bin/bash",
482-
expectedEnviron: []string{"FOO=bar"},
483478
},
484479
{
485480
image: "codercom/enterprise-minimal:ubuntu",
486-
env: []string{"FOO=bar"},
481+
env: []string{"FOO=bar", "MULTILINE=foo\nbar\nbaz"},
487482
containerUser: "root",
488483
expectedUsername: "root",
489484
expectedUserHomedir: "/root",
490485
expectedUserShell: "/bin/bash",
491-
expectedEnviron: []string{"FOO=bar"},
492486
},
493487
} {
494488
t.Run(fmt.Sprintf("#%d", idx), func(t *testing.T) {
@@ -531,7 +525,7 @@ func TestDockerEnvInfoer(t *testing.T) {
531525
require.Equal(t, tt.expectedUserShell, sh, "Expected user shell to match")
532526

533527
environ := dei.Environ()
534-
require.Subset(t, environ, tt.expectedEnviron, "Expected environment to match")
528+
require.Subset(t, environ, tt.env, "Expected environment to match")
535529
})
536530
}
537531
}

0 commit comments

Comments
 (0)