Allow advanced customization of nginx parameters in addon #3874
Conversation
My use-case is that I want to setup mTLS, except for webhooks endpoints. Because ssl_verify_client cannot be set at the `location` level but needs to be set at the `server` level, and then whether we are authenticated should be checked at the location level, this requires modifying the `location /` directive of the configuration. It follows that [the template](https://github.com/home-assistant/addons/blob/91160e1b5d00e13091b0537d85a3b3112e4a3f60/nginx_proxy/rootfs/etc/nginx/nginx.conf.gtpl) is not customizable enough. To give full flexibility to the user to patch the server config however they like, we allow them to execute arbitrary commands or scripts from within the container before starting nginx, similarly to what is done by the SSH addon and appdaemon.
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the NGINX Home Assistant SSL proxy configuration by adding support for initialization commands. The changes include updating the Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Script as Startup Script
participant Nginx as Nginx Server
Config->>Script: Provide init_commands
Script->>Script: Validate init_commands
alt Commands exist
Script->>Script: Execute each command
alt Command fails
Script-->>Config: Log error
Script->>Script: Exit with failure
else All commands succeed
Script->>Nginx: Start server
end
else No init_commands
Script->>Nginx: Start server directly
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nginx_proxy/config.yaml(2 hunks)nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/nginx/run(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
nginx_proxy/config.yaml (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🔇 Additional comments (2)
nginx_proxy/config.yaml (2)
30-30: Good default for optional commands.
Providing an empty list (init_commands: []) as the default ensures that no commands are executed unless explicitly specified, which is a safe and clear approach for new users.
46-47: Schema aligns with the new feature.
Defining init_commands as a list of strings properly reflects the new functionality and ensures valid user input for each command. This is consistent and well-integrated with the rest of the schema.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nginx_proxy/DOCS.md(1 hunks)nginx_proxy/translations/en.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
nginx_proxy/DOCS.md (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
So you would then expect users to write sed script to change a config they don't really can see. This does seem a bit odd to me. I'd rather prefer if we extend the template so things can be adjusted in whatever ways necessary. We could maybe even offer a full custom NGINX config instead (which IMHO, is still better than asking the user to modifying a "hidden" config via scripting). Can you share a full example of what you want to archive in your particular use case? That might make it easier to come up with a good solution. |
|
I was thinking I'd probably not largely override the file if I had that script. It's mostly about giving full flexibility. If people aren't able to In my case specifically what I want to do is this: with Currently my workaround is that I've configured a stub server as the default server (with an unused domain name), and the actual server that I use is configured manually, so I have to copy and maintain template contents to the I'd love to just Having a template parameter for "extra default location statements" that would yield at the top would indeed solve my use case, but it feels like someone may have other needs that this wouldn't cover and I don't care about writing this as a dedicated template parameter or as a Maybe another option would be to be able to fully disable default server, so people could just always configure it as an extra server. |
But even cp'ing a useful config seems hard/almost unrealistic without gleaning at the existing config. Of course init_commands allow for lots of flexibility, but it also has a very high chance of breaking if we change the default template. Once such option is in, you can only loose as an add-on maintainer: Touch anything and you make people angry because you broke their production system 😥 I mean, we could introduce a "generic" add-on, where you can just have just a init command in config. Then you can apk add whatever package you need and configure it using that script. This of course is an extreme case, but it is kinda the other end of the spectrum in terms of flexibility. Just allowing generic init command for this add-on is too open IMHO. This is the NGINX Proxy add-on, it is somewhat opinionated. That being said, I can see your use case, and it in my opinion it somewhat in scope of what user would want to do. We could go two avenues: Explicitly support client certificate (e.g. by introducing It should be possible to do this with a single location section as well, with something along these lines: If we can get a way with a single location config, then maybe just adding a location include? Another alternative would be to enable location customizations. It seems that if two location sections are in a server section, the last one takes precedence. So we could move this section to the end of the server section: This then would allow users to override I think I'd lean towards the second solution for now, add a location customization option. With that we can cover your use case and likely others today. An explicit client certificate option is still possible in the future. |
I seem to recall that I tested this and actually got a fatal error that I had 2
I see what you mean... When I started up my server and lazily decided to just trust HA's login page in the beginning, it wasn't two weeks before I received alerts of people trying to login from china and VPN servers though 😅 so maybe making it easier for people to setup mTLS might be nice 🙂 |
Yeah, totally agree, reducing attack surface is a very good security practice! That said, as you probably know/experienced, deploying mTLS has it's own set of challenges 😅 Let's attempt option 2 for now (location include). This allows to use the NGINX proxy to actually test this a bit more. |
My use-case is that I want to setup mTLS, except for webhooks endpoints.
Because nginx's
ssl_verify_clientcannot be set at thelocationlevel but needs to be set at theserverlevel, and then whether we are authenticated should be checked at the location level, this requires modifying thelocation /directive of the configuration.It follows that the template is not customizable enough.
To give full flexibility to the user to patch the server config however they like, we allow them to execute arbitrary commands or scripts from within the container before starting nginx, similarly to what is done by the SSH addon and appdaemon.
Summary by CodeRabbit
New Features
Improvements