Skip to content

[fix] Made OpenVPN container optional #490 #549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
pandafy wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[fix] Made OpenVPN container optional #490 #549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ OPENWISP_CELERY_MONITORING_COMMAND_FLAGS=--concurrency=1
OPENWISP_CELERY_MONITORING_CHECKS_COMMAND_FLAGS=--concurrency=1
USE_OPENWISP_CELERY_FIRMWARE=True
OPENWISP_CELERY_FIRMWARE_COMMAND_FLAGS=--concurrency=1
CELERY_SERVICE_NETWORK_MODE=service:openvpn
# Metric collection
METRIC_COLLECTION=True
# collectstatic
Expand Down
3 changes: 2 additions & 1 deletion deploy/auto-install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ setup_docker_openwisp() {
if [[ -z "$vpn_domain" ]]; then
set_env "VPN_DOMAIN" "openvpn.${domain}"
elif [[ "${vpn_domain,,}" == "n" ]]; then
set_env "VPN_DOMAIN" "example.com"
set_env "VPN_DOMAIN" ""
set_env CELERY_SERVICE_NETWORK_MODE ""
else
set_env "VPN_DOMAIN" "$vpn_domain"
fi
Expand Down
7 changes: 4 additions & 3 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ x-celery-depends-on: &celery-depends-on
dashboard:
condition: service_started
openvpn:
required: false
condition: service_healthy

services:
Expand Down Expand Up @@ -81,7 +82,7 @@ services:
env_file:
- .env
<<: *celery-depends-on
network_mode: "service:openvpn"
network_mode: "${CELERY_SERVICE_NETWORK_MODE}"
Copy link

@coderabbitai coderabbitai bot Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "docker-compose.yml" -o -name "docker-compose.yaml" | head -5

Repository: 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 -5

Repository: 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 2

Repository: 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:

  1. Docker Compose interpolation docs (variable substitution behavior). [https://docs.docker.com/compose/how-tos/environment-variables/variable-interpolation/] [1]
  2. Docker docs — environment variables / set behavior. [https://docs.docker.com/compose/how-tos/environment-variables/set-environment-variables/] [2]
  3. 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.


celery_monitoring:
image: openwisp/openwisp-dashboard:latest
Expand All @@ -95,7 +96,7 @@ services:
env_file:
- .env
<<: *celery-depends-on
network_mode: "service:openvpn"
network_mode: "${CELERY_SERVICE_NETWORK_MODE}"

celerybeat:
image: openwisp/openwisp-dashboard:latest
Expand Down Expand Up @@ -167,7 +168,7 @@ services:

openvpn:
image: openwisp/openwisp-openvpn:latest
restart: always
restart: on-failure
Copy link

@coderabbitai coderabbitai bot Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:
+    - vpn

Then 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.

build:
context: images
dockerfile: openwisp_openvpn/Dockerfile
Expand Down
18 changes: 10 additions & 8 deletions images/openwisp_dashboard/load_init_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,18 +241,20 @@ def create_default_topology(vpn):
create_admin()
# Steps for creating new vpn client template with all the
# required objects (CA, Certificate, VPN Server).
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)
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)

create_default_credentials()
create_ssh_key_template()

if env_bool(os.environ.get("USE_OPENWISP_TOPOLOGY")):
if is_vpn_enabled and env_bool(os.environ.get("USE_OPENWISP_TOPOLOGY")):
Topology = load_model("topology", "Topology")
create_default_topology(default_vpn)

Expand Down