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

When pulling images and authentication fails, first try anonymous pull #4451

Merged
merged 1 commit into from
Jul 11, 2020

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Jul 7, 2020

Fixes #4447

Signed-off-by: David Gageot david@gageot.net

@dgageot dgageot requested a review from a team as a code owner July 7, 2020 19:51
@dgageot dgageot requested a review from MarlonGamez July 7, 2020 19:51
@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #4451 into master will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4451      +/-   ##
==========================================
- Coverage   72.26%   72.26%   -0.01%     
==========================================
  Files         331      332       +1     
  Lines       12863    12878      +15     
==========================================
+ Hits         9296     9306      +10     
- Misses       2975     2978       +3     
- Partials      592      594       +2     
Impacted Files Coverage Δ
pkg/skaffold/build/buildpacks/lifecycle.go 82.71% <0.00%> (-1.04%) ⬇️
pkg/skaffold/docker/image.go 76.00% <0.00%> (+0.44%) ⬆️
pkg/skaffold/build/buildpacks/fetcher.go 64.28% <64.28%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6af3198...1a0b117. Read the comment docs.

Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

This is an improvement but it still has a puzzling effect:

Generating tags...
 - skaffold-buildpacks -> skaffold-buildpacks:v1.12.0-34-g6c8ef32c1
Checking cache...
 - skaffold-buildpacks: Not found. Building
Found [minikube] context, using local docker daemon.
Building [skaffold-buildpacks]...
ERROR: (gcloud.auth.docker-helper) You do not currently have an active account selected.
Please run:

  $ gcloud auth login

to obtain new credentials, or if you have already logged in with a
different account:

  $ gcloud config set account ACCOUNT

to select an already authenticated account to use.
v1: Pulling from buildpacks/builder
ab0f59294661: Already exists 
c2ae8b336f77: Already exists 
3c2cba919283: Already exists 
55b4247b83ec: Pulling fs layer 
2a71cd660153: Pulling fs layer 
39b3e94ed04c: Pulling fs layer 
3461ea70964b: Waiting 
[...]

@dgageot
Copy link
Contributor Author

dgageot commented Jul 7, 2020 via email

@briandealwis
Copy link
Member

I was hoping the following would work (try as public, and then with auth), except it fails with my private repository on Docker Hub when I'm logged out:

--- pkg/skaffold/docker/image.go
+++ pkg/skaffold/docker/image.go
@@ -312,17 +312,11 @@ func (l *localDaemon) isAlreadyPushed(ctx context.Context, ref, registryAuth str
 
 // Pull pulls an image reference from a registry.
 func (l *localDaemon) Pull(ctx context.Context, out io.Writer, ref string) error {
-       // Eargerly create credentials.
-       registryAuth, err := l.encodedRegistryAuth(ctx, DefaultAuthHelper, ref)
-       // Let's ignore the error because maybe the image is public
-       // and can be pulled without credentials.
-
        rc, err := l.apiClient.ImagePull(ctx, ref, types.ImagePullOptions{
-               RegistryAuth: registryAuth,
                PrivilegeFunc: func() (string, error) {
                        // If the first pull is unauthorized, retry without ignoring the
                        // authentication error.
-                       return registryAuth, err
+                       return l.encodedRegistryAuth(ctx, DefaultAuthHelper, ref)
                },
        })
        if err != nil {

The failure happens in ImagePull() the tryImageCreate() returns an github.com/docker/docker/errdefs.ErrNotFound error, not an ErrUnauthorized.

@dgageot
Copy link
Contributor Author

dgageot commented Jul 7, 2020 via email

Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

So that gcloud error is disconcerting, though (1) it's no different from what the user sees if they use docker pull, and (2) it really is a user problem since they have configured the docker-credential-gcloud as a helper, but they're not actually logged in.

pkg/skaffold/docker/image.go Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 11, 2020
@dgageot dgageot merged commit df9df6c into GoogleContainerTools:master Jul 11, 2020
briandealwis pushed a commit that referenced this pull request Jul 13, 2020
…ling. (#4451)

Fixes #4447

Signed-off-by: David Gageot <david@gageot.net>
@briandealwis briandealwis added this to the v1.12.1 milestone Jul 14, 2020
briandealwis pushed a commit that referenced this pull request Jul 14, 2020
…ling. (#4451)

Fixes #4447

Signed-off-by: David Gageot <david@gageot.net>
@briandealwis briandealwis mentioned this pull request Jul 14, 2020
@briandealwis
Copy link
Member

This came up again in Sean's friction log. We can suppress the output with CLOUDSDK_CORE_VERBOSITY=none and the error message is still meaningful enough for the user to figure out what's going on:

diff --git examples/buildpacks/skaffold.yaml examples/buildpacks/skaffold.yaml
index d78c0a329..4938c8f14 100644
--- examples/buildpacks/skaffold.yaml
+++ examples/buildpacks/skaffold.yaml
@@ -4,7 +4,7 @@ build:
   artifacts:
   - image: skaffold-buildpacks
     buildpacks:
-      builder: "gcr.io/buildpacks/builder:v1"
+      builder: "gcr.io/<private-project>/first"
       env:
       - GOPROXY={{.GOPROXY}}
     sync:
$  CLOUDSDK_CORE_VERBOSITY=none skaffold build
Generating tags...
 - skaffold-buildpacks -> skaffold-buildpacks:v1.13.1-50-g456977b80-dirty
Checking cache...
 - skaffold-buildpacks: Not found. Building
Found [minikube] context, using local docker daemon.
Building [skaffold-buildpacks]...
couldn't build "skaffold-buildpacks": failed to fetch builder image 'gcr.io/<private-project>/first:latest': pulling image from repository: Error response from daemon: unauthorized: You don't have the needed permissions to perform this operation, and you may have invalid credentials. To authenticate your request, follow the steps in: https://cloud.google.com/container-registry/docs/advanced-authentication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fetching the gcr.io/buildpacks/builder:v1 image should not require being logged in to gcloud
4 participants