diff --git a/napalm_openwrt/vlan_mixin.py b/napalm_openwrt/vlan_mixin.py index 3a3e08f..9b29e25 100644 --- a/napalm_openwrt/vlan_mixin.py +++ b/napalm_openwrt/vlan_mixin.py @@ -17,6 +17,63 @@ import re from typing import Any +def _split_uci_list(value: str) -> list[str]: + """Split a ``uci show`` option value into its items. + + List options are rendered space-separated with each item quoted + (``ports='lan1:t' 'lan2:t'``); scalar options carry a single quoted value. + """ + items = re.findall(r"'([^']*)'", value) + if items: + return [i for i in items if i] + stripped = value.strip().strip("'") + return [stripped] if stripped else [] + + +def _uci_scalar(value: str) -> str: + """Return the single unquoted value of a scalar ``uci show`` option.""" + items = _split_uci_list(value) + return items[0] if items else "" + + +def _parse_port_spec(spec: str) -> tuple[str, bool]: + """Return ``(port, is_tagged)`` for an OpenWrt bridge-vlan port spec. + + ``eth0:t`` is a tagged member; ``eth0:u*`` (untagged + PVID), ``eth0:*`` + and a bare ``eth0`` are untagged members. + """ + port, _, flags = spec.partition(":") + return port, "t" in flags + + +def _uci_sections_of_type(uci_out: str, sec_type: str) -> list[dict[str, str]]: + """Collect all ``network`` sections of *sec_type* from ``uci show network``. + + Handles both anonymous (``network.@bridge-vlan[0]``) and named + (``network.apbr_vlan10``) sections — only the former was recognised before, + so tool- or hand-provisioned bridges were invisible. + + Option values are kept raw (quotes included) so that list options stay + distinguishable from scalars; use :func:`_uci_scalar` or + :func:`_split_uci_list` to read them. + """ + decl = re.compile(rf"^network\.(\S+)={re.escape(sec_type)}$") + option = re.compile(r"^network\.(\S+?)\.(\w+)=(.*)$") + + section_ids: set[str] = set() + for line in uci_out.splitlines(): + m = decl.match(line.strip()) + if m: + section_ids.add(m.group(1)) + + sections: dict[str, dict[str, str]] = {sid: {} for sid in section_ids} + for line in uci_out.splitlines(): + m = option.match(line.strip()) + if m and m.group(1) in section_ids: + sections[m.group(1)][m.group(2)] = m.group(3).strip() + return list(sections.values()) + + class OpenWrtVLANMixin: """Mixin providing VLAN and network-instance NAPALM getters.""" @@ -68,19 +125,28 @@ class OpenWrtVLANMixin: if current_port not in vlans[vlan_id]["tagged"]: vlans[vlan_id]["tagged"].append(current_port) - # Enrich with UCI VLAN names from explicit bridge-vlan sections - uci_entries: dict[str, dict[str, str]] = {} - for line in uci_out.splitlines(): - m = re.match(r"network\.@bridge-vlan\[(\d+)\]\.(\w+)='([^']*)'", line.strip()) - if m: - idx, key, value = m.group(1), m.group(2), m.group(3) - uci_entries.setdefault(idx, {})[key] = value - - for entry in uci_entries.values(): - if "vlan" in entry and "name" in entry: - vlan_id = str(int(entry["vlan"])) - if vlan_id in vlans: - vlans[vlan_id]["name"] = entry["name"] + # Enrich with the configured membership from UCI bridge-vlan sections. + # On devices whose BusyBox ships without the `bridge` utility this is + # the only source that names the physical ports at all — the + # sub-interface fallback below can only ever report br-ap/br-ap.N. + # Runtime data from `bridge vlan show` describes what the kernel + # actually enforces, so it keeps precedence where both are present. + for entry in _uci_sections_of_type(uci_out, "bridge-vlan"): + if "vlan" not in entry: + continue + try: + vlan_id = str(int(_uci_scalar(entry["vlan"]))) + except ValueError: + continue + vlan = vlans.setdefault(vlan_id, {"name": "", "tagged": [], "untagged": []}) + uci_name = _uci_scalar(entry.get("name", "")) + if uci_name and not vlan["name"]: + vlan["name"] = uci_name + for spec in _split_uci_list(entry.get("ports", "")): + port, tagged = _parse_port_spec(spec) + if port in vlan["tagged"] or port in vlan["untagged"]: + continue + vlan["tagged" if tagged else "untagged"].append(port) # Also derive VLAN names from UCI network interface sections that # reference subinterfaces like eth0.N or br-ap.N: diff --git a/tests/unit/test_driver.py b/tests/unit/test_driver.py index 691e16f..4f92d08 100644 --- a/tests/unit/test_driver.py +++ b/tests/unit/test_driver.py @@ -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: mtu 65536 qdisc noqueue +2: eth0: mtu 1500 qdisc fq_codel master br-ap +3: br-ap: mtu 1500 qdisc noqueue +4: br-ap.10@br-ap: mtu 1500 qdisc noqueue +5: br-ap.30@br-ap: 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'")