[fix/change] Bridge VLAN filtering parsing raises exception #356 #363

pandafy · 2025-08-27T16:59:10Z

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

Fixes #356
Fixes #357

coveralls · 2025-08-27T17:01:25Z

coverage: 99.212% (+0.01%) from 99.201%
when pulling ee9115a on issues/356-vlan-filter-parsing
into e664391 on master.

docs/source/backends/openwrt.rst

nemesifier · 2025-09-10T14:47:15Z

tests/openwrt/test_interfaces_dsa.py

                "name": "br-lan.1",
                "mtu": 1500,
                "mac": "61:4A:A0:D7:3F:0E",
+                "network": "if_br_lan_1",


Can you clarify the need for this change here?

Maybe with the changes we discussed it won't be needed to change this line (just for the sake of changing as few lines to tests as possible).

nemesifier

It works fine, I think we should mention in the docs the possibility of overriding the automatically generated VLANs with one example, what do you think?

I have some more questions below.

netjsonconfig/backends/openwrt/converters/interfaces.py

nemesifier · 2025-09-10T15:17:57Z

netjsonconfig/backends/openwrt/converters/interfaces.py

+            if "device" in interface_name:
+                interface_name = interface_name.replace("device", "int")
+            else:
+                interface_name = f"int_{interface_name}"


What does "int" refer to?

nemesifier

As discussed, let's update this example:
https://netjsonconfig.openwisp.org/en/1.1.1/backends/openwrt.html#using-vlan-filtering-on-a-bridge

To demonstrate the overriding and set a static IP / DHCP.

nemesifier · 2025-09-10T17:32:16Z

netjsonconfig/backends/openwrt/converters/interfaces.py

-            ".name": uci_vlan[".name"],
+            # To avoid conflicts, auto-generated interfaces are prefixed with "if"
+            # because UCI does not support multiple blocks with the same name.
+            ".name": f"if_{uci_name}_{vid}",


What about:

Suggested change

".name": f"if_{uci_name}_{vid}",

".name": f"{uci_name}_{vid}",

nemesifier · 2025-09-10T17:36:32Z

tests/openwrt/test_interfaces_dsa.py

                "name": "br-lan.1",
                "mtu": 1500,
                "mac": "61:4A:A0:D7:3F:0E",
+                "network": "if_br_lan_1",


Maybe with the changes we discussed it won't be needed to change this line (just for the sake of changing as few lines to tests as possible).

nemesifier

Parsing also has a couple issues:

the UCI name of device objects doesn't match expectations: device_int_switch instead of device_switch
the UCI name of bridge-vlan objects also doesn't match expectations: int_switch_100 which should have been switch_100 but I am suggesting to use vlan_switch_100 instead for consistency
same for interface objects: if_int_switch_100 while it should have been if_switch_100 but I am suggesting to just go with switch_100 (for consistency with current norms)

Fixes #356

nemesifier

I did another round of testing, here's my findings.

In the original config I see:

config interface 'jail'
        option device 'br-lan.80'
        option ifname 'jail'
        option proto 'dhcp'

After importing and generating again I see:

config interface 'br_lan_80'
  option device 'br-lan.80'
  option proto 'none'

I'd expect:

config interface 'br_lan_80'
  option device 'br-lan.80'
  option proto 'dhcp'

This seems to happen because another interface definition overrides it:

config interface 'vlan_br_lan_80'
        option device 'br-lan.80'
        option proto 'none'

How hard would it be to merge multiple definitions of the same interface (as long as the UCI names are different) instead of overriding?

Eg:

dhcp client + proto none (no address) = dhcp client
static ip setup + different static ip setup = 2 ip address objects
dhcp + static ip = 2 ip address object (1 dhcp and 1 static)

This would be consistent with what we're doing when we convert from NetJSON to UCI.

I also see something that doesn't seem to make sense:

config interface 'vlan_br_lan_80'
  option device 'br-lan.80'
  list ports 'lan1:t'
  list ports 'lan4:t'
  list ports 'lan6:u'
  option vlan '80'
  option proto 'none'

I was expecting:

config bridge-vlan 'vlan_br_lan_80'
  option device 'br-lan.80'
  list ports 'lan1:t'
  list ports 'lan4:t'
  list ports 'lan6:u'
  option vlan '80'

pandafy · 2025-09-18T17:28:50Z

I have tested the changes to parse the configuration tarball provided in the issue description. It generates the following netjson

