[change] Avoid unnecessary CRL restarts when revokelist content is unchanged #589#610
[change] Avoid unnecessary CRL restarts when revokelist content is unchanged #589#6104f4d wants to merge 5 commits into
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:
📝 WalkthroughWalkthroughExtracts OpenVPN helpers into images/openwisp_openvpn/openvpn_utils.sh, moves cron entries into images/openwisp_openvpn/openvpn.crontab (installed at startup), updates init_command.sh, openvpn.sh, and revokelist.sh to source the new utilities, implements crl_download_if_changed to replace revoked.crl only on content changes and restart OpenVPN conditionally, renames a Dockerfile env var, and adds integration tests for revokelist and config-update behavior. Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 63-71: Replace the fixed temporary filename used for CRL updates
to avoid races: in the code around local tmp_crl and calls to crl_download_to,
create a unique temp file (e.g., via mktemp or appending $$/timestamp) and
assign it to tmp_crl, use that variable for the download, comparison (cmp -s
"$tmp_crl" revoked.crl) and mv, and register a trap/cleanup to unlink tmp_crl on
failure/exit; update references to tmp_crl (the local variable and the
crl_download_to call) so all cleanup and the final move operate on the unique
temp file.
- Line 86: The cron line in openvpn_utils.sh uses a misspelled environment
variable (TOPLOGY_UPDATE_INTERVAL) causing an empty/invalid cron entry; update
the variable name to TOPLOGY_UPDATE_INTERVAL (matching the docs) in the echo
that writes the cron job and also fix the matching typo in the Dockerfile where
the variable is set so both use TOPLOGY_UPDATE_INTERVAL; ensure the cron string
still exports TOPOLOGY_UUID and TOPOLOGY_KEY as before and validate the
resulting cron syntax after the change.
- Around line 3-6: In get_redis_value(), replace the POSIX-incompatible echo -en
framing with a printf-based framing so the Redis GET request correctly includes
CRLF on ash/Alpine; use printf with a format string that inserts the key and
emits "\r\n" (e.g., printf 'GET %s\r\n' "$key") piped to nc redis 6379, keeping
the existing awk extraction unchanged.
🪄 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: 9bc3b838-a66f-49ed-9fab-270ec767ba56
📒 Files selected for processing (6)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.sh
💤 Files with no reviewable changes (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 (1)
📚 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
🪛 Shellcheck (0.11.0)
images/openwisp_openvpn/openvpn.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/revokelist.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 5-5: In POSIX sh, echo flags are undefined.
(SC3037)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-38: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 35-36: Quote this to prevent word splitting.
(SC2046)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
[warning] 40-48: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 47-47: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
[warning] 50-54: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 51-51: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 56-60: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 62-77: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 63-63: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 79-88: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 81-81: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 81-81: Quote this to prevent word splitting.
(SC2046)
[warning] 82-82: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 82-82: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (4)
images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/common/init_command.sh (1)
33-39: LGTM!images/openwisp_openvpn/openvpn.sh (1)
7-7: LGTM!images/openwisp_openvpn/revokelist.sh (1)
7-17: LGTM!
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
4944e57 to
4d2edf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 3-6: The get_redis_value function currently calls `nc redis 6379`
with no timeout; modify get_redis_value to run the netcat call with a bounded
timeout (e.g. using `timeout 5 nc redis 6379` or `nc -w 5 redis 6379`) and
detect a failure so the function returns an empty string (or non-zero) instead
of blocking; ensure you still pipe the successful response into `awk 'NR==2
{gsub(/\r/, ""); print}'` and handle the case where `timeout`/`nc` exits
non-zero by not printing garbage or hanging.
- Around line 34-48: The download/checksum functions openvpn_config_checksum and
openvpn_config_download don't fail fast or validate outputs; update them to
check curl exit statuses and non-empty outputs, aborting on failure: for
openvpn_config_checksum ensure the curl call succeeds and OFILE is non-empty
(exit or retry if not), and for openvpn_config_download verify each curl exit
code and that vpn.tar.gz and checksum files have expected size/content before
extracting; after tar xzf, verify the extraction succeeded and produced the
required .pem files and set strict permissions, failing (exit non-zero)
immediately on any validation error.
In `@tests/runtests.py`:
- Around line 465-469: After generating {test_dir}/revokelist.sh with the sed
substitutions in tests/runtests.py, add a quick post-transform assertion that
the output contains the expected stubbed lines (e.g. "cd {test_dir}/work" and
"source {test_dir}/openvpn_utils.sh" or the ":" replacement for "source
/utils.sh") and does not still contain the original patterns ("cd /" or "source
/utils.sh"). Locate the sed block that writes /revokelist.sh and, immediately
after writing the file, read {test_dir}/revokelist.sh and fail fast
(raise/assert with a clear message) if those expected replacements are not
present so the test deterministically fails when the sed patch stops matching.
🪄 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: e3122d9f-e85e-40f3-b293-e662291362fa
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 Learnings (1)
📚 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
🪛 Shellcheck (0.11.0)
images/openwisp_openvpn/openvpn.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/revokelist.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-38: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 35-36: Quote this to prevent word splitting.
(SC2046)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
[warning] 40-48: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 47-47: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
[warning] 50-54: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 51-51: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 56-60: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 62-82: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 63-63: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 84-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 86-86: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 86-86: Quote this to prevent word splitting.
(SC2046)
[warning] 87-87: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 87-87: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (5)
images/openwisp_openvpn/Dockerfile (1)
28-28: ⚡ Quick winThe typo fix is correct and consistently applied throughout the codebase.
The change from
TOPLOGY_UPDATE_INTERVALtoTOPOLOGY_UPDATE_INTERVALhas been properly integrated. No references to the old misspelling remain in the codebase, and the corrected variable name is consistently used in both the environment definition (Dockerfile), the consuming script (openvpn_utils.sh), and documentation (settings.rst).images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/common/init_command.sh (1)
33-39: LGTM!images/openwisp_openvpn/openvpn.sh (1)
7-7: LGTM!images/openwisp_openvpn/revokelist.sh (1)
7-17: LGTM!
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
2 times, most recently
from
86f364c to
f715d95
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
images/openwisp_openvpn/openvpn_utils.sh (2)
34-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on checksum/config download errors before applying artifacts.
These curl/tar/chmod steps currently continue on transient or malformed responses, which can trigger incorrect restart behavior.
Suggested fix
function openvpn_config_checksum { - export OFILE=$(curl --silent --insecure \ - ${API_INTERNAL}/controller/vpn/checksum/$UUID/?key=$KEY) - export NFILE=$(cat checksum) + OFILE="$(curl --silent --show-error --fail --insecure \ + "${API_INTERNAL}/controller/vpn/checksum/${UUID}/?key=${KEY}")" || return 1 + [ -n "$OFILE" ] || return 1 + NFILE="$(cat checksum 2>/dev/null)" || return 1 } function openvpn_config_download { - curl --silent --retry 10 --retry-delay 5 --retry-max-time 300\ - --insecure --output vpn.tar.gz \ - ${API_INTERNAL}/controller/vpn/download-config/$UUID/?key=$KEY - curl --silent --insecure --output checksum \ - ${API_INTERNAL}/controller/vpn/checksum/$UUID/?key=$KEY - tar xzf vpn.tar.gz - chmod 600 *.pem + curl --silent --show-error --fail --retry 10 --retry-delay 5 --retry-max-time 300 \ + --insecure --output vpn.tar.gz \ + "${API_INTERNAL}/controller/vpn/download-config/${UUID}/?key=${KEY}" || return 1 + curl --silent --show-error --fail --insecure --output checksum \ + "${API_INTERNAL}/controller/vpn/checksum/${UUID}/?key=${KEY}" || return 1 + tar xzf vpn.tar.gz || return 1 + chmod 600 -- ./*.pem || return 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@images/openwisp_openvpn/openvpn_utils.sh` around lines 34 - 47, The openvpn_config_checksum and openvpn_config_download functions do not fail fast on curl/tar/chmod failures or on empty/malformed responses; update them so each external command is checked and the function exits non‑zero immediately on error: in openvpn_config_checksum verify the curl exit code and that OFILE is non-empty and well-formed before exporting, and in openvpn_config_download check the exit codes of both curl calls, validate the downloaded checksum file (NFILE) and compare it to OFILE (or compute a local checksum) before running tar xzf and chmod, and ensure tar and chmod errors are caught and cause an immediate exit; reference these functions (openvpn_config_checksum, openvpn_config_download) when adding these validations and early returns.
5-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to Redis reads to avoid hanging jobs.
nc redis 6379can block indefinitely and stall init/cron flows; use a bounded timeout and fail cleanly.Suggested fix
- printf 'GET %s\r\n' "$key" | nc redis 6379 | awk 'NR==2 {gsub(/\r/, ""); print}' + printf 'GET %s\r\n' "$key" | nc -w 5 redis 6379 | awk 'NR==2 {gsub(/\r/, ""); print}'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@images/openwisp_openvpn/openvpn_utils.sh` at line 5, The Redis read currently uses an unbounded netcat call ("printf ... | nc redis 6379 | awk ...") which can hang; wrap the nc call with a bounded timeout (e.g. use the timeout utility: "timeout 5s nc redis 6379" or netcat options like "nc -w 5 -q 1") and ensure the pipeline handles non-zero exit (treat timeout as a failure) so the script fails cleanly instead of hanging; update the line containing printf ... | nc redis 6379 | awk 'NR==2 {gsub(/\r/, ""); print}' to use the timeout-wrapped nc and propagate or handle its non-zero exit accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Line 91: The cron line writer currently echoes a crontab entry using
TOPOLOGY_UPDATE_INTERVAL without validation; add a guard before writing that
ensures TOPOLOGY_UPDATE_INTERVAL is non-empty and a positive integer (e.g.,
regex check like '^[0-9]+$'), and if invalid either skip creating the cron entry
and log a clear error/warning or fall back to a safe default interval; apply
this validation in the same scope that references TOPOLOGY_UPDATE_INTERVAL,
TOPOLOGY_UUID, TOPOLOGY_KEY and the /send-topology.sh cron line so the script
never writes an invalid cron entry.
In `@tests/runtests.py`:
- Around line 479-480: The test currently executes revokelist.sh with sh which
can break because the rewritten script uses the Bash-only builtin source; update
the invocation in tests/runtests.py so the command runs under bash (i.e.,
replace the use of "sh {test_dir}/revokelist.sh" with "bash
{test_dir}/revokelist.sh") so that the sourced openvpn_utils.sh and other Bash
semantics used by revokelist.sh execute correctly; ensure the environment prefix
(CRL_TEST_MODE and PATH adjustments) remains unchanged and still applies to the
bash invocation.
---
Duplicate comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 34-47: The openvpn_config_checksum and openvpn_config_download
functions do not fail fast on curl/tar/chmod failures or on empty/malformed
responses; update them so each external command is checked and the function
exits non‑zero immediately on error: in openvpn_config_checksum verify the curl
exit code and that OFILE is non-empty and well-formed before exporting, and in
openvpn_config_download check the exit codes of both curl calls, validate the
downloaded checksum file (NFILE) and compare it to OFILE (or compute a local
checksum) before running tar xzf and chmod, and ensure tar and chmod errors are
caught and cause an immediate exit; reference these functions
(openvpn_config_checksum, openvpn_config_download) when adding these validations
and early returns.
- Line 5: The Redis read currently uses an unbounded netcat call ("printf ... |
nc redis 6379 | awk ...") which can hang; wrap the nc call with a bounded
timeout (e.g. use the timeout utility: "timeout 5s nc redis 6379" or netcat
options like "nc -w 5 -q 1") and ensure the pipeline handles non-zero exit
(treat timeout as a failure) so the script fails cleanly instead of hanging;
update the line containing printf ... | nc redis 6379 | awk 'NR==2 {gsub(/\r/,
""); print}' to use the timeout-wrapped nc and propagate or handle its non-zero
exit accordingly.
🪄 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: a6812144-d1fb-49ab-a7e1-3540b519df2e
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 Learnings (1)
📚 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
🪛 Shellcheck (0.11.0)
images/openwisp_openvpn/openvpn.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/revokelist.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-38: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 35-36: Quote this to prevent word splitting.
(SC2046)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
[warning] 40-48: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 47-47: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
[warning] 50-54: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 51-51: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 56-60: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 62-82: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 63-63: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 84-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 86-86: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 86-86: Quote this to prevent word splitting.
(SC2046)
[warning] 87-87: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 87-87: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (5)
images/openwisp_openvpn/Dockerfile (1)
28-28: ⚡ Quick winSpelling correction is correct and safe. The change from
TOPLOGY_UPDATE_INTERVALtoTOPOLOGY_UPDATE_INTERVALfixes a typo. No code references the misspelled name, and the correct spelling is already used inopenvpn_utils.sh(line 91) where the variable is actually referenced.images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
images/openwisp_openvpn/openvpn.sh (1)
7-7: LGTM!images/openwisp_openvpn/revokelist.sh (1)
7-7: LGTM!Also applies to: 10-17
4f4d
changed the title
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
f715d95 to
0340a4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 125-132: init_send_network_topology currently writes a cron entry
even when TOPOLOGY_UUID or TOPOLOGY_KEY are empty; change the logic so the cron
is only installed when both credentials are present: after loading/exporting
TOPOLOGY_UUID and TOPOLOGY_KEY (symbols TOPOLOGY_UUID, TOPOLOGY_KEY), test that
both are non-empty (e.g. [ -n "$TOPOLOGY_UUID" ] && [ -n "$TOPOLOGY_KEY" ])
before composing the crontab entry that uses TOPOLOGY_UPDATE_INTERVAL and
/send-topology.sh; if either is empty, skip creating the cron and avoid writing
an invalid job.
In `@images/openwisp_openvpn/revokelist.sh`:
- Line 7: The script revokelist.sh uses the Bash-only "source" builtin (e.g.
lines calling source /openvpn_utils.sh), which breaks when /bin/sh is dash/ash;
replace each use of "source" with the POSIX-compliant "." operator (e.g. ".
/openvpn_utils.sh") so the CRL refresh runs under /bin/sh; update every
occurrence of source in revokelist.sh (and any other scripts using source) to
the dot-sourcing form to ensure compatibility.
🪄 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: 1819afc9-76f6-46f1-86cc-1a36a73b4324
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 Learnings (1)
📚 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
🪛 Shellcheck (0.11.0)
images/openwisp_openvpn/openvpn.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/revokelist.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-51: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 53-81: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 54-54: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 55-55: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 83-87: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 84-84: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 89-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 95-115: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 96-96: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 117-133: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 126-126: Quote this to prevent word splitting.
(SC2046)
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 127-127: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (7)
images/openwisp_openvpn/Dockerfile (1)
28-28: No remaining references to the old variable name exist.The environment variable name correction from
TOPLOGY_UPDATE_INTERVALtoTOPOLOGY_UPDATE_INTERVALis valid and complete. Verification confirms all references inopenvpn_utils.shalready use the corrected spelling, and there are no lingering references to the old misspelled name in the codebase. The variable is also properly documented.images/openwisp_openvpn/openvpn_utils.sh (1)
3-123: LGTM!Also applies to: 133-134
images/common/init_command.sh (1)
33-39: LGTM!images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/openvpn.sh (1)
7-19: LGTM!images/openwisp_openvpn/revokelist.sh (1)
10-17: LGTM!tests/runtests.py (1)
7-7: LGTM!Also applies to: 315-320, 423-527, 530-546
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
0340a4c to
cf0c09c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 62-80: The mv of the downloaded checksum isn't checked so
openvpn_config_download can return success even if mv fails; update the mv
"$tmp_checksum" checksum operation to propagate failure (add || return 1) and
ensure the function uses the existing tmp_checksum/tmp_tar cleanup and trap
logic (symbols: openvpn_config_download, tmp_checksum, tmp_tar, mv
"$tmp_checksum" checksum, trap 'rm -f ...' EXIT HUP INT TERM) so any mv failure
causes the function to return 1.
🪄 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: 53dff560-0432-4cfe-a4f0-868033fe248d
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 Learnings (1)
📚 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-51: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 53-81: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 54-54: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 55-55: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 83-87: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 84-84: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 89-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 95-115: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 96-96: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 117-137: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 126-126: Quote this to prevent word splitting.
(SC2046)
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 127-127: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (9)
images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!images/openwisp_openvpn/revokelist.sh (1)
6-17: LGTM!tests/runtests.py (3)
7-7: LGTM!Also applies to: 315-322
423-531: LGTM!
534-551: LGTM!images/openwisp_openvpn/openvpn_utils.sh (1)
3-51: LGTM!Also applies to: 83-137
images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/openvpn.sh (1)
6-19: LGTM!images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 106-109: The mv of "$tmp_crl" to revoked.crl is not checked so
failures still return success; modify the block that handles tmp_crl promotion
(the mv "$tmp_crl" revoked.crl call) to check mv's exit status and, on failure,
log an error (or echo to stderr), remove or preserve the tmp file as
appropriate, restore any trap state, and return a non-zero status instead of 0;
ensure you still clear the trap (trap - EXIT HUP INT TERM) only after a
successful mv to avoid reporting a false "changed" state.
🪄 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: e696c45d-65b8-4356-9602-6c8c7fe5cc17
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 Learnings (1)
📚 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-51: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 53-81: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 54-54: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 55-55: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 83-87: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 84-84: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 89-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 95-115: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 96-96: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 117-137: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 126-126: Quote this to prevent word splitting.
(SC2046)
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 127-127: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (9)
images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/openvpn.sh (1)
6-7: LGTM!Also applies to: 10-19
images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
images/openwisp_openvpn/revokelist.sh (1)
6-17: LGTM!tests/runtests.py (4)
7-7: LGTM!
315-322: LGTM!
534-551: LGTM!
462-466: The conversion fromcrl_download_toreturn code 1 to exit status 2 is correctly implemented.The
crl_download_if_changedfunction (lines 101–103 inopenvpn_utils.sh) already converts the failure return code: whencrl_download_toreturns 1, the function immediately returns 2. This ensures the test stub'sreturn 1in fail mode properly triggers the error message assertion, as the calling code inrevokelist.shchecks forcrl_status -eq 2to emit the "Failed to download CRL" message.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
@coderabbitai review |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
✅ Actions performedReview triggered.
|
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
Commit Message and OpenVPN FailureHello @4f4d, Commit Message Failure: Fix: OpenVPN Container Failure: Fix: |
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
ad5c4bd to
1ae091b
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
1ae091b to
595e634
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Review (Since commit 1cfcac5)Changes Reviewed
The change converts separate command execution and Previously Resolved IssuesAll 18 previously identified issues remain resolved. Security & Reliability Highlights
Files Reviewed (1 file)
The implementation is well-structured, properly tested, and follows security best practices. All review findings have been resolved. Reviewed by kimi-k2.5-0127 · 86,914 tokens |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks for the work on this, the CRL part is going in the right direction. I built the OpenVPN image locally, verified /openvpn.crontab loads correctly, and exercised revokelist.sh with controlled CRL contents. Initial/changed CRL restarts OpenVPN, unchanged CRL does not restart, and failed CRL download keeps the existing revoked.crl, which is what we want.
I am requesting changes on the VPN config update path: we need to be stricter about applying downloaded configs and also preserve the useful self-recovery behavior when local state is missing/corrupt.
|
|
||
| remote_checksum=$(curl --silent --show-error --fail --insecure \ | ||
| ${API_INTERNAL}/controller/vpn/checksum/$UUID/?key=$KEY) || return 1 | ||
| if [ -z "$remote_checksum" ] || [ ! -s checksum ]; then |
There was a problem hiding this comment.
This treats a missing or empty local checksum the same as a failed remote checksum fetch. I tested the missing-local-checksum case by stubbing the remote checksum to return remote-checksum and leaving local checksum absent. Expected behavior: download the config, update checksum, and restart OpenVPN. Actual behavior: openvpn.sh exits with Failed to fetch or validate OpenVPN checksum, does not download anything, and does not restart.
We must still fail if the remote checksum cannot be downloaded or is empty, but a missing local checksum should mean local state is stale/corrupt and should trigger a safe config re-download.
| ${API_INTERNAL}/controller/vpn/checksum/$UUID/?key=$KEY || return 1 | ||
| test -s "$tmp_checksum" || return 1 | ||
|
|
||
| tar xzf "$tmp_tar" || return 1 |
There was a problem hiding this comment.
This extracts the downloaded tarball directly into / before verifying the downloaded config against the checksum returned by the API. We need an integrity check before applying anything.
Expected behavior:
- download config and checksum into temporary paths
- verify the downloaded config checksum matches the expected checksum
- only then replace the live files and restart OpenVPN
If config download fails, checksum download fails, checksum is empty, or the downloaded config checksum does not match the API checksum, we must log an error, keep the existing config/checksum untouched, and not restart OpenVPN. Otherwise a partial/corrupted/mismatched config can be applied and later picked up by the next service/container restart.
|
|
||
| if [ "${OFILE}" != "${NFILE}" ]; then | ||
| openvpn_config_download | ||
| if ! openvpn_config_download; then |
There was a problem hiding this comment.
Please add coverage for the config integrity path too. The CRL regression test is useful, but we also need to simulate a downloaded config/checksum mismatch and assert that the old config remains in place, checksum remains unchanged, supervisorctl restart openvpn is not called, and an error is logged. I am sorry this was not specified in high detail before.
There was a problem hiding this comment.
Added config-path regression coverage in tests/runtests.py.
The new test covers:
- checksum mismatch preserving the existing config/checksum and avoiding restart
- missing local checksum triggering safe re-download and restart
The updated branch is green.
Moved OpenVPN-specific helpers into the image directory, made CRL refresh restart OpenVPN only when the CRL changes, and moved the base OpenVPN cron entries into an image-local crontab. Closes openwisp#589
Preserve self-recovery when the local checksum is missing, verify downloaded OpenVPN config archives against the API checksum before applying them, and add regression coverage for mismatch and recovery cases. Related to openwisp#589
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
f543df0 to
3f1d2a7
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 126-130: crl_download_to currently does a single curl fetch which
causes crl_download_if_changed to fail on transient errors; modify
crl_download_to to perform multiple attempts with exponential backoff (e.g.,
configurable MAX_ATTEMPTS and initial DELAY, doubling delay each retry) while
still writing to the provided output_path and preserving curl flags (--silent
--show-error --fail --insecure --output). Keep the same function signature,
return non-zero on final failure so callers (crl_download_if_changed) detect
failure, and ensure the final successful attempt exits normally; include brief
comments and use the CA_UUID and DASHBOARD_INTERNAL variables as before.
🪄 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: 34902685-9f02-4ef4-9502-ad27a3a67963
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 Learnings (1)
📚 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-53: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 55-124: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 56-56: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 57-57: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 58-58: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 59-59: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 60-60: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 126-130: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 127-127: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 132-136: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 138-158: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 139-139: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 160-180: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 169-169: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 169-169: Quote this to prevent word splitting.
(SC2046)
[warning] 170-170: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 170-170: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (6)
images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
images/openwisp_openvpn/openvpn.sh (1)
6-7: LGTM!Also applies to: 10-13, 16-19
images/openwisp_openvpn/revokelist.sh (1)
6-7: LGTM!Also applies to: 10-17
images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!tests/runtests.py (1)
7-7: LGTM!Also applies to: 315-316, 318-320, 423-531, 532-678, 681-682, 687-688, 697-697
Use the same retry and backoff options for the temporary CRL download path that are already used for the direct CRL refresh path. Related to openwisp#589
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 13-14: The separate call to `ip -6 route show default` before `if
[ $? = 0 ]; then` can fail under `set -e` and abort startup; instead, run the
command inside the conditional so failures don't trigger `set -e` (for example
replace the two-step pattern using `ip -6 route show default` and `if [ $? = 0
]; then` with a single conditional that executes the command inside the `if` and
redirects output to /dev/null). Update the usage of `ip -6 route show default`
and the `if [ $? = 0 ]; then` check accordingly so the script safely detects
IPv6 default routes without allowing `set -e` to cause an early exit.
🪄 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: 9fe58c9c-f81a-45f4-a7f8-8c22dd78cecf
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 Learnings (1)
📚 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-53: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 55-124: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 56-56: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 57-57: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 58-58: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 59-59: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 60-60: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 126-131: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 127-127: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 130-130: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 130-130: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 133-137: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 136-136: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 136-136: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 139-159: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 140-140: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 161-181: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 170-170: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 170-170: Quote this to prevent word splitting.
(SC2046)
[warning] 171-171: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 171-171: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (6)
images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
images/openwisp_openvpn/openvpn.sh (1)
6-7: LGTM!Also applies to: 10-13, 16-19
images/openwisp_openvpn/revokelist.sh (1)
6-7: LGTM!Also applies to: 10-17
images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!tests/runtests.py (1)
7-7: LGTM!Also applies to: 315-321, 423-678, 681-688, 697-697
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 125-136: The crl_download function duplicates crl_download_to
(which already defaults output_path to revoked.crl); remove the redundant
crl_download function and update any callers to invoke crl_download_to (or keep
a tiny wrapper that forwards to crl_download_to) so there is a single
implementation. Specifically, delete the crl_download definition from the file
(or replace it with: crl_download() { crl_download_to; }) and ensure callers
referencing crl_download now call crl_download_to (or the new wrapper) to avoid
duplication.
- Around line 21-31: In openvpn_config, the unquoted command substitutions for
UUID, KEY and CA_UUID can be split if Redis values contain whitespace; update
the assignments that call get_redis_value to use quoted substitutions (i.e.,
assign UUID, KEY, CA_UUID from "$(get_redis_value ...)" ) so the results are
treated as single values and exported safely; touch the lines that export UUID,
KEY and CA_UUID in the function openvpn_config to apply this change.
🪄 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: 035d3067-477d-47fe-94b0-a7777efe7dae
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 Learnings (1)
📚 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-19: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 21-31: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 27-27: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 33-52: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 34-34: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 38-38: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 38-38: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 38-38: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 54-123: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 55-55: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 56-56: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 57-57: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 58-58: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 59-59: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 74-74: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 74-74: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 74-74: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 78-78: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 78-78: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 78-78: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 125-130: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 126-126: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 132-136: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 138-158: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 139-139: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 160-180: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 169-169: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 169-169: Quote this to prevent word splitting.
(SC2046)
[warning] 170-170: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 170-170: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (15)
tests/runtests.py (4)
532-677: Test harness captures inner script exit code incorrectly on mismatch case.The test expects
exit_code=1for mismatch, butresult.exit_codecaptures the outersh -lcexit code, not the inneropenvpn.shexit code. Since the command ends after runningopenvpn.shwithout|| true, this should propagate correctly. However, if the inner script exits 1,set -euin the outer script should also cause exit 1.After re-reading: the outer script has
set -eubut thesh {test_dir}/openvpn.shis the last command, so its exit code propagates. This is correct.
7-7: LGTM!Also applies to: 315-322
423-530: LGTM!
679-698: LGTM!images/openwisp_openvpn/openvpn_utils.sh (6)
3-6: LGTM!
8-19: LGTM!
33-52: LGTM!
54-123: LGTM!
138-158: LGTM!
160-180: LGTM!images/common/init_command.sh (1)
33-39: LGTM!images/openwisp_openvpn/openvpn.sh (1)
6-21: LGTM!images/openwisp_openvpn/revokelist.sh (1)
6-17: LGTM!images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!
| function openvpn_config { | ||
| # Fectch UUID and Key of the default VPN only if they | ||
| # are not already set. The user may override the UUID and Key | ||
| # by setting them in the environment variables to use deploy | ||
| # a different VPN server. | ||
| if [ -z "$UUID" ]; then | ||
| export UUID=$(get_redis_value "openwisp_default_vpn_uuid") | ||
| export KEY=$(get_redis_value "openwisp_default_vpn_key") | ||
| export CA_UUID=$(get_redis_value "openwisp_default_vpn_ca_uuid") | ||
| fi | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Unquoted command substitution can break on whitespace in Redis values.
If a Redis value contains spaces or special characters, word splitting will occur. While unlikely for UUIDs/keys, quoting is defensive.
Suggested fix
function openvpn_config {
# Fectch UUID and Key of the default VPN only if they
# are not already set. The user may override the UUID and Key
# by setting them in the environment variables to use deploy
# a different VPN server.
if [ -z "$UUID" ]; then
- export UUID=$(get_redis_value "openwisp_default_vpn_uuid")
- export KEY=$(get_redis_value "openwisp_default_vpn_key")
- export CA_UUID=$(get_redis_value "openwisp_default_vpn_ca_uuid")
+ UUID=$(get_redis_value "openwisp_default_vpn_uuid")
+ KEY=$(get_redis_value "openwisp_default_vpn_key")
+ CA_UUID=$(get_redis_value "openwisp_default_vpn_ca_uuid")
+ export UUID KEY CA_UUID
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.
| function openvpn_config { | |
| # Fectch UUID and Key of the default VPN only if they | |
| # are not already set. The user may override the UUID and Key | |
| # by setting them in the environment variables to use deploy | |
| # a different VPN server. | |
| if [ -z "$UUID" ]; then | |
| export UUID=$(get_redis_value "openwisp_default_vpn_uuid") | |
| export KEY=$(get_redis_value "openwisp_default_vpn_key") | |
| export CA_UUID=$(get_redis_value "openwisp_default_vpn_ca_uuid") | |
| fi | |
| } | |
| function openvpn_config { | |
| # Fectch UUID and Key of the default VPN only if they | |
| # are not already set. The user may override the UUID and Key | |
| # by setting them in the environment variables to use deploy | |
| # a different VPN server. | |
| if [ -z "$UUID" ]; then | |
| UUID="$(get_redis_value "openwisp_default_vpn_uuid")" | |
| KEY="$(get_redis_value "openwisp_default_vpn_key")" | |
| CA_UUID="$(get_redis_value "openwisp_default_vpn_ca_uuid")" | |
| export UUID KEY CA_UUID | |
| fi | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 21-31: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 27-27: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@images/openwisp_openvpn/openvpn_utils.sh` around lines 21 - 31, In
openvpn_config, the unquoted command substitutions for UUID, KEY and CA_UUID can
be split if Redis values contain whitespace; update the assignments that call
get_redis_value to use quoted substitutions (i.e., assign UUID, KEY, CA_UUID
from "$(get_redis_value ...)" ) so the results are treated as single values and
exported safely; touch the lines that export UUID, KEY and CA_UUID in the
function openvpn_config to apply this change.
| function crl_download_to { | ||
| local output_path="${1:-revoked.crl}" | ||
| curl --silent --show-error --fail --retry 10 --retry-delay 5 --retry-max-time 300 \ | ||
| --insecure --output "$output_path" \ | ||
| ${DASHBOARD_INTERNAL}/admin/pki/ca/x509/ca/${CA_UUID}.crl | ||
| } | ||
|
|
||
| function crl_download { | ||
| curl --silent --show-error --fail --retry 10 --retry-delay 5 --retry-max-time 300 \ | ||
| --insecure --output revoked.crl \ | ||
| ${DASHBOARD_INTERNAL}/admin/pki/ca/x509/ca/${CA_UUID}.crl | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
crl_download duplicates crl_download_to with default argument.
crl_download_to already defaults output_path to revoked.crl. The separate crl_download function is redundant.
Suggested consolidation
function crl_download_to {
local output_path="${1:-revoked.crl}"
curl --silent --show-error --fail --retry 10 --retry-delay 5 --retry-max-time 300 \
--insecure --output "$output_path" \
${DASHBOARD_INTERNAL}/admin/pki/ca/x509/ca/${CA_UUID}.crl
}
-function crl_download {
- curl --silent --show-error --fail --retry 10 --retry-delay 5 --retry-max-time 300 \
- --insecure --output revoked.crl \
- ${DASHBOARD_INTERNAL}/admin/pki/ca/x509/ca/${CA_UUID}.crl
-}
+# Alias for backward compatibility
+crl_download() { crl_download_to; }🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 125-130: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 126-126: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 132-136: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@images/openwisp_openvpn/openvpn_utils.sh` around lines 125 - 136, The
crl_download function duplicates crl_download_to (which already defaults
output_path to revoked.crl); remove the redundant crl_download function and
update any callers to invoke crl_download_to (or keep a tiny wrapper that
forwards to crl_download_to) so there is a single implementation. Specifically,
delete the crl_download definition from the file (or replace it with:
crl_download() { crl_download_to; }) and ensure callers referencing crl_download
now call crl_download_to (or the new wrapper) to avoid duplication.
2 participants
Checklist
Reference to Existing Issue
Closes #589
Description of Changes
images/common/utils.shtoimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/openvpn.crontaband load them frominit_command.shrevokelist.shto download the CRL into a temporary file and restart OpenVPN only when the CRL actually changesTOPOLOGY_UPDATE_INTERVALbefore appending the optional topology cron entryLocal Validation
./run-qa-checkscompleted successfullymake compose-buildcompleted successfully, including the patched OpenVPN imageopenvpn.crontabis loaded correctlyrevoked.crland triggers the restart pathrevoked.crland triggers the restart pathrevoked.crland does not trigger a restart