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 active gcloud credentials for executing cloudbuild when available #2731

Merged
merged 1 commit into from
Aug 28, 2019

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Aug 27, 2019

This change ensures that skaffold always tries to use a user's active gcloud credentials when making calls through the Cloud Build and Cloud Storage client libraries, instead of requiring that the user has Application Default Credentials created on their host machine. Unfortunately the only way these credentials can be accessed is through shelling out to gcloud, so that's what we do here.

Fixes #1966

@nkubala nkubala force-pushed the gcb-creds branch 2 times, most recently from ddd8051 to 392ed26 Compare August 28, 2019 00:59
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

LGTM after two nits

pkg/skaffold/gcp/auth.go Outdated Show resolved Hide resolved
pkg/skaffold/gcp/auth.go Outdated Show resolved Hide resolved
logrus.Infof("unable to retrieve google creds: %s", err.Error())
logrus.Infof("falling back to application default credentials")
}
_, err = c.TokenSource.Token()
Copy link
Contributor

Choose a reason for hiding this comment

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

this line might panic - if CredentialsFromJSON results in error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, added a return here

@@ -349,6 +349,7 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T
google.golang.org/api v0.4.0/go.mod h1:8k5glujaEP+g9n7WNsDg8QP6cUVNI86fCNMcbazEtwE=
google.golang.org/api v0.7.0 h1:9sdfJOzWlkqPltHAuzT2Cp+yrBeY1KRVYgms8soxMwM=
google.golang.org/api v0.7.0/go.mod h1:WtwebWUNSVBH/HAw79HIFXZNqEvBhG+Ra+ax0hx3E3M=
google.golang.org/api v0.9.0 h1:jbyannxz0XFD3zdjgrSUsaJbgpH4eTrkdhRChkHPfO8=
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we need this update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷‍♂️

@codecov
Copy link

codecov bot commented Aug 28, 2019

Codecov Report

Merging #2731 into master will decrease coverage by 0.28%.
The diff coverage is 0%.

Impacted Files Coverage Δ
pkg/skaffold/build/gcb/cloud_build.go 0% <0%> (ø) ⬆️
pkg/skaffold/sources/upload.go 0% <0%> (ø) ⬆️
pkg/skaffold/gcp/auth.go 37.83% <0%> (-62.17%) ⬇️
pkg/skaffold/build/cluster/sources/gcs.go 0% <0%> (ø) ⬆️
pkg/skaffold/gcp/client.go 0% <0%> (ø)

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

I'm gonna let the test coverage decrease slip this time as we need this for the release tomorrow...
I would love to see unit tests covering the active vs ADC logic as a follow-up though.

@balopat balopat merged commit 8a7c8a8 into GoogleContainerTools:master Aug 28, 2019
@nkubala nkubala deleted the gcb-creds branch June 17, 2021 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CloudBuild uses application default credentials ONLY
3 participants