Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes 404 error when authenticating with v2 reg #724

Merged
merged 1 commit into from
Jan 26, 2022
Merged

Fixes 404 error when authenticating with v2 reg #724

merged 1 commit into from
Jan 26, 2022

Conversation

dylanrhysscott
Copy link
Contributor

@dylanrhysscott dylanrhysscott commented Jan 23, 2022

After applying this fix I can successfully authenticate with github registry

[vagrant@localhost nerdctl]$ nerdctl login -u <USERNAME> ghcr.io
Enter Password:
Login Succeeded
[vagrant@localhost nerdctl]$

[vagrant@localhost ~]$ nerdctl pull ghcr.io/<IMAGE>
ghcr.io/<IMAGE>:                            resolved       |++++++++++++++++++++++++++++++++++++++|
manifest-sha256:9d7715dd1e7a20a03c1fda6b625ab73aa8e1a9cacddad885b79876bd2a8ebce8: done           |++++++++++++++++++++++++++++++++++++++|
config-sha256:6f7f6bfd79125762caacefdb0e83ece7e101c85279d31fbc36f61d643f878a42:   done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:abd092694f3b3635a1642c6fed15a81f02bbcd44045fc5dfb138eda3531899bf:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:b53a42fddd847a0d1d550565601c9299fecae84e94ffcb8c9a9e5e98231e0ee0:    done           |++++++++++++++++++++++++++++++++++++++|
elapsed: 3.8 s                                                                    total:  7.8 Mi (2.1 MiB/s)
[vagrant@localhost ~]$

This maybe a slightly naive approach so welcome feedback on a better place for this - Not sure who to ping from containerd

Closes #715

cmd/nerdctl/login.go Outdated Show resolved Hide resolved
@@ -253,6 +253,12 @@ func tryLoginWithRegHost(ctx context.Context, rh docker.RegistryHost) error {
Host: rh.Host,
Path: rh.Path,
}
if rh.Path == "/v2" {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if !strings.HasSuffix(rh.Path, "/") {
  rh.Path += "/"

This way you avoid hard-coded v2 in the code. Don't you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I'd forgotten about HasSuffix from that package - looks like this PR will be closed off and merged elsewhere but that would be a better approach

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda @alisson276 I'm looking at this suggestion and I'm not sure to implement seems a good change but as far as I can tell the login issues are only affecting v2 and adding this applies it to every version of registry - Would it be best to just apply the leading slash to /v2 only or globally?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just checking if rh.Path == "/v2" is fine.

Copy link

@alisson276 alisson276 Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just think that keep in sync with the containerd library is not a good idea. If, for any reason, they change that path, it must be synced here. If you are worried about always adding a slash, the safest approach is to try twice: first using the rh.Path (no matter what it is, if v2, v3, whatever) and if fails, do a second try appending the slash at the end (IMO).

Copy link

@alisson276 alisson276 Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I correctly understood, in the PR they are asking to add this explicit in the GET call... in other words, do a:

req, err := http.NewRequest(http.MethodGet, u.String() + "/", nil)

So, this if is ok, I just don't know if fixing the v2 is a good idea, but this is just my opinion. I'm just trying to contribute because this problem afftects my environment too.

In docker CLI it doesn't happen. do you know how this is handled there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alisson276 v2 is part of the docker registry specification which I think is pretty static and as far as I know unlikely to change but it maybe a valid concern in future - with regards to doesn't happen with Docker question docker uses the dockerd daemon and not containerd directly as well as using /v1 and not /v2. This might explain the difference

@AkihiroSuda
Copy link
Member

Thanks, I'm closing this, as we now have containerd/containerd#6470

#724 (comment)

@AkihiroSuda AkihiroSuda reopened this Jan 25, 2022
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Jan 25, 2022

Reopened per discussion in containerd/containerd#6470 .
Sorry for going back and force.

@AkihiroSuda AkihiroSuda added this to the v0.16.1 milestone Jan 25, 2022
cmd/nerdctl/login.go Outdated Show resolved Hide resolved
Signed-off-by: Dylan Scott <dylanrhysscott@gmail.com>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@AkihiroSuda AkihiroSuda merged commit 58cf501 into containerd:master Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Valid login to ghcr.io returns 404 and pulling images still returning 401 due to failed flow
3 participants