- Notifications
You must be signed in to change notification settings - Fork3.1k
feat: proxy protocol support#2587
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bvli commentedJun 11, 2025
I'd like to see this feature implemented as well.. Are there any reason for not merging this PR? |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| {{- end }} | ||
| {{ template "access_log" (dict "Enable" $globals.config.enable_access_log) }} | ||
| listen {{ $globals.config.external_http_port }} {{ $default_server }}; | ||
| listen {{ $globals.config.external_http_port }} {{- $default_server }} {{- $proxy_protocol }}; |
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.
question: I'm not sure of what's get rendered here ifdefault_server andproxy_protocol both have a non blank value, aren't the two strings concatenated because of{{- ? 🤔
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.
Nope, everything looks correct because of this:https://github.com/nginx-proxy/nginx-proxy/pull/2587/files/6baf7bbf57785df273cd5ed5871937b9a0a1da2d#diff-a2a77c71854a4489d896f49c59a5f864e5b1e290f6b1ba16f9f39c1a32829c57R887
I made this change so the spacing doesn't look all wonky if neither are enabled.
Here's a test if you're curious:
- Run a proxy-protocol test case like so:
~$ make build-nginx-proxy-test-alpine build-webserver&& docker compose -f ./test/compose.base.yml -f ./test/test_proxy_protocol/test_proxy-protocol-global-enabled.yml up -d
- Exec the container and check the proxy lines look okay:
~$ dockerexec nginx-proxy nginx -T2>&1| grep'listen.*proxy_protocol' listen 80 proxy_protocol; listen 443 ssl proxy_protocol; listen 80 proxy_protocol; listen 443 ssl proxy_protocol;
- Add a default server by adding
DEFAULT_HOST: "proxy-protocol-global-enabled.nginx-proxy.tld"to thenginx-proxyservice' environment in./test/test_proxy_protocol/test_proxy-protocol-global-enabled.yml. Rerun the command in 1. - Re-grep to check the result:
~$ dockerexec nginx-proxy nginx -T2>&1| grep'listen.*proxy_protocol' listen 80 default_server proxy_protocol; listen 443 ssl default_server proxy_protocol;
- Disable proxy protocol by setting the
ENABLE_PROXY_PROTOCOLenvironment var to false in the same file above. Rerun the command in step 1 and grep fordefault_server.
~$ dockerexec nginx-proxy nginx -T2>&1| grep'listen.*default_server' listen 80 default_server; listen 443 ssl default_server;
- Finally with both disabled:
listen 80; listen 443 ssl; listen 80; listen 443 ssl;
Rerunning the steps above on main, default server disabled looks like this:
listen 80; listen 443 ssl; listen 80 ; listen 443 ssl ;And enabled like this:
listen 80 default_server; listen 443 ssl default_server;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.
So you've actually fixed the pre existing spacing issue, thank you 👍
buchdag commentedJul 26, 2025
@bvli I have close to zero practical knowledge about the proxy protocol, so it's a bit hard for me to evaluate or test this. Aside from a few minor refactor and a question (ping@antoniomika) the PR looks fine though. |
antoniomika commentedJul 26, 2025
Thanks for the review@buchdag! I addressed your comments and squashed/rebased the PR. Let me know if there's anything else I can help with. |
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Niek <100143256+SchoNie@users.noreply.github.com>
buchdag commentedJul 27, 2025
Thanks for the PR and the explanations@antoniomika 👍 |
6ebf9d6 intonginx-proxy:mainUh oh!
There was an error while loading.Please reload this page.
Similar to#1487, I took a look at adding proxy-protocol support to this project.
The changes included here add a new
ENABLE_PROXY_PROTOCOLenv var, which will enable proxy-protocol support for all docker-gen created listeners. Because settingproxy_protocolon alistenenables the functionality for allserver's that use the same port, I opted to make this option global.Unlike the other PR, I decided to map the
x-forwarded-for(andx-forwarded-port) entry to conform with current use-cases. If setting the real ip module settings is desired, there already exists methods for adding other configurations to the template.I also added tests to verify this functionalityDOES NOT break or modify any of the current proxy header use cases. This feature is opt-in, as nginx will stop responding to HTTP requests that aren't made using the proxy protocol.