feat: add support for additional nginx configuration #834
Conversation
WalkthroughThe Nginx configuration template has been updated to include a new directive within the Changes
Poem
📜 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: 0
🧹 Nitpick comments (2)
nginx.conf.template (2)
48-49: Document the additional configuration capabilityThis change introduces important new functionality that should be documented.
Consider adding a comment above the include directive explaining:
+ # Include additional nginx configurations + # Place custom .conf files in /var/config/nginx/conf.d/ include /var/config/nginx/conf.d/*.conf;Also, please ensure this feature is documented in:
- README.md
- Deployment guide
- Configuration guide
48-49: Consider impact of configuration orderThe include directive is placed after all other location blocks, which means included configurations can potentially override existing settings. While this might be intentional, it should be clearly documented to prevent unexpected behavior.
Consider:
- Adding a warning comment about override potential
- Documenting any restrictions on what can be included
- Implementing validation for included configurations if possible
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nginx.conf.template(1 hunks)
🔇 Additional comments (1)
nginx.conf.template (1)
48-49: Verify directory existence and permissions
The include directive for additional configurations is correctly placed and follows nginx best practices. However, there are some operational considerations to address:
-
Ensure the
/var/config/nginx/conf.ddirectory:- Exists when the container starts
- Has appropriate read permissions for the nginx user
- Is properly documented in the deployment guide
-
Consider the startup behavior when no .conf files exist
Let's verify the container setup:
✅ Verification successful
Nginx configuration directory setup is properly handled
The configuration is secure and follows best practices:
/var/configdirectory is created with proper ownership (nginx user 499)/var/config/nginxdirectory is created at runtime before nginx starts- All nginx configurations are copied from
/etc/nginxto/var/config/nginx - The process runs as the nginx user (499) for security
The include directive will work correctly as:
- The directory structure is properly created
- The nginx user has the necessary permissions
- Even if no additional .conf files exist, nginx will start normally
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Dockerfile or deployment manifests handle the config directory
# Check for directory creation in Dockerfile
rg -l "mkdir.*(/var/config/nginx/conf\.d|/var/config/nginx)"
# Check for volume mounts or config maps in k8s manifests
fd -e yaml -e yml | xargs rg "/var/config/nginx/conf\.d"
Length of output: 201
Script:
#!/bin/bash
# Let's check the Dockerfile to see how the directory is created and its permissions
cat Dockerfile | grep -A 5 -B 5 "mkdir.*var/config/nginx"
Length of output: 519
- added suport for additional configuration files via /var/config/nginx/conf.d directory Signed-off-by: Stephan <[email protected]>
houhoucoop
force-pushed
the
steled-patch-1
branch
from
8169228 to
c46e68d
Compare
|
I’m thinking of closing this PR since we might not need it, any ideas? |
|
I thought it would bei good to have this possibility as I'm using cilium ingress and currently it is not possible to configure it with annotations so I need to configure them directly in the apps. |
What this PR does / why we need it
It would be great to be able to configure nginx via additional config files from the /var/config/nginx/conf.d directory.
Issue
Test Result
Additional documentation or context
Summary by CodeRabbit
New Features
Bug Fixes