feat(gax): support transparent retries during mTLS certificate rotations

.gitignore

@@ -86,6 +86,8 @@ monorepo
 *.tfstate.lock.info
 
 .jqwik-database
-
 # Python build artifacts for library_generation tool
 sdk-platform-java/hermetic_build/library_generation/build/
+
+**/Agentic_Identities/**
+**/*.patch

comments_13246.txt

@@ -0,0 +1,53 @@
+--- ISSUE COMMENTS ---
+
+--- REVIEW COMMENTS ---
+@gemini-code-assist[bot] on sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/RefreshingHttpJsonChannel.java:187:
+![critical](https://www.gstatic.com/codereviewagent/critical.svg)
+
+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.
+
+@gemini-code-assist[bot] on sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiResultRetryAlgorithm.java:None:
+![high](https://www.gstatic.com/codereviewagent/high-priority.svg)
+
+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`.
+
+@gemini-code-assist[bot] on sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/RefreshingHttpJsonChannel.java:None:
+![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
+
+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.
+
+```suggestion
+        long now = System.nanoTime();
+```
+
+@gemini-code-assist[bot] on sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/RefreshingHttpJsonChannel.java:226:
+![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
+
+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.
+
+@blakeli0 on sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/RefreshingHttpJsonChannel.java:None:
+I'm not sure lock free is the best way. Because this mean other failed request would skip this refreshing block and be scheduled with a retry, which would fail again if refreshing is still in progress.
+
+@blakeli0 on sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/RefreshingHttpJsonChannel.java:1:
+Looks good overall. When we actually get to implementation phase, we need test it very carefully to prevent resource leaking.
+
+@lqiu96 on sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiResultRetryAlgorithm.java:57:
+IIUC, I think this should be done in the AttemptCallable itself, right? Other callable types have the refresh logic Bidi, Client, and Server. Otherwise this may potentionally refresh twice for those. 
+
+@lqiu96 on sdk-platform-java/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java:514:
+IIUC, this is the new gated logic right? Basically to ensure that we only check the fingerprint if the cert exists on the system. No need to have the rotation if the file doesn't exist
+
+@lqiu96 on sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/RefreshingHttpJsonChannel.java:53:
+IIUC, this is just a 1:1 copy of the gRPC ChannelPool logic?
+
+@blakeli0 on sdk-platform-java/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java:506:
+I think we need to split this shouldRefresh logic to two parts
+1. getWorkloadCertPath() should be a static field loaded on app start up. So that we can comfortably skip refreshing for most cases.
+2. Get current finger print and compare with the active ones. This part should also be moved to a transport neutral place since I see the logic is duplicated in RefreshingHttpJsonChannel.
+
+@blakeli0 on sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiResultRetryAlgorithm.java:57:
+Agreed. Also it is mixing concerns by putting refreshing channels logic in a `shouldRetry` method.
+
+@vverman on sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/RefreshingHttpJsonChannel.java:None:
+Done!
+

fetch_log.py

@@ -0,0 +1,56 @@
+import urllib.request
+import json
+import zipfile
+import io
+import sys
+import ssl
+
+ctx = ssl.create_default_context()
+ctx.check_hostname = False
+ctx.verify_mode = ssl.CERT_NONE
+
+repo = "googleapis/google-cloud-java"
+pr_num = 13873
+url = f"https://api.github.com/repos/{repo}/pulls/{pr_num}"
+
+print(f"Fetching PR data from {url}")
+req = urllib.request.Request(url)
+with urllib.request.urlopen(req, context=ctx) as res:
+    pr_data = json.loads(res.read().decode())
+    sha = pr_data["head"]["sha"]
+
+runs_url = f"https://api.github.com/repos/{repo}/commits/{sha}/check-runs?per_page=100"
+print(f"Fetching runs from {runs_url}")
+req2 = urllib.request.Request(runs_url)
+with urllib.request.urlopen(req2, context=ctx) as res2:
+    runs_data = json.loads(res2.read().decode())
+
+target_run = None
+for run in runs_data["check_runs"]:
+    if "java-bigquery" in run["name"] and "25" in run["name"]:
+        target_run = run
+        break
+
+if not target_run:
+    print("Could not find the check run.")
+    sys.exit(0)
+
+run_name = target_run["name"]
+run_id = target_run["id"]
+status = target_run["conclusion"]
+print(f"Found run: {run_name} (ID: {run_id}, Status: {status})")
+
+log_url = f"https://api.github.com/repos/{repo}/actions/jobs/{run_id}/logs"
+print(f"Fetching logs from {log_url}")
+try:
+    req_log = urllib.request.Request(log_url)
+    with urllib.request.urlopen(req_log, context=ctx) as res_log:
+        logs = res_log.read().decode()
+        lines = logs.split("\n")
+        print("\n".join(lines[-200:]))
+except Exception as e:
+    import urllib.error
+    if isinstance(e, urllib.error.HTTPError):
+        print(f"Log fetch failed: {e.code} {e.reason}")
+    else:
+        print(f"Error fetching logs: {e}")

fix_AgentIdentityUtils.py

@@ -0,0 +1,44 @@
+import re
+with open('/usr/local/google/home/mcastelaz/google-cloud-java/google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/AgentIdentityUtils.java', 'r') as f:
+    text = f.read()
+
+# Fix Javadoc positions
+text = text.replace(
+    '    @SuppressWarnings("unchecked")\n    /**\n     * Extracts the certificate and private key paths from the JSON configuration file.',
+    '    /**\n     * Extracts the certificate and private key paths from the JSON configuration file.\n     *\n     * @param certConfigPath the configuration file path intended to be parsed\n     * @return an object encapsulating resolved paths mapped from the definition\n     * @throws IOException if parsing JSON mapping encounters structural errors\n     */\n    @SuppressWarnings("unchecked")'
+)
+
+# Fix MissingJavadocMethod on setters
+text = text.replace(
+    '    public static void setEnvReader(EnvReader reader)',
+    '    /**\n     * Sets the env reader for testing.\n     *\n     * @param reader the environment reader\n     */\n    @VisibleForTesting\n    public static void setEnvReader(final EnvReader reader)'
+)
+text = text.replace(
+    '    static void setTimeService(TimeService service)',
+    '    /**\n     * Sets the time service for testing.\n     *\n     * @param service the time service\n     */\n    @VisibleForTesting\n    static void setTimeService(final TimeService service)'
+)
+text = text.replace(
+    '    static void resetTimeService()',
+    '    /**\n     * Resets the time service.\n     */\n    @VisibleForTesting\n    static void resetTimeService()'
+)
+text = text.replace(
+    '    static void setWellKnownDir(final String dir)',
+    '    /**\n     * Sets the well known directory for testing.\n     *\n     * @param dir the directory path\n     */\n    @VisibleForTesting\n    static void setWellKnownDir(final String dir)'
+)
+
+# Add finals to parameters
+text = text.replace('String getEnv(String name)', 'String getEnv(final String name)')
+text = text.replace('resolveCertAndKeyPaths(String certConfigPath)', 'resolveCertAndKeyPaths(final String certConfigPath)')
+text = text.replace('loadAndVerifyCredentials(String certPath, String keyPath)', 'loadAndVerifyCredentials(final String certPath, final String keyPath)')
+text = text.replace('checkExistsOrAccessDenied(java.nio.file.Path path)', 'checkExistsOrAccessDenied(final java.nio.file.Path path)')
+text = text.replace('getPathsFromConfigWithRetry(String certConfigPath)', 'getPathsFromConfigWithRetry(final String certConfigPath)')
+text = text.replace('readCertificateChain(String certPath)', 'readCertificateChain(final String certPath)')
+text = text.replace('verifyKeyPair(X509Certificate cert, PrivateKey privateKey)', 'verifyKeyPair(final X509Certificate cert, final PrivateKey privateKey)')
+text = text.replace('readPrivateKey(String keyPath, String algorithm)', 'readPrivateKey(final String keyPath, final String algorithm)')
+text = text.replace('shouldEnableMtls(boolean certsPresent, boolean configExists)', 'shouldEnableMtls(final boolean certsPresent, final boolean configExists)')
+text = text.replace('extractPathsFromConfig(String certConfigPath)', 'extractPathsFromConfig(final String certConfigPath)')
+text = text.replace('parseCertificateContent(String certContent)', 'parseCertificateContent(final String certContent)')
+text = text.replace('shouldRequestBoundToken(X509Certificate cert)', 'shouldRequestBoundToken(final X509Certificate cert)')
+
+with open('/usr/local/google/home/mcastelaz/google-cloud-java/google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/AgentIdentityUtils.java', 'w') as f:
+    f.write(text)

get_reviews.py

@@ -0,0 +1,7 @@
+import urllib.request
+import json
+req = urllib.request.Request('https://api.github.com/repos/googleapis/google-cloud-java/pulls/13901/comments')
+with urllib.request.urlopen(req) as response:
+    comments = json.loads(response.read().decode())
+    for c in comments:
+        print(f"[{c['user']['login']}] {c['path']}:{c.get('line')}\n{c['body']}\n")