Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion internal/upstream/core/connection_docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions internal/upstream/core/connection_launcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions internal/upstream/core/connection_stdio.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down
112 changes: 112 additions & 0 deletions internal/upstream/core/docker_run_command_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
Loading