fix(mikrotik): support RouterOS 7.21 wrapped/commented BGP output

RouterOS 7.21 emits BGP route detail differently over the looking-glass SSH
driver, which broke the structured parser:

- Active/best routes learned from a commented peer put the flag column and the
  ";;; comment" on a line of their own (afi=ip wraps to the next line), so the
  "A" flag was lost and no best route was highlighted. Derive active/filtered
  from the always-present "contribution=" field instead, falling back to the
  flag column for ROS v6.
- Long ".communities" lists wrap across terminal-width lines; everything after
  the wrap was silently dropped. De-wrap comma-separated lists before
  tokenizing.
- The garbage cleaner stripped any legend-looking line without "=", which also
  ate commented route flag lines. Match the "<char> - " legend shape instead.
- Parse the v7 dotted ".ext-communities" key.

Also restore the empty-BGP-table retry (4 attempts, 10s delay, MikroTik BGP
queries only) that re-runs when RouterOS answers before its routing table has
finished assembling.

Test fixtures use RFC 5737 (IPv4) documentation addresses and RFC 5398 example
ASNs only; no real network data.

Author: WilhelmZA

hyperglass/api/routes.py

@@ -3,6 +3,7 @@
 # Standard Library
 import json
 import time
+import asyncio
 import typing as t
 from datetime import UTC, datetime
 
@@ -36,6 +37,57 @@
     "query",
 )
 
+# MikroTik occasionally answers a BGP query before its routing table has finished
+# assembling, returning an empty route table on the first attempt. For MikroTik
+# BGP queries only, retry a few times (with a delay) while the result is empty.
+_MIKROTIK_PLATFORMS = ("mikrotik_routeros", "mikrotik_switchos", "mikrotik")
+_MIKROTIK_BGP_MAX_ATTEMPTS = 4
+_MIKROTIK_BGP_RETRY_DELAY = 10  # seconds
+
+
+def _is_empty_bgp_table(output: t.Any) -> bool:
+    """True if output is a structured BGP table containing no routes."""
+    if not is_type(output, OutputDataModel):
+        return False
+    try:
+        raw = json.loads(output.export_json())
+    except Exception:
+        return False
+    return (
+        isinstance(raw, dict)
+        and raw.get("count", None) == 0
+        and not raw.get("routes")
+    )
+
+
+async def _execute_query(data: "Query") -> t.Any:
+    """Execute a query, retrying MikroTik BGP queries that return an empty table."""
+    directive_name = getattr(getattr(data, "directive", None), "name", "") or ""
+    is_bgp_query = "bgp_" in data.query_type or "bgp" in directive_name.lower()
+    is_mikrotik_bgp = data.device.platform in _MIKROTIK_PLATFORMS and is_bgp_query
+    max_attempts = _MIKROTIK_BGP_MAX_ATTEMPTS if is_mikrotik_bgp else 1
+
+    _log = log.bind(directive=data.query_type, device=data.device.name)
+    if is_mikrotik_bgp:
+        _log.bind(max_attempts=max_attempts, retry_delay=_MIKROTIK_BGP_RETRY_DELAY).debug(
+            "MikroTik BGP empty-table retry enabled"
+        )
+
+    output = None
+    for attempt in range(1, max_attempts + 1):
+        if attempt > 1:
+            _log.bind(attempt=attempt, max_attempts=max_attempts).warning(
+                "MikroTik returned empty BGP table - retrying after {}s", _MIKROTIK_BGP_RETRY_DELAY
+            )
+            await asyncio.sleep(_MIKROTIK_BGP_RETRY_DELAY)
+
+        output = await execute(data)
+
+        if not (is_mikrotik_bgp and attempt < max_attempts and _is_empty_bgp_table(output)):
+            break
+
+    return output
+
 
 @get("/api/devices/{id:str}", dependencies={"devices": Provide(get_devices)})
 async def device(devices: Devices, id: str) -> APIDevice:
@@ -107,8 +159,8 @@ async def query(_state: HyperglassState, request: Request, data: Query) -> Query
                 structured=data.device.structured_output or False,
             )
         else:
-            # Pass request to execution module
-            output = await execute(data)
+            # Pass request to execution module (retries empty MikroTik BGP tables)
+            output = await _execute_query(data)
 
         endtime = time.time()
         elapsedtime = round(endtime - starttime, 4)

hyperglass/models/parsing/mikrotik.py

@@ -126,16 +126,21 @@ def peer_rid(self) -> str:
 
 
 def _extract_paths(lines: t.List[str]) -> MikrotikPaths:
-    """Simple count based on lines with dst/dst-address and preferred 'A' flag."""
+    """Count available/best paths.
+
+    Prefers the RouterOS v7 ``contribution=active`` marker (which sits on the same
+    line as ``dst-address``) and falls back to the legacy ``A`` flag for ROS v6.
+    """
     available = 0
     best = 0
     for raw in lines:
         if ("dst-address=" in raw) or (" dst=" in f" {raw} "):
             available += 1
-            m = FLAGS_RE.match(raw)
-            if m:
-                flags = set(m.group(1))
-                if "A" in flags:
+            if "contribution=active" in raw:
+                best += 1
+            elif "contribution=" not in raw:
+                m = FLAGS_RE.match(raw)
+                if m and "A" in set(m.group(1)):
                     best += 1
     return MikrotikPaths(available=available, best=best, select=best)
 
@@ -176,14 +181,19 @@ def _process_kv(route: dict, key: str, val: str):
     elif key in (".large-communities", "large-communities", "bgp-large-communities"):
         if val and val.lower() != "none":
             route["large_communities"] = [c.strip() for c in val.split(",") if c.strip()]
-    elif key == "bgp-ext-communities":
+    elif key in (".ext-communities", "ext-communities", "bgp-ext-communities"):
         if val and val.lower() != "none":
             route["ext_communities"] = [c.strip() for c in val.split(",") if c.strip()]
     elif key == "rpki":
         clean_val = val.strip().strip('"').lower()
         route["rpki_state"] = RPKI_STATE_MAP.get(clean_val, 2)
     elif key == "belongs-to":
         route["belongs_to"] = val
+    elif key == "contribution":
+        # ROS v7 selection state: active | candidate | filtered. This is the
+        # authoritative active/filtered signal (see _parse_route_block); the
+        # legacy flag column is only used as a fallback for ROS v6.
+        route["contribution"] = val.strip().lower()
 
 
 def _extract_route_entries(lines: t.List[str]) -> t.List[MikrotikRouteEntry]:
@@ -229,7 +239,13 @@ def _parse_route_block(block: t.List[str]) -> t.Optional[MikrotikRouteEntry]:
     if "dst-address=" not in full_block_text and " dst=" not in f" {full_block_text} ":
         return None
 
-    flags = ""
+    # RouterOS wraps long output at the terminal width, splitting unquoted
+    # comma-separated lists (.communities / .large-communities) across physical
+    # lines. Joining with a space above turns the wrap into ", " inside the list;
+    # collapse it back so the whole list is captured as a single token. RouterOS
+    # never emits ", " within a route value otherwise, so this is safe.
+    full_block_text = re.sub(r",\s+", ",", full_block_text)
+
     rd = {
         "prefix": "",
         "gateway": "",
@@ -249,25 +265,27 @@ def _parse_route_block(block: t.List[str]) -> t.Optional[MikrotikRouteEntry]:
         "rpki_state": RPKI_STATE_MAP.get("unknown", 2),
     }
 
-    # Interpret flags in the first line:
-    # - "A" => active (preferred)
-    # - otherwise not active
+    # Find all key=value tokens in the entire block
+    for k, v in TOKEN_RE.findall(full_block_text):
+        _process_kv(rd, k, v)
+
+    # Determine active/filtered state. Prefer the ROS v7 "contribution" marker,
+    # which always appears in the key=value data (robust to comment lines and to
+    # the flag column being wrapped/stripped). Fall back to the legacy flag
+    # column ("A" active; F/U/X filtered) for ROS v6, which has no contribution.
+    contribution = rd.pop("contribution", "")
     m = FLAGS_RE.match(block[0])
-    if m:
-        flags = m.group(1)
+    flags = m.group(1) if m else ""
+    if contribution:
+        rd["is_active"] = contribution == "active"
+        rd["is_filtered"] = contribution == "filtered"
+    elif flags:
         flag_set = set(flags)
-        is_active = "A" in flag_set
-        is_filtered = bool(flag_set & {"F", "f", "U", "u", "X", "x"})
-
-        if is_active:
+        if "A" in flag_set:
             rd["is_active"] = True
             rd["is_filtered"] = False
         else:
