diff --git a/CHANGELOG.md b/CHANGELOG.md index e1caf99408c..3e494305948 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1,3 @@ +- 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) +- Stopped skipping functions whose declarative security roles changed, so the first deploy after this release redeploys the functions in a codebase that uses `requiresRole` and contains Genkit functions. - [Fixed] Report the GCFv2-to-GCFv1 downgrade error during validation instead of a misleading CPU error (#5461). 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/checkIam.spec.ts b/src/deploy/functions/checkIam.spec.ts index b1dadcef3c6..84b34c4d565 100644 --- a/src/deploy/functions/checkIam.spec.ts +++ b/src/deploy/functions/checkIam.spec.ts @@ -285,6 +285,71 @@ describe("checkIam", () => { 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", diff --git a/src/deploy/functions/checkIam.ts b/src/deploy/functions/checkIam.ts index f39031ac079..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. @@ -197,7 +205,12 @@ export async function ensureGenkitMonitoringRoles( have: backend.Backend, dryRun?: boolean, ): Promise { - const wantEndpoints = backend.allEndpoints(want).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) { 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.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 32ab6731fe8..1da81c7fe2b 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 { @@ -68,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. @@ -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 diff --git a/src/deploy/functions/release/planner.spec.ts b/src/deploy/functions/release/planner.spec.ts index 8b44a92f14d..231d8715223 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 95646f11822..e5eadf9b1da 100644 --- a/src/deploy/functions/release/planner.ts +++ b/src/deploy/functions/release/planner.ts @@ -85,15 +85,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])