Route containerd ns mirror requests to configured registries - #405
pinguinfuss wants to merge 8 commits into
Conversation
containerd's hosts.toml mirrors append ?ns=<registry-host> to every
request. The container handler ignored it, so a single _default mirror
entry could not serve more than one registry.
ns is a closed-world lookup key and is never dialed. Docker Hub aliases
and the host of upstream.oci_default select the default route, hosts of
upstream.oci entries select their named upstream. Unknown hosts return
NAME_UNKNOWN so containerd falls back to its next host. Registry URLs
with a path are not indexed, because ns names the registry at the root
of a host. Host collisions log a warning and resolve deterministically.
With ns the path is the verbatim upstream repository, the reserved
upstream/ prefix is rejected, and repository prefix routes such as
Homebrew's do not apply. Cache names match the unprefixed and
upstream/{name}/ routes, so all routes share cached blobs, manifests and
tag lists. ns is no longer forwarded on tag-list requests, and the
pagination Link is rewritten per request instead of at store time so
clients on different routes get links for their own route.
Refs git-pkgs#303
Tag-list rows now store the upstream Link verbatim and rewrite it per request. Rows written by earlier versions hold a Link already rewritten for one route. Under the unchanged key an older binary running next to this one (rolling update on shared Postgres, or a rollback) would serve the raw Link unrewritten, sending clients to the wrong route for the next page. A format marker in the key keeps the two apart at the cost of one cache miss per tag list after the upgrade. Adversarial review finding F1: tag-list cache rows hold raw upstream Links under unchanged keys, so an older binary serves them unrewritten.
Registries whose URL has a path and hosts shared by two entries are not reachable through ns; point to the configuration guide for both rules. Adversarial review finding F3: README says ns covers every upstream.oci registry.
Configured registry URLs may carry userinfo credentials, which the new startup warnings wrote to the log verbatim. Log them with the password masked, and say precisely what a path-prefixed default registry means: its host is not indexed, while the Docker Hub aliases still select it. Adversarial review finding F2: startup warnings print registry URLs unredacted and the default-registry message is misleading.
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Please preserve non-default registry ports during namespace lookup and include the containerd configuration needed to enable the hosts directory.
| host, port = strings.TrimSuffix(strings.TrimPrefix(hostport, "["), "]"), "" | ||
| } | ||
| host = strings.ToLower(host) | ||
| if port != "" && port != "80" && port != "443" { |
There was a problem hiding this comment.
P2: Preserve ports that are non-default for the configured scheme. Both https://registry.example:80 and https://registry.example currently become the same lookup key, so requests for either can reach the wrong registry. Normalize only scheme-default ports and add coverage through the HTTP handler with both endpoints configured.
There was a problem hiding this comment.
Ports: only the scheme's default port is optional now; https://registry.example:80 and https://registry.example are separate keys, and the ns value is used as containerd sends it. There is a handler test with two registries on one host that differ only in the port.
| containerd mirrors send the original registry host in an `ns` query | ||
| parameter, so one mirror entry can serve Docker Hub and the registries | ||
| configured in `upstream.oci` whose URL has no path. Create | ||
| `/etc/containerd/certs.d/_default/hosts.toml`: |
There was a problem hiding this comment.
P2: Include the containerd config_path setup. Creating _default/hosts.toml alone does not enable mirroring when CRI's hosts directory is unset. Add the relevant plugin configuration and a verification command, following https://github.com/containerd/containerd/blob/main/docs/hosts.md#cri.
There was a problem hiding this comment.
Docs: added the config_path snippet for containerd 2.x and 1.x, and the check now goes through CRI (containerd config dump + crictl pull) instead of ctr --hosts-dir, which would have passed without config_path anyway.
Dropping 80 and 443 regardless of scheme turned https://host:80 and https://host into the same key, so a request for one of them could land on the other registry. Only the default port of the URL's scheme is optional now. A configured host is indexed with and without that port, because image references spell it either way; any other port has to match exactly, and the ns value is used as containerd sends it, apart from case and IPv6 brackets. Since a URL now yields two keys that can belong to different routes, the collision warning lists the owner per key. The handler test runs two registries on one host that differ only in the port. Tests can't bind 80 or 443, so a dialer maps those addresses to the fake servers.
containerd sends ns on every request to a mirror host, override_path or
not. Per-registry mirrors pointing at /v2/upstream/{name} therefore
arrive as upstream/{name}/...?ns=<registry>, and refusing every prefixed
name under ns broke them, including the only way to mirror an upstream
whose URL has a path.
Such requests are now handled like the prefix route without ns, as long
as ns names that upstream's own host under the same port rules as the
index. Any other ns on a prefixed name is still NAME_UNKNOWN, so ns
can't be used to create cache entries under another registry's name.
A _default/hosts.toml does nothing while CRI has no hosts directory configured, so add the config.toml snippet for containerd 2.x and 1.x. To check the setup, look at containerd config dump and pull with crictl, then find the request in the proxy log; ctr --hosts-dir reads the directory on its own and would succeed even without config_path. Also mention that existing per-registry override_path entries keep working and that k3s generates the directory itself.
|
Thanks for the review, both points are in. One thing I changed on top while testing this: containerd also sends |
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
The port handling and containerd setup findings are addressed. Please also preserve Docker Hub aliases in the new prefix-route namespace check so existing per-registry mirrors keep working.
| if err != nil || parsed.Host == "" { | ||
| return false | ||
| } | ||
| return slices.Contains(namespaceKeysForHost(parsed), namespaceKeyForRequest(namespace)) |
There was a problem hiding this comment.
P2: Accept equivalent Docker Hub hosts in the prefix-route check. With upstream.oci.hub: https://registry-1.docker.io and a containerd docker.io/hosts.toml mirror pointing at /v2/upstream/hub with override_path = true, containerd sends ns=docker.io. This check only accepts registry-1.docker.io, so the request now returns 404 NAME_UNKNOWN and the client either bypasses the proxy through fallback or fails the pull. A targeted HTTP-handler test confirmed /v2/upstream/hub/library/nginx/manifests/latest returns 200 without ns and with ns=registry-1.docker.io, but 404 with ns=docker.io. Please recognize the Docker Hub aliases here while preserving the port checks, and add regression coverage through the HTTP handler.
2 participants
This teaches the
/v2handler thensquery parameter that containerd appends to mirror requests, so a single_defaulthosts.toml (or k3smirrors: "*") can front Docker Hub plus everything inupstream.oci.The idea is simple:
nsis only ever looked up, never dialed. The Docker Hub aliases and the host ofoci_defaultmap to the default registry, the host of eachupstream.ociURL maps to that upstream, and anything else gets a404 NAME_UNKNOWNso containerd moves on to its next host. Requests withoutnsare untouched. Hosts are compared case-insensitively with ports 80/443 dropped, using the same function on both sides. Withnsthe path is the verbatim upstream repository,upstream/...is rejected, and the cache names line up with the existing routes – pulling an image via ns, viaupstream/{name}/or unprefixed shares blobs, manifests and tag lists.A few decisions worth a look:
ns.ns=art.corpmeans the registry at the root of that host; mapping it onto something likehttps://art.corp/artifactory/api/docker/remotewould silently routeart.corp/docker-local/appinto a different repository. Those upstreams still work viaupstream/{name}/, and the proxy warns at startup.ns=docker.ioskips the Homebrew prefix route on purpose – the client asked for Docker Hub, so it gets Docker Hub. Withoutnsnothing changes there.Linkheader used to be rewritten when the entry was stored. Now that routes share the entry it is rewritten per request, and the cache key got a format marker so an older binary (rolling update, rollback) never sees the raw link. Costs one extra cache miss per tag list after upgrading.Tests live in
container_ns_test.goand cover the cases from the issue plus host normalization (IPv6, ports, case), the path exclusion and the legacy tag-list rows. I have only run this against the fake registries in the test suite so far, not against a real containerd node. README anddocs/configuration.mdgot a containerd section with a_defaulthosts.toml example.Closes #303