- Notifications
You must be signed in to change notification settings - Fork3.2k
PROXY Protocol support (rd 2)#4505
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
base:develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ger:develop into develop
…oxy-manager into develop# Conflicts:#backend/schema/endpoints/proxy-hosts.json#backend/templates/_listen.conf
cc@Iaotle |
CI Error: |
Seems like this was a timing issue with the CI during the apt update |
code-a-cola commentedApr 28, 2025
Having the same issue on my pull request. |
beatles1 commentedJun 11, 2025
What's the difference between this and#4105 ? This functionality would be really useful for me and just wondering which would be better to help testing for? |
@beatles1 the only difference between this one and the one you linked is this MR layers in a commit on top that resolves the merge conflicts with upstream. Merging this MR should also mark the other one merged. |
beatles1 commentedJun 13, 2025
Awesome, hopefully the CI can be re-run and then I'll test the Docker image |
Iaotle commentedJun 13, 2025
@adrum I feel kinda bad for rushing you to re-make the PR, you put in effort fixing merge conflicts and submitting a PR just to end up in limbo </3 I admire the effort though! |
AlperShal commentedJun 14, 2025
@jc21 Really sorry for tagging you but looks like this is failing because of a CI issue. And the bot didn't post the details about why CI failed after the build bump. Can you please investigate this? |
| "enable_proxy_protocol": { | ||
| "description": "Enable PROXY Protocol support", | ||
| "example": true, | ||
| "type": "boolean" |
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.
CI is failing because of this.
Endpoint: POST /nginx/proxy-hostsResponse Data: { "id": 1, "created_on": "2025-06-30 10:59:25", "modified_on": "2025-06-30 10:59:25", "owner_user_id": 1, "domain_names": [ "test.example.com" ], "forward_host": "1.1.1.1", "forward_port": 80, "access_list_id": 0, "certificate_id": 0, "ssl_forced": false, "caching_enabled": false, "block_exploits": false, "advanced_config": "", "meta": { "letsencrypt_agree": false, "dns_challenge": false }, "allow_websocket_upgrade": false, "http2_support": false, "forward_scheme": "http", "enabled": true, "locations": [], "hsts_enabled": false, "hsts_subdomains": false, "enable_proxy_protocol": 0, "load_balancer_ip": "", "certificate": null, "owner": { "id": 1, "created_on": "2025-06-30 10:57:10", "modified_on": "2025-06-30 10:57:10", "is_disabled": false, "email": "admin@example.com", "name": "Administrator", "nickname": "Admin", "avatar": "", "roles": [ "admin" ] }, "access_list": null}[validateSwaggerSchema ERROR] Validation Errors: [ { "instancePath": "/enable_proxy_protocol", "schemaPath": "#/properties/enable_proxy_protocol/ "keyword": "type", "params": { "type": "boolean" }, "message": "must be boolean", "schema": "boolean", "parentSchema": { "description": "Enable PROXY Protocol support", "example": true, "type": "boolean" }, "data": 0 }]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.
This is probably due to a missing part of the ORM glue. Add theenable_proxy_protocol string to the array ofboolFields in
backend/models/proxy_host.js:14NikeLaosClericus commentedJul 17, 2025
I've just started testing this in my homelab (I create pr#4660 just to get a working image). I can report that I get the same errors that@Coolicky was getting in pr#4105 when enabling PP for Streamsspecifically when setting the Proxy IP. Aren't being handled for Stream confs at all (or incorrectly?) |
NikeLaosClericus commentedJul 17, 2025
My next comment is on design choice. I dislike that fact that enabling PP disables (replaces) the standard web hosting ports altogether. Mine, manually modified, for example: And as far as I can tell, the inclusion of thereal_ip lines does nothing to harm port 80/443 standard implementations, while still working correctly for 88/444. This is necessary for my implementation as my upstream proxy server is an nginx Stream configuration using SNI for domain based routing of https/443 connections (without termination). So I need my server configurations in NPM to simultaneously accept 444 PROXY and 80 non-PROXY connections. |
NikeLaosClericus commentedJul 17, 2025
All-in-all, this is absolutely a necessary inclusion for NPM that I'm grateful it's being worked on and is close, but can the implementation be tweaked so that it doesn't 'turn of' standard hosting when enabled, or provide an additional switch to add both if desired? Unrelated, but it occurred to me that I could have circumvented this by adding an additional proxy host to NPM for the same domains, if NPM allowed per port/protocol configurations. |
NikeLaosClericus commentedJul 17, 2025
How I modified /app/templates/_listen.conf, enabling both simultaneously: |
This is essentially#1882 with the merge conflicts resolved.
I first tried submitting a PR to the source branch, but it's not yet been merged.SBado#1
I thought I might have better luck here 😄