Update function deploy to add necessary Genkit monitoring permissions when deploying Genkit function #8636

bryanatkinson · 2025-05-21T20:22:17Z

Description

Genkit Monitoring requires that the service account running the Genkit code has permission to write metrics, traces and logs. This change adds those permissions to the default service account when deploying as a Firebase Function.

Scenarios Tested

Tested with newly added Genkit function and ensure the permissions are set on the first firebase deploy
Added unit tests

inlined

We only do IAM stuff on new function creation because it requires an OWNER permission rather than an editor permission and we want large teams to be able to have finer grained ACLs
I'm concerned that this is granting the binding to the default compute service account, rather than the accounts that the actual functions run as (which defaults to the compute SA but is not guaranteed to be so).

For 2, it's not clear that this is technically correct. @taeold do you know why obtainDefaultComputeServiceAgentBindings is here? Is this part of the bug/feature request that we don't allow configuring Eventarc to use custom service accounts yet?

I would consider a new dedicated function that

Finds all new endpoints that are Genkit callable endpoints
Finds the set of service accounts they run as (defaulting to the default compute SA)
Adds those role bindings for the SA to the project.

bryanatkinson · 2025-05-30T17:36:48Z

We only do IAM stuff on new function creation because it requires an OWNER permission rather than an editor permission and we want large teams to be able to have finer grained ACLs

I'm concerned that this is granting the binding to the default compute service account, rather than the accounts that the actual functions run as (which defaults to the compute SA but is not guaranteed to be so).

For 2, it's not clear that this is technically correct. @taeold do you know why obtainDefaultComputeServiceAgentBindings is here? Is this part of the bug/feature request that we don't allow configuring Eventarc to use custom service accounts yet?

I would consider a new dedicated function that

Finds all new endpoints that are Genkit callable endpoints

Finds the set of service accounts they run as (defaulting to the default compute SA)

Adds those role bindings for the SA to the project.

I have updated to move this into a separate function called in the prepare step, and to find all service accounts for new Genkit endpoints only. Let me know if this doesn't match what you were thinking.

inlined · 2025-05-30T21:59:56Z

src/deploy/functions/checkIam.spec.ts

+      expect(getIamStub).to.have.been.calledWith(projectNumber);
+      expect(setIamStub).to.have.been.calledOnce;
+    });
+    it("should not update policy if it already has necessary bindings", async () => {


nit: missing newline

inlined · 2025-05-30T22:03:46Z

src/deploy/functions/checkIam.ts

+  const wantEndpoints = backend.allEndpoints(want).filter(isGenkitEndpoint);
+  const haveEndpoints = backend.allEndpoints(have).filter(isGenkitEndpoint);
+  const newEndpoints = wantEndpoints.filter(
+    (wantE) => !haveEndpoints.find((haveE) => haveE.id === wantE.id),


Technically you should compare the region too. There's a helper here:

wantEndopints.filter(backend.missingEndpoint(haveEndpoints))

Nice! Updated.

inlined · 2025-05-30T22:06:54Z

src/deploy/functions/checkIam.ts

+    return;
+  }
+
+  const defaultComputeServiceAgent = await gce.getDefaultServiceAccount(projectNumber);


It'd be ideal to only calculate this if necessary. I'm not sure what this code would do on a project that has disabled the default SA.

Updated to only make this call if the default is needed.

inlined · 2025-05-30T22:11:02Z

src/deploy/functions/checkIam.ts

@@ -215,8 +288,8 @@ export async function ensureServiceAgentRoles(
    iam.printManualIamConfig(requiredBindings, projectId, "functions");
    utils.logLabeledBullet(
      "functions",
-      "Could not verify the necessary IAM configuration for the following newly-integrated services: " +
-        `${newServices.map((service) => service.api).join(", ")}` +
+      "Could not verify the necessary IAM configuration for the following newly-integrated services or endpoints: " +


endpoints is an internal term because we were experimenting with code that would eventually become App Hosting. It is meaningless to users.

Reverted this back to just say "services"

…d use convenience method to filter down to new endpoints

bryanatkinson requested review from andrewbrook and ifielker May 21, 2025 20:22

github-project-automation bot added this to [Cloud] Extensions + Functions May 21, 2025

ifielker requested a review from inlined May 21, 2025 20:23

andrewbrook approved these changes May 21, 2025

View reviewed changes

github-project-automation bot moved this to Approved [PR] in [Cloud] Extensions + Functions May 21, 2025

bryanatkinson force-pushed the ba-genkit-function-permissions branch 2 times, most recently from cd747d4 to df10513 Compare May 28, 2025 18:14

inlined requested changes May 28, 2025

View reviewed changes

github-project-automation bot moved this from Approved [PR] to Changes Requested [PR] in [Cloud] Extensions + Functions May 28, 2025

inlined approved these changes May 30, 2025

View reviewed changes

github-project-automation bot moved this from Changes Requested [PR] to Approved [PR] in [Cloud] Extensions + Functions May 30, 2025

bryanatkinson force-pushed the ba-genkit-function-permissions branch from 27729cb to ebe05b7 Compare June 3, 2025 15:18

bryanatkinson added 9 commits June 3, 2025 14:06

Add necessary Genkit monitoring roles during functions deploy.

8696cb9

Add tests

e7f10a4

Remove it.only from test

1d10f4b

Fix formatting

4da944e

Fix formatting

6894645

Fix formatting

0f514dc

Move genkit permissions to a separate function in checkIam

6b9ecfc

Add missing return type

f5c053c

Only determine default service account if none has been specified, an…

5f567a9

…d use convenience method to filter down to new endpoints

bryanatkinson force-pushed the ba-genkit-function-permissions branch from ebe05b7 to 5f567a9 Compare June 3, 2025 18:07

bryanatkinson merged commit b103ff1 into master Jun 3, 2025
48 of 50 checks passed

bryanatkinson deleted the ba-genkit-function-permissions branch June 3, 2025 18:39

github-project-automation bot moved this from Approved [PR] to Done in [Cloud] Extensions + Functions Jun 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update function deploy to add necessary Genkit monitoring permissions when deploying Genkit function #8636

Update function deploy to add necessary Genkit monitoring permissions when deploying Genkit function #8636

Uh oh!

bryanatkinson commented May 21, 2025

Uh oh!

inlined left a comment

Uh oh!

bryanatkinson commented May 30, 2025

Uh oh!

inlined May 30, 2025

Uh oh!

bryanatkinson Jun 3, 2025

Uh oh!

inlined May 30, 2025

Uh oh!

bryanatkinson Jun 3, 2025

Uh oh!

inlined May 30, 2025

Uh oh!

bryanatkinson Jun 3, 2025

Uh oh!

inlined May 30, 2025

Uh oh!

bryanatkinson Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

Update function deploy to add necessary Genkit monitoring permissions when deploying Genkit function #8636

Update function deploy to add necessary Genkit monitoring permissions when deploying Genkit function #8636

Uh oh!

Conversation

bryanatkinson commented May 21, 2025

Description

Scenarios Tested

Uh oh!

inlined left a comment

Choose a reason for hiding this comment

Uh oh!

bryanatkinson commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!