Skip to content

Default to gcloud auth#3282

Merged
nkubala merged 1 commit intoGoogleContainerTools:masterfrom
dgageot:default-to-gcloud-auth
Dec 13, 2019
Merged

Default to gcloud auth#3282
nkubala merged 1 commit intoGoogleContainerTools:masterfrom
dgageot:default-to-gcloud-auth

Conversation

@dgageot
Copy link
Copy Markdown
Contributor

@dgageot dgageot commented Nov 22, 2019

Make sure that gcloud doesn't need to be configured as a helper to be used
to push to/pull from *.gcr.io

This used to be the case but using GetAllAuthConfigs() over GetAuthConfig(registry string) broke this behaviour.

Also use the gcloud auth code from go-containerregistry

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 22, 2019

Codecov Report

Merging #3282 into master will decrease coverage by 0.01%.
The diff coverage is 64.28%.

Impacted Files Coverage Δ
pkg/skaffold/gcp/auth.go 28.12% <100%> (-9.72%) ⬇️
pkg/skaffold/docker/auth.go 56.14% <50%> (+0.37%) ⬆️
pkg/skaffold/server/server.go 58.57% <0%> (ø) ⬆️

@dgageot dgageot force-pushed the default-to-gcloud-auth branch 2 times, most recently from 53e0172 to 7aafc03 Compare November 23, 2019 08:25
@balopat
Copy link
Copy Markdown
Contributor

balopat commented Nov 26, 2019

This is failing with gcloud auth errors on integration tests. PTAL @dgageot

@dgageot dgageot force-pushed the default-to-gcloud-auth branch from 7aafc03 to bfa86b6 Compare November 27, 2019 08:43
@dgageot
Copy link
Copy Markdown
Contributor Author

dgageot commented Nov 27, 2019

@balopat I cut the PR in two pieces. The other part needs some more work. This part passes the tests

Copy link
Copy Markdown
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 one positive from the previous code was that the credential-helpers were only configured if the registry was a gcr.io repo. And we logged that we were auto-configuring it (since it wasn't in their docker config.json).

Should we also checking for docker-credential-gcr too?

func (credsHelper) GetAuthConfig(registry string) (types.AuthConfig, error) {
cf, err := loadDockerConfig()
if err != nil {
return types.AuthConfig{}, err
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This means a broken config.json will cause skaffold to abort. Maybe we should log an error, but continue on?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

1/ docker-credential-gcloud should be favored. See https://github.com/GoogleCloudPlatform/docker-credential-gcr "For normal development setups, users are encouraged to use gcloud auth configure-docker, instead"
2/ In the docker build code, we ignore the error, like the Docker CLI does it.
3/ We can't just configure it for gcr.io when GetAllAuthConfigs() is used. When GetAuthConfig is used, configuring the helper for *.gcr.io has the exact same effect

@dgageot dgageot force-pushed the default-to-gcloud-auth branch from bfa86b6 to ef6fc8b Compare November 27, 2019 18:41
@dgageot dgageot force-pushed the default-to-gcloud-auth branch from ef6fc8b to 4c76fdf Compare December 10, 2019 06:15
Signed-off-by: David Gageot <david@gageot.net>
@dgageot dgageot force-pushed the default-to-gcloud-auth branch from 4c76fdf to 91073a0 Compare December 13, 2019 05:29
Copy link
Copy Markdown
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

I won't lie, I'm always a bit hesitant with auth changes, but everything looks fine to my eye. let's merge this and then make sure and monitor the next release to make sure we're not breaking existing users.

@nkubala nkubala merged commit ad9e982 into GoogleContainerTools:master Dec 13, 2019
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.

5 participants