Update docker image #1917
Update docker image #1917
Conversation
Add `BEHIND_PROXY` to compose Move `TRUSTED_PROXIES` and added default values Added notes on what to do when running behind a proxy. Update entrypoint and caddyfile for better proxy support. Add a full stack compose file with database and redis containers for more advanced setups.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (2)
Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure 📝 WalkthroughWalkthroughAdds a new full-stack Docker Compose file and updates compose, Caddyfile, and entrypoint to support proxy-aware startup by exporting PARSED_LE_EMAIL, PARSED_APP_URL, and APP_ASSET; removes the DB migration step from the entrypoint. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Reverse Proxy / Client
participant Panel as Panel Container (Entrypoint → Caddy & App)
participant Caddy as Caddy Server (inside Panel)
rect rgb(250,250,255)
Note over Panel: Container startup (docker/entrypoint.sh)
Panel->>Panel: Read env: APP_URL, LE_EMAIL, BEHIND_PROXY
Panel->>Panel: Export PARSED_APP_URL, PARSED_LE_EMAIL, APP_ASSET
alt BEHIND_PROXY = "true"
Panel->>Panel: PARSED_APP_URL = ":80"
Panel->>Panel: PARSED_LE_EMAIL = ""
Panel->>Panel: APP_ASSET set from APP_URL
else
Panel->>Panel: PARSED_* derived from APP_URL/LE_EMAIL
end
Panel->>Caddy: Render Caddyfile with {$PARSED_APP_URL}, {$PARSED_LE_EMAIL}
Caddy-->>Panel: Start listening on configured addresses
end
rect rgb(235,255,235)
Note over Client,Caddy: Request handling
Client->>Caddy: HTTP request (port 80) or HTTPS (port 443)
Caddy->>Panel: Proxy/request routing to app as configured
Panel-->>Client: HTTP/HTTPS response
end
Pre-merge checks❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
compose-full-stack.yml(1 hunks)compose.yml(2 hunks)docker/Caddyfile(1 hunks)docker/entrypoint.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
docker/entrypoint.sh
[error] 17-17: In dash, echo flags besides -n not supported.
(SC3036)
[error] 45-45: In dash, [[ ]] is not supported.
(SC3010)
[error] 45-45: In dash, == in place of = is not supported.
(SC3014)
🔇 Additional comments (4)
docker/entrypoint.sh (1)
36-37: Verify removal of database migrations is intentional.The database migration step was removed from the entrypoint. This change should be verified to ensure migrations are handled elsewhere (e.g., manually by operators or in a separate initialization step), as skipping migrations could lead to schema mismatches.
Please confirm:
- Are migrations now expected to be run manually?
- Is there documentation or a separate process for running migrations in the updated workflow?
docker/Caddyfile (1)
9-12: LGTM! Proxy-aware configuration correctly implemented.The use of
PARSED_ADMIN_EMAILandPARSED_APP_URLcorrectly aligns with the entrypoint changes. WhenBEHIND_PROXY=true,PARSED_APP_URLwill be set to:80, which prevents Caddy from forcing HTTPS redirects and resolves the core issue described in #1000.compose-full-stack.yml (1)
1-97: Well-structured full-stack configuration.The new compose file provides a complete setup with database, cache, and panel services using environment anchors effectively. The configuration aligns well with the proxy support objectives and provides clear templates for users to customize.
compose.yml (1)
9-10: Good addition of proxy configuration templates.The commented
BEHIND_PROXYandTRUSTED_PROXIESvariables provide clear guidance for users running behind a reverse proxy. The default IPs align with the network subnet defined in the file.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docker/entrypoint.sh (1)
41-51: Fix typos and use portable shell syntax for ash compatibility.The proxy-aware logic correctly implements the PR objectives, but has unresolved issues flagged in previous reviews:
- Line 46 typo: "behin" should be "behind"
- Line 50 typo: "APP_ASSETT" should be "APP_ASSET"
- Line 45 syntax:
[[ ]]is bash-specific; use[ ]for ash/POSIX compatibility (also fixes Shellcheck SC3010/SC3014)Apply this diff to fix all three issues:
-# when running behind a proxy -if [[ ${BEHIND_PROXY} == "true" ]]; then - echo "running behin proxy" +# when running behind a proxy +if [ "${BEHIND_PROXY}" = "true" ]; then + echo "running behind proxy" echo "listening on port 80 internally" export PARSED_ADMIN_EMAIL="" export PARSED_APP_URL=":80" - export APP_ASSETT=${APP_URL} + export APP_ASSET=${APP_URL} fiNote: Rmartinoscar already provided these exact suggestions in previous review comments.
🧹 Nitpick comments (2)
docker/entrypoint.sh (2)
12-18: Use portable echo syntax for ash compatibility.The script uses
#!/bin/ash -e, but lines 12, 14, 15, 17, and 18 rely onecho -e, which Shellcheck flags as unsupported in ash/dash (SC3036). To improve portability and silence shell linting warnings, useprintfinstead, which is POSIX-compliant:if [ -z $APP_KEY ]; then - echo -e "Generating key." + printf "Generating key.\n" APP_KEY=$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1) - echo -e "Generated app key: $APP_KEY" - echo -e "APP_KEY=$APP_KEY" > /pelican-data/.env + printf "Generated app key: %s\n" "$APP_KEY" + printf "APP_KEY=%s\n" "$APP_KEY" > /pelican-data/.env else - echo -e "APP_KEY exists in environment, using that." - echo -e "APP_KEY=$APP_KEY" > /pelican-data/.env + printf "APP_KEY exists in environment, using that.\n" + printf "APP_KEY=%s\n" "$APP_KEY" > /pelican-data/.env fi
54-66: Use portable conditional syntax for ash compatibility.Lines 54 and 62 use
[[ ]]conditionals (bash-specific), which Shellcheck flags as unsupported in ash/dash. Replace with portable[ ]syntax:## disable caddy if SKIP_CADDY is set -if [[ "${SKIP_CADDY:-}" == "true" ]]; then +if [ "${SKIP_CADDY:-}" = "true" ]; then echo "Starting PHP-FPM only" else echo "Starting PHP-FPM and Caddy" export SUPERVISORD_CADDY=true # handle trusted proxies for caddy - if [[ ! -z ${TRUSTED_PROXIES} ]]; then + if [ -n "${TRUSTED_PROXIES}" ]; then export CADDY_TRUSTED_PROXIES=$(echo "trusted_proxies static ${TRUSTED_PROXIES}" | sed 's/,/ /g') export CADDY_STRICT_PROXIES="trusted_proxies_strict" fi fiNote:
[ -n VAR ]is more portable and idiomatic than[ ! -z VAR ].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/entrypoint.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
docker/entrypoint.sh
[error] 12-12: In dash, echo flags besides -n not supported.
(SC3036)
[error] 14-14: In dash, echo flags besides -n not supported.
(SC3036)
[error] 15-15: In dash, echo flags besides -n not supported.
(SC3036)
[error] 45-45: In dash, [[ ]] is not supported.
(SC3010)
[error] 45-45: In dash, == in place of = is not supported.
(SC3014)
🔇 Additional comments (1)
docker/entrypoint.sh (1)
41-42: PARSED_ export approach is sound.*The introduction of
PARSED_ADMIN_EMAILandPARSED_APP_URLexports enables runtime configuration of Caddy based on deployment context. WhenBEHIND_PROXY=true, settingPARSED_APP_URLto:80prevents Caddy from enforcing external redirects, which correctly addresses PR objectives. The logic aligns well with the related Caddyfile changes.
Co-authored-by: MartinOscar <40749467+rmartinoscar@users.noreply.github.com>
replaces the admin email variable with `LE_EMAIL` to signify that it's for let's encrypt cert generation. also removes an extra equals sign
Co-authored-by: MartinOscar <40749467+rmartinoscar@users.noreply.github.com>
Co-authored-by: MartinOscar <40749467+rmartinoscar@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/entrypoint.sh (1)
62-62: Same portability issue on line 62.Line 62 uses
[[ ]]and the-ztest operator, which is fine, but should use[ ]for consistency.Apply this diff:
- if [[ ! -z ${TRUSTED_PROXIES} ]]; then + if [ -n "${TRUSTED_PROXIES}" ]; thenNote: Using
-n(string is not empty) is clearer than! -z(not zero length) and properly quotes the variable.
🧹 Nitpick comments (1)
compose-full-stack.yml (1)
26-27: Emphasize the critical security requirement to change database passwords.While "SUPERNEEDSTOCHANGE" and "NEEDSTOCHANGE" are obviously placeholders, consider adding a more prominent warning in the comment block above (lines 24-25) to stress that these passwords must be changed before deployment to production.
Example enhancement:
- # Do not remove the "&db-password" from the end of the line below, it is important - # for Panel functionality. + # IMPORTANT: Change these passwords before deploying to production! + # Do not remove the "&db-password" from the end of the line below, it is important + # for Panel functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
compose-full-stack.yml(1 hunks)compose.yml(2 hunks)docker/Caddyfile(1 hunks)docker/entrypoint.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- compose.yml
🧰 Additional context used
🪛 Shellcheck (0.11.0)
docker/entrypoint.sh
[error] 12-12: In dash, echo flags besides -n not supported.
(SC3036)
[error] 14-14: In dash, echo flags besides -n not supported.
(SC3036)
[error] 15-15: In dash, echo flags besides -n not supported.
(SC3036)
[error] 45-45: In dash, [[ ]] is not supported.
(SC3010)
[error] 45-45: In dash, == in place of = is not supported.
(SC3014)
🔇 Additional comments (4)
compose-full-stack.yml (2)
40-51: LGTM!The database and cache service configurations are well-structured. Using
mariadb:10.11with explicitmysql_native_passwordauthentication ensures compatibility, andredis:alpineprovides a lightweight caching solution.
76-78: Environment variable names are correct.The verification confirms that
CACHE_STORE,SESSION_DRIVER, andQUEUE_CONNECTIONare the correct variable names currently used throughout the application. These are referenced in the Laravel configuration files (config/cache.php,config/session.php) and are actively used in installer steps and console commands. The compose-full-stack.yml configuration is accurate.docker/Caddyfile (2)
9-9: LGTM!Using
{$PARSED_LE_EMAIL}correctly supports the proxy-aware deployment model. WhenBEHIND_PROXY=true, this variable is set to empty (indocker/entrypoint.shline 48), which disables automatic HTTPS certificate requests and prevents redirect loops when the panel is behind a TLS-terminating proxy.
12-12: LGTM!Using
{$PARSED_APP_URL}correctly addresses the PR objective to fix HTTP serving whenSITE_URLis HTTPS. When behind a proxy, this variable is set to:80(indocker/entrypoint.shline 49), ensuring Caddy listens on port 80 without forcing redirects to 443, resolving issue #1000.
Co-authored-by: MartinOscar <40749467+rmartinoscar@users.noreply.github.com>
|
Error: adapting config using caddyfile: parsing caddyfile tokens for 'email': wrong argument count or unexpected line ending after 'email', at /etc/caddy/Caddyfile:9 |
2 participants
Add
BEHIND_PROXYto composeMove
TRUSTED_PROXIESand added default valuesAdded notes on what to do when running behind a proxy.
Update entrypoint and caddyfile for better proxy support.
Add a full stack compose file with database and redis containers for more advanced setups.
resolves #1000