Skip to content
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

Merged
merged 14 commits into from
Mar 8, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Adds alerting event provider (#4258).
49 changes: 49 additions & 0 deletions src/deploy/functions/services/firebaseAlerts.ts
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 });
Copy link
Contributor

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 in prepare.ts so subsequent calls to getProjectNumber is properly cached instead of making one-off calls here?

Copy link
Contributor Author

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

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
}
}
13 changes: 11 additions & 2 deletions src/deploy/functions/services/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 */
Expand All @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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> = {
Expand All @@ -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,
};

/**
Expand Down
2 changes: 1 addition & 1 deletion src/deploy/functions/triggerRegionHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { serviceForEndpoint } from "./services";
* @param have the list of function specs we have deployed
*/
export async function ensureTriggerRegions(want: backend.Backend): Promise<void> {
const regionLookups: Array<Promise<void>> = [];
const regionLookups: Array<Promise<void> | void> = [];
colerogers marked this conversation as resolved.
Show resolved Hide resolved

for (const ep of backend.allEndpoints(want)) {
if (ep.platform === "gcfv1" || !backend.isEventTriggered(ep)) {
Expand Down
7 changes: 6 additions & 1 deletion src/functions/events/v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,9 @@ export const STORAGE_EVENTS = [
"google.cloud.storage.object.v1.metadataUpdated",
] as const;

export type Event = typeof PUBSUB_PUBLISH_EVENT | typeof STORAGE_EVENTS[number];
export const FIREBASE_ALERTS_PUBLISH_EVENT = "firebase.firebasealerts.alerts.v1.published";

export type Event =
| typeof PUBSUB_PUBLISH_EVENT
| typeof STORAGE_EVENTS[number]
| typeof FIREBASE_ALERTS_PUBLISH_EVENT;
120 changes: 120 additions & 0 deletions src/test/deploy/functions/services/firebaseAlerts.spec.ts
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"
);
});
});