Skip to content

PROXY protocol support for Streams#5511

Open
reversefold wants to merge 33 commits into
NginxProxyManager:developfrom
reversefold:stream-proxy-protocol
Open

PROXY protocol support for Streams#5511
reversefold wants to merge 33 commits into
NginxProxyManager:developfrom
reversefold:stream-proxy-protocol

Conversation

@reversefold
Copy link
Copy Markdown

Based on #5506, opening as a separate PR as the stream support has been problematic in CI testing but works fine when testing locally. I am opening a draft PR to try to work out the CI issues.

If someone could point to how to run the Cypress tests locally I would happily run them locally instead of waiting for the Jenkins CI.

SBado and others added 30 commits February 23, 2022 10:17
…oxy-manager into develop

# Conflicts:
#	backend/schema/endpoints/proxy-hosts.json
#	backend/templates/_listen.conf
Initial work to move prior work to new UI and API
Make the variable name a little more generic
@nginxproxymanagerci
Copy link
Copy Markdown

Docker Image for build 5 is available on DockerHub:

nginxproxymanager/nginx-proxy-manager-dev:pr-5511

Note

Ensure you backup your NPM instance before testing this image! Especially if there are database changes.
This is a different docker image namespace than the official image.

Warning

Changes and additions to DNS Providers require verification by at least 2 members of the community!

@reversefold
Copy link
Copy Markdown
Author

Finally. I found the last few things causing problems. A documented way to run the tests locally would have made this much quicker to debug, not to mention CI which actually flags what exactly is failing.

@reversefold reversefold marked this pull request as ready for review May 3, 2026 10:38
@reversefold reversefold changed the title [DRAFT] PROXY protocol support for Streams PROXY protocol support for Streams May 3, 2026
@jc21
Copy link
Copy Markdown
Member

jc21 commented May 14, 2026

Code Review

Thanks for the PR — PROXY protocol support is a useful addition. Here are some issues I found that should be addressed before merge.


Blocking Bugs

1. Migration down() uses wrong variable and wrong knex parameter — will crash on rollback

Both migration files define migrate_name (snake_case) at the top but reference migrateName (camelCase) inside down(), which is undefined and will throw a ReferenceError. Additionally, the down() parameter is named _knex but the function body calls knex.schema — a second crash on the same path.

Affected files:

  • backend/migrations/20220209144645_proxy_protocol.js (lines 48–57)
  • backend/migrations/20260425014442_proxy_protocol_stream.js (lines 95–104)
// Both files have this double bug:
const down = (_knex) => {                          // parameter is _knex...
    logger.info(`[${migrateName}] Migrating Down...`); // but uses migrateName (undefined)
    return knex.schema.table(...)                  // and uses knex (also undefined)

2. Migration filename is dated 2022 — incorrect run order

20220209144645_proxy_protocol.js will be ordered before all migrations that have run since February 2022, which could cause schema conflicts. It should be renamed with the current date (e.g. 20260514XXXXXX_proxy_protocol.js).


3. Trailing comma in JSON schema files — will cause parse errors

JSON does not permit trailing commas. These two files have one after the enable_proxy_protocol $ref:

  • backend/schema/paths/nginx/streams/post.json
  • backend/schema/paths/nginx/streams/streamID/put.json
"enable_proxy_protocol": {
    "$ref": "...#/properties/enable_proxy_protocol",  // ← trailing comma
},

This will break schema validation at runtime.


4. Debug comment rendered into every stream nginx config

backend/templates/stream.conf contains:

# {{ enable_proxy_protocol }}

This renders a nginx comment with the raw field value into every generated stream config. It needs to be removed.


Bugs

5. stream.conf boolean check is incomplete

stream.conf only checks enable_proxy_protocol == 1, not == true. All other templates in this PR check both conditions:

{%- if enable_proxy_protocol == 1 or enable_proxy_protocol == true %}

If the value comes through as a boolean true rather than integer 1, proxy protocol will silently not activate for streams.


6. real_ip_header proxy_protocol emitted without set_real_ip_from

In backend/templates/_proxy_protocol.conf:

{% if load_balancer_ip != '' %}
  set_real_ip_from {{ load_balancer_ip }};
{% endif %}
real_ip_header proxy_protocol;

If load_balancer_ip is empty, real_ip_header proxy_protocol; is still written to the config without any set_real_ip_from directive. nginx requires at least one set_real_ip_from for real_ip_header to function — without it nginx may log warnings and not extract the real IP. Either make load_balancer_ip required when proxy protocol is enabled, or guard real_ip_header proxy_protocol; inside the same if block.


Minor

7. Inconsistent UI between modals

ProxyHostModal.tsx applies a bg-lime class to the toggle when proxy protocol is enabled; StreamModal.tsx does not. The two toggle implementations also diverge — one spreads {...field}, the other manually wires checked and onChange. Worth making consistent.

8. loadBalancerIp validation not wired into Formik

The loadBalancerIp input sets required={form.values.enableProxyProtocol} but there is no corresponding Formik validate function or Yup schema entry. The HTML required attribute alone won't block form submission.

9. Dead code comments in JSX

ProxyHostModal.tsx and StreamModal.tsx both contain large commented-out blocks of legacy AngularJS-style markup. These should be removed rather than left as dead code.


Test Coverage

The Cypress tests are appropriately extended to assert enable_proxy_protocol defaults to false. A test that actually enables the flag and verifies the generated nginx config content would be valuable once CI is stable.

@jc21
Copy link
Copy Markdown
Member

jc21 commented May 14, 2026

Finally. I found the last few things causing problems. A documented way to run the tests locally would have made this much quicker to debug, not to mention CI which actually flags what exactly is failing.

CI logs are public, for this PR you can find your builds here: https://ci.nginxproxymanager.com/job/nginx-proxy-manager/job/PR-5511/ This is also linked in the Checks on this PR.

Also, since this proxy change needs ports 88/444, you may want to consider adding documentation to docs/src/advanced-config/index.md mentioning that they'll need to expose those ports in their docker configuration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants