fix(vlans): read bridge-vlan membership from UCI, repair stale tests
get_vlans() relied on `bridge vlan show` for port membership. On devices whose BusyBox ships without the bridge/ip-full packages the command does not exist, _send_command returns the shell error, and the parser silently finds nothing in it. All that survived was the sub-interface fallback, which restates the bridge topology (br-ap tagged, br-ap.10 untagged) and never names the uplink port — the only port whose tagging matters. Measured on two Sophos AP100 (BusyBox 1.37.0): eth0 was absent from the output entirely, while UCI held ports='eth0:u*' for the management VLAN and 'eth0:t' for the rest. UCI bridge-vlan sections are now parsed as a second source. They are readable without the bridge binary and describe the configured state. Section collection goes through the type declaration, so named sections (network.apbr_vlan10) are recognised alongside anonymous ones — the old regex matched only @bridge-vlan[N], so a hand-built bridge was invisible even for name lookup. Runtime data keeps precedence where it exists, since that is what the kernel actually enforces. Option values are kept raw through collection; stripping quotes there would collapse ports='lan1:t' 'lan2:t' into a single mangled item. Also repairs TestGetVlans, red on master since get_vlans() moved to separate tagged/untagged lists while the tests still asserted the old 'interfaces' key, and TestGetFacts, which never learned about the number_of_interfaces key get_facts() sets deliberately. Both had left the interesting behaviour uncovered.
This commit is contained in:
Christian Manivong
+149
-8
@@ -154,9 +154,14 @@ class TestGetFacts:
|
||||
facts = driver.get_facts()
|
||||
assert set(facts.keys()) == {
|
||||
"vendor", "model", "hostname", "fqdn", "os_version",
|
||||
"serial_number", "uptime", "interface_list",
|
||||
"serial_number", "uptime", "interface_list", "number_of_interfaces",
|
||||
}
|
||||
|
||||
def test_number_of_interfaces_matches_list(self, driver):
|
||||
driver._send_command = self._make_send()
|
||||
facts = driver.get_facts()
|
||||
assert facts["number_of_interfaces"] == len(facts["interface_list"])
|
||||
|
||||
def test_vendor(self, driver):
|
||||
driver._send_command = self._make_send()
|
||||
assert driver.get_facts()["vendor"] == "OpenWrt"
|
||||
@@ -350,17 +355,26 @@ class TestGetVlans:
|
||||
result = driver.get_vlans()
|
||||
for vlan_data in result.values():
|
||||
assert "name" in vlan_data
|
||||
assert "interfaces" in vlan_data
|
||||
assert "tagged" in vlan_data
|
||||
assert "untagged" in vlan_data
|
||||
|
||||
def test_interfaces_for_vlan10(self, driver):
|
||||
def test_pvid_port_is_untagged_member(self, driver):
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
assert set(result["10"]["interfaces"]) == {"eth0", "br-lan"}
|
||||
assert set(result["1"]["untagged"]) == {"eth0", "br-lan"}
|
||||
assert result["1"]["tagged"] == []
|
||||
|
||||
def test_interfaces_for_vlan20(self, driver):
|
||||
def test_continuation_lines_are_tagged_members(self, driver):
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
assert set(result["20"]["interfaces"]) == {"eth0", "br-lan", "eth1"}
|
||||
assert set(result["10"]["tagged"]) == {"eth0", "br-lan"}
|
||||
assert result["10"]["untagged"] == []
|
||||
|
||||
def test_same_vlan_can_mix_tagged_and_untagged_ports(self, driver):
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
assert set(result["20"]["tagged"]) == {"eth0", "br-lan"}
|
||||
assert set(result["20"]["untagged"]) == {"eth1"}
|
||||
|
||||
def test_uci_names_applied(self, driver):
|
||||
driver._send_command = self._send
|
||||
@@ -373,11 +387,18 @@ class TestGetVlans:
|
||||
result = driver.get_vlans()
|
||||
assert result["1"]["name"] == ""
|
||||
|
||||
def test_no_duplicate_interfaces(self, driver):
|
||||
def test_no_duplicate_ports(self, driver):
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
for vlan_data in result.values():
|
||||
assert len(vlan_data["interfaces"]) == len(set(vlan_data["interfaces"]))
|
||||
for key in ("tagged", "untagged"):
|
||||
assert len(vlan_data[key]) == len(set(vlan_data[key]))
|
||||
|
||||
def test_port_is_never_both_tagged_and_untagged(self, driver):
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
for vlan_data in result.values():
|
||||
assert not set(vlan_data["tagged"]) & set(vlan_data["untagged"])
|
||||
|
||||
def test_empty_bridge_output(self, driver):
|
||||
driver._send_command = lambda cmd, **kw: ""
|
||||
@@ -385,6 +406,126 @@ class TestGetVlans:
|
||||
assert result == {}
|
||||
|
||||
|
||||
# A VLAN-aware bridge as configured by an AP profile: management VLAN untagged
|
||||
# (native/PVID) on the uplink, SSID VLANs tagged. Sections are *named*, which
|
||||
# is what OpenWrt writes for a hand-built or tool-provisioned bridge.
|
||||
UCI_NAMED_BRIDGE_VLANS = """\
|
||||
network.apbr=device
|
||||
network.apbr.name='br-ap'
|
||||
network.apbr.type='bridge'
|
||||
network.apbr.ports='eth0'
|
||||
network.apbr.vlan_filtering='1'
|
||||
network.apbr_vlan10=bridge-vlan
|
||||
network.apbr_vlan10.device='br-ap'
|
||||
network.apbr_vlan10.vlan='10'
|
||||
network.apbr_vlan10.ports='eth0:u*'
|
||||
network.apbr_vlan30=bridge-vlan
|
||||
network.apbr_vlan30.device='br-ap'
|
||||
network.apbr_vlan30.vlan='30'
|
||||
network.apbr_vlan30.ports='eth0:t'
|
||||
network.mgmt=interface
|
||||
network.mgmt.device='br-ap.10'
|
||||
network.mgmt.proto='dhcp'
|
||||
network.esche=interface
|
||||
network.esche.device='br-ap.30'
|
||||
network.esche.proto='none'
|
||||
"""
|
||||
|
||||
IP_LINK_AP = """\
|
||||
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue
|
||||
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel master br-ap
|
||||
3: br-ap: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue
|
||||
4: br-ap.10@br-ap: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue
|
||||
5: br-ap.30@br-ap: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue
|
||||
"""
|
||||
|
||||
|
||||
class TestGetVlansWithoutBridgeBinary:
|
||||
"""Membership must come from UCI when `bridge` is absent (BusyBox-only APs).
|
||||
|
||||
Devices such as the Sophos AP100 ship BusyBox without the `bridge`/`ip-full`
|
||||
packages, so `bridge vlan show` fails. Falling back to sub-interface
|
||||
topology alone reports `br-ap`/`br-ap.10` and never names the uplink port,
|
||||
which is the only port whose tagging actually matters.
|
||||
"""
|
||||
|
||||
def _send(self, cmd, **kw):
|
||||
if "bridge vlan" in cmd:
|
||||
return "ash: bridge: not found"
|
||||
if "uci show network" in cmd:
|
||||
return UCI_NAMED_BRIDGE_VLANS
|
||||
if "ip link show" in cmd:
|
||||
return IP_LINK_AP
|
||||
return ""
|
||||
|
||||
def test_untagged_pvid_uplink_membership(self, driver):
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
assert "eth0" in result["10"]["untagged"]
|
||||
assert "eth0" not in result["10"]["tagged"]
|
||||
|
||||
def test_tagged_uplink_membership(self, driver):
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
assert "eth0" in result["30"]["tagged"]
|
||||
assert "eth0" not in result["30"]["untagged"]
|
||||
|
||||
def test_named_sections_are_parsed(self, driver):
|
||||
"""The old parser only matched anonymous @bridge-vlan[N] sections."""
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
assert {"10", "30"} <= set(result)
|
||||
|
||||
def test_shell_error_is_not_parsed_as_membership(self, driver):
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
for vlan_data in result.values():
|
||||
assert "ash:" not in vlan_data["tagged"] + vlan_data["untagged"]
|
||||
|
||||
def test_name_falls_back_to_interface_section(self, driver):
|
||||
driver._send_command = self._send
|
||||
result = driver.get_vlans()
|
||||
assert result["10"]["name"] == "mgmt"
|
||||
assert result["30"]["name"] == "esche"
|
||||
|
||||
def test_runtime_membership_wins_over_uci(self, driver):
|
||||
"""`bridge vlan show` describes what the kernel actually does."""
|
||||
|
||||
def _send(cmd, **kw):
|
||||
if "bridge vlan" in cmd:
|
||||
return "port vlan-id\neth0 10 PVID Egress Untagged\n"
|
||||
if "uci show network" in cmd:
|
||||
# UCI claims tagged; the kernel says untagged.
|
||||
return (
|
||||
"network.apbr_vlan10=bridge-vlan\n"
|
||||
"network.apbr_vlan10.device='br-ap'\n"
|
||||
"network.apbr_vlan10.vlan='10'\n"
|
||||
"network.apbr_vlan10.ports='eth0:t'\n"
|
||||
)
|
||||
return ""
|
||||
|
||||
driver._send_command = _send
|
||||
result = driver.get_vlans()
|
||||
assert "eth0" in result["10"]["untagged"]
|
||||
assert "eth0" not in result["10"]["tagged"]
|
||||
|
||||
def test_multi_port_list_is_split(self, driver):
|
||||
def _send(cmd, **kw):
|
||||
if "uci show network" in cmd:
|
||||
return (
|
||||
"network.brv=bridge-vlan\n"
|
||||
"network.brv.device='br-lan'\n"
|
||||
"network.brv.vlan='20'\n"
|
||||
"network.brv.ports='lan1:t' 'lan2:t' 'lan3'\n"
|
||||
)
|
||||
return ""
|
||||
|
||||
driver._send_command = _send
|
||||
result = driver.get_vlans()
|
||||
assert set(result["20"]["tagged"]) == {"lan1", "lan2"}
|
||||
assert set(result["20"]["untagged"]) == {"lan3"}
|
||||
|
||||
|
||||
class TestConfigManagement:
|
||||
def test_load_merge_candidate(self, driver):
|
||||
driver.load_merge_candidate(config="uci set system.@system[0].hostname='MyRouter'")
|
||||
|
||||
Reference in New Issue
Block a user