From 3fb5d0b52dac52b47b51ebb98119d0fed8997e27 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 24 Jun 2025 12:56:54 +0300 Subject: [PATCH] fix(agent/agentcontainers): use correct env for execer commands (#18508) --- agent/agentcontainers/api.go | 34 +++++++++- agent/agentcontainers/api_test.go | 86 ++++++++++++++++++++++++ agent/agentcontainers/devcontainercli.go | 2 - agent/agentcontainers/execer.go | 77 +++++++++++++++++++++ agent/agentssh/agentssh.go | 77 ++++++++++++--------- agent/api.go | 1 + cli/exp_rpty_test.go | 9 ++- 7 files changed, 251 insertions(+), 35 deletions(-) create mode 100644 agent/agentcontainers/execer.go diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 8896c32175..3fc4498056 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -24,6 +24,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/agent/agentcontainers/watcher" "github.com/coder/coder/v2/agent/agentexec" + "github.com/coder/coder/v2/agent/usershell" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/agentsdk" @@ -57,6 +58,7 @@ type API struct { logger slog.Logger watcher watcher.Watcher execer agentexec.Execer + commandEnv CommandEnv ccli ContainerCLI containerLabelIncludeFilter map[string]string // Labels to filter containers by. dccli DevcontainerCLI @@ -109,6 +111,29 @@ func WithExecer(execer agentexec.Execer) Option { } } +// WithCommandEnv sets the CommandEnv implementation to use. +func WithCommandEnv(ce CommandEnv) Option { + return func(api *API) { + api.commandEnv = func(ei usershell.EnvInfoer, preEnv []string) (string, string, []string, error) { + shell, dir, env, err := ce(ei, preEnv) + if err != nil { + return shell, dir, env, err + } + env = slices.DeleteFunc(env, func(s string) bool { + // Ensure we filter out environment variables that come + // from the parent agent and are incorrect or not + // relevant for the devcontainer. + return strings.HasPrefix(s, "CODER_WORKSPACE_AGENT_NAME=") || + strings.HasPrefix(s, "CODER_WORKSPACE_AGENT_URL=") || + strings.HasPrefix(s, "CODER_AGENT_TOKEN=") || + strings.HasPrefix(s, "CODER_AGENT_AUTH=") || + strings.HasPrefix(s, "CODER_AGENT_DEVCONTAINERS_ENABLE=") + }) + return shell, dir, env, nil + } + } +} + // WithContainerCLI sets the agentcontainers.ContainerCLI implementation // to use. The default implementation uses the Docker CLI. func WithContainerCLI(ccli ContainerCLI) Option { @@ -151,7 +176,7 @@ func WithSubAgentURL(url string) Option { } } -// WithSubAgent sets the environment variables for the sub-agent. +// WithSubAgentEnv sets the environment variables for the sub-agent. func WithSubAgentEnv(env ...string) Option { return func(api *API) { api.subAgentEnv = env @@ -259,6 +284,13 @@ func NewAPI(logger slog.Logger, options ...Option) *API { for _, opt := range options { opt(api) } + if api.commandEnv != nil { + api.execer = newCommandEnvExecer( + api.logger, + api.commandEnv, + api.execer, + ) + } if api.ccli == nil { api.ccli = NewDockerCLI(api.execer) } diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index a59a3bfd67..a5541399f6 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "os" + "os/exec" "runtime" "strings" "sync" @@ -26,7 +27,9 @@ import ( "github.com/coder/coder/v2/agent/agentcontainers" "github.com/coder/coder/v2/agent/agentcontainers/acmock" "github.com/coder/coder/v2/agent/agentcontainers/watcher" + "github.com/coder/coder/v2/agent/usershell" "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/pty" "github.com/coder/coder/v2/testutil" "github.com/coder/quartz" ) @@ -291,6 +294,38 @@ func (m *fakeSubAgentClient) Delete(ctx context.Context, id uuid.UUID) error { return nil } +// fakeExecer implements agentexec.Execer for testing and tracks execution details. +type fakeExecer struct { + commands [][]string + createdCommands []*exec.Cmd +} + +func (f *fakeExecer) CommandContext(ctx context.Context, cmd string, args ...string) *exec.Cmd { + f.commands = append(f.commands, append([]string{cmd}, args...)) + // Create a command that returns empty JSON for docker commands. + c := exec.CommandContext(ctx, "echo", "[]") + f.createdCommands = append(f.createdCommands, c) + return c +} + +func (f *fakeExecer) PTYCommandContext(ctx context.Context, cmd string, args ...string) *pty.Cmd { + f.commands = append(f.commands, append([]string{cmd}, args...)) + return &pty.Cmd{ + Context: ctx, + Path: cmd, + Args: append([]string{cmd}, args...), + Env: []string{}, + Dir: "", + } +} + +func (f *fakeExecer) getLastCommand() *exec.Cmd { + if len(f.createdCommands) == 0 { + return nil + } + return f.createdCommands[len(f.createdCommands)-1] +} + func TestAPI(t *testing.T) { t.Parallel() @@ -1970,6 +2005,57 @@ func TestAPI(t *testing.T) { // Then: We expected it to succeed require.Len(t, fSAC.created, 1) }) + + t.Run("CommandEnv", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + // Create fake execer to track execution details. + fakeExec := &fakeExecer{} + + // Custom CommandEnv that returns specific values. + testShell := "/bin/custom-shell" + testDir := t.TempDir() + testEnv := []string{"CUSTOM_VAR=test_value", "PATH=/custom/path"} + + commandEnv := func(ei usershell.EnvInfoer, addEnv []string) (shell, dir string, env []string, err error) { + return testShell, testDir, testEnv, nil + } + + mClock := quartz.NewMock(t) // Stop time. + + // Create API with CommandEnv. + api := agentcontainers.NewAPI(logger, + agentcontainers.WithClock(mClock), + agentcontainers.WithExecer(fakeExec), + agentcontainers.WithCommandEnv(commandEnv), + ) + defer api.Close() + + // Call RefreshContainers directly to trigger CommandEnv usage. + _ = api.RefreshContainers(ctx) // Ignore error since docker commands will fail. + + // Verify commands were executed through the custom shell and environment. + require.NotEmpty(t, fakeExec.commands, "commands should be executed") + + // Want: /bin/custom-shell -c "docker ps --all --quiet --no-trunc" + require.Equal(t, testShell, fakeExec.commands[0][0], "custom shell should be used") + if runtime.GOOS == "windows" { + require.Equal(t, "/c", fakeExec.commands[0][1], "shell should be called with /c on Windows") + } else { + require.Equal(t, "-c", fakeExec.commands[0][1], "shell should be called with -c") + } + require.Len(t, fakeExec.commands[0], 3, "command should have 3 arguments") + require.GreaterOrEqual(t, strings.Count(fakeExec.commands[0][2], " "), 2, "command/script should have multiple arguments") + + // Verify the environment was set on the command. + lastCmd := fakeExec.getLastCommand() + require.NotNil(t, lastCmd, "command should be created") + require.Equal(t, testDir, lastCmd.Dir, "custom directory should be used") + require.Equal(t, testEnv, lastCmd.Env, "custom environment should be used") + }) } // mustFindDevcontainerByPath returns the devcontainer with the given workspace diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index e87c3362c6..850dc84d5a 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -7,7 +7,6 @@ import ( "encoding/json" "errors" "io" - "os" "golang.org/x/xerrors" @@ -280,7 +279,6 @@ func (d *devcontainerCLI) ReadConfig(ctx context.Context, workspaceFolder, confi } c := d.execer.CommandContext(ctx, "devcontainer", args...) - c.Env = append(c.Env, "PATH="+os.Getenv("PATH")) c.Env = append(c.Env, env...) var stdoutBuf bytes.Buffer diff --git a/agent/agentcontainers/execer.go b/agent/agentcontainers/execer.go new file mode 100644 index 0000000000..8bdb7f2439 --- /dev/null +++ b/agent/agentcontainers/execer.go @@ -0,0 +1,77 @@ +package agentcontainers + +import ( + "context" + "os/exec" + "runtime" + + "github.com/kballard/go-shellquote" + + "cdr.dev/slog" + "github.com/coder/coder/v2/agent/agentexec" + "github.com/coder/coder/v2/agent/usershell" + "github.com/coder/coder/v2/pty" +) + +// CommandEnv is a function that returns the shell, working directory, +// and environment variables to use when executing a command. It takes +// an EnvInfoer and a pre-existing environment slice as arguments. +// This signature matches agentssh.Server.CommandEnv. +type CommandEnv func(ei usershell.EnvInfoer, addEnv []string) (shell, dir string, env []string, err error) + +// commandEnvExecer is an agentexec.Execer that uses a CommandEnv to +// determine the shell, working directory, and environment variables +// for commands. It wraps another agentexec.Execer to provide the +// necessary context. +type commandEnvExecer struct { + logger slog.Logger + commandEnv CommandEnv + execer agentexec.Execer +} + +func newCommandEnvExecer( + logger slog.Logger, + commandEnv CommandEnv, + execer agentexec.Execer, +) *commandEnvExecer { + return &commandEnvExecer{ + logger: logger, + commandEnv: commandEnv, + execer: execer, + } +} + +// Ensure commandEnvExecer implements agentexec.Execer. +var _ agentexec.Execer = (*commandEnvExecer)(nil) + +func (e *commandEnvExecer) prepare(ctx context.Context, inName string, inArgs ...string) (name string, args []string, dir string, env []string) { + shell, dir, env, err := e.commandEnv(nil, nil) + if err != nil { + e.logger.Error(ctx, "get command environment failed", slog.Error(err)) + return inName, inArgs, "", nil + } + + caller := "-c" + if runtime.GOOS == "windows" { + caller = "/c" + } + name = shell + args = []string{caller, shellquote.Join(append([]string{inName}, inArgs...)...)} + return name, args, dir, env +} + +func (e *commandEnvExecer) CommandContext(ctx context.Context, cmd string, args ...string) *exec.Cmd { + name, args, dir, env := e.prepare(ctx, cmd, args...) + c := e.execer.CommandContext(ctx, name, args...) + c.Dir = dir + c.Env = env + return c +} + +func (e *commandEnvExecer) PTYCommandContext(ctx context.Context, cmd string, args ...string) *pty.Cmd { + name, args, dir, env := e.prepare(ctx, cmd, args...) + c := e.execer.PTYCommandContext(ctx, name, args...) + c.Dir = dir + c.Env = env + return c +} diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index a2fec79deb..fb4cc055e3 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -816,6 +816,49 @@ func (s *Server) sftpHandler(logger slog.Logger, session ssh.Session) error { return xerrors.Errorf("sftp server closed with error: %w", err) } +func (s *Server) CommandEnv(ei usershell.EnvInfoer, addEnv []string) (shell, dir string, env []string, err error) { + if ei == nil { + ei = &usershell.SystemEnvInfo{} + } + + currentUser, err := ei.User() + if err != nil { + return "", "", nil, xerrors.Errorf("get current user: %w", err) + } + username := currentUser.Username + + shell, err = ei.Shell(username) + if err != nil { + return "", "", nil, xerrors.Errorf("get user shell: %w", err) + } + + dir = s.config.WorkingDirectory() + + // If the metadata directory doesn't exist, we run the command + // in the users home directory. + _, err = os.Stat(dir) + if dir == "" || err != nil { + // Default to user home if a directory is not set. + homedir, err := ei.HomeDir() + if err != nil { + return "", "", nil, xerrors.Errorf("get home dir: %w", err) + } + dir = homedir + } + env = append(ei.Environ(), addEnv...) + // Set login variables (see `man login`). + env = append(env, fmt.Sprintf("USER=%s", username)) + env = append(env, fmt.Sprintf("LOGNAME=%s", username)) + env = append(env, fmt.Sprintf("SHELL=%s", shell)) + + env, err = s.config.UpdateEnv(env) + if err != nil { + return "", "", nil, xerrors.Errorf("apply env: %w", err) + } + + return shell, dir, env, nil +} + // 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. @@ -827,15 +870,10 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string, if ei == nil { ei = &usershell.SystemEnvInfo{} } - currentUser, err := ei.User() - if err != nil { - return nil, xerrors.Errorf("get current user: %w", err) - } - username := currentUser.Username - shell, err := ei.Shell(username) + shell, dir, env, err := s.CommandEnv(ei, env) if err != nil { - return nil, xerrors.Errorf("get user shell: %w", err) + return nil, xerrors.Errorf("prepare command env: %w", err) } // OpenSSH executes all commands with the users current shell. @@ -893,24 +931,8 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string, ) } 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 := ei.HomeDir() - if err != nil { - return nil, xerrors.Errorf("get home dir: %w", err) - } - cmd.Dir = homedir - } - cmd.Env = append(ei.Environ(), env...) - // Set login variables (see `man login`). - cmd.Env = append(cmd.Env, fmt.Sprintf("USER=%s", username)) - cmd.Env = append(cmd.Env, fmt.Sprintf("LOGNAME=%s", username)) - cmd.Env = append(cmd.Env, fmt.Sprintf("SHELL=%s", shell)) + cmd.Dir = dir + cmd.Env = env // Set SSH connection environment variables (these are also set by OpenSSH // and thus expected to be present by SSH clients). Since the agent does @@ -921,11 +943,6 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string, cmd.Env = append(cmd.Env, fmt.Sprintf("SSH_CLIENT=%s %s %s", srcAddr, srcPort, dstPort)) cmd.Env = append(cmd.Env, fmt.Sprintf("SSH_CONNECTION=%s %s %s %s", srcAddr, srcPort, dstAddr, dstPort)) - cmd.Env, err = s.config.UpdateEnv(cmd.Env) - if err != nil { - return nil, xerrors.Errorf("apply env: %w", err) - } - return cmd, nil } diff --git a/agent/api.go b/agent/api.go index 464c5fc5da..fa761988b2 100644 --- a/agent/api.go +++ b/agent/api.go @@ -43,6 +43,7 @@ func (a *agent) apiHandler(aAPI proto.DRPCAgentClient26) (http.Handler, func() e if a.experimentalDevcontainersEnabled { containerAPIOpts := []agentcontainers.Option{ agentcontainers.WithExecer(a.execer), + agentcontainers.WithCommandEnv(a.sshServer.CommandEnv), agentcontainers.WithScriptLogger(func(logSourceID uuid.UUID) agentcontainers.ScriptLogger { return a.logSender.GetScriptLogger(logSourceID) }), diff --git a/cli/exp_rpty_test.go b/cli/exp_rpty_test.go index 923bf09bb0..04c8798c16 100644 --- a/cli/exp_rpty_test.go +++ b/cli/exp_rpty_test.go @@ -87,6 +87,8 @@ func TestExpRpty(t *testing.T) { t.Skip("Skipping test on non-Linux platform") } + wantLabel := "coder.devcontainers.TestExpRpty.Container" + client, workspace, agentToken := setupWorkspaceForAgent(t) ctx := testutil.Context(t, testutil.WaitLong) pool, err := dockertest.NewPool("") @@ -94,7 +96,10 @@ func TestExpRpty(t *testing.T) { ct, err := pool.RunWithOptions(&dockertest.RunOptions{ Repository: "busybox", Tag: "latest", - Cmd: []string{"sleep", "infnity"}, + Cmd: []string{"sleep", "infinity"}, + Labels: map[string]string{ + wantLabel: "true", + }, }, func(config *docker.HostConfig) { config.AutoRemove = true config.RestartPolicy = docker.RestartPolicy{Name: "no"} @@ -113,7 +118,7 @@ func TestExpRpty(t *testing.T) { _ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) { o.ExperimentalDevcontainersEnabled = true o.ContainerAPIOptions = append(o.ContainerAPIOptions, - agentcontainers.WithContainerLabelIncludeFilter("this.label.does.not.exist.ignore.devcontainers", "true"), + agentcontainers.WithContainerLabelIncludeFilter(wantLabel, "true"), ) }) _ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()