rewrite: Add client-side redirect support

[astei]

1. What does this change do, exactly?

This adds a new "redirect" directive to the complex rewrite directive. Providing an HTTP response code will force Caddy to issue an HTTP redirect instead of internally rewriting the request.

2. Please link to the relevant issues.

#1749, #726, #856

3. Which documentation changes (if any) need to be made because of this PR?

Information about the new redirect directive should be added to https://caddyserver.com/docs/rewrite.

4. Checklist

• I have written tests and verified that they fail without my change
• I have squashed any insignificant commits
• This change has comments for package types, values, functions, and non-obvious lines of code
• I am willing to help maintain this change if there are issues with it later

[diazona]

That was quick! Thanks :-) Before the people "in charge" get in here, I noticed a few things that might be worth thinking about:

• If rewrite can now issue client-side redirects, does redir provide any functionality that rewrite (with redirect) doesn't? (Other than backwards compatibility, of course)
• Should it be prohibited to have more than one redirect directive in a given rewrite block? I don't read Go so I could be wrong, but I don't see that the code checks for this.
• Will the redirect directive only work when a regular expression is provided for the rewrite, or can it apply to string-match rules too? If it only works with regexes, I'd consider that kind of unintuitive.

I don't intend any of this as negative criticism, just things that I think are worth having thought about before the change is finalized.

[astei]

@diazona

• redir can still serve as a convenient one-line directive for quick redirects.
• If more than one redirect directive is included, it'll simply use the last one it finds. (Even then, why would you use multiple redirect directives?)
• It will work not just for regex matches - you can use this for if matches as well, but this works best for regexp rewrites.

[mholt]

Hey Andrew, thanks so much for taking the time to implement this. I have a few reservations, but nothing we can't work through.

I really don't believe the rewrite middleware should be issuing redirects. This behavior is confusing. Do we just need regular expression matching for redir instead? If so, we could consider having redir and rewrite share a large chunk of the parsing code so that they both have similar functionality.

[astei]

@mholt

I chose to implement this as a part of the rewrite directive as I felt it was the least disruptive approach I could take.

I wouldn't have an issue with refactoring redir and rewrite to share the vast majority of their code when I can find the time.

[mholt]

Okay. If my understanding is correct that all redir needs is support for regular expressions, I think we need to go that route to avoid weird bugs and confusing behavior. Rewrite for internal rewrites, redir for external redirects. Maybe all we need to do is share the regexp- and if-related code.

Want me to leave this PR open in the meantime?

[diazona]

Okay. If my understanding is correct that all redir needs is support for regular expressions, I think we need to go that route to avoid weird bugs and confusing behavior. Rewrite for internal rewrites, redir for external redirects. Maybe all we need to do is share the regexp- and if-related code.

Indeed, what I had in mind when proposing the feature request was to either give redir support for regular expressions, or consolidate both redir and rewrite into one directive that would handle both internal and external redirects. I left it open-ended because I wasn't sure how you'd prefer to have this implemented.

Sharing the request-matching code (regexp and conditionals) between rewrite and redir would be exactly in line with what I'd be looking for. I do think it makes sense, in general, to give the two directives equivalent power to select which requests they apply to.

[astei]

@mholt I'll probably open another PR for this. For the moment, I'd like it if this PR stayed open until I can open a new one.

[mholt]

Sounds good, consider this your active TODO item. Thanks for tackling this!! ❤️

[astei]

Going to check in:

I've been busy with other things, which has understandingly limited my ability to work on this. I will hopefully be able to start working on the refactor this week.

[mholt]

Thanks for the check-in. We appreciate it! (Likewise, I have a state holiday tomorrow so I'm off. 😄)

[mholt]

Hey @astei - just checking in. Let us know if you need help and would like someone else to finish this up...

[astei]

I have moved into my dorm to start classes in a few days and don't have my computer yet. I will have it tomorrow.On Aug 4, 2017 9:03 PM, Matt Holt <notifications@github.com> wrote:Hey @astei - just checking in. Let us know if you need help and would like someone else to finish this up... —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

[mholt]

Ah, I know how that goes. Good luck with the transition!

[astei]

I finally have a little time to work on this.

[mholt]

Great 😄 Looking forward to it.

[mholt]

I think it's been long enough that perhaps we should close this for now. Much has changed in the last few months, including #1948, which might alleviate the need for these changes. Feel free to comment if this PR needs further discussion or another look, just closing to clear out my TODO list. Thanks!