daemon/RegistryHosts: Don't lose mirrors#46565
Conversation
vvoland
added
area/distribution
|
Note: Mirrors are still broken for buildkit and containerd integration on master (doesn't affect 24.0) until moby/buildkit#4293 is merged and gets vendored into moby. We should probably block the 25.0 release before this is resolved. |
This will go away when buildkit PR is merged. |
|
I guess for this one we need the moby/buildkit#4297 backport merged and vendored, or is that only for Buildkit standalone? |
`docker.io` is present in the `IndexConfigs` so the `Mirrors` property would get lost because a fresh `RegistryConfig` object was created. Instead of creating a new object, reuse the existing one and just mutate its fields. Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
vvoland
force-pushed
the
c8d-mirrors-fix
branch
from
88a554b to
aca9ea4
Compare
|
Revendored buildkit from the |
| c := m[k] | ||
| if !v.Secure { | ||
| t := true | ||
| c.PlainHTTP = &t | ||
| c.Insecure = &t |
There was a problem hiding this comment.
Perhaps we have checks elsewhere, but does this mean we allow docker.io to be used with "insecure"?
There was a problem hiding this comment.
It shouldn't because docker.io is always overriden with Secure: true entry:
Lines 236 to 241 in aacd100
but... I think it could be worked around if you explicitly use registry-1.docker.io? But it might get translated to docker.io before this is used, so perhaps it's not actually possible.
| } | ||
| if _, ok := m[host]; !ok && daemon.registryService.IsInsecureRegistry(host) { | ||
| c := resolverconfig.RegistryConfig{} | ||
| if c, ok := m[host]; !ok && daemon.registryService.IsInsecureRegistry(host) { |
There was a problem hiding this comment.
Not something new in this PR, but IsInsecureRegistry also appears to be checking config.IndexConfigs. Are we doing the same thing twice in this function?
There was a problem hiding this comment.
Hmm, good question, I thought it was needed because it also does the isCIDRMatch check for insecure registries specified by CIDR (InsecureRegistryCIDRs).
But if I understand correctly, it actually doesn't handle them, because these are not a part of IndexConfigs anyway...
There was a problem hiding this comment.
Yeah, the overall combination of mirrors and insecure registries (and how they're both "separate" and "depending on each other") is ... messy
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
let's bring this one in; we can look at cleaning up the double check in a follow-up (not a new issue)
LGTM
3 participants
docker.iois present in theIndexConfigsso theMirrorsproperty would get lost because a freshRegistryConfigobject was created.Instead of creating a new object, reuse the existing one and just mutate its fields.
- What I did
Fixed mirrors not working with buildkit.
- How I did it
See commit.
- How to verify it
Try to use registry mirror.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)