search

nodejs / release-cloudflare-worker

Infra for serving Node.js downloads and documentation.
https://nodejs.org/dist
MIT License
22 stars 5 forks source link

consider adding rewrite rules for docs and latest versions #33

Closed MoLow closed 1 year ago

MoLow commented 1 year ago

when promoting a release, symbolic links are automatically added for documentation and for release/latest using https://github.com/nodejs/nodejs-latest-linker:

https://github.com/nodejs/build/blob/0a1b2e9ad35ba7aab215e799529781a3522e00ce/ansible/www-standalone/tools/promote/promote_release.sh#L39

for example:

it might make sense to add these rules directly to the Cloudflare worker to avoid the need to copy duplicate files when promoting a release

jbergstroem commented 1 year ago

As we publish a new version of node, we could call a github action that updates the redirect config? https://developers.cloudflare.com/rules/url-forwarding/bulk-redirects/reference/json-objects/

Edit: ..or a rewrite url: https://developers.cloudflare.com/rules/transform/url-rewrite/create-api/

flakey5 commented 1 year ago

we could call a github action that updates the redirect config?

I think either of those approaches are worth a shot and would be a nice way to do it but I'm not sure if they'd work for a worker.

If they don't we might need to try something like a bot or Github action that keeps a file named something like symlinkRedirects.ts up to date with the latest paths described like

export default {
  '/download/release/latest/': '/download/release/v20.7.0',
  '/dist/latest/': '/dist/v20.7.0',
  // ...
}

And then when we get a HTTP GET we can do something like

if (requestUrl.pathname in symlinkRedirects) {
  return Response.redirect(symlinkRedirects[requestUrl.pathname])
}

That would add the need for human action to approve the change, merge it, and then deploy it to prod however.

We might be able to get away with having the redirects in the environment variables of the worker so then the CI would just need to overwrite them via the wrangler cli but not sure if that would work. We would need to make sure to not overwrite the values that Cloudflare has by not setting them in the wrangler.toml

jbergstroem commented 1 year ago

I think either of those approaches are worth a shot and would be a nice way to do it but I'm not sure if they'd work for a worker.

Right - both those approaches (redirect, rewrite) are processed before the request arrives at the worker. They are both more performant and invokes less worker cpu time.

flakey5 commented 1 year ago

Right - both those approaches (redirect, rewrite) are processed before the request arrives at the worker

In that case then yeah I think that's the way to go.

Also apologies for the edit, was on mobile and hit the wrong button without realizing

MoLow commented 1 year ago

We now have (with https://github.com/nodejs/release-cloudflare-worker/pull/34 and https://github.com/nodejs/release-cloudflare-worker/pull/38) a list of redirects inside this repo, we should find the best way to use that.

flakey5 commented 1 year ago

I'd assume that updating the redirect config @jbergstroem pointed out could be done with another js file that reads the json file made by the script in #34 and interacts with Cloudflare's v4 api.

I wonder if we could add another action that fires only on pr merge and only if the json file gets updated. So for example whenever we merge a pr like #36, it'd fire and call the script to update the redirect config. Would require some more human input though to merge the pr unless we'd like setup something to auto-merge them. Regardless production releases are still manual

MoLow commented 1 year ago

Iv opened a PR handling this.

I wonder if we could add another action that fires only on pr merge and only if the json file gets updated. So for example whenever we merge a pr like https://github.com/nodejs/release-cloudflare-worker/pull/36, it'd fire and call the script to update the redirect config. Would require some more human input though to merge the pr unless we'd like setup something to auto-merge them. Regardless production releases are still manual

I'm not sure what you mean, but what I'v had in mind is adding a step in the process of a release, manually deploying the worker https://github.com/nodejs/node/blob/092fb9f541ce8cc07289b5a69eb93892445739f5/doc/contributing/releases.md?plain=1#L1127

flakey5 commented 1 year ago

I'm not sure what you mean

I was referring to this comment on doing the redirects (or a rewrite if possible) via Cloudflare rules. By doing so we'd avoid the need to handle it in the worker

flakey5 commented 1 year ago

Thought about it a bit more and curious as to if the Cloudflare rules approach would overlap with nodejs/build#3270, I think so since it's modifying config. Wonder if we should go with handling the redirects in the worker (#39) and then revisit the Cloudflare rules approach sometime in the future after Terraform is fully integrated to see if it'd be worth moving over?

MoLow commented 1 year ago

I think https://github.com/nodejs/build/issues/3270 is going to take some while before it happens. additionally, the amount of rules is > 700 that update on each release. this makes converting them into Cloudflare config much more complicated. even if we convert the rules into wildcard rules they will be pretty complex and won't follow the exact logic of https://github.com/nodejs/nodejs-latest-linker. so for me its a +1 on redirecting inside the worker