Add ability to use public GCR repos without being authenticated #1140

samos123 · 2020-03-16T21:55:47Z

Apply lazy configuration of docker credential helper for GCR at run-time only when GCR is used as the destination.

Fixes #1122

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes unit tests
Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

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

Release Notes

Add ability to use public GCR images without being authenticated to GCR

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.

pkg/executor/push.go

Kaniko by default used to configure the GCR credential helper however this caused Kaniko to fail when trying to use a base image from a public GCR image. This patch makes it possible to use public GCR images as base image when using docker even when you're not authenticated to GCR. Co-authored-by: Nate Williams <nate.williams@files.com>

googlebot · 2020-03-17T16:12:16Z

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.

samos123 · 2020-03-17T16:14:53Z

@filesnate I added you as co-author since you made the original PR and I just improved on it. If that's fine with you, please add a comment here with following message: "@googlebot I consent."

Also thank you for the thorough reviews and testing to make sure it works for your use case as well.

filesnate · 2020-03-17T16:44:00Z

@googlebot I consent.

googlebot · 2020-03-17T16:44:05Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

filesnate · 2020-03-17T16:44:40Z

@filesnate I added you as co-author since you made the original PR and I just improved on it. If that's fine with you, please add a comment here with following message: "@googlebot I consent."

Also thank you for the thorough reviews and testing to make sure it works for your use case as well.

Thank YOU for doing the heavy lifting in the go code that I way less familiar with!

tejal29 · 2020-03-17T21:32:13Z

pkg/executor/push.go

+		// Historically kaniko was pre-configured by default with gcr credential helper,
+		// in here we keep the backwards compatibility by enabling the GCR helper only
+		// when gcr.io is in one of the destinations.
+		if strings.Contains(destRef.RegistryStr(), "gcr.io") {


Can you please test to push to a public GCR repo one needs to be authenticated? My gut is mostly yes in which case this is good. If not, then we might need to add this is documentation.

It would be great if you can add tests for this function.

You can declare 2 variables

// for testing var ( execute = execCommad checkRemotePushPermissions = remote.CheckPushPermission stat = os.Stat )

In your tests, you could create mock for these and make sure,

when mockOsStat return os.ErrNotExist, execute is called

when mockOsStat return nil and some random fileinfo, execute is not called.

I added the testing. I noticed we were already using afero in push.go and push_test.go so there wasn't a need to mock os.Stat. Thank you so much for detailed recommendations on how to test, really helpful.

There is no way to make a GCR repo pushable by the public. You can only push if you're explicitly given permission to push to a GCR repo. For reference: https://cloud.google.com/container-registry/docs/access-control or did you mean something else?

pkg/executor/push.go

tejal29

Add tests and 1 nit.

tejal29

You will also need to remove the DOCKER_CREDENTIAL_GCR_CONFIG from all the Dockerfiles.

samos123 · 2020-03-21T23:35:59Z

DOCKER_CREDENTIAL_GCR_CONFIG should be left in the Dockerfiles since we're not setting it in push.go. It will only be used when docker-credential-gcr configure-docker is run through push.go. Setting the environment variable in push.go would be less clean imo.

pkg/executor/push_test.go

tejal29 · 2020-03-24T01:01:33Z

I just realized this will remove file/kaniko/.docker/config.json in user workspace if it previously existed as a side effect. - ofc a rare case.
It would be ideal if for testing,

we could override the variable,
create a config file using ioutil.TempFile and then
delete it in defer.

samos123 · 2020-03-24T17:31:45Z

/kaniko/.docker/config.json won't get deleted since afero creates membased temporary mock FS during unit test. In addition execCommand is stubbed out with a fake exec command during unit tests.

googlebot added the cla: yes CLA signed by all commit authors label Mar 16, 2020

samos123 changed the title ~~Fix 1122 public gcr~~ Add ability to use public GCR repos without being authenticated Mar 16, 2020

samos123 requested a review from tejal29 March 16, 2020 21:56

samos123 mentioned this pull request Mar 16, 2020

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

Closed

2 tasks

filesnate reviewed Mar 16, 2020

View reviewed changes

pkg/executor/push.go Show resolved Hide resolved

samos123 requested a review from cvgw March 17, 2020 00:58

filesnate approved these changes Mar 17, 2020

View reviewed changes

samos123 and others added 2 commits March 17, 2020 09:09

Add comments

6626869

samos123 force-pushed the fix-1122-public-gcr branch from c452326 to 6626869 Compare March 17, 2020 16:12

googlebot added cla: no CLA not signed by all commit authors and removed cla: yes CLA signed by all commit authors labels Mar 17, 2020

googlebot added cla: yes CLA signed by all commit authors and removed cla: no CLA not signed by all commit authors labels Mar 17, 2020

tejal29 reviewed Mar 17, 2020

View reviewed changes

pkg/executor/push.go Outdated Show resolved Hide resolved

tejal29 requested changes Mar 17, 2020

View reviewed changes

tejal29 reviewed Mar 18, 2020

View reviewed changes

Add unit tests for CheckPushPermissions

8518b37

samos123 added 2 commits March 21, 2020 16:45

Create constant for Docker conf location

c56f16b

Cleanup push_test CheckPushPermission unit tests

c2393df

tejal29 reviewed Mar 23, 2020

View reviewed changes

pkg/executor/push_test.go Show resolved Hide resolved

Test: Remove DockerConf after CheckPushPermission

1fb1edd

tejal29 approved these changes Mar 23, 2020

View reviewed changes

samos123 merged commit 1b662e7 into GoogleContainerTools:master Mar 24, 2020

ddgenome mentioned this pull request May 4, 2020

GCR credential helper check does not respect DOCKER_CONFIG environment variable #1228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to use public GCR repos without being authenticated #1140

Add ability to use public GCR repos without being authenticated #1140

samos123 commented Mar 16, 2020

googlebot commented Mar 17, 2020

samos123 commented Mar 17, 2020 •

edited

Loading

filesnate commented Mar 17, 2020

googlebot commented Mar 17, 2020

filesnate commented Mar 17, 2020

tejal29 Mar 17, 2020 •

edited

Loading

samos123 Mar 21, 2020

tejal29 left a comment

tejal29 left a comment

samos123 commented Mar 21, 2020

tejal29 commented Mar 24, 2020

samos123 commented Mar 24, 2020

Add ability to use public GCR repos without being authenticated #1140

Add ability to use public GCR repos without being authenticated #1140

Conversation

samos123 commented Mar 16, 2020

googlebot commented Mar 17, 2020

samos123 commented Mar 17, 2020 • edited Loading

filesnate commented Mar 17, 2020

googlebot commented Mar 17, 2020

filesnate commented Mar 17, 2020

tejal29 Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

samos123 Mar 21, 2020

Choose a reason for hiding this comment

tejal29 left a comment

Choose a reason for hiding this comment

tejal29 left a comment

Choose a reason for hiding this comment

samos123 commented Mar 21, 2020

tejal29 commented Mar 24, 2020

samos123 commented Mar 24, 2020

samos123 commented Mar 17, 2020 •

edited

Loading

tejal29 Mar 17, 2020 •

edited

Loading