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

Use a blank docker config file to avoid issues with the requiring credentials for public gcr.io images #1125

Closed

Conversation

filesnate
Copy link
Contributor

@filesnate filesnate commented Mar 11, 2020

Use a blank docker config file to avoid issues with the requiring credentials for public gcr.io images

  • Users who require credentials will need to supply their own config.json file anyway,
    so let them set it up as needed.

Fixes #1122.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Create a blank docker config.json file to allow pulling public images hosted on *.gcr.io without credentials.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no CLA not signed by all commit authors label Mar 11, 2020
@filesnate
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

…ault config.json which requires credentials accessing gcr.io registries

- Users who require credentials will need to supply their own config.json file anyway,
  so let them set it up as needed.
@@ -27,6 +27,8 @@ RUN make -C /go/src/github.com/awslabs/amazon-ecr-credential-helper linux-amd64
# ACR docker credential helper
ADD https://aadacr.blob.core.windows.net/acr-docker-credential-helper/docker-credential-acr-linux-amd64.tar.gz /usr/local/bin
RUN tar -C /usr/local/bin/ -xvzf /usr/local/bin/docker-credential-acr-linux-amd64.tar.gz
# Blank out the credentials file
RUN echo "{}" > /root/.docker/config.json
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe a better approach would be to remove the line RUN docker-credential-gcr configure-docker which is the line that adds .docker/config.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't do that, as then kaniko blows up when it tries to read a non-existent config.json file. :(

[ Been there, done that, got the scars to prove it. ]

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thanks for that extra info. @tejal29 your thoughts?

Copy link
Contributor

@samos123 samos123 left a comment

Choose a reason for hiding this comment

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

Thank you for figuring out the issue that I also hit previously! I wasn't able to figure out exactly where the config was coming from before.

In order to test this, could you remove the code in the integration tests that replaces public GCR images with images from the local docker registry? Basically this whole source file should be removed and all the code that uses any of the functions in that file: https://github.com/GoogleContainerTools/kaniko/blob/master/integration/migrate_gcr.go

I'm also happy to take over this PR if you get stuck.

@filesnate
Copy link
Contributor Author

@samos123 - Feel free to rewrite the go code, I'm just trying to get a kaniko image that I can use to pull/build images that are publically available on gcr.io. If we could move this forward to get a working build, and then as a follow PR rewrite the migrate_gcr.go code as a new PR, I think that would most optimal for me.

Previously there was a need to import public GCR images into the local
docker registry and replace the base images to use the local docker
registry. This is no longer needed since Kaniko now works with public
GCR images also for unauthenticated users.
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no CLA not signed by all commit authors and removed cla: yes CLA signed by all commit authors labels Mar 14, 2020
@samos123
Copy link
Contributor

samos123 commented Mar 14, 2020

I've added a commit to this PR since I prefer to ensure the PR as a whole is tested with an integration test. Our previous integration tests didn't test public GCR images, but the commit I just added removes the migrate_gcr.go source, which causes the integration test to test kaniko with a public GCR image.

Could you leave a comment with the following content: "@googlebot I consent." so that I'm allowed to add the commit to this PR? Appreciate your patience and taking the time to respond to all my questions!

Copy link
Contributor

@samos123 samos123 left a comment

Choose a reason for hiding this comment

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

This will break existing GCR users that are using kaniko likes this:

docker run -v /home/stoelinga/.config/gcloud:/root/.config/gcloud -v $PWD/sam/public_gcr:/workspace gcr.io/gsam-123/kaniko-executor:debug-filesnate --dockerfile Dockerfile --destination gcr.io/gsam-123/test-gcr:latest --context dir:///workspace/
error checking push permissions -- make sure you entered the correct tag name, and that you are authenticated correctly, and try again: checking push permission for "gcr.io/gsam-123/test-gcr:latest": creating push check transport for gcr.io failed: 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

In master branch this works as expected however after building kaniko-executor from this PR it fails with authentication issue. This fix requires some more work and discussion to make sure we're not breaking the users that use GCR with Kaniko today.

