feat(gax): support transparent retries during mTLS certificate rotations - #13901
feat(gax): support transparent retries during mTLS certificate rotations#13901macastelaz wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for dynamic mTLS certificate rotation across both gRPC and HTTP/JSON transport channels by tracking certificate fingerprints, hot-swapping active channels, and triggering refreshes upon encountering UnauthenticatedException. The code review highlights several critical issues: a bug in ApiResultRetryAlgorithm that bypasses configured retries for non-retryable UnauthenticatedExceptions, a transient IllegalStateException during channel hot-swapping in RefreshingHttpJsonChannel, a backward compatibility break in CertificateBasedAccess when GOOGLE_API_USE_CLIENT_CERTIFICATE is explicitly set to "true", and a memory leak due to unpruned terminated entries in RefreshingHttpJsonChannel. Additionally, the reviewer recommends replacing synchronization on an AtomicReference with a lock-free read and an explicit ReentrantLock to avoid lock contention.
| private ChannelEntry getRetainedEntry() { | ||
| while (true) { | ||
| ChannelEntry entry = activeEntry.get(); | ||
| if (entry.retain()) { | ||
| return entry; | ||
| } | ||
| if (entry.shutdownRequested.get()) { | ||
| throw new IllegalStateException("Channel has been shut down"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In getRetainedEntry(), if entry.retain() returns false (because a refresh occurred and oldEntry.shutdownRequested was set to true), the code immediately throws IllegalStateException("Channel has been shut down"). However, the channel pool itself is not shut down; only the old entry is being retired. This will cause transient IllegalStateExceptions for active requests during certificate rotation. We should only throw IllegalStateException if the active entry itself has been shut down (i.e., entry == activeEntry.get()), and otherwise retry the loop with the new active entry.
| private ChannelEntry getRetainedEntry() { | |
| while (true) { | |
| ChannelEntry entry = activeEntry.get(); | |
| if (entry.retain()) { | |
| return entry; | |
| } | |
| if (entry.shutdownRequested.get()) { | |
| throw new IllegalStateException("Channel has been shut down"); | |
| } | |
| } | |
| } | |
| private ChannelEntry getRetainedEntry() { | |
| while (true) { | |
| ChannelEntry entry = activeEntry.get(); | |
| if (entry.retain()) { | |
| return entry; | |
| } | |
| if (entry == activeEntry.get()) { | |
| throw new IllegalStateException("Channel has been shut down"); | |
| } | |
| } | |
| } |
| } | ||
| } | ||
|
|
||
| private final AtomicReference<DiskCheckResult> lastDiskCheck = new AtomicReference<>(null); |
There was a problem hiding this comment.
Synchronizing on an AtomicReference (like lastDiskCheck) is a code smell. AtomicReference is designed for lock-free thread-safe operations. In performance-sensitive code, if a lock is needed to prevent concurrent disk reads, we should use an explicit ReentrantLock instead of the synchronized keyword to protect shared state.
| private final AtomicReference<DiskCheckResult> lastDiskCheck = new AtomicReference<>(null); | |
| private final AtomicReference<DiskCheckResult> lastDiskCheck = new AtomicReference<>(null); | |
| private final ReentrantLock diskCheckLock = new ReentrantLock(); |
References
- In performance-sensitive code, prefer using explicit locks over the 'synchronized' keyword to protect shared state while ensuring thread safety and visibility.
| synchronized (lastDiskCheck) { | ||
| cached = lastDiskCheck.get(); | ||
| if (cached != null | ||
| && (now - cached.timestampNanos < java.util.concurrent.TimeUnit.SECONDS.toNanos(1))) { | ||
| return cached.fingerprint; | ||
| } | ||
| String fingerprint = WorkloadCertificateUtils.getCertificateFingerprint(certPath); | ||
| lastDiskCheck.set(new DiskCheckResult(fingerprint, System.nanoTime())); | ||
| return fingerprint; | ||
| } |
There was a problem hiding this comment.
To avoid lock contention on the critical path, we should perform a lock-free read on lastDiskCheck first. If the cache is expired, we can then acquire an explicit ReentrantLock to perform the disk read and update the cache, rather than using synchronized.
| synchronized (lastDiskCheck) { | |
| cached = lastDiskCheck.get(); | |
| if (cached != null | |
| && (now - cached.timestampNanos < java.util.concurrent.TimeUnit.SECONDS.toNanos(1))) { | |
| return cached.fingerprint; | |
| } | |
| String fingerprint = WorkloadCertificateUtils.getCertificateFingerprint(certPath); | |
| lastDiskCheck.set(new DiskCheckResult(fingerprint, System.nanoTime())); | |
| return fingerprint; | |
| } | |
| DiskCheckResult cached = lastDiskCheck.get(); | |
| long now = System.nanoTime(); | |
| if (cached != null | |
| && (now - cached.timestampNanos < java.util.concurrent.TimeUnit.SECONDS.toNanos(1))) { | |
| return cached.fingerprint; | |
| } | |
| diskCheckLock.lock(); | |
| try { | |
| cached = lastDiskCheck.get(); | |
| if (cached != null | |
| && (now - cached.timestampNanos < java.util.concurrent.TimeUnit.SECONDS.toNanos(1))) { | |
| return cached.fingerprint; | |
| } | |
| String fingerprint = WorkloadCertificateUtils.getCertificateFingerprint(certPath); | |
| lastDiskCheck.set(new DiskCheckResult(fingerprint, System.nanoTime())); | |
| return fingerprint; | |
| } finally { | |
| diskCheckLock.unlock(); | |
| } |
References
- In performance-sensitive code, prefer using explicit locks over the 'synchronized' keyword to protect shared state while ensuring thread safety and visibility.
- To avoid lock contention on frequently called monitoring or routing methods (such as
getLoad()), maintain state using atomic variables (e.g.,AtomicLong,AtomicReference) to allow lock-free reads, rather than acquiring locks on the critical path.
Description
This PR introduces robust capability to handle dynamic mTLS certificate rotations transparently, seamlessly retrying in-flight failures and draining transports gracefully without hard-aborting ongoing work.
Changes Made
Graceful Draining (
RefreshingHttpJsonChannel&ChannelPool): Previously, when mTLS certificates rotated on disk, the system would aggressively shut down the obsolete channel. This PR introduces reference-counted outstanding request tracking, allowing all actively executing requests on the retiring channel to complete cleanly before closing the underlying transport.Transparent Retries (
ApiResultRetryAlgorithm): HandlesUNAUTHENTICATEDerrors cleanly by catching and evaluating them against the current retry strategy, treating certificate-bound 401s as retryable transitions once the channel is refreshed.Certificate Source De-duplication (
CertificateBasedAccess): Normalizes the static discovery ofGOOGLE_API_USE_CLIENT_CERTIFICATEenvironment overrides and workload certificate checks, addressing concurrent initialization bottlenecks.Based on PR #13246