Skip to content

Conversation

@vvoland
Copy link
Collaborator

@vvoland vvoland commented Aug 31, 2022

This makes the containerd pull/push respect the insecureRegistries configuration.

Signed-off-by: Paweł Gronowski [email protected]

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@vvoland vvoland requested review from ndeloof, rumpl and thaJeztah August 31, 2022 15:36
@vvoland
Copy link
Collaborator Author

vvoland commented Sep 1, 2022

It works, but only if the http:///https:// scheme is explicitly present in the insecure-registries entries, due to:

u, err := url.Parse(v)

However, the non-c8d Docker does advise against using explicit schema:

moby/registry/config.go

Lines 194 to 203 in 0db5099

for _, r := range registries {
// validate insecure registry
if _, err := ValidateIndexName(r); err != nil {
return err
}
if strings.HasPrefix(strings.ToLower(r), "http://") {
logrus.Warnf("insecure registry %s should not contain 'http://' and 'http://' has been removed from the insecure registry config", r)
r = r[7:]
} else if strings.HasPrefix(strings.ToLower(r), "https://") {
logrus.Warnf("insecure registry %s should not contain 'https://' and 'https://' has been removed from the insecure registry config", r)

And because the daemon.RegistryHosts() is passed to buildkit (in the upstream Docker) then probably insecure registries are broken when building images? This probably needs to be looked at.

The non-c8d Docker doesn't need the schemas, because the behaviour for handling insecure registries is first to try the HTTPS ignoring the certificate errors, and if that fails fallback to HTTP.

@vvoland
Copy link
Collaborator Author

vvoland commented Sep 1, 2022

I added the fallback to the HTTP and changed the (daemon *Daemon) RegistryHosts to correctly parse scheme-less URL with ports.
It's a bit hacky, but it works.

@ndeloof
Copy link
Collaborator

ndeloof commented Sep 6, 2022

tested, but doesn't work for me.
My test setup:

  • create a registry container
  • run moby dev container with `--link registry``
  • register registry:5000 as insecureRegistry in daemon.json
$ docker pull registry:5000/alpine
Using default tag: latest
Error response from daemon: failed to resolve reference "registry:5000/alpine:latest": failed to do request: Head "https://registry:5000/v2/alpine/manifests/latest": http: server gave HTTP response to HTTPS client


for _, v := range daemon.configStore.InsecureRegistries {
u, err := url.Parse(v)
if err != nil && !strings.HasPrefix(v, "http://") && !strings.HasPrefix(v, "https://") {
Copy link
Collaborator

@ndeloof ndeloof Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parsing registry:5000 without a scheme doesn't trigger an error

Trying to parse a hostname and path without a scheme is invalid but may not necessarily return an error, due to parsing ambiguities

indeed, registry:5000 is parsed as Scheme="registry",...

@ndeloof ndeloof mentioned this pull request Sep 6, 2022
@ndeloof ndeloof closed this Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants