[change] OpenVPN: added modern compression options #198#376
[change] OpenVPN: added modern compression options #198#376nemesifier merged 14 commits intoopenwisp:masterfrom
Conversation
pandafy
force-pushed
the
issues/198-add-compressions-options
branch
from
9367e98 to
3e19a3a
Compare
|
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:
📝 WalkthroughWalkthroughThe PR adds modern OpenVPN compression options across docs, schema, parser, renderer, and tests. It introduces a Sequence Diagram(s)sequenceDiagram
participant Native as Native OpenVPN snippet
participant Parser as OpenVpn Parser
participant Model as NetJSON Config Model
participant Renderer as Backend Renderer
participant Output as Rendered config/file
Native->>Parser: provide lines (compress / allow-compression / comp_lzo)
Parser->>Model: parse options into keys (compress, allow_compression, comp_lzo)
Model->>Renderer: provide tunnel properties (compress, allow_compression, comp_lzo)
Renderer->>Output: render appropriate directives (compress / allow-compression or legacy comp_lzo)
Note over Parser,Renderer: Tests verify parsing, rendering, migration, and algorithms
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netjsonconfig/backends/openvpn/schema.py`:
- Around line 185-199: The allow_compression schema has mismatched enum order vs
enum_titles which will map user selections to the wrong value; update the
allow_compression definition so the "enum" array order matches the
"options.enum_titles" order (or vice versa) — e.g., ensure the second enum value
corresponds to "No compression allowed" and the third to "Yes - allow
compression" by reordering ["asym","no","yes"] or swapping the enum_titles to
match ["asym","yes","no"]; modify the allow_compression block in schema.py
accordingly (function/variable: allow_compression).
- Around line 164-201: The schema is missing a declared push_compress property;
add a new "push_compress" entry alongside "compress" and "allow_compression" in
the same dict so schema validation/UI exposes that server-side option. Define
"push_compress" with the same title/description/type/enum/default/options
pattern used by "compress" (i.e., enum [""] + compression_algorithms, default
"", and matching enum_titles) and set an appropriate propertyOrder (e.g.,
between 9.1 and 9.2 or next available). Ensure the property name is exactly
"push_compress" so it matches existing docs/tests.
In `@tests/openvpn/test_backend.py`:
- Around line 1162-1165: Remove the leftover inline review artifacts "# ADD
THIS" from the test configuration dict in tests/openvpn/test_backend.py:
specifically remove the comment suffixes on the "nobind" key and the
"resolv_retry" key so the dict entries become clean boolean/string values;
update the two entries (the "nobind" assignment and the "resolv_retry"
assignment) to have no trailing comments and run the tests to ensure no
formatting issues remain.
In `@tests/openvpn/test_parser.py`:
- Around line 366-381: The test_parse_allow_compression currently asserts only
allow_compression and should be hardened to verify the entire parsed VPN block;
update the test (function test_parse_allow_compression) to build an expected
dict for the OpenVpn parse result (matching the structure used in
test_parse_compress) and assert equality against o.config["openvpn"][0] returned
by OpenVpn(native=...); ensure keys like ca, cert, dev, dev-type, dh, key, mode,
proto and allow_compression are present and have the expected values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc1c41fc-c285-4f7a-9895-183b2232733a
📒 Files selected for processing (4)
docs/source/backends/openvpn.rstnetjsonconfig/backends/openvpn/schema.pytests/openvpn/test_backend.pytests/openvpn/test_parser.py
📜 Review details
🔇 Additional comments (1)
docs/source/backends/openvpn.rst (1)
110-120: Compression docs update looks good.Deprecation messaging for
comp_lzoand the newcompress/allow_compression/push_compressentries are clearly documented.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/openvpn/test_backend.py (1)
1162-1165:⚠️ Potential issue | 🟡 MinorRemove leftover inline review artifacts in test fixture.
# ADD THIScomments look like temporary patch notes and should not be committed.Proposed cleanup
- "nobind": True, # ADD THIS + "nobind": True, @@ - "resolv_retry": "infinite", # ADD THIS + "resolv_retry": "infinite",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openvpn/test_backend.py` around lines 1162 - 1165, The test fixture in tests/openvpn/test_backend.py contains leftover inline review artifacts ("# ADD THIS") next to the "nobind" and "resolv_retry" keys in the dictionary used for the OpenVPN config; remove those inline comments so the fixture only contains the actual keys and values (e.g., "nobind": True and "resolv_retry": "infinite") and ensure there are no other stray review comments in the same test fixture.netjsonconfig/backends/openvpn/schema.py (1)
164-192:⚠️ Potential issue | 🟠 MajorAdd missing
push_compressschema field for server mode.The PR objective includes server-side
push_compress, but this schema change still only modelscompressandallow_compression. Withoutpush_compressin schema, validation/UI cannot express that option.Proposed fix
"server": { "title": "Server", "type": "object", "properties": { @@ "topology": { @@ "propertyOrder": 7, }, + "push_compress": { + "title": "push compression algorithm", + "description": "Server-side option to push compression settings to clients", + "type": "string", + "enum": [""] + compression_algorithms, + "default": "", + "options": { + "enum_titles": [ + "Disabled", + "LZO compression (for compatibility)", + "LZ4 compression", + "LZ4-v2 compression (recommended)", + "Compression framing without actual compression", + "Compression framing v2 without actual compression", + ] + }, + "propertyOrder": 9.3, + }, "tls_server": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netjsonconfig/backends/openvpn/schema.py` around lines 164 - 192, Add a new "push_compress" schema entry alongside "compress" and "allow_compression": define "push_compress" with the same shape as "compress" (title like "push compression", description stating this is the server-side pushed compression option for server mode, "type": "string", "enum": [""] + compression_algorithms, "default": "", and the same "options":{"enum_titles": [...]}) and give it a propertyOrder between the existing values (e.g. 9.15) so UI/validation can represent the server push_compress setting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@netjsonconfig/backends/openvpn/schema.py`:
- Around line 164-192: Add a new "push_compress" schema entry alongside
"compress" and "allow_compression": define "push_compress" with the same shape
as "compress" (title like "push compression", description stating this is the
server-side pushed compression option for server mode, "type": "string", "enum":
[""] + compression_algorithms, "default": "", and the same
"options":{"enum_titles": [...]}) and give it a propertyOrder between the
existing values (e.g. 9.15) so UI/validation can represent the server
push_compress setting.
In `@tests/openvpn/test_backend.py`:
- Around line 1162-1165: The test fixture in tests/openvpn/test_backend.py
contains leftover inline review artifacts ("# ADD THIS") next to the "nobind"
and "resolv_retry" keys in the dictionary used for the OpenVPN config; remove
those inline comments so the fixture only contains the actual keys and values
(e.g., "nobind": True and "resolv_retry": "infinite") and ensure there are no
other stray review comments in the same test fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c86ce7d-a906-4cbd-b95e-b6b86fe8193d
📒 Files selected for processing (4)
docs/source/backends/openvpn.rstnetjsonconfig/backends/openvpn/schema.pytests/openvpn/test_backend.pytests/openwrt/test_openvpn.py
📜 Review details
🔇 Additional comments (4)
netjsonconfig/backends/openvpn/schema.py (1)
93-99: Good addition of centralized compression enum values.Defining
compression_algorithmsonce and reusing it in schema enums keeps validation consistent and easier to maintain.docs/source/backends/openvpn.rst (1)
110-121: Docs update is clear and actionable.Deprecation messaging for
comp_lzoplus explicit allowed values forcompress/allow_compressionimproves migration clarity.tests/openwrt/test_openvpn.py (1)
576-662: Nice coverage for OpenWrt compression mapping.The new render/parse tests for
compressandallow_compressionare a solid guardrail for UCI compatibility.tests/openvpn/test_backend.py (1)
1189-1413: Comprehensive compression behavior coverage.The added cases for algorithm variants, coexistence with
comp_lzo, and empty migration behavior are strong and reduce regression risk.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/openvpn/test_backend.py`:
- Around line 1255-1276: The loop over algorithms in the test should use
unittest's subTest so failures report which algorithm failed; update the test in
tests/openvpn/test_backend.py to wrap each iteration that constructs
OpenVpn(...) and calls c.render() / self.assertIn(...) inside a with
self.subTest(algo=algo): block (referencing the OpenVpn constructor and its
render method and the assertIn call) so each algorithm is a separate subtest and
failures are isolated and reported per algorithm.
- Around line 1411-1413: The test currently does a broad negative check with
self.assertNotIn("compress", output) which can match unrelated tokens; update
the assertion in tests/openvpn/test_backend.py to target the exact OpenVPN
directive instead (e.g. use a word-boundary regex or check for the directive
followed by whitespace/punctuation). Replace the loose check on "compress" with
a directive-specific assertion such as assertNotRegex(output, r"\bcompress\b")
or assertNotIn("compress ", output) so only the standalone "compress" directive
is rejected while allowing tokens like "allow-compression".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ed3dec6-4927-4d97-a6d4-3a562fab0e07
📒 Files selected for processing (1)
tests/openvpn/test_backend.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/netjsonconfig PR: 376
File: netjsonconfig/backends/openvpn/schema.py:164-192
Timestamp: 2026-03-05T18:42:01.007Z
Learning: OpenVPN (including 2.6) does not have a standalone `--push_compress` directive. Pushing compression settings to clients is done via the generic `push "compress <alg>"` mechanism. Do not suggest adding a `push_compress` schema property to the OpenVPN backend schema in netjsonconfig.
📚 Learning: 2026-03-05T18:42:01.007Z
Learnt from: pandafy
Repo: openwisp/netjsonconfig PR: 376
File: netjsonconfig/backends/openvpn/schema.py:164-192
Timestamp: 2026-03-05T18:42:01.007Z
Learning: OpenVPN (including 2.6) does not have a standalone `--push_compress` directive. Pushing compression settings to clients is done via the generic `push "compress <alg>"` mechanism. Do not suggest adding a `push_compress` schema property to the OpenVPN backend schema in netjsonconfig.
Applied to files:
tests/openvpn/test_backend.py
🔇 Additional comments (2)
tests/openvpn/test_backend.py (2)
1189-1387: Good coverage ofallow_compressionand render output formatting.The tests for
no/asym/yesvalidate hyphenated directive rendering and expected full config output cleanly.
1147-1413: No action needed on these tests. The reviewed test cases at lines 1147–1413 correctly validate the compression options that the backend actually supports:compressandallow_compression. The backend schema does not expose apush_compressproperty, and there is no push-compression rendering logic in the OpenVPN backend code. The tests are appropriately scoped to client-side compression configuration, which are the only compression features the backend provides. Adding a regression test for an unsupported directive that was never exposed would be unnecessary.> Likely an incorrect or invalid review comment.
Add support for new OpenVPN compression options to the schema: - compress: modern replacement for comp_lzo with support for lzo, lz4, lz4-v2, stub, stub-v2, and empty string for migration - allow_compression: control compression policy (asym, no, yes) - push_compress: server-side option to push compression to clients Mark comp_lzo as deprecated in favor of the new --compress option. These changes align with recent OpenVPN versions that deprecated comp-lzo in favor of more flexible compression controls. Fixes openwisp#198
Add comprehensive test coverage for compress, allow_compression, and push_compress options including: - Rendering tests for all compression algorithms - Parsing tests for native OpenVPN configs - Tests for coexistence with deprecated comp_lzo option Tests cover both client and server configurations to ensure proper functionality of the new compression options. Fixes openwisp#198
…isp#198 Update OpenVPN backend documentation to reflect the addition of modern compression options: - Add compress option documentation with all supported algorithms (lzo, lz4, lz4-v2, stub, stub-v2, and empty string for migration) - Add allow_compression option documentation with policy controls (asym, no, yes) - Add push_compress option documentation for server-side compression settings - Mark comp_lzo as deprecated in favor of the new compress option - Update general settings table with new compression-related options - Add notes about compression option compatibility with recent OpenVPN versions The documentation now accurately reflects the schema changes that align with OpenVPN's modern compression configuration approach, which deprecated comp-lzo in favor of the more flexible --compress and --allow-compression options. Fixes openwisp#198
pandafy
force-pushed
the
issues/198-add-compressions-options
branch
from
c1475ad to
4c85237
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/openvpn/test_parser.py (1)
366-381: 🧹 Nitpick | 🔵 TrivialStrengthen
test_parse_allow_compressionwith full-structure assertion.This test only checks
allow_compressionvalue, whiletest_parse_compressabove validates the entire parsed structure. For consistency and regression protection, assert the full config dict.Proposed fix
def test_parse_allow_compression(self): """Test parsing allow-compression option""" native = """# openvpn config: test-server allow-compression no ca ca.pem cert cert.pem dev tap0 dev-type tap dh dh.pem key key.pem mode server proto udp """ + expected = { + "openvpn": [ + { + "allow_compression": "no", + "ca": "ca.pem", + "cert": "cert.pem", + "dev": "tap0", + "dev_type": "tap", + "dh": "dh.pem", + "key": "key.pem", + "mode": "server", + "name": "test-server", + "proto": "udp", + } + ] + } o = OpenVpn(native=native) - self.assertEqual(o.config["openvpn"][0]["allow_compression"], "no") + self.assertDictEqual(o.config, expected)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openvpn/test_parser.py` around lines 366 - 381, The test_parse_allow_compression currently asserts only o.config["openvpn"][0]["allow_compression"]; update it to assert the full parsed config dict for the OpenVpn instance (use the same structure shape as test_parse_compress) by building an expected dict that contains all keys (allow_compression, ca, cert, dev, dev-type, dh, key, mode, proto) and comparing it to o.config["openvpn"][0]; reference the test function name test_parse_allow_compression and the OpenVpn class/instance (o) when locating where to replace the single-key assertion with a full-structure equality assertion.netjsonconfig/backends/openvpn/schema.py (1)
186-193: 🧹 Nitpick | 🔵 TrivialConsider adding
enum_titlesforallow_compressionfor UI consistency.The
compressproperty and other enum properties likemtu_discincludeenum_titlesfor user-friendly display in schema-driven UIs. Adding them here would maintain consistency.Suggested addition
"allow_compression": { "title": "allow compression", "description": "Control whether compression is allowed.", "type": "string", "enum": ["asym", "no", "yes"], "default": "no", + "options": { + "enum_titles": [ + "Asymmetric (one direction only)", + "No compression allowed", + "Yes - allow compression", + ] + }, "propertyOrder": 9.2, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netjsonconfig/backends/openvpn/schema.py` around lines 186 - 193, The allow_compression enum in the OpenVPN schema (property name "allow_compression" in netjsonconfig/backends/openvpn/schema.py) lacks user-friendly labels; add an "enum_titles" array matching the enum values ["asym","no","yes"] (for example ["Asymmetric","Disabled","Enabled"] or whichever wording matches existing "compress" and "mtu_disc" labels) so schema-driven UIs display consistent, readable titles for each option.tests/openvpn/test_backend.py (1)
1255-1277:⚠️ Potential issue | 🟡 MinorAssertion is outside
subTestblock - test failures won't be isolated.Lines 1276-1277 (
output = c.render()andself.assertIn(...)) are at the same indentation as thewith self.subTestblock, meaning they execute after the subTest context exits. This defeats the purpose of usingsubTest.Proposed fix
for algo in algorithms: with self.subTest(algo=algo): c = OpenVpn( { "openvpn": [ { "ca": "ca.pem", "cert": "cert.pem", "compress": algo, "dev": "tun0", "dev_type": "tun", "key": "key.pem", "mode": "p2p", "name": f"test-{algo}", "proto": "udp", "remote": [{"host": "vpn.example.com", "port": 1194}], "tls_client": True, } ] } ) - output = c.render() - self.assertIn(f"compress {algo}", output) + output = c.render() + self.assertIn(f"compress {algo}", output)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openvpn/test_backend.py` around lines 1255 - 1277, The assertion and render call are outside the with self.subTest block, so failures won’t be isolated; move the lines that call c.render() and self.assertIn(f"compress {algo}", output) inside the with self.subTest(algo=algo) context (i.e., indent the two lines into the block where OpenVpn is instantiated) so each algorithm’s render and assertion run within its subTest for the OpenVpn instance and render() check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/backends/openvpn.rst`:
- Around line 110-112: Update the documentation entry for the comp_lzo option to
match the schema: change the listed default from "adaptive" to an empty string
(""), keep the deprecation note and allowed values (yes, no, adaptive), and
mention that compress should be used instead (reference: comp_lzo and compress).
In `@netjsonconfig/backends/openvpn/schema.py`:
- Around line 155-163: The comp_lzo schema entry has a default value of "" but
its "enum" lacks the empty string; update the comp_lzo property in the schema
(the dict keyed "comp_lzo") to include "" in its "enum" array (e.g., ["", "yes",
"no", "adaptive"]) so the default is an explicit allowed value and matches the
deprecation note about leaving it empty.
---
Duplicate comments:
In `@netjsonconfig/backends/openvpn/schema.py`:
- Around line 186-193: The allow_compression enum in the OpenVPN schema
(property name "allow_compression" in netjsonconfig/backends/openvpn/schema.py)
lacks user-friendly labels; add an "enum_titles" array matching the enum values
["asym","no","yes"] (for example ["Asymmetric","Disabled","Enabled"] or
whichever wording matches existing "compress" and "mtu_disc" labels) so
schema-driven UIs display consistent, readable titles for each option.
In `@tests/openvpn/test_backend.py`:
- Around line 1255-1277: The assertion and render call are outside the with
self.subTest block, so failures won’t be isolated; move the lines that call
c.render() and self.assertIn(f"compress {algo}", output) inside the with
self.subTest(algo=algo) context (i.e., indent the two lines into the block where
OpenVpn is instantiated) so each algorithm’s render and assertion run within its
subTest for the OpenVpn instance and render() check.
In `@tests/openvpn/test_parser.py`:
- Around line 366-381: The test_parse_allow_compression currently asserts only
o.config["openvpn"][0]["allow_compression"]; update it to assert the full parsed
config dict for the OpenVpn instance (use the same structure shape as
test_parse_compress) by building an expected dict that contains all keys
(allow_compression, ca, cert, dev, dev-type, dh, key, mode, proto) and comparing
it to o.config["openvpn"][0]; reference the test function name
test_parse_allow_compression and the OpenVpn class/instance (o) when locating
where to replace the single-key assertion with a full-structure equality
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a5930af-57d1-427e-b3ab-5825b2704442
📒 Files selected for processing (5)
docs/source/backends/openvpn.rstnetjsonconfig/backends/openvpn/schema.pytests/openvpn/test_backend.pytests/openvpn/test_parser.pytests/openwrt/test_openvpn.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/netjsonconfig PR: 376
File: netjsonconfig/backends/openvpn/schema.py:164-192
Timestamp: 2026-03-05T18:42:01.007Z
Learning: OpenVPN (including 2.6) does not have a standalone `--push_compress` directive. Pushing compression settings to clients is done via the generic `push "compress <alg>"` mechanism. Do not suggest adding a `push_compress` schema property to the OpenVPN backend schema in netjsonconfig.
📚 Learning: 2026-03-05T18:42:01.007Z
Learnt from: pandafy
Repo: openwisp/netjsonconfig PR: 376
File: netjsonconfig/backends/openvpn/schema.py:164-192
Timestamp: 2026-03-05T18:42:01.007Z
Learning: OpenVPN (including 2.6) does not have a standalone `--push_compress` directive. Pushing compression settings to clients is done via the generic `push "compress <alg>"` mechanism. Do not suggest adding a `push_compress` schema property to the OpenVPN backend schema in netjsonconfig.
Applied to files:
docs/source/backends/openvpn.rstnetjsonconfig/backends/openvpn/schema.pytests/openvpn/test_parser.pytests/openwrt/test_openvpn.pytests/openvpn/test_backend.py
🔇 Additional comments (11)
netjsonconfig/backends/openvpn/schema.py (1)
93-99: LGTM!The
compression_algorithmslist andcompressproperty are well-defined. The empty string default correctly enables migration fromcomp-lzowhile ensuring the directive isn't rendered when not explicitly configured.Also applies to: 165-185
docs/source/backends/openvpn.rst (1)
113-121: LGTM!The documentation for
compressandallow_compressionis accurate and provides clear guidance on allowed values and usage, including the migration path fromcomp_lzo.tests/openwrt/test_openvpn.py (2)
576-618: LGTM!The
compresstests provide good coverage with well-structured fixtures and both render/parse roundtrip verification. The test values align with the schema definition.
620-662: LGTM!The
allow_compressiontests follow the same pattern and provide complete coverage for the OpenWrt backend. The fixtures and assertions are correct.tests/openvpn/test_parser.py (1)
331-364: LGTM!The
test_parse_compresstest is well-structured with a complete expected dictionary assertion, ensuring the entire parsed configuration is validated.tests/openvpn/test_backend.py (6)
1147-1187: LGTM!The
test_compression_optionstest provides comprehensive coverage of thelz4-v2compression algorithm with a complete expected output assertion.
1189-1223: LGTM!The
test_allow_compressiontest correctly validates theallow-compression nodirective rendering.
1225-1249: LGTM!Good test for backward compatibility, verifying that both
comp_lzo(deprecated) andcompresscan coexist in the same configuration.
1279-1315: LGTM!The
test_compress_stubtest validates the stub compression option with a complete expected output assertion.
1317-1387: LGTM!Good coverage for
allow_compressionwith bothasymandyesvalues, each with complete expected output assertions.
1389-1414: LGTM!The
test_compress_empty_migrationtest correctly verifies the migration behavior:comp-lzo adaptiveis rendered whilecompress ""is dropped by the converter. The use ofassertNotRegexwith word boundary properly targets only thecompressdirective.
|
Video from testing with openwisp-controller Screencast.from.06-03-26.02.08.31.PM.IST.webmUpdated docs: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netjsonconfig/backends/openvpn/schema.py`:
- Around line 167-174: The schema currently sets allow_compression default to
"no", which causes defaults to be materialized; either remove the default (or
change it to a null/unset sentinel) in the allow_compression property in
schema.py so untouched VPN entries don't emit "allow-compression no", or update
OpenVpnConverter.__intermediate_vpn() to treat the string "no" as a falsy value
to be stripped (same way it strips False, 0, and ""), e.g., treat "no" as unset
when building intermediate representation; locate the allow_compression entry
and the __intermediate_vpn method to implement one of these fixes.
- Around line 93-99: The schema's compress enum currently rejects OpenVPN's
"migrate" option; update the compression_algorithms enum used for the compress
field by adding the string "migrate" to the list (the existing
compression_algorithms variable / compress enum definition in schema.py) so the
validator accepts --compress migrate; ensure the new value is included alongside
"lzo", "lz4", "lz4-v2", "stub", and "stub-v2".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31f8673d-0f02-46cc-b219-9365057d35fb
📒 Files selected for processing (4)
docs/source/backends/openvpn.rstnetjsonconfig/backends/openvpn/schema.pytests/openvpn/test_backend.pytests/openvpn/test_parser.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/netjsonconfig PR: 376
File: netjsonconfig/backends/openvpn/schema.py:164-192
Timestamp: 2026-03-05T18:42:01.007Z
Learning: OpenVPN (including 2.6) does not have a standalone `--push_compress` directive. Pushing compression settings to clients is done via the generic `push "compress <alg>"` mechanism. Do not suggest adding a `push_compress` schema property to the OpenVPN backend schema in netjsonconfig.
📚 Learning: 2026-03-05T18:42:01.007Z
Learnt from: pandafy
Repo: openwisp/netjsonconfig PR: 376
File: netjsonconfig/backends/openvpn/schema.py:164-192
Timestamp: 2026-03-05T18:42:01.007Z
Learning: OpenVPN (including 2.6) does not have a standalone `--push_compress` directive. Pushing compression settings to clients is done via the generic `push "compress <alg>"` mechanism. Do not suggest adding a `push_compress` schema property to the OpenVPN backend schema in netjsonconfig.
Applied to files:
tests/openvpn/test_parser.pydocs/source/backends/openvpn.rstnetjsonconfig/backends/openvpn/schema.pytests/openvpn/test_backend.py
|
@coderabbitai review! |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netjsonconfig/backends/openvpn/schema.py`:
- Around line 156-174: The schema currently restricts "compress" and
"allow_compression" to strings and uses "" to mean unset, which blocks the
valueless (bare) directives that the parser represents as True; update both keys
to accept either a boolean true (to represent the bare directive) or the
existing string forms using oneOf: for "compress" one branch should be
{"type":"boolean","enum":[true]} and the other the current string branch (enum:
[""] + compression_algorithms, default ""), and for "allow_compression" one
branch should be {"type":"boolean","enum":[true]} and the other the current
string branch (enum: ["", "asym", "no", "yes"], default ""), preserving
propertyOrder values; this lets True round-trip to a bare directive while
keeping "" as omission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41fda2ac-ed48-41ed-a781-b2a0c55b657a
📒 Files selected for processing (2)
netjsonconfig/backends/openvpn/schema.pytests/openvpn/test_backend.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/netjsonconfig PR: 376
File: netjsonconfig/backends/openvpn/schema.py:164-192
Timestamp: 2026-03-05T18:42:01.007Z
Learning: OpenVPN (including 2.6) does not have a standalone `--push_compress` directive. Pushing compression settings to clients is done via the generic `push "compress <alg>"` mechanism. Do not suggest adding a `push_compress` schema property to the OpenVPN backend schema in netjsonconfig.
📚 Learning: 2026-03-05T18:42:01.007Z
Learnt from: pandafy
Repo: openwisp/netjsonconfig PR: 376
File: netjsonconfig/backends/openvpn/schema.py:164-192
Timestamp: 2026-03-05T18:42:01.007Z
Learning: OpenVPN (including 2.6) does not have a standalone `--push_compress` directive. Pushing compression settings to clients is done via the generic `push "compress <alg>"` mechanism. Do not suggest adding a `push_compress` schema property to the OpenVPN backend schema in netjsonconfig.
Applied to files:
netjsonconfig/backends/openvpn/schema.pytests/openvpn/test_backend.py
github-project-automation
Bot
moved this to In progress
in OpenWISP Priorities for next releases
nemesifier
changed the title
|
@coderabbitai review or approve if ready |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the implementation details before deciding to approve. 🏁 Script executed: Length of output: 688 🏁 Script executed: Length of output: 429 🏁 Script executed: Length of output: 9904 I found one issue I need to verify before approving. Let me check the full schema for 🏁 Script executed: Length of output: 8213 🏁 Script executed: Length of output: 1681
1. The "compress": {
"title": "compression algorithm",
"description": (
"Specifies the compression algorithm for the VPN data channel."
" Leaving the value empty removes the compress directive from the"
" generated configuration."
" Note: use of compression is discouraged due to security"
" vulnerabilities (VORACLE attack). Prefer 'stub' or 'stub-v2'"
" for backward compatibility without enabling actual compression."
),
...
},2. The schema's 3. The docs table (Line 114) shows the default for 🧠 Learnings used |
|
@coderabbitai the OpenVPN 2.6 docs mentions
https://openvpn.net/community-docs/community-articles/openvpn-2-6-manual.html |
|
Tip For best results, initiate chat on the files or code changes.
The schema description labeling The remaining two points from my earlier review are still worth a quick check:
If those two are also already handled or intentional, happy to approve. ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
The generated UI from the new schema looks messy to me:
It doens't make sense to have resolv-retry in the middle. Please see also the other problems listed below, before submitting again, double check the generated UI to make sure the non deprecated options are shown first, the deprecated options right after and the deprecated options suggest using the non-deprecated options using a name that matches what is shown in the UI.
Deprecated properties should appear after non-deprecated ones
compress (propertyOrder 9) and allow_compression (9.1) appear before comp_lzo (9.2), which is correct ordering-wise in the current code. However, comp_lzo at 9.2 still sits in the middle of the property list, between local (8) and auth (11). It should be moved to the end of the form, after all active options, so deprecated fields don't distract users from the current options.
Deprecated property should reference the human-readable replacement
The comp_lzo description says "Use the compress directive instead", which references the internal config directive name (compress). Users interacting with the UI will see the title "compression algorithm", not the JSON key compress. The deprecation notice should reference the human-readable label so users know which field to look for.
Remediation: Change the comp_lzo description to something like:
"DEPRECATED: Legacy LZO compression option. Use the \"compression algorithm\" option instead. Leave empty unless compatibility with legacy OpenVPN clients is required."
push_compress is mentioned in commit messages but missing from the schema
The commit message for ba8025d lists push_compress as a new server-side option to push compression settings to clients, but the schema does not include any push_compress property. Either this was dropped during development without updating the commit message, or it was accidentally omitted.
Remediation: If push_compress is intended, add it to the server definition in the schema. If it was intentionally excluded, no code change is needed, but the commit message is misleading.
auto_client does not propagate new compression options
In openvpn.py, the auto_client method has a copy_keys list that copies server settings to auto-generated client configs. This list includes comp_lzo but does not include compress or allow_compression. When a server uses the modern compression options, auto-generated clients will silently lose those settings.
Remediation: Add "compress" and "allow_compression" to the copy_keys list in auto_client.
comp_lzo default change is a breaking change
On master, comp_lzo has "default": "adaptive" and does not allow an empty string. This branch changes the default to "" and adds "" to the enum. Any existing configuration that relies on the implicit comp_lzo: adaptive default will now render without comp-lzo at all, silently changing OpenVPN behavior. This is a backward-incompatible change that needs a migration strategy or at minimum documentation of the impact.
Remediation: Document this as a breaking change. Consider whether a schema migration is needed to explicitly set comp_lzo: adaptive on existing configurations that relied on the old default.
Missing enum_titles for compression enums
Other enum properties in the schema (e.g., engine, mtu_disc, script_security, verb) provide human-readable enum_titles via the options field. The new compress, allow_compression, and updated comp_lzo properties all lack enum_titles. This means the UI will show raw values like "", lz4-v2, stub-v2, and asym without explanation.
Remediation: Add options.enum_titles to all three properties. For example, compress could use titles like "Disabled", "LZO", "LZ4", "LZ4 v2", "Stub (framing only)", "Stub v2 (framing only)", "Migrate (transition from comp-lzo)".
Module docstring still says "OpenVpn 2.3"
The schema.py docstring on line 2 reads OpenVpn 2.3 specific JSON-Schema definition, but this branch specifically adds OpenVPN 2.5+/2.6 features. The docs were updated to reference the OpenVPN 2.6 manual, but the docstring was not.
Remediation: Update the docstring to reflect the supported version range, e.g., "OpenVPN JSON-Schema definition" or "OpenVPN 2.x JSON-Schema definition".
| "type": "string", | ||
| "enum": [""] + compression_algorithms, | ||
| "default": "", | ||
| "propertyOrder": 9, |
There was a problem hiding this comment.
The deprecated property should be shown after the non-deprecated ones.
VPN Server Form
VPN template form
Thank you for pointing this out.
Because of this, the
Although the schema default for In I verified this with the following test:
The resulting configuration still contained: This happens because the JSON Schema default is only used by the JSON schema editor UI and is not applied during configuration rendering when a value already exists in the stored configuration. Therefore:
For these reasons, this change does not introduce a breaking change in practice. |
|
@coderabbitai review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 24458
These are inconsistent. Additionally, since Please pick one of the following fixes: Option A – If the intent is to always render ``allow_compression`` string ``no`` ``asym`` (compression allowedOption B – If the intent is to omit the "enum": ["", "asym", "no", "yes"],
"default": "",And update the docs accordingly. Everything else looks good:
Please fix the docs/schema default inconsistency and I'll approve right away. 🧠 Learnings used |
|
@coderabbitai review, focus on real issues, approve if ready. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3919
The enum now correctly includes
Everything else is in order:
One line fix and I'll approve immediately. 🧠 Learnings used |
|
@coderabbitai review, focus on real issues, approve if ready. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3428
All items are now resolved:
Approving! [approve] 🧠 Learnings used |
github-project-automation
Bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
github-project-automation
Bot
moved this from Reviewer approved
to Done
in OpenWISP Priorities for next releases
4 participants
Checklist
Reference to Existing Issue
Closes #198.
Description of Changes
Schema (netjsonconfig/backends/openvpn/schema.py)
Tests (tests/openvpn/)
Documentation (docs/source/backends/openvpn.rst)
Backward Compatibility