feat(firestore-bigquery-export): authenticate TRANSFORM_FUNCTION calls#2868
feat(firestore-bigquery-export): authenticate TRANSFORM_FUNCTION calls#2868Murayu0225 wants to merge 3 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request adds support for authenticating calls to the configured transform function in the Firestore BigQuery Change Tracker. It introduces google-auth-library to mint an ID token for the transform function URL and appends the authorization headers to the outgoing fetch request, falling back to an unauthenticated call if token minting fails. The review feedback suggests optimizing performance by caching the resolved IdTokenClient on the class instance to avoid repeated initialization, and improving error handling by checking response.ok before parsing the response as JSON.
| let authHeaders = {}; | ||
| try { | ||
| const auth = new GoogleAuth(); | ||
| const client = await auth.getIdTokenClient( | ||
| this.config.transformFunction | ||
| ); | ||
| authHeaders = await client.getRequestHeaders(); | ||
| } catch (err) { | ||
| logs.transformFunctionAuthFailed(err); | ||
| } |
There was a problem hiding this comment.
Instantiating new GoogleAuth() and calling getIdTokenClient on every single transformRows invocation introduces unnecessary overhead, especially since the transformFunction URL is constant for the tracker instance.
We can optimize this by caching the resolved IdTokenClient on the class instance (e.g., using (this as any).authClient). This avoids repeated initialization and improves performance during high-throughput document writes while maintaining clean test isolation.
let authHeaders = {};
try {
if (!(this as any).authClient) {
const auth = new GoogleAuth();
(this as any).authClient = await auth.getIdTokenClient(
this.config.transformFunction
);
}
authHeaders = await (this as any).authClient.getRequestHeaders();
} catch (err) {
logs.transformFunctionAuthFailed(err);
}There was a problem hiding this comment.
This would be a nice optimization but not blocking for this PR
| const response = await fetch(this.config.transformFunction, { | ||
| method: "post", | ||
| body: JSON.stringify({ data: rows }), | ||
| headers: { "Content-Type": "application/json" }, | ||
| headers: { "Content-Type": "application/json", ...authHeaders }, | ||
| }); | ||
| const responseJson = await response.json(); |
There was a problem hiding this comment.
When the transform function returns a non-2xx status code (e.g., 401 Unauthorized or 403 Forbidden due to misconfigured IAM permissions), the response body is often HTML rather than JSON. Calling response.json() directly in these cases will throw a cryptic JSON parsing error.
Checking response.ok first and throwing a descriptive error makes debugging authentication and configuration issues much easier.
const response = await fetch(this.config.transformFunction, {
method: "post",
body: JSON.stringify({ data: rows }),
headers: { "Content-Type": "application/json", ...authHeaders },
});
if (!response.ok) {
throw new Error(
'Transform function failed with status ' + response.status + ': ' + response.statusText
);
}
const responseJson = await response.json();
IzaakGough
left a comment
There was a problem hiding this comment.
Looks good overall! Can we address the gemini comment regarding the response.ok handling?
| let authHeaders = {}; | ||
| try { | ||
| const auth = new GoogleAuth(); | ||
| const client = await auth.getIdTokenClient( | ||
| this.config.transformFunction | ||
| ); | ||
| authHeaders = await client.getRequestHeaders(); | ||
| } catch (err) { | ||
| logs.transformFunctionAuthFailed(err); | ||
| } |
There was a problem hiding this comment.
This would be a nice optimization but not blocking for this PR
| const response = await fetch(this.config.transformFunction, { | ||
| method: "post", | ||
| body: JSON.stringify({ data: rows }), | ||
| headers: { "Content-Type": "application/json" }, | ||
| headers: { "Content-Type": "application/json", ...authHeaders }, | ||
| }); | ||
| const responseJson = await response.json(); |
|
Thanks for the review! I've addressed the comment regarding response.ok handling. Could you take another look? |
IzaakGough
left a comment
There was a problem hiding this comment.
@Murayu0225 This looks great! Thanks for addressing the comments.
|
@IzaakGough Thanks for checking! I've fixed the linting error that was causing the CI to fail. |
Resolves #2804
transformRowsnow mints an OIDC ID token for the configuredTRANSFORM_FUNCTIONURL and attaches it as anAuthorization: Bearer ...header. Users can then restrict the transform endpoint via Cloud Run IAM by grantingroles/run.invokerto the extension's runtime service account; the extension itself does not modify IAM policies.Backward compatibility:
google-auth-libraryis added as a direct dependency (previously pulled in transitively viafirebase-admin). Unit tests cover the authenticated, fallback, and disabled paths.