Adds required IAM bindings for EventArc events #4324
Conversation
src/deploy/functions/checkIam.ts
Outdated
| allRequiredBindings.push(obtainPubSubServiceAgentBindings(projectNumber, policy)); | ||
| allRequiredBindings.push(obtainDefaultComputeServiceAgentBindings(projectNumber, policy)); | ||
| allRequiredBindings.push(obtainEventarcServiceAgentBindings(projectNumber, policy)); |
There was a problem hiding this comment.
IIUC, we try to setup these bindings on every deployment, regardless of whether the deployment requires it. Is that true? I'd like to avoid it, especially since I suspect CF3v1 customers don't even have eventarc enabled.
There was a problem hiding this comment.
Resolved over chat, we don't set these since we map to v2 services and v1 is ignored
| @@ -43,7 +47,7 @@ export const StorageService: Service = { | |||
| export const FirebaseAlertsService: Service = { | |||
| name: "firebasealerts", | |||
| api: "logging.googleapis.com", | |||
| requiredProjectBindings: obtainFirebaseAlertsBindings, | |||
| requiredProjectBindings: noopProjectBindings, | |||
| }); | ||
| }); | ||
|
|
||
| it("should add the service agent as a member", () => { |
There was a problem hiding this comment.
nit* "it should append service account to existing policy" or something like that to describe help (me!) differentiate the test case a little more.
There was a problem hiding this comment.
Updated these set of tests
| expect(bindings.length).to.equal(1); | ||
| expect(bindings[0]).to.deep.equal({ | ||
| role: checkIam.SERVICE_ACCOUNT_TOKEN_CREATOR_ROLE, | ||
| members: ["serviceAccount:service-123456789@gcp-sa-pubsub.iam.gserviceaccount.com"], |
There was a problem hiding this comment.
nit: serviceAccount:service-${projectNumber}@gcp-sa-pubsub.iam.gserviceaccount.com
There was a problem hiding this comment.
woops, fixed these set of strings
| expect(bindings[0]).to.deep.equal({ | ||
| role: checkIam.RUN_INVOKER_ROLE, | ||
| members: [`serviceAccount:${projectNumber}-compute@developer.gserviceaccount.com`], | ||
| }); | ||
| expect(bindings[1]).to.deep.equal({ | ||
| role: checkIam.EVENTARC_EVENT_RECEIVER_ROLE, | ||
| members: [`serviceAccount:${projectNumber}-compute@developer.gserviceaccount.com`], | ||
| }); |
There was a problem hiding this comment.
You can use .include.members, e.g.:
to.include.members(expected1, expected2)to remove "ordering" property.
There was a problem hiding this comment.
Oh nice, way cleaner 🔥
|
|
||
| expect(getIamStub).to.have.been.calledOnce; | ||
| expect(setIamStub).to.have.been.calledOnce; | ||
| expect(setIamStub).to.have.been.calledWith(projectNumber, newIamPolicy, "bindings"); |
There was a problem hiding this comment.
aside - why does setIam gets called if no new binding is introduced?
There was a problem hiding this comment.
ahh good catch, added a check to return early if we don't have new bindings to add
| expect(setIamStub).to.have.been.calledWith(projectNumber, newIamPolicy, "bindings"); | ||
| }); | ||
|
|
||
| it("should add the default bindings for a new v2 alerts function without v2 deployed functions", async () => { |
There was a problem hiding this comment.
hm I seem to be missing something. Can you explain to me why PUBSUB TOKEN CREATOR binding isn't required when setting up firealerts functions?
There was a problem hiding this comment.
You mean the roles/pubsub.publisher role with the storage service account, right? That binding is only required for storage since the storage service account needs to be able to create events.
There was a problem hiding this comment.
Here's the docs -> https://cloud.google.com/eventarc/docs/creating-triggers#preparing
There was a problem hiding this comment.
What a nightmare 🤦🏼♂️ . I'm glad we are making this easier for our users.
* adding required bindings for pubsub & compute service agents * adding tests * adding eventarc service agent bindings * adding noop for services, check for existing v2 functions, and more tests * addressing comments
Adds the required roles to the pubsub, default compute, and eventarc service agents in order to correctly deploy EventArc triggers.