Treat 10.x.x.x registry addresses as local/insecure

[berndtj]

10.x.x.x registries should be considered insecure in the same manor registries suffixed with .local are. This is needed to pull from in cluster insecure registries:

Needed for: GoogleContainerTools/kaniko#405

[jonjohnsonjr]

Fixing this actually exacerbates another issue: #279

We probably want the behavior to first attempt https and fallback to http only for things that match the heuristic... (really, we need a flag or env var or something).

[berndtj]

Yeah, it does. I think the real problem is that we don't have a distinction between source and destination within Kaniko, so the insecure flag only applies to destination. Fallback actually sounds like a pretty good solution that would "just work".

[berndtj]

I've got another fix which should fix both my issue and #279. If we just do a fallback in the ping, it seems to work. I'm still evaluating this.

[berndtj]

Here is a fallback proposal: https://github.com/google/go-containerregistry/compare/master...berndtj:http-fallback?expand=1

I actually prefer the fix in this PR as I'm not really afraid of any unintended consequences and it's more explicit. My suggestion for a long-term fix and the "right" way would be to make the "insecure" property (and flag) take a list of CIDRs and domains. So for kaniko it would look like:

executor --insecure '10.0.0.0/8' --insecure '*.local'

There would be some work in propagating that though. This would also have to be propogated in knative serving. I think the above patch doesn't hurt anything by assuming RFC1918 addresses are insecure (GKE nodes assume this as well by default).

[berndtj]

@jonjohnsonjr I don't think I read your comment that closely. Is your suggestion that we more or less combine the two PRs above in that we keep the patterns and then if the patterns match, we do a fall-back, and only for those addresses. I think I can make that work... where reg.Scheme() would return back a list of schemes to use based on the match (i.e. localhost just returns back ["http"], and *.local returns ["https", "http"] and default just returns ["https"]). Does that sound like it would address all of the issues in a less hacky way?

[jonjohnsonjr]

Essentially yes, but I don't like it :(

I think what I'd really want is for our behavior to just be the same as this: https://docs.docker.com/registry/insecure/

I don't love how we implicitly assume certain endpoints are safe to be insecure, but it's harder to plumb these flags/configs around, so I'm not sure what to do 🙃

/cc @mattmoor what do you think?

[jonjohnsonjr]

@berndtj I haven't forgotten about this, just pondering still what the best path forward would be.

I think I'm fine with this PR going in as-is if we followup with the fallback.

re: the fallback proposal, I think it would be cleaner to pull the retry into the bearer transport's Roundtrip.

I'm not sure if it's easy (or even possible) to discriminate TLS-related errors from other connection-related errors, but perhaps retrying anything is a good idea.

In the bearer transport we could add something like:

	res, err := sendRequest()
	if err != nil {
                if bt.registry.Scheme() == "http" {
                        in.URL.Scheme = "http"
                        return sendRequest()
                }
		return nil, err
	}

And use "https" for every request first. It might make sense to split that logic out into an insecureFallbackTransport or something and wrap the bearerTransport's inner transport with it here.

[berndtj]

I can clean it up. I'm not thrilled with this methodology either. I think long-term it needs to be explicitly settable.