-
Notifications
You must be signed in to change notification settings - Fork 900
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
Add blocking trigger type #4395
Changes from all commits
25f3d62
6fe07de
f616332
8566842
6a4b7b9
43b2316
eb016f7
9ce55af
801cb5f
0b22d42
94c4db4
b1cbf95
bee1b93
ed416c8
a298aad
62999db
e35f6c0
fc47475
43516bc
851c3a3
b674320
bd9a0b9
7f4fce2
7b69417
320d222
fbd7e3e
4fff5c0
153bffb
c671094
3c95a35
9faf7a8
77de23e
e758d2b
bb56d25
a4bfd25
a2f66cd
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 |
---|---|---|
|
@@ -21,6 +21,7 @@ import { ensureServiceAgentRoles } from "./checkIam"; | |
import { FirebaseError } from "../../error"; | ||
import { normalizeAndValidate } from "../../functions/projectConfig"; | ||
import { previews } from "../../previews"; | ||
import { AUTH_BLOCKING_EVENTS } from "../../functions/events/v1"; | ||
|
||
function hasUserConfig(config: Record<string, unknown>): boolean { | ||
// "firebase" key is always going to exist in runtime config. | ||
|
@@ -180,6 +181,7 @@ export async function prepare( | |
inferDetailsFromExisting(wantBackend, haveBackend, usedDotenv); | ||
await ensureTriggerRegions(wantBackend); | ||
validate.endpointsAreValid(wantBackend); | ||
inferBlockingDetails(wantBackend); | ||
|
||
payload.functions = { wantBackend: wantBackend, haveBackend: haveBackend }; | ||
|
||
|
@@ -283,3 +285,35 @@ function maybeCopyTriggerRegion(wantE: backend.Endpoint, haveE: backend.Endpoint | |
} | ||
wantE.eventTrigger.region = haveE.eventTrigger.region; | ||
} | ||
|
||
/** Figures out the blocking endpoint options by taking the OR of every trigger option and reassigning that value back to the endpoint. */ | ||
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. Why is this the correct behavior? If I have a beforeUserCreated function I must have the same security options as a beforeUserLoggedIn function? 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. It's part of the trigger proposal - http://go/cf3-auth-blocking-triggers Since these are resource level options, they must be the same. Instead of erroring if a user sets the |
||
export function inferBlockingDetails(want: backend.Backend): void { | ||
const authBlockingEndpoints = backend | ||
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. nit* technically 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. Actually here I do a check in |
||
.allEndpoints(want) | ||
.filter( | ||
(ep) => | ||
backend.isBlockingTriggered(ep) && | ||
AUTH_BLOCKING_EVENTS.includes(ep.blockingTrigger.eventType as any) | ||
) as (backend.Endpoint & backend.BlockingTriggered)[]; | ||
|
||
if (authBlockingEndpoints.length === 0) { | ||
return; | ||
} | ||
|
||
let accessToken = false; | ||
let idToken = false; | ||
let refreshToken = false; | ||
for (const blockingEp of authBlockingEndpoints) { | ||
accessToken ||= !!blockingEp.blockingTrigger.options?.accessToken; | ||
idToken ||= !!blockingEp.blockingTrigger.options?.idToken; | ||
refreshToken ||= !!blockingEp.blockingTrigger.options?.refreshToken; | ||
} | ||
for (const blockingEp of authBlockingEndpoints) { | ||
if (!blockingEp.blockingTrigger.options) { | ||
blockingEp.blockingTrigger.options = {}; | ||
} | ||
blockingEp.blockingTrigger.options.accessToken = accessToken; | ||
blockingEp.blockingTrigger.options.idToken = idToken; | ||
blockingEp.blockingTrigger.options.refreshToken = refreshToken; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,8 @@ import * as reporter from "./reporter"; | |
import * as run from "../../../gcp/run"; | ||
import * as scheduler from "../../../gcp/cloudscheduler"; | ||
import * as utils from "../../../utils"; | ||
import * as services from "../services"; | ||
import { AUTH_BLOCKING_EVENTS } from "../../../functions/events/v1"; | ||
|
||
// TODO: Tune this for better performance. | ||
const gcfV1PollerOptions: Omit<poller.OperationPollerOptions, "operationResourceName"> = { | ||
|
@@ -54,6 +56,7 @@ export interface FabricatorArgs { | |
const rethrowAs = | ||
<T>(endpoint: backend.Endpoint, op: reporter.OperationType) => | ||
(err: unknown): T => { | ||
logger.error((err as Error).message); | ||
colerogers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
throw new reporter.DeploymentError(endpoint, op, err); | ||
}; | ||
|
||
|
@@ -248,6 +251,16 @@ export class Fabricator { | |
}) | ||
.catch(rethrowAs(endpoint, "set invoker")); | ||
} | ||
} else if ( | ||
backend.isBlockingTriggered(endpoint) && | ||
AUTH_BLOCKING_EVENTS.includes(endpoint.blockingTrigger.eventType as any) | ||
) { | ||
// Auth Blocking functions should always be public | ||
await this.executor | ||
.run(async () => { | ||
await gcf.setInvokerCreate(endpoint.project, backend.functionName(endpoint), ["public"]); | ||
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. Here and elsewhere, it's 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. Added a check
|
||
}) | ||
.catch(rethrowAs(endpoint, "set invoker")); | ||
} | ||
} | ||
|
||
|
@@ -317,6 +330,14 @@ export class Fabricator { | |
}) | ||
.catch(rethrowAs(endpoint, "set invoker")); | ||
} | ||
} else if ( | ||
backend.isBlockingTriggered(endpoint) && | ||
AUTH_BLOCKING_EVENTS.includes(endpoint.blockingTrigger.eventType as any) | ||
) { | ||
// Auth Blocking functions should always be public | ||
await this.executor | ||
.run(() => run.setInvokerCreate(endpoint.project, serviceName, ["public"])) | ||
.catch(rethrowAs(endpoint, "set invoker")); | ||
} | ||
|
||
const mem = endpoint.availableMemoryMb || backend.DEFAULT_MEMORY; | ||
|
@@ -354,6 +375,11 @@ export class Fabricator { | |
invoker = endpoint.httpsTrigger.invoker; | ||
} else if (backend.isTaskQueueTriggered(endpoint)) { | ||
invoker = endpoint.taskQueueTrigger.invoker; | ||
} else if ( | ||
backend.isBlockingTriggered(endpoint) && | ||
AUTH_BLOCKING_EVENTS.includes(endpoint.blockingTrigger.eventType as any) | ||
) { | ||
invoker = ["public"]; | ||
} | ||
if (invoker) { | ||
await this.executor | ||
|
@@ -395,6 +421,11 @@ export class Fabricator { | |
invoker = endpoint.httpsTrigger.invoker; | ||
} else if (backend.isTaskQueueTriggered(endpoint)) { | ||
invoker = endpoint.taskQueueTrigger.invoker; | ||
} else if ( | ||
backend.isBlockingTriggered(endpoint) && | ||
AUTH_BLOCKING_EVENTS.includes(endpoint.blockingTrigger.eventType as any) | ||
) { | ||
invoker = ["public"]; | ||
} | ||
if (invoker) { | ||
await this.executor | ||
|
@@ -472,6 +503,8 @@ export class Fabricator { | |
assertExhaustive(endpoint.platform); | ||
} else if (backend.isTaskQueueTriggered(endpoint)) { | ||
await this.upsertTaskQueue(endpoint); | ||
} else if (backend.isBlockingTriggered(endpoint)) { | ||
await this.registerBlockingTrigger(endpoint); | ||
} | ||
} | ||
|
||
|
@@ -487,6 +520,8 @@ export class Fabricator { | |
assertExhaustive(endpoint.platform); | ||
} else if (backend.isTaskQueueTriggered(endpoint)) { | ||
await this.disableTaskQueue(endpoint); | ||
} else if (backend.isBlockingTriggered(endpoint)) { | ||
await this.unregisterBlockingTrigger(endpoint); | ||
} | ||
} | ||
|
||
|
@@ -519,6 +554,14 @@ export class Fabricator { | |
} | ||
} | ||
|
||
async registerBlockingTrigger( | ||
endpoint: backend.Endpoint & backend.BlockingTriggered | ||
): Promise<void> { | ||
await this.executor | ||
.run(() => services.serviceForEndpoint(endpoint).registerTrigger(endpoint)) | ||
.catch(rethrowAs(endpoint, "register blocking trigger")); | ||
} | ||
|
||
async deleteScheduleV1(endpoint: backend.Endpoint & backend.ScheduleTriggered): Promise<void> { | ||
const job = scheduler.jobFromEndpoint(endpoint, this.appEngineLocation); | ||
await this.executor | ||
|
@@ -546,6 +589,14 @@ export class Fabricator { | |
.catch(rethrowAs(endpoint, "disable task queue")); | ||
} | ||
|
||
async unregisterBlockingTrigger( | ||
endpoint: backend.Endpoint & backend.BlockingTriggered | ||
): Promise<void> { | ||
await this.executor | ||
.run(() => services.serviceForEndpoint(endpoint).unregisterTrigger(endpoint)) | ||
.catch(rethrowAs(endpoint, "unregister blocking trigger")); | ||
} | ||
|
||
logOpStart(op: string, endpoint: backend.Endpoint): void { | ||
const runtime = getHumanFriendlyRuntimeName(endpoint.runtime); | ||
const label = helper.getFunctionLabel(endpoint); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ import * as api from "../../../../api"; | |
import * as proto from "../../../../gcp/proto"; | ||
import * as args from "../../args"; | ||
import * as runtimes from "../../runtimes"; | ||
import * as v2events from "../../../../functions/events/v2"; | ||
import * as events from "../../../../functions/events"; | ||
|
||
const TRIGGER_PARSER = path.resolve(__dirname, "./triggerParser.js"); | ||
|
||
|
@@ -68,6 +68,10 @@ export interface TriggerAnnotation { | |
}; | ||
invoker?: string[]; | ||
}; | ||
blockingTrigger?: { | ||
eventType: string; | ||
options?: Record<string, any>; | ||
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. nit* I've been preferring 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 #4443 |
||
}; | ||
failurePolicy?: {}; | ||
schedule?: ScheduleAnnotation; | ||
timeZone?: string; | ||
|
@@ -180,7 +184,10 @@ export function addResourcesToBackend( | |
|
||
// +!! is 1 for truthy values and 0 for falsy values | ||
const triggerCount = | ||
+!!annotation.httpsTrigger + +!!annotation.eventTrigger + +!!annotation.taskQueueTrigger; | ||
+!!annotation.httpsTrigger + | ||
+!!annotation.eventTrigger + | ||
+!!annotation.taskQueueTrigger + | ||
+!!annotation.blockingTrigger; | ||
if (triggerCount !== 1) { | ||
throw new FirebaseError( | ||
"Unexpected annotation generated by the Firebase Functions SDK. This should never happen." | ||
|
@@ -211,6 +218,19 @@ export function addResourcesToBackend( | |
reason: "Needed for scheduled functions.", | ||
}); | ||
triggered = { scheduleTrigger: annotation.schedule }; | ||
} else if (annotation.blockingTrigger) { | ||
if (events.v1.AUTH_BLOCKING_EVENTS.includes(annotation.blockingTrigger.eventType as any)) { | ||
want.requiredAPIs.push({ | ||
api: "identitytoolkit.googleapis.com", | ||
reason: "Needed for auth blocking functions.", | ||
}); | ||
} | ||
triggered = { | ||
blockingTrigger: { | ||
eventType: annotation.blockingTrigger.eventType, | ||
options: annotation.blockingTrigger.options, | ||
}, | ||
}; | ||
} else { | ||
triggered = { | ||
eventTrigger: { | ||
|
@@ -223,12 +243,12 @@ export function addResourcesToBackend( | |
// TODO: yank this edge case for a v2 trigger on the pre-container contract | ||
// once we use container contract for the functionsv2 experiment. | ||
if (annotation.platform === "gcfv2") { | ||
if (annotation.eventTrigger!.eventType === v2events.PUBSUB_PUBLISH_EVENT) { | ||
if (annotation.eventTrigger!.eventType === events.v2.PUBSUB_PUBLISH_EVENT) { | ||
triggered.eventTrigger.eventFilters = { topic: annotation.eventTrigger!.resource }; | ||
} | ||
|
||
if ( | ||
v2events.STORAGE_EVENTS.find( | ||
events.v2.STORAGE_EVENTS.find( | ||
(event) => event === (annotation.eventTrigger?.eventType || "") | ||
) | ||
) { | ||
|
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.
nit* remove events per linter
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.
Removed in #4443