From b4ef6b2020cc7171cebf395fe6684a79ea1aed34 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 10 Mar 2026 15:44:46 -0500 Subject: [PATCH] Improve topology discovery matching --- lib/towerops/snmp/neighbor_discovery.ex | 61 +++++- lib/towerops/topology.ex | 187 ++++++++++++++---- lib/towerops/topology/identifier.ex | 93 +++++++++ .../towerops/snmp/neighbor_discovery_test.exs | 19 ++ test/towerops/topology_test.exs | 109 ++++++++++ 5 files changed, 423 insertions(+), 46 deletions(-) create mode 100644 lib/towerops/topology/identifier.ex diff --git a/lib/towerops/snmp/neighbor_discovery.ex b/lib/towerops/snmp/neighbor_discovery.ex index 6d275130..bdf5ae39 100644 --- a/lib/towerops/snmp/neighbor_discovery.ex +++ b/lib/towerops/snmp/neighbor_discovery.ex @@ -297,16 +297,21 @@ defmodule Towerops.Snmp.NeighborDiscovery do end defp format_chassis_id(value) when is_binary(value) do - # LLDP Chassis ID has subtype byte + value - # If it's a printable string (subtype 4 = MAC as text, 7 = local), use it directly - if String.valid?(value) and String.printable?(value) do - String.trim(value) - else - # Otherwise convert binary to hex MAC format - value - |> :binary.bin_to_list() - |> Enum.map_join(":", &String.pad_leading(Integer.to_string(&1, 16), 2, "0")) - |> String.downcase() + cond do + value == "" -> + nil + + printable_binary?(value) -> + String.trim(value) + + byte_size(value) == 6 -> + format_binary_mac(value) + + byte_size(value) > 1 -> + parse_chassis_id_with_subtype(value) + + true -> + nil end end @@ -323,6 +328,42 @@ defmodule Towerops.Snmp.NeighborDiscovery do defp format_port_id(value), do: to_string_safe(value) + defp printable_binary?(value) do + String.valid?(value) and String.printable?(value) + end + + defp parse_chassis_id_with_subtype(<>) do + case subtype do + 4 when byte_size(rest) == 6 -> + format_binary_mac(rest) + + 5 -> + format_ip_address(rest) + + _ -> + cond do + subtype == 7 and printable_binary?(rest) -> + String.trim(rest) + + printable_binary?(rest) -> + String.trim(rest) + + byte_size(rest) == 6 -> + format_binary_mac(rest) + + true -> + format_binary_mac(<>) + end + end + end + + defp format_binary_mac(value) when is_binary(value) do + value + |> :binary.bin_to_list() + |> Enum.map_join(":", &String.pad_leading(Integer.to_string(&1, 16), 2, "0")) + |> String.downcase() + end + defp format_ip_address(value) when is_binary(value) and byte_size(value) == 4 do value |> :binary.bin_to_list() diff --git a/lib/towerops/topology.ex b/lib/towerops/topology.ex index 02f7f258..28a7184a 100644 --- a/lib/towerops/topology.ex +++ b/lib/towerops/topology.ex @@ -12,11 +12,13 @@ defmodule Towerops.Topology do alias Towerops.Snmp.ArpEntry alias Towerops.Snmp.Device, as: SnmpDevice alias Towerops.Snmp.Interface + alias Towerops.Snmp.IpAddress alias Towerops.Snmp.MacAddress alias Towerops.Snmp.Neighbor alias Towerops.Topology.DeviceLink alias Towerops.Topology.DeviceLinkEvidence alias Towerops.Topology.DeviceNeighbor + alias Towerops.Topology.Identifier alias Towerops.Topology.Lldp require Logger @@ -39,16 +41,30 @@ defmodule Towerops.Topology do left_join: sd in assoc(d, :snmp_device), left_join: i in Interface, on: sd.id == i.snmp_device_id, - preload: [snmp_device: {sd, interfaces: i}] + left_join: ip in IpAddress, + on: i.id == ip.snmp_interface_id, + preload: [snmp_device: {sd, interfaces: {i, ip_addresses: ip}}] ) ) - by_ip = Map.new(devices, fn d -> {d.ip_address, d.id} end) + by_ip = + devices + |> Enum.flat_map(fn device -> + [device.ip_address | device_interface_ips(device)] + |> Enum.map(&Identifier.normalize_ip/1) + |> Enum.reject(&is_nil/1) + |> Enum.map(&{&1, device.id}) + end) + |> Map.new() by_name = devices - |> Enum.filter(&(&1.name != nil)) - |> Map.new(&{String.downcase(&1.name), &1.id}) + |> Enum.flat_map(fn device -> + [device.name, device.snmp_device && device.snmp_device.sys_name] + |> Enum.flat_map(&Identifier.candidate_names/1) + |> Enum.map(&{&1, device.id}) + end) + |> Map.new() by_mac = devices @@ -60,7 +76,9 @@ defmodule Towerops.Topology do sd -> sd.interfaces |> Enum.filter(&(&1.if_phys_address != nil)) - |> Enum.map(&{&1.if_phys_address, device.id}) + |> Enum.map(&Identifier.normalize_mac(&1.if_phys_address)) + |> Enum.reject(&is_nil/1) + |> Enum.map(&{&1, device.id}) end end) |> Map.new() @@ -71,19 +89,23 @@ defmodule Towerops.Topology do @doc """ Finds a device ID by IP address from the lookup. """ - def find_device_by_ip(lookup, ip), do: Map.get(lookup.by_ip, ip) + def find_device_by_ip(lookup, ip), do: Map.get(lookup.by_ip, Identifier.normalize_ip(ip)) @doc """ Finds a device ID by name (case-insensitive) from the lookup. """ - def find_device_by_name(lookup, name) when is_binary(name), do: Map.get(lookup.by_name, String.downcase(name)) + def find_device_by_name(lookup, name) when is_binary(name) do + name + |> Identifier.candidate_names() + |> Enum.find_value(&Map.get(lookup.by_name, &1)) + end def find_device_by_name(_lookup, _name), do: nil @doc """ Finds a device ID by MAC address from the lookup. """ - def find_device_by_mac(lookup, mac), do: Map.get(lookup.by_mac, mac) + def find_device_by_mac(lookup, mac), do: Map.get(lookup.by_mac, Identifier.normalize_mac(mac)) @doc """ Resolves a device ID from evidence fields, trying MAC first, then IP, then name. @@ -136,9 +158,9 @@ defmodule Towerops.Topology do evidence_type: evidence_type, confidence: confidence, source_interface_id: row.source_interface_id, - remote_mac: row.remote_chassis_id, - remote_ip: row.remote_address, - remote_name: row.remote_system_name, + remote_mac: Identifier.normalize_mac(row.remote_chassis_id), + remote_ip: Identifier.normalize_ip(row.remote_address), + remote_name: Identifier.normalize_name(row.remote_system_name), remote_port: row.remote_port_id, remote_capabilities: row.remote_capabilities, evidence_data: %{ @@ -175,7 +197,7 @@ defmodule Towerops.Topology do evidence_type: "mac_on_interface", confidence: 0.7, source_interface_id: row.interface_id, - remote_mac: row.mac_address, + remote_mac: Identifier.normalize_mac(row.mac_address), remote_ip: nil, remote_name: nil, remote_port: nil, @@ -217,8 +239,8 @@ defmodule Towerops.Topology do evidence_type: "arp_entry", confidence: 0.6, source_interface_id: row.interface_id, - remote_mac: row.mac_address, - remote_ip: row.ip_address, + remote_mac: Identifier.normalize_mac(row.mac_address), + remote_ip: Identifier.normalize_ip(row.ip_address), remote_name: nil, remote_port: nil, remote_capabilities: nil, @@ -244,15 +266,7 @@ defmodule Towerops.Topology do evidence_attrs = Map.get(attrs, :evidence, []) link_attrs = Map.delete(attrs, :evidence) - existing = - Repo.one( - from(l in DeviceLink, - where: - l.source_device_id == ^link_attrs.source_device_id and - l.source_interface_id == ^link_attrs[:source_interface_id] and - l.discovered_remote_mac == ^link_attrs[:discovered_remote_mac] - ) - ) + existing = find_existing_link(link_attrs) result = case existing do @@ -477,11 +491,11 @@ defmodule Towerops.Topology do defp build_discovered_nodes(links, "all") do links |> Enum.filter(&is_nil(&1.target_device_id)) - |> Enum.uniq_by(& &1.discovered_remote_mac) + |> Enum.uniq_by(&discovered_node_id/1) |> Enum.map(fn link -> %{ - id: "discovered_#{link.discovered_remote_mac}", - label: link.discovered_remote_name || link.discovered_remote_mac, + id: discovered_node_id(link), + label: link.discovered_remote_name || link.discovered_remote_ip || link.discovered_remote_mac, type: :unknown, device_role: nil, status: :unknown, @@ -503,7 +517,7 @@ defmodule Towerops.Topology do target_id = if link.target_device_id, do: link.target_device_id, - else: "discovered_#{link.discovered_remote_mac}" + else: discovered_node_id(link) %{ id: link.id, @@ -536,12 +550,13 @@ defmodule Towerops.Topology do defp upsert_grouped_evidence({_key, evidence_list}, device_id, lookup, now) do best = Enum.max_by(evidence_list, & &1.confidence) + remote_identity = summarize_remote_identity(evidence_list) matched_id = resolve_device(lookup, %{ - mac: best.remote_mac, - ip: best.remote_ip, - name: best.remote_name + mac: remote_identity.remote_mac, + ip: remote_identity.remote_ip, + name: remote_identity.remote_name }) if matched_id == device_id do @@ -553,9 +568,9 @@ defmodule Towerops.Topology do source_interface_id: best.source_interface_id, link_type: evidence_type_to_link_type(best.evidence_type), confidence: combine_evidence_confidence(evidence_list), - discovered_remote_mac: best.remote_mac, - discovered_remote_ip: best.remote_ip, - discovered_remote_name: best.remote_name, + discovered_remote_mac: remote_identity.remote_mac, + discovered_remote_ip: remote_identity.remote_ip, + discovered_remote_name: remote_identity.remote_name, metadata: %{}, last_confirmed_at: now, evidence: @@ -571,9 +586,7 @@ defmodule Towerops.Topology do end defp group_evidence_by_remote(evidence_list) do - Enum.group_by(evidence_list, fn ev -> - ev.remote_mac || ev.remote_ip || ev.remote_name || "unknown" - end) + Enum.group_by(evidence_list, &evidence_group_key/1) end defp combine_evidence_confidence(evidence_list) do @@ -659,6 +672,108 @@ defmodule Towerops.Topology do defp platform_match?(description, platforms), do: Enum.any?(platforms, &String.contains?(description, &1)) + defp device_interface_ips(%Device{snmp_device: nil}), do: [] + + defp device_interface_ips(%Device{snmp_device: %SnmpDevice{} = snmp_device}) do + Enum.flat_map(snmp_device.interfaces, fn interface -> + Enum.map(interface.ip_addresses, & &1.ip_address) + end) + end + + defp find_existing_link(link_attrs) do + source_device_id = link_attrs.source_device_id + source_interface_id = Map.get(link_attrs, :source_interface_id) + + base_query = + from(l in DeviceLink, + where: l.source_device_id == ^source_device_id and l.source_interface_id == ^source_interface_id + ) + + link_attrs + |> existing_link_query(base_query) + |> Repo.one() + end + + defp existing_link_query(%{discovered_remote_mac: remote_mac}, base_query) when is_binary(remote_mac) do + from(l in base_query, where: l.discovered_remote_mac == ^remote_mac) + end + + defp existing_link_query(%{discovered_remote_ip: remote_ip} = link_attrs, base_query) when is_binary(remote_ip) do + remote_name = Map.get(link_attrs, :discovered_remote_name) + + case remote_name do + name when is_binary(name) -> + from(l in base_query, + where: + is_nil(l.discovered_remote_mac) and l.discovered_remote_ip == ^remote_ip and + l.discovered_remote_name == ^name + ) + + _ -> + from(l in base_query, + where: is_nil(l.discovered_remote_mac) and l.discovered_remote_ip == ^remote_ip + ) + end + end + + defp existing_link_query(%{discovered_remote_name: remote_name}, base_query) when is_binary(remote_name) do + from(l in base_query, + where: + is_nil(l.discovered_remote_mac) and is_nil(l.discovered_remote_ip) and + l.discovered_remote_name == ^remote_name + ) + end + + defp existing_link_query(%{link_type: link_type}, base_query) do + from(l in base_query, + where: + is_nil(l.discovered_remote_mac) and is_nil(l.discovered_remote_ip) and + is_nil(l.discovered_remote_name) and l.link_type == ^link_type + ) + end + + defp evidence_group_key(ev) do + remote_mac = Identifier.normalize_mac(ev.remote_mac) + remote_ip = Identifier.normalize_ip(ev.remote_ip) + remote_name = Identifier.normalize_name(ev.remote_name) + + case remote_mac || remote_ip || remote_name do + nil -> + {"anonymous", ev.source_interface_id, ev.evidence_type, inspect(ev.evidence_data)} + + _ -> + {ev.source_interface_id, remote_mac, remote_ip, remote_name} + end + end + + defp summarize_remote_identity(evidence_list) do + %{ + remote_mac: select_remote_field(evidence_list, :remote_mac), + remote_ip: select_remote_field(evidence_list, :remote_ip), + remote_name: select_remote_field(evidence_list, :remote_name) + } + end + + defp select_remote_field(evidence_list, field) do + evidence_list + |> Enum.filter(&(is_binary(Map.get(&1, field)) and Map.get(&1, field) != "")) + |> Enum.max_by(& &1.confidence, fn -> nil end) + |> case do + nil -> nil + ev -> Map.get(ev, field) + end + end + + defp discovered_node_id(link) do + suffix = + link.discovered_remote_mac || + link.discovered_remote_ip || + link.discovered_remote_name || + "anonymous_#{link.id}" + + "discovered_#{suffix}" + end + # --- LLDP Neighbor Discovery (via SNMP) --- @doc """ diff --git a/lib/towerops/topology/identifier.ex b/lib/towerops/topology/identifier.ex new file mode 100644 index 00000000..e56fed7b --- /dev/null +++ b/lib/towerops/topology/identifier.ex @@ -0,0 +1,93 @@ +defmodule Towerops.Topology.Identifier do + @moduledoc """ + Identifier normalization helpers for topology discovery and matching. + """ + + @spec normalize_mac(term()) :: String.t() | nil + def normalize_mac(nil), do: nil + + def normalize_mac(<>) when is_integer(a) do + [a, b, c, d, e, f] + |> Enum.map_join(":", &(&1 |> Integer.to_string(16) |> String.pad_leading(2, "0"))) + |> String.downcase() + end + + def normalize_mac(mac) when is_binary(mac) do + mac + |> String.trim() + |> String.downcase() + |> String.replace(~r/[^0-9a-f]/, "") + |> case do + <> -> + "#{<>}:#{<>}:#{<>}:#{<>}:#{<>}:#{<>}" + + _ -> + nil + end + end + + def normalize_mac(_), do: nil + + @spec normalize_ip(term()) :: String.t() | nil + def normalize_ip(nil), do: nil + + def normalize_ip(ip) when is_binary(ip) do + ip + |> String.trim() + |> strip_ipv6_zone() + |> case do + "" -> + nil + + candidate -> + case :inet.parse_address(String.to_charlist(candidate)) do + {:ok, tuple} -> + tuple + |> :inet.ntoa() + |> to_string() + |> String.downcase() + + _ -> + String.downcase(candidate) + end + end + end + + def normalize_ip(_), do: nil + + @spec normalize_name(term()) :: String.t() | nil + def normalize_name(nil), do: nil + + def normalize_name(name) when is_binary(name) do + case name |> String.trim() |> String.trim_trailing(".") |> String.downcase() do + "" -> nil + normalized -> normalized + end + end + + def normalize_name(_), do: nil + + @spec candidate_names(term()) :: [String.t()] + def candidate_names(name) do + case normalize_name(name) do + nil -> + [] + + normalized -> + short_name = + normalized + |> String.split(".", parts: 2) + |> List.first() + + [normalized, short_name] + |> Enum.reject(&is_nil/1) + |> Enum.uniq() + end + end + + defp strip_ipv6_zone(ip) do + ip + |> String.split("%", parts: 2) + |> List.first() + end +end diff --git a/test/towerops/snmp/neighbor_discovery_test.exs b/test/towerops/snmp/neighbor_discovery_test.exs index 2bec964a..c25c5a64 100644 --- a/test/towerops/snmp/neighbor_discovery_test.exs +++ b/test/towerops/snmp/neighbor_discovery_test.exs @@ -97,6 +97,25 @@ defmodule Towerops.Snmp.NeighborDiscoveryTest do assert neighbor.remote_address == "192.168.1.100" end + test "normalizes subtype-prefixed LLDP MAC chassis IDs", %{interfaces: interfaces} do + expect(SnmpMock, :walk, fn _, _oid, _ -> + {:ok, + [ + %{oid: "1.0.8802.1.1.2.1.4.1.1.5.0.1.1", value: <<4, 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF>>}, + %{oid: "1.0.8802.1.1.2.1.4.1.1.9.0.1.1", value: "switch-a.example.com"} + ]} + end) + + expect(SnmpMock, :walk, fn _, _, _ -> {:ok, []} end) + expect(SnmpMock, :walk, fn _, _, _ -> {:ok, []} end) + + client_opts = [ip: "192.168.1.1", community: "public", version: "2c"] + {:ok, neighbors} = NeighborDiscovery.discover_neighbors(client_opts, interfaces) + + [neighbor] = neighbors + assert neighbor.remote_chassis_id == "aa:bb:cc:dd:ee:ff" + end + test "discovers LLDP neighbors without management addresses", %{interfaces: interfaces} do # First walk: LLDP remote table expect(SnmpMock, :walk, fn _, _oid, _ -> diff --git a/test/towerops/topology_test.exs b/test/towerops/topology_test.exs index 5692243c..7015331c 100644 --- a/test/towerops/topology_test.exs +++ b/test/towerops/topology_test.exs @@ -5,6 +5,7 @@ defmodule Towerops.TopologyTest do alias Towerops.Snmp.Device alias Towerops.Snmp.Interface + alias Towerops.Snmp.IpAddress alias Towerops.Topology alias Towerops.Topology.DeviceLink @@ -75,12 +76,39 @@ defmodule Towerops.TopologyTest do assert Topology.find_device_by_ip(lookup, "10.0.0.2") == switch.id end + test "matches device by discovered interface IP address", %{ + organization: org, + router_iface: router_iface, + router: router + } do + %IpAddress{} + |> IpAddress.changeset(%{ + snmp_interface_id: router_iface.id, + ip_address: "10.0.10.1", + ip_type: "ipv4", + last_checked_at: DateTime.utc_now() + }) + |> Repo.insert!() + + lookup = Topology.build_device_lookup(org.id) + assert Topology.find_device_by_ip(lookup, "10.0.10.1") == router.id + end + test "matches device by name (case-insensitive)", %{organization: org, router: router} do lookup = Topology.build_device_lookup(org.id) assert Topology.find_device_by_name(lookup, "core-router") == router.id assert Topology.find_device_by_name(lookup, "CORE-ROUTER") == router.id end + test "matches device by sysName short hostname", %{organization: org, router: router, router_snmp: router_snmp} do + router_snmp + |> Device.changeset(%{sys_name: "core-router.example.net"}) + |> Repo.update!() + + lookup = Topology.build_device_lookup(org.id) + assert Topology.find_device_by_name(lookup, "core-router") == router.id + end + test "matches device by MAC address", %{organization: org, router: router} do lookup = Topology.build_device_lookup(org.id) assert Topology.find_device_by_mac(lookup, "aa:bb:cc:00:11:22") == router.id @@ -508,6 +536,87 @@ defmodule Towerops.TopologyTest do assert {:ok, :unchanged} = Topology.process_device(router, org.id) assert Topology.list_device_links(router.id) == [] end + + test "matches ARP-discovered devices by interface IPs", %{ + organization: org, + router: router, + switch: switch, + router_iface: router_iface + } do + switch_snmp = + %Device{} + |> Device.changeset(%{device_id: switch.id, sys_name: "main-switch"}) + |> Repo.insert!() + + switch_iface = + %Interface{} + |> Interface.changeset(%{ + snmp_device_id: switch_snmp.id, + if_index: 10, + if_name: "vlan10", + if_phys_address: "bb:cc:dd:00:11:22" + }) + |> Repo.insert!() + + %IpAddress{} + |> IpAddress.changeset(%{ + snmp_interface_id: switch_iface.id, + ip_address: "10.0.10.2", + ip_type: "ipv4", + last_checked_at: DateTime.utc_now() + }) + |> Repo.insert!() + + {:ok, _} = + Towerops.Snmp.upsert_arp_entry(%{ + device_id: router.id, + interface_id: router_iface.id, + ip_address: "10.0.10.2", + mac_address: "ff:ff:ff:00:10:02", + last_seen_at: DateTime.utc_now() + }) + + assert {:ok, :changed} = Topology.process_device(router, org.id) + + [link] = Topology.list_device_links(router.id) + assert link.target_device_id == switch.id + assert link.discovered_remote_ip == "10.0.10.2" + end + + test "keeps anonymous neighbors separate per interface", %{ + router: router, + organization: org, + router_snmp: router_snmp + } do + iface_two = + %Interface{} + |> Interface.changeset(%{ + snmp_device_id: router_snmp.id, + if_index: 2, + if_name: "eth1", + if_phys_address: "aa:bb:cc:00:11:23" + }) + |> Repo.insert!() + + {:ok, _} = + Towerops.Snmp.upsert_neighbor(%{ + device_id: router.id, + interface_id: iface_two.id, + protocol: "lldp", + last_discovered_at: DateTime.utc_now() + }) + + {:ok, _} = + Towerops.Snmp.upsert_neighbor(%{ + device_id: router.id, + interface_id: iface_two.id, + protocol: "cdp", + last_discovered_at: DateTime.utc_now() + }) + + assert {:ok, :changed} = Topology.process_device(router, org.id) + assert length(Topology.list_device_links(router.id)) == 2 + end end describe "list_device_links/1" do