-            rd["is_filtered"] = is_filtered
-
-    # Find all key=value tokens in the entire block
-    for k, v in TOKEN_RE.findall(full_block_text):
-        _process_kv(rd, k, v)
+            rd["is_filtered"] = bool(flag_set & {"F", "f", "U", "u", "X", "x"})
 
     if rd["prefix"]:
         try:

hyperglass/plugins/_builtin/mikrotik_garbage_output.py

@@ -19,6 +19,13 @@
     from hyperglass.models.api.query import Query
 
 
+# Matches a RouterOS flag-legend line, e.g. "c - connect, s - static" or
+# "H - hw-offloaded; + - ecmp". Deliberately anchored to "<char> - " so it never
+# matches a route line such as "b   ;;; comment" or "Ab   afi=ip ..." (whose flag
+# column is followed by whitespace, not " - ").
+LEGEND_LINE_RE = re.compile(r"^[A-Za-z+]\s-\s")
+
+
 class MikrotikGarbageOutput(OutputPlugin):
     """Parse Mikrotik output to remove garbage before structured parsing."""
 
@@ -84,21 +91,14 @@ def _clean_traceroute_output(self, raw_output: str) -> str:
         if not tables:
             # Fallback to previous behavior: remove prompts and flags
             filtered_lines: t.List[str] = []
-            in_flags_section = False
             for line in lines:
                 stripped_line = line.strip()
                 if stripped_line.startswith("@") and stripped_line.endswith("] >"):
                     continue
                 if "[Q quit|D dump|C-z pause]" in stripped_line:
                     continue
-                if stripped_line.startswith("Flags:"):
-                    in_flags_section = True
+                if stripped_line.startswith("Flags:") or LEGEND_LINE_RE.match(stripped_line):
                     continue
-                if in_flags_section:
-                    if "=" in stripped_line:
-                        in_flags_section = False
-                    else:
-                        continue
                 filtered_lines.append(line)
             return "\n".join(filtered_lines)
 
@@ -223,7 +223,6 @@ def process(self, *, output: OutputType, query: "Query") -> Series[str]:
             # Original logic for other outputs (BGP routes, etc.)
             lines = raw_output.splitlines()
             filtered_lines = []
-            in_flags_section = False
 
             for line in lines:
                 stripped_line = line.strip()
@@ -236,17 +235,13 @@ def process(self, *, output: OutputType, query: "Query") -> Series[str]:
                 if "[Q quit|D dump|C-z pause]" in stripped_line:
                     continue
 
-                # Begin detecting the Flags section
-                if stripped_line.startswith("Flags:"):
-                    in_flags_section = True
-                    continue  # Skip the "Flags:" line itself
-
-                # Within the flags section, check whether the line is still part of it
-                if in_flags_section:
-                    if "=" in stripped_line:
-                        in_flags_section = False
-                    else:
-                        continue  # Skip the flag-legend lines
+                # Skip the flag legend: the "Flags:" header and its continuation
+                # lines ("c - connect, ...", "H - hw-offloaded; + - ecmp, ...").
+                # Matching the "<char> - " shape (rather than "no '=' present")
+                # ensures a commented route line like "b   ;;; peer-name" — which
+                # also has no "=" — is preserved instead of being dropped.
+                if stripped_line.startswith("Flags:") or LEGEND_LINE_RE.match(stripped_line):
+                    continue
 
                 filtered_lines.append(line)
 

hyperglass/plugins/tests/test_bgp_route_mikrotik.py

@@ -1,9 +1,11 @@
 """MikroTik structured BGP route parsing tests.
 
-The sample below is a trimmed, real `routing route print detail` capture from a
-RouterOS 7.20.6 device (CCR2216), reduced to one active and one filtered route.
-Long community lists are kept on a single line, matching what the device emits
-to the looking-glass SSH driver (no terminal-width wrapping).
+The samples below are synthetic captures shaped exactly like RouterOS
+`routing route print detail` output, but scrubbed of any real network data: all
+addresses use the RFC 5737 (IPv4) documentation ranges, autonomous-system
+numbers use the RFC 5398 documentation range (64496-64511), and peer/interface
+names are generic. Only the *structure* of the output is real, which is all the
+parser cares about.
 """
 
 # flake8: noqa
@@ -22,16 +24,18 @@
 if t.TYPE_CHECKING:
     from hyperglass.state import HyperglassState
 
+# One active and one filtered route, each emitted on a single (unwrapped) line,
+# as a wide terminal would produce.
 SAMPLE = """Flags: X - disabled, F - filtered, U - unreachable, A - active;
 c - connect, s - static, r - rip, b - bgp, n - bgp-net, o - ospf, i - isis, d - dhcp, v - vpn, m - modem, a - ldp-address, l - ldp-mapping, g - slaac, y - bgp-mpls-vpn, e - evpn;
 H - hw-offloaded; + - ecmp, B - blackhole
- Ab   afi=ip contribution=active dst-address=1.0.0.0/24 routing-table=main pref-src=192.0.2.1 gateway=196.60.8.198
-       immediate-gw=196.60.8.198%[sfp28-01] distance=20 scope=40 target-scope=10 belongs-to="bgp-IP-196.60.8.198"
-       rpki=valid bgp.session=NAP-JHB-CloudFlare-v4-1 .as-path="13335" .communities=13335:10045 .large-communities=328964:2000:0,328964:2001:0 .local-pref=250 .med=0 .origin=igp
+ Ab   afi=ip contribution=active dst-address=203.0.113.0/24 routing-table=main pref-src=192.0.2.1 gateway=198.51.100.1
+       immediate-gw=198.51.100.1%[ether1] distance=20 scope=40 target-scope=10 belongs-to="bgp-IP-198.51.100.1"
+       rpki=valid bgp.session=peer-ix-1 .as-path="64496" .communities=64496:10045 .large-communities=64500:2000:0,64500:2001:0 .local-pref=250 .med=0 .origin=igp
 
- Fb   afi=ip contribution=filtered dst-address=5.101.88.0/24 routing-table=main pref-src=192.0.2.1 gateway=192.0.2.2
-       immediate-gw=192.0.2.2%[sfp28-10] distance=20 scope=40 target-scope=10 belongs-to="bgp-IP-192.0.2.2"
-       rpki=invalid bgp.session=TRANSIT-ANGOLA-v4-1 .as-path="37468,41095,51601,50113,207569" .communities=37468:14100,37468:2000 .med=0 .origin=incomplete
+ Fb   afi=ip contribution=filtered dst-address=198.51.100.0/24 routing-table=main pref-src=192.0.2.1 gateway=192.0.2.2
+       immediate-gw=192.0.2.2%[ether2] distance=20 scope=40 target-scope=10 belongs-to="bgp-IP-192.0.2.2"
+       rpki=invalid bgp.session=transit-1 .as-path="64497,64498,64499,64500,64501" .communities=64497:14100,64497:2000 .med=0 .origin=incomplete
 """
 
 
@@ -58,21 +62,94 @@ def test_mikrotik_bgp_route(state):
     assert table.count == 2, f"Expected 2 routes, got {table.count}"
 
     by_prefix = {r.prefix: r for r in table.routes}
-    assert set(by_prefix) == {"1.0.0.0/24", "5.101.88.0/24"}
+    assert set(by_prefix) == {"203.0.113.0/24", "198.51.100.0/24"}
 
-    # Active route (flag "A"): RPKI valid (state 1), single-ASN path.
-    active = by_prefix["1.0.0.0/24"]
+    # Active route (contribution=active): RPKI valid (state 1), single-ASN path.
+    active = by_prefix["203.0.113.0/24"]
     assert active.active is True
     assert active.filtered is False
-    assert active.as_path == [13335]
-    assert active.next_hop == "196.60.8.198"
-    assert "13335:10045" in active.communities
+    assert active.as_path == [64496]
+    assert active.next_hop == "198.51.100.1"
+    assert "64496:10045" in active.communities
     assert active.rpki_state == 1  # valid
 
-    # Filtered route (flag "F" => filtered per the RouterOS flags legend): RPKI
-    # invalid (state 0), multi-ASN path.
-    filtered = by_prefix["5.101.88.0/24"]
+    # Filtered route (contribution=filtered): RPKI invalid (state 0), multi-ASN path.
+    filtered = by_prefix["198.51.100.0/24"]
     assert filtered.active is False
