fix(frameworks): re-assert SSR public invoker on deploy#10690
Open
leoortizz wants to merge 3 commits into
Open
fix(frameworks): re-assert SSR public invoker on deploy#10690leoortizz wants to merge 3 commits into
leoortizz wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces changes to ensure that frameworks-managed SSR functions remain publicly invokable by additively granting the allUsers member the Cloud Run Invoker role (roles/run.invoker). This is implemented via a new ensureInvokerPublic utility in src/gcp/run.ts and integrated into the Fabricator deployment flow. Feedback on the changes highlights a potential TypeError in src/gcp/run.ts if currentInvokerBinding.members is undefined, suggesting the use of optional chaining to safely check for the presence of allUsers.
The generated SSR function declares no invoker, so updates never re-applied it; a binding lost out-of-band left channels 403ing. Now each frameworks deploy re-adds allUsers additively, warning instead of failing when blocked. Fixes #10631
64171f1 to
25b6972
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #10631.
Framework SSR runs on a Cloud Run function that Hosting invokes anonymously, so its service needs
allUsersasrun.invoker. The generated function declares no invoker, so only the create path set it and the update path never re-applied it. When that binding gets removed out-of-band such as by an org policy, later deploys don't restore it, andhosting:channel:deployships a channel that 403s on all server-rendered content while reporting success.This adds
run.ensureInvokerPublic(service), called for frameworks SSR functions on deploy. It is additive: it appendsallUsers, keeps any existing invoker members, and is a no-op whenallUsersis already present, so it never overwrites deliberate IAM changes. It is also non-fatal: if the grant is blocked it logs a warning instead of failing the deploy. The check is scoped to frameworks codebases, so other functions are untouched.Scenarios Tested
Ran a Next.js app with next/image and dynamic routes on a live project. I stripped
allUsersand ranhosting:channel:deploy; the binding was restored automatically and server-rendered routes returned 200. I then pre-seeded a custom invoker service account, strippedallUsers, and deployed;allUserswas added back and the custom account was preserved.The issue author suspected the preview deploys might be stripping the binding, so I checked: shipped 15.19.1 does not modify the invoker policy on live or channel deploys, which points to the loss being external rather than deploy-caused.
Added unit tests for
ensureInvokerPubliccovering add, additive, and no-op, and for theupdateV2Functionbranch covering ensure, warn-on-failure, and skip for non-frameworks functions.Sample Commands
No new commands or flags.