diff --git a/enterprise/cmd/executor/internal/command/docker.go b/enterprise/cmd/executor/internal/command/docker.go index f391ca44817c..63601db2fa7f 100644 --- a/enterprise/cmd/executor/internal/command/docker.go +++ b/enterprise/cmd/executor/internal/command/docker.go @@ -17,6 +17,8 @@ const ScriptsPath = ".sourcegraph-executor" // given options. func formatRawOrDockerCommand(spec CommandSpec, dir string, options Options, dockerConfigPath string) command { // TODO - remove this once src-cli is not required anymore for SSBC. + // Note: unless the `native-ssbc-execution` feature-flag is enabled, this + // is the default execution method for server-side batch changes. if spec.Image == "" { env := spec.Env if dockerConfigPath != "" { @@ -36,6 +38,13 @@ func formatRawOrDockerCommand(spec CommandSpec, dir string, options Options, doc hostDir = filepath.Join(options.ResourceOptions.DockerHostMountPath, filepath.Base(dir)) } + // TODO check that the original spec.Image isn't fully qualified so we don't inject our path + // and create an invalid docker name + image := spec.Image + if options.DockerOptions.RegistryUrl != "" { + image = fmt.Sprintf("%s/%s", options.DockerOptions.RegistryUrl, image) + } + return command{ Key: spec.Key, Command: flatten( @@ -48,7 +57,7 @@ func formatRawOrDockerCommand(spec CommandSpec, dir string, options Options, doc dockerWorkingdirectoryFlags(spec.Dir), dockerEnvFlags(spec.Env), dockerEntrypointFlags(), - spec.Image, + image, filepath.Join("/data", ScriptsPath, spec.ScriptPath), ), Operation: spec.Operation, diff --git a/enterprise/cmd/executor/internal/command/docker_test.go b/enterprise/cmd/executor/internal/command/docker_test.go index 39370593fbc0..f20bffc5bccd 100644 --- a/enterprise/cmd/executor/internal/command/docker_test.go +++ b/enterprise/cmd/executor/internal/command/docker_test.go @@ -226,3 +226,89 @@ func TestFormatRawOrDockerCommandDockerScriptWithDockerHostMountPath(t *testing. t.Errorf("unexpected command (-want +got):\n%s", diff) } } + +func TestFormatRawOrDockerCommandDockerScriptWithRegistryPrefix(t *testing.T) { + actual := formatRawOrDockerCommand( + CommandSpec{ + Image: "alpine:latest", + ScriptPath: "myscript.sh", + Dir: "subdir", + Operation: makeTestOperation(), + }, + "/proj/src", + Options{ + ResourceOptions: ResourceOptions{ + NumCPUs: 4, + Memory: "20G", + }, + DockerOptions: DockerOptions{ + RegistryUrl: "us-central1.pkg.dev/project/repo", + }, + }, + "/tmp/docker-config", + ) + + expected := command{ + Command: []string{ + "docker", + "--config", "/tmp/docker-config", + "run", "--rm", + "--cpus", "4", + "--memory", "20G", + "-v", "/proj/src:/data", + "-w", "/data/subdir", + "--entrypoint", + "/bin/sh", + "us-central1.pkg.dev/project/repo/alpine:latest", + "/data/.sourcegraph-executor/myscript.sh", + }, + } + + if diff := cmp.Diff(expected, actual, commandComparer); diff != "" { + t.Errorf("unexpected command (-want +got):\n%s", diff) + } + +} + +func TestFormatRawOrDockerCommandDockerScriptWithRegistryPrefixAndRepo(t *testing.T) { + actual := formatRawOrDockerCommand( + CommandSpec{ + Image: "sourcegraph/executor:insiders", + ScriptPath: "myscript.sh", + Dir: "subdir", + Operation: makeTestOperation(), + }, + "/proj/src", + Options{ + ResourceOptions: ResourceOptions{ + NumCPUs: 4, + Memory: "20G", + }, + DockerOptions: DockerOptions{ + RegistryUrl: "us-central1.pkg.dev/project/repo", + }, + }, + "/tmp/docker-config", + ) + + expected := command{ + Command: []string{ + "docker", + "--config", "/tmp/docker-config", + "run", "--rm", + "--cpus", "4", + "--memory", "20G", + "-v", "/proj/src:/data", + "-w", "/data/subdir", + "--entrypoint", + "/bin/sh", + "us-central1.pkg.dev/project/repo/sourcegraph/executor:insiders", + "/data/.sourcegraph-executor/myscript.sh", + }, + } + + if diff := cmp.Diff(expected, actual, commandComparer); diff != "" { + t.Errorf("unexpected command (-want +got):\n%s", diff) + } + +} diff --git a/enterprise/cmd/executor/internal/command/runner.go b/enterprise/cmd/executor/internal/command/runner.go index fd6a64b01a2f..db3e56c94f38 100644 --- a/enterprise/cmd/executor/internal/command/runner.go +++ b/enterprise/cmd/executor/internal/command/runner.go @@ -64,6 +64,11 @@ type DockerOptions struct { // container. This can be useful to add host.docker.internal as an endpoint inside // the container. AddHostGateway bool + // RegistryUrl, if set, will be injected before the image name to fully-qualify the + // image name for situations where registries with subpaths are used, e.g. Artifact Registry + // where the registry is https://-docker.pkg.dev./// + // and therefore cannot be configured within the docker daemon + RegistryUrl string } type FirecrackerOptions struct { diff --git a/enterprise/cmd/executor/internal/config/config.go b/enterprise/cmd/executor/internal/config/config.go index e956f99b0853..f703c4c81096 100644 --- a/enterprise/cmd/executor/internal/config/config.go +++ b/enterprise/cmd/executor/internal/config/config.go @@ -2,8 +2,10 @@ package config import ( "encoding/json" + "net/url" "runtime" "strconv" + "strings" "time" "github.com/c2h5oh/datasize" @@ -46,6 +48,7 @@ type Config struct { DockerRegistryNodeExporterURL string WorkerHostname string DockerRegistryMirrorURL string + DockerRegistryPrefixURL string DockerAuthConfig types.DockerAuthConfig dockerAuthConfigStr string dockerAuthConfigUnmarshalError error @@ -82,6 +85,7 @@ func (c *Config) Load() { c.DockerRegistryNodeExporterURL = c.GetOptional("DOCKER_REGISTRY_NODE_EXPORTER_URL", "The URL of the Docker Registry instance's node_exporter, without the /metrics path.") c.MaxActiveTime = c.GetInterval("EXECUTOR_MAX_ACTIVE_TIME", "0", "The maximum time that can be spent by the worker dequeueing records to be handled.") c.DockerRegistryMirrorURL = c.GetOptional("EXECUTOR_DOCKER_REGISTRY_MIRROR_URL", "The address of a docker registry mirror to use in firecracker VMs. Supports multiple values, separated with a comma.") + c.DockerRegistryPrefixURL = c.GetOptional("EXECUTOR_DOCKER_REGISTRY_PREFIX_URL", "The address of a docker reigsry mirror to use in firecracker VMs that will be injected before all container names to fully qualify their URL. Supports only a single registry.") c.dockerAuthConfigStr = c.GetOptional("EXECUTOR_DOCKER_AUTH_CONFIG", "The content of the docker config file including auth for services. If using firecracker, only static credentials are supported, not credential stores nor credential helpers.") if c.dockerAuthConfigStr != "" { @@ -123,5 +127,19 @@ func (c *Config) Validate() error { } } + // Verify that no Docker registry URLs contain a subpath that will crash the Docker daemon startup + // https://github.com/docker/engine/blob/8955d8da8951695a98eb7e15bead19d402c6eb27/registry/config.go#L303-L321 + registries := strings.Split(c.DockerRegistryMirrorURL, ",") + for _, registry := range registries { + uri, err := url.Parse(registry) + if err != nil { + c.AddError(errors.Newf("invalid registry URL %s provided", uri)) + } + if uri.Path != "" { + c.AddError(errors.Newf("registry URL %s contains a subpath, consider using EXECUTOR_DOCKER_REGISTRY_PREFIX_URL instead", uri)) + } + + } + return c.BaseConfig.Validate() } diff --git a/enterprise/cmd/executor/internal/run/util.go b/enterprise/cmd/executor/internal/run/util.go index d696f8b0744c..8b9fffd7eb3d 100644 --- a/enterprise/cmd/executor/internal/run/util.go +++ b/enterprise/cmd/executor/internal/run/util.go @@ -141,6 +141,7 @@ func dockerOptions(c *config.Config) command.DockerOptions { // host entry and route to it to the containers. This is used for LSIF // uploads and should not be required anymore once we support native uploads. AddHostGateway: u.Hostname() == "host.docker.internal", + RegistryUrl: c.DockerRegistryPrefixURL, } } diff --git a/enterprise/cmd/executor/internal/worker/handler.go b/enterprise/cmd/executor/internal/worker/handler.go index d9efeb1a13da..a03c6e506ecf 100644 --- a/enterprise/cmd/executor/internal/worker/handler.go +++ b/enterprise/cmd/executor/internal/worker/handler.go @@ -143,8 +143,16 @@ func (h *handler) Handle(ctx context.Context, logger log.Logger, job types.Job) } }() + // TODO check that the original spec.Image isn't fully qualified so we don't inject our path + // and create an invalid docker name + // Invoke each docker step sequentially for i, dockerStep := range job.DockerSteps { + image := dockerStep.Image + if options.DockerOptions.RegistryUrl != "" { + image = fmt.Sprintf("%s/%s", options.DockerOptions.RegistryUrl, image) + } + var key string if dockerStep.Key != "" { key = fmt.Sprintf("step.docker.%s", dockerStep.Key) @@ -153,7 +161,7 @@ func (h *handler) Handle(ctx context.Context, logger log.Logger, job types.Job) } dockerStepCommand := command.CommandSpec{ Key: key, - Image: dockerStep.Image, + Image: image, ScriptPath: ws.ScriptFilenames()[i], Dir: dockerStep.Dir, Env: dockerStep.Env, @@ -176,11 +184,16 @@ func (h *handler) Handle(ctx context.Context, logger log.Logger, job types.Job) key = fmt.Sprintf("step.src.%d", i) } + env := cliStep.Env + if options.DockerOptions.RegistryUrl != "" { + env = append(env, fmt.Sprintf("DOCKER_PREFIX_REGISTRY_URL=%s", options.DockerOptions.RegistryUrl)) + } + cliStepCommand := command.CommandSpec{ Key: key, Command: append([]string{"src"}, cliStep.Commands...), Dir: cliStep.Dir, - Env: cliStep.Env, + Env: env, Operation: h.operations.Exec, }