-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add support for proxy_protocol in proxy_hosts and streams #4105
base: develop
Are you sure you want to change the base?
Add support for proxy_protocol in proxy_hosts and streams #4105
Conversation
fc6a313
to
2916ff3
Compare
Closes NginxProxyManager#1114 Related To NginxProxyManager#1882 Related To NginxProxyManager#3537 Related To NginxProxyManager#3618 Co-authored-by: jwklijnsma <[email protected]> Co-authored-by: SBado <[email protected]>
2916ff3
to
480c772
Compare
Docker Image for build 4 is available on Note: ensure you backup your NPM instance before testing this image! Especially if there are database changes |
@jc21 What does the requires-verification label mean? Is it that somebody should test this version manually by setting their tag to If so I am currently running this image an tested a few of the functions (both streams and proxy hosts work with and without the proxy procotol. Editing existing proxy_host and streams works (something that does need automated tests, because I only noticed my incomplete implementation when testing manually, not in the cypress tests/CI - but that is for another time). Anyway, you probably shouldn't rely on my word alone, so maybe somebody else could verify my changes :) |
You're correct, in the absence of automated tests I do ask the community to verify changes, and I don't expect anyone to write those tests. Also I need to manually test things myself and it's hard to find enough time to go through all of the PR's but over Xmas I intend to. |
Hi @snordmann Gave it a try pulled the image First thing is that i cannot update existing settings. I'm getting the following js popup Also if created from scratch with proxy_protocol, its always offline Otherwise saw no regression in terms of other functions. |
I've been using the nginxproxymanager/nginx-proxy-manager-dev:pr-4105 image successfully for several months without any issues. I have both hosts using with and without Proxy Protocol enabled. I'm hoping this PR can be merged soon so I can benefit from other changes being added to the main release. Let me know if there's any information that I can provide that will help with verification. |
Hi, I am very interested in this PR! I have setup a second NPM instance using the PR and it works! My setup is:
Testing with a small whoami service, I can now see the external IP on my server, whereas before it showed me the IP of the gateway. One thing I haven't quite figured out yet: While at home, I have a DNS redirect so that any request from a device in my LAN goes directly to the home-server, without taking the route through the gateway. Without internal redirect: Unfortunately, the services configured in NPM to accept the proxy protocol now won't load at all, because I don't go through a proxy which uses the protocol. Can NPM accept proxied and un-proxied traffic at the same time? Or do I have to configure a second host for every service such that it can be also reached internally? One workaround idea I have: setup another internal gateway which also uses the proxy protocol. But that does seems a bit over the top. |
Closes #1114
Related To #1882
Related To #3537
Related To #3618