Adds new alerting event provider #4258
Conversation
| projectId: string, | ||
| existingPolicy: iam.Policy | ||
| ): Promise<Array<iam.Binding>> { | ||
| const projectNumber = await getProjectNumber({ projectId }); |
There was a problem hiding this comment.
I'd like for us to avoid making extra API call if possible; it adds small amount of delay in our deploy (though I agree that it isn't that much)
It looks like we are the only caller of ensureServiceAgentRoles which ultimately makes call to this function. Can we pass around projectNumber in prepare.ts so subsequent calls to getProjectNumber is properly cached instead of making one-off calls here?
There was a problem hiding this comment.
Great point, I added this fetch call in prepare and I passed both projectId & projectNumber into requireProjectBindings in the latest commit. I combined them into an object, let me know if you prefer them to be separate params
src/deploy/functions/checkIam.ts
Outdated
| @@ -148,7 +148,7 @@ export function mergeBindings(policy: iam.Policy, allRequiredBindings: iam.Bindi | |||
| * @param have backend that we have currently deployed | |||
| */ | |||
| export async function ensureServiceAgentRoles( | |||
| projectId: string, | |||
| project: { projectId: string; projectNumber: string }, | |||
There was a problem hiding this comment.
nit* In theory, having just projectNumber should be sufficient since all GCP API calls accept either projectId or projectNumber. Will leave it up to you to decide whether that would be appropriate.
There was a problem hiding this comment.
Oh snap, I didn't realize treated them as interchangeable!!
| requiredProjectBindings: ((pId: any, p: any) => Promise<Array<iam.Binding>>) | undefined; | ||
| ensureTriggerRegion: (ep: backend.Endpoint & backend.EventTriggered) => Promise<void>; | ||
| requiredProjectBindings: | ||
| | ((project: Project, policy: iam.Policy) => Promise<Array<iam.Binding>> | Array<iam.Binding>) |
There was a problem hiding this comment.
Since this type got really complicated, what if we instead keep the original type signature and instead have obtainFirebaseAlertsBindings return a promise instead? e.g.
export function obtainFirebaseAlertsBindings(
project: { projectId: string; projectNumber: string },
existingPolicy: iam.Policy
): Promise<Array<iam.Binding>> {
// ...implementation
return Promise.resolve([pubsubBinding]);
}There was a problem hiding this comment.
fixed in the latest commit
| requiredProjectBindings: | ||
| | ((project: Project, policy: iam.Policy) => Promise<Array<iam.Binding>> | Array<iam.Binding>) | ||
| | undefined; | ||
| ensureTriggerRegion: (ep: backend.Endpoint & backend.EventTriggered) => Promise<void> | void; |
There was a problem hiding this comment.
Same trick as I mentioned above - what if we "clean" this type up by requiring everything to return a promise - it is easy to make non-promise returning fns to return a promise instead.
There was a problem hiding this comment.
fixed in the latest commit
| * @param projectId project identifier | ||
| * @param existingPolicy the project level IAM policy | ||
| */ | ||
| export function obtainFirebaseAlertsBindings( |
There was a problem hiding this comment.
Is this really a Firebase Alerts feature or something that all event providers are going to need?
There was a problem hiding this comment.
NVM. See below comments. I think we could have had something like:
export function obtainFirebaseAlertBindings(projectNumber) {
iam.ensureBinding(firebaseAlertRole, firebaseAlertPermission)
}
There was a problem hiding this comment.
You're correct, this is not a Firebase Alerts specific binding. It was yanked in #4324 and I added a noop instead
| const pubsubServiceAgent = `serviceAccount:service-${projectNumber}@gcp-sa-pubsub.iam.gserviceaccount.com`; | ||
| let pubsubBinding = existingPolicy.bindings.find( | ||
| (b) => b.role === SERVICE_ACCOUNT_TOKEN_CREATOR_ROLE | ||
| ); | ||
| if (!pubsubBinding) { | ||
| pubsubBinding = { | ||
| role: SERVICE_ACCOUNT_TOKEN_CREATOR_ROLE, | ||
| members: [], | ||
| }; | ||
| } | ||
| if (!pubsubBinding.members.find((m) => m === pubsubServiceAgent)) { | ||
| pubsubBinding.members.push(pubsubServiceAgent); | ||
| } | ||
| return Promise.resolve([pubsubBinding]); | ||
| } |
There was a problem hiding this comment.
This logic happens over and over and over again in our codebase. Maybe we should have something in gcp/iam.ts that ensures a binding exists or is removed?
There was a problem hiding this comment.
SGTM, I believe that @taeold was thinking of refactoring our IAM code to look more like the Fabricator code
| /** A firebase alerts service object */ | ||
| export const FirebaseAlertsService: Service = { | ||
| name: "firebasealerts", | ||
| api: "logging.googleapis.com", |
There was a problem hiding this comment.
This is really the API for Firebase Alerts?? How is this not a conflict with Cloud Audit Logs?
There was a problem hiding this comment.
This was just a placeholder, I'll update it with the correct API. Additionally, this field is not currently being used in the CLI
Adds Firebase Alerts as a deployable event provider.
note: we cannot deploy until the container contract changes are merged