credentials: support google default creds #2315

menghanl · 2018-09-20T00:55:51Z

Google default creds is a combo of ALTS, TLS and OAuth2. The right set of creds will be picked to use based on environment.

This PR contains:

A new creds.Bundle type
- changes to use it in ClientConn and transport
- dial option to set the bundle for a ClientConn
- balancer options and NewSubConnOption to set it for SubConn
Google default creds implementation by @cesarghali
grpclb changes to use different creds mode for different servers
interop client changes for google default creds testing

cesarghali · 2018-09-20T21:07:37Z

credentials/google/google.go

+ if !vmOnGCP {
+ return nil, errors.New("ALTS, as part of google default credentials, is only supported on GCP")
+ }
+ newCreds.transportCreds = alts.NewClientCreds(alts.DefaultClientOptions())


Can you please add a comment that Google Default Credentials are only use by clients, so we only create a new ALTS client creds, or something like that.

cesarghali · 2018-09-20T21:08:37Z

credentials/google/google.go

+ newCreds.transportCreds = credentials.NewTLS(nil)
+ case internal.CredsBundleModeALTS, internal.CredsBundleModeGRPCLB:
+ if !vmOnGCP {
+ return nil, errors.New("ALTS, as part of google default credentials, is only supported on GCP")


Do we need to prefix the error message with something like google default creds:, similar to the other error before.

cesarghali · 2018-09-20T21:08:46Z

credentials/google/google.go

+ }
+ newCreds.transportCreds = alts.NewClientCreds(alts.DefaultClientOptions())
+ default:
+ return nil, fmt.Errorf("unsupported mode: %v", mode)


Same RE the prefix.

cesarghali · 2018-09-20T21:11:46Z

dialoptions.go

+// WithTransportCredentials.
+//
+// This API is experimental.
+func WithCreds(b credentials.Bundle) DialOption {


How about changing the name to something like WithCredentialsBundle, or something more expressive. WithCreds might be a bit ambiguous about the creds type, e.g., the above API is for transport credentials.

cesarghali · 2018-09-20T21:17:59Z

internal/transport/http2_client.go

@@ -166,7 +166,19 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr TargetInfo, opts Conne
 isSecure bool
 authInfo credentials.AuthInfo
 )
