Repository navigation
fix(app): Query metadata server for ComputeCredential project ID - #590
lahirumaramba wants to merge 2 commits into
Conversation
Apps running on Cloud Run, Compute Engine, or Google Kubernetes Engine often authenticate with compute credentials instead of a service account key file. Because compute credentials do not embed a project ID, `FirebaseApp.GetProjectId()` returns `null` unless the caller sets a project ID in `AppOptions` or an environment variable, causing Messaging and Auth calls to fail. This change falls back to reading the project ID from the local Google Cloud metadata server when an app uses a `ComputeCredential`, and caches the result on the `FirebaseApp` instance. Explicit `AppOptions.ProjectId` and environment variables still take precedence. ### Technical Details Fixes #588 - `FirebaseAdmin/FirebaseAdmin/Extensions.cs`: Add `ToComputeCredential()` alongside `ToServiceAccountCredential()` to unwrap nested `GoogleCredential` instances. - `FirebaseAdmin/FirebaseAdmin/FirebaseApp.cs`: Fall back to querying `http://metadata.google.internal/computeMetadata/v1/project/project-id` via `HttpClientFactory.CreateDefaultHttpClient()` (matching `IAMSigner`) and cache the trimmed project ID per `FirebaseApp` instance via `Lazy<string>`. - `FirebaseAdmin/FirebaseAdmin.Tests/FirebaseAppTest.cs`: Add unit tests covering metadata server discovery, per-app caching, error fallback, and option precedence using `MockHttpClientFactory` and `MockMessageHandler`.
There was a problem hiding this comment.
Code Review
This pull request introduces support for discovering the GCP project ID from the local metadata server when using ComputeCredential. It adds a ToComputeCredential extension method to extract the credential, lazily queries the metadata server, and includes corresponding unit tests. The review feedback highlights three key improvements: setting a timeout on the metadata server HTTP request to prevent startup hangs, accessing the private this.options field directly to avoid unnecessary allocations, and adding a null check to the ToComputeCredential extension method to prevent potential null reference exceptions.
| using (var client = clientFactory.CreateDefaultHttpClient()) | ||
| { | ||
| client.DefaultRequestHeaders.Add("Metadata-Flavor", "Google"); | ||
| var resp = await client.GetAsync(MetadataServerUrl).ConfigureAwait(false); | ||
| resp.EnsureSuccessStatusCode(); | ||
| var projectId = (await resp.Content.ReadAsStringAsync().ConfigureAwait(false)).Trim(); | ||
| return string.IsNullOrEmpty(projectId) ? null : projectId; | ||
| } |
There was a problem hiding this comment.
The HTTP request to the local metadata server does not specify a timeout. By default, HttpClient has a timeout of 100 seconds. If the metadata server is unreachable (e.g., when running in a non-GCP environment or during local testing), this call will block the thread for a long time, potentially causing application startup hangs or health check failures. Setting a short timeout (e.g., 2 seconds) is highly recommended for metadata server queries.
using (var client = clientFactory.CreateDefaultHttpClient())
{
client.Timeout = TimeSpan.FromSeconds(2);
client.DefaultRequestHeaders.Add("Metadata-Flavor", "Google");
var resp = await client.GetAsync(MetadataServerUrl).ConfigureAwait(false);
resp.EnsureSuccessStatusCode();
var projectId = (await resp.Content.ReadAsStringAsync().ConfigureAwait(false)).Trim();
return string.IsNullOrEmpty(projectId) ? null : projectId;
}| if (this.Options.Credential.ToComputeCredential() != null) | ||
| { | ||
| return this.metadataProjectId.Value; | ||
| } |
There was a problem hiding this comment.
The Options property getter creates and returns a new copy of AppOptions on every access. Accessing this.Options multiple times within GetProjectId() leads to unnecessary object allocations. Since this is an internal method of FirebaseApp, you should access the private field this.options directly to avoid these allocations.
if (this.options.Credential.ToComputeCredential() != null)
{
return this.metadataProjectId.Value;
}| public static ComputeCredential ToComputeCredential( | ||
| this GoogleCredential credential) | ||
| { | ||
| if (credential.UnderlyingCredential is GoogleCredential) | ||
| { | ||
| return ((GoogleCredential)credential.UnderlyingCredential) | ||
| .ToComputeCredential(); | ||
| } | ||
|
|
||
| return credential.UnderlyingCredential as ComputeCredential; | ||
| } |
There was a problem hiding this comment.
The extension method ToComputeCredential does not perform a null check on the credential parameter. If called on a null GoogleCredential instance, it will throw a NullReferenceException. Adding a null check improves robustness and adheres to defensive programming practices.
public static ComputeCredential ToComputeCredential(
this GoogleCredential credential)
{
if (credential == null)
{
return null;
}
if (credential.UnderlyingCredential is GoogleCredential)
{
return ((GoogleCredential)credential.UnderlyingCredential)
.ToComputeCredential();
}
return credential.UnderlyingCredential as ComputeCredential;
}
Apps running on Cloud Run, Compute Engine, or Google Kubernetes Engine often authenticate with compute credentials instead of a service account key file. Because compute credentials do not embed a project ID,
FirebaseApp.GetProjectId()returnsnullunless the caller sets a project ID inAppOptionsor an environment variable, causing Messaging and Auth calls to fail.This change falls back to reading the project ID from the local Google Cloud metadata server when an app uses a
ComputeCredential, and caches the result on theFirebaseAppinstance. ExplicitAppOptions.ProjectIdand environment variables still take precedence.Technical Details
Fixes #588
FirebaseAdmin/FirebaseAdmin/Extensions.cs: AddToComputeCredential()alongsideToServiceAccountCredential()to unwrap nestedGoogleCredentialinstances.FirebaseAdmin/FirebaseAdmin/FirebaseApp.cs: Fall back to queryinghttp://metadata.google.internal/computeMetadata/v1/project/project-idviaHttpClientFactory.CreateDefaultHttpClient()(matchingIAMSigner) and cache the trimmed project ID perFirebaseAppinstance viaLazy<string>.FirebaseAdmin/FirebaseAdmin.Tests/FirebaseAppTest.cs: Add unit tests covering metadata server discovery, per-app caching, error fallback, and option precedence usingMockHttpClientFactoryandMockMessageHandler.