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

c8d/pull: Don't wrap no basic auth error #46662

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Oct 17, 2023

Don't wrap the no basic auth credentials error from containerd and return it as-is.

The error will look like:

failed to resolve reference "docker.io/library/aodkoakds:latest": pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed

- How to verify it
Cherry-pick: containerd/containerd#9234

make TEST_FILTER='TestLogoutWithExternalAuth'   TEST_IGNORE_CGROUP_CHECK=1   DOCKER_GRAPHDRIVER=overlayfs TEST_INTEGRATION_USE_SNAPSHOTTER=1    test-integration

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@vvoland vvoland added area/api area/distribution kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration area/ux labels Oct 17, 2023
@vvoland vvoland added this to the 25.0.0 milestone Oct 17, 2023
@vvoland vvoland changed the title c8d/pull: Wrap no basic auth error c8d/pull: Don't wrap no basic auth error Oct 26, 2023
@vvoland vvoland force-pushed the c8d-pull-access-denied-msg-2 branch from 06a9323 to 5b85d12 Compare October 26, 2023 13:20
@vvoland vvoland self-assigned this Oct 26, 2023
@@ -124,6 +125,9 @@ func (i *ImageService) PullImage(ctx context.Context, ref reference.Named, platf
img, err := i.client.Pull(ctx, ref.String(), opts...)
if err != nil {
if errors.Is(err, docker.ErrInvalidAuthorization) {
if strings.Contains(err.Error(), "no basic auth credentials") {
Copy link
Member

Choose a reason for hiding this comment

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

one last nit; can you add a comment with a link to the containerd code, so that we have some reference what we're trying to match (to give it some context)?

can probably use a permalink to the tag;
https://github.com/containerd/containerd/blob/v1.7.8/remotes/docker/authorizer.go#L189-L191

Don't wrap the `no basic auth credentials` error from containerd and
return it as-is.

The error will look like:
```
failed to resolve reference "docker.io/library/aodkoakds:latest": pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed
```

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland force-pushed the c8d-pull-access-denied-msg-2 branch from 5b85d12 to df34db1 Compare October 30, 2023 08:40
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah merged commit dcf7287 into moby:master Oct 30, 2023
121 of 138 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api area/distribution area/ux containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs
Projects
Development

Successfully merging this pull request may close these issues.

3 participants