From d93d55811f91d91b3a6d0b05a012333775d153b2 Mon Sep 17 00:00:00 2001 From: Izaak Gough Date: Thu, 24 Sep 2026 13:07:26 +0100 Subject: [PATCH 1/3] fix(functions): grant Genkit monitoring roles after the managed service account exists On the first deploy of a codebase using declarative security, prepare granted the Genkit monitoring roles to a managed service account the fabricator had not created yet, so the project IAM update was rejected. The grant now runs in release after grantNewRoles. Dry runs still report the pending bindings from prepare. Fixes #11123 --- CHANGELOG.md | 1 + src/deploy/functions/checkIam.spec.ts | 98 ++----------------- src/deploy/functions/checkIam.ts | 12 +-- src/deploy/functions/prepare.ts | 16 +-- .../functions/release/fabricator.spec.ts | 33 +++++++ src/deploy/functions/release/fabricator.ts | 20 ++-- 6 files changed, 70 insertions(+), 110 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e69de29bb2d..205a6e9d539 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -0,0 +1 @@ +- [fixed] Grant Genkit monitoring roles after the managed service account is created when a codebase first opts into declarative security. (#11123) diff --git a/src/deploy/functions/checkIam.spec.ts b/src/deploy/functions/checkIam.spec.ts index b1dadcef3c6..f509ca3b8b8 100644 --- a/src/deploy/functions/checkIam.spec.ts +++ b/src/deploy/functions/checkIam.spec.ts @@ -246,64 +246,13 @@ describe("checkIam", () => { describe("ensureGenkitMonitoringRoles", () => { it("should return early if we do not have new endpoints", async () => { - const fn1: backend.Endpoint = { - id: "genkitFn1", - platform: "gcfv2", - entryPoint: "genkitFn1", - callableTrigger: { - genkitAction: "action", - }, - ...SPEC, - }; - const fn2: backend.Endpoint = { - id: "genkitFn2", - platform: "gcfv2", - entryPoint: "genkitFn2", - callableTrigger: { - genkitAction: "action", - }, - ...SPEC, - }; - const wantFn: backend.Endpoint = { - id: "wantGenkitFnFn", - entryPoint: "wantGenkitFn", - platform: "gcfv2", - callableTrigger: { - genkitAction: "action", - }, - ...SPEC, - }; - - await checkIam.ensureGenkitMonitoringRoles( - projectId, - projectNumber, - backend.of(wantFn), - backend.of(fn1, fn2, wantFn), - ); + await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, []); expect(getIamStub).to.not.have.been.called; expect(setIamStub).to.not.have.been.called; }); it("should return early if none of the new endpoints are genkit", async () => { - const fn1: backend.Endpoint = { - id: "genkitFn1", - platform: "gcfv2", - entryPoint: "genkitFn1", - callableTrigger: { - genkitAction: "action", - }, - ...SPEC, - }; - const fn2: backend.Endpoint = { - id: "genkitFn2", - platform: "gcfv2", - entryPoint: "genkitFn2", - callableTrigger: { - genkitAction: "action", - }, - ...SPEC, - }; const wantFn1: backend.Endpoint = { id: "wantFn1", entryPoint: "wantFn1", @@ -323,12 +272,7 @@ describe("checkIam", () => { ...SPEC, }; - await checkIam.ensureGenkitMonitoringRoles( - projectId, - projectNumber, - backend.of(wantFn1, wantFn2), - backend.of(fn1, fn2), - ); + await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn1, wantFn2]); expect(getIamStub).to.not.have.been.called; expect(setIamStub).to.not.have.been.called; @@ -346,14 +290,8 @@ describe("checkIam", () => { ...SPEC, }; - await expect( - checkIam.ensureGenkitMonitoringRoles( - projectId, - projectNumber, - backend.of(wantFn), - backend.empty(), - ), - ).to.not.be.rejected; + await expect(checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn])).to.not + .be.rejected; expect(getIamStub).to.have.been.calledOnce; expect(getIamStub).to.have.been.calledWith(projectNumber); expect(setIamStub).to.not.have.been.called; @@ -376,12 +314,7 @@ describe("checkIam", () => { }; await expect( - checkIam.ensureGenkitMonitoringRoles( - projectId, - projectNumber, - backend.of(wantFn), - backend.empty(), - ), + checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn]), ).to.be.rejectedWith( "We failed to modify the IAM policy for the project. The functions " + "deployment requires specific roles to be granted to service agents," + @@ -425,12 +358,7 @@ describe("checkIam", () => { ...SPEC, }; - await checkIam.ensureGenkitMonitoringRoles( - projectId, - projectNumber, - backend.of(wantFn), - backend.empty(), - ); + await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn]); expect(getIamStub).to.have.been.calledOnce; expect(getIamStub).to.have.been.calledWith(projectNumber); @@ -467,12 +395,7 @@ describe("checkIam", () => { ...SPEC, }; - await checkIam.ensureGenkitMonitoringRoles( - projectId, - projectNumber, - backend.of(wantFn), - backend.empty(), - ); + await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn]); expect(getIamStub).to.have.been.calledOnce; expect(getIamStub).to.have.been.calledWith(projectNumber); @@ -554,12 +477,7 @@ describe("checkIam", () => { ...SPEC, }; - await checkIam.ensureGenkitMonitoringRoles( - projectId, - projectNumber, - backend.of(fn1, fn2, fn3, fn4), - backend.empty(), - ); + await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [fn1, fn2, fn3, fn4]); expect(getIamStub).to.have.been.calledOnce; expect(getIamStub).to.have.been.calledWith(projectNumber); diff --git a/src/deploy/functions/checkIam.ts b/src/deploy/functions/checkIam.ts index f39031ac079..f6fbc777a93 100644 --- a/src/deploy/functions/checkIam.ts +++ b/src/deploy/functions/checkIam.ts @@ -185,20 +185,20 @@ export async function obtainDefaultComputeServiceAgentBindings( /** * Checks and sets the roles for any genkit deployed functions that are required * for Firebase Genkit Monitoring. + * + * Must run after any managed service account referenced by the endpoints exists: + * the project IAM policy rejects members that do not exist yet. * @param projectId human readable project id * @param projectNumber project number - * @param want backend that we want to deploy - * @param have backend that we have currently deployed + * @param createdEndpoints endpoints being created by this deploy */ export async function ensureGenkitMonitoringRoles( projectId: string, projectNumber: string, - want: backend.Backend, - have: backend.Backend, + createdEndpoints: backend.Endpoint[], dryRun?: boolean, ): Promise { - const wantEndpoints = backend.allEndpoints(want).filter(isGenkitEndpoint); - const newEndpoints = wantEndpoints.filter(backend.missingEndpoint(have)); + const newEndpoints = createdEndpoints.filter(isGenkitEndpoint); if (newEndpoints.length === 0) { return; diff --git a/src/deploy/functions/prepare.ts b/src/deploy/functions/prepare.ts index 1ccfb4fd2a6..3649ce0aa9c 100644 --- a/src/deploy/functions/prepare.ts +++ b/src/deploy/functions/prepare.ts @@ -538,15 +538,15 @@ export async function prepare( haveBackend, options.dryRun, ); - await ensureGenkitMonitoringRoles( - projectId, - projectNumber, - matchingBackend, - haveBackend, - options.dryRun, - ); - // Actual granting of secret access permissions has been moved to the fabricator in release because declarative security may mean that the desired service account hasn't been created + // Genkit monitoring roles and secret access are granted by the fabricator in release because + // declarative security may mean that the desired service account hasn't been created yet. if (options.dryRun) { + await ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.allEndpoints(matchingBackend).filter(backend.missingEndpoint(haveBackend)), + options.dryRun, + ); const secretAccessDelta = await ensure.secretsAccessDelta({ projectId, wantBackend: matchingBackend, diff --git a/src/deploy/functions/release/fabricator.spec.ts b/src/deploy/functions/release/fabricator.spec.ts index 64127af797d..40528fb4a97 100644 --- a/src/deploy/functions/release/fabricator.spec.ts +++ b/src/deploy/functions/release/fabricator.spec.ts @@ -26,6 +26,7 @@ import { deepCopy } from "@angular-devkit/core"; import * as gce from "../../../gcp/computeEngine"; import * as iam from "../../../gcp/iam"; import * as resourcemanager from "../../../gcp/resourceManager"; +import * as checkIam from "../checkIam"; describe("Fabricator", () => { // Stub all GCP APIs to make sure this test is hermetic @@ -2232,6 +2233,38 @@ describe("Fabricator", () => { ); }); + it("should grant Genkit monitoring roles after creating the SA in applyPlan", async () => { + const ensureGenkitRolesStub = sinon.stub(checkIam, "ensureGenkitMonitoringRoles").resolves(); + const genkitEndpoint = endpoint( + { callableTrigger: { genkitAction: "flow" } }, + { serviceAccount: "firebase-fn-123@my-proj.iam.gserviceaccount.com" }, + ); + const deploymentPlan: planner.DeploymentPlan = { + default: { + plannedBackend: backend.of(genkitEndpoint), + regionalChangesets: { + "us-central1": { + endpointsToCreate: [genkitEndpoint], + endpointsToUpdate: [], + endpointsToDelete: [], + endpointsToSkip: [], + }, + }, + serviceAccountToCreate: "firebase-fn-123@my-proj.iam.gserviceaccount.com", + managedServiceAccount: "firebase-fn-123@my-proj.iam.gserviceaccount.com", + }, + }; + sinon.stub(fab, "applyUpserts").resolves([{ endpoint: genkitEndpoint, durationMs: 100 }]); + + await fab.applyPlan(deploymentPlan); + + expect(createServiceAccountStub).to.have.been.calledOnce; + expect(ensureGenkitRolesStub).to.have.been.calledOnceWithExactly("test-project", "1234567", [ + genkitEndpoint, + ]); + expect(ensureGenkitRolesStub).to.have.been.calledAfter(createServiceAccountStub); + }); + it("should clean up newly created SA on 100% deployment failure", async () => { const endpoint: backend.Endpoint = { id: "fn1", diff --git a/src/deploy/functions/release/fabricator.ts b/src/deploy/functions/release/fabricator.ts index ee652b22b08..6377b7cc83f 100644 --- a/src/deploy/functions/release/fabricator.ts +++ b/src/deploy/functions/release/fabricator.ts @@ -8,6 +8,7 @@ import { parseErrorCode, } from "./executor"; import * as ensure from "../ensure"; +import * as checkIam from "../checkIam"; import { FirebaseError } from "../../../error"; import { SourceTokenScraper } from "./sourceTokenScraper"; @@ -243,6 +244,19 @@ export class Fabricator { await this.grantNewRoles(codebasePlan, codebase); } + // Accumulate all regional changesets across all codebases + const allChangesets: planner.Changeset[] = []; + for (const codebasePlan of Object.values(plan)) { + allChangesets.push(...Object.values(codebasePlan.regionalChangesets)); + } + + // Also a project IAM policy update, so it stays sequential with grantNewRoles. + await checkIam.ensureGenkitMonitoringRoles( + this.projectId, + this.projectNumber, + allChangesets.flatMap((changes) => changes.endpointsToCreate), + ); + const secretAccessPromises = Object.values(plan).flatMap((codebasePlan) => Object.entries(codebasePlan.secretAccessPlan || {}).map(([secret, serviceAccounts]) => this.executor.run( @@ -260,12 +274,6 @@ export class Fabricator { ); await Promise.all(secretAccessPromises); - // Accumulate all regional changesets across all codebases - const allChangesets: planner.Changeset[] = []; - for (const codebasePlan of Object.values(plan)) { - allChangesets.push(...Object.values(codebasePlan.regionalChangesets)); - } - // Phase 1: Creates and Updates const createAndUpdatePromises = allChangesets.map((changes) => { const scraperV1 = new SourceTokenScraper(); From ec9514c44367e05d8600bb2f6c2791bf9b6b3663 Mon Sep 17 00:00:00 2001 From: Izaak Gough Date: Thu, 24 Sep 2026 13:41:49 +0100 Subject: [PATCH 2/3] fix(functions): grant Genkit monitoring roles through the codebase's required roles Folding the roles into requiredRoles puts them in the etag and the plan, so grantNewRoles grants them once the managed service account exists and later deploys do not revoke them. The prepare-time grant now skips managed accounts and keeps handling the default and explicit accounts. --- CHANGELOG.md | 2 +- src/deploy/functions/checkIam.spec.ts | 163 +++++++++++++++++- src/deploy/functions/checkIam.ts | 27 ++- src/deploy/functions/prepare.spec.ts | 50 ++++++ src/deploy/functions/prepare.ts | 29 +++- .../functions/release/fabricator.spec.ts | 33 ---- src/deploy/functions/release/fabricator.ts | 20 +-- 7 files changed, 252 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 205a6e9d539..e8e0146c7fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1 @@ -- [fixed] Grant Genkit monitoring roles after the managed service account is created when a codebase first opts into declarative security. (#11123) +- [fixed] Fixed the first deploy of a codebase using `requiresRole` failing when it contains a Genkit function, and kept the Genkit monitoring roles on the managed service account across later deploys. (#11123) diff --git a/src/deploy/functions/checkIam.spec.ts b/src/deploy/functions/checkIam.spec.ts index f509ca3b8b8..84b34c4d565 100644 --- a/src/deploy/functions/checkIam.spec.ts +++ b/src/deploy/functions/checkIam.spec.ts @@ -246,13 +246,129 @@ describe("checkIam", () => { describe("ensureGenkitMonitoringRoles", () => { it("should return early if we do not have new endpoints", async () => { - await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, []); + const fn1: backend.Endpoint = { + id: "genkitFn1", + platform: "gcfv2", + entryPoint: "genkitFn1", + callableTrigger: { + genkitAction: "action", + }, + ...SPEC, + }; + const fn2: backend.Endpoint = { + id: "genkitFn2", + platform: "gcfv2", + entryPoint: "genkitFn2", + callableTrigger: { + genkitAction: "action", + }, + ...SPEC, + }; + const wantFn: backend.Endpoint = { + id: "wantGenkitFnFn", + entryPoint: "wantGenkitFn", + platform: "gcfv2", + callableTrigger: { + genkitAction: "action", + }, + ...SPEC, + }; + + await checkIam.ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.of(wantFn), + backend.of(fn1, fn2, wantFn), + ); expect(getIamStub).to.not.have.been.called; expect(setIamStub).to.not.have.been.called; }); + it("should skip genkit endpoints that run as a managed service account", async () => { + const managedFn: backend.Endpoint = { + id: "managedGenkitFn", + platform: "gcfv2", + entryPoint: "managedGenkitFn", + serviceAccount: `firebase-fn-123@${projectId}.iam.gserviceaccount.com`, + callableTrigger: { + genkitAction: "action", + }, + ...SPEC, + }; + + await checkIam.ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.of(managedFn), + backend.empty(), + ); + + expect(getIamStub).to.not.have.been.called; + expect(setIamStub).to.not.have.been.called; + }); + + it("should only bind the accounts that are not managed", async () => { + const serviceAccount = `test-sa@${projectId}.iam.gserviceaccount.com`; + getIamStub.resolves({ etag: "etag", version: 3, bindings: [BINDING] }); + setIamStub.resolves({}); + const managedFn: backend.Endpoint = { + id: "managedGenkitFn", + platform: "gcfv2", + entryPoint: "managedGenkitFn", + serviceAccount: `firebase-fn-123@${projectId}.iam.gserviceaccount.com`, + callableTrigger: { + genkitAction: "action", + }, + ...SPEC, + }; + const customFn: backend.Endpoint = { + id: "customGenkitFn", + platform: "gcfv2", + entryPoint: "customGenkitFn", + serviceAccount, + callableTrigger: { + genkitAction: "action", + }, + ...SPEC, + }; + + await checkIam.ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.of(managedFn, customFn), + backend.empty(), + ); + + expect(setIamStub).to.have.been.calledOnce; + const policy = setIamStub.firstCall.args[1] as { + bindings: { role: string; members: string[] }[]; + }; + for (const role of checkIam.GENKIT_MONITORING_ROLES) { + const binding = policy.bindings.find((b) => b.role === role); + expect(binding?.members).to.deep.equal([`serviceAccount:${serviceAccount}`]); + } + }); + it("should return early if none of the new endpoints are genkit", async () => { + const fn1: backend.Endpoint = { + id: "genkitFn1", + platform: "gcfv2", + entryPoint: "genkitFn1", + callableTrigger: { + genkitAction: "action", + }, + ...SPEC, + }; + const fn2: backend.Endpoint = { + id: "genkitFn2", + platform: "gcfv2", + entryPoint: "genkitFn2", + callableTrigger: { + genkitAction: "action", + }, + ...SPEC, + }; const wantFn1: backend.Endpoint = { id: "wantFn1", entryPoint: "wantFn1", @@ -272,7 +388,12 @@ describe("checkIam", () => { ...SPEC, }; - await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn1, wantFn2]); + await checkIam.ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.of(wantFn1, wantFn2), + backend.of(fn1, fn2), + ); expect(getIamStub).to.not.have.been.called; expect(setIamStub).to.not.have.been.called; @@ -290,8 +411,14 @@ describe("checkIam", () => { ...SPEC, }; - await expect(checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn])).to.not - .be.rejected; + await expect( + checkIam.ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.of(wantFn), + backend.empty(), + ), + ).to.not.be.rejected; expect(getIamStub).to.have.been.calledOnce; expect(getIamStub).to.have.been.calledWith(projectNumber); expect(setIamStub).to.not.have.been.called; @@ -314,7 +441,12 @@ describe("checkIam", () => { }; await expect( - checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn]), + checkIam.ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.of(wantFn), + backend.empty(), + ), ).to.be.rejectedWith( "We failed to modify the IAM policy for the project. The functions " + "deployment requires specific roles to be granted to service agents," + @@ -358,7 +490,12 @@ describe("checkIam", () => { ...SPEC, }; - await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn]); + await checkIam.ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.of(wantFn), + backend.empty(), + ); expect(getIamStub).to.have.been.calledOnce; expect(getIamStub).to.have.been.calledWith(projectNumber); @@ -395,7 +532,12 @@ describe("checkIam", () => { ...SPEC, }; - await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [wantFn]); + await checkIam.ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.of(wantFn), + backend.empty(), + ); expect(getIamStub).to.have.been.calledOnce; expect(getIamStub).to.have.been.calledWith(projectNumber); @@ -477,7 +619,12 @@ describe("checkIam", () => { ...SPEC, }; - await checkIam.ensureGenkitMonitoringRoles(projectId, projectNumber, [fn1, fn2, fn3, fn4]); + await checkIam.ensureGenkitMonitoringRoles( + projectId, + projectNumber, + backend.of(fn1, fn2, fn3, fn4), + backend.empty(), + ); expect(getIamStub).to.have.been.calledOnce; expect(getIamStub).to.have.been.calledWith(projectNumber); diff --git a/src/deploy/functions/checkIam.ts b/src/deploy/functions/checkIam.ts index f6fbc777a93..3cf01d5f93e 100644 --- a/src/deploy/functions/checkIam.ts +++ b/src/deploy/functions/checkIam.ts @@ -141,12 +141,20 @@ function reduceEventsToServices(services: Array, endpoint: backend.Endp } /** Checks whether the given endpoint is a Genkit callable function. */ -function isGenkitEndpoint(endpoint: backend.Endpoint): boolean { +export function isGenkitEndpoint(endpoint: backend.Endpoint): boolean { return ( backend.isCallableTriggered(endpoint) && endpoint.callableTrigger.genkitAction !== undefined ); } +/** Checks whether the endpoint runs as a service account managed by declarative security. */ +function usesManagedServiceAccount(endpoint: backend.Endpoint): boolean { + return ( + typeof endpoint.serviceAccount === "string" && + endpoint.serviceAccount.startsWith("firebase-fn-") + ); +} + /** * Finds the required project level IAM bindings for the Pub/Sub service agent. * If the user enabled Pub/Sub on or before April 8, 2021, then we must enable the token creator role. @@ -185,20 +193,25 @@ export async function obtainDefaultComputeServiceAgentBindings( /** * Checks and sets the roles for any genkit deployed functions that are required * for Firebase Genkit Monitoring. - * - * Must run after any managed service account referenced by the endpoints exists: - * the project IAM policy rejects members that do not exist yet. * @param projectId human readable project id * @param projectNumber project number - * @param createdEndpoints endpoints being created by this deploy + * @param want backend that we want to deploy + * @param have backend that we have currently deployed */ export async function ensureGenkitMonitoringRoles( projectId: string, projectNumber: string, - createdEndpoints: backend.Endpoint[], + want: backend.Backend, + have: backend.Backend, dryRun?: boolean, ): Promise { - const newEndpoints = createdEndpoints.filter(isGenkitEndpoint); + // A managed service account may not exist until release, so its Genkit roles are part of the + // codebase's required roles and are granted with them in the fabricator. + const wantEndpoints = backend + .allEndpoints(want) + .filter(isGenkitEndpoint) + .filter((endpoint) => !usesManagedServiceAccount(endpoint)); + const newEndpoints = wantEndpoints.filter(backend.missingEndpoint(have)); if (newEndpoints.length === 0) { return; diff --git a/src/deploy/functions/prepare.spec.ts b/src/deploy/functions/prepare.spec.ts index a219917acf1..2e80de2483c 100644 --- a/src/deploy/functions/prepare.spec.ts +++ b/src/deploy/functions/prepare.spec.ts @@ -29,6 +29,7 @@ import * as functionsEnv from "../../functions/env"; import * as ensure from "./ensure"; import * as functionsConfig from "../../functionsConfig"; import * as args from "./args"; +import * as checkIam from "./checkIam"; describe("partition env helper", () => { it("splits a Record into two based on which keys begin with FIREBASE_SECRET_REF", () => { @@ -1517,6 +1518,55 @@ describe("prepare", () => { expect(e.labels?.["firebase-declarative-security-etag"]).to.equal(result.newEtag); }); + it("should add the Genkit monitoring roles to requiredRoles and the etag when a Genkit function is present", async () => { + const genkitFn: backend.Endpoint = { + ...ENDPOINT_BASE, + id: "genkit", + callableTrigger: { genkitAction: "flow" }, + }; + const want = backend.of({ ...ENDPOINT }, genkitFn); + want.requiredRoles = ["roles/viewer"]; + const have = backend.empty(); + + const result = await prepare.discoverSecurityDetails("default", want, have, "project"); + + const expectedRoles = ["roles/viewer", ...checkIam.GENKIT_MONITORING_ROLES]; + expect(want.requiredRoles).to.have.members(expectedRoles); + expect(want.requiredRoles).to.have.length(expectedRoles.length); + expect(result.newEtag).to.equal(iam.computeRolesEtag(expectedRoles)); + expect(result.newEtag).to.not.equal(iam.computeRolesEtag(["roles/viewer"])); + }); + + it("should report the Genkit monitoring roles as held when the etag already covers them", async () => { + const expectedRoles = ["roles/viewer", ...checkIam.GENKIT_MONITORING_ROLES]; + const etag = iam.computeRolesEtag(expectedRoles); + const genkitFn: backend.Endpoint = { + ...ENDPOINT_BASE, + id: "genkit", + callableTrigger: { genkitAction: "flow" }, + serviceAccount: "firebase-fn-123@project.iam.gserviceaccount.com", + labels: { "firebase-declarative-security-etag": etag }, + }; + const want = backend.of(genkitFn); + want.requiredRoles = ["roles/viewer"]; + const have = backend.of({ ...genkitFn, labels: { ...genkitFn.labels } }); + + const result = await prepare.discoverSecurityDetails("default", want, have, "project"); + + expect(result.newEtag).to.equal(etag); + expect(result.haveRoles).to.have.members(expectedRoles); + }); + + it("should not add the Genkit monitoring roles when no Genkit function is present", async () => { + const want = backend.of({ ...ENDPOINT }); + want.requiredRoles = ["roles/viewer"]; + const have = backend.empty(); + + await prepare.discoverSecurityDetails("default", want, have, "project"); + + expect(want.requiredRoles).to.deep.equal(["roles/viewer"]); + }); + it("should skip permission checks when haveRolesEtag matches newEtag", async () => { const etag = iam.computeRolesEtag(["roles/viewer"]); const e: backend.Endpoint = { diff --git a/src/deploy/functions/prepare.ts b/src/deploy/functions/prepare.ts index 3649ce0aa9c..e68d62c5905 100644 --- a/src/deploy/functions/prepare.ts +++ b/src/deploy/functions/prepare.ts @@ -40,7 +40,12 @@ import { promptForFailurePolicies, promptForMinInstances } from "./prompts"; import { needProjectId, needProjectNumber } from "../../projectUtils"; import { logger } from "../../logger"; import { ensureTriggerRegions } from "./triggerRegionHelper"; -import { ensureServiceAgentRoles, ensureGenkitMonitoringRoles } from "./checkIam"; +import { + ensureServiceAgentRoles, + ensureGenkitMonitoringRoles, + isGenkitEndpoint, + GENKIT_MONITORING_ROLES, +} from "./checkIam"; import { FirebaseError, getErrStack } from "../../error"; import { @@ -93,6 +98,12 @@ export async function discoverSecurityDetails( managedSA?: string; newEtag?: string; }> { + // Genkit Monitoring needs these roles on whichever account runs a Genkit function. Folding + // them into requiredRoles keeps them in the etag and the plan, so the fabricator grants them + // once the managed service account exists and later deploys do not revoke them. + if (want.requiredRoles && backend.someEndpoint(want, isGenkitEndpoint)) { + want.requiredRoles = Array.from(new Set([...want.requiredRoles, ...GENKIT_MONITORING_ROLES])); + } const requiredRoles = want.requiredRoles; // Note: On partial first rollouts (where at least one function successfully deployed), // haveBackend contains all active endpoints in GCP from list calls. firstHave.serviceAccount @@ -538,15 +549,15 @@ export async function prepare( haveBackend, options.dryRun, ); - // Genkit monitoring roles and secret access are granted by the fabricator in release because - // declarative security may mean that the desired service account hasn't been created yet. + await ensureGenkitMonitoringRoles( + projectId, + projectNumber, + matchingBackend, + haveBackend, + options.dryRun, + ); + // Actual granting of secret access permissions has been moved to the fabricator in release because declarative security may mean that the desired service account hasn't been created if (options.dryRun) { - await ensureGenkitMonitoringRoles( - projectId, - projectNumber, - backend.allEndpoints(matchingBackend).filter(backend.missingEndpoint(haveBackend)), - options.dryRun, - ); const secretAccessDelta = await ensure.secretsAccessDelta({ projectId, wantBackend: matchingBackend, diff --git a/src/deploy/functions/release/fabricator.spec.ts b/src/deploy/functions/release/fabricator.spec.ts index 40528fb4a97..64127af797d 100644 --- a/src/deploy/functions/release/fabricator.spec.ts +++ b/src/deploy/functions/release/fabricator.spec.ts @@ -26,7 +26,6 @@ import { deepCopy } from "@angular-devkit/core"; import * as gce from "../../../gcp/computeEngine"; import * as iam from "../../../gcp/iam"; import * as resourcemanager from "../../../gcp/resourceManager"; -import * as checkIam from "../checkIam"; describe("Fabricator", () => { // Stub all GCP APIs to make sure this test is hermetic @@ -2233,38 +2232,6 @@ describe("Fabricator", () => { ); }); - it("should grant Genkit monitoring roles after creating the SA in applyPlan", async () => { - const ensureGenkitRolesStub = sinon.stub(checkIam, "ensureGenkitMonitoringRoles").resolves(); - const genkitEndpoint = endpoint( - { callableTrigger: { genkitAction: "flow" } }, - { serviceAccount: "firebase-fn-123@my-proj.iam.gserviceaccount.com" }, - ); - const deploymentPlan: planner.DeploymentPlan = { - default: { - plannedBackend: backend.of(genkitEndpoint), - regionalChangesets: { - "us-central1": { - endpointsToCreate: [genkitEndpoint], - endpointsToUpdate: [], - endpointsToDelete: [], - endpointsToSkip: [], - }, - }, - serviceAccountToCreate: "firebase-fn-123@my-proj.iam.gserviceaccount.com", - managedServiceAccount: "firebase-fn-123@my-proj.iam.gserviceaccount.com", - }, - }; - sinon.stub(fab, "applyUpserts").resolves([{ endpoint: genkitEndpoint, durationMs: 100 }]); - - await fab.applyPlan(deploymentPlan); - - expect(createServiceAccountStub).to.have.been.calledOnce; - expect(ensureGenkitRolesStub).to.have.been.calledOnceWithExactly("test-project", "1234567", [ - genkitEndpoint, - ]); - expect(ensureGenkitRolesStub).to.have.been.calledAfter(createServiceAccountStub); - }); - it("should clean up newly created SA on 100% deployment failure", async () => { const endpoint: backend.Endpoint = { id: "fn1", diff --git a/src/deploy/functions/release/fabricator.ts b/src/deploy/functions/release/fabricator.ts index 6377b7cc83f..ee652b22b08 100644 --- a/src/deploy/functions/release/fabricator.ts +++ b/src/deploy/functions/release/fabricator.ts @@ -8,7 +8,6 @@ import { parseErrorCode, } from "./executor"; import * as ensure from "../ensure"; -import * as checkIam from "../checkIam"; import { FirebaseError } from "../../../error"; import { SourceTokenScraper } from "./sourceTokenScraper"; @@ -244,19 +243,6 @@ export class Fabricator { await this.grantNewRoles(codebasePlan, codebase); } - // Accumulate all regional changesets across all codebases - const allChangesets: planner.Changeset[] = []; - for (const codebasePlan of Object.values(plan)) { - allChangesets.push(...Object.values(codebasePlan.regionalChangesets)); - } - - // Also a project IAM policy update, so it stays sequential with grantNewRoles. - await checkIam.ensureGenkitMonitoringRoles( - this.projectId, - this.projectNumber, - allChangesets.flatMap((changes) => changes.endpointsToCreate), - ); - const secretAccessPromises = Object.values(plan).flatMap((codebasePlan) => Object.entries(codebasePlan.secretAccessPlan || {}).map(([secret, serviceAccounts]) => this.executor.run( @@ -274,6 +260,12 @@ export class Fabricator { ); await Promise.all(secretAccessPromises); + // Accumulate all regional changesets across all codebases + const allChangesets: planner.Changeset[] = []; + for (const codebasePlan of Object.values(plan)) { + allChangesets.push(...Object.values(codebasePlan.regionalChangesets)); + } + // Phase 1: Creates and Updates const createAndUpdatePromises = allChangesets.map((changes) => { const scraperV1 = new SourceTokenScraper(); From 0a69a4f24dd21ff1bdb56753e50966ff0e7bdbc8 Mon Sep 17 00:00:00 2001 From: Izaak Gough Date: Thu, 24 Sep 2026 16:08:02 +0100 Subject: [PATCH 3/3] fix(functions): redeploy functions when the declarative security etag changes The etag can change without the source changing, so every function was skipped as unchanged and kept a stale label, leaving later deploys to redo the IAM checks the mismatch implies. The skip predicate and the source upload check now share one up-to-date test that compares the label as well as the hash. --- CHANGELOG.md | 1 + src/deploy/functions/backend.ts | 19 ++++ src/deploy/functions/deploy.spec.ts | 19 ++++ src/deploy/functions/deploy.ts | 9 +- src/deploy/functions/prepare.ts | 2 +- src/deploy/functions/release/planner.spec.ts | 94 ++++++++++++++++++++ src/deploy/functions/release/planner.ts | 10 +-- 7 files changed, 139 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8e0146c7fc..acb9ef428b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1,2 @@ - [fixed] Fixed the first deploy of a codebase using `requiresRole` failing when it contains a Genkit function, and kept the Genkit monitoring roles on the managed service account across later deploys. (#11123) +- [changed] Functions whose declarative security roles changed are no longer skipped as unchanged, so the first deploy after this release redeploys the functions in a codebase that uses `requiresRole` and contains Genkit functions. diff --git a/src/deploy/functions/backend.ts b/src/deploy/functions/backend.ts index 570bb765ce0..2eb4554a2f0 100644 --- a/src/deploy/functions/backend.ts +++ b/src/deploy/functions/backend.ts @@ -260,6 +260,25 @@ export const DEFAULT_TIMEOUT_SECONDS = 60; export const MIN_CPU_FOR_CONCURRENCY = 1; export const SCHEDULED_FUNCTION_LABEL = Object.freeze({ deployment: "firebase-schedule" }); +/** Label recording the set of declarative security roles an endpoint was deployed with. */ +export const DECLARATIVE_SECURITY_ETAG_LABEL = "firebase-declarative-security-etag"; + +/** + * Whether the deployed copy of an endpoint already matches what we want to deploy. + * The hash covers source, environment variables and secrets. The declarative security etag can + * change without any of those changing, so it is compared separately. + */ +export function endpointUpToDate(want: Endpoint, have: Endpoint): boolean { + return !!( + have.state === "ACTIVE" && + have.hash && + want.hash && + want.hash === have.hash && + want.labels?.[DECLARATIVE_SECURITY_ETAG_LABEL] === + have.labels?.[DECLARATIVE_SECURITY_ETAG_LABEL] + ); +} + /** * IDs used to identify a regional resource. * This type exists so we can have lightweight references from a Pub/Sub topic diff --git a/src/deploy/functions/deploy.spec.ts b/src/deploy/functions/deploy.spec.ts index e47852e8156..2f643997211 100644 --- a/src/deploy/functions/deploy.spec.ts +++ b/src/deploy/functions/deploy.spec.ts @@ -81,6 +81,25 @@ describe("deploy", () => { expect(result).to.be.true; }); + it("should not skip if the declarative security etag changed", () => { + endpoint1InWantBackend.hash = "1"; + endpoint2InWantBackend.hash = "2"; + endpoint1InHaveBackend.hash = endpoint1InWantBackend.hash; + endpoint2InHaveBackend.hash = endpoint2InWantBackend.hash; + endpoint1InWantBackend.labels = { + [backend.DECLARATIVE_SECURITY_ETAG_LABEL]: "new-etag", + }; + endpoint1InHaveBackend.labels = { + [backend.DECLARATIVE_SECURITY_ETAG_LABEL]: "old-etag", + }; + + // Execute + const result = deploy.shouldUploadBeSkipped(CONTEXT, wantBackend, haveBackend); + + // Expect + expect(result).to.be.false; + }); + it("should not skip if hashes don't match", () => { endpoint1InWantBackend.hash = "1"; endpoint2InWantBackend.hash = "2"; diff --git a/src/deploy/functions/deploy.ts b/src/deploy/functions/deploy.ts index 9048f23530f..9bd95dc111e 100644 --- a/src/deploy/functions/deploy.ts +++ b/src/deploy/functions/deploy.ts @@ -227,11 +227,8 @@ export function shouldUploadBeSkipped( if (!haveEndpoint) { return false; } - return ( - haveEndpoint.hash && - wantEndpoint.hash && - haveEndpoint.hash === wantEndpoint.hash && - haveEndpoint.state === "ACTIVE" - ); + // Must agree with the planner's skip predicate: an endpoint the planner updates needs its + // source uploaded, or the update fails without storage. + return backend.endpointUpToDate(wantEndpoint, haveEndpoint); }); } diff --git a/src/deploy/functions/prepare.ts b/src/deploy/functions/prepare.ts index e68d62c5905..133b617e595 100644 --- a/src/deploy/functions/prepare.ts +++ b/src/deploy/functions/prepare.ts @@ -73,7 +73,7 @@ import * as iam from "../../gcp/iam"; import * as resourcemanager from "../../gcp/resourceManager"; export const EVENTARC_SOURCE_ENV = "EVENTARC_CLOUD_EVENT_SOURCE"; -export const DECLARATIVE_SECURITY_ETAG_LABEL = "firebase-declarative-security-etag"; +export const DECLARATIVE_SECURITY_ETAG_LABEL = backend.DECLARATIVE_SECURITY_ETAG_LABEL; /** * Discovers and coordinates declarative security details for a codebase. diff --git a/src/deploy/functions/release/planner.spec.ts b/src/deploy/functions/release/planner.spec.ts index 09df0cb5d1a..311bf038158 100644 --- a/src/deploy/functions/release/planner.spec.ts +++ b/src/deploy/functions/release/planner.spec.ts @@ -6,6 +6,7 @@ import * as planner from "./planner"; import * as deploymentTool from "../../../deploymentTool"; import * as utils from "../../../utils"; import * as v2events from "../../../functions/events/v2"; +import { GENKIT_MONITORING_ROLES } from "../checkIam"; describe("planner", () => { let logLabeledBullet: sinon.SinonStub; @@ -227,6 +228,59 @@ describe("planner", () => { expect(result.region.endpointsToUpdate).to.have.lengthOf(1); expect(result.region.endpointsToUpdate[0].endpoint.id).to.equal("func"); }); + + it("should skip functions carrying the declarative security etag they were deployed with", () => { + const funcWant = func("func", "region"); + const funcHave = func("func", "region"); + funcWant.hash = "same-hash"; + funcHave.hash = "same-hash"; + funcWant.labels = { [backend.DECLARATIVE_SECURITY_ETAG_LABEL]: "etag" }; + funcHave.labels = { [backend.DECLARATIVE_SECURITY_ETAG_LABEL]: "etag" }; + + const result = planner.calculateChangesets( + { func: funcWant }, + { func: funcHave }, + (e) => e.region, + ); + + expect(result.region.endpointsToSkip).to.have.lengthOf(1); + }); + + it("should not skip functions whose declarative security etag changed", () => { + const funcWant = func("func", "region"); + const funcHave = func("func", "region"); + funcWant.hash = "same-hash"; + funcHave.hash = "same-hash"; + funcWant.labels = { [backend.DECLARATIVE_SECURITY_ETAG_LABEL]: "new-etag" }; + funcHave.labels = { [backend.DECLARATIVE_SECURITY_ETAG_LABEL]: "old-etag" }; + + const result = planner.calculateChangesets( + { func: funcWant }, + { func: funcHave }, + (e) => e.region, + ); + + expect(result.region.endpointsToSkip).to.have.lengthOf(0); + expect(result.region.endpointsToUpdate).to.have.lengthOf(1); + expect(result.region.endpointsToUpdate[0].endpoint.id).to.equal("func"); + }); + + it("should not skip functions that are dropping the declarative security etag", () => { + const funcWant = func("func", "region"); + const funcHave = func("func", "region"); + funcWant.hash = "same-hash"; + funcHave.hash = "same-hash"; + funcHave.labels = { [backend.DECLARATIVE_SECURITY_ETAG_LABEL]: "old-etag" }; + + const result = planner.calculateChangesets( + { func: funcWant }, + { func: funcHave }, + (e) => e.region, + ); + + expect(result.region.endpointsToSkip).to.have.lengthOf(0); + expect(result.region.endpointsToUpdate).to.have.lengthOf(1); + }); }); describe("calculateChangesets", () => { @@ -463,6 +517,46 @@ describe("planner", () => { }); }); + it("does not revoke the Genkit monitoring roles on a later deploy", async () => { + const managedSA = "firebase-fn-123@my-project.iam.gserviceaccount.com"; + const genkitFn = func("genkit", "region", { callableTrigger: { genkitAction: "flow" } }); + const wantBackend = backend.of(genkitFn); + wantBackend.requiredRoles = ["roles/viewer", ...GENKIT_MONITORING_ROLES]; + + const plan = await planner.createDeploymentPlan({ + wantBackend, + haveBackend: backend.of(genkitFn), + codebase, + projectId: "my-project", + haveRoles: ["roles/viewer", ...GENKIT_MONITORING_ROLES], + existingManagedSA: managedSA, + managedSA, + }); + + expect(plan.rolesToRemove).to.deep.equal([]); + expect(plan.rolesToAdd).to.deep.equal([]); + }); + + it("adds the Genkit monitoring roles a managed service account is missing", async () => { + const managedSA = "firebase-fn-123@my-project.iam.gserviceaccount.com"; + const genkitFn = func("genkit", "region", { callableTrigger: { genkitAction: "flow" } }); + const wantBackend = backend.of(genkitFn); + wantBackend.requiredRoles = ["roles/viewer", ...GENKIT_MONITORING_ROLES]; + + const plan = await planner.createDeploymentPlan({ + wantBackend, + haveBackend: backend.of(genkitFn), + codebase, + projectId: "my-project", + haveRoles: ["roles/viewer"], + existingManagedSA: managedSA, + managedSA, + }); + + expect(plan.rolesToAdd).to.deep.equal([...GENKIT_MONITORING_ROLES]); + expect(plan.rolesToRemove).to.deep.equal([]); + }); + it("applies filters", async () => { const group1Created = func("g1-created", "region"); const group1Updated = func("g1-updated", "region"); diff --git a/src/deploy/functions/release/planner.ts b/src/deploy/functions/release/planner.ts index 19de0d06d03..0037ece197f 100644 --- a/src/deploy/functions/release/planner.ts +++ b/src/deploy/functions/release/planner.ts @@ -84,15 +84,9 @@ export function calculateChangesets( keyFn, ); - // If the hashes are matching, that means the local function is the same as the server copy. + // A function whose deployed copy already matches is left alone, unless --only named it. const toSkipPredicate = (id: string): boolean => - !!( - !want[id].targetedByOnly && // Don't skip the function if its --only targeted. - have[id].state === "ACTIVE" && // Only skip the function if its in a known good state - have[id].hash && - want[id].hash && - want[id].hash === have[id].hash - ); + !want[id].targetedByOnly && backend.endpointUpToDate(want[id], have[id]); const toSkipEndpointsMap = Object.keys(want) .filter((id) => have[id])