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
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
- Fixes bug where functions' memory configurations weren't preserved in batched function deploys (#4253).
- Adds alerting event provider (#4258).
8 changes: 4 additions & 4 deletions src/deploy/functions/checkIam.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
projectNumber: string,
colerogers marked this conversation as resolved.
Show resolved Hide resolved
want: backend.Backend,
have: backend.Backend
): Promise<void> {
Expand All @@ -164,7 +164,7 @@ export async function ensureServiceAgentRoles(
// get the full project iam policy
let policy: iam.Policy;
try {
policy = await getIamPolicy(projectId);
policy = await getIamPolicy(projectNumber);
} catch (err: any) {
utils.logLabeledBullet(
"functions",
Expand All @@ -178,13 +178,13 @@ export async function ensureServiceAgentRoles(
// run in parallel all the missingProjectBindings jobs
const findRequiredBindings: Array<Promise<Array<iam.Binding>>> = [];
newServices.forEach((service) =>
findRequiredBindings.push(service.requiredProjectBindings!(projectId, policy))
findRequiredBindings.push(service.requiredProjectBindings!(projectNumber, policy))
);
const allRequiredBindings = await Promise.all(findRequiredBindings);
mergeBindings(policy, allRequiredBindings);
// set the updated policy
try {
await setIamPolicy(projectId, policy, "bindings");
await setIamPolicy(projectNumber, policy, "bindings");
} catch (err: any) {
throw new FirebaseError(
"We failed to modify the IAM policy for the project. The functions " +
Expand Down
5 changes: 3 additions & 2 deletions src/deploy/functions/prepare.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { logBullet } from "../../utils";
import { getFunctionsConfig, prepareFunctionsUpload } from "./prepareFunctionsUpload";
import { promptForFailurePolicies, promptForMinInstances } from "./prompts";
import { previews } from "../../previews";
import { needProjectId } from "../../projectUtils";
import { needProjectId, needProjectNumber } from "../../projectUtils";
import { track } from "../../track";
import { logger } from "../../logger";
import { ensureTriggerRegions } from "./triggerRegionHelper";
Expand All @@ -37,6 +37,7 @@ export async function prepare(
payload: args.Payload
): Promise<void> {
const projectId = needProjectId(options);
const projectNumber = await needProjectNumber(options);

const sourceDirName = options.config.get("functions.source") as string;
if (!sourceDirName) {
Expand Down Expand Up @@ -160,7 +161,7 @@ export async function prepare(
});

const haveBackend = await backend.existingBackend(context);
await ensureServiceAgentRoles(projectId, wantBackend, haveBackend);
await ensureServiceAgentRoles(projectNumber, wantBackend, haveBackend);
inferDetailsFromExisting(wantBackend, haveBackend, usedDotenv);
await ensureTriggerRegions(wantBackend);

Expand Down
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 function obtainFirebaseAlertsBindings(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

projectNumber: string,
existingPolicy: iam.Policy
): Promise<Array<iam.Binding>> {
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]);
}
Comment on lines +18 to +32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* 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
): Promise<void> {
if (!endpoint.eventTrigger.region) {
endpoint.eventTrigger.region = "global";
}
if (endpoint.eventTrigger.region !== "global") {
throw new FirebaseError("A firebase alerts trigger must specify 'global' trigger location");
}
return Promise.resolve();
}
15 changes: 13 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 @@ -11,7 +12,9 @@ export interface Service {
readonly api: string;

// dispatch functions
requiredProjectBindings: ((pId: any, p: any) => Promise<Array<iam.Binding>>) | undefined;
requiredProjectBindings:
| ((projectNumber: string, policy: iam.Policy) => Promise<Array<iam.Binding>>)
| undefined;
ensureTriggerRegion: (ep: backend.Endpoint & backend.EventTriggered) => Promise<void>;
}

Expand All @@ -30,12 +33,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 +54,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
4 changes: 2 additions & 2 deletions src/deploy/functions/services/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ const PUBSUB_PUBLISHER_ROLE = "roles/pubsub.publisher";
* @param existingPolicy the project level IAM policy
*/
export async function obtainStorageBindings(
projectId: string,
projectNumber: string,
existingPolicy: iam.Policy
): Promise<Array<iam.Binding>> {
const storageResponse = await storage.getServiceAccount(projectId);
const storageResponse = await storage.getServiceAccount(projectNumber);
const storageServiceAgent = `serviceAccount:${storageResponse.email_address}`;
let pubsubBinding = existingPolicy.bindings.find((b) => b.role === PUBSUB_PUBLISHER_ROLE);
if (!pubsubBinding) {
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;
14 changes: 8 additions & 6 deletions src/gcp/resourceManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,30 @@ export const firebaseRoles = {
* Fetches the IAM Policy of a project.
* https://cloud.google.com/resource-manager/reference/rest/v1/projects/getIamPolicy
*
* @param projectId the id of the project whose IAM Policy you want to get
* @param projectIdOrNumber the id of the project whose IAM Policy you want to get
*/
export async function getIamPolicy(projectId: string): Promise<Policy> {
const response = await apiClient.post<void, Policy>(`/projects/${projectId}:getIamPolicy`);
export async function getIamPolicy(projectIdOrNumber: string): Promise<Policy> {
const response = await apiClient.post<void, Policy>(
`/projects/${projectIdOrNumber}:getIamPolicy`
);
return response.body;
}

/**
* Sets the IAM Policy of a project.
* https://cloud.google.com/resource-manager/reference/rest/v1/projects/setIamPolicy
*
* @param projectId the id of the project for which you want to set a new IAM Policy
* @param projectIdOrNumber the id of the project for which you want to set a new IAM Policy
* @param newPolicy the new IAM policy for the project
* @param updateMask A FieldMask specifying which fields of the policy to modify
*/
export async function setIamPolicy(
projectId: string,
projectIdOrNumber: string,
newPolicy: Policy,
updateMask = ""
): Promise<Policy> {
const response = await apiClient.post<{ policy: Policy; updateMask: string }, Policy>(
`/projects/${projectId}:setIamPolicy`,
`/projects/${projectIdOrNumber}:setIamPolicy`,
{
policy: newPolicy,
updateMask: updateMask,
Expand Down
19 changes: 11 additions & 8 deletions src/test/deploy/functions/checkIam.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import * as storage from "../../../gcp/storage";
import * as rm from "../../../gcp/resourceManager";
import * as backend from "../../../deploy/functions/backend";

const projectNumber = "123456789";

const STORAGE_RES = {
email_address: "service-123@gs-project-accounts.iam.gserviceaccount.com",
kind: "storage#serviceAccount",
Expand All @@ -17,7 +19,7 @@ const BINDING = {

const SPEC = {
region: "us-west1",
project: "my-project",
project: projectNumber,
runtime: "nodejs14",
};

Expand Down Expand Up @@ -105,10 +107,11 @@ describe("checkIam", () => {
...SPEC,
};

await expect(checkIam.ensureServiceAgentRoles("project", backend.of(wantFn), backend.empty()))
.to.not.be.rejected;
await expect(
checkIam.ensureServiceAgentRoles(projectNumber, backend.of(wantFn), backend.empty())
).to.not.be.rejected;
expect(getIamStub).to.have.been.calledOnce;
expect(getIamStub).to.have.been.calledWith("project");
expect(getIamStub).to.have.been.calledWith(projectNumber);
expect(storageStub).to.not.have.been.called;
expect(setIamStub).to.not.have.been.called;
});
Expand Down Expand Up @@ -155,7 +158,7 @@ describe("checkIam", () => {
};

await checkIam.ensureServiceAgentRoles(
"project",
projectNumber,
backend.of(wantFn),
backend.of(v1EventFn, v2CallableFn, wantFn)
);
Expand Down Expand Up @@ -199,7 +202,7 @@ describe("checkIam", () => {
...SPEC,
};

await checkIam.ensureServiceAgentRoles("project", backend.of(wantFn), backend.of(haveFn));
await checkIam.ensureServiceAgentRoles(projectNumber, backend.of(wantFn), backend.of(haveFn));

expect(storageStub).to.not.have.been.called;
expect(getIamStub).to.not.have.been.called;
Expand Down Expand Up @@ -242,12 +245,12 @@ describe("checkIam", () => {
...SPEC,
};

await checkIam.ensureServiceAgentRoles("project", backend.of(wantFn), backend.empty());
await checkIam.ensureServiceAgentRoles(projectNumber, backend.of(wantFn), backend.empty());

expect(storageStub).to.have.been.calledOnce;
expect(getIamStub).to.have.been.calledOnce;
expect(setIamStub).to.have.been.calledOnce;
expect(setIamStub).to.have.been.calledWith("project", newIamPolicy, "bindings");
expect(setIamStub).to.have.been.calledWith(projectNumber, newIamPolicy, "bindings");
});
});
});
111 changes: 111 additions & 0 deletions src/test/deploy/functions/services/firebaseAlerts.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import { expect } from "chai";
import { Endpoint } from "../../../../deploy/functions/backend";
import * as firebaseAlerts from "../../../../deploy/functions/services/firebaseAlerts";

const projectNumber = "123456789";

const endpoint: Endpoint = {
id: "endpoint",
region: "us-central1",
project: projectNumber,
eventTrigger: {
retry: false,
eventType: "firebase.firebasealerts.alerts.v1.published",
eventFilters: [],
},
entryPoint: "endpoint",
platform: "gcfv2",
runtime: "nodejs16",
};

describe("obtainFirebaseAlertsBindings", () => {
const iamPolicy = {
etag: "etag",
version: 3,
bindings: [
{
role: "some/role",
members: ["someuser"],
},
],
};

it("should add the binding", async () => {
const policy = { ...iamPolicy };

const bindings = await firebaseAlerts.obtainFirebaseAlertsBindings(projectNumber, 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(projectNumber, 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(projectNumber, 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", () => {
it("should set the trigger location to global", async () => {
const ep = { ...endpoint };

await firebaseAlerts.ensureFirebaseAlertsTriggerRegion(ep);

expect(endpoint.eventTrigger.region).to.eq("global");
});

it("should not error if the trigger location is global", async () => {
const ep = { ...endpoint };
ep.eventTrigger.region = "global";

await 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 trigger must specify 'global' trigger location"
);
});
});