-
Notifications
You must be signed in to change notification settings - Fork 4.5k
xds: implement file-based JWT Call Credentials (A97) #8431
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8431 +/- ##
==========================================
+ Coverage 82.27% 82.51% +0.24%
==========================================
Files 414 415 +1
Lines 40424 40731 +307
==========================================
+ Hits 33259 33610 +351
+ Misses 5795 5757 -38
+ Partials 1370 1364 -6
🚀 New features to boost your workflow:
|
@dfawley hey 👋 Given you approved A97, would you mind having a cursory look at the PR to confirm if at least at a high level the approach looks good? |
I will take a look at this , I need to go through the gRFC first. |
Sorry for the delay here. @easwars would you be able to review this change? I think you have more background into some of the things than I do, like the bootstrap integration. Thank you! |
Implement grpc/proposal#492 (A97). This is achieved via:
credentials/jwt
package to provide file-based PerRPCCallCredentials. It can be used beyond XDS. The package handles token reloading, caching, and validation as per A97internal/xds/bootstrap/jwtcreds
package to provide acredentials.Bundle
implementation using the new jwt package.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 enabledWhilst implementing the above, I considered
credentials/oauth
andcredentials/xds
packages instead of creating a new one. The former package hasNewJWTAccessFromKey
andjwtAccess
which seem very relevant at first. However, I think thejwtAccess
behaviour seems more tailored towards Google services. Also, the refresh, caching, and error behaviour for A97 is quite different than what's already there and therefore a separate implementation would have still made sense.WRT
credentials/xds
, it could have been extended to both handle transport and call credentials. However, this is a bit at odds with A97 which says that the implementation should be non-XDS specific and, from reading between the lines, usable beyond XDS.I think the current approach makes review easier but because of the similarities with the other two packages, it is a bit confusing to navigate. Please let me know whether the structure should change.
WRT the internal bootstrap implementation, I 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.Lastly, I appreciate this is a large PR so please lmk if you would rather I split it (e.g.
credentials/
andxds/
parts).Relates to istio/istio#53532
RELEASE NOTES:
credentials/jwt
package providing file-based JWT PerRPCCredentials (A97)GRPC_EXPERIMENTAL_XDS_BOOTSTRAP_CALL_CREDS
)