-
-
Notifications
You must be signed in to change notification settings - Fork 424
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
Forwards to local services regardless of path and REST method, and can proxy them rather than redirect #354
base: master
Are you sure you want to change the base?
Conversation
add redirect with 307
add path to target
Oh, I should mention this branch is based on the nice work done by christianwerz here: #353 |
… url correctly in the case of proxying
@@ -335,10 +335,16 @@ class Group extends EventEmitter { | |||
// Make sure to send only one response | |||
const send = once(() => { | |||
let target = item.target + (item.target.endsWith('/') ? '' : '/') + path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, it seems like ideally we would just not even parse the path out from the query -- whatever it is we'd just append the whole thing. but maybe that's too much work to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's shorter codewise to do that, but I suspect a bit less clear. When we get here, we know the original target (which unfortunately excludes the query params), and the new host, so it seems clearest just to append the three together I think. Otherwise we need to take the entire raw url, overwrite the host (for the redirect case) and parse out (potentially) the service name (for both cases). But, because the hotel port is configurable, and there might be some additional mapping outside of hotel that would modify the url in an unpredictable way, it seems clearer and safer just to grab the parts we're highly confident are correct and append them into a url that should be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'm saying we should just make it so that when we get here we know the original target, including query params.
Resolves Issue #350.
I have an interest in using hotel to manage not just UI servers, but also RESTful services as well. In order to do that effectively, we need to be able to accept multiple REST methods, not just GET, and be able to serve paths beyond just the root.
Unfortunately, most clients do not deal with redirects on non-GET methods particularly well (though the change #353 by @christianwerz made to switch the redirect to a 307 does help some clients). Because of that, I've also added the ability to specify a
--forward-by-proxy
option when doing anadd
which will tell the router to proxy the request directly, rather than returning a redirect.