Adds new alerting event provider #4258

colerogers · 2022-03-07T16:34:49Z

Adds Firebase Alerts as a deployable event provider.

firebase deploy --only functions

note: we cannot deploy until the container contract changes are merged

src/deploy/functions/services/firebaseAlerts.ts

src/deploy/functions/triggerRegionHelper.ts

taeold · 2022-03-07T20:09:44Z

src/deploy/functions/services/firebaseAlerts.ts

+  projectId: string,
+  existingPolicy: iam.Policy
+): Promise<Array<iam.Binding>> {
+  const projectNumber = await getProjectNumber({ projectId });


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?

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

…jectBindings

src/deploy/functions/prepare.ts

taeold · 2022-03-08T04:05:06Z

src/deploy/functions/checkIam.ts

@@ -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 },


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.

Oh snap, I didn't realize treated them as interchangeable!!

taeold · 2022-03-08T04:10:31Z

src/deploy/functions/services/index.ts

-  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>)


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]); }

fixed in the latest commit

taeold · 2022-03-08T04:12:35Z

src/deploy/functions/services/index.ts

+  requiredProjectBindings:
+    | ((project: Project, policy: iam.Policy) => Promise<Array<iam.Binding>> | Array<iam.Binding>)
+    | undefined;
+  ensureTriggerRegion: (ep: backend.Endpoint & backend.EventTriggered) => Promise<void> | void;


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.

fixed in the latest commit

src/deploy/functions/triggerRegionHelper.ts

src/deploy/functions/checkIam.ts

inlined · 2022-03-28T21:21:09Z

src/deploy/functions/services/firebaseAlerts.ts

+ * @param projectId project identifier
+ * @param existingPolicy the project level IAM policy
+ */
+export function obtainFirebaseAlertsBindings(


Is this really a Firebase Alerts feature or something that all event providers are going to need?

NVM. See below comments. I think we could have had something like:

export function obtainFirebaseAlertBindings(projectNumber) { iam.ensureBinding(firebaseAlertRole, firebaseAlertPermission) }

You're correct, this is not a Firebase Alerts specific binding. It was yanked in #4324 and I added a noop instead

inlined · 2022-03-28T21:22:52Z

src/deploy/functions/services/firebaseAlerts.ts

+  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]);
+}


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?

SGTM, I believe that @taeold was thinking of refactoring our IAM code to look more like the Fabricator code

inlined · 2022-03-28T21:24:24Z

src/deploy/functions/services/index.ts

+/** A firebase alerts service object */
+export const FirebaseAlertsService: Service = {
+  name: "firebasealerts",
+  api: "logging.googleapis.com",


This is really the API for Firebase Alerts?? How is this not a conflict with Cloud Audit Logs?

This was just a placeholder, I'll update it with the correct API. Additionally, this field is not currently being used in the CLI

colerogers added 8 commits February 16, 2022 17:16

adding notes

3d72de1

Merge remote-tracking branch 'origin/master' into colerogers.firealerts

85d22be

Merge remote-tracking branch 'origin/master' into colerogers.firealerts

cc7fae1

fixing api of firealerts service object

3f0f582

Merge remote-tracking branch 'origin/master' into colerogers.firealerts

93f9383

cleaning up and adding tests

28e78d6

naming

0069bc3

changelog entry

7bd6667

colerogers marked this pull request as ready for review March 7, 2022 16:58

colerogers requested review from inlined and taeold March 7, 2022 16:58

taeold reviewed Mar 7, 2022

View reviewed changes

colerogers added 3 commits March 7, 2022 20:13

we now pass in projectId and projectNumber from prepare to requirePro…

463afba

…jectBindings

merge master

602e62e

updating error message

8fc53ac

colerogers requested a review from taeold March 8, 2022 01:27

taeold reviewed Mar 8, 2022

View reviewed changes

colerogers added 2 commits March 8, 2022 11:27

addressing pr comments

ccc38e7

removing unused import

599f672

taeold approved these changes Mar 8, 2022

View reviewed changes

Merge branch 'master' into colerogers.firealerts

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

9f1d3fe

colerogers merged commit e9ce087 into master Mar 8, 2022

colerogers deleted the colerogers.firealerts branch March 8, 2022 20:11

inlined reviewed Mar 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds new alerting event provider #4258

Adds new alerting event provider #4258

colerogers commented Mar 7, 2022 •

edited

Loading

taeold Mar 7, 2022

colerogers Mar 8, 2022

taeold Mar 8, 2022

colerogers Mar 8, 2022

taeold Mar 8, 2022

colerogers Mar 8, 2022

taeold Mar 8, 2022

colerogers Mar 8, 2022

inlined Mar 28, 2022

inlined Mar 28, 2022

colerogers Mar 29, 2022

inlined Mar 28, 2022

colerogers Mar 29, 2022

inlined Mar 28, 2022

colerogers Mar 29, 2022

Adds new alerting event provider #4258

Adds new alerting event provider #4258

Conversation

colerogers commented Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colerogers commented Mar 7, 2022 •

edited

Loading