-
Notifications
You must be signed in to change notification settings - Fork 902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds new alerting event provider #4258
Changes from 8 commits
3d72de1
85d22be
cc7fae1
3f0f582
93f9383
28e78d6
0069bc3
7bd6667
463afba
602e62e
8fc53ac
ccc38e7
599f672
9f1d3fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
- Adds alerting event provider (#4258). |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import * as backend from "../backend"; | ||
import * as iam from "../../../gcp/iam"; | ||
import { getProjectNumber } from "../../../getProjectNumber"; | ||
import { FirebaseError } from "../../../error"; | ||
|
||
export const SERVICE_ACCOUNT_TOKEN_CREATOR_ROLE = "roles/iam.serviceAccountTokenCreator"; | ||
|
||
/** | ||
* Finds the required project level IAM bindings for the Pub/Sub service agent | ||
* If the user enabled Pub/Sub on or before April 8, 2021, then we must enable the token creator role | ||
* @param projectId project identifier | ||
* @param existingPolicy the project level IAM policy | ||
*/ | ||
export async function obtainFirebaseAlertsBindings( | ||
projectId: string, | ||
existingPolicy: iam.Policy | ||
): Promise<Array<iam.Binding>> { | ||
const projectNumber = await getProjectNumber({ projectId }); | ||
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 [pubsubBinding]; | ||
} | ||
|
||
/** | ||
* Sets a Firebase Alerts event trigger's region to 'global' since the service is global | ||
* @param endpoint the storage endpoint | ||
* @param eventTrigger the endpoints event trigger | ||
*/ | ||
export function ensureFirebaseAlertsTriggerRegion( | ||
endpoint: backend.Endpoint & backend.EventTriggered | ||
): void { | ||
if (!endpoint.eventTrigger.region) { | ||
endpoint.eventTrigger.region = "global"; | ||
} | ||
if (endpoint.eventTrigger.region !== "global") { | ||
throw new FirebaseError("A firebase alerts function must have a 'global' trigger location"); | ||
colerogers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ import * as backend from "../backend"; | |
import * as iam from "../../../gcp/iam"; | ||
import * as v2events from "../../../functions/events/v2"; | ||
import { obtainStorageBindings, ensureStorageTriggerRegion } from "./storage"; | ||
import { obtainFirebaseAlertsBindings, ensureFirebaseAlertsTriggerRegion } from "./firebaseAlerts"; | ||
|
||
const noop = (): Promise<void> => Promise.resolve(); | ||
|
||
|
@@ -12,7 +13,7 @@ export interface Service { | |
|
||
// dispatch functions | ||
requiredProjectBindings: ((pId: any, p: any) => Promise<Array<iam.Binding>>) | undefined; | ||
ensureTriggerRegion: (ep: backend.Endpoint & backend.EventTriggered) => Promise<void>; | ||
ensureTriggerRegion: (ep: backend.Endpoint & backend.EventTriggered) => Promise<void> | void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in the latest commit |
||
} | ||
|
||
/** A noop service object, useful for v1 events */ | ||
|
@@ -30,12 +31,19 @@ export const PubSubService: Service = { | |
ensureTriggerRegion: noop, | ||
}; | ||
/** A storage service object */ | ||
export const StorageService = { | ||
export const StorageService: Service = { | ||
name: "storage", | ||
api: "storage.googleapis.com", | ||
requiredProjectBindings: obtainStorageBindings, | ||
ensureTriggerRegion: ensureStorageTriggerRegion, | ||
}; | ||
/** A firebase alerts service object */ | ||
export const FirebaseAlertsService: Service = { | ||
name: "firebasealerts", | ||
api: "logging.googleapis.com", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was just a placeholder, I'll update it with the correct API. Additionally, this field is not currently being used in the CLI |
||
requiredProjectBindings: obtainFirebaseAlertsBindings, | ||
ensureTriggerRegion: ensureFirebaseAlertsTriggerRegion, | ||
}; | ||
|
||
/** Mapping from event type string to service object */ | ||
export const EVENT_SERVICE_MAPPING: Record<v2events.Event, Service> = { | ||
|
@@ -44,6 +52,7 @@ export const EVENT_SERVICE_MAPPING: Record<v2events.Event, Service> = { | |
"google.cloud.storage.object.v1.archived": StorageService, | ||
"google.cloud.storage.object.v1.deleted": StorageService, | ||
"google.cloud.storage.object.v1.metadataUpdated": StorageService, | ||
"firebase.firebasealerts.alerts.v1.published": FirebaseAlertsService, | ||
}; | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
import { expect } from "chai"; | ||
import { Endpoint } from "../../../../deploy/functions/backend"; | ||
import * as firebaseAlerts from "../../../../deploy/functions/services/firebaseAlerts"; | ||
import * as getProjectNumber from "../../../../getProjectNumber"; | ||
import * as sinon from "sinon"; | ||
|
||
describe("obtainFirebaseAlertsBindings", () => { | ||
let projectNumberStub: sinon.SinonStub; | ||
|
||
const iamPolicy = { | ||
etag: "etag", | ||
version: 3, | ||
bindings: [ | ||
{ | ||
role: "some/role", | ||
members: ["someuser"], | ||
}, | ||
], | ||
}; | ||
|
||
beforeEach(() => { | ||
projectNumberStub = sinon.stub(getProjectNumber, "getProjectNumber").resolves("123456789"); | ||
}); | ||
|
||
afterEach(() => { | ||
sinon.verifyAndRestore(); | ||
}); | ||
|
||
it("should add the binding", async () => { | ||
const policy = { ...iamPolicy }; | ||
|
||
const bindings = await firebaseAlerts.obtainFirebaseAlertsBindings("project", policy); | ||
|
||
expect(bindings.length).to.equal(1); | ||
expect(bindings[0]).to.deep.equal({ | ||
role: firebaseAlerts.SERVICE_ACCOUNT_TOKEN_CREATOR_ROLE, | ||
members: ["serviceAccount:service-123456789@gcp-sa-pubsub.iam.gserviceaccount.com"], | ||
}); | ||
}); | ||
|
||
it("should add the service agent as a member", async () => { | ||
const policy = { ...iamPolicy }; | ||
policy.bindings = [ | ||
{ | ||
role: firebaseAlerts.SERVICE_ACCOUNT_TOKEN_CREATOR_ROLE, | ||
members: ["someuser"], | ||
}, | ||
]; | ||
|
||
const bindings = await firebaseAlerts.obtainFirebaseAlertsBindings("project", policy); | ||
|
||
expect(bindings.length).to.equal(1); | ||
expect(bindings[0]).to.deep.equal({ | ||
role: firebaseAlerts.SERVICE_ACCOUNT_TOKEN_CREATOR_ROLE, | ||
members: [ | ||
"someuser", | ||
"serviceAccount:service-123456789@gcp-sa-pubsub.iam.gserviceaccount.com", | ||
], | ||
}); | ||
}); | ||
|
||
it("should do nothing if we have the binding", async () => { | ||
const policy = { ...iamPolicy }; | ||
policy.bindings = [ | ||
{ | ||
role: firebaseAlerts.SERVICE_ACCOUNT_TOKEN_CREATOR_ROLE, | ||
members: ["serviceAccount:service-123456789@gcp-sa-pubsub.iam.gserviceaccount.com"], | ||
}, | ||
]; | ||
|
||
const bindings = await firebaseAlerts.obtainFirebaseAlertsBindings("project", policy); | ||
|
||
expect(bindings.length).to.equal(1); | ||
expect(bindings[0]).to.deep.equal({ | ||
role: firebaseAlerts.SERVICE_ACCOUNT_TOKEN_CREATOR_ROLE, | ||
members: ["serviceAccount:service-123456789@gcp-sa-pubsub.iam.gserviceaccount.com"], | ||
}); | ||
}); | ||
}); | ||
|
||
describe("ensureFirebaseAlertsTriggerRegion", () => { | ||
const endpoint: Endpoint = { | ||
id: "endpoint", | ||
region: "us-central1", | ||
project: "my-project", | ||
eventTrigger: { | ||
retry: false, | ||
eventType: "firebase.firebasealerts.alerts.v1.published", | ||
eventFilters: [], | ||
}, | ||
entryPoint: "", | ||
platform: "gcfv2", | ||
runtime: "nodejs16", | ||
}; | ||
it("should set the trigger location to global", () => { | ||
const ep = { ...endpoint }; | ||
|
||
firebaseAlerts.ensureFirebaseAlertsTriggerRegion(ep); | ||
|
||
expect(endpoint.eventTrigger.region).to.eq("global"); | ||
}); | ||
|
||
it("should not error if the trigger location is global", () => { | ||
const ep = { ...endpoint }; | ||
ep.eventTrigger.region = "global"; | ||
|
||
firebaseAlerts.ensureFirebaseAlertsTriggerRegion(endpoint); | ||
|
||
expect(endpoint.eventTrigger.region).to.eq("global"); | ||
}); | ||
|
||
it("should error if the trigger location is not global", () => { | ||
const ep = { ...endpoint }; | ||
ep.eventTrigger.region = "us-west1"; | ||
|
||
expect(() => firebaseAlerts.ensureFirebaseAlertsTriggerRegion(endpoint)).to.throw( | ||
"A firebase alerts function must have a 'global' trigger location" | ||
); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inprepare.ts
so subsequent calls togetProjectNumber
is properly cached instead of making one-off calls here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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