executor: optionally inject a docker registry prefixed url #48499
Conversation
| // and create an invalid docker name | ||
| image := spec.Image | ||
| if options.DockerOptions.RegistryUrl != "" { | ||
| image = fmt.Sprintf("%s/%s", options.DockerOptions.RegistryUrl, image) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah well, I've just seen the TODO comment above - so likely this needs tweaking before this is good to go?
There was a problem hiding this comment.
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.
| 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")) |
Co-authored-by: Erik Seliger <erikseliger@me.com>
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 8010604...cd14206.
|
|
@danieldides would you like additional reviews or is this ready to be merged? |
|
Closing for now. Not sure this is the best path forward anymore. May revisit in the future as part of https://github.com/sourcegraph/customer/issues/2083 |
Part of https://github.com/sourcegraph/customer/issues/1872
Allow an new, optional configuration passed to the executor to configure a Docker registry that should be prefixed on all
dockercommands. E.g., turnubuntu:latestintous-central1.pkg.dev/project/repo/ubuntu:latest. This will allow us to utilize Artifact Registry for executors.Current solutions do not work because the Docker daemon does not support registry URLs with subpaths included in it.
This depends on sourcegraph/src-cli#945 for SSBC. I've tried my best to catch all other Docker commands but I'm not 100% sure I caught everything. Autoindexing in particular evades me a little.
Test plan
Ran it locally. Some basic unit tests. Cannot test Firecracker injection until a container is built and deployed.