containerd: set override_path for registry mirrors with a path - #18819
kubernetes-prow[bot] merged 2 commits into
Conversation
|
|
|
Welcome @peter-svensson! |
|
Hi @peter-svensson. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
hakman
left a comment
There was a problem hiding this comment.
Thanks for the fix @peter-svensson. I would suggest some small fixes and adding a test for endpointHasPath().
func TestEndpointHasPath(t *testing.T) {
grid := []struct {
endpoint string
expected bool
}{
{endpoint: "https://mirror.example.com", expected: false},
{endpoint: "https://mirror.example.com/", expected: false},
{endpoint: "https://mirror.example.com//", expected: false},
{endpoint: "http://10.0.0.5:5000", expected: false},
{endpoint: "mirror.example.com", expected: false},
{endpoint: "mirror.example.com:5000", expected: false},
{endpoint: "10.0.0.5", expected: false},
{endpoint: "https://mirror.example.com/v2", expected: true},
{endpoint: "https://123456789012.dkr.ecr.us-east-1.amazonaws.com/v2/docker-hub", expected: true},
{endpoint: "mirror.example.com:5000/v2/docker-hub", expected: true},
{endpoint: "10.0.0.5:5000/v2/docker-hub", expected: true},
{endpoint: "https://mirror.example.com/./", expected: false},
{endpoint: "https://mirror.example.com/v2/..", expected: false},
{endpoint: "https://mirror.example.com/prefix/./v2", expected: true},
{endpoint: "https://mirror.example.com/v2/docker-hub/", expected: true},
{endpoint: "[::1]:5000/v2/x", expected: true},
{endpoint: "https://[fd00::1]:5000", expected: false},
}
for _, g := range grid {
if actual := endpointHasPath(g.endpoint); actual != g.expected {
t.Errorf("endpointHasPath(%q): got %v, expected %v", g.endpoint, actual, g.expected)
}
}
}| // endpointHasPath reports whether a mirror endpoint carries its own API path, e.g. | ||
| // https://<account>.dkr.ecr.<region>.amazonaws.com/v2/<prefix> for an ECR pull-through cache. | ||
| // containerd appends /v2 to hosts.toml host paths unless override_path is set, whereas the | ||
| // legacy registry.mirrors endpoint list used a non-empty path as-is. Setting override_path | ||
| // for these endpoints keeps the behaviour of the legacy configuration. | ||
| func endpointHasPath(endpoint string) bool { |
There was a problem hiding this comment.
| // endpointHasPath reports whether a mirror endpoint carries its own API path, e.g. | |
| // https://<account>.dkr.ecr.<region>.amazonaws.com/v2/<prefix> for an ECR pull-through cache. | |
| // containerd appends /v2 to hosts.toml host paths unless override_path is set, whereas the | |
| // legacy registry.mirrors endpoint list used a non-empty path as-is. Setting override_path | |
| // for these endpoints keeps the behaviour of the legacy configuration. | |
| func endpointHasPath(endpoint string) bool { | |
| // endpointHasPath reports whether a mirror endpoint has a non-root path. These endpoints get | |
| // override_path, so containerd uses the path as-is instead of appending /v2, as the legacy | |
| // registry.mirrors config did. | |
| func endpointHasPath(endpoint string) bool { | |
| // Match containerd's parseHostConfig normalization so we check the same path. | |
| if !strings.HasPrefix(endpoint, "http") { | |
| endpoint = "https://" + endpoint | |
| } |
| if err != nil { | ||
| return false | ||
| } | ||
| return strings.TrimSuffix(u.Path, "/") != "" |
There was a problem hiding this comment.
| return strings.TrimSuffix(u.Path, "/") != "" | |
| return u.Path != "" && path.Clean(u.Path) != "/" |
|
Thanks @hakman! Applied both suggestions and added The normalization matters beyond style: without the |
Since registry mirrors moved from the inline registry.mirrors config to certs.d/<registry>/hosts.toml, containerd appends /v2 to every mirror host URL. The legacy endpoint list used a non-empty path as-is, so mirrors that carry their own API path, such as an ECR pull-through cache at https://<account>.dkr.ecr.<region>.amazonaws.com/v2/<prefix>, now resolve to .../v2/<prefix>/v2/<repo> and fail. Emit override_path = true for endpoints with a non-root path, which restores the previous behaviour. Endpoints without a path are unchanged.
…g the path Endpoints without a scheme (mirror.example.com:5000/v2/docker-hub) were misparsed by url.Parse, and paths such as "//" or "/v2/.." counted as a path. Prefix https:// the way containerd parseHostConfig does and compare the cleaned path against "/". Add a table test for endpointHasPath.
70ddb6e to
b9c131f
Compare
|
/ok-to-test |
|
Thanks @peter-svensson! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…-upstream-release-1.37 Automated cherry pick of #18819: containerd: set override_path for registry mirrors with a path
…-upstream-release-1.36 Automated cherry pick of #18819: containerd: set override_path for registry mirrors with a path
What this PR does / why we need it:
#18291 moved
containerd.registryMirrorsfrom the inlineregistry.mirrors.<name>.endpointconfig to/etc/containerd/certs.d/<name>/hosts.toml. The two formats treat a path in the mirror URL differently:registry.mirrorsendpoints: a non-empty path is used as-is,/v2is only added when the path is empty.hosts.toml: containerd always appends/v2to the host path unlessoverride_path = trueis set (hosts.md).Mirrors that carry their own API path therefore stopped working in 1.36. With an ECR pull-through cache configured as
kops 1.36 nodes request
while the correct URL
.../v2/docker-hub/<repo>/manifests/<digest>returns 200 with the same credentials. containerd then falls back to the upstream registry, so the failure is easy to miss until upstream pulls fail as well.This PR writes
override_path = truefor mirror endpoints whose URL has a non-root path, which matches the pre-1.36 behaviour. Endpoints without a path (https://mirror.example.com) produce the samehosts.tomlas before.Which issue(s) this PR fixes:
Special notes for your reviewer:
This is a regression from 1.35, so a cherry-pick to
release-1.36would be appreciated.The
complexcontainerd builder test gets an ECR-stylequay.iomirror to cover the path case; the existing mirrors in that test are unchanged in the golden output.