-
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 23 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 |
---|---|---|
@@ -0,0 +1 @@ | ||
- Adds a blocking trigger type (#4395). |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import * as gcf from "../../gcp/cloudfunctions"; | |
import * as gcfV2 from "../../gcp/cloudfunctionsv2"; | ||
import * as utils from "../../utils"; | ||
import * as runtimes from "./runtimes"; | ||
import * as events from "../../functions/events"; | ||
import { FirebaseError } from "../../error"; | ||
import { Context } from "./args"; | ||
import { previews } from "../../previews"; | ||
|
@@ -131,6 +132,19 @@ export interface TaskQueueTriggered { | |
taskQueueTrigger: TaskQueueTrigger; | ||
} | ||
|
||
export interface BlockingTrigger { | ||
eventType: | ||
| typeof events.v1.AUTH_BLOCKING_EVENTS[number] | ||
| typeof events.v2.AUTH_BLOCKING_EVENTS[number]; | ||
accessToken?: boolean; | ||
idToken?: boolean; | ||
refreshToken?: boolean; | ||
colerogers marked this conversation as resolved.
Show resolved
Hide resolved
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. I have no suggestion on how to fix this, but I'm worried that this is not scalable. These should be blocking functions, not auth functions. Maybe we should have a map of features or something? 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. @inlined How about we go with a record structure for options and for auth blocking triggers we'd assume they'd use these three options. So the trigger will now be
What do you think? The biggest thing is we'd lose out on is type completion in ts |
||
} | ||
|
||
export interface BlockingTriggered { | ||
blockingTrigger: BlockingTrigger; | ||
} | ||
|
||
/** A user-friendly string for the kind of trigger of an endpoint. */ | ||
export function endpointTriggerType(endpoint: Endpoint): string { | ||
if (isScheduleTriggered(endpoint)) { | ||
|
@@ -143,6 +157,8 @@ export function endpointTriggerType(endpoint: Endpoint): string { | |
return endpoint.eventTrigger.eventType; | ||
} else if (isTaskQueueTriggered(endpoint)) { | ||
return "taskQueue"; | ||
} else if (isBlockingTriggered(endpoint)) { | ||
return "blocking"; | ||
colerogers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
throw new Error("Unexpected trigger type for endpoint " + JSON.stringify(endpoint)); | ||
} | ||
|
@@ -227,7 +243,8 @@ export type Triggered = | |
| CallableTriggered | ||
| EventTriggered | ||
| ScheduleTriggered | ||
| TaskQueueTriggered; | ||
| TaskQueueTriggered | ||
| BlockingTriggered; | ||
|
||
/** Whether something has an HttpsTrigger */ | ||
export function isHttpsTriggered(triggered: Triggered): triggered is HttpsTriggered { | ||
|
@@ -254,6 +271,11 @@ export function isTaskQueueTriggered(triggered: Triggered): triggered is TaskQue | |
return {}.hasOwnProperty.call(triggered, "taskQueueTrigger"); | ||
} | ||
|
||
/** Whether something has a BlockingTrigger */ | ||
export function isBlockingTriggered(triggered: Triggered): triggered is BlockingTriggered { | ||
return {}.hasOwnProperty.call(triggered, "blockingTrigger"); | ||
} | ||
|
||
/** | ||
* An endpoint that serves traffic to a stack of services. | ||
* For now, this is always a Cloud Function. Future iterations may use complex | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -204,6 +204,7 @@ export async function prepare( | |
|
||
await ensureServiceAgentRoles(projectNumber, wantBackend, haveBackend); | ||
inferDetailsFromExisting(wantBackend, haveBackend, usedDotenv); | ||
inferBlockingDetails(wantBackend); | ||
await ensureTriggerRegions(wantBackend); | ||
|
||
// Display a warning and prompt if any functions in the release have failurePolicies. | ||
|
@@ -271,3 +272,29 @@ 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 blockingEndpoints = backend | ||
.allEndpoints(want) | ||
.filter((ep) => backend.isBlockingTriggered(ep)) as (backend.Endpoint & | ||
backend.BlockingTriggered)[]; | ||
|
||
if (blockingEndpoints.length === 0) { | ||
return; | ||
} | ||
|
||
let accessToken = false; | ||
let idToken = false; | ||
let refreshToken = false; | ||
for (const blockingEp of blockingEndpoints) { | ||
accessToken ||= !!blockingEp.blockingTrigger.accessToken; | ||
idToken ||= !!blockingEp.blockingTrigger.idToken; | ||
refreshToken ||= !!blockingEp.blockingTrigger.refreshToken; | ||
} | ||
for (const blockingEp of blockingEndpoints) { | ||
blockingEp.blockingTrigger.accessToken = accessToken; | ||
blockingEp.blockingTrigger.idToken = idToken; | ||
blockingEp.blockingTrigger.refreshToken = refreshToken; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import * as reporter from "./reporter"; | |
import * as run from "../../../gcp/run"; | ||
import * as scheduler from "../../../gcp/cloudscheduler"; | ||
import * as utils from "../../../utils"; | ||
import * as authBlocking from "../services/authBlocking"; | ||
colerogers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// TODO: Tune this for better performance. | ||
const gcfV1PollerOptions: Omit<poller.OperationPollerOptions, "operationResourceName"> = { | ||
|
@@ -54,6 +55,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); | ||
}; | ||
|
||
|
@@ -164,7 +166,7 @@ export class Fabricator { | |
assertExhaustive(endpoint.platform); | ||
} | ||
|
||
await this.setTrigger(endpoint); | ||
await this.setTrigger(endpoint, false); | ||
} | ||
|
||
async updateEndpoint(update: planner.EndpointUpdate, scraper: SourceTokenScraper): Promise<void> { | ||
|
@@ -183,7 +185,7 @@ export class Fabricator { | |
assertExhaustive(update.endpoint.platform); | ||
} | ||
|
||
await this.setTrigger(update.endpoint); | ||
await this.setTrigger(update.endpoint, true); | ||
} | ||
|
||
async deleteEndpoint(endpoint: backend.Endpoint): Promise<void> { | ||
|
@@ -248,6 +250,13 @@ export class Fabricator { | |
}) | ||
.catch(rethrowAs(endpoint, "set invoker")); | ||
} | ||
} else if (backend.isBlockingTriggered(endpoint)) { | ||
// 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 +326,11 @@ export class Fabricator { | |
}) | ||
.catch(rethrowAs(endpoint, "set invoker")); | ||
} | ||
} else if (backend.isBlockingTriggered(endpoint)) { | ||
// 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 +368,8 @@ export class Fabricator { | |
invoker = endpoint.httpsTrigger.invoker; | ||
} else if (backend.isTaskQueueTriggered(endpoint)) { | ||
invoker = endpoint.taskQueueTrigger.invoker; | ||
} else if (backend.isBlockingTriggered(endpoint)) { | ||
invoker = ["public"]; | ||
colerogers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
if (invoker) { | ||
await this.executor | ||
|
@@ -395,6 +411,8 @@ export class Fabricator { | |
invoker = endpoint.httpsTrigger.invoker; | ||
} else if (backend.isTaskQueueTriggered(endpoint)) { | ||
invoker = endpoint.taskQueueTrigger.invoker; | ||
} else if (backend.isBlockingTriggered(endpoint)) { | ||
invoker = ["public"]; | ||
} | ||
if (invoker) { | ||
await this.executor | ||
|
@@ -460,7 +478,7 @@ export class Fabricator { | |
|
||
// Set/Delete trigger is responsible for wiring up a function with any trigger not owned | ||
// by the GCF API. This includes schedules, task queues, and blocking function triggers. | ||
async setTrigger(endpoint: backend.Endpoint): Promise<void> { | ||
async setTrigger(endpoint: backend.Endpoint, update: boolean): Promise<void> { | ||
if (backend.isScheduleTriggered(endpoint)) { | ||
if (endpoint.platform === "gcfv1") { | ||
await this.upsertScheduleV1(endpoint); | ||
|
@@ -472,6 +490,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, update); | ||
} | ||
} | ||
|
||
|
@@ -487,6 +507,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 +541,15 @@ export class Fabricator { | |
} | ||
} | ||
|
||
async registerBlockingTrigger( | ||
endpoint: backend.Endpoint & backend.BlockingTriggered, | ||
update: boolean | ||
): Promise<void> { | ||
await this.executor | ||
.run(() => authBlocking.registerTrigger(endpoint, update)) | ||
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. Again, this is only appropriate if it's an auth blocking 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. Changed it to be
|
||
.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 +577,14 @@ export class Fabricator { | |
.catch(rethrowAs(endpoint, "disable task queue")); | ||
} | ||
|
||
async unregisterBlockingTrigger( | ||
endpoint: backend.Endpoint & backend.BlockingTriggered | ||
): Promise<void> { | ||
await this.executor | ||
.run(() => authBlocking.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); | ||
|
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