[fix] Make parameter tls_cipher an array #349

[okraits]

Fixes #349

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• I have updated the documentation.

Reference to Existing Issue

Closes #349.

Description of Changes

Made the parameter tls_cipher an array and updated the documentation accordingly. There were no tests to update.

[coveralls]

coverage: 99.18% (+0.002%) from 99.178%
when pulling a36757a on tls-cipher-list
into 21c5797 on master.

[nemesifier]

@okraits this change would be backward incompatible. Why is it needed?

Can you provide an example of a value that you can't supply now and you'd be able to supply with the list format?

[okraits]

@okraits this change would be backward incompatible. Why is it needed?

Can you provide an example of a value that you can't supply now and you'd be able to supply with the list format?

I gave an example and the reasoning in the related issue.

[nemesifier]

What about avoiding the schema change and change the code internally to convert the string to a list with 1 element so that it's rendered as a list?

Is the problem just the rendering of UCI option vs UCI list?
Or do we actually need to allow multiple lines with different values?

[okraits]

What about avoiding the schema change and change the code internally to convert the string to a list with 1 element so that it's rendered as a list?

I think this would be an appropriate solution as well.

Is the problem just the rendering of UCI option vs UCI list? Or do we actually need to allow multiple lines with different values?

Rendering the parameter as an UCI list is required for the parameter to work. In the LuCI OpenVPN app it's possible to create multiple list items with different values but I think for most usecases of netjsonconfig it would be sufficient to have one list item.

[nemesifier]

What about avoiding the schema change and change the code internally to convert the string to a list with 1 element so that it's rendered as a list?

I think this would be an appropriate solution as well.

We can do this here:
https://github.com/openwisp/netjsonconfig/blob/master/netjsonconfig/backends/openwrt/converters/openvpn.py

We need two tests:

• classic conversion to NetJSON to UCI, probably modifying existing tests is going to be enough
• backward conversion from UCI to NetJSON, not sure if we can modify an existing one or we need to add a new one

Is the problem just the rendering of UCI option vs UCI list? Or do we actually need to allow multiple lines with different values?

Rendering the parameter as an UCI list is required for the parameter to work. In the LuCI OpenVPN app it's possible to create multiple list items with different values but I think for most usecases of netjsonconfig it would be sufficient to have one list item.

Ok so it sounds to methat handling this internally it's the best option as it's just an output issue.

[okraits]

@nemesifier I implemented the change as suggested. Do you think we need more or other tests?

[okraits]

@nemesifier Any opinion on this?

[pandafy]

Thanks for your patience @okraits.

It took me some time to understand the working of the tls_cipher setting in OpenVPN, hence the delay.

[coderabbitai]

Warning

Rate limit exceeded

@okraits has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 32 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 344db41 and a36757a.

📒 Files selected for processing (4)

• netjsonconfig/backends/openwrt/converters/openvpn.py
• tests/openvpn/test_backend.py
• tests/openvpn/test_parser.py
• tests/openwrt/test_openvpn.py

📝 Walkthrough

Walkthrough

The diff adds support and normalization for OpenVPN tls_cipher. The parser captures tls-cipher into a tls_cipher field. The OpenWRT converter normalizes an intermediate tls_cipher string into a list by splitting on : (discarding empty parts) and, when producing netjson/UCI output, joins a non-empty tls_cipher list with : into a string. Tests were updated to include tls_cipher in server/client test data and to expect a tls-cipher line in rendered client configs and status output.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description references the linked issue #349, completes the checklist, and explains the change. However, it states 'There were no tests to update' while the raw summary shows tests were added in multiple test files, creating an inconsistency.	Clarify whether tests were actually updated or added, as the summary shows changes in test_backend.py, test_parser.py, and test_openvpn.py that contradict the 'no tests to update' statement.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: making the tls_cipher parameter into an array, and references the related issue #349.
Linked Issues check	✅ Passed	The PR addresses the core requirement of issue #349 by converting tls_cipher from a string to an array. The converter properly splits string values on ':' and joins list values back, and tests validate both NetJSON→UCI and UCI→NetJSON conversions.
Out of Scope Changes check	✅ Passed	All changes are scoped to the tls_cipher field conversion from string to array as specified in issue #349. No unrelated modifications to other functionality were introduced.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch tls-cipher-list

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@tests/openvpn/test_backend.py`:
- Line 260: The trailing blanket linter suppression on the "tls_cipher" test
line is unused; remove the unnecessary "# noqa" suffix from the string
assignment for "tls_cipher" in tests/openvpn/test_backend.py (the line
containing "tls_cipher":
"TLS-DHE-RSA-WITH-AES-256-CBC-SHA:TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA:`@SECLEVEL`=0")
so the linter no longer reports an unused suppression.

In `@tests/openvpn/test_parser.py`:
- Line 114: Remove the unnecessary noqa suppression from the test data: delete
the trailing "# noqa" at the end of the line containing the "tls_cipher" entry
in tests/openvpn/test_parser.py so the linter no longer reports an unused
suppression; ensure the string value for "tls_cipher" remains unchanged.

In `@tests/openwrt/test_openvpn.py`:
- Line 214: Remove the unused "# noqa" suppression on the tls_cipher line in
tests/openwrt/test_openvpn.py by deleting the trailing "# noqa" after the
"tls_cipher" value; ensure the line remains unchanged otherwise and run the
linter to confirm the warning is resolved.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bacdc34 and bef0354.

📒 Files selected for processing (4)

• netjsonconfig/backends/openwrt/converters/openvpn.py
• tests/openvpn/test_backend.py
• tests/openvpn/test_parser.py
• tests/openwrt/test_openvpn.py

🚧 Files skipped from review as they are similar to previous changes (1)

• netjsonconfig/backends/openwrt/converters/openvpn.py

🧰 Additional context used

🪛 Ruff (0.14.14)

tests/openvpn/test_backend.py

[warning] 260-260: Unused blanket noqa directive

Remove unused noqa directive

(RUF100)

tests/openvpn/test_parser.py

[warning] 114-114: Unused blanket noqa directive

Remove unused noqa directive

(RUF100)

tests/openwrt/test_openvpn.py

[warning] 214-214: Unused blanket noqa directive

Remove unused noqa directive

(RUF100)

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@netjsonconfig/backends/openwrt/converters/openvpn.py`:
- Around line 18-24: The Black formatting failed and the loop in
__intermediate_vpn should be simplified: replace the manual loop that splits
vpn.get("tls_cipher") and appends non-empty parts with a list comprehension like
vpn["tls_cipher"] = [part for part in ciphers.split(':') if part] (guarded by
the existing isinstance and assignment), then run openwisp-qa-format (or Black)
to auto-fix formatting and ensure the file passes CI; keep the final return
calling super().__intermediate_vpn(vpn, remove=[""]) unchanged.

