[fix] Made OpenVPN container optional #490 #549
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
pandafy
force-pushed
the
issues/490-openvpn-not-required
branch
from
ba41e9a to
46d187c
Compare
github-project-automation
bot
moved this to In progress
in OpenWISP Priorities for next releases
pandafy
force-pushed
the
issues/490-openvpn-not-required
branch
from
46d187c to
92a1eaf
Compare
pandafy
force-pushed
the
issues/490-openvpn-not-required
branch
from
92a1eaf to
eb16b2e
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes implement conditional VPN activation in the deployment configuration. When OpenVPN is disabled (selecting 'n' during installation), VPN-related environment variables are cleared, service dependencies are made optional, and data initialization logic is gated behind VPN-enabled checks to prevent unnecessary service startup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
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
🧹 Nitpick comments (1)
images/openwisp_dashboard/load_init_data.py (1)
245-259: Consider refactoring for clarity.While the logic is technically correct (short-circuit evaluation ensures
default_vpnis only referenced when defined), the code structure could be clearer. Static analyzers and developers might flag the reference todefault_vpnon line 259 when it's defined conditionally on line 248.🔎 Suggested refactors for better clarity
Option 1: Nest topology creation inside VPN initialization block
is_vpn_enabled = os.environ.get("VPN_DOMAIN", "") != "" if is_vpn_enabled: default_ca = create_default_ca() default_cert = create_default_cert(default_ca) default_vpn = create_default_vpn( default_ca, default_cert, ) create_default_vpn_template(default_vpn) + + if env_bool(os.environ.get("USE_OPENWISP_TOPOLOGY")): + Topology = load_model("topology", "Topology") + create_default_topology(default_vpn) create_default_credentials() create_ssh_key_template() -if is_vpn_enabled and env_bool(os.environ.get("USE_OPENWISP_TOPOLOGY")): - Topology = load_model("topology", "Topology") - create_default_topology(default_vpn)Option 2: Initialize default_vpn = None for clarity
is_vpn_enabled = os.environ.get("VPN_DOMAIN", "") != "" +default_vpn = None if is_vpn_enabled: default_ca = create_default_ca() default_cert = create_default_cert(default_ca) default_vpn = create_default_vpn( default_ca, default_cert, ) create_default_vpn_template(default_vpn)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.envdeploy/auto-install.shdocker-compose.ymlimages/openwisp_dashboard/load_init_data.py
🧰 Additional context used
🧬 Code graph analysis (1)
images/openwisp_dashboard/load_init_data.py (1)
images/common/openwisp/utils.py (1)
env_bool(29-30)
🪛 dotenv-linter (4.0.0)
.env
[warning] 61-61: [UnorderedKey] The CELERY_SERVICE_NETWORK_MODE key should go before the CERT_ADMIN_EMAIL key
(UnorderedKey)
🔇 Additional comments (4)
.env (1)
61-61: LGTM! Environment-driven network mode configuration.The new
CELERY_SERVICE_NETWORK_MODEvariable correctly defaults toservice:openvpnfor backward compatibility and will be cleared when VPN is disabled during installation.Note: The static analysis warning about key ordering is cosmetic and can be safely ignored.
docker-compose.yml (1)
10-10: Good! Makes OpenVPN dependency optional.This correctly allows dependent services to start even when OpenVPN is disabled.
images/openwisp_dashboard/load_init_data.py (1)
244-244: Good! Clear VPN enablement check.The
is_vpn_enabledcheck correctly determines VPN status based on theVPN_DOMAINenvironment variable, aligning with the installation script behavior.deploy/auto-install.sh (1)
152-153: Logic is correct; minor style inconsistency noted.The case-insensitive check and variable clearing correctly implement VPN disabling. However, there's minor inconsistency in quoting style (line 152 uses quotes around the variable name, line 153 doesn't).
More importantly, this relates to the concern raised in docker-compose.yml: setting
CELERY_SERVICE_NETWORK_MODEto an empty string may cause issues when Docker Compose evaluatesnetwork_mode: "${CELERY_SERVICE_NETWORK_MODE}". Please verify the behavior or consider using an explicit fallback value like"bridge".
| - .env | ||
| <<: *celery-depends-on | ||
| network_mode: "service:openvpn" | ||
| network_mode: "${CELERY_SERVICE_NETWORK_MODE}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "docker-compose.yml" -o -name "docker-compose.yaml" | head -5Repository: openwisp/docker-openwisp
Length of output: 88
🏁 Script executed:
# Also search for auto-install.sh to understand when CELERY_SERVICE_NETWORK_MODE is set
find . -name "auto-install.sh" | head -5Repository: openwisp/docker-openwisp
Length of output: 92
🏁 Script executed:
# Check if docker-compose or docker compose is available for testing
which docker docker-compose 2>/dev/null || echo "Docker not available in sandbox"Repository: openwisp/docker-openwisp
Length of output: 99
🏁 Script executed:
cat -n ./docker-compose.yml | sed -n '80,105p'Repository: openwisp/docker-openwisp
Length of output: 1055
🏁 Script executed:
grep -n "CELERY_SERVICE_NETWORK_MODE" ./deploy/auto-install.sh -A 2 -B 2Repository: openwisp/docker-openwisp
Length of output: 245
🏁 Script executed:
# Also search for how CELERY_SERVICE_NETWORK_MODE is initialized/used
grep -rn "CELERY_SERVICE_NETWORK_MODE" . --include="*.sh" --include="*.yml"Repository: openwisp/docker-openwisp
Length of output: 288
🏁 Script executed:
# Let's see more context around the VPN configuration to understand the full picture
cat -n ./deploy/auto-install.sh | sed -n '140,165p'Repository: openwisp/docker-openwisp
Length of output: 1091
🌐 Web query:
Docker Compose empty environment variable network_mode behavior
💡 Result:
Short answer: Docker Compose variable interpolation replaces an unset variable with an empty string. For options like network_mode that are set in an earlier file, an empty-string substitution does NOT clear the key — Compose/YAML either treats the empty value as no effective override (so the previous value remains) or the empty value is invalid. In practice this means using ${VAR} where VAR is unset will not reliably "unset" network_mode. See Docker docs on variable interpolation and discussion/workarounds on Stack Overflow/GitHub. [1][2]
Practical options:
- Provide a real value (e.g. bridge) or explicitly set networks: [ default ] in your override to restore default networking. [2][3]
- Use a sentinel string in the override (and special-case it) — many recommend setting network_mode to a non-host dummy value or explicitly configure networks instead. [3]
- Remove the key in the override file (YAML/Compose can’t remove a scalar set in a base file by assigning an empty interpolated value); instead override by specifying networks or a valid network_mode. [3]
Sources:
- Docker Compose interpolation docs (variable substitution behavior). [https://docs.docker.com/compose/how-tos/environment-variables/variable-interpolation/] [1]
- Docker docs — environment variables / set behavior. [https://docs.docker.com/compose/how-tos/environment-variables/set-environment-variables/] [2]
- Community/StackOverflow discussion on unsetting network_mode and override workarounds. [https://stackoverflow.com/questions/69584848/how-to-unset-network-mode-in-an-overriding-docker-compose-file] [3]
🌐 Web query:
Docker Compose network_mode empty string value validation
💡 Result:
Short answer: Docker Compose requires network_mode to be a string (one of the allowed values like "bridge", "host", "none", "service:NAME", "container:ID"). An empty string is not treated as a valid string by the YAML/Compose parser (empty scalar → null), so Compose will reject it. Use either remove/omit network_mode (so Compose uses the default network), set networks: [default], or set a valid network_mode value (for no networking use "none"). [1][2]
Sources:
- Compose specification — network_mode rules and allowed values. [1]
- StackOverflow / user reports showing Compose rejects non-string/empty list and that network_mode must be a string. [2]
Fix invalid network_mode configuration when VPN is disabled.
When VPN is disabled, CELERY_SERVICE_NETWORK_MODE is set to an empty string at lines 85 and 99 in docker-compose.yml, resulting in network_mode: "". Docker Compose treats empty YAML scalars as null and rejects empty string values for network_mode, which will cause container startup failures.
Use Docker Compose conditional syntax to omit the network_mode key entirely when VPN is disabled, or explicitly set it to a valid value like "bridge".
🤖 Prompt for AI Agents
In docker-compose.yml around line 85 (and similarly line 99), network_mode is
set to the variable CELERY_SERVICE_NETWORK_MODE which can be an empty string
when VPN is disabled; update these entries to avoid emitting network_mode: "" by
using Docker Compose conditional interpolation to omit the key when empty or
default to a valid value (e.g., bridge). Concretely, replace the current
assignment with an expression that either expands to nothing (so the
network_mode key is omitted) or uses a default like
"${CELERY_SERVICE_NETWORK_MODE:-bridge}", ensuring docker-compose receives
either a valid mode or no network_mode key at all.
| openvpn: | ||
| image: openwisp/openwisp-openvpn:latest | ||
| restart: always | ||
| restart: on-failure |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Restart policy change doesn't prevent container from starting.
Changing the restart policy from always to on-failure reduces restart attempts when OpenVPN fails, but doesn't prevent the container from starting initially when VPN is disabled. The container will still attempt to start and likely fail due to missing configuration or /dev/net/tun in LXC environments.
Consider using Docker Compose profiles to conditionally include/exclude the openvpn service entirely when VPN is disabled:
🔎 Proposed approach using profiles
In docker-compose.yml:
openvpn:
image: openwisp/openwisp-openvpn:latest
- restart: on-failure
+ restart: always
+ profiles:
+ - vpnThen conditionally enable the profile in the startup command based on whether VPN is enabled.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docker-compose.yml at line 171, changing the restart policy to on-failure
does not prevent the openvpn service from attempting to start when VPN is
disabled; instead add a Docker Compose profile to the openvpn service (e.g.,
service key "profiles: ['openvpn']") so the service is excluded by default,
update any documentation/startup scripts to include the profile when VPN is
enabled (docker-compose --profile openvpn up) and ensure any
orchestration/launch scripts set that flag based on the VPN-enabled config; this
removes the service from the default composition and prevents container start
when VPN is disabled.
Checklist
Reference to Existing Issue
Fixes #490
TODOS
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.