Website Routing Rules#490
Conversation
I don't think we can support any other error code handling without somehow hooking into the errors that are being thrown in earlier middleware. Too hard bucket?
|
I think this is good to go now. I've implemented all the variations of Conditions and Redirect rules I can see in the docs, and they're thoroughly tested along with the config validation. The only thing I haven't added support for is If you have time, a review would be great. |
|
Wow, thanks so much for this PR! I love the approach, these changes look like they'll fit right in. I'll have to take a closer look this weekend. It looks like you were diligent with all the test cases, so as long as they look right to me I think we should be able to merge this quickly. |
|
Not a problem! Thanks for the project :) I've just pushed another commit because I realised elements that contain XML entities weren't being decoded, which messes things up if you're rewriting URLs to query strings. I added a new test case to cover this. |
jagregory
force-pushed
the
website-routing-rules
branch
from
dbb3a31 to
2a3b792
Compare
Needed for WebsiteConfiguration where you're doing a ReplaceKeyPrefixWith to something that puts the Key into a QueryString. e.g. `/thumbnail/image.png` is transformed to `/resize?size=thumbnail&key=image.png` The & in the query strings have to be encoded in the website config, and without this change they stay encoded and break things.
jagregory
force-pushed
the
website-routing-rules
branch
from
2a3b792 to
633a9e4
Compare
kherock
left a comment
kherock
left a comment
There was a problem hiding this comment.
@jagregory Thanks again for the quality PR. I feel like everything is actually already in place to check off the last subtask for HttpErrorCodeReturnedEquals, but let me know if I'm missing something.
|
|
||
| if (config.routingRules) { | ||
| for (const routingRule of config.routingRules) { | ||
| if (routingRule.shouldRedirect(key, 404)) { |
There was a problem hiding this comment.
Can you add a comment here describing that we're deviating from actual S3 behavior by only handling 404s?
Also, is there any reason you can't attempt to read err.status || 500 here? The existing implementation was initially written to only add special handling for the NoSuchKey error, but in this case, I think it should be possible to perform this routing rule check before the NoSuchKey check on L81, grabbing whatever error code is set.
There was a problem hiding this comment.
I attempted this before, because I agree it should be possible; however, I couldn't get a testcase working to simulate the behaviour. I can give it another shot, but if you have any tips on how to get the server into a state where it'll respond with a different status code that'd be very helpful.
There was a problem hiding this comment.
After a bit more experimenting with S3, I still can’t find an actual scenario where I can get it to redirect with anything other than a 404. S3 doesn’t do blanket redirections, I assume depending on where the error occurs will decide whether the routing rules kick in.
- S3 responds with a 403 when a file isn’t public accessible = No redirect
- S3 responds with a 405 when you try accessing a key with an unexpected method (e.g. a PUT) = No redirect
I’m a bit stumped. I can’t find any examples of people doing anything other than 404.
So I'm a bit wary of allowing any error to trigger a redirect in s3rver, as that's not the same behaviour as S3, but it's kinda hard to determine what actually is the expected behaviour! I'll add a comment for now.
There was a problem hiding this comment.
More experimenting yielded: The provided HTTP error code (200) is not valid. Valid codes are 4XX or 5XX, so I'll add this as an extra validation check too.
|
@kherock I think this is probably as good as it's going to get right now, is there anything else outstanding to get this merged? Thanks! |
|
apologies, I was on vacation last week and forgot to take care of this. The changes look great, I want to take care of a couple other issues first, but expect a release soon! |
|
Thanks! |
|
v3.3.0 is released. Thank you for contributing! |
2 participants
Hey, I need support for routing rules for my setup so I figured I'd give it a shot at implementing them. This is a work in progress, but I figured I'd open the PR early.
The example I have implemented so far is what I personally need, but it isn't the simplest scenario. I wanted to prove out that I could get this working for my case before implementing anything else. I only have one test at the moment, but I'll expand them as I go.
Any feedback is welcome. I'll update this PR as I progress.
TODO
KeyPrefixEqualsonly conditionalHttpErrorCodeReturnedEqualsonly conditionalKeyPrefixEqualsandHttpErrorCodeReturnedEqualsconditonalHostNamevalueProtocolvalueReplaceKeyPrefixWithvalueReplaceKeyWithvalueHttpRedirectCodeHttpErrorCodeReturnedEqualsfor a non-404 status code