Registry host configuration cleanup

[dmcgowan]

First commit copies the resolver code from buildkit with fix for insecure registries and could be backported.
Second commit cleans up the resolve logic using containerd's host config.

closes #47240

[thaJeztah]

Couple of failures that look related. Could be that the test was written for a specific error output though (these are the integration-cli tests, which use a fixed, old version of the CLI), or perhaps we're missing some conversion for errors returned 🤔

=== Failed
=== FAIL: amd64.integration-cli TestDockerRegistryAuthHtpasswdSuite/TestBuildFromAuthenticatedRegistry (0.34s)
    docker_cli_build_test.go:4988: assertion failed: 
        Command:  /usr/local/cli-integration/docker push 127.0.0.1:5000/baseimage
        ExitCode: 1
        Error:    exit status 1
        Stdout:   The push refers to repository [127.0.0.1:5000/baseimage]
        76df9210b28c: Waiting
        
        Stderr:   unexpected status from HEAD request to http://127.0.0.1:5000/v2/baseimage/blobs/sha256:76df9210b28cbd4bc127844914d0a23937ed213048dc6289b2a2d4f7d675c75e: 401 Unauthorized
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error
    check_test.go:532: [da6a45cfad359] daemon is not started
    --- FAIL: TestDockerRegistryAuthHtpasswdSuite/TestBuildFromAuthenticatedRegistry (0.34s)

=== FAIL: amd64.integration-cli TestDockerRegistryAuthHtpasswdSuite/TestBuildWithExternalAuth (0.31s)
    docker_cli_build_test.go:5024: assertion failed: 
        Command:  /usr/local/cli-integration/docker --config /tmp/integration-cli-1513645647 push 127.0.0.1:5000/dockercli/busybox:authtest
        ExitCode: 1
        Error:    exit status 1
        Stdout:   The push refers to repository [127.0.0.1:5000/dockercli/busybox]
        76df9210b28c: Waiting
        
        Stderr:   unexpected status from HEAD request to http://127.0.0.1:5000/v2/dockercli/busybox/blobs/sha256:76df9210b28cbd4bc127844914d0a23937ed213048dc6289b2a2d4f7d675c75e: 401 Unauthorized
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error
    check_test.go:532: [d254f721c0d6c] daemon is not started
    --- FAIL: TestDockerRegistryAuthHtpasswdSuite/TestBuildWithExternalAuth (0.31s)

=== FAIL: amd64.integration-cli TestDockerRegistryAuthHtpasswdSuite/TestLogoutWithExternalAuth (0.87s)
    docker_cli_logout_test.go:53: assertion failed: error is not nil: exit status 1
    --- FAIL: TestDockerRegistryAuthHtpasswdSuite/TestLogoutWithExternalAuth (0.87s)

=== FAIL: amd64.integration-cli TestDockerRegistryAuthHtpasswdSuite/TestPullWithExternalAuth (0.29s)
    docker_cli_pull_local_test.go:439: assertion failed: 
        Command:  /usr/local/cli-integration/docker --config /tmp/integration-cli-2597430029 push 127.0.0.1:5000/dockercli/busybox:authtest
        ExitCode: 1
        Error:    exit status 1
        Stdout:   The push refers to repository [127.0.0.1:5000/dockercli/busybox]
        76df9210b28c: Waiting
        
        Stderr:   unexpected status from HEAD request to http://127.0.0.1:5000/v2/dockercli/busybox/blobs/sha256:1c35c441208254cb7c3844ba95a96485388cef9ccc0646d562c7fc026e04c807: 401 Unauthorized
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error
    check_test.go:532: [d37782d3d4dbc] daemon is not started
    --- FAIL: TestDockerRegistryAuthHtpasswdSuite/TestPullWithExternalAuth (0.29s)

=== FAIL: amd64.integration-cli TestDockerRegistryAuthHtpasswdSuite/TestPullWithExternalAuthLoginWithScheme (0.29s)
    docker_cli_pull_local_test.go:397: assertion failed: 
        Command:  /usr/local/cli-integration/docker --config /tmp/integration-cli-846080656 push 127.0.0.1:5000/dockercli/busybox:authtest
        ExitCode: 1
        Error:    exit status 1
        Stdout:   The push refers to repository [127.0.0.1:5000/dockercli/busybox]
        76df9210b28c: Waiting
        
        Stderr:   unexpected status from HEAD request to http://127.0.0.1:5000/v2/dockercli/busybox/blobs/sha256:1c35c441208254cb7c3844ba95a96485388cef9ccc0646d562c7fc026e04c807: 401 Unauthorized
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error
    check_test.go:532: [df1a930ceacf0] daemon is not started
    --- FAIL: TestDockerRegistryAuthHtpasswdSuite/TestPullWithExternalAuthLoginWithScheme (0.29s)

=== FAIL: amd64.integration-cli TestDockerRegistryAuthHtpasswdSuite/TestPushNoCredentialsNoRetry (0.17s)
    docker_cli_push_test.go:225: assertion failed: expression is false: strings.Contains(out, "no basic auth credentials")
    check_test.go:532: [da543621fde40] daemon is not started
    --- FAIL: TestDockerRegistryAuthHtpasswdSuite/TestPushNoCredentialsNoRetry (0.17s)

=== FAIL: amd64.integration-cli TestDockerRegistryAuthHtpasswdSuite (2.88s)

[dmcgowan]

Marked this as ready to go. I fixed the authorizer with the containerd image service resolver. The other failures now don't seem related, look like flakes.

[vvoland]

Won't this make only the localhost insecure registries use the http fallback?
Also, shouldn't localhost registries should always use the http fallback by default, even if they're not explicitly an insecure registry?

[thaJeztah]

Didn't look at this PR in-depth, but I think we have some code somewhere that automatically adds localhost (more factually; the loopback range) as insecure), so it's possible that it's already automatically configured as insecure before we hit this code (????);

Insecure Registries:
  127.0.0.0/8

(Now wondering if that should also include the IPv6 loopback address ::1 , but I guess that's a separate topic).

[thaJeztah]

@dmcgowan could you have a look?

[dmcgowan]

Won't this make only the localhost insecure registries use the http fallback?

It was intentional here to only use this for localhost since stepping down from insecure TLS to HTTP on localhost does not have the same implications as non-localhost. Although to keep the functionality the same in regards to the existing ambiguity over the meaning of "insecure" this could be applied to all hosts. In that case an empty TLS config would need to be generated if not set still to match existing logic I think.

I need to find a better approach for skipping over the legacy config, currently it checks for a nil configuration but I don't believe that is ever nil. Ideally the new (and less ambiguous) configuration files can be used without hitting the legacy merge logic when no legacy configuration is provided.

Also, shouldn't localhost registries should always use the http fallback by default, even if they're not explicitly an insecure registry?

I think that is up to the default configuration, as @thaJeztah mentioned, localhost is added by default. (see https://github.com/moby/moby/blob/master/registry/config.go#L187).

Now wondering if that should also include the IPv6 loopback address ::1 , but I guess that's a separate topic

Probably wouldn't hurt to add as a default, we should just make sure the behavior is the same with the new configuration and deprecate the old one.

[dmcgowan]

There is a pending fix for the http handler on the containerd side that we can wait for in 1.7 before updating this one.

Also on another look I'm not sure checking for the service config being nil as the switch to merge in the legacy config is sufficient, especially if the legacy config is allowing for a more loose definition of "insecure" for non-localhost registries.

[dmcgowan]

Pushed an update the fixes I mentioned.

As for the containerd fix, we need to handle TLS handshake timeouts (see containerd/containerd#10014). Although the current version we have also has this issue, so it might not be a blocker for taking this change in. There will be a followup regardless.

[thaJeztah]

Would u.Path ever have multiple trailing slashes (after parsing?) if not, and it will always be one, then perhaps this should be TrimSuffix https://pkg.go.dev/strings#TrimSuffix

[dmcgowan]

It is needed to trim but updated to TrimSuffix