nginx_proxy: Adding support for TCP Proxy Protocol #3274
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Hi @agners, just wondering what would be the next step to get the PR merged? |
There was a problem hiding this comment.
I am not very familiar with the various proxy options/configurations, but wouldn't the first option documented in https://docs.nginx.com/nginx/admin-guide/load-balancer/using-proxy-protocol/ to get the originating IP address more generic? (E.g. not using the RealIP module? 🤔 ).
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Hmm it could be? Tbh i've been using $proxy_protocol_addr as that was being used in a couple of other examples with proxy protocol. |
There is also this note:
🤔 I am really not an expert in NGINX/Proxy setups. But I think if things can work without the RealIP module, it seems the better approach to me. |
agners
changed the title
|
@agners just wondering if we're good to merge the pr after your comments had been taken care of |
There was a problem hiding this comment.
Sorry for the late review 😢
I actually wanted to test this, and I finally came around. During testing, I realized one thing: Instead of using the sub config we could just promote the real_ip_from as top level config. The sub-config makes sense if there are multiple configs underneath. The enable property made sense in the past when there were no optional configs.
But today, we have optional configs, so promoting real_ip_from to top level, we can just check if it is set, and if so enable the proxy protocol. Use bashio::config.exists to check if the user has set a value:
if bashio::config.exists 'real_ip_from'; then
The current approach if the user sets proxy protocol to enable, the add-on will only tell at runtime through logs that real_ip_from is missing. With the single config approach, all requirements are checked at config time. So I'd prefer this approach for that reason.
If you prefer the current approach, that is fine too, but we have to remove the default value of real_ip_from, otherwise users which don't use that feature are forced to set a dummy value there.
Also, in any case the top-level config needs translation strings. So please also add name/description strings for the top-level config in translations/en.yaml.
|
@agners No worries, thanks for taking your time reviewing it! I've included your suggested changes, I agree this makes it easier and makes sense! I've tested the latest changes locally as well and it's working as intended. |
WalkthroughWalkthroughThe changes in the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (6)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
nginx_proxy/DOCS.md (2)
46-46: Consider providing example values or a default value forreal_ip_from.Including example values or a default value can help users understand how to configure this option correctly.
real_ip_from: - 192.168.1.0/24 - 10.0.0.0/8
82-85: Improve the explanation of thereal_ip_fromoption.To align with the documentation style guide, front the goal in the instructional sentence and use bold for the link text.
- If specified, configures Nginx to use Proxy Protocol to get the Real Ip from an upstream load balancer; [for more information](https://docs.nginx.com/nginx/admin-guide/load-balancer/using-proxy-protocol/). + Configure Nginx to use Proxy Protocol to get the Real IP from an upstream load balancer if specified; see **for more information**(https://docs.nginx.com/nginx/admin-guide/load-balancer/using-proxy-protocol/).
|
@agners Made more substantial changes to move replacement logic by taking advantage of templating; tested the changes locally by specifying different values in the ui options (changing the boolean values & text inputs) and the generated |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/nginx/run (1)
13-16: Template file missing:The template file
/etc/nginx/nginx.conf.gtpldoes not exist in the expected location. This will prevent the successful generation ofnginx.confusingtempio. Please ensure that the file is present and correctly located in the repository.Analysis chain
Verify the existence of the template file.
Ensure that the template file
/etc/nginx/nginx.conf.gtplexists and is correctly formatted for use withtempio.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and format of the template file. # Test: Check if the template file exists. if [ -f /etc/nginx/nginx.conf.gtpl ]; then echo "Template file exists." else echo "Template file does not exist." fiLength of output: 108
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
hi, just installed the update and now my nginx is unable to start, it seems this option is not optional?
Edit: after adding a random IP, saving and removing it again, it starts up again |
|
This feature breaks the funcionality of the plugin. Setting a "real_ip_from" is mandantory, otherwise the plugin won't start. If setting the local network as allowed the plugin starts, but no connection is possible. Would be nice to read a word in the changelog if breaking changes are part of the release. |
|
Making This should address the issue: #3725. |
|
Might this be related to some having trouble getting to HA after updating to 3.10.0? |
|
It seems that adding real_ip_from: [] to settings yaml, or setting anything in the GUI and then removing it, is letting people at HA thorugh nginx with 3.10.0 |
|
Sorry! This was definitely not intended to be a breaking change, thanks for the quick fix @agners |
This allows Nginx to support the TCP proxy protocol, this is useful if you still want to TLS terminate on the home-assistant host but you have another entrypoint for all your TLS connections (i.e. traefik is your entrypoint and you redirect from it to home-assistant based on the HTTPS domain being requested, this makes sure that any kind of network request to home-assistant remains encrypted).
What this allows is that home-assistant will know the true ip of the user making requests (making this change because I saw in the logs that there was a failed log-in but the ip pointed to my traefik instance).
Tested this by installing these changes as a local add-on on home-assistant.
Summary by CodeRabbit
New Features
real_ip_fromconfiguration option, allowing specification of trusted IP addresses for real IP forwarding.Bug Fixes
Documentation
Chores