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

Default to gcloud auth #3282

Merged
merged 1 commit into from
Dec 13, 2019

Conversation

dgageot
Copy link
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

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
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
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
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
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
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
@dgageot dgageot force-pushed the default-to-gcloud-auth branch from 4c76fdf to 91073a0 Compare December 13, 2019 05:29
Copy link
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