Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

executor: optionally inject a docker registry prefixed url #48499

Closed
wants to merge 4 commits into from
Closed

executor: optionally inject a docker registry prefixed url #48499

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0996bf3
executor: optionally inject a docker registry prefixed url
danieldides Mar 1, 2023
ca0be3e
Merge branch 'main' into dd/inject-container-registy
danieldides Mar 1, 2023
a9313dc
Update enterprise/cmd/executor/internal/command/docker.go
danieldides Mar 2, 2023
cd14206
Add missing URI field
danieldides Mar 2, 2023
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
11 changes: 10 additions & 1 deletion enterprise/cmd/executor/internal/command/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 != "" {
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this work for non-docker hub images, and if the image is a full URL?

nodejs:16 // probably works
arm64/nodejs:16 // probably works as well
index.docker.io/nodejs:16 // does this work?
privateartifactory.sgdev.org/nodejs/nodejs:16 // does this work?

Also, I think for non-hub images we need to skip this mirror, as it cannot access the image.

Also, can the artifact registry handle/forward authentication? So for example if I use the image eseliger/private-nodejs:16 and configure a docker hub credential, will that just work? Otherwise, I think we'll have to bypass the mirror for auth'd requests, too even if it's docker hub. I think that is in line with how docker works when you actually configure a registry mirror.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah well, I've just seen the TODO comment above - so likely this needs tweaking before this is good to go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this work for non-docker hub images, and if the image is a full URL?

I've tried to resolve this with this pr: https://github.com/sourcegraph/sourcegraph/pull/48659. It's a best-effort. Since the official Docker spec does not allow subpaths in the registry URL (and I understand why now 😭) there's no easy way to truly determine how to parse a full URL container image name when we allow an arbitrary number of / in the subpath.

Also, can the artifact registry handle/forward authentication? So for example if I use the image eseliger/private-nodejs:16 and configure a docker hub credential

According to the Artifact Registry documents, at the moment: no

Docker: For public images in Docker Hub.

If you'd like to use private containers you are instructed to pull from the private repos and push them to Aritfact registry, or configure a virtual repository that prioritizes your private images first.

Ultimately, I'm not sure this feature is really ready to be used by a wider audience without a LOT of disclaimers that you can very easily "hold it wrong". The primary user for this will be Cloud, for which I've tried to cover our specific use-case as best I can.

}

return command{
Key: spec.Key,
Command: flatten(
Expand All @@ -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,
Expand Down
86 changes: 86 additions & 0 deletions enterprise/cmd/executor/internal/command/docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

}
5 changes: 5 additions & 0 deletions enterprise/cmd/executor/internal/command/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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://<location>-docker.pkg.dev./<project>/<repository>/<image>
// and therefore cannot be configured within the docker daemon
RegistryUrl string
}

type FirecrackerOptions struct {
Expand Down
18 changes: 18 additions & 0 deletions enterprise/cmd/executor/internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package config

import (
"encoding/json"
"net/url"
"runtime"
"strconv"
"strings"
"time"

"github.com/c2h5oh/datasize"
Expand Down Expand Up @@ -46,6 +48,7 @@ type Config struct {
DockerRegistryNodeExporterURL string
WorkerHostname string
DockerRegistryMirrorURL string
DockerRegistryPrefixURL string
DockerAuthConfig types.DockerAuthConfig
dockerAuthConfigStr string
dockerAuthConfigUnmarshalError error
Expand Down Expand Up @@ -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 != "" {
Expand Down Expand Up @@ -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"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice validation!

}

}

return c.BaseConfig.Validate()
}
1 change: 1 addition & 0 deletions enterprise/cmd/executor/internal/run/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down
17 changes: 15 additions & 2 deletions enterprise/cmd/executor/internal/worker/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand All @@ -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,
}

Expand Down