diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index 1630bd855..7fec7ff94 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -157,7 +157,10 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm // as the wrapped command (it sidesteps the login shell's PATH re-derivation), // dropping to bare "docker" only when resolution failed. func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, dockerArgs []string, shellWrapped bool, dockerDir string) { - resolved, resErr := resolveDockerBinary(c.logger) + resolved, resErr := c.configuredDockerBinary() + if resolved == "" { + resolved, resErr = resolveDockerBinary(c.logger) + } switch { case resErr == nil && isDirectExecutable(resolved) && dockerDaemonEnvGuaranteed(): // INFO (not Debug): make the spawn decision observable in main.log so a @@ -393,6 +396,54 @@ func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile return append(shellArgs, "--cidfile", cidFile) } +// configuredDockerBinary returns the server's own command when it is a +// user-supplied `docker run` naming the docker CLI by an absolute or relative +// path to an executable, so that binary is spawned instead of whichever docker the +// resolver finds. "" means resolve as usual. +func (c *Client) configuredDockerBinary() (string, error) { + if c.config == nil || !isDockerRunCommand(c.config.Command, c.config.Args) { + return "", nil + } + command := c.config.Command + // A relative path is evaluated against working_dir, as exec does with + // Cmd.Dir (or the process directory without one). A bare name is a PATH + // lookup, which the resolver handles. + if !filepath.IsAbs(command) && strings.ContainsAny(command, `/\`) { + base := c.config.WorkingDir + if base == "" { + wd, err := os.Getwd() + if err != nil { + return "", nil + } + base = wd + } + abs, err := filepath.Abs(filepath.Join(base, command)) + if err != nil { + return "", nil + } + command = abs + } + if !isDirectExecutable(command) { + return "", nil + } + return command, nil +} + +// isDockerRunCommand reports whether a configured command is a user-supplied +// `docker run`. The command may be bare or a path in either separator style, +// and may carry the Windows .exe suffix (case-insensitive, as Windows lookup is). +func isDockerRunCommand(command string, args []string) bool { + if len(args) == 0 || args[0] != cmdRun { + return false + } + base := command + if i := strings.LastIndexAny(base, `/\`); i >= 0 { + base = base[i+1:] + } + base = strings.ToLower(base) + return base == cmdDocker || base == cmdDocker+".exe" +} + // extractContainerNameFromArgs extracts the container name from Docker run arguments func (c *Client) extractContainerNameFromArgs(dockerArgs []string) string { // Look for --name flag in the arguments diff --git a/internal/upstream/core/connection_launcher.go b/internal/upstream/core/connection_launcher.go index 53e6afc78..88e13fb32 100644 --- a/internal/upstream/core/connection_launcher.go +++ b/internal/upstream/core/connection_launcher.go @@ -58,7 +58,7 @@ func (c *Client) connectWithLauncher(ctx context.Context) error { // (matches connectStdio's behaviour at internal/upstream/core/ // connection_stdio.go:97). Otherwise concurrent reconnects could // race to create overlapping containers for the same upstream. - willUseDocker := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(c.config.Args) > 0 && c.config.Args[0] == cmdRun + willUseDocker := isDockerRunCommand(c.config.Command, c.config.Args) if !willUseDocker && c.isolationManager != nil { willUseDocker = c.isolationManager.ShouldIsolate(c.config) } @@ -292,7 +292,7 @@ func (c *Client) buildLauncherCmd(_ context.Context, willUseDocker bool) (*exec. finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile) } } - } else if isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun; isDirectDockerRun { + } else if isDirectDockerRun := isDockerRunCommand(c.config.Command, args); isDirectDockerRun { // USER-SUPPLIED `docker run …` upstream launched via the launcher path // (config.Command IS `docker`). Reuse the SAME resolve→spawn decision as the // isolation path and the stdio path (resolveDockerSpawn) so this entrypoint diff --git a/internal/upstream/core/connection_stdio.go b/internal/upstream/core/connection_stdio.go index e2642a193..91b895915 100644 --- a/internal/upstream/core/connection_stdio.go +++ b/internal/upstream/core/connection_stdio.go @@ -97,7 +97,7 @@ func (c *Client) connectStdio(ctx context.Context) error { var cidFile string // Check if this will be a Docker command (either explicit or through isolation) - willUseDocker := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun + willUseDocker := isDockerRunCommand(c.config.Command, args) if !willUseDocker && c.isolationManager != nil { willUseDocker = c.isolationManager.ShouldIsolate(c.config) } @@ -172,7 +172,7 @@ func (c *Client) connectStdio(ctx context.Context) error { finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile) } } - } else if isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun; isDirectDockerRun { + } else if isDirectDockerRun := isDockerRunCommand(c.config.Command, args); isDirectDockerRun { // USER-SUPPLIED `docker run …` upstream (config.Command IS `docker`). // Inject env vars as -e flags, then reuse the SAME resolve→spawn decision // as the isolation path (resolveDockerSpawn) so this path also direct-execs diff --git a/internal/upstream/core/docker_run_command_test.go b/internal/upstream/core/docker_run_command_test.go new file mode 100644 index 000000000..67c5faf01 --- /dev/null +++ b/internal/upstream/core/docker_run_command_test.go @@ -0,0 +1,112 @@ +package core + +import ( + "fmt" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +// A Windows config names the CLI docker.exe, often by its full backslash path. +// Missing it routes the server through the plain shell wrap, so config env is +// never injected as -e flags and the container is not tracked for cleanup. +func TestIsDockerRunCommand(t *testing.T) { + run := []string{"run", "--rm", "-i", "ghcr.io/some/image"} + cases := []struct { + command string + args []string + want bool + }{ + {"docker", run, true}, + {"/usr/local/bin/docker", run, true}, + {"docker.exe", run, true}, + {"DOCKER.EXE", run, true}, + {`C:\Program Files\Docker\Docker\resources\bin\docker.exe`, run, true}, + {`C:\Program Files\Docker\Docker\resources\bin\docker`, run, true}, + {"docker", []string{"ps"}, false}, + {"docker", nil, false}, + {"/usr/bin/docker-compose", run, false}, + {"dockerize", run, false}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, isDockerRunCommand(tc.command, tc.args), "%q %v", tc.command, tc.args) + } +} + +// A user-supplied `docker run` whose command is an absolute path to the docker +// CLI must spawn that binary, not whichever docker the resolver finds. Before +// this, C:\custom\docker.exe (or /opt/x/bin/docker) was replaced by the +// resolved or bare `docker`, failing when that one was absent from PATH. +func TestResolveDockerSpawnHonorsConfiguredAbsoluteDocker(t *testing.T) { + configured := writeFakeDockerExecutable(t) + other := writeFakeDockerExecutable(t) + + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + + for _, tc := range []struct { + name string + goos string + resolve func(*zap.Logger) (string, error) + wantWrap bool + }{ + {"direct exec, resolver finds another docker", osDarwin, func(*zap.Logger) (string, error) { return other, nil }, false}, + {"direct exec, resolver fails", osDarwin, func(*zap.Logger) (string, error) { return "", fmt.Errorf("docker not found") }, false}, + {"shell wrap keeps the configured binary", "linux", func(*zap.Logger) (string, error) { return other, nil }, true}, + } { + t.Run(tc.name, func(t *testing.T) { + forceDockerDaemonEnvGOOS(t, tc.goos) + t.Setenv("DOCKER_HOST", "") + t.Setenv("DOCKER_CONTEXT", "") + resolveDockerBinary = tc.resolve + c := newUserSuppliedDockerTestClient() + c.config.Command = configured + + cmd, args, shellWrapped, dir := c.resolveDockerSpawn(c.config.Args) + require.Equal(t, tc.wantWrap, shellWrapped) + require.Equal(t, filepath.Dir(configured), dir) + if shellWrapped { + require.Contains(t, args[len(args)-1], configured) + return + } + require.Equal(t, configured, cmd) + }) + } +} + +// A relative docker path (./bin/docker, .\bin\docker.exe) names a binary too, +// relative to the server's working_dir; it must not be swapped for whichever +// docker the resolver finds (Sol review round 2 of #1777). +func TestResolveDockerSpawnHonorsConfiguredRelativeDocker(t *testing.T) { + configured := writeFakeDockerExecutable(t) + other := writeFakeDockerExecutable(t) + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(*zap.Logger) (string, error) { return other, nil } + forceDockerDaemonEnvGOOS(t, osDarwin) + + c := newUserSuppliedDockerTestClient() + c.config.WorkingDir = filepath.Dir(filepath.Dir(configured)) + c.config.Command = "." + string(filepath.Separator) + filepath.Join(filepath.Base(filepath.Dir(configured)), filepath.Base(configured)) + + cmd, _, shellWrapped, _ := c.resolveDockerSpawn(c.config.Args) + require.False(t, shellWrapped) + require.Equal(t, configured, cmd) + + // A relative working_dir is itself relative to the process directory. + // Chdir next to it: on Windows the temp dir and the checkout can sit on + // different drives, where no relative path between them exists. + workDir := filepath.Dir(filepath.Dir(configured)) + t.Chdir(filepath.Dir(workDir)) + c.config.WorkingDir = filepath.Base(workDir) + cmd, _, _, _ = c.resolveDockerSpawn(c.config.Args) + require.Equal(t, configured, cmd) + + // A bare name is a PATH lookup, so the resolver still decides. + c.config.Command = "docker" + cmd, _, _, _ = c.resolveDockerSpawn(c.config.Args) + require.Equal(t, other, cmd) +}