Make the ServerAddress optional for the main docker registry. #1315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Eric Promislow <epromislow@suse.com>
ericpromislow
added a commit
to rancher-sandbox/rancher-desktop
that referenced
this pull request
Aug 12, 2022
If a user is logged in, but there's no auths["https://index.docker.io/v1/"] entry in the root's `.docker/config.json` file, the docker-resolver code will get confused. So here's how to get into the state that this change fixes: * Factory-reset or create a clean state * Start up RD in docker mode (with k8s off to speed things up) * docker login * Switch to containerd * nerdctl pull busybox Without this change, you'll see an error along the lines of FATA[0000] expected ac.ServerAddress ("") to be "https://index.docker.io/v1/" With this change, nerdctl should proceed as usual. Note that with no `auths` field in `/root/.docker/config.json`, running `docker login` doesn't add it, but running `nerdctl login` does. And if the user is logged out when the code is run, `nerdctl logout` changes the field to `auths: {}`, but the behavior is the same for logged out users, whether auths is empty or has an entry for index.docker.io. The real error here is that when a `configsStore` field is given in `.docker/config.json`, the `auths` field should be ignored. Docker is ignoring it, nerdctl isn't. Upstream issue: github.com/containerd/nerdctl/pull/1315 with PR 1315, but some integration tests are failing (looks like due to flakes) and I don't see a straightforward way to provide unit tests. Signed-off-by: Eric Promislow <epromislow@suse.com>
|
The |
ericpromislow
added a commit
to rancher-sandbox/rancher-desktop
that referenced
this pull request
Aug 15, 2022
A bug in the nerdctl client expects to see an `auths` entry for `https://index.docker.io/v1/` in `~/.docker/config.json` even when we're getting the credentials from the `credsStore` field. This is a workaround for upstream PR containerd/nerdctl#1315 Signed-off-by: Eric Promislow <epromislow@suse.com>
fahedouch
reviewed
Aug 16, 2022
| @@ -201,7 +201,7 @@ func NewAuthCreds(refHostname string) (AuthCreds, error) { | |||
| // - ac.ServerAddress: "https://index.docker.io/v1/". | |||
| if !isAuthConfigEmpty(ac) { | |||
| if authConfigHostname == registry.IndexServer { | |||
There was a problem hiding this comment.
if ac.ServerAddress == "" {
ac.ServerAddress = registry.IndexServer
}
I think we should enforce the default registry when it is empty for all refHostname
ericpromislow
added a commit
to rancher-sandbox/rancher-desktop
that referenced
this pull request
Aug 16, 2022
A bug in the nerdctl client expects to see an `auths` entry for `https://index.docker.io/v1/` in `~/.docker/config.json` even when we're getting the credentials from the `credsStore` field. This is a workaround for upstream PR containerd/nerdctl#1315 Signed-off-by: Eric Promislow <epromislow@suse.com>
ktock
reviewed
Aug 18, 2022
| if ac.ServerAddress != registry.IndexServer { | ||
| if ac.ServerAddress != "" && ac.ServerAddress != registry.IndexServer { |
There was a problem hiding this comment.
nit: To preserve the behaviour of printing the debug log on ac.ServerAddress == "", I think the if logic should be something like:
if ac.ServerAddress == "" {
// This can happen with Amazon ECR: https://github.com/containerd/nerdctl/issues/733
logrus.Debugf("failed to get ac.ServerAddress for authConfigHostname=%q (refHostname=%q)",
authConfigHostname, refHostname)
} else if authConfigHostname == registry.IndexServer {
if ac.ServerAddress != registry.IndexServer {
return nil, fmt.Errorf("expected ac.ServerAddress (%q) to be %q", ac.ServerAddress, registry.IndexServer)
}
} else {
acsaHostname := credentials.ConvertToHostname(ac.ServerAddress)
if acsaHostname != authConfigHostname {
return nil, fmt.Errorf("expected the hostname part of ac.ServerAddress (%q) to be authConfigHostname=%q, got %q",
ac.ServerAddress, authConfigHostname, acsaHostname)
}
}
There was a problem hiding this comment.
@ericpromislow @jandubois ↑ LGTY?
There was a problem hiding this comment.
@AkihiroSuda Yes, LGTM, and I've tested that it fixes the repro case specified in #1308 (comment)
There was a problem hiding this comment.
BTW, @ericpromislow is still away, so you won't get any feedback from him for at least another 10 days.
There was a problem hiding this comment.
@jandubois Thanks for testing and providing the info
@ktock Please add your commit to this PR (or just open a new PR)?
|
@ericpromislow Could you take a look at comments? 🙏 |
Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
|
@ktock - I understand the nit, but the problem with it is that if you have the following cases:
then your suggested code will give an incorrect warning message that it failed to get |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1308
Signed-off-by: Eric Promislow epromislow@suse.com