-
Notifications
You must be signed in to change notification settings - Fork 442
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
Make ADDRESS option optional, determined automatically if not provided. #119
base: master
Are you sure you want to change the base?
Conversation
Resolves #116 and #117 (edit: and now #130) #118 submitted by @nathanparekh does a good job of improving documentation but would need to be tweaked a bit if this PR is also accepted. |
I have tested this extensively, in Heroku only. Heroku is now back to deploying simply and working straight from the 'Deploy to Heroku' button only. The I made a couple of 'executive decisions' in implementation, feedback welcome. |
@jgroffen - Overall looks really good. The only thing that stuck out to me while trying it out with manual and using Docker was that something like With regards to #118, I'd be glad to update the instructions again if/when this gets merged or even write something up to include as part of this. |
Hello @nathanparekh , Thanks for testing it for me with Docker! Docker does it's port forwarding using TCP (at network level 4) - the mechanism I'm using relies on being told at the HTTP level (network level 7) in HTTP Response headers. So a Docker-only deploy unfortunately needs ADDRESS to be set properly (a good reason for your patch that updates documentation) - but if there is a web proxy in-front of the application (Docker or otherwise) that is being a good citizen and telling the proxied web server useful information like what the original host, port and protocol are then this patch will make CrewLink-server take advantage of that info. This patch works for Heroku and most likely AWS, Azure, etc where an L7 load balancer (essentially a reverse proxy) is routing traffic to the app. |
The ADDRESS env variable is used for advertising the address of the service only. If it is not provided, it can be determined automatically using the HTTP Request headers 'Host' and 'X-Forwarded-*'. This approach uses ExpressJS provided functionality but requires 'trusting' headers forwarded from proxies. Deployers can control the trusted proxies using the TRUST_PROXY option, documented in the code only as it's very edge-case. This approach supports the best of both worlds; ADDRESS will use a logical (probably accurate) value by default, but can be overridden in the case of unusual proxy scenario or a canonical URL is preferred.
I have rebased back on to master to resolve the merge conflict and support the v2.0.0/v2.0.1 release. |
The ADDRESS env variable is used for advertising the address of the service
only. If it is not provided, it can be determined automatically using the
HTTP Request headers 'Host' and 'X-Forwarded-*'. This approach uses ExpressJS
provided functionality but requires 'trusting' headers forwarded from proxies.
Deployers can control the trusted proxies using the TRUST_PROXY option,
documented in the code only as it's very edge-case.
This approach supports the best of both worlds; ADDRESS will use a logical
(probably accurate) value by default, but can be overridden in the case of
an unusual proxy scenario, or a canonical URL is preferred, or as mentioned by
@nathanparekh when no forwarding proxy is involved such as using Docker
port forwarding only.
Closes #116, #117, and #130
Affects #118 (documentation updates will need some rewording)