-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xds bootstrap: enable using JWT Call Credentials (part 2 for A97) #8536
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
base: master
Are you sure you want to change the base?
Conversation
@dimpavloff : I see a bunch of commits. Please let me know when it is ready for another review pass. Thanks. |
@easwars I think this is ready for another review :) |
internal/xds/bootstrap/bootstrap.go
Outdated
func (sc *ServerConfig) String() string { | ||
if len(sc.serverFeatures) == 0 { | ||
return fmt.Sprintf("%s-%s", sc.serverURI, sc.selectedCreds.String()) | ||
return fmt.Sprintf("%s-%s", sc.serverURI, sc.selectedChannelCreds.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this method be updated to include the CallCredentials?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defined a type CallCredsConfigs []CallCredsConfig
and implemented Stringer
for this purpose. LMK if you prefer a different approach
Apologies for the delay in the review. I'm a bit swamped with other things. I will get to this asap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to make a complete pass. But some comments to get you going. I will try to make another pass asap.
internal/envconfig/xds.go
Outdated
// https://github.com/grpc/proposal/blob/master/A86-xds-http-connect.md | ||
XDSHTTPConnectEnabled = boolFromEnv("GRPC_EXPERIMENTAL_XDS_HTTP_CONNECT", false) | ||
|
||
// XDSBootstrapCallCredsEnabled controls if JWT call credentials can be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This environment variable guards the newly added bootstrap field call_creds
. I agree that the only call credentials that we currently support via this mechanism is the JWT call creds, but that could change any day (before we get rid of this env var). So, I would prefer we more explicitly call out what this env var guards.
internal/xds/bootstrap/bootstrap.go
Outdated
func (sc *ServerConfig) String() string { | ||
if len(sc.serverFeatures) == 0 { | ||
return fmt.Sprintf("%s-%s", sc.serverURI, sc.selectedCreds.String()) | ||
return fmt.Sprintf("%s-%s", sc.serverURI, sc.selectedChannelCreds.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
|
||
// NewCallCredentials returns a credentials.PerRPCCredentials The input config | ||
// should match the structure specified in gRFC A97 structure. | ||
// See gRFC A97: https://github.com/grpc/proposal/blob/master/A97-xds-jwt-call-creds.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits here.
- There needs to be a period after the first sentence.
- I think the next two lines can be shortened. (this is an internal API, so we can omit the link to the actual gRFC)
- There is no mention about the second return value,
func()
So, maybe something like
// NewCallCredentials returns a new JWT token based call credentials. The input
// config must match the structure specified in gRFC A97. The caller is expected
// to invoke the second return value when they are done using the returned call creds.
"google.golang.org/grpc/credentials" | ||
) | ||
|
||
func TestNewCallCredentials(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the Tester
defined in the grpctest
package here:
grpc-go/internal/grpctest/grpctest.go
Line 52 in d0ebcdf
type Tester struct{} |
It has some setup and teardown magic that we find useful for all of our tests.
|
||
if !tt.wantSuccess { | ||
if err == nil { | ||
t.Fatal("Expected error, got nil") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please identify the function being tested in the test error message. See: https://google.github.io/styleguide/go/decisions#identify-the-function
internal/xds/bootstrap/bootstrap.go
Outdated
// CallCreds contains the call credentials configuration for individual RPCs. | ||
// It implements gRFC A97 call credentials structure. | ||
type CallCreds struct { | ||
// Type contains a unique name identifying the call credentials type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm .. The gRFC does not state any uniqueness requirements for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, you're right, it doesn't. I was influenced by the ChannelCreds.Type but there it makes more sense given that only one gets picked up. I've updated the comment
internal/xds/bootstrap/bootstrap.go
Outdated
} | ||
|
||
// CallCreds returns the call credentials configuration for this server. | ||
func (sc *ServerConfig) CallCreds() []CallCreds { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call this CallCredsConfig
and the next method CallCreds
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to keep symmetry with ChannelCreds
and SelectedChannelCreds
. However, SelectedCallCreds
now returns []credentials.PerRPCCredentials
rather than the config, still, which breaks this symmetry.
Realistically, I'm not sure even if the public methods are needed -- the PerRPCCredentials are added to the ServerConfig.extraDialOptions
and used via ServerConfig.DialOptions()
WDYT, shall I remove the public methods? I've renamed them in the meantime. I've used CallCredsConfigs
, plural, because I also renamed the Go type to CallCredsConfig
and the method returns a collection of those. I've also renamed the relevant fields in ServerConfig
(and ServerConfigTestingOptions
) in similar way.
internal/xds/bootstrap/bootstrap.go
Outdated
if envconfig.XDSBootstrapCallCredsEnabled { | ||
// Process call credentials - unlike channel creds, we use ALL supported | ||
// types. Also, call credentials are optional as per gRFC A97. | ||
for _, callCredConfig := range server.CallCreds { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can we use a shorter name for the loop variable. Maybe cfg
?
internal/xds/bootstrap/bootstrap.go
Outdated
// Skip unsupported call credential types (don't fail bootstrap). | ||
continue | ||
} | ||
callCred, cancel, err := c.Build(callCredConfig.Config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We refer to them as credentials
and not credential
even when there is a single one. This was a major source of confusion to me when I joined the team as well :)
So, s/callCred/callCreds/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, yes, this did confuse me a bit when starting. Thanks for spotting this
} | ||
} | ||
|
||
func TestServerConfigCallCredsIntegration(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to have the s
receiver on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @easwars ; I think it's ready for another pass
internal/xds/bootstrap/bootstrap.go
Outdated
// CallCreds contains the call credentials configuration for individual RPCs. | ||
// It implements gRFC A97 call credentials structure. | ||
type CallCreds struct { | ||
// Type contains a unique name identifying the call credentials type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, you're right, it doesn't. I was influenced by the ChannelCreds.Type but there it makes more sense given that only one gets picked up. I've updated the comment
internal/xds/bootstrap/bootstrap.go
Outdated
} | ||
|
||
// CallCreds returns the call credentials configuration for this server. | ||
func (sc *ServerConfig) CallCreds() []CallCreds { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to keep symmetry with ChannelCreds
and SelectedChannelCreds
. However, SelectedCallCreds
now returns []credentials.PerRPCCredentials
rather than the config, still, which breaks this symmetry.
Realistically, I'm not sure even if the public methods are needed -- the PerRPCCredentials are added to the ServerConfig.extraDialOptions
and used via ServerConfig.DialOptions()
WDYT, shall I remove the public methods? I've renamed them in the meantime. I've used CallCredsConfigs
, plural, because I also renamed the Go type to CallCredsConfig
and the method returns a collection of those. I've also renamed the relevant fields in ServerConfig
(and ServerConfigTestingOptions
) in similar way.
internal/xds/bootstrap/bootstrap.go
Outdated
// Skip unsupported call credential types (don't fail bootstrap). | ||
continue | ||
} | ||
callCred, cancel, err := c.Build(callCredConfig.Config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, yes, this did confuse me a bit when starting. Thanks for spotting this
if err != nil { | ||
t.Fatalf("NewCallCredentials failed: %v", err) | ||
} | ||
if callCreds == nil { | ||
t.Fatal("Expected non-nil bundle") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, much better, thanks!
internal/xds/bootstrap/bootstrap.go
Outdated
func (sc *ServerConfig) String() string { | ||
if len(sc.serverFeatures) == 0 { | ||
return fmt.Sprintf("%s-%s", sc.serverURI, sc.selectedCreds.String()) | ||
return fmt.Sprintf("%s-%s", sc.serverURI, sc.selectedChannelCreds.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defined a type CallCredsConfigs []CallCredsConfig
and implemented Stringer
for this purpose. LMK if you prefer a different approach
Part two for grpc/proposal#492 (A97), following #8431 .
What this PR does is:
internal/xds/bootstrap
with support for loading multiple PerRPCCallCredentials specifed in a newcall_creds
field in the boostrap file as per A97xds/internal/xdsclient/clientimpl.go
to use the call credentials when constructing the clientxds/bootstrap
to register thejwtcreds
call credentials and make them available ifGRPC_EXPERIMENTAL_XDS_BOOTSTRAP_CALL_CREDS
is enabledI have added
DialOptionsWithCallCredsForTransport
because, even though current and future call credentials are likely to all expect secure transport, I thought it would be safer to check of insecure transport just in case. If you prefer, I can just updateDialOptions
to use all call credentials regardless of the transport.Relates to istio/istio#53532
RELEASE NOTES: