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: Return same access denied error as graphdrivers #46656

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Oct 16, 2023

- What I did

- How I did it

- How to verify it
Fixes ContainerCollectionTest.test_run_with_image_that_does_not_exist from docker-py

Partially fixes TestImagePullNonExisting

$ make TEST_FILTER='TestImagePullNonExisting'   TEST_IGNORE_CGROUP_CHECK=1 DOCKER_BUILDKIT=0   DOCKER_GRAPHDRIVER=overlayfs TEST_INTEGRATION_USE_SNAPSHOTTER=1    test-integration
(...)
    --- FAIL: TestImagePullNonExisting/library/asdfasdf:foobar (0.02s)
    --- FAIL: TestImagePullNonExisting/library/asdfasdf:latest (0.02s)
    --- FAIL: TestImagePullNonExisting/asdfasdf:latest (0.02s)
    --- FAIL: TestImagePullNonExisting/asdfasdf:foobar (0.03s)
-    --- FAIL: TestImagePullNonExisting/library/asdfasdf (0.02s)
-    --- FAIL: TestImagePullNonExisting/asdfasdf (0.02s)
+    --- PASS: TestImagePullNonExisting/library/asdfasdf (1.00s)
+    --- PASS: TestImagePullNonExisting/asdfasdf (1.01s)
FAIL

- Description for the changelog

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

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland added area/distribution status/2-code-review kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration labels Oct 16, 2023
@vvoland vvoland added this to the 25.0.0 milestone Oct 16, 2023
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.

Thanks, I hate it 😂

LGTM

@thaJeztah thaJeztah merged commit 79521da into moby:master Oct 16, 2023
119 of 138 checks passed
@@ -120,6 +123,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) {
Copy link
Member

Choose a reason for hiding this comment

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

This will clash with containerd/containerd#9234

maybe we should wrap the returned error?

Copy link
Member

Choose a reason for hiding this comment

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

doh! forgot about that one; I guess we need to revert this one when we bump to a containerd vendor with that patch

Copy link
Member

Choose a reason for hiding this comment

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

I guess the check will be even uglier, we'll have to check the error message itself and return the right error

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't we still be able to match the ErrInvalidAuthorization that it wraps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened: #46662

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/distribution containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs status/2-code-review
Projects
Development

Successfully merging this pull request may close these issues.

3 participants