fix: preserve existing cloud run invokers for scheduled v2 functions

src/deploy/functions/release/fabricator.spec.ts

@@ -447,7 +447,7 @@
     it("handles topics that already exist", async () => {
       pubsub.createTopic.callsFake(() => {
         const err = new Error("Already exists");
         (err as any).status = 409;
         return Promise.reject(err);
       });
       gcfv2.createFunction.resolves({ name: "op", done: false });
@@ -522,7 +522,7 @@
       eventarc.createChannel.callsFake(({ name }) => {
         expect(name).to.equal("channel");
         const err = new Error("Already exists");
         (err as any).status = 409;
         return Promise.reject(err);
       });
       gcfv2.createFunction.resolves({ name: "op", done: false });
@@ -588,7 +588,7 @@
       eventarc.getChannel.resolves(undefined);
       eventarc.createChannel.callsFake(() => {
         const err = new Error("🤷‍♂️");
         (err as any).status = 400;
         return Promise.reject(err);
       });
 
@@ -820,32 +820,37 @@
     });
 
     describe("scheduledTrigger", () => {
-      it("sets the default compute service account by default", async () => {
+      it("sets the default compute service account by default and preserves existing invokers", async () => {
         gcfv2.createFunction.resolves({ name: "op", done: false });
         poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
-        run.setInvokerCreate.resolves();
+        run.setInvokerUpdate.resolves();
         const ep = endpoint(
           { scheduleTrigger: { schedule: "every 5 minutes" } },
           { platform: "gcfv2" },
         );
 
         await fab.createV2Function(ep, new scraper.SourceTokenScraper());
-        expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", [
-          await gce.getDefaultServiceAccount(fab.projectNumber),
-        ]);
+        expect(run.setInvokerUpdate).to.have.been.calledWith(
+          ep.project,
+          "service",
+          [await gce.getDefaultServiceAccount(fab.projectNumber)],
+          { mergeExistingMembers: true },
+        );
       });
 
       it("sets the an explicit service account", async () => {
         gcfv2.createFunction.resolves({ name: "op", done: false });
         poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
-        run.setInvokerCreate.resolves();
+        run.setInvokerUpdate.resolves();
         const ep = endpoint(
           { scheduleTrigger: { schedule: "every 5 minutes" } },
           { platform: "gcfv2", serviceAccount: "sa@" },
         );
 
         await fab.createV2Function(ep, new scraper.SourceTokenScraper());
-        expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", ["sa@"]);
+        expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["sa@"], {
+          mergeExistingMembers: true,
+        });
       });
     });
 
@@ -964,6 +969,21 @@
       expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["public"]);
     });
 
