-
Notifications
You must be signed in to change notification settings - Fork 1k
Router worker performs free tier limiting #9360
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
Router worker performs free tier limiting #9360
Conversation
🦋 Changeset detectedLatest commit: a1cd3f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
51da078 to
28f55f4
Compare
|
This is rebased over #9416 and thus the diff includes those commits as well until it is merged |
e3cc92b to
4f4956f
Compare
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.
Product approved this messaging, yeah? Looks good to me — just checking.
Do we want to snazz it up a bit with some CSS/illustration at all?
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.
yeah i expected this to have plenty of product feedback here and to iterate based on that. i did no styling (or even markup really) and just brought over the copy of what's currently in FL. happy to do a basic pass first
|
|
||
| const maybeSecondRequest = request.clone(); | ||
|
|
||
| let asset: "static_routing" | "ignored" | true | false = false; |
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 don't love mutable and non-linear variables like this (though absolutely have used them in a few places myself!)
Is it at all cleaner if this is becomes an arg to the routeToUserWorker and routeToAssets functions?
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.
cleaner is relative here IMO. the typing becomes cleaner but the call sites is more verbose. no strong opinion 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.
Not a big fan of this variable being mutated and used in different places. Can we make this a parameter instead?
diff --git a/packages/workers-shared/router-worker/src/worker.ts b/packages/workers-shared/router-worker/src/worker.ts
index d88375ffe..9c188c36b 100644
--- a/packages/workers-shared/router-worker/src/worker.ts
+++ b/packages/workers-shared/router-worker/src/worker.ts
@@ -104,9 +104,7 @@ export default {
const maybeSecondRequest = request.clone();
- let asset: "static_routing" | "ignored" | true | false = false;
-
- const routeToUserWorker = async () => {
+ const routeToUserWorker = async ({ asset }: { asset: AssetTag }) => {
if (!config.has_user_worker) {
throw new Error(
"Fetch for user worker without having a user worker binding"
@@ -126,19 +124,19 @@ export default {
return env.JAEGER.enterSpan("dispatch_worker", async (span) => {
span.setTags({
hasUserWorker: true,
- asset: asset,
+ asset,
dispatchType: DISPATCH_TYPE.WORKER,
});
return env.USER_WORKER.fetch(maybeSecondRequest);
});
};
- const routeToAssets = async () => {
+ const routeToAssets = async ({ asset }: { asset: AssetTag }) => {
analytics.setData({ dispatchtype: DISPATCH_TYPE.ASSETS });
return await env.JAEGER.enterSpan("dispatch_assets", async (span) => {
span.setTags({
hasUserWorker: config.has_user_worker,
- asset: asset,
+ asset,
dispatchType: DISPATCH_TYPE.ASSETS,
});
@@ -160,8 +158,7 @@ export default {
analytics.setData({
staticRoutingDecision: STATIC_ROUTING_DECISION.ROUTED,
});
- asset = "static_routing";
- return await routeToAssets();
+ return await routeToAssets({ asset: "static_routing" });
}
// evaluate "include" rules
const includeRulesMatcher = generateStaticRoutingRuleMatcher(
@@ -181,8 +178,7 @@ export default {
analytics.setData({
staticRoutingDecision: STATIC_ROUTING_DECISION.ROUTED,
});
- asset = "static_routing";
- return await routeToUserWorker();
+ return await routeToUserWorker({ asset: "static_routing" });
}
analytics.setData({
@@ -195,19 +191,17 @@ export default {
// User's configuration indicates they want user-Worker to run ahead of any
// assets. Do not provide any fallback logic.
if (config.invoke_user_worker_ahead_of_assets) {
- asset = "ignored";
- return await routeToUserWorker();
+ return await routeToUserWorker({ asset: "ignored" });
}
// If we have a user-Worker, but no assets, dispatch to Worker script
const assetsExist = await env.ASSET_WORKER.unstable_canFetch(request);
- asset = assetsExist;
if (config.has_user_worker && !assetsExist) {
- return await routeToUserWorker();
+ return await routeToUserWorker({ asset: assetsExist });
}
// Otherwise, we either don't have a user worker, OR we have matching assets and should fetch from the assets binding
- return await routeToAssets();
+ return await routeToAssets({ asset: assetsExist });
} catch (err) {
if (userWorkerInvocation) {
// Don't send user Worker errors to sentry; we have no way to distinguish between
@@ -228,3 +222,5 @@ export default {
}
},
};
+
+type AssetTag = "static_routing" | "ignored" | true | false;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 think the fail-closed coming hard on even for existing projects is a blocker to this. But the rest of the code (last 4 commits) looks totally fine to me.
4f4956f to
88f8f6d
Compare
88f8f6d to
3883a66
Compare
3883a66 to
36e8672
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
23aec17 to
3dfe021
Compare
|
|
||
| const maybeSecondRequest = request.clone(); | ||
|
|
||
| let asset: "static_routing" | "ignored" | true | false = false; |
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.
Not a big fan of this variable being mutated and used in different places. Can we make this a parameter instead?
diff --git a/packages/workers-shared/router-worker/src/worker.ts b/packages/workers-shared/router-worker/src/worker.ts
index d88375ffe..9c188c36b 100644
--- a/packages/workers-shared/router-worker/src/worker.ts
+++ b/packages/workers-shared/router-worker/src/worker.ts
@@ -104,9 +104,7 @@ export default {
const maybeSecondRequest = request.clone();
- let asset: "static_routing" | "ignored" | true | false = false;
-
- const routeToUserWorker = async () => {
+ const routeToUserWorker = async ({ asset }: { asset: AssetTag }) => {
if (!config.has_user_worker) {
throw new Error(
"Fetch for user worker without having a user worker binding"
@@ -126,19 +124,19 @@ export default {
return env.JAEGER.enterSpan("dispatch_worker", async (span) => {
span.setTags({
hasUserWorker: true,
- asset: asset,
+ asset,
dispatchType: DISPATCH_TYPE.WORKER,
});
return env.USER_WORKER.fetch(maybeSecondRequest);
});
};
- const routeToAssets = async () => {
+ const routeToAssets = async ({ asset }: { asset: AssetTag }) => {
analytics.setData({ dispatchtype: DISPATCH_TYPE.ASSETS });
return await env.JAEGER.enterSpan("dispatch_assets", async (span) => {
span.setTags({
hasUserWorker: config.has_user_worker,
- asset: asset,
+ asset,
dispatchType: DISPATCH_TYPE.ASSETS,
});
@@ -160,8 +158,7 @@ export default {
analytics.setData({
staticRoutingDecision: STATIC_ROUTING_DECISION.ROUTED,
});
- asset = "static_routing";
- return await routeToAssets();
+ return await routeToAssets({ asset: "static_routing" });
}
// evaluate "include" rules
const includeRulesMatcher = generateStaticRoutingRuleMatcher(
@@ -181,8 +178,7 @@ export default {
analytics.setData({
staticRoutingDecision: STATIC_ROUTING_DECISION.ROUTED,
});
- asset = "static_routing";
- return await routeToUserWorker();
+ return await routeToUserWorker({ asset: "static_routing" });
}
analytics.setData({
@@ -195,19 +191,17 @@ export default {
// User's configuration indicates they want user-Worker to run ahead of any
// assets. Do not provide any fallback logic.
if (config.invoke_user_worker_ahead_of_assets) {
- asset = "ignored";
- return await routeToUserWorker();
+ return await routeToUserWorker({ asset: "ignored" });
}
// If we have a user-Worker, but no assets, dispatch to Worker script
const assetsExist = await env.ASSET_WORKER.unstable_canFetch(request);
- asset = assetsExist;
if (config.has_user_worker && !assetsExist) {
- return await routeToUserWorker();
+ return await routeToUserWorker({ asset: assetsExist });
}
// Otherwise, we either don't have a user worker, OR we have matching assets and should fetch from the assets binding
- return await routeToAssets();
+ return await routeToAssets({ asset: assetsExist });
} catch (err) {
if (userWorkerInvocation) {
// Don't send user Worker errors to sentry; we have no way to distinguish between
@@ -228,3 +222,5 @@ export default {
}
},
};
+
+type AssetTag = "static_routing" | "ignored" | true | false;| export const EyeballRouterConfigSchema = z.union([ | ||
| z.object({ | ||
| limitedAssetsOnly: z.boolean().optional(), | ||
| }), | ||
| z.null(), | ||
| ]); |
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.
Why support both undefined and null. I don't believe there is any semantic difference here? Can we just drop the null and simplify this and the RequiredEyeballRouterConfig type?
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.
the optional param binding which provides this param (as the EYEBALL_CONFIG binding) provides the json when present, otherwise the value is presented in the environment as null, so that union reflects that implementation
the field limitedAssetsOnly within the object is optional to be defensive, but currently the code providing the object always sets the field when providing the object. this can be changed, but i prefer being defensive, as this config may be extended and not all fields may be set in the future
|
@petebacondarwin yes, this will be observed when there is a This is still the right choice in my opinion, as implicitly disabling your compute when you trip a limit is unexpected and certainly a footgun (even if that compute is usually just returning a 404 for the fallback). And of course, setting not_found_handling to single-page-application or 404-page means we won't even hit those fallbacks, no limited behavior will be encountered. and yes, this all only applies to free tier customers. |
01fa580 to
23712be
Compare
| } from "../../utils/types"; | ||
| import type { ColoMetadata, Environment, ReadyAnalytics } from "./types"; | ||
|
|
||
| const limitedResponse = `<html> |
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.
styling and wording approved now? It's a bit of a departure from our usual styling.
GregBrimble
left a comment
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.
style! 💅
Centralizing some of these pieces will make it easier to add free tier limiting
Provided via a new optional param binding. When provided, it contains info about free tier limiting for this account.
Returns an html page (included as direct module, via esbuild) with a 429 status code, mirroring the current free tier limiting behavior. It also sets a new field in our analytics so we can observe free tier invocation denials. This page can evolve as we need it to, this is just my first pass at the response.
There's no loader configured for html files by default in wrangler it seems. So rather than mess with loaders or a custom build, inlining this page is easy enough
23712be to
a1cd3f8
Compare
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.
Since this is already in production and working. Let's land this and then we can discuss any tidy-up refactorings that I would like to see afterwards. Wrong PR sorry.
|
this PR has been superseded by cloudflare/cloudflare-docs#23008 |
WC-3584
The Router worker now limits requests when free users are over their daily limit. This decision is made via the new optional param binding (EYEBALL_ROUTER_CONFIG) that is passed in. The Router worker then denies all requests to the user worker when that param indicates limiting should occur.