- if creds := opts.TransportCredentials; creds != nil {
+ var creds credentials.TransportCredentials


Maybe call this transportCreds?

menghanl

Thanks for the review. PTAL.

menghanl · 2018-09-20T23:07:27Z

credentials/google/google.go

+ newCreds.transportCreds = credentials.NewTLS(nil)
+ case internal.CredsBundleModeALTS, internal.CredsBundleModeGRPCLB:
+ if !vmOnGCP {
+ return nil, errors.New("ALTS, as part of google default credentials, is only supported on GCP")


menghanl · 2018-09-20T23:10:50Z

credentials/google/google.go

+ if !vmOnGCP {
+ return nil, errors.New("ALTS, as part of google default credentials, is only supported on GCP")
+ }
+ newCreds.transportCreds = alts.NewClientCreds(alts.DefaultClientOptions())


menghanl · 2018-09-20T23:11:09Z

credentials/google/google.go

+ }
+ newCreds.transportCreds = alts.NewClientCreds(alts.DefaultClientOptions())
+ default:
+ return nil, fmt.Errorf("unsupported mode: %v", mode)


menghanl · 2018-09-20T23:15:32Z

dialoptions.go

+// WithTransportCredentials.
+//
+// This API is experimental.
+func WithCreds(b credentials.Bundle) DialOption {


menghanl · 2018-09-20T23:16:41Z

internal/transport/http2_client.go

@@ -166,7 +166,19 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr TargetInfo, opts Conne
 isSecure bool
 authInfo credentials.AuthInfo
 )
- if creds := opts.TransportCredentials; creds != nil {
+ var creds credentials.TransportCredentials


cesarghali

LGTM. Thanks for the fixes.

apolcyn · 2018-09-21T17:34:19Z

interop/client/client.go

@@ -39,6 +40,7 @@ var (
 caFile = flag.String("ca_file", "", "The file containning the CA root cert file")
 useTLS = flag.Bool("use_tls", false, "Connection uses TLS if true")
 useALTS = flag.Bool("use_alts", false, "Connection uses ALTS if true (this option can only be used on GCP)")
+ useGoogleDefaultCreds = flag.Bool("use_google_default_creds", false, "Uses google default creds if true")


My suggestion is ugly here, but in order to be consistent with Java and C++, mind switching the command arg from "--use_google_default_credentials" to "--custom_credentials_type", and then when use google default credentials when "google_default_credentials" is passed in as the value?

Doing so would just make it consistent with the other languages (See e.g.https://github.com/grpc/grpc/blob/master/tools/run_tests/run_interop_tests.py#L782)

This indeed makes me feel sad. But done anyway.

apolcyn · 2018-09-21T18:10:35Z

internal/transport/http2_client.go

+ perRPCCreds = append(perRPCCreds, t)
+ }
+ }
+ if transportCreds == nil {


nit: to be consistent with how perRPCCreds is set, initialize transportCreds := opt.TransportCredentials at the top and then set:

if t := b.TransportCredentials(); t != nil { transportCreds = t }

?

apolcyn · 2018-09-21T18:18:58Z

credentials/google/google.go

+ case internal.CredsBundleModeTLS:
+ newCreds.transportCreds = credentials.NewTLS(nil)
+ case internal.CredsBundleModeALTS, internal.CredsBundleModeGRPCLB:
+ if !vmOnGCP {


Hmm, I'm not sure it's necessary to make the also check that we're running on GCP within this credentials bundle; doesn't ALTS credentials itself already make this check?

@cesarghali what do you think about this?

@cesarghali I removed this checking so we don't need to export IsRunningOnGCP. Let me know if you think this is not OK.

I also modified the error string that will be returned by ALTS if not running on GCP.

ALTS does the check. However, would oauth.NewApplicationDefault work properly if we're not on GCP?

oauth.NewApplicationDefault calls FindDefaultCredentials, and seems it handles non-GCP cases: https://github.com/golang/oauth2/blob/d2e6202438beef2727060aa7cabdd924d92ebfd9/google/go19.go#L47:6

Sounds good, thanks for checking.

apolcyn · 2018-09-21T18:25:56Z

credentials/google/google.go

+ ctx, cancel := context.WithTimeout(context.Background(), tokenRequestTimeout)
+ defer cancel()
+ var err error
+ newCreds.perRPCCreds, err = oauth.NewApplicationDefault(ctx)


The connection to the grpclb server doesn't actually need OAuth credentials.

Should probably be consistent with Java and C++ on this.
cc @jiangtaoli2016 @yihuazhang can we confirm that Java and C++ don't send OAuth creds to the balancer?

I am pretty sure c++ strips call credentials (Oauth creds) when communicating with the balancer.

cc @ejona86 @zhangkun83 Could you pls confirm Java case?

Skipped for GRPCLB.

Java does not currently have a way to set CallCredentials in ManagedChannelBuilder. So there's naturally no way it could be used when communicating with the balancer.

apolcyn · 2018-09-21T18:57:45Z

credentials/credentials.go

+ // existing Bundle may cause races.
+ //
+ // SwitchMode returns nil if the requested mode is not supported.
+ SwitchMode(mode string) (Bundle, error)


General comment: going by the name and documentation for this SwitchMode API, it seems that there's an implementations have an implied state change when it's called. But statefulness doesn't actually seem to be needed for the the GoogleDefaultCredentials implementation. Is this API just geared to handle the general case where a state change on the creds bunde can occur? If not, is there some way to make this stateless?

It is stateless -- or at least, there is state, but it is immutable.

Maybe a different name would be better. BundleWithMode or BundleFor or NewBundle or NewWithMode?

Renamed to NewWithMode.

dfawley · 2018-09-24T23:12:02Z

clientconn.go

@@ -80,6 +80,9 @@ var (
 // being set for ClientConn. Users should either set one or explicitly
 // call WithInsecure DialOption to disable security.
 errNoTransportSecurity = errors.New("grpc: no transport security set (use grpc.WithInsecure() explicitly or set credentials)")
+ // errTransportCredsAndBundle indicates that creds bundle is used together
+ // with other individual Transport Credentials.
+ errTransportCredsAndBundle = errors.New("grpc: credentials.Bundle shouldn't be used together with individual TransportCredentials")


Nits: "shouldn't" -> "may not" (more precise/formal) and s/together// (it's cleaner).

dfawley · 2018-09-24T23:13:26Z

credentials/alts/utils.go

 // running on GCP.
-func isRunningOnGCP() bool {
+func IsRunningOnGCP() bool {


Can this be moved to internal/?

Un-exported. The GCP checking in google default creds is removed.

dfawley · 2018-09-24T23:15:07Z

credentials/credentials.go

+ // existing Bundle may cause races.
+ //
+ // SwitchMode returns nil if the requested mode is not supported.
+ SwitchMode(mode string) (Bundle, error)


It is stateless -- or at least, there is state, but it is immutable.

Maybe a different name would be better. BundleWithMode or BundleFor or NewBundle or NewWithMode?

dfawley · 2018-09-24T23:15:45Z

credentials/google/google.go

+//
+// This API is experimental.
+func NewDefaultCredentials() credentials.Bundle {
+ return new()


Why not move the contents of new here?

dfawley · 2018-09-24T23:16:30Z

credentials/google/google.go

+var vmOnGCP bool
+
+func init() {
+ vmOnGCP = alts.IsRunningOnGCP()


IIUC, this should be fine instead: var vmOnGCP = alts.IsRunningOnGCP() (no init function)

dfawley · 2018-09-24T23:17:20Z

credentials/google/google.go

+}
+
+func (c *creds) TransportCredentials() credentials.TransportCredentials {
+ if c == nil {


I don't think this defensive programming is necessary. I don't believe it is possible to get a nil *creds from this package. (?)

dfawley · 2018-09-24T23:21:17Z

balancer/grpclb/grpclb.go

+ if err != nil {
+ grpclog.Warningf("lbBalancer: client connection creds SwitchMode failed: %v", err)
+ }
+ lb.grpclbBackendCreds, err = opt.CredsBundle.SwitchMode(internal.CredsBundleModeALTS)


I didn't expect GRPCLB to need to be aware of ALTS at all. Can this be named something else, like CredsBundleModeGRPCLBBackend?

Renamed to CredsBundleModeBackendFromBalancer...

dfawley · 2018-09-24T23:23:16Z

internal/internal.go

+ // CredsBundleModeGRPCLB switches GoogleDefaultCreds to grpclb CredsBundleMode.
+ CredsBundleModeGRPCLB = "grpclb"
+ // CredsBundleModeTLS switches GoogleDefaultCreds to TLS CredsBundleMode.
+ CredsBundleModeTLS = "tls"


I believe this could be moved inside credentials/google.go, since it isn't referenced elsewhere?

In general, I want to keep grpc and grpclb talking only in terms of concepts and not specific technologies (i.e. "balancer" or "backend" not "tls" or "alts"). (This is related to my other comment about CredsBundleModeALTS.)

Those consts are kept in internal but renamed to non alts/tls specific names.

dfawley · 2018-09-25T18:19:31Z

dialoptions.go

@@ -286,7 +286,8 @@ func WithInsecure() DialOption {
 }

 // WithTransportCredentials returns a DialOption which configures a connection
-// level security credentials (e.g., TLS/SSL).
+// level security credentials (e.g., TLS/SSL). This should not be used together
+// with WithCreds.


WithCreds->WithCredentialsBundle

dfawley · 2018-09-25T18:21:19Z

test/creds_test.go

+
+package test
+
+// TODO: move all creds releated tests to this file.


Please file an issue and reference it here.

…r a SubConn

handle CredsBundleModeGRPCLB in google default creds pass creds bundle to balancer builder add google default creds to interop

…solver

- rename mode consts - rename SwitchMode to NewWithMode - comments and minor changes

menghanl requested review from cesarghali, dfawley and apolcyn September 20, 2018 00:56

cesarghali suggested changes Sep 20, 2018

View reviewed changes

menghanl commented Sep 20, 2018

View reviewed changes

menghanl force-pushed the google_default_creds branch from b63d244 to 7ac3bd6 Compare September 20, 2018 23:46

cesarghali approved these changes Sep 20, 2018

View reviewed changes

apolcyn reviewed Sep 21, 2018

View reviewed changes

menghanl force-pushed the google_default_creds branch 2 times, most recently from 567d9bf to 99430b6 Compare September 21, 2018 18:19

apolcyn reviewed Sep 21, 2018

View reviewed changes

dfawley reviewed Sep 24, 2018

View reviewed changes

dfawley reviewed Sep 25, 2018

View reviewed changes

dfawley approved these changes Sep 25, 2018

View reviewed changes

menghanl and others added 14 commits September 25, 2018 12:33

credentials: add Bundle interface and allow balancer to set Bundle fo…

f87ea88

…r a SubConn

use bundle in grpclb

4c1cbdd

comments, names

342dae0

Implement google transport creds

6814f06

Credentials Bundle API change

f449e15

fix bugs found in tests

9c214e4

handle CredsBundleModeGRPCLB in google default creds pass creds bundle to balancer builder add google default creds to interop

remove token scope from creds.New

62af2e5

call cancel from context

a8fe6c4

delete client binary...

2e25347

set grpc dial grpclb server target to remoteLBName that comes from re…

b0e6b80

…solver

fix review comments

63987d0

change google default creds commandline flag

cb5920e

refactor interop client

b97d223

fix review comments 2

4a2f887

menghanl added 4 commits September 25, 2018 12:33

fix comments

7a1e421

- rename mode consts - rename SwitchMode to NewWithMode - comments and minor changes

not checking IsOnGCP in google default creds

cfc7076

one more deletion

0c17045

review comments

5b91244

menghanl force-pushed the google_default_creds branch from cb906bd to 5b91244 Compare September 25, 2018 19:33

menghanl merged commit 4dedfdc into grpc:master Sep 25, 2018

menghanl deleted the google_default_creds branch September 25, 2018 21:00

dfawley added the Type: Feature New features or improvements in behavior label Sep 27, 2018

dfawley added this to the 1.16 Release milestone Sep 27, 2018

apolcyn mentioned this pull request Oct 2, 2018

Allow suffixing interop matrix image tags with the git commit they were built at grpc/grpc#16737

Closed

lock bot locked as resolved and limited conversation to collaborators Mar 26, 2019


		package test

		// TODO: move all creds releated tests to this file.

credentials: support google default creds #2315

credentials: support google default creds #2315

Conversation

menghanl commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarghali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment