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

Forwards to local services regardless of path and REST method, and can proxy them rather than redirect #354

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dbraley
Copy link

@dbraley dbraley commented Feb 20, 2020

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 an add which will tell the router to proxy the request directly, rather than returning a redirect.

@dbraley
Copy link
Author

dbraley commented Feb 20, 2020

Oh, I should mention this branch is based on the nice work done by christianwerz here: #353

@dbraley dbraley requested a review from typicode February 20, 2020 16:42
@@ -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

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.

Copy link
Author

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.

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.

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.

3 participants