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.

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
+        # Enrich with the configured membership from UCI bridge-vlan sections.
-        uci_entries: dict[str, dict[str, str]] = {}
+        # On devices whose BusyBox ships without the `bridge` utility this is
-        for line in uci_out.splitlines():
+        # the only source that names the physical ports at all — the
-            m = re.match(r"network\.@bridge-vlan\[(\d+)\]\.(\w+)='([^']*)'", line.strip())
+        # sub-interface fallback below can only ever report br-ap/br-ap.N.
-            if m:
+        # Runtime data from `bridge vlan show` describes what the kernel
-                idx, key, value = m.group(1), m.group(2), m.group(3)
+        # actually enforces, so it keeps precedence where both are present.
-                uci_entries.setdefault(idx, {})[key] = value
+        for entry in _uci_sections_of_type(uci_out, "bridge-vlan"):
-
+            if "vlan" not in entry:
-        for entry in uci_entries.values():
+                continue
-            if "vlan" in entry and "name" in entry:
+            try:
-                vlan_id = str(int(entry["vlan"]))
+                vlan_id = str(int(_uci_scalar(entry["vlan"])))
-                if vlan_id in vlans:
+            except ValueError:
-                    vlans[vlan_id]["name"] = entry["name"]
+                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:

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: <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'")