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.

Sign up for GitHub

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
Open

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
22737aa
allow all methods
christianwerz Feb 11, 2020
16c28c1
add wildcard for path
christianwerz Feb 11, 2020
b9266e3
Some fixes to make the tests pass and code act as expected
dbraley Feb 20, 2020
04fc52f
Add ability to forward to dev services by proxy rather than redirect
dbraley Feb 20, 2020
58778f9
Add query paramters to the url in the case of forwarding, and fix the…
dbraley Apr 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/daemon/group.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

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.

Copy link

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.

let parsedUrl = url.parse(req.url)
if (parsedUrl.search) {
target = target + parsedUrl.search
}

if (item.env && item.env.mechanism === 'proxy') {
let target = item.target + (item.target.endsWith('/') ? '' : '/') + path
// Adjusting the request is the easiest way to proxy the correct url
req.url = target
log(`Proxy - ${id} → ${target}`)
this.proxyWeb(req, res, target)
this.proxyWeb(req, res, item.target)
} else {
log(`Redirect - ${id} → ${target}`)
res.redirect(307, target)
Expand Down