Implement gRFC A97: xDS JWT Call Credentials - #12951
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces JwtTokenFileCallCredentials to load, parse, cache, and refresh JWT tokens from a file, and integrates it into the xDS bootstrap configuration to support call_creds. The review feedback identifies two bugs in JwtTokenFileCallCredentials: a critical issue where valid cached tokens are ignored during backoff after a failed background refresh, and a potential integer overflow when calculating expirationTimeMillis from large exp claims.
Add a defensive check to prevent overflow by capping the expiration time at Long.MAX_VALUE if expSeconds is too large. Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces JwtTokenFileCallCredentials to load, parse, and cache JWT tokens from a file, and integrates it into the xDS bootstrap configuration under an experimental flag. It also updates GrpcXdsTransportFactory to support composite call credentials. The review feedback highlights two important safety improvements in JwtTokenFileCallCredentials: using a bounded stream when reading the token file to prevent potential OutOfMemoryError on special devices, and adding a null check on the parsed JSON element to avoid a NullPointerException when handling malformed payloads.
There was a problem hiding this comment.
No need for the new overload, as this method is now left with no other callers in non-test code. Just modify signature of existing method.
There was a problem hiding this comment.
While it has no production callers left, the 6-argument ServerInfo.create() is heavily used across dozens of test suites. Keeping it as a @VisibleForTesting overload prevents polluting all those test calls with unnecessary null values for CallCredentials.
There was a problem hiding this comment.
Modifying a lot of tests to switch over to the new method with CallCredentials is fine. The only cases where we prefer to have overloaded methods omitting certain arguments are if there are legitimate production code callers for it or if the method is public and we dont' want to break downstream projects. But Bootstrapper although public, is an @Internal class, so the 2nd consideration doesn't really apply here.
1088069 to
535f3c9
Compare
There was a problem hiding this comment.
Modifying a lot of tests to switch over to the new method with CallCredentials is fine. The only cases where we prefer to have overloaded methods omitting certain arguments are if there are legitimate production code callers for it or if the method is public and we dont' want to break downstream projects. But Bootstrapper although public, is an @Internal class, so the 2nd consideration doesn't really apply here.
| headers.put(AUTHORIZATION_HEADER, "Bearer " + tokenInfo.token); | ||
| for (MetadataApplier applier : appliersToApply) { | ||
| try { | ||
| applier.apply(headers); |
There was a problem hiding this comment.
Our Metadata.java is not threadsafe.
I believe every RPC should get its own distinct, unshared Metadata because if multiple RPCs process this shared Metadata instance concurrently, or if an interceptor mutates it, you will get data corruption
CC: @kannanjgithub WDYT?
|
|
||
| if (hasValidCache) { | ||
| tokenToApply = new TokenInfo(cachedToken, expirationTimeMillis); | ||
| if (readState == ReadState.BACKOFF && now >= nextAttemptTimeMillis) { |
There was a problem hiding this comment.
Is this redundant?
Because we already execute this exact check at the very beginning of the synchronized block and since this block executes under a lock, readState cannot possibly revert to ReadState.BACKOFF.
| triggerRead = true; | ||
| } | ||
| } else { | ||
| if (readState == ReadState.BACKOFF && now >= nextAttemptTimeMillis) { |
Implements gRFC A97 to support file-based JSON Web Token (JWT) Call Credentials for xDS-enabled clients. Implemented using an experimental coding agent.
Summary of Changes