Skip to content

Commit 9a9f1db

Browse files
IzaakGoughIzaakGough
committed
fix: preserve existing cloud run invokers for scheduled v2 functions
1 parent e28281f commit 9a9f1db

4 files changed

Lines changed: 97 additions & 16 deletions

File tree

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

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -820,32 +820,37 @@ describe("Fabricator", () => {
820820
});
821821

822822
describe("scheduledTrigger", () => {
823-
it("sets the default compute service account by default", async () => {
823+
it("sets the default compute service account by default and preserves existing invokers", async () => {
824824
gcfv2.createFunction.resolves({ name: "op", done: false });
825825
poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
826-
run.setInvokerCreate.resolves();
826+
run.setInvokerUpdate.resolves();
827827
const ep = endpoint(
828828
{ scheduleTrigger: { schedule: "every 5 minutes" } },
829829
{ platform: "gcfv2" },
830830
);
831831

832832
await fab.createV2Function(ep, new scraper.SourceTokenScraper());
833-
expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", [
834-
await gce.getDefaultServiceAccount(fab.projectNumber),
835-
]);
833+
expect(run.setInvokerUpdate).to.have.been.calledWith(
834+
ep.project,
835+
"service",
836+
[await gce.getDefaultServiceAccount(fab.projectNumber)],
837+
{ mergeExistingMembers: true },
838+
);
836839
});
837840

838841
it("sets the an explicit service account", async () => {
839842
gcfv2.createFunction.resolves({ name: "op", done: false });
840843
poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
841-
run.setInvokerCreate.resolves();
844+
run.setInvokerUpdate.resolves();
842845
const ep = endpoint(
843846
{ scheduleTrigger: { schedule: "every 5 minutes" } },
844847
{ platform: "gcfv2", serviceAccount: "sa@" },
845848
);
846849

847850
await fab.createV2Function(ep, new scraper.SourceTokenScraper());
848-
expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", ["sa@"]);
851+
expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["sa@"], {
852+
mergeExistingMembers: true,
853+
});
849854
});
850855
});
851856

@@ -964,6 +969,21 @@ describe("Fabricator", () => {
964969
expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["public"]);
965970
});
966971

972+
it("sets scheduler invoker and preserves existing invokers on scheduled functions", async () => {
973+
gcfv2.updateFunction.resolves({ name: "op", done: false });
974+
poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
975+
run.setInvokerUpdate.resolves();
976+
const ep = endpoint(
977+
{ scheduleTrigger: { schedule: "every 5 minutes" } },
978+
{ platform: "gcfv2", serviceAccount: "sa@" },
979+
);
980+
981+
await fab.updateV2Function(ep, new scraper.SourceTokenScraper());
982+
expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["sa@"], {
983+
mergeExistingMembers: true,
984+
});
985+
});
986+
967987
it("does not set invoker by default", async () => {
968988
gcfv2.updateFunction.resolves({ name: "op", done: false });
969989
poller.pollOperation.resolves({ serviceConfig: { service: "service" } });

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -504,7 +504,11 @@ export class Fabricator {
504504
? [endpoint.serviceAccount]
505505
: [await gce.getDefaultServiceAccount(this.projectNumber)];
506506
await this.executor
507-
.run(() => run.setInvokerCreate(endpoint.project, serviceName, invoker))
507+
.run(() =>
508+
run.setInvokerUpdate(endpoint.project, serviceName, invoker, {
509+
mergeExistingMembers: true,
510+
}),
511+
)
508512
.catch(rethrowAs(endpoint, "set invoker"));
509513
}
510514
}
@@ -623,8 +627,11 @@ export class Fabricator {
623627
}
624628

625629
if (invoker) {
630+
const invokerOptions = backend.isScheduleTriggered(endpoint)
631+
? { mergeExistingMembers: true }
632+
: undefined;
626633
await this.executor
627-
.run(() => run.setInvokerUpdate(endpoint.project, serviceName, invoker!))
634+
.run(() => run.setInvokerUpdate(endpoint.project, serviceName, invoker!, invokerOptions))
628635
.catch(rethrowAs(endpoint, "set invoker"));
629636
}
630637
}

‎src/gcp/run.spec.ts‎

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ describe("run", () => {
136136
apiGetStub.onFirstCall().throws("Error calling get api.");
137137

138138
await expect(
139-
run.setInvokerUpdate("project", "service", ["public"], client),
139+
run.setInvokerUpdate("project", "service", ["public"], undefined, client),
140140
).to.be.rejectedWith("Failed to get the IAM Policy on the Service service");
141141

142142
expect(apiGetStub).to.be.called;
@@ -147,7 +147,7 @@ describe("run", () => {
147147
apiPostStub.throws("Error calling set api.");
148148

149149
await expect(
150-
run.setInvokerUpdate("project", "service", ["public"], client),
150+
run.setInvokerUpdate("project", "service", ["public"], undefined, client),
151151
).to.be.rejectedWith("Failed to set the IAM Policy on the Service service");
152152
expect(apiGetStub).to.be.calledOnce;
153153
expect(apiPostStub).to.be.calledOnce;
@@ -170,8 +170,8 @@ describe("run", () => {
170170
return Promise.resolve();
171171
});
172172

173-
await expect(run.setInvokerUpdate("project", "service", ["public"], client)).to.not.be
174-
.rejected;
173+
await expect(run.setInvokerUpdate("project", "service", ["public"], undefined, client)).to
174+
.not.be.rejected;
175175
expect(apiGetStub).to.be.calledOnce;
176176
expect(apiPostStub).to.be.calledOnce;
177177
});
@@ -200,8 +200,8 @@ describe("run", () => {
200200
return Promise.resolve();
201201
});
202202

203-
await expect(run.setInvokerUpdate("project", "service", ["private"], client)).to.not.be
204-
.rejected;
203+
await expect(run.setInvokerUpdate("project", "service", ["private"], undefined, client)).to
204+
.not.be.rejected;
205205
expect(apiGetStub).to.be.calledOnce;
206206
expect(apiPostStub).to.be.calledOnce;
207207
});
@@ -228,6 +228,51 @@ describe("run", () => {
228228
"service-account2@project.iam.gserviceaccount.com",
229229
"service-account3@",
230230
],
231+
undefined,
232+
client,
233+
),
234+
).to.not.be.rejected;
235+
expect(apiGetStub).to.be.calledOnce;
236+
expect(apiPostStub).to.be.calledOnce;
237+
});
238+
239+
it("should merge existing invoker members when requested", async () => {
240+
apiGetStub.onFirstCall().resolves({
241+
body: {
242+
bindings: [
243+
{
244+
role: "roles/run.invoker",
245+
members: ["serviceAccount:custom@project.iam.gserviceaccount.com"],
246+
},
247+
{ role: "random-role", members: ["user:pineapple"] },
248+
],
249+
etag: "1234",
250+
version: 3,
251+
},
252+
});
253+
apiPostStub.onFirstCall().callsFake((path: string, json: any) => {
254+
const invokerBinding = json.policy.bindings.find(
255+
(binding: any) => binding.role === "roles/run.invoker",
256+
);
257+
expect(invokerBinding.members.sort()).to.deep.eq([
258+
"serviceAccount:custom@project.iam.gserviceaccount.com",
259+
"serviceAccount:scheduler@project.iam.gserviceaccount.com",
260+
]);
261+
expect(json.policy.bindings).to.deep.include({
262+
role: "random-role",
263+
members: ["user:pineapple"],
264+
});
265+
expect(json.policy.etag).to.equal("1234");
266+
267+
return Promise.resolve();
268+
});
269+
270+
await expect(
271+
run.setInvokerUpdate(
272+
"project",
273+
"service",
274+
["scheduler@"],
275+
{ mergeExistingMembers: true },
231276
client,
232277
),
233278
).to.not.be.rejected;
@@ -262,6 +307,7 @@ describe("run", () => {
262307
"service-account3@",
263308
"service-account1@",
264309
],
310+
undefined,
265311
client,
266312
),
267313
).to.not.be.rejected;

‎src/gcp/run.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,10 @@ export async function setInvokerCreate(
303303
await setIamPolicy(serviceName, policy, httpClient);
304304
}
305305

306+
export interface InvokerUpdateOptions {
307+
mergeExistingMembers?: boolean;
308+
}
309+
306310
/**
307311
* Gets the current IAM policy for the run service and overrides the invoker role with the supplied invoker members
308312
* @param projectId id of the project
@@ -314,17 +318,21 @@ export async function setInvokerUpdate(
314318
projectId: string,
315319
serviceName: string,
316320
invoker: string[],
321+
options: InvokerUpdateOptions = {},
317322
httpClient: Client = client, // for unit testing
318323
) {
319324
if (invoker.length === 0) {
320325
throw new FirebaseError("Invoker cannot be an empty array");
321326
}
322-
const invokerMembers = proto.getInvokerMembers(invoker, projectId);
327+
const desiredInvokerMembers = proto.getInvokerMembers(invoker, projectId);
323328
const invokerRole = "roles/run.invoker";
324329
const currentPolicy = await getIamPolicy(serviceName, httpClient);
325330
const currentInvokerBinding = currentPolicy.bindings?.find(
326331
(binding) => binding.role === invokerRole,
327332
);
333+
const invokerMembers = options.mergeExistingMembers
334+
? Array.from(new Set([...(currentInvokerBinding?.members || []), ...desiredInvokerMembers]))
335+
: desiredInvokerMembers;
328336
if (
329337
currentInvokerBinding &&
330338
JSON.stringify(currentInvokerBinding.members.sort()) === JSON.stringify(invokerMembers.sort())

0 commit comments

Comments
 (0)
⚡