daemon: use the registry service config for getting registry hosts #45992
Conversation
The RegistryHosts lookup function is used by both BuildKit and by the containerd snapshotter. However, this function differs in behaviour from the config parser for the RegistryConfig: - The protocol for insecure registries is treated as significant by RegistryHosts, while the RegistryConfig strips this information. - RegistryConfig validates and deduplicates mirrors. - RegistryConfig does not parse the insecure-registries as URLs, which can lead to parsing opaque URLs as was possible by the RegistryHosts function. This patch updates the lookup function to ensure consistency. Signed-off-by: Justin Chadwell <me@jedevc.com>
insecure-registries supports using CIDR notation, however, buildkit in moby was not respecting these. We can update the RegistryHosts function to support this by inserting the correct host into the lookup map if it's explicitly marked as insecure. Signed-off-by: Justin Chadwell <me@jedevc.com>
| // RegistryHosts returns the registry hosts configuration for the host component | ||
| // of a distribution image reference. |
There was a problem hiding this comment.
Is the "for the host component" part still accurate after this?
(I need to look into the logic; I know there's a lot of ugliness in this area, some for historic reasons 😞)
There was a problem hiding this comment.
I think so, yup. We still take a host as the argument, and store the resulting hosts.
We already discarded paths/fragments/etc in this function, so no behaviour changes on that front.
|
@tianon @neersighted PTAL :) |
| c.PlainHTTP = &t | ||
| c.Insecure = &t |
There was a problem hiding this comment.
Does this mean we assume all insecure registries are plain HTTP, or is this used to trigger the "try HTTPS ignoring the cert, then fall back to HTTP" logic? Sorry, I'm not very familiar with the underlying code here 😭
(IMO this detection behavior of "insecure-registries" is still a misfeature and it'd be a benefit to us to go the other direction and encourage users to make it explicit, but I think I might've already lost that fight with some of the previous related PRs 🙈)
There was a problem hiding this comment.
or is this used to trigger the "try HTTPS ignoring the cert, then fall back to HTTP" logic
Exactly this 🎉 This is how we get here in BuildKit: https://github.com/moby/buildkit/blob/master/util/resolver/resolver.go#L25.
This function gets called in resolver.NewRegistryConfig which is called by moby, which returns the two hosts, which get tried in order (return on first success).
and it'd be a benefit to us to go the other direction and encourage users to make it explicit, but I think I might've already lost that fight with some of the previous related PRs
I don't think this battle is necessarily lost -- given our lack of knowledge about how this worked, I think it's fair to assume most users don't really know. So we could make a point of deprecating the current behavior at some point, and try and require protocols.
|
Looking further, this looks like this may also resolve:
from https://docs.docker.com/desktop/containerd/#docker-desktop-4120-release. I can't find a tracking issue for this in GitHub though, not sure if this way already fixed by something else? |
|
We briefly discussed this in the maintainers call. This is indeed one of the "historic" areas where a lot of context has been lost, and behavior is very under-defined. Let's get this one in for the 25.0 release at least, and if the world doesn't explode, we can consider backporting to 24.0 and 23.0 branches if needed. |
|
Did this need a backport? |
|
@cpuguy83 see my comment above, but if you think it's safe, we can still consider to already backport it |
|
When used internally, you need to use http, and you want to configure the item to turn it on or off. |
- What I did
Fixes docker/buildx#1642 (this one was an utter pain to track down 🎉)
The logic in
RegistryHostsappears to be shared between BuildKit and the containerd snapshotter, so this may also be a containerd related fix?- How I did it
The logic previously used in
RegistryHostswas duplicated logic from the config we've already parsed - however, there were significant differences in exactly the outputs it produced.To resolve the above buildx issue, it's sufficient to just update
RegistryHoststo use the already parsed configs, which have been validated and cleaned appropriately. We can also add support for the CIDR notation allowed in insecure-registries while we're in the area.- How to verify it
See the reproduction instructions in docker/buildx#1642 (comment). Before the patches in this PR you should see:
And after the patches, the build should complete successfully, with no error displayed.
- Description for the changelog
Resolve
insecure-registriesconsistently for BuildKit builds- A picture of a cute animal (not mandatory but encouraged)
cc @tianon @tonistiigi @crazy-max