[fix] Change openwrt list_identifiers to not merge vlan interfaces - #413
pascal260303 wants to merge 2 commits into
Conversation
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (2)Ensure tests cover relevant success, error, boundary, and unusual input scenarios.⚙️ CodeRabbit configuration file Files:
Flag potential security vulnerabilities Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries Flag unused or redundant code Flag outdated or incorrect comments/docstrings Ensure new code handles err...⚙️ CodeRabbit configuration file Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe OpenWrt backend now matches list entries using Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
pascal260303
changed the title
caf6a74 to
51522f0
Compare
|
coverage: 99.152%. remained the same — pascal260303:master into openwisp:master |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| parser = OpenWrtParser | ||
| renderer = OpenWrtRenderer | ||
| list_identifiers = ["name", "config_value", "id"] | ||
| list_identifiers = ["config_value", "id", "network", "name"] |
There was a problem hiding this comment.
This is a delicate part of the code, changing this can easily have negative consequences on many OpenWISP deployments: after this patch, users could see their configurations change, because a different element may match (see my next comment for a more detailed explanation).
The identifiers for VLAN interfaces seem to be a special case, therefore I think we need to be able to pass different list identifiers for these special cases, I prepared a patch based on this concept, see #414.
Please try it when you can.
There was a problem hiding this comment.
Oh you're right.
I will try it tomorrow
| parser = OpenWrtParser | ||
| renderer = OpenWrtRenderer | ||
| list_identifiers = ["name", "config_value", "id"] | ||
| list_identifiers = ["config_value", "id", "network", "name"] |
There was a problem hiding this comment.
This valid VLAN configuration still fails:
template1 = {
"interfaces": [
{"type": "8021q", "name": "eth0", "vid": 10}
]
}
template2 = {
"interfaces": [
{"type": "8021q", "name": "eth0", "vid": 20}
]
}The code checks config_value, id, network, then name.
Neither VLAN has config_value, id, or network, so both use:
"name": "eth0"They are merged, and VLAN 10 is lost.
It also introduces this unrelated regression:
template = {
"interfaces": [
{"type": "ethernet", "name": "eth0", "network": "lan", "mtu": 1500}
]
}
config = {
"interfaces": [
{"type": "ethernet", "name": "eth0", "network": "wan", "autostart": True}
]
}Previously these merged by name, allowing the device configuration to override its logical network. This change treats lan and wan as separate identities and produces two interfaces for eth0.
|
I tested #414 and it works and is obviously a better solution than what i did here. |
3 participants
Checklist
Reference to Existing Issue
Closes #412 .
Description of Changes
Change the order and content of
list_identifiersto prevent merging of different vlan interfaces which share the same trunk port, therefore the same name.Added a test which merges two templates with 8021q interface definitions using the Openwrt backend which previously resulted in only one interface staying but now both stay.