PROXY protocol support for Streams#5511
Conversation
…/nginx-proxy-manager:develop into develop
…oxy-manager into develop # Conflicts: # backend/schema/endpoints/proxy-hosts.json # backend/templates/_listen.conf
Update proxy_host.js
Initial work to move prior work to new UI and API
Make the variable name a little more generic
…ng but Streams isn't...
This reverts commit 548f3bd.
|
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. |
Code ReviewThanks for the PR — PROXY protocol support is a useful addition. Here are some issues I found that should be addressed before merge. Blocking Bugs1. Migration Both migration files define Affected files:
// 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
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": "...#/properties/enable_proxy_protocol", // ← trailing comma
},This will break schema validation at runtime. 4. Debug comment rendered into every stream nginx config
This renders a nginx comment with the raw field value into every generated stream config. It needs to be removed. Bugs5.
{%- if enable_proxy_protocol == 1 or enable_proxy_protocol == true %}If the value comes through as a boolean 6. In {% if load_balancer_ip != '' %}
set_real_ip_from {{ load_balancer_ip }};
{% endif %}
real_ip_header proxy_protocol;If Minor7. Inconsistent UI between modals
8. The 9. Dead code comments in JSX
Test CoverageThe Cypress tests are appropriately extended to assert |
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 Also, since this proxy change needs ports 88/444, you may want to consider adding documentation to |
…h this was based on
…as the other template
|
Pushed fixes for 1-6 & 9, as well as an update to the advanced config docs. For comment number 7, I was following the styles of the files that I was editing. The reason that the proxy host checkbox uses lime for the color is because the rest of the toggles in that file use it. The reason that the stream form does not use lime is because the toggles there do not. Same answer for spreading of For comment number 8, I cannot submit the form with no value in the "load balancer ip" field, so it seems to be working as intended from a UX perspective, at least as far as a value being required. Could you give some pointers as to where and how the YUP schema or validation would be implemented? I am not familiar with Formik and was working primarily off of examples elsewhere in this project. |
|
Docker Image for build 8 is available on DockerHub: Note Ensure you backup your NPM instance before testing this image! Especially if there are database changes. Warning Changes and additions to DNS Providers require verification by at least 2 members of the community! |
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.