-
-
Notifications
You must be signed in to change notification settings - Fork 104
[feature] Add support for non-default Nginx external ports #497
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
Conversation
E.g. for installations running behind a reverse proxy
nemesifier
left a comment
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.
Thanks for contributing @c-gabri, see my comments below.
|
@nemesifier My pleasure, thank you for this awesome software: gives me headaches sometimes and there's certainly room for improvement, but overall it is a huge help for managing my production network, could not live without it any more. Glad to help a little bit where I can. |
|
@nemesifier Not sure if I should do something about Merge Tests/CI Build failing: I've managed to properly format changed files, but I see it's now complaining about commit messages, which I'm not sure if/how I can change. Sorry but I'm not a git pro and this is one of my first pull requests. Let me know if there's something I should do. |
It's been improving, thanks also to feedback and contributions like yours.
I'll try to build locally, alpine very often yanks packages which contain any known security vulnerability and for this reason very often our builds fail (most likely there's a new version of one of the system packages). Unfortunately dependabot cannot handle this automatically for us yet so it's a bit of a drag that we have to deal with. |
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.
Here's what I found in my round of testing.
After setting this in my .env:
NGINX_SSL_PORT=4443
NGINX_PORT=8080
Redirect from non SSL to SSL returns 404
Normally, if one opens the non SSL URL, it will redirect to the SSL URL:
| return 301 https://${DOLLAR}host${DOLLAR}request_uri; |
Due to the fact that SSL is now on port 4443, this won't work.
HTTP requests to the API container fail
The application is not aware that it should send requests to :4443.
Conclusions
Does it work for you? If the solution you shared here does, you can simply edit your docker-compose file or whatever other methodology you're using, without the need of sending a PR.
However, if you care about getting this included we need to make sure the rest of the application knows where to point to and will require more work.
Works for me because I don't connect directly but through a reverse proxy (it is the main use case for this PR), which takes care of
But yes, if we want to make this feature more generic, i.e. make custom ports work regardless of the use with a reverse proxy, then more work is needed to make the application aware of the custom ports. I like the idea, if you do too I can convert this to a draft and look into this a little bit in my spare time. Or we can merge now and I create a new issue/PR for the more generic use case. Your choice. |
|
@nemesifier Not sure if I've completely cracked this but I have definitely made progress.
What I'm testing, while verifying no unusual errors appear in the web console and that things look as they should:
Not sure what more comprehensive tests should be run to check if the new code covers all possible cases. I've just made the bare minimum additions/edits to fix the errors that would appear in front of me during tests using a bit of common sense. Take it for a spin and let me know how it goes ;) |
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.
Thanks for following up, I'll test asap.
PS: I added this to the 25.09 release board.
Help from anyone is welcome in testing/reviewing PRs on that board to speed up the next releas (not many blockers left to solve).
nemesifier
left a comment
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.
I tested this and it works. Thank you @c-gabri! 👏
Checklist
Reference to Existing Issue
Closes #496.
Description of Changes
Let users set custom external HTTPS/HTTP ports for the
nginxcontainer, using new, documented.envvariablesNGINX_SSL_PORTandNGINX_PORT. Useful if e.g. OpenWISP is running behind a reverse proxy on the same host (quite common for Docker applications).Screenshot
Please include any relevant screenshots.