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.

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

xds: Support custom credentials using XdsCredentialsRegistry #8924

Merged
merged 9 commits into from
Mar 28, 2022

Conversation

anicr7
Copy link
Contributor

@anicr7 anicr7 commented Feb 16, 2022

Currently the credentials used for xDS communications is hardcoded in the BootstrapperImpl. The bootstrap config chooses one of the possible hardcoded credential.

This PR adds support for a credential plugin which allows users to register custom credentials through XdsCredentialProviders. gRPC will automatically discover the implementations via Java's SPI mechanism

Bootstrapper will use XdsCredentialRegistry to retrieve the list of supported credentials. The current hardcoded list of credentials(google_default, insecure and tls) are registered by default to keep the behavior as is.

using the XdsCredentialsRegistry. XdsCredentialsRegistry uses Java
Service Provider mechanism to load providers(XdsCredentialProvider) at
runtime.

The XdsCredentialsRegistry will replace the hardcoded list of
credentials currently being used in Xds Bootstrapper.
…this in the documentation of XdsCredentialProvider
@sanjaypujare
Copy link
Contributor

@anicr7 what does it take to also add support for the Istio/K8S use-case where grpc would use:

  • a JWT (from /var/run/secrets/isitio-token or /var/run/secrets/[kubernetes.io/serviceaccount)
  • TLS using the CA crt from /var/run/secrets/kubernetes.io/serviceaccount/ca.crt

It will be very good to get that support in. CC @costinm

@anicr7
Copy link
Contributor Author

anicr7 commented Feb 16, 2022

@anicr7 what does it take to also add support for the Istio/K8S use-case where grpc would use:

  • a JWT (from /var/run/secrets/isitio-token or /var/run/secrets/[kubernetes.io/serviceaccount)
  • TLS using the CA crt from /var/run/secrets/kubernetes.io/serviceaccount/ca.crt

It will be very good to get that support in. CC @costinm

cc: @markdroth, @ejona86, @dfawley

Sanjay,
Are you asking about configuring the channel credentials(GoogleDefault or TLS) based on relevant parameters like token or cert? This PR does not directly add support for these parameters, but the credential providers will receive the json credential config. So adding support to consume these parameters in the providers should be trivial.

However you would have to clearly define/document the parameters that can be passed in for different credentials and that would be majority of the effort involved here.

@costinm
Copy link

costinm commented Feb 16, 2022

There are 2 problems for connecting to a XDS server ( like Istio, but other implementations of XDS exist and could be used):

  • the JWT - this PR defines a great API, but it would be great to also have a file-based implementation, with the file specified in bootstrap or in a system property/env variable. This would cover K8S - both the default serviceaccount (which works in many cases but is not recommended) or a projected volume - which adds a proper audience to the token. It is great to also support custom credentials, but token-in-a-file is a pretty common use case and could be built-in.

  • in many cases, the XDS server uses a custom certificate - the root CA may be same as K8S (if CertSigningRequest is used - it's an option Istio supports but doesn't use by default), or may Istio built-in (Citadel), CertManager, Vault or some other custom one. In all cases, the root CA crt will be in a file.

IMO a more generic mechanism to allow user to customize the XDS client would be great - some users may use other ecosystem features like metrics, traces - on the XDS client connection.

But while a plugin allowing custom code is great - having a built-in file for JWT and root cert would be great.

@ejona86 ejona86 self-requested a review February 18, 2022 21:12
@anicr7
Copy link
Contributor Author

anicr7 commented Mar 3, 2022

@ejona86 Friendly ping :).

anicr7 and others added 4 commits March 25, 2022 21:43
1. Rename the function from getChannelCredentials to
   newChannelCredentials instead.

2. Add a test case to make sure the relevant xds credentials are
   available through Service loader

3. Change bootstrapper to pass in config map instead of the complete
   channel credentials json map.

4. Add some documentation about the wrapper xds credentials.
@anicr7 anicr7 requested a review from ejona86 March 26, 2022 06:07
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Only one small comment

@ejona86
Copy link
Member

ejona86 commented Mar 28, 2022

To be clear to other commenters: This PR just allows the pre-existing bootstrap plugin functionality to be supplemented at runtime with xDS not needing a hard dependency on each credential type. Creating a CallCredentials registry to allow JWT or enhancing TlsChannelCredentials to support an option to specify root certs are possible, but would require a cross-language design and is outside the scope of this PR.

@ejona86 ejona86 merged commit d3f7dc0 into grpc:master Mar 28, 2022
@ejona86
Copy link
Member

ejona86 commented Mar 28, 2022

@anicr7, I copied the PR description to the commit description, since the PR description seemed more complete. It is generally useful to write a nice commit description and have it auto-copied when constructing the PR.

@anicr7 anicr7 deleted the xds_credentials_registry branch March 28, 2022 17:28
@anicr7
Copy link
Contributor Author

anicr7 commented Mar 28, 2022

@anicr7, I copied the PR description to the commit description, since the PR description seemed more complete. It is generally useful to write a nice commit description and have it auto-copied when constructing the PR.

Thanks Eric!

anicr7 added a commit to anicr7/grpc-java that referenced this pull request Mar 28, 2022
and using Immutable interface types.

1. Unnecessary fully qualified names
Currently in XdsCredentialsRegistry, the child classes are referred by
their fully qualified names i.e.
'io.grpc.xds.internal.GoogleDefaultXdsCredentialsProvider' instead of
importing GoogleDefaultXdsCredentialsProvider and just using
GoogleDefaultXdsCredentialsProvider.class.

2. Use immutable interfaces instead of the generic collection interface
   i.e. ImmutableMap instead of just Map.

These improvements are related to grpc#8924.
anicr7 added a commit to anicr7/grpc-java that referenced this pull request Mar 28, 2022
and using Immutable interface types.

1. Unnecessary fully qualified names
Currently in XdsCredentialsRegistry, the child classes are referred by
their fully qualified names i.e.
'io.grpc.xds.internal.GoogleDefaultXdsCredentialsProvider' instead of
importing GoogleDefaultXdsCredentialsProvider and just using
GoogleDefaultXdsCredentialsProvider.class.

2. Use immutable interfaces instead of the generic collection interface
   i.e. ImmutableMap instead of just Map.

These improvements are related to grpc#8924.
ejona86 pushed a commit that referenced this pull request Apr 4, 2022
…es and using Immutable interface types. (#9025)

1. Unnecessary fully qualified names
Currently in XdsCredentialsRegistry, the child classes are referred by
their fully qualified names i.e.
'io.grpc.xds.internal.GoogleDefaultXdsCredentialsProvider' instead of
importing GoogleDefaultXdsCredentialsProvider and just using
GoogleDefaultXdsCredentialsProvider.class.

2. Use immutable interfaces instead of the generic collection interface
   i.e. ImmutableMap instead of just Map.

These improvements are related to #8924.
temawi pushed a commit to temawi/grpc-java that referenced this pull request Apr 8, 2022
Currently the credentials used for xDS communications is hardcoded in the BootstrapperImpl. The bootstrap config chooses one of the possible hardcoded credential.

This commit adds support for a credential plugin which allows users to register custom credentials through XdsCredentialProviders. gRPC will automatically discover the implementations via Java's SPI mechanism

Bootstrapper will use XdsCredentialRegistry to retrieve the list of supported credentials. The current hardcoded list of credentials(google_default, insecure and tls) are registered by default to keep the behavior as is.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants