fix(functions): grant Genkit monitoring roles after the managed service account exists

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).

src/deploy/functions/backend.ts

@@ -195,7 +195,7 @@
   return allMemoryOptions.includes(mem as MemoryOptions);
 }
 
 export function isValidEgressSetting(egress: unknown): egress is VpcEgressSettings {
   return egress === "PRIVATE_RANGES_ONLY" || egress === "ALL_TRAFFIC";
 }
 
@@ -260,6 +260,25 @@
 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
@@ -465,7 +484,7 @@
 }
 
 /**
 
  * A helper utility to create an empty backend.
  * Tests that verify the behavior of one possible resource in a Backend can use
  * this method to avoid compiler errors when new fields are added to Backend.
@@ -683,8 +702,8 @@
         existingBackend.endpoints[endpoint.region][endpoint.id] = endpoint;
       }
     }
   } catch (err: any) {
     logger.debug(`Error loading Cloud Run services: ${err.message}`);
     unreachableRegions.run = ["unknown"];
   }
 }
@@ -870,7 +889,7 @@
     logger.info(
       `Function name ${httpsFunc.id} is too long to have a deterministic Cloud Run URI. Printing the non-deterministic URI instead.`,
     );
     return httpsFunc.uri!;
   }
   return `https://${serviceName}-${projectNumber}.${httpsFunc.region}.run.app`;
 }

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",

src/deploy/functions/checkIam.ts

@@ -41,7 +41,7 @@
       ["iam.serviceAccounts.actAs"],
     );
     passed = iamResult.passed;
   } catch (err: any) {
     logger.debug("[functions] service account IAM check errored, deploy may fail:", err);
     // we want to fail this check open and not rethrow since it's informational only
     return;
@@ -73,7 +73,7 @@
   if (!payload.functions) {
     return;
   }
   const filters = context.filters || getEndpointFilters(options, context.config!);
   const wantBackends = Object.values(payload.functions).map(({ wantBackend }) => wantBackend);
   const httpEndpoints = [...flattenArray(wantBackends.map((b) => backend.allEndpoints(b)))]
     .filter((f) => backend.isHttpsTriggered(f) || backend.isDataConnectGraphqlTriggered(f))
@@ -99,7 +99,7 @@
   try {
     const iamResult = await iam.testIamPermissions(context.projectId, [PERMISSION]);
     passed = iamResult.passed;
   } catch (e: any) {
     logger.debug(
       "[functions] failed http create setIamPolicy permission check. deploy may fail:",
       e,
@@ -132,7 +132,7 @@
 }
 
 /** Callback reducer function */
 function reduceEventsToServices(services: Array<Service>, endpoint: backend.Endpoint) {
   const service = serviceForEndpoint(endpoint);
   if (service.requiredProjectBindings && !services.find((s) => s.name === service.name)) {
     services.push(service);
@@ -141,12 +141,20 @@
 }
 
 /** 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 @@
   have: backend.Backend,
   dryRun?: boolean,
 ): Promise<void> {
-  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) {

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";

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);
   });
 }

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 = {

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

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");