[openvpn] Decouple VPN_NAME from openvpn process config #572#578
[openvpn] Decouple VPN_NAME from openvpn process config #572#578Piyushkhobragade wants to merge 1 commit intoopenwisp:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 WalkthroughImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/common/utils.sh`:
- Around line 248-251: The current logic that sets CONF_FILE by picking the
first globbed *.conf can accidentally pick an existing openvpn.conf; change the
discovery to pick the most recently modified .conf that is not openvpn.conf
(i.e., set CONF_FILE to the newest file excluding "openvpn.conf"), then only mv
that file to openvpn.conf if CONF_FILE is non-empty and different; update
references to CONF_FILE and the mv operation so the freshly downloaded profile
wins over an old openvpn.conf and no stale file is chosen.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56f3499c-d108-4d8c-804a-4a90f9e2ad9c
📒 Files selected for processing (2)
images/common/utils.shimages/openwisp_openvpn/supervisord.conf
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/utils.sh
📚 Learning: 2026-01-06T11:28:41.789Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
Applied to files:
images/openwisp_openvpn/supervisord.conf
🪛 Shellcheck (0.11.0)
images/common/utils.sh
[info] 248-248: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 248-248: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
🔇 Additional comments (1)
images/openwisp_openvpn/supervisord.conf (1)
21-21: Good fix: supervisord command is now safely decoupled fromVPN_NAME.Using a fixed config filename at Line 21 removes the whitespace-related process startup breakage path.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/runtests.py`:
- Around line 260-266: The test checks only for "PASS" in res.stdout, hiding
script failures; update the subprocess result handling around the subprocess.run
call (variable res) to first assert the exit code is zero (e.g., assertEqual on
res.returncode) and include res.stdout and res.stderr in the assertion message
for diagnostics, then keep the existing self.assertIn("PASS", res.stdout) check
to verify the marker.
- Around line 247-250: The test currently builds u_path from the process cwd
which is brittle and then injects it into script unquoted; change u_path
resolution to derive the repo-relative path using the test file location (use
dirname(__file__) / .. to locate the repo and join "images/common/utils.sh",
then abspath) so u_path is independent of cwd, and update the script string
built in tests/runtests.py (the symbol names u_path and script) to quote the
sourced path (e.g. use "source '...'" or equivalent quoting) to prevent shell
breakage on whitespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bba3581-5c56-4d9c-8fc8-4bccca19f8e6
📒 Files selected for processing (1)
tests/runtests.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
🔇 Additional comments (1)
tests/runtests.py (1)
523-523: Good addition to the ad-hoc suiteIncluding
test_openvpn_config_whitespace_handlingin the__main__suite improves local/manual regression coverage for the whitespace bug.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/runtests.py`:
- Around line 242-245: Remove the redundant local imports of os and subprocess
in the import block that currently reads "import os, import shlex, import
subprocess, import tempfile" — keep only "shlex" and "tempfile" since os and
subprocess are already imported at module level; update the import line(s) to
import shlex and tempfile and run tests to ensure no other local-scope import
usage was relied upon.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 090a1629-7f2a-4da4-af0a-f7cd0fa63e98
📒 Files selected for processing (1)
tests/runtests.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
🔇 Additional comments (1)
tests/runtests.py (1)
247-273: LGTM!The test properly validates the whitespace handling fix. Good use of
tempfile.TemporaryDirectory()for cleanup,shlex.quote()for shell-safe path escaping, and the cwd-independent path resolution using__file__. The return code assertion with diagnostic output before the PASS marker check ensures failures are easy to debug.
Piyushkhobragade
force-pushed
the
issues/572-vpn-name-supervisord
branch
from
7156351 to
e6735a6
Compare
|
@coderabbitai resume This will force the bot to look at your latest commit (e6735a6), realize there are no errors left, and hopefully update its official review status. |
|
🧠 Learnings used✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/common/utils.sh`:
- Around line 255-258: The crl_download function uses unquoted expansions which
triggers shellcheck SC2086 and can break on edge-case values; update the curl
invocation in function crl_download to quote the variable expansions so the URL
is constructed as "${DASHBOARD_INTERNAL}/admin/pki/ca/x509/ca/${CA_UUID}.crl"
(and quote the --output argument if you expect spaces) to avoid word-splitting
and globbing when expanding DASHBOARD_INTERNAL and CA_UUID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f23b629e-859b-4481-96f1-9ee65d76882c
📒 Files selected for processing (1)
images/common/utils.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/utils.sh
🪛 Shellcheck (0.11.0)
images/common/utils.sh
[info] 240-240: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 240-240: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 240-240: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 242-242: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 242-242: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 242-242: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 244-244: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
[info] 257-257: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 257-257: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (3)
images/common/utils.sh (3)
1-1: Shebang change may create confusion with/bin/shcallers.The cron job script
images/openwisp_openvpn/openvpn.shuses#!/bin/shand sources this file. When sourced, the shebang is ignored and code runs under the caller's shell. The newopenvpn_config_downloadcode is POSIX-compliant, so it will work correctly.However, this file already contains bash-isms elsewhere (e.g.,
[[ $? = "0" ]]at line 89), which would fail under strict POSIX sh. Consider either:
- Updating
openvpn.shto use#!/bin/bashfor consistency, or- Ensuring all functions callable from
/bin/shscripts remain POSIX-compliant[raise_minor_issue, request_verification]
#!/bin/bash # Check which scripts source utils.sh and their shebangs echo "=== Scripts that source utils.sh ===" rg -l 'source.*/utils\.sh|^\.\s+.*utils\.sh' --type sh echo -e "\n=== Checking shebangs of those scripts ===" for f in $(rg -l 'source.*/utils\.sh|^\.\s+.*utils\.sh' --type sh 2>/dev/null); do echo "$f: $(head -1 "$f")" done
239-242: LGTM!The
curlcommand formatting change is cosmetic and preserves functionality.
245-252: Well-implemented config discovery logic.The implementation correctly addresses the PR objective and previous review feedback:
- Uses
findwith! -name 'openvpn.conf'to avoid selecting stale configs-print -quitefficiently returns the first matchmv -f --properly handles filenames with leading dashes or whitespace- Error case properly returns non-zero (propagated by
set -ein callers)
Piyushkhobragade
force-pushed
the
issues/572-vpn-name-supervisord
branch
from
36630cd to
bded695
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/common/utils.sh`:
- Around line 239-242: The curl commands calling the OpenVPN download and
checksum endpoints pass unquoted URLs allowing word-splitting/variable expansion
issues; update the two curl invocations that write vpn.tar.gz and checksum to
wrap the URL arguments in double quotes and use braced variable syntax (e.g.,
"${API_INTERNAL}/controller/vpn/download-config/${UUID}/?key=${KEY}" and
"${API_INTERNAL}/controller/vpn/checksum/${UUID}/?key=${KEY}") so API_INTERNAL,
UUID and KEY are expanded safely and not split.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ca86d06-4db6-426a-abf2-17f6d3257b94
📒 Files selected for processing (1)
images/common/utils.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/utils.sh
🪛 Shellcheck (0.11.0)
images/common/utils.sh
[info] 240-240: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 240-240: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 240-240: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 242-242: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 242-242: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 242-242: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 244-244: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
🔇 Additional comments (3)
images/common/utils.sh (3)
1-1: Bash shebang is a good alignment with file syntax.Line [1] now matches the existing bash-style constructs used throughout this script.
245-252: Config normalization + fail-fast path looks solid.Lines [245]-[252] correctly normalize non-standard
*.confnames toopenvpn.confand return non-zero when no config is present, which is safer for startup behavior.
257-257: Good defensive quoting incrl_download.Line [257] now properly quotes the URL expansion, preventing word-splitting/globbing edge cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
images/common/utils.sh (1)
243-251:⚠️ Potential issue | 🟠 MajorOnly promote configs from the current archive.
images/openwisp_openvpn/openvpn.shdoescd /before calling this, sofind . -maxdepth 1 ...is scanning the container root, not justvpn.tar.gz. Combined with theelif [ ! -f openvpn.conf ]fallback, a failed or partial download can still look successful by reusing a pre-existingopenvpn.conf, and the cron job then restarts OpenVPN with stale or unrelated config. The cron caller should also gate the restart on this function returning success.🧰 Safer normalization from a scratch directory
- tar xzf vpn.tar.gz - chmod 600 *.pem - # Prefer a newly extracted non-standard filename and normalize it. - CONF_FILE=$(find . -maxdepth 1 -type f -name '*.conf' ! -name 'openvpn.conf' -print -quit) + TMPDIR=$(mktemp -d) || return 1 + tar xzf vpn.tar.gz -C "$TMPDIR" || { + rm -rf -- "$TMPDIR" + return 1 + } + chmod 600 -- "$TMPDIR"/*.pem 2>/dev/null || true + CONF_FILE=$(find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' ! -name 'openvpn.conf' -print -quit) if [ -n "$CONF_FILE" ]; then mv -f -- "$CONF_FILE" openvpn.conf - elif [ ! -f openvpn.conf ]; then + elif [ -f "$TMPDIR/openvpn.conf" ]; then + mv -f -- "$TMPDIR/openvpn.conf" openvpn.conf + else echo "ERROR: no OpenVPN config file found after extraction" >&2 + rm -rf -- "$TMPDIR" return 1 fi + mv -f -- "$TMPDIR"/*.pem . 2>/dev/null || true + rm -rf -- "$TMPDIR"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/common/utils.sh` around lines 243 - 251, The current find invocation (CONF_FILE=$(find . -maxdepth 1 -type f -name '*.conf' ...)) may scan the container root because callers (images/openwisp_openvpn/openvpn.sh) cd / before calling; to fix, extract vpn.tar.gz into a temporary isolated directory and detect configs there: create a temp dir (e.g. TMPDIR=$(mktemp -d)), run tar xzf vpn.tar.gz -C "$TMPDIR", then set CONF_FILE by running find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' ! -name 'openvpn.conf' -print -quit; if found, mv -f -- "$CONF_FILE" openvpn.conf (or mv into the intended destination using explicit paths), otherwise fail with the existing error/return 1; ensure cleanup of TMPDIR on success/failure. This keeps CONF_FILE discovery limited to the extracted archive and avoids reusing stale / root-level configs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/common/utils.sh`:
- Line 1: The scripts use Bash-only [[ ... ]] tests while one script is declared
as /bin/sh; fix by making the syntax consistent: either change both shebangs in
images/common/utils.sh and images/common/init_command.sh to #!/bin/bash (and
ensure any callers that source these scripts run under bash), or convert all
bash-only conditionals to POSIX test syntax (replace [[ ... ]] with [ ... ] and
adjust operators) — specifically update the conditional in wait_nginx_services
(function name) and the conditional checking VPN_DOMAIN in init_command.sh (the
if using [[ -z "$VPN_DOMAIN" ]]) so they use POSIX-compatible [ ... ] forms or
ensure both files run under bash. Ensure any sourced invocation uses the
matching shell.
---
Duplicate comments:
In `@images/common/utils.sh`:
- Around line 243-251: The current find invocation (CONF_FILE=$(find . -maxdepth
1 -type f -name '*.conf' ...)) may scan the container root because callers
(images/openwisp_openvpn/openvpn.sh) cd / before calling; to fix, extract
vpn.tar.gz into a temporary isolated directory and detect configs there: create
a temp dir (e.g. TMPDIR=$(mktemp -d)), run tar xzf vpn.tar.gz -C "$TMPDIR", then
set CONF_FILE by running find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' !
-name 'openvpn.conf' -print -quit; if found, mv -f -- "$CONF_FILE" openvpn.conf
(or mv into the intended destination using explicit paths), otherwise fail with
the existing error/return 1; ensure cleanup of TMPDIR on success/failure. This
keeps CONF_FILE discovery limited to the extracted archive and avoids reusing
stale / root-level configs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03a1180a-cbf1-49d8-a8fd-480ab519c28b
📒 Files selected for processing (1)
images/common/utils.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/utils.sh
🪛 Shellcheck (0.11.0)
images/common/utils.sh
[info] 244-244: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
🔇 Additional comments (1)
images/common/utils.sh (1)
239-242: Good URL quoting hardening.Quoting the composed
curlURLs here removes the word-splitting/globbing edge case and matches the shellcheck feedback.Also applies to: 257-257
|
Addressed the shell compatibility issue raised in review.
I chose this approach instead of rewriting bash-specific constructs to POSIX sh in order to keep the change minimal and avoid introducing unintended behavior changes. All QA checks and CI pass locally. Please let me know if you would prefer a POSIX-compliant rewrite instead. |
|
Following up on the shell compatibility concern: Both init_command.sh and utils.sh are now aligned to use bash, and the execution environment (Docker image) already relies on bash, so using bash-specific constructs (e.g. [[ ... ]]) is consistent with current assumptions. I chose this approach to keep the change minimal and avoid introducing risk from rewriting logic to POSIX sh at this stage. That said, I agree that long-term it may be beneficial to standardize on POSIX-compatible syntax if broader shell portability is a goal. Happy to refactor in that direction if preferred. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/openwisp_freeradius/Dockerfile`:
- Line 12: The Dockerfile's CMD uses bash to run init_command.sh but the Alpine
base doesn't include bash; update the image's package installation step (the RUN
apk add ... line) to include bash (install the bash package) so that the CMD
["bash", "init_command.sh"] can execute; ensure any shebangs or scripts that
expect ash are compatible or explicitly invoke bash in init_command.sh if
needed.
In `@images/openwisp_nfs/Dockerfile`:
- Line 18: The Dockerfile's CMD uses "bash" to run init_command.sh but the base
image (Alpine 3.23) doesn't include bash; update the image build step that
installs packages (the RUN apk add ... command) to include bash so CMD ["bash",
"init_command.sh"] can execute successfully; ensure the package name "bash" is
added to the existing RUN apk add invocation and rebuild the image.
In `@images/openwisp_nginx/Dockerfile`:
- Line 12: The Dockerfile invokes bash in CMD (CMD ["bash", "init_command.sh"])
but the image doesn't install bash; update the apk package installation (the
existing apk add --no-cache ... block) to include bash (or replace CMD to use
/bin/sh if you prefer not to add bash) so the container won't fail with "bash:
not found"; locate the Dockerfile CMD and the apk add lines and either add bash
to the installed packages or change CMD to use sh.
In `@images/openwisp_openvpn/Dockerfile`:
- Line 10: The Dockerfile's CMD ["bash", "init_command.sh"] requires bash but
the image (lisenet/openvpn:2.6.17) is Alpine-based and bash is not installed;
update the RUN apk add ... line to include bash as a dependency so the
init_command.sh shebang and CMD can run—modify the existing RUN apk add command
to add bash (or install bash in the same Dockerfile step) and ensure the image
retains bash for runtime so CMD ["bash", "init_command.sh"] works correctly.
In `@images/openwisp_postfix/Dockerfile`:
- Line 14: The Dockerfile's CMD uses "bash" to run init_command.sh but the image
never installs bash; update the Dockerfile to install a shell that provides bash
(e.g., add the appropriate package install such as bash for Debian/Ubuntu or
bash for Alpine) before the CMD, or change CMD to use a shell that is present
(e.g., "sh init_command.sh"); locate the CMD ["bash", "init_command.sh"] and
either add the package install step earlier in the Dockerfile or replace "bash"
with an available shell.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d755785a-448e-423c-ac44-a9e702a669d5
📒 Files selected for processing (7)
images/common/init_command.shimages/openwisp_dashboard/openvpn.jsonimages/openwisp_freeradius/Dockerfileimages/openwisp_nfs/Dockerfileimages/openwisp_nginx/Dockerfileimages/openwisp_openvpn/Dockerfileimages/openwisp_postfix/Dockerfile
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
📚 Learning: 2026-01-06T11:28:41.789Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
Applied to files:
images/openwisp_freeradius/Dockerfileimages/openwisp_postfix/Dockerfileimages/openwisp_nginx/Dockerfileimages/openwisp_openvpn/Dockerfile
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/init_command.sh
📚 Learning: 2026-03-02T19:44:00.554Z
Learnt from: nemesifier
Repo: openwisp/docker-openwisp PR: 0
File: :0-0
Timestamp: 2026-03-02T19:44:00.554Z
Learning: In the OpenVPN configuration (openvpn.json), "none" is included in the data_ciphers field (e.g., "AES-128-GCM:none") for backward compatibility with older configurations or devices.
Applied to files:
images/openwisp_dashboard/openvpn.json
🪛 Checkov (3.2.508)
images/openwisp_nfs/Dockerfile
[low] 1-18: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-18: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🔇 Additional comments (1)
images/openwisp_dashboard/openvpn.json (1)
15-22: Looks good — formatting-only change with compatibility preserved.Line 15–22 only reformat
data_ciphers; behavior is unchanged, and keeping"none"preserves backward compatibility.Based on learnings: In the OpenVPN configuration (
openvpn.json),"none"is intentionally included indata_ciphersfor backward compatibility.
| adduser -S freerad -G freerad | ||
|
|
||
| CMD ["sh", "init_command.sh"] | ||
| CMD ["bash", "init_command.sh"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n images/openwisp_freeradius/DockerfileRepository: openwisp/docker-openwisp
Length of output: 1265
🌐 Web query:
freeradius-server:3.2.8-alpine base image contents bash
💡 Result:
The freeradius-server:3.2.8-alpine Docker image is the official FreeRADIUS 3.2.x Alpine variant from freeradius/freeradius-server on Docker Hub, likely built from source at release_3_2_8 (August 2025) using Dockerfiles in github.com/FreeRADIUS/freeradius-server/scripts/docker/alpine. Base: Alpine Linux (recent version, e.g., 3.20+ or 3.21 based on layer ADD alpine-minirootfs-3.21.2-x86_64.tar.gz seen in similar images; size ~110MB, smaller than Ubuntu variants). Contents (inferred from dev Dockerfile, official docs, and Alpine freeradius package): - FreeRADIUS server 3.2.8 compiled from source (--prefix=/opt), installed to /opt/{bin,sbin,lib,etc}. - Symlink: ln -s /opt/etc/raddb /etc/raddb (default config: clients.conf, mods-config/files/authorize, etc.; no production users by default). - Runtime packages via apk: talloc, libressl (OpenSSL compat), pcre, libwbclient (Samba), libcurl, json-c, libldap, hiredis, sqlite-dev, and minimal others for core modules (to keep size small; optional modules like Perl/Python/SQL require extra apk add). - Binaries: /opt/sbin/radiusd (default CMD), radtest utils if included; exposes 1812/udp, 1813/udp. - Entrypoint: docker-entrypoint.sh for radiusd handling. - Modules: Core RADIUS modules (rlm_pap, rlm_chap, rlm_mschap, rlm_files, etc.), libfreeradius-{server,radius}.so; many rlm_*.so (e.g., rlm_unix, rlm_expr, rlm_cache); optional like eap, ldap, krb5 via subpackages but minimal in image. - No bash by default (Alpine uses ash /bin/sh; install bash via apk add bash if needed). - Default config accepts test clients on 127.0.0.1 only. To inspect locally: docker pull freeradius/freeradius-server:3.2.8-alpine; docker history ; docker run -it --rm
sh; ls /opt /etc/raddb; radiusd -X. Sources confirm Alpine base for small size, source build (not distro freeradius pkg), minimal runtime deps.
Citations:
- 1: https://hub.docker.com/layers/freeradius/freeradius-server/latest-3.2-alpine/images/sha256-8226c028d1908afacdd4efcfee0248c3f773ffc9528fc9a8b6f5091e9eab4159?context=explore
- 2: https://hub.docker.com/r/freeradius/freeradius-dev/dockerfile/
- 3: https://hub.docker.com/r/freeradius/freeradius-server/
- 4: https://github.com/FreeRADIUS/freeradius-server
- 5: https://github.com/FreeRADIUS/freeradius-server/tree/v3.0.x/scripts/docker
- 6: https://docker.aityp.com/image/docker.io/freeradius/freeradius-server:latest-3.2-alpine
- 7: https://pkgs.alpinelinux.org/package/v3.22/main/x86/freeradius
🌐 Web query:
Alpine Linux bash default included
💡 Result:
No, Alpine Linux does not include bash by default. The default shell is the BusyBox ash shell (/bin/ash or /bin/sh symlink). Bash is available as a separate package that must be installed via apk add bash.
Citations:
- 1: https://wiki.alpinelinux.org/wiki/Shell_management
- 2: https://wiki.alpinelinux.org/wiki/Change_default_shell
- 3: https://stackoverflow.com/questions/35689628/starting-a-shell-in-the-docker-alpine-container
- 4: https://amercader.net/blog/til-alpine-linux-does-not-ship-bash/
- 5: https://pkgs.alpinelinux.org/package/v3.21/main/x86_64/bash
Add bash to container dependencies; CMD will fail at runtime without it.
Line 12 specifies CMD ["bash", "init_command.sh"], but bash is not installed. Alpine Linux (the base image) does not include bash by default—it provides only the ash shell. Without explicitly adding bash to the apk install command, the container will fail when it attempts to start the CMD.
Proposed fix
RUN apk add --no-cache --update \
+ bash \
tzdata \
postgresql17-client && \
rm -rf /var/cache/apk/* /tmp/*📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CMD ["bash", "init_command.sh"] | |
| RUN apk add --no-cache --update \ | |
| bash \ | |
| tzdata \ | |
| postgresql17-client && \ | |
| rm -rf /var/cache/apk/* /tmp/* |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/openwisp_freeradius/Dockerfile` at line 12, The Dockerfile's CMD uses
bash to run init_command.sh but the Alpine base doesn't include bash; update the
image's package installation step (the RUN apk add ... line) to include bash
(install the bash package) so that the CMD ["bash", "init_command.sh"] can
execute; ensure any shebangs or scripts that expect ash are compatible or
explicitly invoke bash in init_command.sh if needed.
images/openwisp_openvpn/Dockerfile
Outdated
| supervisor && \ | ||
| rm -rf /var/cache/apk/* /tmp/* | ||
| CMD ["sh", "init_command.sh"] | ||
| CMD ["bash", "init_command.sh"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat images/openwisp_openvpn/DockerfileRepository: openwisp/docker-openwisp
Length of output: 833
🏁 Script executed:
# Search for init_command.sh to see what it expects
find . -name "init_command.sh" -type fRepository: openwisp/docker-openwisp
Length of output: 137
🌐 Web query:
lisenet/openvpn:2.6.17 docker image bash shell
💡 Result:
To access a bash shell in the lisenet/openvpn:2.6.17 Docker image, follow these steps. Note that the image is likely based on Alpine Linux (forked from kylemanna/docker-openvpn, which uses Alpine), so bash may not be installed by default—use sh instead, or install bash if needed. 1. Pull the image: docker pull lisenet/openvpn:2.6.17 2. Run a container interactively with privileged mode and necessary capabilities for OpenVPN: docker run -it --rm --cap-add=NET_ADMIN --device=/dev/net/tun -p 1194:1194/udp --privileged lisenet/openvpn:2.6.17 sh This starts a shell (sh) inside the container. If bash is available, you can override the entrypoint: docker run -it --rm --entrypoint bash lisenet/openvpn:2.6.17 If bash is not found, install it (Alpine): apk add bash 3. Alternatively, if you have a running container named my-openvpn: docker exec -it my-openvpn sh or docker exec -it my-openvpn bash The image exists (latest release 2.6.17-build37 on GitHub, tags like 2.6.17 available), official repo at https://hub.docker.com/r/lisenet/openvpn, source https://github.com/lisenet/docker-openvpn. Default entrypoint is ovpn_run; config in /etc/openvpn (volume). For production, initialize data volume first as per docs: docker run --rm -v /path/to/ovpn-data:/etc/openvpn lisenet/openvpn:2.6.17 ovpn_genconfig -u udp://VPN.SERVERNAME.COM; docker run --rm -v /path/to/ovpn-data:/etc/openvpn lisenet/openvpn:2.6.17 ovpn_initpki.
Citations:
- 1: https://github.com/lisenet/docker-openvpn
- 2: https://hub.docker.com/r/lisenet/openvpn
- 3: https://www.lisenet.com/2022/openvpn-server-on-kubernetes/
🏁 Script executed:
head -30 ./images/common/init_command.shRepository: openwisp/docker-openwisp
Length of output: 829
Add bash as an explicit dependency in the RUN apk add command.
The Dockerfile executes a CMD that requires bash, but bash is not installed. The base image lisenet/openvpn:2.6.17 is Alpine Linux–based and does not include bash by default. The init_command.sh script has an explicit bash shebang, making bash a required runtime dependency. Without it, the container will fail to start when the CMD tries to execute.
Proposed fix
RUN apk add --no-cache \
+ bash \
curl \
tzdata \
supervisor && \
rm -rf /var/cache/apk/* /tmp/*📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CMD ["bash", "init_command.sh"] | |
| RUN apk add --no-cache \ | |
| bash \ | |
| curl \ | |
| tzdata \ | |
| supervisor && \ | |
| rm -rf /var/cache/apk/* /tmp/* |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/openwisp_openvpn/Dockerfile` at line 10, The Dockerfile's CMD ["bash",
"init_command.sh"] requires bash but the image (lisenet/openvpn:2.6.17) is
Alpine-based and bash is not installed; update the RUN apk add ... line to
include bash as a dependency so the init_command.sh shebang and CMD can
run—modify the existing RUN apk add command to add bash (or install bash in the
same Dockerfile step) and ensure the image retains bash for runtime so CMD
["bash", "init_command.sh"] works correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
images/common/utils.sh (1)
245-249:⚠️ Potential issue | 🔴 CriticalRestrict
.confdiscovery to the extracted archive.Line 246 still scans
.and stops at the first non-openvpn.confmatch. In this startup flow, the same working directory also needs to containsupervisord.confbecauseimages/common/init_command.shlater runssupervisord --configuration supervisord.conf; iffindreturns that file first, Line 248 renames the supervisor config toopenvpn.confand startup breaks. Please discover the profile from the archive contents or a temp extraction directory instead of scanning every*.confin CWD.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/common/utils.sh` around lines 245 - 249, The current discovery uses find . (stored in CONF_FILE) which can pick up unrelated files like supervisord.conf; change the search to only the archive extraction directory (e.g., use a dedicated EXTRACT_DIR or temp dir) instead of '.' and/or explicitly exclude supervisord.conf: replace the find invocation that sets CONF_FILE (the line using find . -maxdepth 1 -type f -name '*.conf' ! -name 'openvpn.conf' -print -quit) to search EXTRACT_DIR (or create and extract into a temp dir) and/or add ! -name 'supervisord.conf' so only profile .conf files from the extracted archive can be renamed to openvpn.conf, leaving supervisord.conf untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/common/init_command.sh`:
- Line 32: The script currently uses a bare exit when VPN_DOMAIN is missing,
which yields a zero status; change the branch that checks VPN_DOMAIN (the if [
-z "$VPN_DOMAIN" ] test) to emit a clear error message to stderr and exit with a
non‑zero status (e.g., exit 1) so the OpenVPN container fails and triggers the
on-failure restart policy; update the branch to echo "VPN_DOMAIN is not set" >&2
(or similar) and then exit 1.
---
Duplicate comments:
In `@images/common/utils.sh`:
- Around line 245-249: The current discovery uses find . (stored in CONF_FILE)
which can pick up unrelated files like supervisord.conf; change the search to
only the archive extraction directory (e.g., use a dedicated EXTRACT_DIR or temp
dir) instead of '.' and/or explicitly exclude supervisord.conf: replace the find
invocation that sets CONF_FILE (the line using find . -maxdepth 1 -type f -name
'*.conf' ! -name 'openvpn.conf' -print -quit) to search EXTRACT_DIR (or create
and extract into a temp dir) and/or add ! -name 'supervisord.conf' so only
profile .conf files from the extracted archive can be renamed to openvpn.conf,
leaving supervisord.conf untouched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3408b992-1a15-422e-922a-5c4fe4f0689b
📒 Files selected for processing (2)
images/common/init_command.shimages/common/utils.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
📚 Learning: 2026-01-06T11:28:41.789Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
Applied to files:
images/common/init_command.sh
📚 Learning: 2026-01-06T11:56:54.242Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:85-85
Timestamp: 2026-01-06T11:56:54.242Z
Learning: In docker-openwisp's docker-compose.yml, setting CELERY_SERVICE_NETWORK_MODE to an empty string ("") causes Docker Compose to treat network_mode as unset/null, allowing celery containers to use the Compose default network with service name DNS resolution. Using "bridge" instead would break service name resolution needed for celery to communicate with dashboard, postgres, and redis services.
Applied to files:
images/common/init_command.sh
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/init_command.shimages/common/utils.sh
🪛 Shellcheck (0.11.0)
images/common/utils.sh
[style] 89-89: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[info] 244-244: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
images/common/init_command.sh
Outdated
| fi | ||
| elif [ "$MODULE_NAME" = 'openvpn' ]; then | ||
| if [[ -z "$VPN_DOMAIN" ]]; then exit; fi | ||
| if [ -z "$VPN_DOMAIN" ]; then exit; fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that a bare `exit` in bash reuses the previous command's status.
bash -c 'if [ -z "" ]; then exit; fi'
printf 'bare exit status: %s\n' "$?"
bash -c 'if [ -z "" ]; then exit 1; fi'
printf 'explicit exit 1 status: %s\n' "$?"Repository: openwisp/docker-openwisp
Length of output: 113
Make the missing-VPN_DOMAIN branch fail explicitly.
Line 32 uses a bare exit, which inherits the status of the preceding test (0 when the condition succeeds). This causes the OpenVPN container to exit cleanly when VPN_DOMAIN is unset, and the on-failure restart policy will not restart it since exit code 0 indicates success.
Suggested fix
- if [ -z "$VPN_DOMAIN" ]; then exit; fi
+ if [ -z "$VPN_DOMAIN" ]; then
+ echo "VPN_DOMAIN is not set; refusing to start OpenVPN." >&2
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -z "$VPN_DOMAIN" ]; then exit; fi | |
| if [ -z "$VPN_DOMAIN" ]; then | |
| echo "VPN_DOMAIN is not set; refusing to start OpenVPN." >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/common/init_command.sh` at line 32, The script currently uses a bare
exit when VPN_DOMAIN is missing, which yields a zero status; change the branch
that checks VPN_DOMAIN (the if [ -z "$VPN_DOMAIN" ] test) to emit a clear error
message to stderr and exit with a non‑zero status (e.g., exit 1) so the OpenVPN
container fails and triggers the on-failure restart policy; update the branch to
echo "VPN_DOMAIN is not set" >&2 (or similar) and then exit 1.
|
Thanks for the review. I have now minimized the changes to only address the core issue (#572). The previous commits included additional shell compatibility adjustments, but those have been reverted to keep the scope focused on handling VPN_DOMAIN safely in the OpenVPN startup. Current change ensures:
Please let me know if any further refinement is needed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Piyushkhobragade
force-pushed
the
issues/572-vpn-name-supervisord
branch
from
06ebe43 to
d2ebbff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/common/utils.sh`:
- Line 244: Shell globbing of "*.pem" can match filenames starting with a dash
which chmod would interpret as options; change the invocation that currently
runs "chmod 600 *.pem" to prefix the glob with "./" (i.e., use "./*.pem") so
dash-prefixed files are treated as filenames, ensuring the command in
images/common/utils.sh modifies only matching files and avoids option-parsing
issues.
- Line 89: The conditional currently uses the string comparison form 'if [ $? =
"0" ]; then' which triggers SC2181; replace this by checking the command
directly or using numeric comparison—either refactor to run the command in the
if (e.g., if <command>; then ...) or change the test to numeric form 'if [ $?
-eq 0 ]; then' where the existing 'if [ $? = "0" ]; then' appears.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f4070f0-b7d5-4a83-a7d9-f8d018c9dc49
📒 Files selected for processing (2)
images/common/init_command.shimages/common/utils.sh
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
📚 Learning: 2026-01-06T11:28:41.789Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
Applied to files:
images/common/init_command.sh
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/init_command.shimages/common/utils.sh
🪛 Shellcheck (0.11.0)
images/common/utils.sh
[style] 89-89: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[info] 244-244: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
🔇 Additional comments (6)
images/common/utils.sh (5)
1-1: LGTM on shebang change.Changing to
#!/bin/bashcorrectly aligns with the bash-specificfunctionkeyword used throughout this file.
100-100: LGTM on POSIX-compliant comparison.Using
=instead of==for string comparison in[ ]is the correct POSIX-compliant approach.
239-242: LGTM on quoted URLs.Properly quoting the URL arguments prevents word splitting on edge-case values in
API_INTERNAL,UUID, andKEY.
245-252: LGTM on config file discovery logic.The
findapproach correctly:
- Locates a newly extracted
.conffile while excluding any existingopenvpn.conf- Renames it to the standardized
openvpn.conffilename- Errors appropriately if no config file is found
This properly addresses the PR objective of decoupling
VPN_NAMEfrom the supervisord config path.
255-258: LGTM on quoted URL incrl_download.The URL is now properly quoted to prevent globbing and word splitting.
images/common/init_command.sh (1)
32-35: LGTM on VPN_DOMAIN guard.The implementation correctly:
- Uses POSIX-compliant
[ -z ... ]syntax- Logs a clear warning message to stderr for debugging
- Exits with status
0to allow graceful skip without triggering theon-failurerestart policy when VPN is intentionally disabledThis aligns with the intentional behavior where the OpenVPN container should not restart when
VPN_DOMAINis unset. Based on learnings: "the OpenVPN container exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy 'on-failure', this prevents the container from restarting when VPN is disabled."
images/common/utils.sh
Outdated
| while :; do | ||
| wget -qS ${DASHBOARD_INTERNAL}/admin/login/ 2>&1 | grep -q "200 OK" | ||
| if [[ $? = "0" ]]; then | ||
| if [ $? = "0" ]; then |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider checking the exit code directly.
Per shellcheck SC2181, capturing $? indirectly can be avoided by checking the command's exit code directly in the conditional.
♻️ Suggested refactor
- wget -qS ${DASHBOARD_INTERNAL}/admin/login/ 2>&1 | grep -q "200 OK"
- if [ $? = "0" ]; then
+ if wget -qS ${DASHBOARD_INTERNAL}/admin/login/ 2>&1 | grep -q "200 OK"; then🧰 Tools
🪛 Shellcheck (0.11.0)
[style] 89-89: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/common/utils.sh` at line 89, The conditional currently uses the string
comparison form 'if [ $? = "0" ]; then' which triggers SC2181; replace this by
checking the command directly or using numeric comparison—either refactor to run
the command in the if (e.g., if <command>; then ...) or change the test to
numeric form 'if [ $? -eq 0 ]; then' where the existing 'if [ $? = "0" ]; then'
appears.
| ${API_INTERNAL}/controller/vpn/checksum/$UUID/?key=$KEY | ||
| "${API_INTERNAL}/controller/vpn/checksum/$UUID/?key=$KEY" | ||
| tar xzf vpn.tar.gz | ||
| chmod 600 *.pem |
There was a problem hiding this comment.
Prefix glob with ./ to prevent dash-prefixed filenames from being interpreted as options.
Per shellcheck SC2035, a filename like -rf.pem would be passed to chmod as an option rather than a file argument.
🛡️ Proposed fix
- chmod 600 *.pem
+ chmod 600 ./*.pem📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chmod 600 *.pem | |
| chmod 600 ./*.pem |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 244-244: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/common/utils.sh` at line 244, Shell globbing of "*.pem" can match
filenames starting with a dash which chmod would interpret as options; change
the invocation that currently runs "chmod 600 *.pem" to prefix the glob with
"./" (i.e., use "./*.pem") so dash-prefixed files are treated as filenames,
ensuring the command in images/common/utils.sh modifies only matching files and
avoids option-parsing issues.
Avoid container failure when VPN_DOMAIN is unset by logging a warning and exiting gracefully. Fixes openwisp#572
Piyushkhobragade
force-pushed
the
issues/572-vpn-name-supervisord
branch
from
d2ebbff to
e3fb79d
Compare
|
Got it, thanks. Those suggestions in utils.sh make sense, but they’re outside the scope of this fix (#572). I’d prefer to keep this PR focused and handle them separately if needed. Let me know if you want me to include them here. |
1 participant
Checklist
Reference to Existing Issue
Closes #572.
Description of Changes
Problem: Supervisord fails to start the OpenVPN process if the
VPN_NAMEenvironment variable contains whitespace.Root Cause: The
VPN_NAMEenvironment variable is directly interpolated into the process manager command without escaping, breaking execution when spaces are present. This creates a tight coupling between the UI-defined object name and internal OS process execution.Fix: Decoupled the internal process management from the environment variable.
supervisord.confto target a staticopenvpn.conffile.openvpn_config_download(utils.sh) to automatically identify and standardize the extracted configuration filename.Side-Effects: Evaluated side effects and system impact; no adverse effects on network routing. The OpenVPN interface remains safely bound to
tun0as configured inopenvpn.json.Screenshot
N/A