For reference here is the exact same command using kaniko-executor from latest master, which works as expected with GCR and correct credentials:

docker run -v /home/stoelinga/.config/gcloud:/root/.config/gcloud -v $PWD/sam/public_gcr:/workspace gcr.io/gsam-123/kaniko-executor:master --dockerfile Dockerfile --destination gcr.io/gsam-123/test-gcr:latest --context dir:///workspace/
INFO[0000] Resolved base name gcr.io/distroless/python2.7:debug to gcr.io/distroless/python2.7:debug
INFO[0000] Resolved base name gcr.io/distroless/python2.7:debug to gcr.io/distroless/python2.7:debug
INFO[0000] Retrieving image manifest gcr.io/distroless/python2.7:debug
INFO[0001] Retrieving image manifest gcr.io/distroless/python2.7:debug
INFO[0003] Built cross stage deps: map[]
INFO[0003] Retrieving image manifest gcr.io/distroless/python2.7:debug
INFO[0003] Retrieving image manifest gcr.io/distroless/python2.7:debug
INFO[0004] Unpacking rootfs as cmd COPY a /a requires it.
INFO[0009] Taking snapshot of full filesystem...
INFO[0015] Resolving paths
INFO[0015] COPY a /a
INFO[0015] Resolving paths
INFO[0015] Taking snapshot of files...

@filesnate
Copy link
Contributor Author

@googlebot I consent.

@tejal29
Copy link
Member

tejal29 commented Mar 16, 2020

This will break existing GCR users that are using kaniko likes this:

docker run -v /home/stoelinga/.config/gcloud:/root/.config/gcloud -v $PWD/sam/public_gcr:/workspace gcr.io/gsam-123/kaniko-executor:debug-filesnate --dockerfile Dockerfile --destination gcr.io/gsam-123/test-gcr:latest --context dir:///workspace/
error checking push permissions -- make sure you entered the correct tag name, and that you are authenticated correctly, and try again: checking push permission for "gcr.io/gsam-123/test-gcr:latest": creating push check transport for gcr.io failed: 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

In master branch this works as expected however after building kaniko-executor from this PR it fails with authentication issue. This fix requires some more work and discussion to make sure we're not breaking the users that use GCR with Kaniko today.

For reference here is the exact same command using kaniko-executor from latest master, which works as expected with GCR and correct credentials:

docker run -v /home/stoelinga/.config/gcloud:/root/.config/gcloud -v $PWD/sam/public_gcr:/workspace gcr.io/gsam-123/kaniko-executor:master --dockerfile Dockerfile --destination gcr.io/gsam-123/test-gcr:latest --context dir:///workspace/
INFO[0000] Resolved base name gcr.io/distroless/python2.7:debug to gcr.io/distroless/python2.7:debug
INFO[0000] Resolved base name gcr.io/distroless/python2.7:debug to gcr.io/distroless/python2.7:debug
INFO[0000] Retrieving image manifest gcr.io/distroless/python2.7:debug
INFO[0001] Retrieving image manifest gcr.io/distroless/python2.7:debug
INFO[0003] Built cross stage deps: map[]
INFO[0003] Retrieving image manifest gcr.io/distroless/python2.7:debug
INFO[0003] Retrieving image manifest gcr.io/distroless/python2.7:debug
INFO[0004] Unpacking rootfs as cmd COPY a /a requires it.
INFO[0009] Taking snapshot of full filesystem...
INFO[0015] Resolving paths
INFO[0015] COPY a /a
INFO[0015] Resolving paths
INFO[0015] Taking snapshot of files...

@samos123 can you please change your command to also mount the ~/.docker/config.json at kaniko/.docker and try again?

@tejal29
Copy link
Member

tejal29 commented Mar 16, 2020

