Use gcloud oauth token before trying application default credentials #198
Conversation
… before trying application default credentials.
proxy/util/gcloudutil.go
Outdated
| cmd.Stdout = buf | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| if strings.Contains(err.Error(), "executable file not found") { |
There was a problem hiding this comment.
Do you still need this check, given that you have already called exec.LookPath()?
There was a problem hiding this comment.
I initially figured it was easier to leave it in place. But it feels like its more likely to hide bugs in the future, so removing it...
proxy/util/gcloudutil.go
Outdated
| } | ||
|
|
||
| data := &GcloudConfigData{} | ||
|
|
| "golang.org/x/oauth2" | ||
| ) | ||
|
|
||
| // GcloudConfigData represents the data returned by `gcloud config config-helper`. |
There was a problem hiding this comment.
Since the format of this output can change anytime, is there something that we can do to guard against that?
There was a problem hiding this comment.
In this case, the gcloud config config-helper is effectively a supported API, so luckily we can depend on the format of the output.
| ) | ||
|
|
||
| type GcloudError struct { | ||
| GcloudError error |
There was a problem hiding this comment.
Do these two members need to be exported? Could we instead have a Status() method on the GcloudError type which returns the status. I don't see the underlying error object directly used anywhere.
proxy/util/gcloudutil.go
Outdated
| data := &GcloudConfigData{} | ||
|
|
||
| if err := json.Unmarshal(buf.Bytes(), data); err != nil { | ||
| logging.Errorf("Failed to unmarshal bytes from gcloud: %v", err) |
There was a problem hiding this comment.
There's a tott about logging errors or returning them, not both. It's called like "contextful logs", and suggests annotating errors you return rather than printing and returning errors.
There was a problem hiding this comment.
Yeah, agreed.
In this case this function is an extraction of the function that invoked gcloud before, so I left the logging/return error behavior as is.
The intent here is that its likely reasonable for individuals to run the proxy, but if they are using the proxy as a library, we do still want to encourage them to use a service account, since that is more reliable for production configurations
|
Submitting |
| } | ||
|
|
||
| buf := new(bytes.Buffer) | ||
| cmd := exec.Command(gcloudCmd, "--format", "json", "config", "config-helper") |
There was a problem hiding this comment.
Found some issue of this.
This gcloud command doesn't seem to respect GOOGLE_APPLICATION_CREDENTIALS. I have Google Cloud SDK 221.0.0.
So assuming the following setup: user starts the proxy in GCE/GKE with GOOGLE_APPLICATION_CREDENTIALS set and that points to a service account in a different project; user does not use -credential_file because according to https://cloud.google.com/sql/docs/mysql/sql-proxy#environment, it is identical to supplying a credential file on the command line. However in this case GCE default service account will be used to start the proxy and it may not have proper permission of the cloud sql instance.
Either this is an unintended result, or documentation needs to be adapted.
There was a problem hiding this comment.
This was unintended, but I'll update the documentation instead of reverting it.
There was a problem hiding this comment.
It seems the order described in doc is still not reflecting code:
Credentials supplied in the proxy invocation command
Credentials supplied by the local environment
Credentials associated with the Compute Engine instance
Credentials from an authenticated Cloud SDK client
There was a problem hiding this comment.
We've just hit the same bug @honnix is talking about. I was about to raise an issue for it as it caused us quite a bit of time loss to figure out what was going on. Especially as in our scenario we cloud_sql_proxy first then about 2 seconds later in another command we're doing gcloud auth login, but that still seems to impact and cause the proxy to fail
gcloud and application default credentials use different GCP projects for API quota purposes. Generally for development, or if customers want to use their own credentials for audit logging purposes, we want customers to use the auth token that is setup with
gcloud auth login, and in particular we want to recommend that application default credentials be associated with service accounts as opposed to personal identities.There are a couple of changes to how we detect and use the gcloud active project in this PR as well:
gcloud.cmd