[openvpn] Decouple VPN_NAME from openvpn process config #572#578
[openvpn] Decouple VPN_NAME from openvpn process config #572#578Piyushkhobragade wants to merge 5 commits intoopenwisp:masterfrom
Conversation
- Hardcoded supervisord to target openvpn.conf - Added dynamic discovery in utils.sh to rename downloaded OpenVPN configs - Resolves supervisord crash when VPN_NAME contains whitespace - Fixes openwisp#572
📝 WalkthroughWalkthroughThe supervisord OpenVPN program was changed to use a fixed config filename Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
images/common/utils.sh
Outdated
| CONF_FILE=$(ls *.conf 2>/dev/null | head -n 1) | ||
| if [ -n "$CONF_FILE" ] && [ "$CONF_FILE" != "openvpn.conf" ]; then | ||
| mv "$CONF_FILE" openvpn.conf | ||
| fi |
There was a problem hiding this comment.
Config discovery can keep stale openvpn.conf and ignore the newly downloaded profile.
At Line 248, picking the first *.conf can select an existing openvpn.conf from an earlier run, so the freshly extracted file may never become active. With images/openwisp_openvpn/openvpn.sh restarting OpenVPN after download, this can apply a restart without applying the new config.
Proposed fix
- CONF_FILE=$(ls *.conf 2>/dev/null | head -n 1)
- if [ -n "$CONF_FILE" ] && [ "$CONF_FILE" != "openvpn.conf" ]; then
- mv "$CONF_FILE" openvpn.conf
- fi
+ # Prefer a newly extracted non-standard filename and normalize it.
+ CONF_FILE=$(find . -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
+ echo "ERROR: no OpenVPN config file found after extraction" >&2
+ return 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.
| CONF_FILE=$(ls *.conf 2>/dev/null | head -n 1) | |
| if [ -n "$CONF_FILE" ] && [ "$CONF_FILE" != "openvpn.conf" ]; then | |
| mv "$CONF_FILE" openvpn.conf | |
| fi | |
| # Prefer a newly extracted non-standard filename and normalize it. | |
| CONF_FILE=$(find . -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 | |
| echo "ERROR: no OpenVPN config file found after extraction" >&2 | |
| return 1 | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[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)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/common/utils.sh` around lines 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.
- Replaced ls with find for safer file discovery. - Handled edge case where stale openvpn.conf could be selected. - Updated logic per CodeRabbit review recommendations. Fixes openwisp#572
- Implemented test_openvpn_config_whitespace_handling in runtests.py - Mocked bash utilities to verify file renaming logic without network overhead - Addressed final CodeRabbit review requirement Fixes openwisp#572
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.
- Updated u_path resolution to be independent of cwd. - Quoted sourced path to prevent shell breakage. - Added explicit returncode assertion for better diagnostics. - Addressed final CodeRabbit review feedback. Fixes openwisp#572
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.
| import os | ||
| import shlex | ||
| import subprocess | ||
| import tempfile |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant imports.
os and subprocess are already imported at the module level (lines 1-2). Only shlex and tempfile are needed here.
Proposed fix
def test_openvpn_config_whitespace_handling(self):
"""Verify openvpn_config_download handles whitespace."""
- import os
import shlex
- import subprocess
import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/runtests.py` around lines 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.
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