credentials: add compute engine creds #2708
Conversation
menghanl
changed the title
credentials/google/google.go
Outdated
| // by this API represents the GCE VM's default service account. | ||
| // | ||
| // This API is experimental. | ||
| func NewComputeEngineChannelCredentials() credentials.Bundle { |
There was a problem hiding this comment.
Done.
I am concerned that NewComputeEngineChannelCredentials will be confusingly similar to NewComputeEngineCredentials, but I'll leave this up to your call.
interop/client/client.go
Outdated
| ) | ||
|
|
||
| func main() { | ||
| flag.Parse() | ||
| var useGDC bool // use google default creds | ||
| var useGDC bool // use google default creds |
There was a problem hiding this comment.
For consistency, please either spell GDC out or abbreviate ComputeEngineChannelCreds in the next line :)
There was a problem hiding this comment.
Done. Abbreviated useComputeEngineChannelCreds to useCEC
interop/client/client.go
Outdated
| @@ -37,7 +37,8 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| googleDefaultCredsName = "google_default_credentials" | |||
| googleDefaultCredsName = "google_default_credentials" | |||
| computeEngineChannelCredsName = "compute_engine_channel_creds" | |||
There was a problem hiding this comment.
I assume "channel" is used here for interop compatibility across languages. If that's the case, maybe still s/Channel// in the code.
It's a little unfortunate that this option doesn't match "google_default_credentials" (no "channel", and "creds" has been abbreviated), but 🤷♂️.
There was a problem hiding this comment.
Makes sense, I made the s/Channel// change throughout the code, so that the only thing with "channel" in the name is the name of the test case and the custom_credentials_type option.
Personally I prefer to refer to this credentials type, across languages in general, as "compute engine channel credentials", at least in documentation and the test case name. Because "compute engine credentials" (without "channel") IMO is already taken as a name.
I agree that creds should probably be expanded out to credentials, though. That change will require changes outside of this PR though.
There was a problem hiding this comment.
I agree that creds should probably be expanded out to credentials, though. That change will require changes outside of this PR though.
I don't really care, since it's just some flags in a binary we use internally.
It's just unfortunate that we ended up with "Google Default Creds" and "Compute Engine Channel Creds", when they're both essentially the same thing.
credentials/google/google.go
Outdated
| newPerRPCCreds: func() credentials.PerRPCCredentials { | ||
| return oauth.NewComputeEngine() | ||
| }, | ||
| } | ||
| bundle, err := c.NewWithMode(internal.CredsBundleModeFallback) | ||
| if err != nil { | ||
| grpclog.Warningf("google default creds: failed to create new creds: %v", err) |
There was a problem hiding this comment.
This should be changed to "compute engine creds"?
dfawley
changed the title
This is the Go analogue of grpc/grpc-java#5475.
Manually tested that the interop test works in all environments.