fix(oauth2_http): Avoid retrying on 4xx errors during GCE metadata ping#13715
fix(oauth2_http): Avoid retrying on 4xx errors during GCE metadata ping#13715macastelaz wants to merge 9 commits into
Conversation
…erification in tests
There was a problem hiding this comment.
Code Review
This pull request updates ComputeEngineCredentials to handle 403 Forbidden responses during the Compute Engine metadata server ping check by immediately returning false instead of logging an unexpected exception. It also introduces a new test case to verify this behavior and adds request counting to MockMetadataServerTransport to ensure no retries occur. There are no review comments, and I have no additional feedback to provide.
fe14912 to
b5f6618
Compare
| if (e instanceof HttpResponseException) { | ||
| int statusCode = ((HttpResponseException) e).getStatusCode(); | ||
| if (statusCode >= 400 && statusCode < 500) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
nit: this can also be done in a follow-up refactor PR if we agree on this.
Do you think we can use HttpRequest's setUnsuccessfulResponseHandler as well as the ExponentialBackoff + setNumberOfRetries to configure retries/ handle failure? Or do you know if there a reason ComputeEngineCredentials can't use that?
There was a problem hiding this comment.
To be honest, I wouldn't call myself an expert here but here's what Gemini had to say:
"Since pingComputeEngineMetadata is a lightweight detection mechanism that executes on startup for non-GCE environments to check if it's on GCE, we use a strict 500ms connection timeout and try to fail fast. If we were to use ExponentialBackOff , any timeout or retryable network failure would introduce sleep delays between retries, which would exponentially delay application startup on local environments. The manual loop ensures we do 3 rapid checks and fail out without hanging the thread."
Given the small number of retries, I would suspect any additional latency from retry delays would be minimal but in theory, the ideal of wanting to of avoid introducing any additional startup latency to backoff retries makes sense to me. Thoughts?
92bc27d to
bf4d821
Compare
Don't retry GCE Metadata 4xx errors which are generally non-retriable errors and only adds startup noise/latency by retrying 3 times.
Fixes #13649