[env] Update VPN environment variables to OPENVPN for WireGuard suppo…#520
Conversation
openwisp#227 Changed VPN_* environment variables to OPENVPN_* to distinguish them from upcoming WireGuard VPN configurations. This prepares the codebase for WireGuard support by making OpenVPN-specific variables clearly identifiable. Changes: - VPN_DOMAIN → OPENVPN_DOMAIN - VPN_NAME → OPENVPN_NAME - VPN_CLIENT_NAME → OPENVPN_CLIENT_NAME Updated across configuration files, Docker images, scripts, and documentation. Fixes openwisp#227
📝 WalkthroughWalkthroughThis pull request systematically renames environment variables across the codebase from generic Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deploy/auto-install.sh (1)
149-156: Use OPENVPN_DOMAIN in all branches.The default and “disable” branches still write
VPN_DOMAIN, so.envnever updatesOPENVPN_DOMAINwhen the user leaves it blank or disables it. This breaks the rename and keeps stale OpenVPN settings.🐛 Proposed fix
- if [[ -z "$vpn_domain" ]]; then - set_env "VPN_DOMAIN" "openvpn.${domain}" + if [[ -z "$vpn_domain" ]]; then + set_env "OPENVPN_DOMAIN" "openvpn.${domain}" set_env CELERY_SERVICE_NETWORK_MODE "service:openvpn" elif [[ "${vpn_domain,,}" == "n" ]]; then - set_env "VPN_DOMAIN" "" + set_env "OPENVPN_DOMAIN" "" set_env CELERY_SERVICE_NETWORK_MODE "" else set_env "OPENVPN_DOMAIN" "$vpn_domain" fiimages/openwisp_dashboard/load_init_data.py (1)
241-245: Update VPN enablement check to OPENVPN_DOMAIN.
is_vpn_enabledstill readsVPN_DOMAIN, so VPN setup will be skipped when onlyOPENVPN_DOMAINis set.🐛 Proposed fix
- is_vpn_enabled = os.environ.get("VPN_DOMAIN", "") != "" + is_vpn_enabled = os.environ.get("OPENVPN_DOMAIN", "") != ""
🤖 Fix all issues with AI agents
In `@docs/user/settings.rst`:
- Around line 58-61: Rename the reST anchor .. _vpn_domain: to ..
_openvpn_domain: in the settings documentation and update the corresponding
cross-reference :ref:`OpenVPN <vpn_domain>` to :ref:`OpenVPN <openvpn_domain>`
so the anchor name matches the environment variable OPENVPN_DOMAIN and the
reference resolves correctly.
In `@images/common/init_command.sh`:
- Line 32: The test using bash-only [[ ]] around OPENVPN_DOMAIN is incompatible
with the /bin/sh shebang; replace the conditional in the init script that checks
OPENVPN_DOMAIN (the line using [[ -z "$OPENVPN_DOMAIN" ]]) with a
POSIX-compatible test using [ -z "$OPENVPN_DOMAIN" ] and ensure proper quoting
and an exit (e.g., exit 1) remains after the test to preserve behavior in the
init_command.sh flow.
🧹 Nitpick comments (1)
docs/user/settings.rst (1)
999-1005: Update description for consistency.For consistency with the renaming and the description of
OPENVPN_NAME(line 994), line 1002 should say "Name of the OpenVPN client template" instead of "Name of the VPN client template".📝 Suggested fix
-- **Explanation:** Name of the VPN client template that will be visible on +- **Explanation:** Name of the OpenVPN client template that will be visible on the OpenWISP dashboard.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.envdeploy/auto-install.shdocs/user/settings.rstimages/common/init_command.shimages/openwisp_base/Dockerfileimages/openwisp_dashboard/load_init_data.pyimages/openwisp_openvpn/supervisord.conf
🧰 Additional context used
🧠 Learnings (1)
📚 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.envimages/openwisp_base/Dockerfiledeploy/auto-install.shimages/common/init_command.sh
🪛 dotenv-linter (4.0.0)
.env
[warning] 10-10: [UnorderedKey] The OPENVPN_DOMAIN key should go before the SSH_PRIVATE_KEY_PATH key
(UnorderedKey)
[warning] 46-46: [UnorderedKey] The OPENVPN_NAME key should go before the OPENWISP_GEOCODING_CHECK key
(UnorderedKey)
[warning] 47-47: [UnorderedKey] The OPENVPN_CLIENT_NAME key should go before the OPENVPN_DOMAIN key
(UnorderedKey)
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 32-32: In POSIX sh, [[ ]] is undefined.
(SC3010)
⏰ 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 comments (5)
images/openwisp_openvpn/supervisord.conf (1)
21-21: Rename looks consistent.Switching to
ENV_OPENVPN_NAMEaligns with the new env naming.images/openwisp_base/Dockerfile (1)
154-157: LGTM.OpenVPN env defaults are renamed consistently.
.env (1)
10-10: LGTM.Env key renames are consistent with the new OpenVPN naming.
Also applies to: 45-47
docs/user/settings.rst (2)
28-28: LGTM!The prefix list correctly updated to reflect the OpenVPN-specific naming convention.
991-997: LGTM!The section correctly renamed and the explanation properly specifies "OpenVPN Server" for clarity.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| .. _vpn_domain: | ||
|
|
||
| ``VPN_DOMAIN`` | ||
| ~~~~~~~~~~~~~~ | ||
| ``OPENVPN_DOMAIN`` | ||
| ~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the current state of the file around lines 58-61
sed -n '55,70p' docs/user/settings.rstRepository: openwisp/docker-openwisp
Length of output: 446
🏁 Script executed:
# Search for cross-references to vpn_domain anchor
rg -n ':ref:`.*vpn_domain.*`' --glob '*.rst'Repository: openwisp/docker-openwisp
Length of output: 50
🏁 Script executed:
# Search for any references to vpn_domain in the codebase
rg -n 'vpn_domain' --glob '*.rst'Repository: openwisp/docker-openwisp
Length of output: 163
🏁 Script executed:
# Check if openvpn_domain anchor or references already exist
rg -n 'openvpn_domain' --glob '*.rst'Repository: openwisp/docker-openwisp
Length of output: 50
🏁 Script executed:
# Examine the context around the vpn_domain reference in quickstart.rst
sed -n '50,65p' docs/user/quickstart.rstRepository: openwisp/docker-openwisp
Length of output: 897
Update the reStructuredText anchor and cross-reference to match the variable name.
The anchor on line 58 of docs/user/settings.rst uses .. _vpn_domain: while the environment variable is named OPENVPN_DOMAIN. This inconsistency should be resolved by renaming the anchor to .. _openvpn_domain:. Additionally, the cross-reference in docs/user/quickstart.rst:58 that uses :ref:OpenVPN <vpn_domain> must be updated to `:ref:`OpenVPN <openvpn_domain> to maintain the working reference.
Changes required
In docs/user/settings.rst line 58:
-.. _vpn_domain:
+.. _openvpn_domain:In docs/user/quickstart.rst line 58:
-<vpn_domain>` services.
+<openvpn_domain>` services.🤖 Prompt for AI Agents
In `@docs/user/settings.rst` around lines 58 - 61, Rename the reST anchor ..
_vpn_domain: to .. _openvpn_domain: in the settings documentation and update the
corresponding cross-reference :ref:`OpenVPN <vpn_domain>` to :ref:`OpenVPN
<openvpn_domain>` so the anchor name matches the environment variable
OPENVPN_DOMAIN and the reference resolves correctly.
| fi | ||
| elif [ "$MODULE_NAME" = 'openvpn' ]; then | ||
| if [[ -z "$VPN_DOMAIN" ]]; then exit; fi | ||
| if [[ -z "$OPENVPN_DOMAIN" ]]; then exit; fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the shebang and content of the file
head -n 35 images/common/init_command.sh | cat -nRepository: openwisp/docker-openwisp
Length of output: 1216
Fix POSIX shell incompatibility: use [ ] instead of [[ ]].
Line 32 uses bash-specific [[ ]] test syntax, but the shebang is /bin/sh. This will cause a syntax error in dash (the default /bin/sh in most container environments). Replace with the POSIX-compatible [ -z "$OPENVPN_DOMAIN" ].
Suggested fix
- if [[ -z "$OPENVPN_DOMAIN" ]]; then exit; fi
+ if [ -z "$OPENVPN_DOMAIN" ]; then exit; 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 "$OPENVPN_DOMAIN" ]]; then exit; fi | |
| if [ -z "$OPENVPN_DOMAIN" ]; then exit; fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 32-32: In POSIX sh, [[ ]] is undefined.
(SC3010)
🤖 Prompt for AI Agents
In `@images/common/init_command.sh` at line 32, The test using bash-only [[ ]]
around OPENVPN_DOMAIN is incompatible with the /bin/sh shebang; replace the
conditional in the init script that checks OPENVPN_DOMAIN (the line using [[ -z
"$OPENVPN_DOMAIN" ]]) with a POSIX-compatible test using [ -z "$OPENVPN_DOMAIN"
] and ensure proper quoting and an exit (e.g., exit 1) remains after the test to
preserve behavior in the init_command.sh flow.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks @Adityashandilya555 for preparing the ground for WireGuard support. The disambiguation makes sense, but there are two problems: a concrete bug in the rename, and a backward compatibility concern that I think changes how we should land this.
Bug: the rename in deploy/auto-install.sh is incomplete
auto-install.sh has three set_env "VPN_DOMAIN" ... calls in master:
set_env "VPN_DOMAIN" "openvpn."(VPN enabled)set_env "VPN_DOMAIN" ""(VPN disabled)set_env "VPN_DOMAIN" ""(custom domain)
This PR only renames the last one to OPENVPN_DOMAIN. The other two still write the old VPN_DOMAIN. Since the rest of the stack now reads OPENVPN_DOMAIN, the effects are real:
- The "disable VPN" path sets
VPN_DOMAIN=""but leavesOPENVPN_DOMAINat its.envdefault, soinit_command.sh'sif [[ -z "" ]]; then exit; fidoes not trigger and the VPN is not actually disabled. - The enabled path writes the user's domain into
VPN_DOMAIN, which nothing reads anymore, soOPENVPN_DOMAINstays at the default.
Please rename all occurrences. A grep -rn 'VPN_DOMAIN\|VPN_NAME\|VPN_CLIENT_NAME' that returns nothing outside of intentional backward-compat handling should be the bar.
Backward compatibility: this breaks existing deployments silently
Every existing install has VPN_DOMAIN / VPN_NAME / VPN_CLIENT_NAME in its .env. After this rename, those keys are ignored and the new OPENVPN_* defaults take over with no warning. For a config-driven project that is a rough upgrade.
Two acceptable ways forward:
- Read the new names with a fallback to the old ones during a deprecation window, and document the deprecation.
- Or hold this rename and land it together with the actual WireGuard support (#227), with a clear upgrade note in the release.
A standalone breaking rename with no WireGuard feature attached and no fallback is the worst of both worlds, so I would not merge it in that form.
Fix the auto-install occurrences and add the fallback (or bundle with #227), and this becomes mergeable.
|
Hi @Adityashandilya555 👋, This is a friendly reminder that this pull request has had no activity for 7 days since changes were requested. We'd love to see this contribution merged! Please take a moment to:
If you're busy or need more time, no worries! Just leave a comment to let us know you're still working on it. Note: within 7 more days, the linked issue will be unassigned to allow other contributors to work on it. Thank you for your contribution! 🙏 |
|
Hi @Adityashandilya555 👋, This pull request has been marked as stale due to 14 days of inactivity after changes were requested. As a result, any linked issues are being unassigned from you so other contributors can pick them up. However, you can still continue working on this PR! If you push new commits or respond to the review feedback:
If you need more time or have questions about the requested changes, please let us know. We're happy to help! 🤝 |
2 participants
…rt #227
Changed VPN_* environment variables to OPENVPN_* to distinguish them from upcoming WireGuard VPN configurations. This prepares the codebase for WireGuard support by making OpenVPN-specific variables clearly identifiable.
Checklist
Reference to Existing Issue
Closes #227
Description of Changes
Updated across configuration files, Docker images, scripts, and documentation.
Screenshot
![Screenshot 2025-10-06 at 10 37 28 PM]()
-bd9005bea847" />