Skip to content
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

consider adding rewrite rules for docs and latest versions #33

Closed
MoLow opened this issue Sep 28, 2023 · 10 comments · Fixed by #39
Closed

consider adding rewrite rules for docs and latest versions #33

MoLow opened this issue Sep 28, 2023 · 10 comments · Fixed by #39

Comments

@MoLow
Copy link
Member

MoLow commented Sep 28, 2023

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
Copy link
Member

jbergstroem commented Sep 28, 2023

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
Copy link
Member

flakey5 commented Sep 29, 2023

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
Copy link
Member

jbergstroem commented Sep 29, 2023

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
Copy link
Member

flakey5 commented Sep 29, 2023

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
Copy link
Member Author

MoLow commented Sep 30, 2023

We now have (with #34 and #38) a list of redirects inside this repo, we should find the best way to use that.

@flakey5
Copy link
Member

flakey5 commented Sep 30, 2023

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
Copy link
Member Author

MoLow commented Sep 30, 2023

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 #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
Copy link
Member

flakey5 commented Oct 1, 2023

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
Copy link
Member

flakey5 commented Oct 2, 2023

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
Copy link
Member Author

MoLow commented Oct 2, 2023

I think nodejs/build#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

@MoLow MoLow closed this as completed in #39 Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants