[Draft] fix: preserve existing flag across triggers#10412
[Draft] fix: preserve existing flag across triggers#10412
Conversation
Introduces `mergeInvokerBinding` in iam.ts — a focused helper that rebuilds
the bindings array around a single unconditional binding for the given role,
optionally unioning its members with the declared set.
`setInvokerUpdate` in both run.ts and cloudfunctions.ts now accepts an
options arg `{mergeExistingMembers?: boolean}`. When true, externally added
members on the existing unconditional invoker binding are preserved — the
building block needed to honor `preserveExternalChanges` at the fabricator
layer (wired up in a later commit).
As a side fix, conditional bindings on the invoker role (those with a
`condition` field) are now preserved across updates. Previously the filter
`.filter(b => b.role !== invokerRole)` dropped every binding with that role,
silently losing any out-of-band conditional access policy. Both `run.ts`
and `cloudfunctions.ts` had the same bug.
Default behavior (options absent, or `mergeExistingMembers: false`) is
unchanged — the replace semantics that existed before still apply.
Adds `preserveExternalChanges?: Field<boolean>` to the v1alpha1 manifest wire format, the build endpoint type, and the manifest validator. Mirrors the existing `omit` plumbing (same Field<boolean> semantics, same copyIfPresent pattern). Without this, the flag survives in the user's SDK config and in firebase-config.json but gets dropped at decode time — which is why it silently does nothing today for IAM invoker preservation. No behavior change yet: the flag is carried through decode, but nothing reads it. Wiring it into the backend endpoint and the fabricator follows.
Adds `preserveExternalChanges?: boolean | null` to backend.ServiceConfiguration so the backend endpoint carries the user's intent into fabricator. `toBackend` resolves the build-layer Field<boolean> via the existing Resolver pattern, swallowing resolution errors and defaulting to `false` — matching how `omit` is treated. Users who never set the flag keep `undefined`, so the existing replace-on-deploy behavior is unchanged by this commit. Still no behavior change at the IAM layer: the value is available to the fabricator but not yet read there. That's the next commit.
… site
Every `setInvoker*` site now branches on `endpoint.preserveExternalChanges`:
- When true: route through `setInvokerUpdate` with
`{ mergeExistingMembers: true }`, so externally-added invoker members are
preserved across deploys.
- When false/absent: keep today's replace semantics so clean-up via
redeploy still works for users who haven't opted in.
Covers all 13 call sites across v1 + v2 × create + update × all trigger
types (HTTPS, callable, task queue, blocking, scheduled, dataConnect,
run-native httpsTrigger). The v1 create path introduces a small
`setV1Invoker` local helper and v2 introduces `setV2Invoker` to avoid
repeating the ternary seven times per function.
Fixes #6549. Subsumes #10411
— that PR made merge unconditional for scheduled v2, which would have
regressed the documented `preserveExternalChanges: false` default for
every other user. This commit gates the behavior on the user's declared
intent and covers every trigger type, not just scheduled.
Adds an end-to-end test that: 1. Deploys with preserveExternalChanges=true 2. Adds an external `user:` member to the v2scheduled function's Cloud Run `roles/run.invoker` binding via the real IAM API 3. Redeploys (flag still on) — asserts the member is still present 4. Redeploys with flag off — asserts the member is removed This is the scenario from issue #6549 end-to-end. Requires a real GCP test project to run (`npm run test:functions-deploy`); unit + fabricator tests from earlier commits cover the decoding/gating paths for CI speed.
| json.policy.bindings[0].members.sort(); | ||
| expect(json.policy.bindings[0].members).to.deep.eq([ | ||
| const invoker = json.policy.bindings.find( | ||
| (b: any) => b.role === "roles/run.invoker" && !b.condition, |
![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 introduces the preserveExternalChanges flag to Firebase Functions deployments, allowing users to prevent the overwriting of externally-modified IAM invoker bindings on Cloud Run and Cloud Functions. The changes include updates to the backend configuration, build process, and fabricator logic to support merging existing IAM members when the flag is enabled. The review feedback identifies several improvement opportunities: removing a silent error suppression in the build process to ensure CEL resolution failures are reported, and standardizing member deduplication in the IAM merging logic across iam.ts, cloudfunctions.ts, and run.ts to ensure consistent behavior and robust optimization checks.
| proto.convertIfPresent( | ||
| bkEndpoint, | ||
| bdEndpoint, | ||
| "preserveExternalChanges", | ||
| nullsafeVisitor((v) => { | ||
| try { | ||
| return r.resolveBoolean(v); | ||
| } catch { | ||
| return false; | ||
| } | ||
| }), | ||
| ); |
There was a problem hiding this comment.
The try-catch block silently swallows resolution errors for the preserveExternalChanges field. If a CEL expression fails to resolve (e.g., due to a missing parameter or type mismatch), it should propagate the error to the user rather than defaulting to false. This ensures consistency with how other fields are handled in toBackend.
proto.convertIfPresent(
bkEndpoint,
bdEndpoint,
"preserveExternalChanges",
nullsafeVisitor((v) => r.resolveBoolean(v)),
);| const finalMembers = | ||
| options.merge && existingUnconditional | ||
| ? Array.from(new Set([...existingUnconditional.members, ...members])) | ||
| : [...members]; | ||
| result.push({ role, members: finalMembers }); |
There was a problem hiding this comment.
The finalMembers calculation does not deduplicate members when options.merge is false. If the input members array contains duplicates, they will be persisted in the IAM policy. It is better to always deduplicate the resulting members list using a Set to ensure policy cleanliness and avoid unnecessary updates due to duplicate entries.
const finalMembers = Array.from(
new Set([
...(options.merge && existingUnconditional ? existingUnconditional.members : []),
...members,
]),
);
result.push({ role, members: finalMembers });| const desiredMembers = | ||
| options?.mergeExistingMembers && currentUnconditional | ||
| ? Array.from(new Set([...currentUnconditional.members, ...invokerMembers])) | ||
| : invokerMembers; | ||
| if ( | ||
| currentInvokerBinding && | ||
| JSON.stringify(currentInvokerBinding.members.sort()) === JSON.stringify(invokerMembers.sort()) | ||
| currentUnconditional && | ||
| JSON.stringify([...currentUnconditional.members].sort()) === | ||
| JSON.stringify([...desiredMembers].sort()) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The desiredMembers calculation should deduplicate invokerMembers even when mergeExistingMembers is false. This ensures that the optimization check on the following lines (which uses JSON.stringify comparison) correctly identifies when no changes are needed, even if the input invoker array contains duplicates. Additionally, deduplicating both sides of the comparison makes the check more robust.
const desiredMembers = Array.from(
new Set([
...(options?.mergeExistingMembers && currentUnconditional ? currentUnconditional.members : []),
...invokerMembers,
]),
);
if (
currentUnconditional &&
JSON.stringify([...currentUnconditional.members].sort()) ===
JSON.stringify([...desiredMembers].sort())
) {
return;
}| const desiredMembers = | ||
| options?.mergeExistingMembers && currentUnconditional | ||
| ? Array.from(new Set([...currentUnconditional.members, ...invokerMembers])) | ||
| : invokerMembers; | ||
| if ( | ||
| currentInvokerBinding && | ||
| JSON.stringify(currentInvokerBinding.members.sort()) === JSON.stringify(invokerMembers.sort()) | ||
| currentUnconditional && | ||
| JSON.stringify([...currentUnconditional.members].sort()) === | ||
| JSON.stringify([...desiredMembers].sort()) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The desiredMembers calculation should deduplicate invokerMembers even when mergeExistingMembers is false. This ensures that the optimization check on the following lines correctly identifies when no changes are needed, even if the input invoker array contains duplicates. This maintains consistency with the logic in cloudfunctions.ts.
const desiredMembers = Array.from(
new Set([
...(options?.mergeExistingMembers && currentUnconditional ? currentUnconditional.members : []),
...invokerMembers,
]),
);
if (
currentUnconditional &&
JSON.stringify([...currentUnconditional.members].sort()) ===
JSON.stringify([...desiredMembers].sort())
) {
return;
}1 participant
Supersedes #10411
Exploring a fix for #6549