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

Lambda for extensible redirect / rewrite rules #61

Closed
wants to merge 2 commits into from
Closed

Lambda for extensible redirect / rewrite rules #61

wants to merge 2 commits into from

Conversation

nvnivs
Copy link
Collaborator

@nvnivs nvnivs commented Apr 26, 2022

@petewilcock this is a very early draft/wip but wanted to run in past you to see what you feel about it.

Use case

As a user when migrating over from an existing website, redirecting links from old page to new pages should be supported.

I thought that the CloudFront Lambda could be exteded to support this use case.

Added a new lambda_rewrite to support the existing rules plus custom ones that can be passed on a list on var.rewrite_rules.

The code for the Lambda is based on this https://github.com/marksteele/edge-rewrite. The repo has been inactive for a few years so i thought that was not worth the work of doing some clever importing.

Done some early basic testing and these 2 scenarios seem to be working:

  • Rewriting / urls to /index.html
  • 301 Redirecting /my-old-url to /shiny-new-url

Potentially this could help with #43 as well, haven't tested that scenario yet but should support host header based matches.

Let me know your thoughts, and sorry for the huge amount of files in the PR, most are the node_modules with the dependencies 😞

@petewilcock
Copy link
Contributor

I like the idea of passing in custom redirect rules, although a PR with these diffs is unreviewable 😅. I believe the proper thing with such a package would be to bundle it as a zip from which a lambda layer is created and associated with the function. That way you get the functionality, neat zip, no review death.

However, there is #57 which would actually remove Lambda@Edge in favour of CloudFront Functions. CloudFront Fucntions only support a Javascript runtime, and we'd need to switch where the redirect happens from the origin-request stage (after CloudFront receives the request, before it reaches the origin), to the viewer-request stage, but it should work.

This also fixes another issue where you can't seem to cleanly delete a Lambda@Edge function because of the way it's replicated to CloudFront Edge caches, which can make a terraform destroy of the module impossible without manual intervention in the console.

So overall, great idea, but going forward I think it's the CloudFront function + JS route, and the function will be tiny, faster, and executions still included in the free tier. Code e.g. here: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/example-function-add-index.html

@nvnivs
Copy link
Collaborator Author

nvnivs commented Apr 27, 2022

Thanks for the feedback @petewilcock !

I was aware of #57, in fact i was planning to have a go at it as part of this.

The reason why this PR didn't bundle the Lambda on a zip is because the rules are being put on a static file on the lambda with this:

resource "local_file" "rules" {
  content   = jsonencode(var.rewrite_rules)
  filename = "${path.module}/lambda_rewrite/rules.json"
}

So the zip is being done at deploy time, hence why all the node_modules on the PR.

I was planning to write the rules somewhere else (S3?) so the lambda could be pre-packaged, but my understanding is that CloudFront functions have no network or disk access, which throws that plan down the can.

So another possibility would use a var to allow a file path to be passed to a custom source of the lambda, to enable users to define their own rewrite / redirect rule, what do you think about that?

@petewilcock
Copy link
Contributor

I think we could simplify this if we build the lambda (or CF) function from a Terraform template file which would loop over a list of maps containing two elements: the regex to match, and regex to rewrite to. That way TF would render out the full function file with 1 to n rewrite rules and create it without having to bundle some external file of rules (which I suspect users would find a bit clunky).

I'm just having a very manic 2-week period so I don't have too much bandwidth to focus on the module, but I'll be getting back to it some time in the middle of May 😓

@nvnivs
Copy link
Collaborator Author

nvnivs commented Apr 28, 2022

The reason i was thinking about a custom file is because rewrite rules can be quite complex with rewrites, redirects, forbidden, based on host headers, query string etc which will be hard to acomplish with through a map.

But all that sounds both clunky and overkill for what the goal is here, which is just to support doing 301 redirects for URL's that changed.

The template sounds much cleaner, going to implement that 👍

Already got the changes to migrate from Lambda@Edge to CloudFront Functions, will raise a new PR and close this one when ready.

Nevermind my noise, look at it whenever you have availability. Your feedback is appreciated as i rather do something that is of general use then just addressing my issue.

@nvnivs
Copy link
Collaborator Author

nvnivs commented Apr 28, 2022

Closing this in favour of the much easier to digest #62

@nvnivs nvnivs closed this Apr 28, 2022
@nvnivs nvnivs deleted the custom_redirect branch April 28, 2022 12:51
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 this pull request may close these issues.

2 participants