fix: preserve existing cloud run invokers for scheduled v2 functions#10411
fix: preserve existing cloud run invokers for scheduled v2 functions#10411IzaakGough wants to merge 1 commit intomainfrom
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Cloud Run invoker logic to support merging existing IAM members instead of overwriting them, specifically for scheduled functions. It replaces setInvokerCreate with setInvokerUpdate in the fabricator's creation flow and introduces an InvokerUpdateOptions interface to toggle member merging. Corresponding unit tests have been updated and expanded to cover these changes. I have no feedback to provide.
|
Ah so the main problem is, as the PR description says - you're making it unconditional, whereas the proper fix would make |
|
Closing for now, have a new branch with a full fix on incoming. |
Summary
Fixes #6549
Scheduled GCFv2 functions currently overwrite the
roles/run.invokerbinding on Cloud Run during deploy. This causes externally added invoker members to be removed on create/update, even if the scheduler-required service account is still present.This change ensures that scheduled GCFv2 deploys preserve existing Cloud Run invoker members by merging them with the required scheduler invoker, instead of replacing the binding outright.
What Changed
Added an optional merge mode to:
Updated scheduled GCFv2 create flow in
src/deploy/functions/release/fabricator.tsto use:
Updated scheduled GCFv2 update flow in the same file to pass the same merge option.
Added helper-level test coverage in
src/gcp/run.spec.tsAdded deploy-layer test coverage in
src/deploy/functions/release/fabricator.spec.tsfor scheduled create/update call paths.Why
Before this change, scheduled GCFv2 deploys computed a single desired invoker:
serviceAccount, orThis value was then written back as the full
roles/run.invokerbinding, which removed any additional invoker members added outside of Firebase.Reproduction
Further details of this reproduction can be seen here
Tests
Added coverage for:
preserving existing invoker members
Scheduled
createV2Functionusing merge mode with:Scheduled
updateV2Functionusing merge modeImportant Behavior Note
This change is currently unconditional for scheduled GCFv2 functions. It does not consult or derive from the
preserveExternalChanges / preserve_external_changesoption.Instead, scheduled-function invoker updates now always merge existing
roles/run.invokermembers by default. This is intentional for this fix, but worth calling out explicitly.Potential Follow-up
A future improvement could thread a real
preserveExternalChangessignal into this path, making the behavior conditional rather than always-on for scheduled GCFv2 functions.