From 0996bf379326c3543d9b60a3118f669ded194140 Mon Sep 17 00:00:00 2001 From: Daniel Dides Date: Wed, 1 Mar 2023 14:55:21 -0600 Subject: [PATCH 1/3] executor: optionally inject a docker registry prefixed url --- .../cmd/executor/internal/command/docker.go | 11 ++- .../executor/internal/command/docker_test.go | 86 +++++++++++++++++++ .../cmd/executor/internal/command/runner.go | 5 ++ .../cmd/executor/internal/config/config.go | 18 ++++ enterprise/cmd/executor/internal/run/util.go | 1 + .../cmd/executor/internal/worker/handler.go | 17 +++- 6 files changed, 135 insertions(+), 3 deletions(-) diff --git a/enterprise/cmd/executor/internal/command/docker.go b/enterprise/cmd/executor/internal/command/docker.go index f391ca44817c..bd8615602ff1 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 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 ad8500ae9f96..75d169b0965a 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 22b0e3716df3..646be64043b1 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 executor.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")) + } + + } + return c.BaseConfig.Validate() } diff --git a/enterprise/cmd/executor/internal/run/util.go b/enterprise/cmd/executor/internal/run/util.go index 27261a5f2637..4ff2c44e3934 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 cf83ca83181f..39eeb1b45ce6 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 executor.Jo } }() + // 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 executor.Jo } 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 executor.Jo 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, } From a9313dcb865c3173af89c240d30c8affed8f8070 Mon Sep 17 00:00:00 2001 From: Daniel Dides <8784265+danieldides@users.noreply.github.com> Date: Thu, 2 Mar 2023 09:15:33 -0600 Subject: [PATCH 2/3] Update enterprise/cmd/executor/internal/command/docker.go Co-authored-by: Erik Seliger --- enterprise/cmd/executor/internal/command/docker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/cmd/executor/internal/command/docker.go b/enterprise/cmd/executor/internal/command/docker.go index bd8615602ff1..63601db2fa7f 100644 --- a/enterprise/cmd/executor/internal/command/docker.go +++ b/enterprise/cmd/executor/internal/command/docker.go @@ -18,7 +18,7 @@ const ScriptsPath = ".sourcegraph-executor" 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 + // is the default execution method for server-side batch changes. if spec.Image == "" { env := spec.Env if dockerConfigPath != "" { From cd14206ecba18d6d95894cab33171b745b9f1641 Mon Sep 17 00:00:00 2001 From: Daniel Dides Date: Thu, 2 Mar 2023 17:01:42 -0600 Subject: [PATCH 3/3] Add missing URI field --- enterprise/cmd/executor/internal/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/cmd/executor/internal/config/config.go b/enterprise/cmd/executor/internal/config/config.go index 48672aa7716c..f703c4c81096 100644 --- a/enterprise/cmd/executor/internal/config/config.go +++ b/enterprise/cmd/executor/internal/config/config.go @@ -136,7 +136,7 @@ func (c *Config) Validate() error { 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")) + c.AddError(errors.Newf("registry URL %s contains a subpath, consider using EXECUTOR_DOCKER_REGISTRY_PREFIX_URL instead", uri)) } }