+    it("sets scheduler invoker and preserves existing invokers on scheduled functions", async () => {
+      gcfv2.updateFunction.resolves({ name: "op", done: false });
+      poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
+      run.setInvokerUpdate.resolves();
+      const ep = endpoint(
+        { scheduleTrigger: { schedule: "every 5 minutes" } },
+        { platform: "gcfv2", serviceAccount: "sa@" },
+      );
+
+      await fab.updateV2Function(ep, new scraper.SourceTokenScraper());
+      expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["sa@"], {
+        mergeExistingMembers: true,
+      });
+    });
+
     it("does not set invoker by default", async () => {
       gcfv2.updateFunction.resolves({ name: "op", done: false });
       poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
@@ -1755,7 +1775,7 @@
       expect(deleteEndpoint).to.not.have.been.called;
 
       // Resolve the create operation
       resolveCreate!();
 
       await applyPlanPromise;
 
@@ -1766,7 +1786,7 @@
 
   describe("createRunFunction", () => {
     it("creates a Cloud Run service with correct configuration", async () => {
       runv2.createService.resolves({ uri: "https://service", name: "service" } as any);
       run.setInvokerUpdate.resolves();
 
       const ep = endpoint(
@@ -1809,7 +1829,7 @@
 
   describe("updateRunFunction", () => {
     it("updates a Cloud Run service with correct configuration", async () => {
       runv2.updateService.resolves({ uri: "https://service", name: "service" } as any);
       run.setInvokerUpdate.resolves();
 
       const ep = endpoint(

src/deploy/functions/release/fabricator.ts

@@ -504,7 +504,11 @@ export class Fabricator {
         ? [endpoint.serviceAccount]
         : [await gce.getDefaultServiceAccount(this.projectNumber)];
       await this.executor
-        .run(() => run.setInvokerCreate(endpoint.project, serviceName, invoker))
+        .run(() =>
+          run.setInvokerUpdate(endpoint.project, serviceName, invoker, {
+            mergeExistingMembers: true,
+          }),
+        )
         .catch(rethrowAs(endpoint, "set invoker"));
     }
   }
@@ -623,8 +627,11 @@ export class Fabricator {
     }
 
     if (invoker) {
+      const invokerOptions = backend.isScheduleTriggered(endpoint)
+        ? { mergeExistingMembers: true }
+        : undefined;
       await this.executor
-        .run(() => run.setInvokerUpdate(endpoint.project, serviceName, invoker!))
+        .run(() => run.setInvokerUpdate(endpoint.project, serviceName, invoker!, invokerOptions))
         .catch(rethrowAs(endpoint, "set invoker"));
     }
   }

src/gcp/run.spec.ts

@@ -136,7 +136,7 @@ describe("run", () => {
         apiGetStub.onFirstCall().throws("Error calling get api.");
 
         await expect(
-          run.setInvokerUpdate("project", "service", ["public"], client),
+          run.setInvokerUpdate("project", "service", ["public"], undefined, client),
         ).to.be.rejectedWith("Failed to get the IAM Policy on the Service service");
 
         expect(apiGetStub).to.be.called;
@@ -147,7 +147,7 @@ describe("run", () => {
         apiPostStub.throws("Error calling set api.");
 
         await expect(
-          run.setInvokerUpdate("project", "service", ["public"], client),
+          run.setInvokerUpdate("project", "service", ["public"], undefined, client),
         ).to.be.rejectedWith("Failed to set the IAM Policy on the Service service");
         expect(apiGetStub).to.be.calledOnce;
         expect(apiPostStub).to.be.calledOnce;
@@ -170,8 +170,8 @@ describe("run", () => {
           return Promise.resolve();
         });
 
-        await expect(run.setInvokerUpdate("project", "service", ["public"], client)).to.not.be
-          .rejected;
+        await expect(run.setInvokerUpdate("project", "service", ["public"], undefined, client)).to
+          .not.be.rejected;
         expect(apiGetStub).to.be.calledOnce;
         expect(apiPostStub).to.be.calledOnce;
       });
@@ -200,8 +200,8 @@ describe("run", () => {
           return Promise.resolve();
         });
 
-        await expect(run.setInvokerUpdate("project", "service", ["private"], client)).to.not.be
-          .rejected;
+        await expect(run.setInvokerUpdate("project", "service", ["private"], undefined, client)).to
+          .not.be.rejected;
         expect(apiGetStub).to.be.calledOnce;
         expect(apiPostStub).to.be.calledOnce;
       });
@@ -228,6 +228,51 @@ describe("run", () => {
               "service-account2@project.iam.gserviceaccount.com",
               "service-account3@",
             ],
+            undefined,
+            client,
+          ),
+        ).to.not.be.rejected;
+        expect(apiGetStub).to.be.calledOnce;
+        expect(apiPostStub).to.be.calledOnce;
+      });
+
+      it("should merge existing invoker members when requested", async () => {
+        apiGetStub.onFirstCall().resolves({
+          body: {
+            bindings: [
+              {
+                role: "roles/run.invoker",
+                members: ["serviceAccount:custom@project.iam.gserviceaccount.com"],
+              },
+              { role: "random-role", members: ["user:pineapple"] },
+            ],
+            etag: "1234",
+            version: 3,
+          },
+        });
+        apiPostStub.onFirstCall().callsFake((path: string, json: any) => {
+          const invokerBinding = json.policy.bindings.find(
+            (binding: any) => binding.role === "roles/run.invoker",
+          );
+          expect(invokerBinding.members.sort()).to.deep.eq([
+            "serviceAccount:custom@project.iam.gserviceaccount.com",
+            "serviceAccount:scheduler@project.iam.gserviceaccount.com",
+          ]);
+          expect(json.policy.bindings).to.deep.include({
+            role: "random-role",
+            members: ["user:pineapple"],
+          });
+          expect(json.policy.etag).to.equal("1234");
+
+          return Promise.resolve();
+        });
+
+        await expect(
+          run.setInvokerUpdate(
+            "project",
+            "service",
+            ["scheduler@"],
+            { mergeExistingMembers: true },
             client,
           ),
         ).to.not.be.rejected;
@@ -262,6 +307,7 @@ describe("run", () => {
               "service-account3@",
               "service-account1@",
             ],
+            undefined,
             client,
           ),
         ).to.not.be.rejected;

src/gcp/run.ts

@@ -303,6 +303,10 @@ export async function setInvokerCreate(
   await setIamPolicy(serviceName, policy, httpClient);
 }
 
+export interface InvokerUpdateOptions {
+  mergeExistingMembers?: boolean;
+}
+
 /**
  * Gets the current IAM policy for the run service and overrides the invoker role with the supplied invoker members
  * @param projectId id of the project
@@ -314,17 +318,21 @@ export async function setInvokerUpdate(
   projectId: string,
   serviceName: string,
   invoker: string[],
+  options: InvokerUpdateOptions = {},
   httpClient: Client = client, // for unit testing
 ) {
   if (invoker.length === 0) {
     throw new FirebaseError("Invoker cannot be an empty array");
   }
-  const invokerMembers = proto.getInvokerMembers(invoker, projectId);
+  const desiredInvokerMembers = proto.getInvokerMembers(invoker, projectId);
   const invokerRole = "roles/run.invoker";
   const currentPolicy = await getIamPolicy(serviceName, httpClient);
   const currentInvokerBinding = currentPolicy.bindings?.find(
     (binding) => binding.role === invokerRole,
   );
+  const invokerMembers = options.mergeExistingMembers
+    ? Array.from(new Set([...(currentInvokerBinding?.members || []), ...desiredInvokerMembers]))
+    : desiredInvokerMembers;
   if (
     currentInvokerBinding &&
     JSON.stringify(currentInvokerBinding.members.sort()) === JSON.stringify(invokerMembers.sort())