-    assert filtered.filtered is True, "F-flagged route should be marked filtered"
-    assert filtered.as_path == [37468, 41095, 51601, 50113, 207569]
+    assert filtered.filtered is True, "filtered route should be marked filtered"
+    assert filtered.as_path == [64497, 64498, 64499, 64500, 64501]
     assert filtered.rpki_state == 0  # invalid
+
+
+# Wrapped/commented capture as RouterOS 7.21 emits it over the looking-glass SSH
+# driver. Two properties that broke the legacy parser are exercised here:
+#   1. The *active* route is learned from a commented peer, so RouterOS emits the
+#      flag column ("Ab") and the ";;; comment" on a line of their own, with
+#      "afi=ip" wrapped to the next line. Active state must come from
+#      "contribution=active", not the (now-separated) flag column.
+#   2. A long ".communities" list is wrapped across two terminal-width lines; all
+#      members must survive (no truncation at the wrap point).
+#   3. An unquoted interface description containing spaces ("vlan10 CORE LINK")
+#      must not swallow the following key=value tokens.
+WRAPPED_SAMPLE = """Flags: X - disabled, F - filtered, U - unreachable, A - active;
+c - connect, s - static, r - rip, b - bgp, n - bgp-net, o - ospf, i - isis, d - dhcp, v - vpn, m - modem, a - ldp-address, l - ldp-mapping, g - slaac, y - bgp-mpls-vpn, e - evpn;
+H - hw-offloaded; + - ecmp, B - blackhole
+ Ab   ;;; peer-ix-1
+    afi=ip
+       contribution=active dst-address=203.0.113.0/24 routing-table=main
+       pref-src=192.0.2.1 gateway=198.51.100.1
+       immediate-gw=198.51.100.1%[ether1] IX PEERING
+       distance=20 scope=40 target-scope=10 belongs-to="bgp-IP-198.51.100.1"
+       rpki=valid bgp.session=peer-ix-1
+       .aggregator="64496:192.0.2.1" .as-path="64496"
+       .communities=64496:100,64496:200,64496:300,64497:100,64497:200,
+       64497:300,64498:100,64498:200,64498:300
+       .large-communities=64500:2000:0,64500:2001:0 .local-pref=250 .origin=igp
+       debug.fwp-ptr=0x00000000
+
+  b   afi=ip contribution=candidate dst-address=203.0.113.0/24 routing-table=main
+       pref-src=192.0.2.1 gateway=192.0.2.10
+       immediate-gw=192.0.2.10%vlan10 CORE LINK distance=200 scope=40
+       target-scope=30 belongs-to="bgp-IP-192.0.2.10"
+       bgp.session=core-edge-1 .as-path="64496" .communities=64496:400
+       .local-pref=250 .origin=igp
+       debug.fwp-ptr=0x00000001
+"""
+
+
+def test_mikrotik_bgp_route_wrapped_commented(state):
+    """Regression: RouterOS 7.21 wrapped output with a commented active route."""
+    parsed = MikrotikBGPTable.parse_text(WRAPPED_SAMPLE)
+
+    # Exactly one active (best) route must be detected, via contribution=active,
+    # even though its flag column sits on a separate ";;; comment" line.
+    active_entries = [r for r in parsed.routes if r.is_active]
+    assert len(active_entries) == 1, "commented active route must still be detected"
+    active = active_entries[0]
+    assert active.gateway == "198.51.100.1"
+
+    # The wrapped 9-member community list must be captured in full, not truncated
+    # at the terminal-width wrap point.
+    assert active.communities == [
+        "64496:100",
+        "64496:200",
+        "64496:300",
+        "64497:100",
+        "64497:200",
+        "64497:300",
+        "64498:100",
+        "64498:200",
+        "64498:300",
+    ]
+
+    # The candidate route's unquoted interface description ("vlan10 CORE LINK")
+    # must not have swallowed the following key=value tokens.
+    candidate = [r for r in parsed.routes if not r.is_active][0]
+    assert candidate.gateway == "192.0.2.10"
+    assert candidate.local_preference == 250
+    assert candidate.communities == ["64496:400"]
+
+    # The full conversion to the canonical BGP table must still succeed.
+    table = parsed.bgp_table()
+    assert isinstance(table, BGPRouteTable)
+    assert table.count == 2, f"Expected 2 routes, got {table.count}"