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.

Sign up for GitHub

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

Jump to bottom

credentials/google: introduce a new API NewComputeEngineCredsWithOptions #4767

Merged
merged 10 commits into from Sep 30, 2021
Merged

credentials/google: introduce a new API NewComputeEngineCredsWithOptions #4767

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
10790dc
DirectPath: make ComputeEngineCredentials support non-default service…
mohanli-ml Sep 14, 2021
2436373
DirectPath: make ComputeEngineCredentials support non-default service…
mohanli-ml Sep 14, 2021
6270425
DirectPath: introduce a new API NewDirectPathCredentials
mohanli-ml Sep 14, 2021
33ebd02
Merge branch 'master' into compute-engine-creds
mohanli-ml Sep 20, 2021
72552c8
DirectPath: introduce a new API NewComputeEngineCredsWithPerRPC
mohanli-ml Sep 20, 2021
e314a44
Merge branch 'master' into compute-engine-creds
mohanli-ml Sep 21, 2021
514cda1
DirectPath: introduce a new API NewComputeEngineCredsWithPerRPC
mohanli-ml Sep 21, 2021
7e645f9
credentials/google: introduce a new API NewComputeEngineCredsWithOptions
mohanli-ml Sep 29, 2021
af130ed
credentials/google: introduce a new API NewComputeEngineCredsWithOptions
mohanli-ml Sep 29, 2021
18f6f5e
credentials/google: introduce a new API NewComputeEngineCredsWithOptions
mohanli-ml Sep 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion credentials/google/google.go
View file
Open in desktop
Expand Up @@ -64,14 +64,25 @@ func NewDefaultCredentials() credentials.Bundle {
//
// This API is experimental.
func NewComputeEngineCredentials() credentials.Bundle {
return NewComputeEngineCredsWithPerRPC(nil)
}

// NewComputeEngineCredsWithPerRPC returns a credentials bundle that is configured to work
Copy link
Contributor

Choose a reason for hiding this comment

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

NewComputeEngineCredsWithOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// with google services. This API must only be used when running on GCE.
//
// This API is experimental.
func NewComputeEngineCredsWithPerRPC(perRPC credentials.PerRPCCredentials) credentials.Bundle {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let make this NewComputeEngineCredsWithOptions, and take a struct as the input. We may need this in the future to support other features (e.g. mtls)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

c := &creds{
newPerRPCCreds: func() credentials.PerRPCCredentials {
if perRPC != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

And as mentioned in another comment, please move this if check out of the closure (you can make two closures, based on the value of perRPC)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return perRPC
}
return oauth.NewComputeEngine()
},
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a little more efficient:

c := &creds{newPerRPCCreds: perRPC}
if c.newPerRPCCreds == nil {
	c.newPerRPCCreds = oauth.NewComputeEngine
}

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, wait, this doesn't work, exactly, because of the func returning PerRPCCredentials vs. actual PerRPCCredentials. Why do we have a factory instead of the credentials, here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The creds struct has both perRPCCreds and newPerRPCCreds.
The factory function is only called in NewWithMode.

It's so that no per-RPC is shared by two bundles?
But then why is it OK to share transport creds?

(Just discussing, not suggesting changes to this PR)

Copy link
Contributor

@menghanl menghanl Sep 21, 2021 •
edited

Choose a reason for hiding this comment

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

Oh, actually, transport creds are not shared. NewWithMode() also creates them.

Copy link
Member

Choose a reason for hiding this comment

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

It's so that no per-RPC is shared by two bundles?
But then why is it OK to share transport creds?

TransportCredentials have a Clone method, but PerRPCCredentials do not.

But why is it not okay to reuse PerRPCCredentials? There are no concerns about initialization or concurrency AFAICT.

Copy link
Member

Choose a reason for hiding this comment

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

We can clean that up later. The comment here would be to avoid checking a conditional inside the closure; instead check outside of the closure and create the &creds differently from the start.

bundle, err := c.NewWithMode(internal.CredsBundleModeFallback)
if err != nil {
logger.Warningf("compute engine creds: failed to create new creds: %v", err)
logger.Warningf("compute engine creds with per rpc: failed to create new creds: %v", err)
}
return bundle
}
Expand Down
5 changes: 3 additions & 2 deletions credentials/google/google_test.go
View file
Open in desktop
Expand Up @@ -76,8 +76,9 @@ func overrideNewCredsFuncs() func() {
func TestClientHandshakeBasedOnClusterName(t *testing.T) {
defer overrideNewCredsFuncs()()
for bundleTyp, tc := range map[string]credentials.Bundle{
"defaultCreds": NewDefaultCredentials(),
"computeCreds": NewComputeEngineCredentials(),
"defaultCreds": NewDefaultCredentials(),
"computeCreds": NewComputeEngineCredentials(),
"computeCredsPerRPC": NewComputeEngineCredsWithPerRPC(nil),
} {
tests := []struct {
name string
Expand Down