Registry host configuration cleanup by dmcgowan · Pull Request #47380 · moby/moby

dmcgowan · 2024-02-14T00:05:34Z

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.

thaJeztah · 2024-02-14T10:31:55Z

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)

daemon/hosts.go

dmcgowan · 2024-02-29T19:07:25Z

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 · 2024-03-05T09:44:59Z

daemon/hosts.go

+				isLocalhost, err := docker.MatchLocalhost(hosts[i].Host)
+				if err != nil {
+					continue
+				}
+				if isLocalhost {


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?

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).

@dmcgowan could you have a look?

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 · 2024-04-22T22:41:29Z

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 · 2024-04-23T00:19:40Z

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.

daemon/hosts.go

thaJeztah · 2024-04-26T18:10:13Z

daemon/hosts.go

+			}
+			if err == nil && u.Host != "" {
+				h.Host = u.Host
+				h.Path = strings.TrimRight(u.Path, "/")


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

It is needed to trim but updated to TrimSuffix

daemon/hosts.go

vvoland

LGTM; left one small nit in test

daemon/hosts_test.go

dmcgowan · 2024-06-12T18:42:19Z

Rebased but we don't need to consider this for 27 for the fix. The fix for #47240 was included in the latest buildkit update with moby/buildkit#4975.

We can consider the improved registry configuration for 28 release.

This logic is going to be updated to use the new containerd resolver and needs all the logic handling resolution in the package where it is used. Signed-off-by: Derek McGowan <[email protected]>

vvoland

LGTM
@thaJeztah PTAL

vvoland · 2024-10-25T09:04:27Z

Rerunning CI

vvoland · 2024-10-25T11:33:30Z

Ah, looks like there's a linter error:

 daemon/hosts.go:16: File is not `goimports`-ed (goimports)
	cerrdefs "github.com/containerd/errdefs"

Signed-off-by: Derek McGowan <[email protected]>

Use the daemon's configuration to check whether the legacy registry configuration is used. Only attempt to merge with the legacy configuration if it has been provided. This avoids merging in based on a defaulted legacy config. Signed-off-by: Derek McGowan <[email protected]>

Note that while it is not safe to use http fallback on non-localhost registries, this can be avoided using the new host directories. The previous legacy insecure configuration is ambiguous and less secure. Signed-off-by: Derek McGowan <[email protected]>

Move the conversion to its own function and add unit tests. Signed-off-by: Derek McGowan <[email protected]>

dmcgowan force-pushed the registry-http-fallback branch from e99532d to e3fb795 Compare February 14, 2024 05:28

vvoland reviewed Feb 15, 2024

View reviewed changes

daemon/hosts.go Outdated Show resolved Hide resolved

dmcgowan force-pushed the registry-http-fallback branch 3 times, most recently from 11ed7f8 to 28acdd4 Compare February 29, 2024 16:44

dmcgowan marked this pull request as ready for review February 29, 2024 17:59

vvoland reviewed Mar 5, 2024

View reviewed changes

thaJeztah added area/distribution Image Distribution status/2-code-review area/daemon Core Engine kind/refactor PR's that refactor, or clean-up code kind/bugfix PR's that fix bugs labels Mar 16, 2024

thaJeztah assigned dmcgowan Mar 16, 2024

dmcgowan force-pushed the registry-http-fallback branch from 28acdd4 to 4e5cf6d Compare April 23, 2024 00:13

dmcgowan mentioned this pull request Apr 25, 2024

vendor: update containerd to v1.7.16 #47763

Merged

dmcgowan force-pushed the registry-http-fallback branch 2 times, most recently from 5abfc63 to 8512eb4 Compare April 26, 2024 17:30

thaJeztah reviewed Apr 26, 2024

View reviewed changes

dmcgowan force-pushed the registry-http-fallback branch 3 times, most recently from 9d15d56 to a404358 Compare April 28, 2024 03:45

dmcgowan added the kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny label May 2, 2024

dmcgowan added this to the 27.0.0 milestone May 2, 2024

vvoland approved these changes May 9, 2024

View reviewed changes

daemon/hosts_test.go Outdated Show resolved Hide resolved

dmcgowan force-pushed the registry-http-fallback branch from a404358 to 336126a Compare May 13, 2024 23:40

dmcgowan removed this from the 27.0.0 milestone Jun 12, 2024

dmcgowan force-pushed the registry-http-fallback branch from 336126a to 07bdfbd Compare June 12, 2024 18:43

dmcgowan added this to the 28.0.0 milestone Jun 20, 2024

vvoland requested a review from thaJeztah July 4, 2024 09:06

Fork buildkit resolver logic to daemon package

7c087c3

This logic is going to be updated to use the new containerd resolver and needs all the logic handling resolution in the package where it is used. Signed-off-by: Derek McGowan <[email protected]>

dmcgowan force-pushed the registry-http-fallback branch 2 times, most recently from fb69fce to 5f7578f Compare October 22, 2024 14:56

vvoland approved these changes Oct 22, 2024

View reviewed changes

vvoland closed this Oct 25, 2024

vvoland reopened this Oct 25, 2024

vvoland mentioned this pull request Oct 25, 2024

[27.x] c8d/httpfallback: Handle connection errors #48758

Merged

dmcgowan added 4 commits October 25, 2024 12:44

Update host resolver to use containerd host config

8b4cb6f

Signed-off-by: Derek McGowan <[email protected]>

Cleanup legacy mirror string to registry host

2aaae08

Move the conversion to its own function and add unit tests. Signed-off-by: Derek McGowan <[email protected]>

dmcgowan force-pushed the registry-http-fallback branch from 5f7578f to 2aaae08 Compare October 25, 2024 19:45

vvoland merged commit dc22579 into moby:master Oct 30, 2024

vvoland mentioned this pull request Nov 5, 2024

c8d: http fallback nil dereference #48818

Closed

thaJeztah mentioned this pull request Nov 6, 2024

fix vendor of github.com/containerd/containerd #48826

Merged

thaJeztah mentioned this pull request Nov 21, 2024

Update containerd to v1.7.24 #48917

Merged

thaJeztah mentioned this pull request Dec 8, 2024

daemon/containerd: hostsWrapper: remove unused regService argument #49049

Merged

stenh0use mentioned this pull request Jan 8, 2025

Extending Spegel to Nomad Docker clusters spegel-org/spegel#303

Open

Conversation

dmcgowan commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Feb 14, 2024

Uh oh!

Uh oh!

dmcgowan commented Feb 29, 2024

Uh oh!

vvoland Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

dmcgowan Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmcgowan commented Apr 23, 2024

Uh oh!

Uh oh!

thaJeztah Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

dmcgowan Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmcgowan commented Jun 12, 2024

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland commented Oct 25, 2024

Uh oh!

vvoland commented Oct 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmcgowan commented Feb 14, 2024 •

edited

Loading

dmcgowan commented Apr 22, 2024 •

edited

Loading