{
    "interfaces": [
        {
            "name": "br-lan",
            "type": "bridge",
            "vlan_filtering": [
                {
                    "vlan": 100,
                    "ports": [
                        {
                            "ifname": "lan1",
                            "tagging": "u",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan2",
                            "tagging": "u",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan3",
                            "tagging": "u",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan4",
                            "tagging": "u",
                            "primary_vid": false
                        }
                    ]
                },
                {
                    "vlan": 178,
                    "ports": [
                        {
                            "ifname": "lan1",
                            "tagging": "t",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan2",
                            "tagging": "t",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan3",
                            "tagging": "t",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan4",
                            "tagging": "t",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan5",
                            "tagging": "u",
                            "primary_vid": false
                        }
                    ]
                },
                {
                    "vlan": 80,
                    "ports": [
                        {
                            "ifname": "lan1",
                            "tagging": "t",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan4",
                            "tagging": "t",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan6",
                            "tagging": "u",
                            "primary_vid": false
                        }
                    ]
                },
                {
                    "vlan": 70,
                    "ports": [
                        {
                            "ifname": "lan1",
                            "tagging": "t",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan4",
                            "tagging": "t",
                            "primary_vid": false
                        },
                        {
                            "ifname": "lan7",
                            "tagging": "u",
                            "primary_vid": false
                        }
                    ]
                }
            ],
            "bridge_members": [
                "lan1",
                "lan2",
                "lan3",
                "lan4",
                "lan5",
                "lan6",
                "lan7"
            ]
        },
        {
            "network": "debug",
            "name": "lan8",
            "type": "ethernet",
            "addresses": [
                {
                    "address": "192.168.1.1",
                    "mask": 24,
                    "proto": "static",
                    "family": "ipv4"
                }
            ]
        },
        {
            "name": "br-lan.178",
            "type": "ethernet",
            "addresses": [
                {
                    "proto": "dhcp",
                    "family": "ipv4"
                }
            ]
        },
        {
            "name": "br-lan.80",
            "type": "ethernet",
            "addresses": [
                {
                    "proto": "dhcp",
                    "family": "ipv4"
                }
            ]
        },
        {
            "name": "br-lan.70",
            "type": "ethernet",
            "addresses": [
                {
                    "proto": "dhcp",
                    "family": "ipv4"
                }
            ]
        }
    ],
    "type": "DeviceConfiguration"
}

nemesifier

The result looks very solid now, thanks @pandafy 👏

pandafy added this to OpenWISP Contributor's Board Aug 27, 2025

github-project-automation bot moved this to To do (general) in OpenWISP Contributor's Board Aug 27, 2025

pandafy force-pushed the issues/356-vlan-filter-parsing branch 2 times, most recently from 26bac47 to 1f609a8 Compare August 29, 2025 11:30

nemesifier added this to 25.09 Release Sep 3, 2025

nemesifier moved this to In progress in 25.09 Release Sep 3, 2025

nemesifier assigned pandafy Sep 6, 2025

nemesifier added the bug label Sep 6, 2025

nemesifier reviewed Sep 10, 2025

View reviewed changes

nemesifier mentioned this pull request Sep 10, 2025

[bug] bridge-vlan section is not generated when ula_prefix is present #357

Closed

nemesifier reviewed Sep 10, 2025

View reviewed changes

nemesifier requested changes Sep 10, 2025

View reviewed changes

github-project-automation bot moved this from To do (general) to In progress in OpenWISP Contributor's Board Sep 10, 2025

pandafy added 5 commits September 12, 2025 01:22

[fix] Bridge VLAN filtering parsing raises exception #356

fc0a64d

Fixes #356

[req-change] Allowed overriding the auto-generated VLAN interface

693e5ff

[docs] Improved code comments

05a9765

[docs] Updated docs

58a9f78

[req-changes] Updated logic and docs

c96a3ab

pandafy force-pushed the issues/356-vlan-filter-parsing branch from ee9115a to c96a3ab Compare September 11, 2025 19:52

[chores] Avoid flaky logic

d330fb0

nemesifier moved this from In progress to Ready for review/testing in 25.09 Release Sep 12, 2025

nemesifier requested changes Sep 12, 2025

View reviewed changes

github-project-automation bot moved this from Ready for review/testing to In progress in 25.09 Release Sep 12, 2025

[fix] Added handling for unordered interface blocks

a70826c

pandafy added 2 commits September 18, 2025 23:13

[fix] Generate fallback interface before parsing VLAN interfaces

dfbac50

[skip ci] Remove unused variables

3152588

nemesifier changed the title ~~[fix] Bridge VLAN filtering parsing raises exception #356~~ [fix/change] Bridge VLAN filtering parsing raises exception #356 Sep 18, 2025

nemesifier approved these changes Sep 18, 2025

View reviewed changes

nemesifier merged commit dc6acce into master Sep 18, 2025
3 checks passed

nemesifier deleted the issues/356-vlan-filter-parsing branch September 18, 2025 18:44

github-project-automation bot moved this from In progress to Done in 25.09 Release Sep 18, 2025

github-project-automation bot moved this from In progress to Done in OpenWISP Contributor's Board Sep 18, 2025

nemesifier mentioned this pull request Sep 23, 2025

[bug] VLAN filtering not appearing on devices openwisp/openwisp-controller#1100

Closed

pandafy mentioned this pull request Sep 24, 2025

[bug] Generated UCI for bridge VLAN filtering causes uci commit error #358

Closed

	".name": f"if_{uci_name}_{vid}",
	".name": f"{uci_name}_{vid}",

Uh oh!

Uh oh!

[fix/change] Bridge VLAN filtering parsing raises exception #356 #363

[fix/change] Bridge VLAN filtering parsing raises exception #356 #363

Uh oh!

Conversation

pandafy commented Aug 27, 2025

Checklist

Reference to Existing Issue

Uh oh!

coveralls commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nemesifier Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

nemesifier Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nemesifier Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

nemesifier Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

pandafy commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Aug 27, 2025 •

edited

Loading

nemesifier left a comment •

edited

Loading

pandafy commented Sep 18, 2025 •

edited

Loading