In `@tests/openvpn/test_backend.py`:
- Line 260: The "tls_cipher" value in the test contains a string exceeding the
110-char limit; split the long string into shorter parts and concatenate them so
the final value is identical. Locate the dict entry for "tls_cipher" in
tests/openvpn/test_backend.py and replace the single long literal with either
adjacent quoted fragments (implicit concatenation or explicit '+' concatenation)
or a short list joined with ''. Ensure the key name "tls_cipher" and the
resulting combined string remain unchanged.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bef0354 and 592511a.

📒 Files selected for processing (4)

• netjsonconfig/backends/openwrt/converters/openvpn.py
• tests/openvpn/test_backend.py
• tests/openvpn/test_parser.py
• tests/openwrt/test_openvpn.py

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/openvpn/test_parser.py
• tests/openwrt/test_openvpn.py

🧰 Additional context used

🪛 GitHub Actions: Netjsonconfig CI Build

tests/openvpn/test_backend.py

[error] 260-260: E501 line too long (120 > 110 characters)

netjsonconfig/backends/openwrt/converters/openvpn.py

[error] 1-1: Black check failed! Hint: did you forget to run openwisp-qa-format?

🔇 Additional comments (2)

netjsonconfig/backends/openwrt/converters/openvpn.py (1)

33-34: LGTM!

The reverse conversion from list to colon-separated string correctly handles the round-trip back to NetJSON format.

tests/openvpn/test_backend.py (1)

306-306: LGTM!

The expected output correctly validates that the tls_cipher field renders to the tls-cipher directive with the colon-delimited cipher list intact.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@tests/openwrt/test_openvpn.py`:
- Line 214: The "tls_cipher" string in tests/openwrt/test_openvpn.py exceeds the
110-char E501 limit; update the value for the 'tls_cipher' key by splitting it
into multiple adjacent string literals (or a parenthesized multi-line string) so
Python concatenates them (e.g., split after a colon) and keep the same exact
combined content; modify the dictionary entry for "tls_cipher" in the test to
use the split string form so the test logic remains unchanged but the source
line lengths are under the limit.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 592511a and 344db41.

📒 Files selected for processing (4)

• netjsonconfig/backends/openwrt/converters/openvpn.py
• tests/openvpn/test_backend.py
• tests/openvpn/test_parser.py
• tests/openwrt/test_openvpn.py

🚧 Files skipped from review as they are similar to previous changes (2)

• netjsonconfig/backends/openwrt/converters/openvpn.py
• tests/openvpn/test_parser.py

🧰 Additional context used

🪛 GitHub Actions: Netjsonconfig CI Build

tests/openwrt/test_openvpn.py

[error] 214-214: E501 line too long (112 > 110). Flake8: line length limit exceeded.

tests/openvpn/test_backend.py

[error] 260-260: E501 line too long (120 > 110). Flake8: line length limit exceeded.

🔇 Additional comments (2)

tests/openwrt/test_openvpn.py (1)

258-260: UCI list rendering for tls_cipher looks correct.

tests/openvpn/test_backend.py (1)

306-306: Expected tls-cipher output addition looks good.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[nemesifier]

Looks good, I will do a round of manual testing asap.