feat(gax): Implement cert-rotation retries for grpc and http-json.#13246
feat(gax): Implement cert-rotation retries for grpc and http-json.#13246vverman wants to merge 1 commit into
Conversation
![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 a mechanism for dynamic mTLS certificate rotation by adding a refresh() capability to transport channels and implementing a RefreshingHttpJsonChannel for hot-swapping connections. It also updates various callables and retry algorithms to trigger these refreshes upon encountering unauthenticated errors. Feedback identifies a critical infinite loop bug in the new channel's entry retention logic and a violation of the ManagedChannel contract regarding termination status. Additionally, the reviewer recommends using System.nanoTime() for monotonic timing and advises against hardcoding environment variable checks in core library logic.
| private ChannelEntry getRetainedEntry() { | ||
| while (true) { | ||
| ChannelEntry entry = activeEntry.get(); | ||
| if (entry.retain()) { | ||
| return entry; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a critical bug in getRetainedEntry() that can lead to an infinite loop and 100% CPU usage. If shutdown() is called on the RefreshingHttpJsonChannel, it marks the current activeEntry for shutdown. Subsequent calls to newCall() will invoke getRetainedEntry(), which will repeatedly call retain() on the same activeEntry. Since shutdownRequested is now true, retain() will return false, causing the while(true) loop to spin indefinitely. You should check if the channel is shutting down and throw an IllegalStateException or return a failed call instead of looping.
| if ("true".equalsIgnoreCase(System.getenv("isMwlidEnvironment")) | ||
| && previousThrowable instanceof UnauthenticatedException) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Hardcoding environment variable checks (isMwlidEnvironment) directly into the core retry logic is an anti-pattern for a library. This makes the library's behavior dependent on external state that isn't explicitly configured through its API, making it harder to test and reason about. This logic should be moved to a configurable property in RetrySettings or ApiCallContext.
| } | ||
| try { | ||
| synchronized (lock) { | ||
| long now = System.currentTimeMillis(); |
There was a problem hiding this comment.
Prefer using System.nanoTime() over System.currentTimeMillis() for measuring elapsed time and durations. System.nanoTime() is monotonic and unaffected by adjustments to the system wall clock, making it more reliable for cooldown logic.
| long now = System.currentTimeMillis(); | |
| long now = System.nanoTime(); |
| public boolean awaitTermination(long duration, TimeUnit unit) throws InterruptedException { | ||
| return activeEntry.get().channel.awaitTermination(duration, unit); | ||
| } |
There was a problem hiding this comment.
The implementation of awaitTermination, isShutdown, and isTerminated only considers the current active entry. During a certificate rotation, there may be multiple older entries still draining. This implementation might return true for isTerminated while older connections are still active, which violates the contract of ManagedChannel. Consider tracking all active entries to provide an aggregate status.
1 participant
No description provided.