util/resolver: Perform Insecure HTTPS + HTTP fallback in one docker.RegistryHost

[vvoland]

If both Insecure and PlainHTTP is requested for the host, use a
transport that falls back to HTTP in case of an HTTP request to a HTTPS
client error.

This also changes the order - before that an HTTP connection was
attempted first. Now an HTTPS connection with insecure TLS check will be
attempted first and will only fallback to HTTP if the former fails.

This fixes push to an insecure HTTPS-only registry.

[jedevc]

This fixes push to an insecure HTTPS-only registry.

Did this not used to work? I've used this functionality before, and it worked fine.

Is this also related to #3382? Also see containerd/containerd#8861 for an upstream fix for something that seems like the same thing - I think I'd prefer that fix, instead of custom fallback logic in buildkit (for something that feels like an upstream containerd issue).

The switching order to prefer insecure-HTTPS over HTTP SGTM 🎉

[vvoland]

It doesn't work - containerd push only uses the first host with push capability. Before this path this happened to be the RegistryHost with the http scheme and doing an http request to a https server will fail and the RegistryHost with https scheme will not be tried so the whole operation will fail.

Changing the order alone isn't sufficient, because it breaks the other case - HTTP only registry (http response to a https client). The transport level fallback solves that because it detects the error and switches to http.

There's also an upstream fix: containerd/containerd#9182 but it would require switching to containerd's ConfigureHosts. That should be a target solution, but that needs to wait for the release and we need to make sure that it would be a drop-in replacement.

[jedevc]

I think the approach SGTM, thanks for the additional context ❤️

Just a note, maybe not something to do right now, but it looks like we don't have any tests for insecure/http registry access? At some point, we should build out that matrix 😢

[tonistiigi]

It doesn't work - containerd push only uses the first host with push capability.

But isn't this the expected behavior? Why would pull and push work differently with hosts array, even after containerd/containerd#9182 . cc @dmcgowan (I'm not sure if we should actually use #9182 for this in the future).

Also, (as follow-up) could we remove this capability? There have been 4-5 patches only for this ambiguous behavior of self-signed TLS and plain HTTP in Moby. Why can't the user just define which one they need, and it would be faster as well instead of trying wrong requests every time.

[vvoland]

Why would pull and push work differently with hosts array

That's the part of the problem - it shouldn't, but it currently does. Containerd pull will try next host if the previous one fails. The reason it's currently not the case for push is that it happens "per-blob" and it can't be assumed that the "next host" will actually refer to the same registry and you could end up pushing part of the image to registry A and the other to registry B.

Also, (as follow-up) could we remove this capability?

Personally I would like to deprecate the current insecure-registries and switch to a better format, possibly just use the containerd hosts.toml directly (https://github.com/containerd/containerd/blob/main/docs/hosts.md).

[dmcgowan]

Also, (as follow-up) could we remove this capability?

Agreed that we should remove these ambiguous configurations and provide a clearer option here. The default localhost behavior is still a big awkward here.

But isn't this the expected behavior?

I would like to keep this behavior as well, where we only use the first host. We don't currently have any defined condition where we would want to try the next host, but the meaning of providing multiple push hosts is still ambiguous. The http/https fallback at least lets us treat it as a single host, although ambiguous how to connect.