feat(auth): make isOnComputeEngine more resilient - #343
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 enhances Google Compute Engine (GCE) detection by adding static detection via the Linux DMI BIOS product name, introducing retry logic, and falling back to a direct IP ping (169.254.169.254) if the metadata host ping fails. However, two issues were identified in the feedback: first, the retry loop incorrectly returns false immediately on an invalid response (such as a non-200 status or missing header), which prevents subsequent retries and fallbacks; second, increasing the default timeout to 3 seconds with 3 retries and IP fallback can cause up to a 12-second delay during startup in non-GCE environments, so a shorter timeout of 1 second is recommended.
|
/gbcrun |
|
/gcbrun |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Google Compute Engine (GCE) detection logic by introducing internalIsOnComputeEngine. This new implementation adds retry logic for transient HTTP errors, a request timeout, and a fallback to static GCE detection on Linux via the DMI BIOS product name. It also updates the test suite to thoroughly cover these scenarios. The review feedback points out a potential unhandled asynchronous exception in Dart when a request times out and subsequently fails without an active listener. It suggests attaching .catchError immediately upon request creation to prevent this, which also allows for simplifying the client cleanup logic in the finally block.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…an/google-cloud-dart into fix_on_compute_engine
|
/gbcrun |
1 similar comment
|
/gbcrun |
|
/gcbrun |
| if (!isLinux) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[optional] I like to omit braces for guard clauses that fit on one line.
| if (!isLinux) { | |
| return false; | |
| } | |
| if (!isLinux) return false; |
| } | ||
| try { | ||
| final content = await File(path).readAsString(); | ||
| return content.trim().startsWith('Google'); |
There was a problem hiding this comment.
Is this the standard pattern for other SDKs too?
| @internal | ||
| Future<bool> internalIsOnComputeEngine({ | ||
| http.Client? client, | ||
| String? Function(String name)? getEnvironmentVariable, |
There was a problem hiding this comment.
[nit] How about readEnvironment?
| // which the `http.Client` API contract doesn't allow. Therefore, only | ||
| // tests are allowed to provide `client`. | ||
| if (closeClient) { | ||
| unawaited(Future.wait(inFlightRequests).whenComplete(httpClient.close)); |
There was a problem hiding this comment.
[nit]
| unawaited(Future.wait(inFlightRequests).whenComplete(httpClient.close)); | |
| unawaited(inFlightRequests.wait.whenComplete(httpClient.close)); |
2 participants
No description provided.