Update Nginx Proxy Manager to v2.12.3 #612
Conversation
WalkthroughThe pull request updates the Dockerfile to use a newer base image and upgrade the Nginx Proxy Manager version. It also revises file path references from Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant DockerBuilder as "Docker Build Process"
participant CertbotInstaller as "Certbot Plugin Installer"
Developer->>DockerBuilder: Initiate build
DockerBuilder->>DockerBuilder: Set base image & update Nginx PM version
DockerBuilder->>DockerBuilder: Apply file path and logging patches
DockerBuilder->>CertbotInstaller: Execute direct pip install (no venv activation)
DockerBuilder->>DockerBuilder: Finalize build & cleanup
Possibly related PRs
Suggested labels
Poem
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
🧹 Outside diff range and nitpick comments (1)
proxy-manager/patches/0003-Patch-npm-certbot-venv-plugin-handling.patch (1)
20-22: Consider enhancing error handlingThe command execution and logging look good, but consider adding specific error messages for common pip installation failures.
return utils.exec(cmd) .then((result) => { logger.complete(`Installed ${pluginKey}`); + }) + .catch((error) => { + logger.error(`Failed to install ${pluginKey}: ${error.message}`); + throw error; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
proxy-manager/Dockerfile(3 hunks)proxy-manager/patches/0001-patch-data-to-config-folder.patch(18 hunks)proxy-manager/patches/0002-Patch-redirect-logs-to-docker-output.patch(5 hunks)proxy-manager/patches/0002-patch-sass-version-in-frontend.patch(0 hunks)proxy-manager/patches/0003-Patch-npm-certbot-venv-plugin-handling.patch(1 hunks)proxy-manager/patches/0004-patch-certbot-venv-plugin-handling.patch(0 hunks)proxy-manager/requirements.txt(1 hunks)
💤 Files with no reviewable changes (2)
- proxy-manager/patches/0002-patch-sass-version-in-frontend.patch
- proxy-manager/patches/0004-patch-certbot-venv-plugin-handling.patch
✅ Files skipped from review due to trivial changes (1)
- proxy-manager/requirements.txt
🔇 Additional comments (6)
proxy-manager/patches/0003-Patch-npm-certbot-venv-plugin-handling.patch (2)
15-16: LGTM: Version replacement handling
The version replacement logic for plugin dependencies and versions is maintained correctly.
18-19: Review virtual environment handling changes
The removal of virtual environment activation/deactivation and cache control flags raises several concerns:
- Without virtual environment activation, packages might install system-wide instead of in the isolated certbot environment
- Removing
--no-cache-dircould affect build reproducibility
Let's verify the virtual environment setup and package installation location:
proxy-manager/Dockerfile (2)
1-1: LGTM: Version updates look good
The updates to the base image (0.2.2 → 0.2.5) and Nginx Proxy Manager (2.10.4 → 2.12.1) are minor version increments, suggesting backward compatibility is maintained.
Also applies to: 16-16
19-21: LGTM: Security-related package updates
Good security practices observed:
- Addition of explicit libcrypto3 and libssl3 dependencies
- Updates to OpenSSL and Python packages to latest patch versions
Also applies to: 26-26, 28-28, 37-37, 39-39
proxy-manager/patches/0002-Patch-redirect-logs-to-docker-output.patch (1)
Line range hint 1-140: LGTM: Excellent logging practice for containerized applications
The patch correctly redirects all Nginx logs to Docker's standard output (/proc/1/fd/1), which:
- Follows container best practices
- Enables centralized log collection
- Simplifies log management in orchestrated environments
proxy-manager/patches/0001-patch-data-to-config-folder.patch (1)
Line range hint 1-427: Path migration implementation looks good, but verify migration strategy
The patch consistently updates all paths from /data to /config, affecting:
- Configuration files
- SSL certificates
- Database location
- Access lists
- Log files
While the implementation is thorough, this change could impact existing installations.
Let's check if there's a migration strategy in place:
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Hi, I'm not a maintainer of this repository and thus I cannot set nor edit labels here. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as abuse.
This comment was marked as abuse.
| include /etc/nginx/conf.d/include/resolvers.conf; | ||
| @@ -69,27 +70,27 @@ http { | ||
| # Log format and fallback log file | ||
| include /etc/nginx/conf.d/include/log.conf; |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
proxy-manager/patches/0001-patch-data-to-config-folder.patch (1)
338-340: 💡 Verification agent❓ Verification inconclusive
Verify the stream module availability.
The addition of the stream module load directive is important for TCP/UDP proxy functionality, but we should verify its availability in the container.
🏁 Script executed:
#!/bin/bash # Check if the stream module is available in the container ls -l /usr/lib/nginx/modules/ngx_stream_module.soLength of output: 142
Action Required: Ensure the stream module is available before loading it.
The load directive was added at lines 338–340 in
proxy-manager/patches/0001-patch-data-to-config-folder.patch, but our verification shows that/usr/lib/nginx/modules/ngx_stream_module.sois missing from the container. Please update your container build (or installation process) so that the stream module is installed, or remove the load directive if it's not needed. This should prevent runtime errors when starting Nginx.
🧹 Nitpick comments (2)
proxy-manager/patches/0001-patch-data-to-config-folder.patch (2)
368-371: Improve cache security by using tmpfs.Moving the cache from
/var/lib/nginx/cacheto/tmp/nginx/cacheis good for containerization, but consider mounting this directory as tmpfs for better security and performance.Add the following to your container configuration:
volumes: + - type: tmpfs + target: /tmp/nginx/cache + tmpfs: + size: 256m
206-209: Consider using Docker logging drivers.Instead of writing logs to files in
/config/logs/, consider using Docker's logging drivers for better integration with container orchestration platforms.Update the logging configuration to use stdout/stderr:
- access_log /config/logs/letsencrypt-requests_access.log standard; - error_log /config/logs/letsencrypt-requests_error.log warn; + access_log /dev/stdout standard; + error_log /dev/stderr warn;Also applies to: 244-247, 267-270
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
proxy-manager/Dockerfile(1 hunks)proxy-manager/patches/0001-patch-data-to-config-folder.patch(18 hunks)proxy-manager/patches/0002-Patch-redirect-logs-to-docker-output.patch(5 hunks)proxy-manager/patches/0002-patch-sass-version-in-frontend.patch(0 hunks)proxy-manager/patches/0003-Patch-npm-certbot-venv-plugin-handling.patch(1 hunks)proxy-manager/patches/0004-patch-certbot-venv-plugin-handling.patch(0 hunks)
💤 Files with no reviewable changes (2)
- proxy-manager/patches/0004-patch-certbot-venv-plugin-handling.patch
- proxy-manager/patches/0002-patch-sass-version-in-frontend.patch
🚧 Files skipped from review as they are similar to previous changes (3)
- proxy-manager/patches/0003-Patch-npm-certbot-venv-plugin-handling.patch
- proxy-manager/patches/0002-Patch-redirect-logs-to-docker-output.patch
- proxy-manager/Dockerfile
🔇 Additional comments (5)
proxy-manager/patches/0001-patch-data-to-config-folder.patch (5)
1-25: LGTM! The patch header and summary look good.The patch correctly documents the scope of changes across 18 files, with a balanced number of insertions (52) and deletions (51), indicating a clean path migration from
/datato/config.
421-422:❓ Verification inconclusive
Verify ACME DNS registration file permissions.
The ACME DNS registration file contains sensitive credentials. Ensure it has appropriate permissions when created.
🏁 Script executed:
#!/bin/bash # Search for file permission settings for the ACME registration file find . -type f -exec grep -l "acme-registration.json.*chmod" {} \;Length of output: 71
Attention: Verify file permission settings for ACME DNS registration file
The patch updated the path from/data/acme-registration.jsonto/config/acme-registration.json, but our search did not reveal any associated permission-setting (e.g., usingchmod) in the codebase. Please manually verify that when the file is created, its access is restricted appropriately to protect sensitive credentials.
383-393:❓ Verification inconclusive
Ensure directory existence for Nginx configurations.
The include directives for various configuration types require their respective directories to exist. Verify that these directories are created during container initialization.
🏁 Script executed:
#!/bin/bash # Check if the script that creates these directories exists find . -type f -exec grep -l "mkdir.*config/nginx" {} \;Length of output: 61
Nginx Directory Creation Verification Required
The updated include directives now reference
/config/nginx/..., but our search for directory creation commands (using patterns likemkdir.*config/nginx) did not return any evidence that these directories are being created during container initialization. Please verify manually or update the container setup to ensure that the following directories exist at runtime:
/config/nginx/default_host/config/nginx/proxy_host/config/nginx/redirection_host/config/nginx/dead_host/config/nginx/tempIf these directories are expected to be mounted or created by an external process, kindly add a note in the documentation. Otherwise, consider adding explicit directory creation (e.g., via Dockerfile or an initialization script) to prevent configuration errors.
117-119:❓ Verification inconclusive
Verify key file security.
The keys.json file contains sensitive data. Ensure:
- Appropriate file permissions are set
- The file is properly secured
🏁 Script executed:
#!/bin/bash # Search for key file permission settings find . -type f -exec grep -l "keys.json.*chmod" {} \; # Check if the file is mentioned in any security-related code rg -l "keys.json" | xargs rg "crypto|encrypt|decrypt"Length of output: 2067
Action Required: Validate Security Settings for keys.json
The patch updates the file path for keys.json from
/data/keys.jsonto/config/keys.json, but our review indicates that there’s no evidence—via an in-code search using permission and security-related keywords—that file permissions are explicitly managed for this sensitive file. Please ensure that:
- The
/config/keys.jsonfile is created or maintained with strict file permissions (e.g., 600) to prevent unauthorized access.- Any deployment scripts or container configurations enforce these permission settings if the application code does not handle it.
Review and confirm that the mechanism securing keys.json is in place, whether through application logic, OS-level configuration, or container security policies.
126-128:❓ Verification inconclusive
Verify database file permissions and backup strategy.
The SQLite database file location has changed. Ensure:
- Appropriate file permissions are set
- The directory is persistent
- A backup strategy is in place
🏁 Script executed:
#!/bin/bash # Search for database file permission settings find . -type f -exec grep -l "database.sqlite.*chmod" {} \; # Search for backup implementation find . -type f -exec grep -l "backup.*database.sqlite" {} \;Length of output: 130
Ensure proper file permissions and backup for the updated SQLite file path
The patch changes the default location from/data/database.sqliteto/config/database.sqlite(lines 126-128). Automated searches did not uncover any explicit implementations for adjusting file permissions or backup routines related to this new path. Please verify manually that:
- The SQLite file at
/config/database.sqlitehas the correct permissions set- The
/configdirectory is persistent as required- A robust backup strategy is in place for the database
frenck
added
dependencies
frenck
changed the title
|
Thanks @frenck for the review!
Please note that I was trying to focus on one thing at a time by chaining this on top of another of my PRs: #611, with the intent of first making the Now I'm looking into the CI failure on this branch but don't fully understand it yet; it sounds like something failed while running node-sass. https://github.com/hassio-addons/addon-nginx-proxy-manager/actions/runs/13442141323/job/37559205781?pr=612 |
|
@mislav it is indeed I don't think this has to be a problem, but I think I think part of the original frontend patch is still needed, to ensure the sass-loader is in the 14.x or maybe 15.x range. |
|
I'll try ramping something up to see if I'm correct. brb. |
|
Nope that's not it. |
|
I'm trying to understand why the main branch is building, but this one isn't... 😟 |
|
This branch upgrades nginx-proxy-manager from v2.10.4...v2.12.3, which in turns bumps its frontend dependency It's all puzzling, especially since I was able to reproduce the node-gyp vs. Python 3.12 incompatibility locally, but in CI we get a different error; one that hints at node-sass incompatibility with Node 22 rather than incompatibility with Python: In any case, it sounds like installing node-sass under a modern Node and Python is a whole can of worms, which is all made especially difficult by the fact that node-sass is End of Life since mid-2024, meaning that it won't get fixes anymore. The maintainers of nginx-proxy-manager would have to switch to an alternative implementation of Sass if that software were to continue installing under modern systems. @frenck What do you think we should do here? Would it be feasible to downgrade Python to v3.11 and Node to v20 in the docker image of the addon? These are the versions that worked for me for building node-sass locally. Alternatively—and this would be my advice—we could also hold off upgrading Nginx Proxy Manager until it switches away from node-sass, upgrades the 5-year-old webpack that it uses (which is also not compatible with Node 22), and otherwise modernizes its stack. |
|
@mislav @frenck Is there some reason to assume the NPM guys are working on switching away from node-sass anytime soon? I'm quite out of the loop, but that's my 5 cents |
Chained PR on top of #611
Updating the patches was quite a pain 😞
Summary by CodeRabbit
Chores
Style