This PR might break "GCB Kaniko" users or anyother users who mount their Cloud config only and not docker config to push to GCR e.g. https://cloud.google.com/cloud-build/docs/kaniko-cache
So, in order to not break users who don't mount ~/.config/gcloud/ we could

  1. automatically determine, if ~/.config/gcloud is preset re-write empty docker credentials.

This approach could be tricky and we will need to test with GCB since GCB Kaniko usage docs don't explicitly mount google cloud config, so we need to look for file to be present.

  1. Or document the reverse, i.e. if you have public images, please mount an empty .docker/config for now untill we can investigate with approach 1.

WDYT?

@filesnate
Copy link
Contributor Author

IMO, that it's been working to-date for some folks is completely un-documented, and a side-effect of a default configuration that wasn't obvious until recently.

I also understand the pain of breaking something that was working for reasons (whatever they may be, the person who's software was working yesterday and breaks today doesn't care). But, I also hate 'magic behavior' where it's not obvious why it's working.

Therefore, I'm of the mind-set that creating a bunch of workarounds to fix it so things continue to work (meaning just continuing the 'magic' behavior) is the wrong solution, since the software is hiding important configuration details from them. IMO, eventually some part of the software will eventually change, requiring the configuration to be changed, so by not solving the problem now, you're just delaying the inevitable issue of requiring the user to configure docker authentication.

So, you can either deal with it now while it's fresh in your mind with the understanding of how to solve it, or put it off to future dev and hope they understand the problem as well as you do now.

@tejal29
Copy link
Member

tejal29 commented Mar 16, 2020

IMO, that it's been working to-date for some folks is completely un-documented, and a side-effect of a default configuration that wasn't obvious until recently.

I also understand the pain of breaking something that was working for reasons (whatever they may be, the person who's software was working yesterday and breaks today doesn't care). But, I also hate 'magic behavior' where it's not obvious why it's working.

Therefore, I'm of the mind-set that creating a bunch of workarounds to fix it so things continue to work (meaning just continuing the 'magic' behavior) is the wrong solution, since the software is hiding important configuration details from them. IMO, eventually some part of the software will eventually change, requiring the configuration to be changed, so by not solving the problem now, you're just delaying the inevitable issue of requiring the user to configure docker authentication.

So, you can either deal with it now while it's fresh in your mind with the understanding of how to solve it, or put it off to future dev and hope they understand the problem as well as you do now.

Thanks @filesnate. Yes i understand the "fixing" it the right way. I just feel a lot fo folks are using kaniko in production pipeline as part of CI jobs. I am definitely up for fixing this the bold way by forcing users to fix their config if they relied on the docker credentials to be initialized.
However, before we do anything like this.

  1. Make sure we understand why it was added in the first place.
    • was it just for GCB users?
  2. Was this behavior change in the go-container registry?
    if yes, shd we fix this issue in go-container-registry library to try pulling/pushing even if credential helper isnt configured correctly?
  3. Users should have ample of notice before iff we decide to change the image.
    • maybe have a warning printed out they they should not relying on docker config initialized by kaniko executor.

After investigating 1 and 2, if we feel this is the right change, it would be great if we can first make change to add a deprecation notice in this PR.
Here is the code where Kaniko detects mount points info.

We could simply print a message if "/root/.config/gcloud" but not "/kaniko/.docker". The warning message printed would be

############## Warning!######################
We are removing initializing gcr credentials in kaniko executor image. 
Please add a volume mount for docker/config.json to add init GCR credentials helpers
############################################

Would you be up for investigating 1 and 2 bullet points?

Thanks
Tejal

@filesnate
Copy link
Contributor Author

I am definitely up for fixing this the bold way by forcing users to fix their config if they relied on the docker credentials to be initialized.
However, before we do anything like this.

  1. Make sure we understand why it was added in the first place.

    • was it just for GCB users?
  2. Was this behavior change in the go-container registry?
    if yes, shd we fix this issue in go-container-registry library to try pulling/pushing even if credential helper isnt configured correctly?
    ...
    Would you be up for investigating 1 and 2 bullet points?

