Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Reverse proxy dynamic subpath #540

Merged
merged 40 commits into from Jun 8, 2020

Conversation

fabio-torchetti
Copy link
Contributor

What is it?

This is a PR to allow dynamic changing of the subpath the server is running behind. The idea is to allow the server to adapt to being behind a proxy that changes the root path.

How?

The main index.html page is rendered on the fly taking into consideration the path the page is being served from. This can be determined in two ways:

1 - A X-External-Path header is passed along with the request
2 - A base is specified in the application configuration

The api base and socket.io path is calculated based on the document baseURI is set in the index.html base tag.

Why?

I am planning to have ZWave To MQTT run behind the ingress of Home Assistant. I really didn't like the option to expose it directly under a new domain. So, in the spirit of the Open Source "If you don't like it, fix it!" this PR came to be.

This related to #317

@fabio-torchetti
Copy link
Contributor Author

Working using Ingress

Screen Shot 2020-06-01 at 07 36 36

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@robertsLando
Copy link
Member

I would like that @chrisns reviews this too before the merge

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
build/webpack.dev.conf.js Outdated Show resolved Hide resolved
docs/subpath.md Outdated Show resolved Hide resolved
app.js Show resolved Hide resolved
docs/subpath.md Outdated Show resolved Hide resolved
docs/subpath.md Outdated Show resolved Hide resolved
src/apis/ConfigApis.js Outdated Show resolved Hide resolved
src/apis/ConfigApis.js Outdated Show resolved Hide resolved
test/lib/renderIndex.test.js Show resolved Hide resolved
views/index.ejs Outdated Show resolved Hide resolved
@fabio-torchetti
Copy link
Contributor Author

@chrisns I prefer to wait for #549 to be merged before this and the logo thing to be fixed

I think it's a good idea. I have fixed the logo during development testing, added a more complete example of nginx configuration on a subpath -- it's the configuration I use to test working behind a reverse proxy.

I have added a development dependency - proxyquire - that allows to test more static classes, like the webConfig. I checked the audit and it does not have any outstanding security issues.

I also added the proxy settings for webpack's webserver, this allows both hot reloading while using a running webserver. This is currently assuming that the server is running locally on port 8091. IE: you should be able to run npm run dev:server & then npm run dev and end up with a hot reloading web interface on :8902.

Give it a try and let me know what you think.

@robertsLando
Copy link
Member

Grazie Fabio 😄

I will try it now give me a moment

@robertsLando
Copy link
Member

@fabio-torchetti When running npm run dev command and going to the ui seems all api calls are failing

src/apis/ConfigApis.js Outdated Show resolved Hide resolved
@chrisns
Copy link
Member

chrisns commented Jun 4, 2020

given we've used rewire elsewhere and making that play nice with proxyquire from research is hard had you considered rewiremock?

@chrisns
Copy link
Member

chrisns commented Jun 4, 2020

given we've used rewire elsewhere and making that play nice with proxyquire from research is hard had you considered rewiremock?

please ignore ^^ comment

see thlorenz/proxyquire#90 (comment)

I'd only introduced rewire really to allow some instrumentation to be able to then write tests without changing the code, its a gross hack and can hopefully be removed at some point in the future.

@robertsLando robertsLando self-requested a review June 5, 2020 11:51
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just resolve conflicts then this can be merged @fabio-torchetti . Thanks ! 😃

@robertsLando
Copy link
Member

@fabio-torchetti I saw your latest commit, if you use vscode you should have it auto running on save :)

@robertsLando robertsLando requested a review from chrisns June 5, 2020 13:51
.vscode/settings.json Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
build/webpack.dev.conf.js Outdated Show resolved Hide resolved
config/index.js Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
test/config/webConfig.test.js Show resolved Hide resolved
test/lib/renderIndex.test.js Outdated Show resolved Hide resolved
fabio-torchetti and others added 5 commits June 5, 2020 19:06
Co-authored-by: Chris Nesbitt-Smith <chris@cns.me.uk>
Co-authored-by: Chris Nesbitt-Smith <chris@cns.me.uk>
Co-authored-by: Chris Nesbitt-Smith <chris@cns.me.uk>
Co-authored-by: Chris Nesbitt-Smith <chris@cns.me.uk>
@robertsLando robertsLando requested a review from chrisns June 8, 2020 07:09
@robertsLando
Copy link
Member

OK let's merge this. Thanks again @fabio-torchetti 🎉

@robertsLando robertsLando merged commit b10e8c9 into OpenZWave:master Jun 8, 2020
@chrisns
Copy link
Member

chrisns commented Jun 8, 2020

thanks for bearing with us @fabio-torchetti that was a change required for what should have been way simpler

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants