New insecure flag handling (PR #1002) breaks http->https redirecting

[Omar007]

We have a generalized/automated deployment workflow that utilizes crane to transfer images to registries, both secure and insecure. Because the registries can be of either type, we have the insecure flag set per default.

Starting with commit 9e56ddd / PR #1002 , which changes how the insecure flag is handled/interpreted, these have now started failing for registries that redirect HTTP to HTTPS.

The problem was near impossible to find based on the error as well as it will show something akin to the following:

failed to copy image: Get "https://...": stopped after 10 redirects

The HTTPS endpoint doesn't redirect at all.

Running crane verbose also doesn't clearly show the problem until you manually check each request it sends out and realize that it completely ignores the returned Location header it received when attempting to access the registry by HTTP.
It keeps retrying over and over on the HTTP endpoint, then eventually reaching 10 redirects (which it technically didn't, it only ever hits 1 but 10 times because it never respects the redirect somehow).

Example logging:

2021/06/14 15:19:47 GET /service/token?<redacted> HTTP/1.1
Host: <redacted>
User-Agent: crane/a27f4a44317b43da4146cbd86f1b817f2c6a9ecb go-containerregistry
Authorization: <redacted>
Referer: http://<redacted>
Accept-Encoding: gzip
2021/06/14 15:19:47 <-- 308 http://<redacted> (1.127813ms) [body redacted: basic token response contains credentials]
2021/06/14 15:19:47 HTTP/1.1 308 Permanent Redirect
Content-Length: 171
Connection: keep-alive
Content-Type: text/html
Date: Mon, 14 Jun 2021 15:19:47 GMT
Location: https://<redacted>
Server: nginx/1.16.1
2021/06/14 15:19:47 --> GET http://<redacted>[body redacted: basic token response contains credentials]
2021/06/14 15:19:47 GET /service/token?<redacted> HTTP/1.1
Host: <redacted>
User-Agent: crane/a27f4a44317b43da4146cbd86f1b817f2c6a9ecb go-containerregistry
Authorization: <redacted>
Referer: http://<redacted>
Accept-Encoding: gzip
2021/06/14 15:19:47 <-- 308 http://<redacted> (1.172246ms) [body redacted: basic token response contains credentials]
2021/06/14 15:19:47 HTTP/1.1 308 Permanent Redirect
Content-Length: 171
Connection: keep-alive
Content-Type: text/html
Date: Mon, 14 Jun 2021 15:19:47 GMT
Location: https://<redacted>
Server: nginx/1.16.1
.......
<REPEAT UNTIL LIMIT>

[Omar007]

Going over both personal issues previously with the insecure behaviour as well as some open as well as old closed issues, from my perspective it would probably be best to revert the change and if it should change, to make it work in the same way as docker's insecure flag.
Previously most things where just a thing to know how to fix/work around and maybe have documented but this seems to be fully breaking.

See also #451 , #738 (#738 (comment)), #991 and #1024

[jonjohnsonjr]

Yeah this was something I was worried about with that change, going to revert it and see how we can fix things.

We're doing some fundamentally weird stuff where this http fallback is backwards. The outer http.Client handles redirects, but the inner http.RoundTripper is the one selecting the scheme, which doesn't handle redirects (I should have considered this).

This scheme logic should live at a higher layer than it does.

from my perspective it would probably be best to revert the change and if it should change, to make it work in the same way as docker's insecure flag.

I haven't looked at exactly how this works in a while, I'd be interested in more details.

[Omar007]

I haven't looked at exactly how this works in a while, I'd be interested in more details.

From what I know it hasn't changed since #738 (comment) but I can look it up. I'll update this in a bit.

It indeed hasn't; https://docs.docker.com/registry/insecure/

With insecure registries enabled, Docker goes through the following steps:
First, try using HTTPS.
If HTTPS is available but the certificate is invalid, ignore the error about the certificate.
If HTTPS is not available, fall back to HTTP.

[jonjohnsonjr]

We should probably have a test to catch any regressions here.

[Omar007]

The revert (11f8769 / #1048 ) resolved this issue so I'm closing this one. I'll also add a comment with the above information on Docker's insecure flag handling to #1024