I have no context to answer questions (1) and (2). I'm just a random dev who attempted to use kaniko to use it to build a publically available gcr.io docker image, and did a little bit of debugging to find the underlying issue, sorry.

Nate

@samos123
Copy link
Contributor

I've done an alternative approach that should work for @filesnate use case but won't break existing users that relied on kaniko being configured with gcr by default. This is the PR: #1140
@filesnate could you review and test that PR to make sure it works for you?

@filesnate
Copy link
Contributor Author

I've done an alternative approach that should work for @filesnate use case but won't break existing users that relied on kaniko being configured with gcr by default. This is the PR: #1140
@filesnate could you review and test that PR to make sure it works for you?

@samos123 - Is there an easy-to-use image tag I could use, or do I have to build a custom image with your PR?

@samos123
Copy link
Contributor

You can use the image I build at your own risk lol. This is the image: gcr.io/gsam-123/kaniko-executor:fix-1122-public-gcr. Please let me know if it works.

Also in the future you can easily build your own image using this:

REGISTRY=localhost:5000
make images
make push

That will build kaniko and push executor to your registry defined in the variable REGISTRY

See below how I used it to test:

docker run -v /home/stoelinga/.config/gcloud:/root/.config/gcloud -v $PWD/sam/public_gcr:/workspace gcr.io/gsam-123/kaniko-executor:fix-1122-public-gcr --dockerfile Dockerfile --destination localhost:5000/test-gcr:latest --context dir:///workspace/

@filesnate
Copy link
Contributor Author

You can use the image I build at your own risk lol. This is the image: gcr.io/gsam-123/kaniko-executor:fix-1122-public-gcr. Please let me know if it works.

Also in the future you can easily build your own image using this:

REGISTRY=localhost:5000
make images
make push

That will build kaniko and push executor to your registry defined in the variable REGISTRY

See below how I used it to test:

docker run -v /home/stoelinga/.config/gcloud:/root/.config/gcloud -v $PWD/sam/public_gcr:/workspace gcr.io/gsam-123/kaniko-executor:fix-1122-public-gcr --dockerfile Dockerfile --destination localhost:5000/test-gcr:latest --context dir:///workspace/

Perfect, thanks!

@filesnate
Copy link
Contributor Author

filesnate commented Mar 17, 2020

You can use the image I build at your own risk lol. This is the image: gcr.io/gsam-123/kaniko-executor:fix-1122-public-gcr. Please let me know if it works.

Also in the future you can easily build your own image using this:

REGISTRY=localhost:5000
make images
make push
sh-3.2$ export REGISTRY=localhost:5000
sh-3.2$ make
Makefile:89: *** target pattern contains no `%'.  Stop.

Someone used space in the push: target (neither standard Make nor GNU-Make likes it).

@filesnate
Copy link
Contributor Author

You can use the image I build at your own risk lol. This is the image: gcr.io/gsam-123/kaniko-executor:fix-1122-public-gcr. Please let me know if it works.
Also in the future you can easily build your own image using this:

REGISTRY=localhost:5000
make images
make push
sh-3.2$ export REGISTRY=localhost:5000
sh-3.2$ make
Makefile:89: *** target pattern contains no `%'.  Stop.

Someone used space in the push: target (neither standard Make nor GNU-Make likes it).

This has been corrected in master, so it's all good (was working on a PR to fix this, but @samos123 already fixed it. 😁 )

@filesnate
Copy link
Contributor Author

I've done an alternative approach that should work for @filesnate use case but won't break existing users that relied on kaniko being configured with gcr by default. This is the PR: #1140
@filesnate could you review and test that PR to make sure it works for you?

My local test for this works correctly using a locally built-image, so I say go for it!

@samos123
Copy link
Contributor

Thanks for testing. I'm going to close this PR. Let's continue the discussion in #1122 and the PR #1140

@samos123 samos123 closed this Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no CLA not signed by all commit authors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kaniko can't pull public images from gcr.io
4 participants