-
Notifications
You must be signed in to change notification settings - Fork 53
feat: add support for skew protection #746
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
Conversation
🦋 Changeset detectedLatest commit: 17064f4 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 |
commit: |
2356748
to
0a6f2dd
Compare
0a6f2dd
to
18ba05a
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.
I just took a really quick look for now, will make a proper review later
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 this is looking pretty good on my initial read though. Going to have another look later on as well.
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 this looks good and the logic makes sense to me.
It might be nice for us to look at having an e2e that runs against a deployment at some point in the future, for testing features like this.
4c21542
to
471111c
Compare
packages/cloudflare/src/cli/build/open-next/compile-skew-protection.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/cli/build/open-next/compile-skew-protection.ts
Outdated
Show resolved
Hide resolved
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.
LGTM
Thanks for the review @sommeeeer ! |
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.
Just a few nits/question, other than that LGTM
packages/cloudflare/src/cli/build/open-next/compile-skew-protection.spec.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/cli/build/open-next/compile-skew-protection.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/cli/build/open-next/compile-skew-protection.ts
Outdated
Show resolved
Hide resolved
* @param options Options to pass to `getPlatformProxy`, i.e. to set the environment | ||
* @returns the env vars | ||
*/ | ||
export async function getEnvFromPlatformProxy(options: GetPlatformProxyOptions) { |
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 we should have only one instance of this, that we spawn at the beginning and that we dispose at the end. It's a bit wasteful to launch multiple workerd process.
And once we have support for remote bindings here, we could reuse it to populate the cache directly (which should be way faster)
path = path.slice(basePath.length); | ||
} | ||
if (path.startsWith("/_next/static/") || isFileInTree(path, __CF_ASSETS_TREE__)) { | ||
return assets.fetch(request); |
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.
Does this one respect the _headers
file ?
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.
It should but I'll test that
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.
Actually it doesn't, see https://developers.cloudflare.com/workers/static-assets/headers/#custom-headers
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.
Follow up:
- Check what Next does add by default and apply that in the asset resolver
- Document how to override (using middleware or Next config)
2f1f47b
to
45edc9f
Compare
43be1dd
to
12c4094
Compare
12c4094
to
637d8f4
Compare
Co-authored-by: conico974 <[email protected]>
637d8f4
to
ca41ac2
Compare
I have rebase the PR on top of #768 as the asset resolver will be used to resolve the assets from previous versions (when Docs plan (today/tomorrow)
@dario-piotrowicz @james-elicx @conico974 please take a look at the PR when you get a chance. Thanks! |
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.
Just a few nits/question, but other than that LGTM.
BTW do we want to allow users to override the assets resolver ?
@@ -4,5 +4,7 @@ | |||
export const dynamic = "force-dynamic"; | |||
|
|||
export async function GET() { | |||
return new Response(JSON.stringify(process.env)); | |||
return new Response(JSON.stringify(process.env, null, 2), { |
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 this change ?
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 used this API for debugging (the env contains the build id, deployment id, and deployment mapping) and the response was easier to read when formatted this way.
|
||
const now = Date.now(); | ||
|
||
client.workers.scripts.versions.list = vi.fn().mockReturnValue([ |
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.
Can the client return this in the wrong order ? This is not tested 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.
In my experience, the client always returns an ordered lists.
BUT I agree we should not rely on the result of a few experiments.
That's why I pay special attention to not order the input (~2 is before ~1) and you can see that the output of listWorkerVersions
is ordered (~2 after ~1)
Co-authored-by: conico974 <[email protected]>
Yes we should but I agree that our current export default {
...defineCloudflareConfig({
incrementalCache: r2IncrementalCache,
}),
cloudflare: {
skewProtection: {
enabled: false,
},
},
} satisfies OpenNextConfig; As of today users can override the assets resolver but the syntax is not great. I'll try to work on a better way to do that. |
There are 2 main files in this PR:
packages/cloudflare/src/cli/commands/skew-protection.ts
(build time)This builds a mapping from
deploymentId
to the worker version at build time.Note that because the worker you are building has no version yet, "current" is used instead.
packages/cloudflare/src/cli/templates/skew-protection.ts
(runtime)At runtime, if a particular version is requested and it is present in the mapping,
we'll fetch the result from a preview URL with a hostname of
<version>-<worker_name>.<domain>.workers.dev
How to use it:
cloudflare.skewProtection.enabled
totrue
in your OpenNext configpackages/cloudflare/src/api/cloudflare-context.ts
deployementId
- you can use thegetDeploymentId()
helperrun_worker_first
totrue
Docs PR opennextjs/docs#164
TODO: