diff --git a/README.md b/README.md index 245bdbb..6751e78 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@

- Version + Version License Docker TypeScript diff --git a/backend/package.json b/backend/package.json index c49ebf6..045dbf5 100644 --- a/backend/package.json +++ b/backend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-backend", - "version": "0.23.34-beta", + "version": "0.23.35-beta", "description": "Mikrotik Network Management Platform - Backend API", "main": "dist/index.js", "scripts": { diff --git a/backend/src/db/migrate.ts b/backend/src/db/migrate.ts index 0c78053..62f0a9d 100644 --- a/backend/src/db/migrate.ts +++ b/backend/src/db/migrate.ts @@ -886,6 +886,31 @@ CREATE INDEX IF NOT EXISTS idx_data_cap_sends ON lte_data_cap_sends(device_id, a -- The password is retained alongside. Key auth falling back to a password is a -- degraded state worth knowing about; key auth failing with no fallback is an -- outage. +-- What each bridge says about the spanning tree (#131). +-- +-- The root bridge used to be *inferred* from port roles, which never worked: the +-- code compared against 'root' while RouterOS reports 'root-port', so no device +-- ever matched and the topology simply crowned whichever device sorted first. +-- +-- RouterOS answers the question directly, so we ask instead of deducing. +-- root_bridge is the device's own verdict; root_bridge_id names the actual root +-- as priority.MAC, which is how a root outside the managed fleet stays +-- identifiable rather than being misattributed to something we do manage. +CREATE TABLE IF NOT EXISTS device_bridges ( + device_id INTEGER NOT NULL REFERENCES devices(id) ON DELETE CASCADE, + bridge_name VARCHAR(64) NOT NULL, + -- This bridge's own identifier, e.g. "0x8000.F4:1E:57:51:74:1E". + bridge_id VARCHAR(64), + root_bridge BOOLEAN, + root_bridge_id VARCHAR(64), + root_port VARCHAR(64), + root_path_cost INTEGER, + protocol_mode VARCHAR(16), + updated_at TIMESTAMPTZ DEFAULT NOW(), + PRIMARY KEY (device_id, bridge_name) +); +CREATE INDEX IF NOT EXISTS idx_device_bridges_rootid ON device_bridges(root_bridge_id); + -- Bulk command execution (#118). -- -- Waves are not a nicety. Running one command across sixty devices in parallel diff --git a/backend/src/routes/topology.ts b/backend/src/routes/topology.ts index 871027f..002f43b 100644 --- a/backend/src/routes/topology.ts +++ b/backend/src/routes/topology.ts @@ -18,9 +18,21 @@ export function setPollerService(p: PollerService): void { } +interface BridgeRow { + device_id: number; + bridge_name: string; + /** This bridge's own id, "priority.MAC" — how a root is matched to a device. */ + bridge_id: string | null; + root_bridge: boolean | null; + root_bridge_id: string | null; + root_port: string | null; + root_path_cost: number | null; + protocol_mode: string | null; +} + // GET /api/topology router.get('/', async (_req: Request, res: Response) => { - const [devices, allLinks, manualLinks, deviceMacs] = await Promise.all([ + const [devices, allLinks, manualLinks, deviceMacs, bridges] = await Promise.all([ query( `SELECT id, name, ip_address, model, device_type, status, ros_version, ip_addresses_jsonb FROM devices ORDER BY name ASC` @@ -47,12 +59,20 @@ router.get('/', async (_req: Request, res: Response) => { UNION SELECT device_id, mac_address FROM wireless_interfaces WHERE mac_address IS NOT NULL` ), + // What each bridge reports about the spanning tree. Authoritative, and the + // reason the root is no longer guessed from port roles (#131). + query( + `SELECT device_id, bridge_name, bridge_id, root_bridge, root_bridge_id, + root_port, root_path_cost, protocol_mode + FROM device_bridges` + ), ]); const graph = buildTopology(devices, allLinks, manualLinks, deviceMacs); res.json({ devices, + bridges, links: graph.links, externalNodes: graph.externalNodes, segConns: graph.segConns, diff --git a/backend/src/services/mikrotik/DeviceCollector.ts b/backend/src/services/mikrotik/DeviceCollector.ts index bdc4f50..9102e56 100644 --- a/backend/src/services/mikrotik/DeviceCollector.ts +++ b/backend/src/services/mikrotik/DeviceCollector.ts @@ -176,6 +176,10 @@ export class DeviceCollector { await this.updateClients(); await this.collectEvents(); await this.collectNeighbors(); + // Topology and spanning tree belong in a full resync: someone pressing Sync + // after rewiring expects the map to catch up, and previously STP only + // refreshed on the five-minute slow poll (#131). + await this.collectStp(); await this.collectLte(); // Wireless data belongs in a full resync too — without this, a manual sync // never refreshed radios/SSIDs (or pruned ones deleted on the device), so @@ -1054,15 +1058,72 @@ export class DeviceCollector { async collectStp(): Promise { try { + // Ask each bridge what it thinks the spanning tree looks like. + // + // This used to be inferred from port roles and got it wrong every time: + // the comparison was against 'root' while RouterOS reports 'root-port', so + // nothing matched and the topology crowned whichever device sorted first. + // The device already knows — `/interface/bridge/monitor` reports whether it + // is the root and names the root it sees (#131). + const bridges = await this.client + .execute('/interface/bridge/print', { detail: '' }) + .catch(() => [] as Record[]); + + const seen: string[] = []; + for (const bridge of bridges) { + const name = bridge['name']; + if (!name) continue; + const mon = await this.client + .execute('/interface/bridge/monitor', { numbers: name, once: '' }) + .catch(() => [] as Record[]); + const m = mon[0]; + if (!m) continue; + seen.push(name); + + const cost = parseInt(m['root-path-cost'] || '', 10); + await query( + `INSERT INTO device_bridges + (device_id, bridge_name, bridge_id, root_bridge, root_bridge_id, + root_port, root_path_cost, protocol_mode, updated_at) + VALUES ($1,$2,$3,$4,$5,$6,$7,$8,NOW()) + ON CONFLICT (device_id, bridge_name) DO UPDATE SET + bridge_id = EXCLUDED.bridge_id, root_bridge = EXCLUDED.root_bridge, + root_bridge_id = EXCLUDED.root_bridge_id, root_port = EXCLUDED.root_port, + root_path_cost = EXCLUDED.root_path_cost, + protocol_mode = EXCLUDED.protocol_mode, updated_at = NOW()`, + [ + this.device.id, name, m['bridge-id'] || null, + m['root-bridge'] === 'true', + m['root-bridge-id'] || null, + m['root-port'] || null, + Number.isFinite(cost) ? cost : null, + bridge['protocol-mode'] || null, + ] + ); + } + + // A bridge that has been deleted should stop being reported as root. + if (seen.length > 0) { + await query( + `DELETE FROM device_bridges WHERE device_id = $1 AND NOT (bridge_name = ANY($2::text[]))`, + [this.device.id, seen] + ); + } else { + await query(`DELETE FROM device_bridges WHERE device_id = $1`, [this.device.id]); + } + const bridgePorts = await this.client .execute('/interface/bridge/port/print', { detail: '' }) .catch(() => []); for (const port of bridgePorts) { const iface = (port['interface'] as string) || ''; - const role = (port['role'] as string) || ''; const bridgeName = (port['bridge'] as string) || ''; - // Derive state from role — alternate/backup are blocking, root/designated are forwarding + // RouterOS suffixes every role with "-port" — "root-port", + // "alternate-port". Comparing against the bare word matched nothing, so + // blocked links were drawn as forwarding. Normalised on write so both + // ends of the system agree on one vocabulary. + const role = ((port['role'] as string) || '').replace(/-port$/, ''); const state = (role === 'alternate' || role === 'backup') ? 'blocking' : 'forwarding'; if (!iface) continue; diff --git a/frontend/package.json b/frontend/package.json index 1fd5b30..fc69477 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-frontend", - "version": "0.23.34-beta", + "version": "0.23.35-beta", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/pages/TopologyPage.tsx b/frontend/src/pages/TopologyPage.tsx index 0fda1fe..e3b50a9 100644 --- a/frontend/src/pages/TopologyPage.tsx +++ b/frontend/src/pages/TopologyPage.tsx @@ -27,6 +27,7 @@ import type { TopologyDevice, TopologyLink, ExternalTopologyNode } from '../type import clsx from 'clsx'; import { useThemeStore } from '../store/themeStore'; import { layoutTree } from '../utils/topologyLayout'; +import { resolveStpRoot, type BridgeInfo, type RootVerdict } from '../utils/stpRoot'; // ─── Helpers ───────────────────────────────────────────────────────────────── @@ -209,6 +210,7 @@ function buildGraph( devices: TopologyDevice[], externalNodes: ExternalTopologyNode[], links: TopologyLink[], + bridges: BridgeInfo[], segConns: { src: string; dst: string; port: string }[], manualLinkIds: { id: number; from_device_id: number; to_device_id: number }[], connectMode: boolean, @@ -259,14 +261,26 @@ function buildGraph( } // ── STP root detection ─────────────────────────────────────────────────────── - const hasStp = links.some((l) => l.stp_role); - const haveRootPort = new Set(links.filter((l) => l.stp_role === 'root').map((l) => String(l.from_device_id))); - const stpRootId = hasStp ? devices.map((d) => String(d.id)).find((id) => !haveRootPort.has(id)) ?? '' : ''; + // + // Resolved from what each bridge reports, not inferred from port roles. The + // old inference compared against a role of 'root' while RouterOS reports + // 'root-port', so it never matched anything and the first device in the list + // was crowned every time (#131). + const hasStp = bridges.length > 0; + const rootVerdicts = new Map(); + const stpRootIds = new Set(); function pickRoot(componentIds: string[]): string { - if (hasStp) { - const r = componentIds.find((id) => deviceIds.has(id) && !haveRootPort.has(id)); - if (r) return r; + const managed = componentIds.filter((id) => deviceIds.has(id)); + if (hasStp && managed.length > 0) { + const verdict = resolveStpRoot(managed, bridges); + for (const id of componentIds) rootVerdicts.set(id, verdict); + if (verdict.deviceId && managed.includes(verdict.deviceId)) { + stpRootIds.add(verdict.deviceId); + return verdict.deviceId; + } + // An external or unknown root leaves the drawing to connectivity, but the + // crown is withheld rather than handed to an arbitrary device. } const inComp = new Set(componentIds); let best = componentIds[0], bestScore = -1; @@ -374,7 +388,7 @@ function buildGraph( id: String(d.id), type: 'deviceNode', position: positions.get(String(d.id)) ?? { x: 0, y: 0 }, - data: { ...d, isRootBridge: hasStp && String(d.id) === stpRootId, connectMode, orphan: orphanIds.has(String(d.id)) } as unknown as Record, + data: { ...d, isRootBridge: stpRootIds.has(String(d.id)), connectMode, orphan: orphanIds.has(String(d.id)) } as unknown as Record, })), ...allExtNodes.map((e) => ({ id: e.id, @@ -517,6 +531,7 @@ export default function TopologyPage() { (data.devices as TopologyDevice[]) || [], (data.externalNodes as ExternalTopologyNode[]) || [], (data.links as TopologyLink[]) || [], + (data.bridges as BridgeInfo[]) || [], (data.segConns as { src: string; dst: string; port: string }[]) || [], (data.manualLinkIds as { id: number; from_device_id: number; to_device_id: number }[]) || [], connectMode, diff --git a/frontend/src/services/api.ts b/frontend/src/services/api.ts index 18a0bd1..1531392 100644 --- a/frontend/src/services/api.ts +++ b/frontend/src/services/api.ts @@ -1014,6 +1014,8 @@ export const topologyApi = { devices: Device[]; links: import('../types').TopologyLink[]; externalNodes: import('../types').ExternalTopologyNode[]; + /** What each bridge reports about the spanning tree (#131). */ + bridges: import('../utils/stpRoot').BridgeInfo[]; segConns: { src: string; dst: string; port: string }[]; manualLinkIds: { id: number; from_device_id: number; to_device_id: number }[]; }>('/topology'), diff --git a/frontend/src/utils/stpRoot.test.ts b/frontend/src/utils/stpRoot.test.ts new file mode 100644 index 0000000..d8090bd --- /dev/null +++ b/frontend/src/utils/stpRoot.test.ts @@ -0,0 +1,131 @@ +import { describe, it, expect } from 'vitest'; +import { resolveStpRoot, bridgePriority, type BridgeInfo } from './stpRoot'; + +const RB4011 = '0x1000.2C:C8:1B:05:2B:B1'; // priority 4096 +const AP_A = '0x2000.AA:AA:AA:AA:AA:AA'; // priority 8192 +const AP_B = '0x2000.BB:BB:BB:BB:BB:BB'; +const FOREIGN = '0x2000.70:A7:41:EF:8F:FB'; // real root, not in the fleet + +const b = ( + device_id: number, bridge_id: string, root_bridge: boolean, root_bridge_id: string | null, +): BridgeInfo => ({ + device_id, bridge_name: 'bridge', bridge_id, root_bridge, root_bridge_id, + root_port: null, root_path_cost: null, protocol_mode: 'rstp', +}); + +describe('resolveStpRoot — the reported case', () => { + // RB4011 at priority 4096 is root; two APs at 8192 both point at it. The old + // logic crowned an AP because it matched port role 'root' instead of + // 'root-port', so nothing matched and the first device in the list won. + const fleet = [ + b(1, RB4011, true, RB4011), + b(2, AP_A, false, RB4011), + b(3, AP_B, false, RB4011), + ]; + + it('names the device that says it is root', () => { + const v = resolveStpRoot(['1', '2', '3'], fleet); + expect(v.deviceId).toBe('1'); + expect(v.confidence).toBe('reported'); + }); + + it('does not depend on the order devices arrive in', () => { + // The precise failure being fixed: the answer used to be whichever sorted + // first, so it must now be stable under any ordering. + for (const order of [['1','2','3'], ['2','3','1'], ['3','2','1'], ['2','1','3']]) { + expect(resolveStpRoot(order, fleet).deviceId).toBe('1'); + } + }); + + it('is unmoved by shuffling the bridge rows themselves', () => { + const shuffled = [fleet[2], fleet[0], fleet[1]]; + expect(resolveStpRoot(['1', '2', '3'], shuffled).deviceId).toBe('1'); + }); +}); + +describe('resolveStpRoot — a root we do not manage', () => { + // Observed on a real switch: it points at priority 8192 on a MAC belonging to + // no device in the inventory. + const fleet = [ + b(8, AP_A, false, FOREIGN), + b(9, AP_B, false, FOREIGN), + ]; + + it('declines to nominate one of ours', () => { + const v = resolveStpRoot(['8', '9'], fleet); + expect(v.deviceId).toBeNull(); + expect(v.confidence).toBe('external'); + }); + + it('reports the bridge id so the operator can identify it', () => { + expect(resolveStpRoot(['8', '9'], fleet).rootBridgeId).toBe(FOREIGN); + }); +}); + +describe('resolveStpRoot — separate spanning trees', () => { + it('gives each component its own root', () => { + // Two of four devices legitimately report root at once on a mixed fleet; + // "the root" is per tree, not per fleet. + const fleet = [ + b(1, RB4011, true, RB4011), + b(2, AP_A, false, RB4011), + b(7, AP_B, true, AP_B), + ]; + expect(resolveStpRoot(['1', '2'], fleet).deviceId).toBe('1'); + expect(resolveStpRoot(['7'], fleet).deviceId).toBe('7'); + }); + + it('prefers the claimant others actually point at', () => { + const fleet = [ + b(1, RB4011, true, RB4011), + b(2, AP_A, true, AP_A), + b(3, AP_B, false, RB4011), + ]; + expect(resolveStpRoot(['1', '2', '3'], fleet).deviceId).toBe('1'); + }); +}); + +describe('resolveStpRoot — degraded input', () => { + it('matches on bridge id when nobody claims the crown', () => { + const fleet = [ + b(1, RB4011, false, RB4011), + b(2, AP_A, false, RB4011), + ]; + const v = resolveStpRoot(['1', '2'], fleet); + expect(v.deviceId).toBe('1'); + expect(v.confidence).toBe('matched'); + }); + + it('compares bridge ids without caring about MAC case', () => { + const fleet = [ + b(1, RB4011.toUpperCase(), false, RB4011.toLowerCase()), + b(2, AP_A, false, RB4011.toLowerCase()), + ]; + expect(resolveStpRoot(['1', '2'], fleet).deviceId).toBe('1'); + }); + + it('admits it does not know when there is no STP data', () => { + // Better than guessing: the caller falls back to a connectivity heuristic + // and can say so, rather than presenting a guess as fact. + expect(resolveStpRoot(['1', '2'], [])).toEqual({ + deviceId: null, confidence: 'unknown', rootBridgeId: null, + }); + }); + + it('ignores bridges belonging to devices outside the component', () => { + const fleet = [b(99, RB4011, true, RB4011)]; + expect(resolveStpRoot(['1', '2'], fleet).confidence).toBe('unknown'); + }); +}); + +describe('bridgePriority', () => { + it('reads the priority out of a bridge id', () => { + expect(bridgePriority(RB4011)).toBe(4096); + expect(bridgePriority(AP_A)).toBe(8192); + expect(bridgePriority('0x8000.F4:1E:57:51:74:1E')).toBe(32768); + }); + it('copes with nothing usable', () => { + expect(bridgePriority(null)).toBeNull(); + expect(bridgePriority('garbage')).toBeNull(); + }); +}); diff --git a/frontend/src/utils/stpRoot.ts b/frontend/src/utils/stpRoot.ts new file mode 100644 index 0000000..7c3ff1e --- /dev/null +++ b/frontend/src/utils/stpRoot.ts @@ -0,0 +1,112 @@ +/** + * Which device is the spanning-tree root, per connected component. + * + * This was previously inferred: find a device with no port whose role is "root". + * The inference never fired, because RouterOS reports the role as "root-port" + * and the comparison was against "root" — so no device ever matched and the + * topology crowned whichever device happened to sort first, consistently and + * confidently (#131). + * + * The device already knows the answer. `/interface/bridge/monitor` reports + * whether it is the root and names the root it can see, so the job here is + * reading that rather than deducing it. + * + * Two properties of real networks shape the result: + * + * **There is a root per spanning tree, not per fleet.** Separate bridge domains + * each have their own, and on a small mixed fleet several devices legitimately + * report `root-bridge: true` at once. Roots are therefore resolved within a + * connected component. + * + * **The root may not be ours.** A managed switch can point at a bridge id + * belonging to equipment nobody has added. Naming a local device in that case + * would be worse than admitting we cannot see it. + */ + +export interface BridgeInfo { + device_id: number; + bridge_name: string; + bridge_id: string | null; + root_bridge: boolean | null; + root_bridge_id: string | null; + root_port: string | null; + root_path_cost: number | null; + protocol_mode: string | null; +} + +export type RootConfidence = + /** A device in this component reports itself as root. */ + | 'reported' + /** No local device claims it, but one owns the bridge id others point at. */ + | 'matched' + /** Devices agree on a root we do not manage. */ + | 'external' + /** Nothing usable; the caller should fall back to a connectivity heuristic. */ + | 'unknown'; + +export interface RootVerdict { + /** Device id as a string, matching topology node ids. Null when not ours. */ + deviceId: string | null; + confidence: RootConfidence; + /** The bridge id of the actual root, when known — shown when it is not ours. */ + rootBridgeId: string | null; +} + +/** Bridge ids are compared case-insensitively; RouterOS is inconsistent about MAC case. */ +const norm = (id: string | null | undefined): string => (id || '').trim().toLowerCase(); + +/** + * Resolve the root for one component. + * + * `componentDeviceIds` are the managed devices in this component, as strings. + */ +export function resolveStpRoot( + componentDeviceIds: string[], + bridges: BridgeInfo[], +): RootVerdict { + const inComponent = new Set(componentDeviceIds); + const local = bridges.filter((b) => inComponent.has(String(b.device_id))); + if (local.length === 0) return { deviceId: null, confidence: 'unknown', rootBridgeId: null }; + + // A device that says it is the root, is the root. + const claims = local.filter((b) => b.root_bridge === true); + if (claims.length === 1) { + return { deviceId: String(claims[0].device_id), confidence: 'reported', rootBridgeId: claims[0].bridge_id }; + } + + // Several claims inside one component means several bridge domains share it — + // a device bridging two segments, for instance. Prefer the one others actually + // point at; that is the tree the component is really hanging from. + const pointedAt = new Set(local.map((b) => norm(b.root_bridge_id)).filter(Boolean)); + if (claims.length > 1) { + const agreed = claims.find((b) => pointedAt.has(norm(b.bridge_id))); + const chosen = agreed ?? claims[0]; + return { deviceId: String(chosen.device_id), confidence: agreed ? 'reported' : 'matched', rootBridgeId: chosen.bridge_id }; + } + + // Nobody claims it. Does anyone own the bridge id the others are pointing at? + for (const target of pointedAt) { + const owner = local.find((b) => norm(b.bridge_id) === target); + if (owner) { + return { deviceId: String(owner.device_id), confidence: 'matched', rootBridgeId: owner.bridge_id }; + } + } + + // They agree on something we do not manage. Say so rather than nominating one + // of ours, which is exactly the mistake this replaces. + const external = [...pointedAt][0]; + if (external) { + const withId = local.find((b) => norm(b.root_bridge_id) === external); + return { deviceId: null, confidence: 'external', rootBridgeId: withId?.root_bridge_id ?? null }; + } + + return { deviceId: null, confidence: 'unknown', rootBridgeId: null }; +} + +/** Priority out of a bridge id like "0x8000.F4:1E:57:51:74:1E", for display. */ +export function bridgePriority(bridgeId: string | null): number | null { + const m = (bridgeId || '').match(/^0x([0-9a-f]+)\./i); + if (!m) return null; + const n = parseInt(m[1], 16); + return Number.isFinite(n) ? n : null; +} diff --git a/frontend/src/version.ts b/frontend/src/version.ts index 79851c8..86be208 100644 --- a/frontend/src/version.ts +++ b/frontend/src/version.ts @@ -1 +1 @@ -export const APP_VERSION = 'v0.23.34 Beta'; +export const APP_VERSION = 'v0.23.35 Beta';