PROXY Protocol support (rd 2) #4505
Open
+127
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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 |
|
Having the same issue on my pull request. |
|
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. |
|
Awesome, hopefully the CI can be re-run and then I'll test the Docker image |
|
@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! |
|
@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? |
jc21
reviewed
Jun 30, 2025
| "enable_proxy_protocol": { | ||
| "description": "Enable PROXY Protocol support", | ||
| "example": true, | ||
| "type": "boolean" |
There was a problem hiding this comment.
CI is failing because of this.
Endpoint: POST /nginx/proxy-hosts
Response 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.
This is probably due to a missing part of the ORM glue. Add the enable_proxy_protocol string to the array of boolFields in
backend/models/proxy_host.js:14
|
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 Streams specifically when setting the Proxy IP. Aren't being handled for Stream confs at all (or incorrectly?) |
|
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 the real_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. |
|
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. |
|
How I modified /app/templates/_listen.conf, enabling both simultaneously: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 😄