Implement gRFC A97: xDS JWT Call Credentials - #12951
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
| */ | ||
| @ThreadSafe | ||
| @Internal | ||
| public final class LoadStatsManager2 { |
There was a problem hiding this comment.
The changes in LoadStatsManager2.java, LoadReportClientTest.java and LoadStatsManager2Test.java are unrelated to gRPC A97.
| } | ||
|
|
||
| @VisibleForTesting | ||
| public static ServerInfo create( |
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.
| private static final long INITIAL_BACKOFF_MS = 1000; | ||
| private static final long MAX_BACKOFF_MS = 60000; | ||
| private static final double BACKOFF_MULTIPLIER = 2.0; | ||
|
|
There was a problem hiding this comment.
Let's align with the connection backoff parameters specified in https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. Also jitter is not implemented here.
| private long expirationTimeMillis; | ||
| private ReadState readState = ReadState.IDLE; | ||
| private Status lastReadFailureStatus; | ||
| private long currentBackoffMs; |
There was a problem hiding this comment.
nit: Change Ms to Millis consistent with other variables.
| private TokenInfo readAndParseTokenFile() throws IOException { | ||
| File file = new File(filePath); | ||
| long length = file.length(); | ||
| if (length > 1048576) { |
There was a problem hiding this comment.
nit: Create a constant.
| } | ||
|
|
||
| @Test | ||
| public void parseBootstrap_xdsServers_jwtTokenFileCallCreds() throws Exception { |
There was a problem hiding this comment.
The file lacks a test where call_creds contains multiple valid supported entries to verify that BootstrapperImpl correctly combines them into a CompositeCallCredentials.
| List<CallCredentials> parsedCreds = new ArrayList<>(); | ||
| for (Map<String, ?> credJson : jsonList) { | ||
| String type = JsonUtil.getString(credJson, "type"); | ||
| if (type == null) { |
There was a problem hiding this comment.
There is no test case in GrpcBootstrapperImplTest passing a call_creds object missing the "type" field (e.g. { "config": { ... } }) to verify this exception branch.
| if (token.isEmpty()) { | ||
| throw new IllegalArgumentException("Token file is empty"); | ||
| } |
There was a problem hiding this comment.
JwtTokenFileCallCredentialsTest lacks a test for an empty (0-byte or whitespace-only) file.
| if (expSeconds > Long.MAX_VALUE / 1000) { | ||
| expirationTimeMillis = Long.MAX_VALUE; | ||
| } else { |
There was a problem hiding this comment.
There is no test verifying that an extremely large exp claim value (e.g., exp: 9999999999999999) safely clamps expirationTimeMillis to Long.MAX_VALUE without arithmetic overflow.
1088069 to
535f3c9
Compare
3 participants
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