diff --git a/docs/SUMMARY.md b/docs/SUMMARY.md index 8f1a38b80e..f3cbe444f0 100644 --- a/docs/SUMMARY.md +++ b/docs/SUMMARY.md @@ -16,6 +16,7 @@ * [Background Jobs & Caching](librenms_import/background_jobs.md) * [Sync & Configuration](usage_tips/virtual_chassis.md) * [Virtual Chassis](usage_tips/virtual_chassis.md) + * [Out-of-Band Management](usage_tips/oob_management.md) * [Interface Mappings](usage_tips/interface_mappings.md) * [Module Sync](usage_tips/module_sync.md) * [Mapping Rules](usage_tips/mapping_rules.md) diff --git a/docs/feature_list.md b/docs/feature_list.md index c46a707020..eeb4ac5bbd 100644 --- a/docs/feature_list.md +++ b/docs/feature_list.md @@ -10,6 +10,15 @@ * Background job processing for large device sets * Duplicate detection to prevent re-importing existing devices +### [Out-of-Band (OOB) Management](usage_tips/oob_management.md) + +* Detects when a LibreNMS device (iDRAC/iLO/BMC/IPMI/CIMC) is the OOB controller of an existing NetBox device +* **Add as OOB** — link the controller to the host and set `oob_ip` on a chosen (or new) interface +* **Promote to host** — re-point a device currently linked to its OOB controller onto the incoming host device +* **Merge NetBox devices** — reconcile two devices (hostname-matched vs serial-matched) that represent one physical box +* Per-server linkage stored in the `librenms_id` custom field as `{"": {"id": N, "oob": {"id": M, "type": "drac"}}}` +* Post-merge **Move to winner** actions to migrate interfaces, IP addresses, and primary/OOB IPs at your own pace + ### [Module / Inventory Sync](usage_tips/module_sync.md) * Compare LibreNMS ENTITY-MIB inventory to NetBox module bays and installed modules diff --git a/docs/img/oob/oob-create-new-interface.png b/docs/img/oob/oob-create-new-interface.png new file mode 100644 index 0000000000..5e231bb2ba Binary files /dev/null and b/docs/img/oob/oob-create-new-interface.png differ diff --git a/docs/img/oob/oob-detected-validation.png b/docs/img/oob/oob-detected-validation.png new file mode 100644 index 0000000000..6a9e3d3511 Binary files /dev/null and b/docs/img/oob/oob-detected-validation.png differ diff --git a/docs/librenms_import/validation.md b/docs/librenms_import/validation.md index 4dd9158aad..a1d7aaddba 100644 --- a/docs/librenms_import/validation.md +++ b/docs/librenms_import/validation.md @@ -46,6 +46,11 @@ The plugin checks for existing devices using: If both a VM and Device with the same hostname exist, the plugin cannot determine which to match and allows import. Set the `librenms_id` custom field on the correct existing object to clarify the match. +## Out-of-Band (OOB) Detection + +When an incoming LibreNMS device looks like an out-of-band controller (iDRAC, iLO, BMC, …) and matches an existing NetBox device, the validation details show an **OOB Detected** panel instead of a plain import button. Rather than creating a duplicate device, the plugin offers the appropriate reconciliation action — **Add as OOB**, **Promote to host**, or **Merge NetBox devices**. See [Out-of-Band (OOB) Management](../usage_tips/oob_management.md) for the full flow. + ## Next Steps - [Import Settings](import_settings.md) - Configure device naming and import options +- [Out-of-Band Management](../usage_tips/oob_management.md) - Reconcile OOB controllers with their host devices diff --git a/docs/usage_tips/custom_field.md b/docs/usage_tips/custom_field.md index 0295a1e052..1b57575719 100644 --- a/docs/usage_tips/custom_field.md +++ b/docs/usage_tips/custom_field.md @@ -49,6 +49,13 @@ If the field was not created automatically (fallback): follow these steps to cre ```json {"production": 42, "staging": 17} ``` + - Out-of-band (OOB) form — when a device is linked to its OOB controller, the per-server value is an object holding the host id and the controller's id/type: + + ```json + {"production": {"id": 42, "oob": {"id": 99, "type": "drac"}}} + ``` + + This shape is written automatically by the OOB flows — see [Out-of-Band Management](oob_management.md). You don't normally edit it by hand. - Legacy single-server example (integer) — read-only/deprecated; do not use for new entries: ``` 42 diff --git a/docs/usage_tips/oob_management.md b/docs/usage_tips/oob_management.md new file mode 100644 index 0000000000..4f63f5b462 --- /dev/null +++ b/docs/usage_tips/oob_management.md @@ -0,0 +1,86 @@ +# Out-of-Band (OOB) Management + +Many servers expose a dedicated **out-of-band management controller** — iDRAC, iLO, BMC, IPMI, CIMC, and similar. LibreNMS usually polls that controller as its **own device**, separate from the host it lives in. NetBox models the same relationship differently: the controller is not a separate Device — its address is the host Device's **OOB IP** (`oob_ip`). + +This plugin bridges the two models. During import it detects when an incoming LibreNMS device is really the OOB side of a host you already have, and offers the right action to reconcile them instead of creating a duplicate device. + +## How the link is stored + +OOB linkage is recorded in the `librenms_id` [custom field](custom_field.md) alongside the host's own LibreNMS ID. The per-server value is promoted from a bare integer to a small object: + +```json +{ + "production": { + "id": 42, + "oob": { "id": 99, "type": "drac" } + } +} +``` + +- `id` — the LibreNMS device ID of the **host**. +- `oob.id` — the LibreNMS device ID of the **OOB controller**. +- `oob.type` — a short label for the controller (`idrac`, `drac`, `ilo`, `bmc`, `ipmi`, `cimc`, …), or the generic `oob` when the specific type can't be determined. + +Only these identity essentials are stored. The controller's IP and firmware version are intentionally **not** persisted here — the IP's source of truth is the host Device's interface-assigned `oob_ip`, and the version lives in LibreNMS and can be read back any time from `oob.id`. + +## OOB detection during import + +When a searched LibreNMS device looks like an OOB controller (by its OS/hardware strings, e.g. an iDRAC) and matches an existing NetBox device, the validation details show an **OOB Detected** panel instead of a plain import button. From there one of three resolution flows is offered, depending on what already exists. + +![OOB Detected validation panel, showing the OOB attach effect and the Add-as-OOB action](../img/oob/oob-detected-validation.png) + +!!! tip "Not seeing the panel?" + The panel only appears when **both** conditions hold: the incoming LibreNMS device's `os`/`hardware` (or hostname) matches an OOB pattern (`idrac`, `ilo`, `ipmi`, `bmc`, `drac`, `cimc`), **and** it matches an existing NetBox device by unique **serial** or by **management IP**. If the incoming hostname already matches a NetBox device name it takes the plain hostname-match path instead, and if the device is already linked to LibreNMS no OOB action is offered. (Device identifiers are blurred in these screenshots.) + +### Add as OOB + +Use when the existing NetBox device is the **host** and the incoming LibreNMS device is its OOB controller. + +The **Add as OOB to *device*** action links the controller's LibreNMS ID into the host's `oob.id` slot. NetBox requires `oob_ip` to be assigned to one of the device's interfaces, so the form includes an **OOB IP interface** picker: + +- A sensible interface is **pre-selected** (matched by name — `idrac`/`ilo`/`bmc`-style). Because the OOB IP is frequently *not* physically on that interface, the selection is **overridable**. +- Choose **+ Create new interface…** to create one (default name suggested) to hang the OOB IP on. + +![OOB IP interface picker with "+ Create new interface" selected and a suggested name](../img/oob/oob-create-new-interface.png) + +The OOB IP is then created (or re-homed) assigned to the chosen interface and set as the device's `oob_ip`. If you make no interface selection, the link is still recorded and the OOB IP is left for you to set later. + +!!! note "Permissions" + Setting the OOB IP can create an Interface, create an IPAddress, or re-home an existing one. The action requires the matching NetBox `add`/`change` permissions for those models; if you lack them the link is still recorded and the IP step is skipped with a warning. See [Permissions & Access](permissions.md). + +### Promote to host + +Use when the existing NetBox device is currently linked to the **OOB controller** (its `librenms_id` points at the controller) and the incoming LibreNMS device is the **host** side. + +**Promote to host of *device*** re-points the linkage: the incoming host's LibreNMS ID becomes the device's `id`, and the previously-linked controller ID is demoted into the `oob` slot. No new device is created. A pre-promote modal lets you optionally override the device's **name**, **device type**, and **platform** — all default to **Keep current**, so the original promote behaviour is unchanged unless you explicitly choose **Use new**. + +### Merge NetBox devices + +Use when **two different NetBox devices** turn out to represent one physical box — typically one created from the LibreNMS hostname and another from the chassis serial, where at least one already carries a LibreNMS link. + +The validation modal lists both candidates (hostname-matched and serial-matched) with their current linkage, and you pick which one to **keep** (the *winner*) and which to absorb (the *donor*). Merging consolidates the donor's LibreNMS link state under the active server key into the winner, clears the donor's active link, and writes a `_migrated_to` marker on the donor pointing at the winner. Interfaces, cables, and primary/OOB IPs are **not** moved automatically — you re-home those incrementally (see below). + +## Migrating a donor device after a merge + +A donor device (one with a `_migrated_to` marker) shows a banner on its LibreNMS sync page with **Move to winner** actions, so you can move resources over at your own pace: + +- **Move interface to winner** — reassigns an interface (and the cables, IPs, and MACs that hang off it) to the winner. Fails if the winner already has an interface with the same name — rename or remove that one first. +- **Move IP address to winner** — re-homes an interface-assigned IP to the winner's same-named interface (move the interface first if it doesn't exist on the winner yet). +- **Transfer primary IPv4 / IPv6 / OOB IP** — points the winner's `primary_ip4` / `primary_ip6` / `oob_ip` foreign key at the donor's value and clears it on the donor. Refuses to overwrite a value already set on the winner — clear it there first. + +Each action runs under a row lock and verifies the `_migrated_to` marker before touching anything. Once the donor has nothing left to migrate you can delete it. + +## Setting Primary and OOB IPs in general + +Outside the OOB import flows, both `primary_ip` and `oob_ip` are driven from interface-assigned addresses: + +- **Primary IP** is set on the device's **IP Addresses** sync tab: with **Set Primary IP** enabled, a synced IP that matches the LibreNMS management IP and is interface-assigned becomes the device's primary. +- **OOB IP** is set through the **Add as OOB** flow above. + +This keeps every IP relationship valid against NetBox's requirement that primary/OOB IPs be assigned to one of the device's own interfaces. + +## See also + +- [Custom Field Setup](custom_field.md) — the `librenms_id` field that stores the linkage. +- [Validation & Configuration](../librenms_import/validation.md) — where OOB is detected during import. +- [Permissions & Access](permissions.md) — permissions required for the OOB/IP actions. diff --git a/mkdocs.yml b/mkdocs.yml index e38dd729a7..efa426418b 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -20,6 +20,7 @@ nav: - Background Jobs & Caching: librenms_import/background_jobs.md - Sync & Configuration: - Virtual Chassis: usage_tips/virtual_chassis.md + - Out-of-Band Management: usage_tips/oob_management.md - Interface Mappings: usage_tips/interface_mappings.md - Module Sync: usage_tips/module_sync.md - Mapping Rules: usage_tips/mapping_rules.md diff --git a/netbox_librenms_plugin/constants.py b/netbox_librenms_plugin/constants.py index 4e542f9d15..b4c7685398 100644 --- a/netbox_librenms_plugin/constants.py +++ b/netbox_librenms_plugin/constants.py @@ -1,6 +1,53 @@ +import re + # Plugin permissions (from LibreNMSSettings model) PERM_VIEW_PLUGIN = "netbox_librenms_plugin.view_librenmssettings" PERM_CHANGE_PLUGIN = "netbox_librenms_plugin.change_librenmssettings" # LibreNMS VLAN state values LIBRENMS_VLAN_STATE_ACTIVE = 1 + +# OOB management controller detection +# Trailing \d*\b restricts matches to whole tokens (optionally with a numeric suffix like +# iDRAC9 / drac9) so a prefix collision inside an unrelated word — e.g. "dracut", "ipmitool" +# — can't misclassify a normal device as an OOB controller. +OOB_TYPE_PATTERN = re.compile(r"\b(idrac|ilo|ipmi|bmc|drac|cimc|oob)\d*\b", re.IGNORECASE) +OOB_TYPES = ("idrac", "ilo", "ipmi", "bmc", "drac", "cimc", "oob") + +# Shared "From OOB controller" badge markup (the bare ; callers add any leading space). +# Centralised so a restyle (color/title/text) happens in one place instead of drifting across the +# cable/module/interface tables and the cable-verify render that each hand-copied it. +OOB_BADGE_HTML = 'OOB' + + +def normalize_oob_type(os_str: str, hardware_str: str = "") -> str | None: + """ + Extract and normalize the OOB controller type from LibreNMS os/hardware strings. + + A vendor-specific match (idrac/ilo/ipmi/bmc/drac/cimc) always wins over the + generic ``oob`` token, even when ``oob`` appears earlier in the text, so e.g. + ``normalize_oob_type("oob", "iDRAC9")`` resolves to ``"idrac"`` rather than + being masked by the generic token. + + Args: + os_str (str): LibreNMS ``os`` field for the device. + hardware_str (str): LibreNMS ``hardware`` field for the device. + + Returns: + str | None: The canonical lowercase token (one of OOB_TYPES), or None if + no token matches. + + Examples: + normalize_oob_type("drac9", "iDRAC9") → "drac" + normalize_oob_type("oob", "iDRAC9") → "idrac" + normalize_oob_type("ilo", "") → "ilo" + normalize_oob_type("ubuntu", "") → None + """ + generic = None + for text in (os_str or "", hardware_str or ""): + for m in OOB_TYPE_PATTERN.finditer(text): + token = m.group(1).lower() + if token != "oob": + return token # vendor-specific match wins immediately + generic = generic or "oob" # remember the generic fallback, keep scanning + return generic diff --git a/netbox_librenms_plugin/import_utils/bulk_import.py b/netbox_librenms_plugin/import_utils/bulk_import.py index d4fde87631..628f32f164 100644 --- a/netbox_librenms_plugin/import_utils/bulk_import.py +++ b/netbox_librenms_plugin/import_utils/bulk_import.py @@ -8,9 +8,20 @@ from ..import_validation_helpers import apply_role_to_validation, recalculate_validation_status, remove_validation_issue from ..librenms_api import LibreNMSAPI -from ..utils import find_by_librenms_id, preload_normalization_rules +from ..utils import ( + AmbiguousLibreNMSIdError, + coerce_librenms_id, + find_by_librenms_id, + get_librenms_oob, + preload_normalization_rules, +) from .cache import get_cache_metadata_key, get_import_device_cache_key, get_validated_device_cache_key -from .device_operations import import_single_device, validate_device_for_import +from .device_operations import ( + _describe_existing_librenms_link, + import_single_device, + resolve_device_by_host_ip, + validate_device_for_import, +) from .filters import _safe_disabled, get_librenms_devices_for_import from .permissions import check_user_permissions, require_permissions from .virtual_chassis import ( @@ -21,6 +32,22 @@ logger = logging.getLogger(__name__) +# Stable fragment of the ambiguous-librenms_id blocker message. Shared by the writer +# (the AmbiguousLibreNMSIdError handler) and the cleaner (the pre-lookup reset in +# _refresh_existing_device) so a resolved duplicate's stale message is reliably removed +# regardless of which librenms_id value was interpolated into it. +_AMBIGUOUS_LIBRENMS_ID_MARKER = "matches more than one existing NetBox record" +# Substrings of the ambiguity blockers that carry the "ambiguous_hostname_or_serial" match type, +# used to strip a stale instance once the duplicate is resolved (mirrors the librenms_id marker +# above). The blocker can be appended by either the refresh serial/IP fallback below ("serial or +# management IP") or validate_device_for_import's duplicate name/serial guard ("hostname/serial"), +# so the cleanup must recognise either wording — otherwise a hostname/serial blocker survives the +# match_type reset and keeps the row blocked until cache expiry. +_AMBIGUOUS_SERIAL_IP_MARKERS = ( + "serial or management IP", + "hostname/serial", +) + def _is_job_cancelled(job) -> bool: """ @@ -348,6 +375,124 @@ def bulk_import_devices( ) +def _refresh_librenms_linkage(validation: dict, device, libre_device: dict, server_key: str) -> None: + """ + Re-derive the LibreNMS-id linkage fields for a refreshed device. + + Cheap and DB-only (reads the device's ``librenms_id`` custom field) — no + LibreNMS API call — so a cached import row picks up OOB-link / host-link + changes made in NetBox since the row was cached. Without this, the cache-hit + path keeps the stale ``existing_match_type``/badge (e.g. an OOB controller + linked after caching still rendered as a conflict until the cache expired). + + Mirrors ``validate_device_for_import``'s linkage logic: always refreshes + ``existing_librenms_link``, and when the device is matched to the scanned + LibreNMS id it classifies the match as ``librenms_oob`` (matched via the OOB + sub-key) or ``librenms_id`` (matched as the host). + + Args: + validation (dict): The import-row validation dict, mutated in place. + device: The refreshed NetBox device (or VM) to re-derive linkage from. + libre_device (dict): The scanned LibreNMS device record (may be empty). + server_key (str): The LibreNMS server key the row was scanned against. + + Returns: + None + """ + link = _describe_existing_librenms_link(device, server_key) + validation["existing_librenms_link"] = link + # Only re-classify librenms-id-based matches; leave serial/hostname/primary_ip + # match types untouched. + if validation.get("existing_match_type") in ("librenms_id", "librenms_oob"): + scanned_id = coerce_librenms_id((libre_device or {}).get("device_id")) + if scanned_id is None: + # The current scan didn't return a usable device_id (libre_device omitted or + # malformed). A missing scanned id is NOT proof the link disappeared — only drop + # the cached match when the DB linkage itself is gone; otherwise leave the prior + # match type until there's a real id to compare against. + if link["host_id"] is None and link["oob_id"] is None: + validation["existing_match_type"] = None + return + oob = get_librenms_oob(device, server_key=server_key) + oob_id = coerce_librenms_id(oob.get("id")) if oob else None + if scanned_id is not None and oob_id is not None and oob_id == scanned_id: + validation["existing_match_type"] = "librenms_oob" + elif scanned_id is not None and link["host_id"] is not None and link["host_id"] == scanned_id: + # Host id still matches the scanned device — a genuine host-side link. + validation["existing_match_type"] = "librenms_id" + else: + # Linkage changed since caching: neither the host id nor the OOB id matches + # the scanned device anymore, so don't keep a stale librenms_id badge. + validation["existing_match_type"] = None + + +def _clear_existing_match_derived_fields(validation: dict) -> None: + """ + Reset the fields produced from an existing match. + + Clears stale serial/OOB/merge/promote actions so they don't linger after that + match is dropped (device deleted, or librenms/OOB link removed since caching). + The subsequent fresh lookup re-populates them if it re-matches. + + Args: + validation (dict): The import-row validation dict, mutated in place. + + Returns: + None + """ + validation["serial_action"] = None + validation["oob_candidate"] = None + validation["serial_confirmed"] = False + validation["serial_duplicate"] = False + validation["serial_role_choice_available"] = False + # Name-sync / migration / device-type state is also derived from the (now dropped) match; + # leaving it set would render a migrate/name-sync action for the old object. The fresh + # lookup below re-derives these only on a re-match, so reset them here. + validation["librenms_id_needs_migration"] = False + validation["name_matches"] = False + validation["name_sync_available"] = False + validation["suggested_name"] = None + validation["device_type_mismatch"] = False + # promote_to_host follows the "absent otherwise" contract (see apply_oob_detection_result). + validation.pop("promote_to_host", None) + validation.pop("merge_candidates", None) + + +def _reassert_new_import_blockers(validation: dict) -> None: + """ + Re-add the create-time role/cluster blocker for unmatched rows. + + ``validate_device_for_import()`` attaches this blocker to unmatched rows. When a + refresh drops a cached match (or never had one) and the fresh lookup finds + nothing, the row is back in the "new import" path. + ``recalculate_validation_status()`` recomputes can_import purely from the issues + list, so without re-adding this blocker a row that still has no role/cluster + selected could flip back to importable and then fail at import time. + + Guarded by the selection state (found/role/cluster), so a row where the user + *has* picked a role/cluster — which sets found=True and removed the issue — is + left importable. + + Args: + validation (dict): The import-row validation dict, mutated in place. + + Returns: + None + """ + if validation.get("import_as_vm"): + cluster = validation.get("cluster") or {} + if not cluster.get("found") and not cluster.get("cluster"): + msg = "Cluster must be manually selected before importing as VM" + if msg not in validation.setdefault("issues", []): + validation["issues"].append(msg) + else: + role = validation.get("device_role") or {} + if not role.get("found") and not role.get("role"): + msg = "Device role must be manually selected before import" + if msg not in validation.setdefault("issues", []): + validation["issues"].append(msg) + + def _refresh_existing_device(validation: dict, libre_device: dict = None, server_key: str = "default") -> None: """ Refresh existing_device from DB to pick up changes made in NetBox since caching. @@ -368,26 +513,69 @@ def _refresh_existing_device(validation: dict, libre_device: dict = None, server if refreshed: validation["existing_device"] = refreshed - if hasattr(refreshed, "role") and refreshed.role: - apply_role_to_validation(validation, refreshed.role, is_vm=bool(validation.get("import_as_vm"))) - elif not validation.get("import_as_vm"): - validation["device_role"] = { - "found": False, - "role": None, - "available_roles": validation.get("device_role", {}).get("available_roles", []), - } - remove_validation_issue(validation, "role") - recalculate_validation_status(validation, is_vm=bool(validation.get("import_as_vm"))) - # Re-assert non-importable state: recalculate bases can_import on - # issues alone, but an existing matched device must never be import-ready. - validation["can_import"] = False - validation["is_ready"] = False - return + # Re-derive linkage so an OOB-link/host-link change since caching + # is reflected in the badge (DB-only; no LibreNMS API call). + prior_match = validation.get("existing_match_type") + _refresh_librenms_linkage(validation, refreshed, libre_device, server_key) + if prior_match in ("librenms_id", "librenms_oob") and validation.get("existing_match_type") is None: + # The librenms-id/OOB link that made this the cached match is gone + # (removed/repointed in NetBox since caching). Treat it like a vanished + # match — clear it and recompute readiness, then fall through to the fresh + # lookup below so the row is re-evaluated under current rules (it may now + # match by hostname/serial/IP, or become importable as new) instead of + # staying blocked until cache expiry. Mirrors the deleted-device branch. + validation["existing_device"] = None + validation["existing_librenms_link"] = None + _clear_existing_match_derived_fields(validation) + if not validation.get("import_as_vm"): + validation["device_role"] = { + "found": False, + "role": None, + "available_roles": validation.get("device_role", {}).get("available_roles", []), + } + else: + # VM rows are gated on cluster, not role: a dropped match must also clear + # the stale cluster selection (preserving available_clusters), or + # _reassert_new_import_blockers() sees found/cluster still set and lets + # the row re-enter the new-import path without a fresh cluster choice. + validation["cluster"] = { + "found": False, + "cluster": None, + "available_clusters": validation.get("cluster", {}).get("available_clusters", []), + } + # Fail-closed: this branch drops the vanished-link match and recomputes + # readiness, then falls through to the fresh lookup that would normally re-add + # the create-time role/cluster blocker. But the fresh lookup early-returns when + # libre_device is None (and its broad except can swallow), so re-assert here too + # — otherwise the row can stay importable with no role/cluster selected. + _reassert_new_import_blockers(validation) + recalculate_validation_status(validation, is_vm=bool(validation.get("import_as_vm"))) + else: + if hasattr(refreshed, "role") and refreshed.role: + apply_role_to_validation(validation, refreshed.role, is_vm=bool(validation.get("import_as_vm"))) + elif not validation.get("import_as_vm"): + validation["device_role"] = { + "found": False, + "role": None, + "available_roles": validation.get("device_role", {}).get("available_roles", []), + } + remove_validation_issue(validation, "role") + recalculate_validation_status(validation, is_vm=bool(validation.get("import_as_vm"))) + # Re-assert non-importable state: recalculate bases can_import on + # issues alone, but an existing matched device must never be import-ready. + validation["can_import"] = False + validation["is_ready"] = False + return else: # Device was deleted since caching — recompute readiness to match # validate_device_for_import logic. validation["existing_device"] = None validation["existing_match_type"] = None + # Nothing is linked anymore — clear the linkage so the row can't + # keep rendering a stale host/OOB badge. + validation["existing_librenms_link"] = None + # Drop serial/OOB/merge/promote actions that pointed at the deleted device. + _clear_existing_match_derived_fields(validation) # Clear stale device_role so is_ready is computed from scratch. # Guard: VMs don't use device_role for readiness, so preserve any # user-selected role rather than silently dropping it. @@ -397,13 +585,30 @@ def _refresh_existing_device(validation: dict, libre_device: dict = None, server "role": None, "available_roles": validation.get("device_role", {}).get("available_roles", []), } + else: + # Mirror the stale-match branch: a deleted cached VM match must drop the + # stale cluster selection (keeping available_clusters) so the row returns to + # the same create-time state as a brand-new VM import row. + validation["cluster"] = { + "found": False, + "cluster": None, + "available_clusters": validation.get("cluster", {}).get("available_clusters", []), + } + # Same fail-closed reasoning as the vanished-link branch above: re-assert the + # create-time blocker before recompute so a deleted-match row can't stay importable + # if the fresh lookup early-returns (libre_device None) or its except swallows. + _reassert_new_import_blockers(validation) recalculate_validation_status(validation, is_vm=bool(validation.get("import_as_vm"))) except Exception as e: existing_id = getattr(existing, "pk", "unknown") if existing else "none" logger.error(f"Failed to refresh existing device (pk={existing_id}): {e}") return - # existing_device was None at cache time — check if device was imported since + # Re-evaluate the match under current DB state. Reached when existing_device was None at + # cache time, or when a cached librenms_id/OOB link disappeared (cleared above) or its + # device was deleted — in every case re-check whether a matching NetBox object exists now, + # using the full id/name/serial/IP breadth so the row can't flip to importable and create + # a duplicate of a device that still exists under a different identity. if not libre_device: return try: @@ -416,71 +621,230 @@ def _refresh_existing_device(validation: dict, libre_device: dict = None, server # imported as a VM even though import_as_vm=False (or vice versa). CrossModel = Device if import_as_vm else VirtualMachine - librenms_id = libre_device.get("device_id") + # Coerce up front so malformed values (e.g. "42.0", floats, booleans) are rejected + # rather than truncated by int() and matched to the wrong record. + librenms_id = coerce_librenms_id(libre_device.get("device_id")) hostname = libre_device.get("hostname", "") sys_name = libre_device.get("sysName", "") + # Clear any stale ambiguous-librenms_id blocker set by a prior refresh before + # re-running the lookup. If the duplicate still exists, _lookup_in_model() below + # re-raises AmbiguousLibreNMSIdError and the except handler re-adds the blocker; + # if it was resolved since, the row must not stay blocked until cache expiry. + if validation.get("ambiguous_librenms_id"): + validation["ambiguous_librenms_id"] = False + if validation.get("existing_match_type") == "ambiguous_librenms_id": + validation["existing_match_type"] = None + for _key in ("issues", "warnings"): + msgs = validation.get(_key) + if isinstance(msgs, list): + validation[_key] = [ + m for m in msgs if not (isinstance(m, str) and _AMBIGUOUS_LIBRENMS_ID_MARKER in m) + ] + + # Same for a stale serial/IP ambiguity blocker: if the duplicate was resolved since caching, + # the fresh fallback below would not re-flag it, but the cached issue/match_type would keep + # the row blocked until cache expiry. Clear it so the row is re-evaluated under current rules. + if validation.get("existing_match_type") == "ambiguous_hostname_or_serial": + validation["existing_match_type"] = None + for _key in ("issues", "warnings"): + msgs = validation.get(_key) + if isinstance(msgs, list): + validation[_key] = [ + m + for m in msgs + if not (isinstance(m, str) and any(marker in m for marker in _AMBIGUOUS_SERIAL_IP_MARKERS)) + ] + new_device = None match_type = None found_as_cross_model = False - def _id_lookup(m): - """librenms_id match in model m, or None.""" - if librenms_id is not None and not isinstance(librenms_id, bool): - # int/str only (find_by_librenms_id's contract). Don't int()-coerce: - # int(42.9) -> 42 would bind an unrelated object by a truncated id. A float - # (or any other type) carries no exact id, so fail closed; str-digit ids are - # canonicalized by find_by_librenms_id itself (mirrors _librenms_id_q's - # lossy-coercion guard, #103). - if not isinstance(librenms_id, (int, str)): - return None - return find_by_librenms_id(m, librenms_id, server_key) - return None - - def _name_lookup(m): - """name-based match in model m → (device, match_type) or (None, None).""" - resolved_name = validation.get("resolved_name") - if resolved_name: - dev = m.objects.filter(name__iexact=resolved_name).first() - if dev: - return dev, "resolved_name" - if hostname: - dev = m.objects.filter(name__iexact=hostname).first() - if dev: - return dev, "hostname" - if sys_name: - dev = m.objects.filter(name__iexact=sys_name).first() - if dev: - return dev, "sysname" - return None, None - - # An exact librenms_id match wins across BOTH models before any name fallback. Otherwise a - # refresh could bind a name-colliding object in the preferred model even when the scanned - # LibreNMS id is already linked to the *other* model (e.g. a VM), disagreeing with - # validation and rendering actions for the wrong object. - dev = _id_lookup(Model) - if dev: - new_device, match_type = dev, "librenms_id" - else: - dev = _id_lookup(CrossModel) - if dev: - new_device, match_type, found_as_cross_model = dev, "librenms_id", True - else: - new_device, match_type = _name_lookup(Model) - if not new_device: - # Catches cross-model imports made after the cache was built. - new_device, match_type = _name_lookup(CrossModel) - if new_device: - found_as_cross_model = True + def _lookup_in_model(m): + """ + Return (device, match_type, ambiguous) by NAME for model m. + + The librenms_id match is resolved up-front by the cross-model collision check below + and short-circuits (sets ``new_device``) before this is ever reached, so re-running + ``find_by_librenms_id(m, ...)`` here would just repeat that query for no result. This + does only the name/hostname/sysName fallbacks. + + NetBox device names are unique only per-site, so a name can resolve to MORE THAN ONE + device. Fail closed exactly like the serial/IP fallback below (and the full + validate_device_for_import() path): when a name matches >1 device, return + ``(None, None, True)`` so the caller blocks the row instead of binding ``.first()`` + to an arbitrary one. + """ + for value, mt in ( + (validation.get("resolved_name"), "resolved_name"), + (hostname, "hostname"), + (sys_name, "sysname"), + ): + if not value: + continue + matches = list(m.objects.filter(name__iexact=value)[:2]) + if len(matches) > 1: + return None, None, True + if matches: + return matches[0], mt, False + return None, None, False + + # Fail closed on a CROSS-MODEL librenms_id collision before selecting a match — i.e. + # the same (server_key, librenms_id) bound to BOTH a Device and a VirtualMachine. A + # LibreNMS device_id is unique within a server, so this never happens in a clean state; + # it's a NetBox-side data-integrity hazard (custom fields have no cross-model uniqueness) + # from a stale/duplicate binding — e.g. a thing imported as a VM then re-imported as a + # Device without clearing the old VM link, or a manual CF edit. validate_device_for_import() + # already detects and blocks exactly this (see device_operations.py "Cross-model collision"), + # but _lookup_in_model(Model) here returns on the first preferred-model id hit and never + # consults CrossModel — so without this guard the refresh re-check would silently bind to + # one model and disagree with the validation path that originally blocked the row. Check + # both models and raise the existing ambiguous-id blocker when both resolve (single-model + # duplicates are already raised inside find_by_librenms_id). + if librenms_id is not None: + model_id_match = find_by_librenms_id(Model, librenms_id, server_key) + cross_id_match = find_by_librenms_id(CrossModel, librenms_id, server_key) + if model_id_match and cross_id_match: + raise AmbiguousLibreNMSIdError( + f"LibreNMS ID {librenms_id} matches both {Model.__name__} and {CrossModel.__name__}" + ) + # An exact librenms_id owner must win over any name/hostname fallback: if the id now + # belongs to the opposite model only, binding by name to a same-named preferred-model + # object would silently re-home the row to the wrong device. Prefer the id match here, + # before _lookup_in_model(Model) can return a name hit. + if model_id_match: + new_device, match_type = model_id_match, "librenms_id" + elif cross_id_match: + new_device, match_type = cross_id_match, "librenms_id" + found_as_cross_model = True + + name_ambiguous = False + if not new_device: + # Look up BOTH models by name, not preferred-first. The old code returned on the + # first Model name hit and never consulted CrossModel, so a cached row whose + # resolved name/hostname/sysName exists as BOTH a Device and a VirtualMachine was + # pinned to the preferred model. validate_device_for_import()'s hostname path treats + # that cross-model case as ambiguous and binds NEITHER (it warns and lets the user + # import as new, then set librenms_id on the correct object), so the refresh re-check + # must do the same or it drifts and renders/links the wrong target. + model_match, model_mt, model_amb = _lookup_in_model(Model) + cross_match, cross_mt, cross_amb = _lookup_in_model(CrossModel) + if model_amb or cross_amb: + # >1 match within a single model — terminal ambiguity, fail closed below. + name_ambiguous = True + elif model_match and cross_match: + # Same name resolves in BOTH models: warn and leave unmatched (do NOT block), + # exactly like the validator's cross-model hostname branch. A serial/IP match can + # still bind below (a stronger identity), mirroring the validator's fall-through. + # setdefault (not a plain get + isinstance guard) so the warning is surfaced even + # when the caller built a minimal validation dict without "warnings", matching the + # AmbiguousLibreNMSIdError handler below. + validation.setdefault("warnings", []).append( + f"Both a VM and Device exist with hostname '{hostname}' in NetBox. Cannot " + "determine which to match. Please set the librenms_id custom field on the " + "correct object." + ) + elif model_match: + new_device, match_type = model_match, model_mt + elif cross_match: + # Cross-model import that happened after the cache was built (e.g. a LibreNMS + # device imported as a VM): the preferred model has no name match, the opposite + # one does. + new_device, match_type = cross_match, cross_mt + found_as_cross_model = True + + if not new_device and name_ambiguous: + # A hostname/sysName resolved to MORE THAN ONE NetBox device (names are unique only + # per-site). Binding to whichever sorts first would render the wrong device as the + # existing match, so fail closed exactly like the serial/IP fallback below and the + # full validate_device_for_import() path — block instead of picking arbitrarily. The + # "hostname/serial" marker keeps this in lock-step with the stale-blocker cleanup above. + msgs = validation.get("issues") + if isinstance(msgs, list): + msgs.append( + "Multiple NetBox devices share this device's hostname/serial; resolve the " + "duplicate before importing." + ) + validation["existing_match_type"] = "ambiguous_hostname_or_serial" + validation["can_import"] = False + validation["is_ready"] = False + + if not new_device and not name_ambiguous and not import_as_vm: + # Serial- and IP-based matches: validate_device_for_import() catches these, so the + # refresh re-check must have the same breadth. Without them a row whose + # librenms_id/name link disappeared (or that never matched) can flip to importable + # and re-import a device that already exists in NetBox under a different name — + # matched only by hardware serial or management IP. Device-only (VMs have no serial + # or primary-IP identity here). The richer serial_action/OOB-candidate heuristics + # stay in the full validation path; here the contract is simply: block the import. + from dcim.models import Device as _Device + + # This fallback fails closed on ambiguity exactly like validate_device_for_import(): + # if the serial OR the management IP resolves to more than one distinct NetBox device, + # binding to whichever row sorts first would render the wrong device as the existing + # match, so flag the row ambiguous and block instead of picking arbitrarily. + ambiguous_fallback = False + serial = (libre_device.get("serial") or "").strip() + if serial and serial != "-": + serial_matches = list(_Device.objects.filter(serial=serial)[:2]) + if len(serial_matches) > 1: + ambiguous_fallback = True + elif serial_matches: + new_device, match_type = serial_matches[0], "serial" + + if not new_device and not ambiguous_fallback: + primary_ip = libre_device.get("ip") + if primary_ip: + # Shared resolver (scans interface-assignment + oob_ip-FK across all duplicate + # net_host rows, fails closed on >1 distinct device) — same helper + # validate_device_for_import() uses, so the two paths can't drift. + device, ip_ambiguous, _matching_ips = resolve_device_by_host_ip(primary_ip) + if ip_ambiguous: + ambiguous_fallback = True + elif device: + new_device, match_type = device, "primary_ip" + + if ambiguous_fallback: + # Block without binding to an arbitrary device: append a blocking issue (the + # new_device=None `else` branch below recomputes can_import from the issues list) + # and mark the row ambiguous so the UI doesn't render a wrong existing match. + msgs = validation.get("issues") + if isinstance(msgs, list): + msgs.append( + "Multiple NetBox devices match this device's serial or management IP; " + "resolve the duplicate before importing." + ) + validation["existing_match_type"] = "ambiguous_hostname_or_serial" + validation["can_import"] = False + validation["is_ready"] = False if new_device: validation["existing_device"] = new_device validation["existing_match_type"] = match_type + # Re-derive linkage so a librenms_id match is correctly shown as the + # host vs. OOB half, and existing_librenms_link is populated for the + # paired badge (DB-only; no LibreNMS API call). + _refresh_librenms_linkage(validation, new_device, libre_device, server_key) validation["can_import"] = False validation["is_ready"] = False # Determine actual model from the found object, not from import_as_vm flag actual_is_vm = found_as_cross_model != import_as_vm # XOR: cross flips the flag validation["import_as_vm"] = actual_is_vm # Update so future refreshes query correct model + # A row that was previously unmatched can carry create-time blockers — "Device role + # must be manually selected" and/or "Cluster must be manually selected" — that + # validate_device_for_import() only adds when there's no existing_device. Now that + # the row resolves to an existing object, none of those apply (and a cross-model + # match can carry the *other* model's blocker). Drop both before recalculating so a + # stale message doesn't linger in the UI; the row stays force-blocked as an existing + # match regardless. The VM path previously cleared neither. + remove_validation_issue(validation, "role") + remove_validation_issue(validation, "cluster") + # A cached new-import row can also carry "No matching site found…" / "No matching + # device type found…" create-time blockers (device_operations.py). They don't apply + # to a now-resolved existing match either, so clear them too or the validation detail + # stays inconsistent with the resolved match. + remove_validation_issue(validation, "site") + remove_validation_issue(validation, "device type") if not actual_is_vm and hasattr(new_device, "role") and new_device.role: apply_role_to_validation(validation, new_device.role, is_vm=False) elif not actual_is_vm: @@ -494,6 +858,32 @@ def _name_lookup(m): # but a late-found existing match must never be import-ready. validation["can_import"] = False validation["is_ready"] = False + else: + # No existing match at all — the row is a genuine new import. If a cached match was + # just cleared above, its create-time role/cluster blocker was lost; re-add it so the + # row can't flip to importable while still missing a required selection. + _reassert_new_import_blockers(validation) + recalculate_validation_status(validation, is_vm=import_as_vm) + except AmbiguousLibreNMSIdError as exc: + # An ambiguous librenms_id (matching multiple records) must block import rather + # than fall through as "not found" and stay importable. + logger.warning("Bulk re-check blocked — ambiguous librenms_id %r: %s", librenms_id, exc) + validation["can_import"] = False + validation["is_ready"] = False + validation["ambiguous_librenms_id"] = True + validation["existing_match_type"] = "ambiguous_librenms_id" + message = ( + f"LibreNMS ID {librenms_id} {_AMBIGUOUS_LIBRENMS_ID_MARKER}; import " + "blocked to avoid binding to the wrong object. Resolve the duplicate librenms_id " + "assignment, then retry." + ) + # Append to issues (not just warnings) — a later recalculate_validation_status() + # recomputes can_import from issues, so a warning alone would be silently re-enabled. + # Dedup so repeated refreshes don't stack the same message. + if message not in validation.setdefault("warnings", []): + validation["warnings"].append(message) + if message not in validation.setdefault("issues", []): + validation["issues"].append(message) except Exception as e: logger.error(f"Failed to check for newly imported device: {e}") diff --git a/netbox_librenms_plugin/import_utils/device_operations.py b/netbox_librenms_plugin/import_utils/device_operations.py index 948f9a9e6f..87059589aa 100644 --- a/netbox_librenms_plugin/import_utils/device_operations.py +++ b/netbox_librenms_plugin/import_utils/device_operations.py @@ -12,13 +12,19 @@ from ..librenms_api import LibreNMSAPI from ..utils import ( + AmbiguousLibreNMSIdError, + coerce_librenms_id, find_by_librenms_id, find_matching_platform, find_matching_site, + get_librenms_device_id, + get_librenms_oob, is_legacy_librenms_id, match_librenms_hardware_to_device_type, set_librenms_device_id, ) +from ..constants import normalize_oob_type +from ..import_validation_helpers import apply_merge_candidates, apply_oob_detection_result from .cache import get_import_device_cache_key from .virtual_chassis import ( _generate_vc_member_name, @@ -29,6 +35,143 @@ logger = logging.getLogger(__name__) +# Prefix on the issue appended by validate_device_for_import()'s catch-all except branch when +# validation aborts on an exception and returns only a PARTIAL result. Downstream fail-closed +# guards (e.g. bulk_import.detect_collisions_for_device_ids) key on this to treat the row as +# not-reliably-checked. Keep it a shared constant so the producer and every consumer can't drift +# apart — a silent text change would otherwise defeat the fail-closed guarantee. +VALIDATION_ERROR_ISSUE_PREFIX = "Validation error:" + + +def _detect_oob_type_from_name(name): + """ + Return the canonical OOB type token found in a device name. + + Routes through normalize_oob_type() so a vendor-specific token wins over the + generic "oob" even when "oob" appears earlier in the name (e.g. + "leaf01-oob-idrac9" -> "idrac", not "oob"). A bare re.search() returns the first + token and would downgrade the hint. + + Args: + name (str): The device name to inspect. + + Returns: + str | None: The canonical OOB type token (idrac/ilo/ipmi/bmc/drac), or None + if no token matches. + """ + if not name: + return None + return normalize_oob_type(name, "") + + +def _describe_existing_librenms_link(obj, server_key): + """ + Describe the current LibreNMS linkage on a NetBox object. + + Always returns a dict (with all-None values if nothing is linked) so callers can + treat it as a plain status object. Tolerates legacy bare-int and dict-form custom + field values. + + Args: + obj: The NetBox object whose ``librenms_id`` custom field is inspected. + server_key (str): The LibreNMS server key to read linkage for. + + Returns: + dict: ``{"host_id": int|None, "oob_id": int|None, "oob_type": str|None}`` + summarising the ``librenms_id`` custom field for *server_key*. + """ + info = {"host_id": None, "oob_id": None, "oob_type": None} + # Host ID via the single canonical accessor (per coding guidelines) rather than touching the + # custom field directly. auto_save=False: this is a read-only describe/badge path and must not + # mutate custom_field_data. get_librenms_device_id handles legacy bare-int / string-digit and + # the per-server dict's "id" key, mirroring find_by_librenms_id's coercion. + host_id = get_librenms_device_id(obj, server_key, auto_save=False) + if host_id is not None and host_id > 0: + info["host_id"] = host_id + # The OOB sub-object via the canonical accessor (mirrors the host-id read above): it returns + # the raw oob dict ({"id": , "type": , ...}) or None, encapsulating the + # dict-form navigation {"": {"id": ..., "oob": {...}}}. + oob = get_librenms_oob(obj, server_key) + if oob is not None: + oob_id = coerce_librenms_id(oob.get("id")) + if oob_id is not None and oob_id > 0: + info["oob_id"] = oob_id + oob_type = oob.get("type") + if isinstance(oob_type, str) and oob_type: + info["oob_type"] = oob_type + return info + + +def _describe_link_note(existing_link): + """ + Return a human-readable phrase describing an existing LibreNMS link. + + Centralizes the host-id / OOB / unlinked wording that was copy-pasted — and had + drifted ("already linked" vs "currently linked", "OOB already linked" vs "as an OOB + controller") — across the VM-hostname, device-hostname, primary-IP and serial-match + import branches, so the phrasing stays consistent and a future change lands in one place. + + Args: + existing_link: A :func:`_describe_existing_librenms_link` dict, or None. + + Returns: + str: One of "currently linked to LibreNMS device #N", "currently linked to LibreNMS + as an OOB controller", or "not linked to LibreNMS". + """ + link = existing_link or {} + if link.get("host_id"): + return f"currently linked to LibreNMS device #{link['host_id']}" + if link.get("oob_id"): + return "currently linked to LibreNMS as an OOB controller" + return "not linked to LibreNMS" + + +def resolve_device_by_host_ip(primary_ip): + """ + Resolve the unique NetBox device whose interface or oob_ip carries a host address. + + Scans EVERY ``IPAddress`` row sharing ``primary_ip`` as its host address (duplicate + net_host rows are possible) across both the interface-assignment path and the ``oob_ip`` + direct-FK path, so a genuine collision fails closed instead of binding to whichever + duplicate row sorts first. An IP can be a device's ``oob_ip`` while assigned to no + interface, so the assigned_object scan alone would miss an OOB-only link. + + Shared by :func:`validate_device_for_import` and ``bulk_import._refresh_existing_device`` + so the two paths can't drift on which device a management IP resolves to. + + Args: + primary_ip: The management IP (host form) to resolve. + + Returns: + tuple: ``(device | None, ambiguous: bool, matching_ips: QuerySet)``. + ``device`` is the single matching device, or ``None`` when none or more than one + match; ``ambiguous`` is ``True`` only when >1 distinct device shares the address + (the caller must block the import); ``matching_ips`` is the net_host queryset so + callers can reuse it (e.g. for the ``oob_ip`` membership check). + """ + from dcim.models import Device + from ipam.models import IPAddress + + # prefetch_related on the assigned_object GenericForeignKey resolves every duplicate net_host + # row's interface in one bulk pass instead of a per-row content-type lookup (small N, but free). + matching_ips = IPAddress.objects.filter(address__net_host=primary_ip).prefetch_related("assigned_object") + candidate_devices = {} + matching_exists = False + for existing_ip in matching_ips: + matching_exists = True + assigned = getattr(existing_ip, "assigned_object", None) + dev = getattr(assigned, "device", None) if assigned else None + if dev: + candidate_devices[dev.pk] = dev + if matching_exists: + for oob_device in Device.objects.filter(oob_ip__in=matching_ips): + candidate_devices[oob_device.pk] = oob_device + if len(candidate_devices) > 1: + return None, True, matching_ips + if candidate_devices: + return next(iter(candidate_devices.values())), False, matching_ips + return None, False, matching_ips + def _try_chassis_device_type_match(api, device_id, preloaded_device_type_rules: dict | None = None): """ @@ -135,6 +278,197 @@ def _determine_device_name( return name +def _flag_ambiguous_librenms_id(result, librenms_id, exc): + """ + Block import when a librenms_id resolves to more than one NetBox object. + + An ambiguous id is a data-integrity violation; treating it as "not found" would + let the device import as new (or bind to an arbitrary row), so fail closed + instead. + + The message is appended to ``issues`` (not just ``warnings``) because the + readiness step recomputes ``can_import`` from ``issues`` — a warning alone would + be silently overridden back to importable when no other issue is present. + + Args: + result (dict): The validation result dict, mutated in place. + librenms_id: The ambiguous LibreNMS id (for the message text). + exc: The :class:`AmbiguousLibreNMSIdError` raised during resolution. + + Returns: + None + """ + logger.warning("Import validation blocked — ambiguous librenms_id %r: %s", librenms_id, exc) + result["ambiguous_librenms_id"] = True + result["can_import"] = False + if result.get("existing_match_type") != "ambiguous_librenms_id": + result["existing_match_type"] = "ambiguous_librenms_id" + message = ( + f"LibreNMS ID {librenms_id} matches more than one existing NetBox record; import " + "blocked to avoid binding to the wrong object. Resolve the duplicate librenms_id " + "assignment, then retry." + ) + result["warnings"].append(message) + result["issues"].append(message) + + +def _detect_serial_match_role(existing_by_serial, existing_link, hostname, serial, libre_device, server_key): + """Decide the role an incoming LibreNMS device plays against a NetBox device matched by serial. + + Pure decision step for the serial-match branch of :func:`validate_device_for_import`: + reads NetBox/LibreNMS state but does **not** mutate ``result``. Computes whether the + incoming device is an OOB-controller candidate, a host-promotion, a plain link, or a + hostname-differs case, and returns the keyword arguments for + :func:`apply_oob_detection_result` (``serial_action``, ``oob_candidate``, + ``promote_to_host``, ``serial_role_choice_available``, ``warnings``). + + Args: + existing_by_serial: the NetBox Device matched by serial. + existing_link: ``_describe_existing_librenms_link`` dict for *existing_by_serial*. + hostname: the incoming LibreNMS hostname (already resolved). + serial: the incoming serial (for warning text only). + libre_device: the raw LibreNMS device payload. + server_key: active LibreNMS server key. + """ + # Compute both possible roles for the incoming LibreNMS device against + # the existing NetBox device, then pick a heuristic default. The UI + # offers a manual toggle whenever both roles are feasible so the user + # can override the heuristic (e.g. mark a "linux"-OS device as OOB or + # demote an apparent host into the OOB slot). + oob_type_from_libre = normalize_oob_type( + libre_device.get("os", ""), + libre_device.get("hardware", ""), + ) + existing_oob = get_librenms_oob(existing_by_serial, server_key=server_key) + + # Only treat this as a possible host/OOB chassis-pair situation when + # there is a real ambiguity: either the existing NetBox device's name + # differs from the incoming LibreNMS hostname (so they likely represent + # two sides of one physical box), or the existing device is already + # linked to a different LibreNMS id. When names match exactly and the + # existing has no link, the user almost certainly just wants to link. + names_match = bool(existing_by_serial.name and existing_by_serial.name.lower() == hostname.lower()) + # Normalize to int so that a string device_id from the API + # (e.g. "17") doesn't cause a false "linked elsewhere" result + # when compared to the int host_id from coerce_librenms_id. + normalized_device_id = coerce_librenms_id(libre_device.get("device_id")) + # Only a real (non-None) incoming id can establish "linked to a DIFFERENT id". A missing or + # zero device_id normalizes to None, which is unknown — not a mismatch — so it must NOT trip + # the chassis-pair / host-promotion heuristic (host_id != None is always True and would offer + # a spurious OOB/Host toggle). + linked_to_other_id = bool( + existing_link + and existing_link["host_id"] + and normalized_device_id is not None + and existing_link["host_id"] != normalized_device_id + ) + already_linked_elsewhere = linked_to_other_id + # A bare hostname mismatch is NOT enough to treat this as a host/OOB chassis pair: a device + # reinstalled with a new hostname keeps its chassis serial, so "same serial, new hostname, no + # link, neither side OOB-flavoured" is a REINSTALL, not a pair — offering "Add as OOB + # controller" there mis-pairs a reinstalled host with its own stale record. Require a real OOB + # signal (incoming os/hardware or hostname looks OOB, or the existing device's name looks OOB) + # or an existing link to a DIFFERENT id before treating a name mismatch as a chassis pair. + existing_oob_from_name = _detect_oob_type_from_name(existing_by_serial.name) + incoming_oob_signal = bool( + oob_type_from_libre + or _detect_oob_type_from_name(libre_device.get("hostname") or libre_device.get("sysName") or "") + ) + has_oob_signal = incoming_oob_signal or bool(existing_oob_from_name) + # A name match normally means "just link" — but not when LibreNMS itself reports the incoming + # device as an OOB controller (os/hardware → oob_type_from_libre). An iDRAC/iLO/IPMI sharing the + # host's chassis serial often also shares (or mirrors) its hostname, so gating purely on + # ``not names_match`` would drop a same-name OOB row into the legacy link path and attach the + # controller's LibreNMS id as the HOST id. A definitive incoming OOB type is enough on its own to + # treat this as a chassis pair, regardless of name; the reinstall guard above stays intact + # because a reinstalled host has no incoming OOB type. + chassis_pair_likely = ( + already_linked_elsewhere or bool(oob_type_from_libre) or ((not names_match) and has_oob_signal) + ) + + oob_possible = chassis_pair_likely and existing_oob is None + host_possible = chassis_pair_likely and bool(linked_to_other_id and not existing_link.get("oob_id")) + + # --- Compute all values before mutating result --- + oob_candidate_data = None + if oob_possible: + inferred_oob_type = ( + oob_type_from_libre + or _detect_oob_type_from_name(libre_device.get("hostname") or libre_device.get("sysName") or "") + or "oob" + ) + oob_candidate_data = { + "device": existing_by_serial, + "type": inferred_oob_type, + "version": libre_device.get("version") or None, + "ip": libre_device.get("ip") or None, + } + + promote_to_host_data = None + if host_possible: + promote_to_host_data = { + "existing_libre_id": existing_link["host_id"], + "existing_oob_type": existing_oob_from_name or "oob", + # Included for bulk-collision detection: lets + # detect_bulk_collisions identify which NetBox device + # would be modified without an extra DB round-trip. + "existing_device": existing_by_serial, + } + + # Heuristic default: incoming-OS clearly OOB -> oob; otherwise if the + # existing device's NAME suggests it is the OOB and a host link can be + # demoted, offer promote; otherwise fall back to whichever is feasible. + if oob_type_from_libre and oob_possible: + serial_action_value = "oob_candidate" + elif host_possible and existing_oob_from_name: + serial_action_value = "promote_to_host" + elif oob_possible and host_possible: + # Both feasible but neither heuristic matches strongly -- + # default to oob_candidate (least-destructive), let the user flip. + serial_action_value = "oob_candidate" + elif oob_possible: + serial_action_value = "oob_candidate" + elif host_possible: + serial_action_value = "promote_to_host" + else: + serial_action_value = None + + block_warnings: list = [] + if oob_type_from_libre and existing_oob is not None: + # OOB-typed incoming but existing already has an OOB linked -- inform without blocking. + # Use a dedicated non-actionable value: "link" would render the generic host-link form + # ("Link to LibreNMS" button) in device_validation_details.html, posting an + # indistinguishable host-link request instead of leaving this branch informational. + serial_action_value = "oob_already_linked" + block_warnings.append( + f"Device '{existing_by_serial.name}' already has an OOB controller linked. " + f"Re-import will update the existing OOB entry." + ) + elif not oob_possible and not host_possible: + # Neither role is feasible -- fall back to legacy hostname/serial + # warning behaviour so the user still sees a useful message. + if existing_by_serial.name and existing_by_serial.name.lower() == hostname.lower(): + block_warnings.append( + f"Device with same serial and hostname exists as '{existing_by_serial.name}' " + f"({_describe_link_note(existing_link)})" + ) + serial_action_value = "link" + else: + block_warnings.append( + f"Device with same serial ({serial}) exists as '{existing_by_serial.name}' " + f"but hostname differs (LibreNMS: '{hostname}'). Device may have been reinstalled." + ) + serial_action_value = "hostname_differs" + + return { + "serial_action": serial_action_value, + "oob_candidate": oob_candidate_data, + "promote_to_host": promote_to_host_data, + "serial_role_choice_available": oob_possible and host_possible, + "warnings": block_warnings, + } + + def validate_device_for_import( libre_device: dict, import_as_vm: bool = False, @@ -217,10 +551,16 @@ def validate_device_for_import( "resolved_name": None, # Final device name after applying user preferences "existing_device": None, "existing_match_type": None, # Track how existing device was matched - "serial_action": None, # None, "link", "conflict", "update_serial", "hostname_differs" + "ambiguous_librenms_id": False, # True when the librenms_id matches >1 NetBox object (import blocked) + "serial_action": None, # None, "link", "conflict", "update_serial", "hostname_differs", "oob_candidate", "promote_to_host", "merge_netbox_devices" "serial_confirmed": False, # True when librenms_id match and serial matches "serial_duplicate": False, # True when incoming serial is already on a different device + "serial_role_choice_available": False, # True when both oob_candidate and promote_to_host are valid choices "librenms_id_needs_migration": False, # True when existing device has legacy bare-int ID + "oob_candidate": None, # dict {device, type, version, ip} when oob_candidate detected + # promote_to_host is only set when the host-promotion path is available; absent otherwise. + "existing_librenms_link": None, # dict {host_id, oob_id, oob_type} describing existing device's current LibreNMS linkage + "merge_candidates": None, # dict {host_named: {pk,name,librenms_link}, oob_named: {pk,name,librenms_link}} when two NB devices look like the same physical box "name_matches": False, # True when existing device name matches LibreNMS sysName "name_sync_available": False, # True when existing device name differs from sysName "suggested_name": None, # sysName to suggest when name_sync_available is True @@ -297,8 +637,30 @@ def validate_device_for_import( # Check for existing VM first (by librenms_id custom field). # find_by_librenms_id() covers both the new per-server JSON format - # and legacy bare-integer values so neither is missed. - existing_vm = find_by_librenms_id(VirtualMachine, librenms_id, server_key) + # and legacy bare-integer values so neither is missed. An ambiguous id + # (matching multiple records) blocks the import — see _flag_ambiguous_librenms_id. + try: + existing_vm = find_by_librenms_id(VirtualMachine, librenms_id, server_key) + except AmbiguousLibreNMSIdError as exc: + existing_vm = None + _flag_ambiguous_librenms_id(result, librenms_id, exc) + + # Cross-model collision: the same librenms_id on both a VM and a Device is ambiguous. + # Without this, the VM lookup wins and the Device lookup below is skipped, silently + # binding to the VM. Detect it and fail closed (the device block is gated on the flag). + if existing_vm is not None: + try: + _device_collision = find_by_librenms_id(Device, librenms_id, server_key) + except AmbiguousLibreNMSIdError as exc: + # An ambiguous device lookup is itself a fail-closed condition: drop the + # VM binding too so the block below cannot rebind it as a definitive + # "librenms_id" match (the import is already flagged ambiguous). + _device_collision = None + _flag_ambiguous_librenms_id(result, librenms_id, exc) + existing_vm = None + if _device_collision is not None: + _flag_ambiguous_librenms_id(result, librenms_id, "matches both a VirtualMachine and a Device") + existing_vm = None if existing_vm: logger.info(f"Found existing VM: {existing_vm.name} (matched by librenms_id={librenms_id})") @@ -306,6 +668,9 @@ def validate_device_for_import( result["existing_match_type"] = "librenms_id" result["import_as_vm"] = True # Force VM mode since VM exists result["can_import"] = False + # Surface the host/OOB linkage so a librenms_id-matched VM renders as linked + # (mirrors the device path); otherwise the UI shows the VM as unlinked. + result["existing_librenms_link"] = _describe_existing_librenms_link(existing_vm, server_key) # Detect legacy bare-integer or string-digit format so UI can offer a migration action. # Direct access needed to detect legacy format for migration prompt: @@ -324,9 +689,14 @@ def validate_device_for_import( # Check for existing Device (by librenms_id custom field). # find_by_librenms_id() covers both the new per-server JSON format - # and legacy bare-integer values so neither is missed. - if not result["existing_device"]: - existing_device = find_by_librenms_id(Device, librenms_id, server_key) + # and legacy bare-integer values so neither is missed. Skip when an ambiguity + # (intra-model or cross-model) was already flagged — binding must fail closed. + if not result["existing_device"] and not result["ambiguous_librenms_id"]: + try: + existing_device = find_by_librenms_id(Device, librenms_id, server_key) + except AmbiguousLibreNMSIdError as exc: + existing_device = None + _flag_ambiguous_librenms_id(result, librenms_id, exc) if existing_device: logger.info(f"Found existing device: {existing_device.name} (matched by librenms_id={librenms_id})") @@ -338,6 +708,15 @@ def validate_device_for_import( result["import_as_vm"] = False result["can_import"] = False + # If the match was via the OOB sub-key, mark it so the UI shows no duplicate warning. + _existing_oob = get_librenms_oob(existing_device, server_key=server_key) + if _existing_oob and coerce_librenms_id(_existing_oob.get("id")) == coerce_librenms_id(librenms_id): + result["existing_match_type"] = "librenms_oob" + + # Surface the full host/OOB linkage so the import table can render + # both halves of an existing pair with consistent paired styling. + result["existing_librenms_link"] = _describe_existing_librenms_link(existing_device, server_key) + # Detect legacy bare-integer or string-digit format so UI can offer a migration action. # Direct access needed to detect legacy format for migration prompt: # LibreNMSAPI.get_librenms_id() returns an int in both formats, so only the @@ -367,9 +746,12 @@ def validate_device_for_import( result["name_sync_available"] = True result["suggested_name"] = hostname - # Check for serial drift on the linked device + # Check for serial drift on the linked device. Skip when the match was via the + # OOB sub-key (existing_match_type == "librenms_oob"): the incoming payload is the + # OOB controller's, so comparing it against the host record's serial would surface + # bogus replacement/conflict warnings on a row that is already correctly linked. incoming_serial = libre_device.get("serial") or "" - if incoming_serial and incoming_serial != "-": + if result["existing_match_type"] != "librenms_oob" and incoming_serial and incoming_serial != "-": if existing_device.serial and existing_device.serial == incoming_serial: result["serial_confirmed"] = True elif existing_device.serial and existing_device.serial != incoming_serial: @@ -391,8 +773,11 @@ def validate_device_for_import( f"LibreNMS: '{incoming_serial}'). Hardware may have been replaced." ) - # Only check hostname/serial/IP if not already matched by librenms_id - if not result["existing_device"]: + # Only check hostname/serial/IP if not already matched by librenms_id. + # Skip when an ambiguous librenms_id was flagged — hostname/serial/IP matching + # would otherwise rebind existing_device/existing_match_type and defeat the + # fail-closed ambiguity contract (mirrors the librenms_id block guard above). + if not result["existing_device"] and not result["ambiguous_librenms_id"]: # Check by hostname/name - Check both VMs and Devices for conflicts existing_vm = VirtualMachine.objects.filter(name__iexact=hostname).first() existing_device = Device.objects.filter(name__iexact=hostname).first() @@ -414,8 +799,14 @@ def validate_device_for_import( result["existing_device"] = existing_vm result["existing_match_type"] = "hostname" result["import_as_vm"] = True # Force VM mode since VM exists + # Describe the VM's current LibreNMS linkage and word the warning to match it — + # a hostname-matched VM can already carry a librenms_id (to a different id/server), + # so a flat "not linked" would contradict the badge. Mirrors the primary-IP path. + existing_link = _describe_existing_librenms_link(existing_vm, server_key) + result["existing_librenms_link"] = existing_link + link_note = _describe_link_note(existing_link) result["warnings"].append( - f"VM with same hostname exists in NetBox as '{existing_vm.name}' (not linked to LibreNMS)" + f"VM with same hostname exists in NetBox as '{existing_vm.name}' ({link_note})" ) result["can_import"] = False elif existing_device: @@ -425,6 +816,9 @@ def validate_device_for_import( # Force Device mode (see the librenms_id branch above): a hostname match to a # Device must not leave a user-selected VM mode active. result["import_as_vm"] = False + # Surface the current host/OOB linkage so a hostname-matched device that + # is already linked to LibreNMS isn't mislabelled as "not linked". + result["existing_librenms_link"] = _describe_existing_librenms_link(existing_device, server_key) # Check for serial conflict on hostname-matched device incoming_serial = libre_device.get("serial") or "" @@ -447,8 +841,9 @@ def validate_device_for_import( f"LibreNMS: '{incoming_serial}'). Hardware may have been replaced." ) else: + link_note = _describe_link_note(result["existing_librenms_link"]) result["warnings"].append( - f"Device with same hostname exists in NetBox as '{existing_device.name}' (not linked to LibreNMS)" + f"Device with same hostname exists in NetBox as '{existing_device.name}' ({link_note})" ) result["can_import"] = False @@ -483,44 +878,306 @@ def validate_device_for_import( result["existing_match_type"] = "serial" result["can_import"] = False - if existing_by_serial.name and existing_by_serial.name.lower() == hostname.lower(): + # Capture existing device's current LibreNMS linkage so the UI can + # present accurate state (NOT just "not linked to LibreNMS"). + existing_link = _describe_existing_librenms_link(existing_by_serial, server_key) + result["existing_librenms_link"] = existing_link + + # Decide OOB-candidate / promote-to-host / link / hostname-differs role + # for the incoming device (pure: reads state, doesn't mutate result), + # then apply the decision. + apply_oob_detection_result( + result, + **_detect_serial_match_role( + existing_by_serial, existing_link, hostname, serial, libre_device, server_key + ), + ) + + # Refresh local variable to reflect any VM-mode adjustments made during detection + # (e.g. existing VM found by hostname sets result["import_as_vm"] = True). + # Must happen before the merge-candidates block below so a VM hostname-match + # doesn't fall through to Device-only merge logic. + import_as_vm = result["import_as_vm"] + + # Fail closed against an ARBITRARY duplicate match for the current side, independent of + # whether a usable serial is available. The hostname/serial match above used .first(), + # so with duplicate NetBox names/serials result["existing_device"] may be an arbitrary + # row and acting on it (link/promote/import) would target the wrong device. This must + # run even for a hostname match with no serial — the merge-candidate block below is + # gated on a serial (it pairs host+OOB) and would otherwise skip the check entirely. + # Compute the duplicate-detection peer lists ONCE: the Stage-1 duplicate guard here and + # the Stage-2 merge-candidate detection below both run the identical UNIQUE [:2] query + # for the matched type, so share the result instead of issuing it twice per device. + _match_type = result.get("existing_match_type") + _serial_now = (libre_device.get("serial") or "").strip() + _dup_eligible = ( + not import_as_vm and result.get("existing_device") is not None and _match_type in ("hostname", "serial") + ) + _hostname_peers = ( + list(Device.objects.filter(name__iexact=hostname)[:2]) + if _dup_eligible and _match_type == "hostname" and hostname + else [] + ) + _serial_peers = ( + # Reuse the Stage-1 serial [:2] result (issue #101 guard, computed above) instead of + # re-issuing the identical UNIQUE query — keeps the single-query invariant while the + # fail-closed guard stays in place. Only defined when existing_match_type == "serial". + serial_matches + if _dup_eligible and _match_type == "serial" and _serial_now and _serial_now != "-" + else [] + ) + if _dup_eligible: + _dup_current = False + if _match_type == "hostname" and hostname: + _dup_current = len(_hostname_peers) > 1 + elif _match_type == "serial": + _dup_current = bool(_serial_now) and _serial_now != "-" and len(_serial_peers) > 1 + if _dup_current: + # Arbitrary .first() match among duplicates: block link/promote/import and + # surface a blocking issue. The match is left for display only. + # Drop the arbitrary existing_device and all match-derived linkage/name state: + # this is a terminal ambiguity, so retaining the .first() row would let + # bulk_import treat it as a real existing match — `_refresh_existing_device` + # short-circuits on a set existing_device (skipping the ambiguity re-check) and + # the exclude_existing / collision paths key off it — pinning the row to the + # wrong device. Only existing_match_type carries the ambiguity forward. + result["existing_device"] = None + result["existing_librenms_link"] = None + result["name_matches"] = False + result["name_sync_available"] = False + result["suggested_name"] = None + result["serial_confirmed"] = False + result["serial_duplicate"] = False + # Demote the match_type off "hostname"/"serial" so neither the device_status + # table (has_actions) nor device_validation_details.html renders a "Link to + # LibreNMS" action — otherwise the arbitrary row could be linked to the wrong + # NetBox device. Mirrors the "ambiguous_librenms_id" terminal-state pattern. + result["existing_match_type"] = "ambiguous_hostname_or_serial" + result["serial_action"] = None + result["oob_candidate"] = None + result.pop("promote_to_host", None) + result["serial_role_choice_available"] = False + result["can_import"] = False + result["is_ready"] = False + _dup_msg = ( + "Multiple NetBox devices share this device's hostname/serial; resolve the " + "duplicate before importing or linking." + ) + if _dup_msg not in result.setdefault("issues", []): + result["issues"].append(_dup_msg) + # Terminal, like the ambiguous_librenms_id and primary-IP-ambiguity guards: + # return now. We cleared existing_device above, so without this the + # primary-IP fallback pass (and the new-import validation) below would run + # and re-bind existing_device + demote match_type to "primary_ip" — silently + # re-homing this duplicate-hostname/serial row onto an arbitrary IP-matched + # device and dropping the terminal blocker the cleanup keys on. + return result + + # Stage 2 — merge-candidates detection. + # When the hostname-matched device and the serial-matched device are + # DIFFERENT NetBox objects, the two probably represent the same + # physical box (host + OOB) imported as separate entries. Surface + # this as a merge action instead of silently picking one. + try: + _serial_for_pair = (libre_device.get("serial") or "").strip() + if ( + _serial_for_pair + and _serial_for_pair != "-" + and not import_as_vm + and result.get("existing_device") is not None + and result.get("existing_match_type") in ("hostname", "serial") + ): + # The CURRENT side comes from result["existing_device"], which an earlier + # hostname/serial match set via .first() — so with duplicate NetBox names or + # serials it could be an arbitrary row, pairing the user with the wrong merge + # target. Re-validate the current side with the same UNIQUE [:2] guard used for + # the peer below: keep it as a candidate only when exactly one row matches, + # otherwise skip the merge suggestion and warn. + _hostname_match = None + _serial_match = None + # A non-unique current side (duplicate name/serial) is already failed closed + # above; here it just means we can't pick a single peer to pair, so skip the + # merge suggestion and warn. + if result.get("existing_match_type") == "hostname" and hostname: + # Reuse the Stage-1 peer list (identical name__iexact[:2] query). + if len(_hostname_peers) == 1: + _hostname_match = _hostname_peers[0] + elif len(_hostname_peers) > 1: result["warnings"].append( - f"Device with same serial and hostname exists as '{existing_by_serial.name}' " - f"(not linked to LibreNMS)" + f"Multiple NetBox devices share hostname '{hostname}'; merge suggestion skipped." ) - result["serial_action"] = "link" - else: + elif result.get("existing_match_type") == "serial": + # Reuse the Stage-1 peer list (identical serial[:2] query). + if len(_serial_peers) == 1: + _serial_match = _serial_peers[0] + elif len(_serial_peers) > 1: + result["warnings"].append( + f"Multiple NetBox devices share serial '{_serial_for_pair}'; merge suggestion skipped." + ) + # Whichever path landed first, look the other one up too. Require a UNIQUE + # peer: serial isn't unique in NetBox (and names are only unique per site), + # so a bare .first() could pair the matched device with an arbitrary row and + # surface the wrong merge target. Fetch up to 2; only pair on exactly one, + # otherwise skip the suggestion and warn. + if _hostname_match and not _serial_match: + _serial_peers = list( + Device.objects.filter(serial=_serial_for_pair).exclude(pk=_hostname_match.pk)[:2] + ) + if len(_serial_peers) == 1: + _serial_match = _serial_peers[0] + elif len(_serial_peers) > 1: result["warnings"].append( - f"Device with same serial ({serial}) exists as '{existing_by_serial.name}' " - f"but hostname differs (LibreNMS: '{hostname}'). Device may have been reinstalled." + f"Multiple NetBox devices share serial '{_serial_for_pair}'; merge suggestion skipped." + ) + elif _serial_match and not _hostname_match and hostname: + _hostname_peers = list( + Device.objects.filter(name__iexact=hostname).exclude(pk=_serial_match.pk)[:2] + ) + if len(_hostname_peers) == 1: + _hostname_match = _hostname_peers[0] + elif len(_hostname_peers) > 1: + result["warnings"].append( + f"Multiple NetBox devices share hostname '{hostname}'; merge suggestion skipped." ) - result["serial_action"] = "hostname_differs" + + if _hostname_match and _serial_match and _hostname_match.pk != _serial_match.pk: + host_link = _describe_existing_librenms_link(_hostname_match, server_key) + oob_link = _describe_existing_librenms_link(_serial_match, server_key) + # Conservative guard: at least one side must already be linked, + # otherwise this is more likely two unrelated devices that share + # serial data by coincidence (test fixtures, mis-keyed assets). + # A LibreNMS link counts whether it's a host link or an OOB link. + if any(link and (link.get("host_id") or link.get("oob_id")) for link in (host_link, oob_link)): + apply_merge_candidates( + result, + host_named={ + "pk": _hostname_match.pk, + "name": _hostname_match.name, + "librenms_link": host_link, + }, + oob_named={ + "pk": _serial_match.pk, + "name": _serial_match.name, + "librenms_link": oob_link, + }, + warning=( + f"Two NetBox devices appear to represent this physical box: " + f"'{_hostname_match.name}' (matches LibreNMS hostname) and " + f"'{_serial_match.name}' (matches chassis serial). " + f"Choose which one to keep and merge the other into it." + ), + ) + + except Exception: # pragma: no cover - defensive: never break validation + logger.exception("merge-candidate detection failed") # Check by primary IP (weaker match, IP could be reassigned) - only for devices if not result["existing_device"]: primary_ip = libre_device.get("ip") if primary_ip and not import_as_vm: - from ipam.models import IPAddress - - existing_ip = IPAddress.objects.filter(address__net_host=primary_ip).first() - if existing_ip and existing_ip.assigned_object: - device = ( - existing_ip.assigned_object.device - if hasattr(existing_ip.assigned_object, "device") - else None + # Shared resolver: scans every net_host row across the interface-assignment and + # oob_ip-FK paths and fails closed on >1 distinct device (mirrors + # _refresh_existing_device in bulk_import.py via the same helper). + device, ip_ambiguous, matching_ips = resolve_device_by_host_ip(primary_ip) + if ip_ambiguous: + # Duplicate net_host rows point at >1 distinct NetBox device — binding to + # an arbitrary one could re-home the import to the wrong device, so fail + # closed (mirrors the librenms_id cross-model ambiguity guard above). + # Put this collision into the same terminal ambiguity state the + # hostname/serial guard uses, so a cached primary-IP collision can be + # cleared by _refresh_existing_device() once the duplicate IP assignment is + # resolved. The message carries the shared "serial or management IP" marker + # the refresh cleanup strips on; without the match_type + marker the stale + # blocker would survive refresh and keep the row blocked until cache expiry. + result["existing_match_type"] = "ambiguous_hostname_or_serial" + result["issues"].append( + f"Multiple NetBox devices match this device's serial or management IP " + f"(IP address {primary_ip}); resolve the duplicate assignment before importing." + ) + result["can_import"] = False + result["is_ready"] = False + elif device: + # Surface any existing host/OOB linkage so the import UI renders the + # correct row state (the librenms_id / serial branches do the same; + # without this an already-linked device shows as "not linked" here). + result["existing_librenms_link"] = _describe_existing_librenms_link(device, server_key) + # Check if this is an OOB candidate via the IP path. + # The OOB controller's IP may already be the device's oob_ip, or the + # LibreNMS device may identify itself as an OOB type (iDRAC/iLO/etc.). + oob_type = normalize_oob_type( + libre_device.get("os", ""), + libre_device.get("hardware", ""), + ) + # Check the device's oob_ip against EVERY row sharing this host address, + # not just existing_ip (.first()): with duplicate net_host rows the device's + # oob_ip may be a different matching row, and comparing only the first would + # wrongly read is_oob_ip as False. + is_oob_ip = device.oob_ip_id is not None and matching_ips.filter(pk=device.oob_ip_id).exists() + has_primary_ip = bool(device.primary_ip4_id or device.primary_ip6_id) + # When the incoming IP already IS the device's oob_ip this is an OOB + # candidate regardless of whether the LibreNMS os/hardware tokens let us + # classify a type. Requiring oob_type here silently downgrades such a row + # to a plain primary-IP match and loses the OOB action flow, so infer a + # type from the hostname (or a generic "oob" fallback), mirroring the + # serial-match branch above. + inferred_oob_type = ( + oob_type + or _detect_oob_type_from_name( + libre_device.get("hostname") or libre_device.get("sysName") or "" + ) + or "oob" ) - if device: + if is_oob_ip or (oob_type and not has_primary_ip): + existing_oob = get_librenms_oob(device, server_key=server_key) + if existing_oob is None: + result["existing_device"] = device + result["existing_match_type"] = "primary_ip" + result["serial_action"] = "oob_candidate" + result["oob_candidate"] = { + "device": device, + "type": inferred_oob_type, + "version": libre_device.get("version") or None, + "ip": libre_device.get("ip") or None, + } + result["can_import"] = False + else: + result["existing_device"] = device + result["existing_match_type"] = "primary_ip" + result["warnings"].append( + f"IP address {primary_ip} already assigned to device '{device.name}' " + f"(OOB already linked)" + ) + result["can_import"] = False + else: result["existing_device"] = device result["existing_match_type"] = "primary_ip" + # Line 728 may already have populated a host/OOB + # linkage; describe it accurately instead of always + # claiming "not linked to LibreNMS". + link_note = _describe_link_note(result.get("existing_librenms_link")) result["warnings"].append( - f"IP address {primary_ip} already assigned to device '{device.name}' (not linked to LibreNMS)" + f"IP address {primary_ip} already assigned to device '{device.name}' ({link_note})" ) result["can_import"] = False - # Refresh local variable to reflect any VM-mode adjustments made during detection - # (e.g. existing VM found by hostname sets result["import_as_vm"] = True) + # Refresh local mode after ALL detection branches. The refresh at the top of the + # unmatched-device block only runs when nothing matched by librenms_id; an existing + # VM matched directly by librenms_id (above) sets result["import_as_vm"]=True but + # skips that block, so without this a linked VM would wrongly take the Device path + # (missing cluster["available_clusters"], running device-only validation/VC detection). import_as_vm = result["import_as_vm"] + # An ambiguous librenms_id (matches >1 NetBox record) is the terminal blocker for this + # row — the user must resolve the duplicate id. Don't run the new-import site/device_type/ + # role/cluster validation below: with existing_device fail-closed to None, it would pile + # unrelated "must select ..." blockers onto a row whose real problem is the ambiguous id + # (mirrors bulk_import.py treating ambiguity as the terminal state). existing_match_type + # is already "ambiguous_librenms_id" (set by _flag_ambiguous_librenms_id). + if result["ambiguous_librenms_id"]: + result["can_import"] = False + result["is_ready"] = False + return result + # Validate based on import type (Device or VM) if import_as_vm: # Always populate available clusters for all VMs (new or existing) so @@ -705,8 +1362,15 @@ def validate_device_for_import( result["device_role"]["found"] = True result["device_role"]["role"] = existing.role - # Check for device type mismatch between existing device and LibreNMS - if hasattr(existing, "device_type") and existing.device_type: + # Check for device type mismatch between existing device and LibreNMS. Skip for an + # OOB-sub-key match (existing_match_type == "librenms_oob"): the LibreNMS payload is + # the OOB controller's, so a host-vs-OOB device-type compare is a bogus "wrong device" + # warning on a correctly-linked row. + if ( + result.get("existing_match_type") != "librenms_oob" + and hasattr(existing, "device_type") + and existing.device_type + ): librenms_dt = result["device_type"].get("device_type") if librenms_dt and existing.device_type.pk != librenms_dt.pk: result["device_type_mismatch"] = True @@ -740,7 +1404,7 @@ def validate_device_for_import( except Exception as e: logger.exception(f"Error validating device for import: {libre_device.get('hostname', 'unknown')}") - result["issues"].append(f"Validation error: {str(e)}") + result["issues"].append(f"{VALIDATION_ERROR_ISSUE_PREFIX} {str(e)}") return result @@ -827,6 +1491,35 @@ def import_single_device( "synced": {}, } + # Hard fail-closed guard: an ambiguous librenms_id (matches >1 NetBox record) is the + # terminal blocker — validate_device_for_import() sets existing_device=None for it, so the + # check above doesn't catch it, and a manual_mappings import would otherwise create a + # duplicate Device under the ambiguous id. Block the create outright. + if validation.get("ambiguous_librenms_id"): + return { + "success": False, + "device": None, + "message": "", + "error": "Import blocked: ambiguous LibreNMS ID matches multiple NetBox records.", + "synced": {}, + } + + # Parallel terminal-ambiguity guard: a duplicate hostname/serial match is also a terminal + # blocker — validate_device_for_import() sets existing_device=None AND + # existing_match_type="ambiguous_hostname_or_serial" for it, so neither the existing_device + # check above nor the ambiguous_librenms_id guard catches it. Without this, a manual_mappings + # import (which supplies site/type/role and so skips the `if not site` fail-closed below) + # would create a duplicate Device under the unresolved ambiguity — the same fail-open the + # ambiguous_librenms_id guard exists to prevent. + if validation.get("existing_match_type") == "ambiguous_hostname_or_serial": + return { + "success": False, + "device": None, + "message": "", + "error": "Import blocked: this device's hostname, serial, or management IP matches multiple NetBox devices; resolve the duplicate first.", + "synced": {}, + } + # Use validation-derived matches, allow manual mappings to override specific fields site = validation["site"].get("site") device_type = validation["device_type"].get("device_type") @@ -1056,3 +1749,28 @@ def fetch_device_with_cache( cache.set(cache_key, libre_device, timeout=api.cache_timeout) return libre_device + + +def __getattr__(name): + """ + Lazily re-export ``bulk_import_devices_shared`` (PEP 562 module ``__getattr__``). + + A top-level ``from .bulk_import import bulk_import_devices_shared`` would be a + circular import (``bulk_import`` imports ``validate_device_for_import`` from this + module at load time), so PEP 562 defers the import until the attribute is + accessed. + + Args: + name (str): The attribute name being looked up on the module. + + Returns: + The ``bulk_import_devices_shared`` callable when requested. + + Raises: + AttributeError: If *name* is any other attribute. + """ + if name == "bulk_import_devices_shared": + from .bulk_import import bulk_import_devices_shared + + return bulk_import_devices_shared + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") diff --git a/netbox_librenms_plugin/import_validation_helpers.py b/netbox_librenms_plugin/import_validation_helpers.py index cf26c6a407..70f2892dec 100644 --- a/netbox_librenms_plugin/import_validation_helpers.py +++ b/netbox_librenms_plugin/import_validation_helpers.py @@ -137,6 +137,102 @@ def remove_validation_issue(validation: dict, keyword: str) -> None: validation["issues"] = [issue for issue in validation["issues"] if keyword.lower() not in issue.lower()] +def apply_oob_detection_result( + result: dict, + *, + serial_action: "str | None", + oob_candidate: "dict | None", + promote_to_host: "dict | None", + serial_role_choice_available: bool, + warnings: "list | None" = None, +) -> None: + """Apply OOB/promote-to-host serial detection results to the validation dict. + + Call this after computing all OOB/promote-to-host flags from the LibreNMS + and NetBox data. All mutations to ``result["oob_candidate"]``, + ``result["promote_to_host"]``, ``result["serial_action"]``, + ``result["serial_role_choice_available"]``, and their associated warnings + are routed through here so the mutation pattern stays consistent and + testable independently of the DB-heavy computation in device_operations. + + Args: + result: Validation dict produced by validate_device_for_import() + serial_action: The resolved action string, or None + oob_candidate: Dict {device, type, version, ip} when OOB role is available + promote_to_host: Dict {existing_libre_id, existing_oob_type, existing_device} + when host-promotion is available + serial_role_choice_available: True when both oob_candidate and + promote_to_host are feasible and the UI should offer a toggle + warnings: Optional list of warning strings to append to result["warnings"] + """ + result["serial_action"] = serial_action + result["oob_candidate"] = oob_candidate + # Honor the "absent otherwise" contract: only carry promote_to_host when a real + # promotion target exists, clearing any stale key rather than storing a None sentinel. + if promote_to_host is None: + result.pop("promote_to_host", None) + else: + result["promote_to_host"] = promote_to_host + result["serial_role_choice_available"] = serial_role_choice_available + # Clear merge-only state: this is the non-merge path, so if the same result + # dict was previously marked a merge candidate, the stale merge UI data must + # not linger (apply_merge_candidates is the only writer of merge_candidates). + result["merge_candidates"] = None + result.setdefault("warnings", []) + for warning in warnings or []: + result["warnings"].append(warning) + + +def apply_merge_candidates( + result: dict, + *, + host_named: dict, + oob_named: dict, + warning: str, +) -> None: + """Apply merge-candidates detection results to the validation dict. + + Called when the hostname-matched and serial-matched NetBox devices are + different objects and at least one already has a LibreNMS linkage, + indicating they likely represent the two sides of a single physical box. + + Sets ``serial_action`` to ``"merge_netbox_devices"``, populates + ``merge_candidates``, sets ``can_import`` to False, and appends the + supplied warning so callers do not need to know the dict shape. + + Args: + result: Validation dict produced by validate_device_for_import() + host_named: Dict {pk, name, librenms_link} for the hostname-matched device + oob_named: Dict {pk, name, librenms_link} for the serial-matched device + warning: Warning string describing the merge situation + """ + result["serial_action"] = "merge_netbox_devices" + result["merge_candidates"] = { + "host_named": host_named, + "oob_named": oob_named, + } + result["can_import"] = False + # Keep readiness in lockstep with can_import: an earlier path (e.g. hostname-first row + # processing) may have set is_ready=True, which would otherwise leave contradictory state + # (is_ready=True while merge mode blocks import). + result["is_ready"] = False + result["oob_candidate"] = None + # Clear earlier serial-conflict state so the merge path is the single source of truth: + # a hostname-first row may have already set serial_duplicate / serial_confirmed, which + # would otherwise leave a stale "serial conflict" signal alongside "merge these devices". + result["serial_duplicate"] = False + result["serial_confirmed"] = False + # "absent otherwise" contract — the merge path has no promotion target. + result.pop("promote_to_host", None) + result["serial_role_choice_available"] = False + # Merge supersedes the serial/hostname-detection signals that ran earlier in this + # validation pass, so their warnings (e.g. "hostname differs", "already has an OOB + # controller linked", "serial conflict") would now contradict the merge guidance. + # Reset to just the merge warning; later validation stages (role/platform/cluster) + # append their own warnings after this point, so nothing actionable is lost. + result["warnings"] = [warning] + + def recalculate_validation_status(validation: dict, is_vm: bool = False) -> None: """ Recalculate can_import and is_ready flags based on current validation state. @@ -155,6 +251,15 @@ def recalculate_validation_status(validation: dict, is_vm: bool = False) -> None Required fields for VMs: - cluster """ + # Merge mode is a hard block that does not live in ``issues`` — apply_merge_candidates() + # sets can_import=False directly. Without this guard a later mutation (e.g. applying a role + # selection, which calls back here) would recompute can_import purely from ``issues`` and + # silently re-enable importing a merge-candidate row, bypassing the merge resolution. + if validation.get("serial_action") == "merge_netbox_devices" or validation.get("merge_candidates"): + validation["can_import"] = False + validation["is_ready"] = False + return + validation["can_import"] = len(validation["issues"]) == 0 if is_vm: diff --git a/netbox_librenms_plugin/librenms_api.py b/netbox_librenms_plugin/librenms_api.py index 7b2ded8334..49cd7855a0 100644 --- a/netbox_librenms_plugin/librenms_api.py +++ b/netbox_librenms_plugin/librenms_api.py @@ -18,11 +18,48 @@ logger = logging.getLogger(__name__) +def build_librenms_api(server_key): + """ + Build a :class:`LibreNMSAPI` for a server key, tolerating bad keys. + + ``LibreNMSAPI(server_key=...)`` raises ``KeyError`` for an unknown non-default + key and ``ValueError`` when the URL/token is missing. Views take ``server_key`` + from request POST, where a stale page or tampered request can carry a key that + no longer exists — returning ``None`` lets the caller surface a user-facing + error instead of an unhandled 500. + + Args: + server_key (str): The configured LibreNMS server key to build a client for. + + Returns: + LibreNMSAPI | None: A client for *server_key*, or None when the key is + unknown or the server is misconfigured. + """ + try: + return LibreNMSAPI(server_key=server_key) + except (KeyError, ValueError): + return None + + class LibreNMSAPI: """ Client to interact with the LibreNMS API and retrieve interface data for devices. """ + @staticmethod + def _is_usable_server_config(config): + """ + Return True only for a server mapping that ``__init__`` can bind. + + A server entry is usable only when it is a dict carrying a non-empty + ``librenms_url`` and ``api_token`` — the same fields ``__init__`` requires + before it will build a client. Sharing this predicate keeps the server + picker (``get_available_servers``) and the auto-default fallback from + offering, or silently selecting, a partially configured entry that would + immediately raise ``ValueError``. + """ + return isinstance(config, dict) and bool(config.get("librenms_url")) and bool(config.get("api_token")) + def __init__(self, server_key=None): """ Initialize LibreNMS API client with support for multiple servers. @@ -75,18 +112,16 @@ def __init__(self, server_key=None): raise KeyError( f"Server '{server_key}' not found in LibreNMS plugin configuration. Available servers: {available}" ) - # Pick the first *usable* mapping — a dict that actually carries a url + token. A - # non-dict or incomplete entry would otherwise be selected and then raise at the config - # read below, masking a perfectly usable later server (e.g. {"bad": {}, "prod": {...}}). - # If none are usable, fail with a clear error (issue #110). + # Skip partially configured entries so the auto-default doesn't land on a server + # missing its url/token (which __init__ would reject below) while a later entry is + # fully usable. first_key = next( - ( - key - for key, config in servers_config.items() - if isinstance(config, dict) and config.get("librenms_url") and config.get("api_token") - ), + (k for k, cfg in servers_config.items() if self._is_usable_server_config(cfg)), None, ) + # #110: a non-empty servers_config with no usable entry must surface a clear error + # rather than silently falling through to a (possibly stale) legacy single-server + # config. build_librenms_api() converts this ValueError into a clean None. if first_key is None: raise ValueError("No valid LibreNMS server configuration entries found.") logger.info( @@ -105,7 +140,10 @@ def __init__(self, server_key=None): # entry (e.g. {"default": "not-a-dict"}) would otherwise raise an opaque TypeError # at the key reads below. Fail with a clear configuration error instead (issue #110). if not isinstance(config, dict): - raise ValueError(f"Invalid LibreNMS server configuration for '{server_key}': expected a mapping.") + raise ValueError( + f"LibreNMS server '{server_key}' is misconfigured " + f"(expected a mapping, got {type(config).__name__})." + ) # Read with .get() rather than direct indexing: a dict-shaped but incomplete entry # (e.g. {"default": {}}) passes the isinstance check, so config["librenms_url"] would # raise an opaque KeyError instead of the ValueError contract callers rely on. The @@ -209,12 +247,17 @@ def get_available_servers(cls): # Multi-server configuration result = {} for key, config in servers_config.items(): - # Skip non-usable entries: a non-dict raises on config.get(...) below, and a - # dict-shaped but incomplete entry (no librenms_url/api_token, e.g. {"bad": {}}) - # would pass the redirect/rebind membership check that keys off this map and then - # blow up LibreNMSAPI(server_key="bad"). A server that can't be constructed must not - # be selectable — exposing it 500s a sync POST instead of degrading to the active one. - if not isinstance(config, dict) or not config.get("librenms_url") or not config.get("api_token"): + # Only offer servers that __init__ can actually bind: a dict with a non-empty + # librenms_url and api_token. Mirroring the constructor's validation keeps a + # malformed (non-mapping) or partially configured entry from appearing selectable + # and then failing the moment it is chosen. + if not cls._is_usable_server_config(config): + logger.warning( + "Skipping unusable LibreNMS server config %r (needs a librenms_url and api_token).", + key, + ) + continue + if not config.get("librenms_url") or not config.get("api_token"): continue result[key] = config.get("display_name", key) return result @@ -225,7 +268,7 @@ def get_available_servers(cls): return {"default": f"Default Server ({legacy_url})"} return {"default": "Default Server"} - def get_stored_librenms_id(self, obj): + def get_stored_librenms_id(self, obj, server_key=None): """ Return the stored or cached LibreNMS ID for an object without discovery. @@ -234,18 +277,25 @@ def get_stored_librenms_id(self, obj): Args: obj: NetBox object with a librenms_id custom field or cache identity + server_key: LibreNMS server key to read the per-server id under; defaults + to this client's bound ``server_key``. A caller scoped to a specific + server (e.g. the module verify path, which sets ``_active_server_key`` + but leaves the API bound to the default client) must pass its key so + the multi-server dict CF is read under the right server rather than the + client's default. Returns: int: LibreNMS ID if found in the custom field or cache, None otherwise """ from netbox_librenms_plugin.utils import get_librenms_device_id - librenms_id = get_librenms_device_id(obj, self.server_key, auto_save=False) + resolved_key = server_key or self.server_key + librenms_id = get_librenms_device_id(obj, resolved_key, auto_save=False) if librenms_id is not None: return librenms_id - # Check cache - cache_key = self._get_cache_key(obj) + # Check cache (scoped to the same server the CF was read under) + cache_key = self._get_cache_key(obj, server_key=resolved_key) librenms_id = cache.get(cache_key) if librenms_id is not None: return librenms_id @@ -281,7 +331,7 @@ def get_librenms_id(self, obj): primary_ip_address = getattr(primary_ip, "address", None) ip_address = getattr(primary_ip_address, "ip", None) if primary_ip else None dns_name = getattr(primary_ip, "dns_name", None) if primary_ip else None - hostname = getattr(obj, "name", None) or None + hostname = getattr(obj, "name", None) # Try IP address if ip_address: @@ -308,31 +358,38 @@ def get_librenms_id(self, obj): @staticmethod def _normalize_librenms_id(value): - """Coerce a raw LibreNMS ID value to int or None. + """ + Coerce a raw LibreNMS ID value to int or None. + + Thin wrapper around :func:`netbox_librenms_plugin.utils.coerce_librenms_id` + kept for back-compat with internal callers in this module. + + Args: + value: The raw LibreNMS id value (int, digit string, or other). - Booleans are rejected because bool is a subclass of int in Python, - so int(True) silently becomes 1 — a valid-looking device ID. + Returns: + int | None: The coerced id, or None if it can't be coerced. """ - if value is None or isinstance(value, bool): - return None - try: - return int(value) - except (ValueError, TypeError): - return None + from netbox_librenms_plugin.utils import coerce_librenms_id + + return coerce_librenms_id(value) - def _get_cache_key(self, obj): + def _get_cache_key(self, obj, server_key=None): """ Generate a unique cache key for an object. Args: obj: NetBox device or VM object + server_key: LibreNMS server key to scope the key to; defaults to this + client's bound ``server_key``. Pass an explicit key when reading on + behalf of a different (scoped) server than the client is bound to. Returns: str: Cache key """ object_type = obj._meta.model_name - server_key = getattr(self, "server_key", "default") - return f"librenms_device_id_{object_type}_{obj.pk}_{server_key}" + resolved_key = server_key if server_key is not None else getattr(self, "server_key", "default") + return f"librenms_device_id_{object_type}_{obj.pk}_{resolved_key}" def _store_librenms_id(self, obj, librenms_id): """ diff --git a/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js b/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js index edfda206a8..c20823c010 100644 --- a/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js +++ b/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js @@ -1203,12 +1203,91 @@ // the outer HTMX modal. Buttons inside nested modals (e.g. the // Promote-to-host modal rendered inside #htmx-modal-content) // must be left for Bootstrap's own dismiss handler so they - // close the inner modal, not the outer one. + // close the inner modal, not the outer one. We also avoid + // preventDefault here so form submit buttons that happen to + // carry data-bs-dismiss="modal" in nested modals still submit. const nearestModal = dismissTrigger.closest('.modal'); if (nearestModal === modalElement) { event.preventDefault(); hideModal(modalElement, fallbackBackdropRef); + } else if ( + nearestModal && + !(typeof bootstrap !== 'undefined' && bootstrap.Modal) && + !(typeof window.bootstrap !== 'undefined' && window.bootstrap.Modal) + ) { + // No-Bootstrap fallback: Bootstrap's own dismiss handler isn't available to + // close the nested modal, so the user would otherwise be stuck inside it. + // Manually hide just the nested modal (not the outer HTMX modal). + // Only suppress default for INERT dismiss controls: a dismiss button that also + // submits a form or triggers an hx-* request must still execute that action, so + // don't preventDefault for those (we still close the nested modal below). + const isActionControl = + // Check the EXPLICIT type attribute, not the computed .type: a ' ) - @staticmethod - def _build_validation_details_url(device_id: int, validation: dict) -> str: + def _build_validation_details_url(self, device_id: int, validation: dict) -> str: """ Build validation details URL with appropriate query parameters. @@ -632,6 +724,14 @@ def _build_validation_details_url(device_id: int, validation: dict) -> str: # Build query params based on import type params = [] + # Scope the modal to the server the import page was rendered for, so the modal-open GET + # (which reaches DeviceValidationDetailsView via its own URL, with no parent handler to + # inject the import-scoped client) fetches from that server rather than the global + # LibreNMSSettings.selected_server (which may have drifted). + server_key = getattr(self, "server_key", None) + if server_key: + params.append(f"server_key={quote_plus(str(server_key))}") + # Add cluster_id if this is a VM import if validation.get("cluster", {}).get("found") and validation.get("cluster", {}).get("cluster"): cluster_id = validation["cluster"]["cluster"].id diff --git a/netbox_librenms_plugin/tables/interfaces.py b/netbox_librenms_plugin/tables/interfaces.py index 34dbc65794..23d0c580ff 100644 --- a/netbox_librenms_plugin/tables/interfaces.py +++ b/netbox_librenms_plugin/tables/interfaces.py @@ -7,6 +7,7 @@ from utilities.paginator import EnhancedPaginator from utilities.templatetags.helpers import humanize_speed +from netbox_librenms_plugin.constants import OOB_BADGE_HTML from netbox_librenms_plugin.models import InterfaceTypeMapping from netbox_librenms_plugin.utils import ( check_vlan_group_matches, @@ -313,7 +314,15 @@ def render_speed(self, value, record): def render_name(self, value, record): """Render interface name with appropriate styling based on comparison with NetBox""" - return self._render_field(value, record, self.interface_name_field, "name") + rendered = self._render_field(value, record, self.interface_name_field, "name") + badges = "" + if record.get("_source") == "oob": + badges += OOB_BADGE_HTML + if record.get("_dedup_conflict"): + badges += 'Shared LOM' + if badges: + return format_html("{}{}", rendered, mark_safe(badges)) + return rendered def _get_interface_status_display(self, enabled, record): """ @@ -501,7 +510,16 @@ def format_interface_data(self, port_data, device): # Add NetBox interface data interface_name = port_data.get(self.interface_name_field) - port_data["netbox_interface"] = device.interfaces.filter(name=interface_name).first() + # OOB-controller rows live on a SEPARATE LibreNMS device — mirror the + # interfaces-tab guard (BaseInterfaceTableView.get_context_data): never bind + # one to a host interface by name. Otherwise a row-level re-render (the VC + # member dropdown via SingleInterfaceVerifyView) flips a deliberately-unmatched + # shared-LOM row to green "matched", comparing speed/MTU/MAC against an + # unrelated host interface and inviting a sync the server then silently skips. + if port_data.get("_source") == "oob": + port_data["netbox_interface"] = None + else: + port_data["netbox_interface"] = device.interfaces.filter(name=interface_name).first() port_data["exists_in_netbox"] = bool(port_data["netbox_interface"]) # Clear description if it matches interface name @@ -574,7 +592,7 @@ def render_device_selection(self, value, record): base_id = f"device_selection_{interface_name}_{hash(interface_name)}" options = [ - f'' + f'' for member in members ] diff --git a/netbox_librenms_plugin/tables/ipaddresses.py b/netbox_librenms_plugin/tables/ipaddresses.py index 0324c6de22..2416984631 100644 --- a/netbox_librenms_plugin/tables/ipaddresses.py +++ b/netbox_librenms_plugin/tables/ipaddresses.py @@ -14,6 +14,17 @@ class IPAddressTable(tables.Table): def __init__(self, *args, **kwargs): """Initialize IP address table.""" super().__init__(*args, **kwargs) + # Identify the owning sync tab so the paginator links (inc/paginator.html builds + # ?tab={{ table.tab }}) keep the user on the IP Addresses tab. Without this, table.tab + # renders empty and paging falls back to the default (Interfaces) tab. + self.tab = "ipaddresses" + # Give this table its own pagination namespace. configure() passes self.prefix to + # get_table_paginate_count() (and RequestConfig), which keys per_page on + # "{prefix}per_page"; left empty, the IP table shares the generic per-page param with + # other tabs instead of reading "ipaddresses_per_page". Orthogonal to self.tab. Matches + # the cables/modules/interfaces/vlans tables; preserve an explicit caller override. + if not self.prefix: + self.prefix = "ipaddresses_" class Meta: """Meta options for IPAddressTable.""" diff --git a/netbox_librenms_plugin/tables/modules.py b/netbox_librenms_plugin/tables/modules.py index 318c73b866..46e618ab03 100644 --- a/netbox_librenms_plugin/tables/modules.py +++ b/netbox_librenms_plugin/tables/modules.py @@ -7,6 +7,7 @@ from netbox.tables.columns import ToggleColumn from utilities.paginator import EnhancedPaginator +from netbox_librenms_plugin.constants import OOB_BADGE_HTML from netbox_librenms_plugin.utils import get_table_paginate_count @@ -181,16 +182,26 @@ def render_name(self, value, record): rendered_name = display_name depth = record.get("depth", 0) + # Static trusted markup — use mark_safe, not format_html (which requires + # interpolation args and raises TypeError when given a bare string). + oob_badge = ( + mark_safe(OOB_BADGE_HTML) # noqa: S308 + if record.get("_source") == "oob" + else "" + ) if depth == 0: - return rendered_name + return format_html("{}{}", rendered_name, oob_badge) # Build visual tree prefix based on nesting depth padding_px = depth * 20 prefix = "└─ " + # Keep the OOB badge inside the padded container so it stays indented + # with the module name on nested rows (was rendering at column 0). return format_html( - '{}{}', + '{}{}{}', padding_px, prefix, rendered_name, + oob_badge, ) def render_model(self, value, record): diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync.html index 1408f65f90..609389eb08 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync.html @@ -7,6 +7,8 @@

Cable Sync

{% csrf_token %} + {# Carry the tab's server_key so the refresh rebinds to it (mirrors _module_sync.html). #} + {% include "netbox_librenms_plugin/inc/_hidden_server_key.html" %} {% if has_librenms_id %} {% with model_name=object|meta:"model_name" %} {% if model_name == "device" %} diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.html index 485676360e..b3ed0ee40d 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.html @@ -3,16 +3,34 @@ {% if cable_sync.table %} +{# Migrated donors must not sync: hiding only the button leaves the POST form live (Enter in a filter still submits), so drop the form in migrated mode. #} +{% if migrated_to_marker %} +
+ {% comment %} + No POST form in migrated mode, but the cable table still renders interactive controls whose + verify-cable fetch (handleCableChange) reads document.querySelector('[name=csrfmiddlewaretoken]').value + and input[name="server_key"]. Emit standalone hidden inputs so those JS-driven requests don't hit a + null CSRF token (TypeError/403) or fall back to the default server on a non-default deployment. A bare + hidden input never auto-submits, so it doesn't reintroduce the live-form problem migrated mode avoids. + Mirrors _interface_sync_content.html. + {% endcomment %} + + {% if cable_sync.server_key %}{% endif %} +{% else %} + {# The sync POST's own inputs (token + selected_port) live inside the branch only. #} {% csrf_token %} {% if cable_sync.server_key %}{% endif %} +{% endif %}
+ {% if not migrated_to_marker %} + {% endif %} info @@ -70,7 +88,11 @@
+{% if migrated_to_marker %} +
+{% else %} +{% endif %} {% else %}
diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html index 7bb4aeca25..c6afd05224 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html @@ -7,8 +7,12 @@

Interface Sync

{% csrf_token %} - {# Carry the active server_key so the refresh hits the right LibreNMS server/cache (else a non-default server tab uses the fallback). #} - {% if server_key %}{% endif %} + {% comment %} + htmx includes the enclosing form's values on non-GET requests, so the refresh + POST carries the tab's server_key and the view rebinds to it — otherwise the + refresh fetches and caches under the GLOBAL selected server (mirrors _module_sync.html). + {% endcomment %} + {% include "netbox_librenms_plugin/inc/_hidden_server_key.html" %} {% if has_librenms_id %} {% with model_name=object|meta:"model_name" %} {% if model_name == "device" %} diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html index 5620026c9b..3dc4d8e594 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html @@ -4,23 +4,47 @@ {% if interface_sync.table %} +{% if interface_sync.oob_incomplete %} + +{% endif %} + +{# Migrated donors must not sync: hiding only the button leaves the POST form live (Enter in a filter still submits), so drop the form in migrated mode. #} {% with model_name=interface_sync.object|meta:"model_name" %} +{% if migrated_to_marker %} +
+ {% comment %} + No POST form in migrated mode, but the interface table still renders interactive + relationship/VC-member dropdowns whose verify-interface POST reads the token via + document.querySelector('[name=csrfmiddlewaretoken]'). Emit a standalone token so those + JS-driven requests don't hit a null token (TypeError/403). A bare hidden input never + auto-submits, so it doesn't reintroduce the live-form problem flagged above. + {% endcomment %} + +{% else %} - {% endwith %} + {# Form-only inputs live inside the form branch so migrated (donor) mode, which renders a plain
wrapper, never emits hidden fields outside any . #} {% csrf_token %} {% if interface_sync.server_key %}{% endif %} +{% endif %} + {% endwith %} {% block table_actions %}
+ {% if not migrated_to_marker %} + {% endif %} info
+ {% if not migrated_to_marker %}
Exclude from Sync:
@@ -61,6 +85,7 @@
Exclude from Sync:
+ {% endif %}
@@ -78,7 +103,7 @@
Exclude from Sync:
{% if interface_sync.netbox_only_interfaces %} + title="{% if migrated_to_marker %}Click to view and move NetBox-only interfaces{% else %}Click to view and delete NetBox-only interfaces{% endif %}"> {{interface_sync.netbox_only_interfaces|length}} NetBox only interfaces @@ -198,7 +223,11 @@
+{% if migrated_to_marker %} +
+{% else %} +{% endif %} {% else %}
@@ -291,9 +320,15 @@
+ {% endif %} + {% if validation.serial_action == 'link' or validation.serial_action == 'hostname_differs' %}
+ {% comment %} + Carry the server the modal was rendered for (mirrors the Add-as-OOB form above) so a + multi-server link/update runs against that server, not the session/default one -- otherwise + set_librenms_device_id writes the mapping under the wrong server namespace. + {% endcomment %} + {% include "netbox_librenms_plugin/inc/_hidden_server_key.html" %} {% if validation.device_type_mismatch %}
{% endif %} @@ -573,8 +724,39 @@
{% endif %}
+ {% endif %} {% elif validation.existing_match_type == 'primary_ip' %} + {# device_add_as_oob sets device.oob_ip (VMs lack it) — only offer OOB attach for real devices; a VM IP-match falls through to the info message below. #} + {% if validation.serial_action == 'oob_candidate' and existing_device_model_name != "virtualmachine" %} +
+ OOB Detected + + — Exists as + {{ validation.existing_device.name }} + (matched via IP {{ libre_device.ip }}). + +
+
+ + LibreNMS device {{ libre_device.sysName|default:libre_device.hostname }} + appears to be an OOB management controller + ({{ validation.oob_candidate.type }} + {% if validation.oob_candidate.ip %}— {{ validation.oob_candidate.ip }}{% endif %}). +
+
+
+ {% csrf_token %} + + {% include "netbox_librenms_plugin/inc/_hidden_server_key.html" %} + {% include "netbox_librenms_plugin/htmx/_oob_interface_select.html" %} + +
+
+ {% else %}
IP match @@ -583,6 +765,16 @@
Consider adding LibreNMS ID manually.
+ {% endif %} + + {% elif validation.existing_match_type == 'librenms_oob' %} +
+ OOB linked + + — LibreNMS ID {{ libre_device.device_id }} is already linked as the OOB controller for + {{ validation.existing_device.name }}. + +
{% else %}
@@ -639,8 +831,10 @@
class="btn btn-primary btn-sm" target="_blank" rel="noopener noreferrer"> View in NetBox - {% if validation.existing_match_type == 'librenms_id' %} - Full Sync Page @@ -651,3 +845,64 @@
Close
+ + diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_hidden_server_key.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_hidden_server_key.html new file mode 100644 index 0000000000..44d825e6b1 --- /dev/null +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_hidden_server_key.html @@ -0,0 +1,8 @@ +{% comment %} +Hidden server_key input shared by the sync-page action forms: carries the tab's +server scope on the POST so the handling view rebinds to it rather than the GLOBAL +selected server (with overlapping device_ids across servers, a fallback would fetch +and cache the WRONG server's device). Renders nothing when server_key is absent so +single-server deployments keep posting without it. +{% endcomment %} +{% if server_key %}{% endif %} diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_migrate_move_button.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_migrate_move_button.html new file mode 100644 index 0000000000..74fa06835c --- /dev/null +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_migrate_move_button.html @@ -0,0 +1,34 @@ +{% comment %} +Shared "Move to winner" button cell for the device-merge migration tables (interface + IP). + +The ``{% url ... as %}`` form does NOT raise NoReverseMatch: the move-to-winner routes are +registered by the device-merge feature further up the stack, so on branches without them +``move_url`` is '' and the live button degrades to read-only instead of 500ing the whole tab. + +Params (passed via {% include ... with %}): + move_url_name - the move-to-winner URL name (string) + obj_id - pk of the row object + obj_label - display value for the confirm text (interface.name / ip.address) + entity_word - "interface" or "IP" (drives confirm/title wording) + winner - the migrated_to_winner device + server_key - resolved server_key for the hx-vals scope (may be empty) + obj_interface_name - (IP only) the interface the IP is assigned to, for the title detail +``has_write_permission`` and ``csrf_token`` come from the parent context. +{% endcomment %} +{% url move_url_name pk=obj_id as move_url %} +{% if has_write_permission and winner and move_url %} + +{% elif winner %} +read-only +{% else %} +winner missing +{% endif %} diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html index adba2892d1..73334f6717 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html @@ -35,6 +35,77 @@ {% block content %} +{% if migrated_to_marker %} + +{% endif %} + {% if all_server_mappings %}
@@ -76,6 +147,9 @@ {% else %} ID {{ mapping.device_id }} {% endif %} + {% if mapping.is_oob_only %} + OOB only + {% endif %} {% if not mapping.is_configured %} @@ -130,8 +204,10 @@
{% endif %} - -{% if not is_vc_member or object.pk == librenms_sync_device.pk %} + +{% if not migrated_to_marker and not is_vc_member or not migrated_to_marker and object.pk == librenms_sync_device.pk %}
@@ -172,6 +248,12 @@
{% csrf_token %} + {% comment %} + Scope the conversion to the tab's server: the view rebinds from the POSTed + server_key, so omitting it would verify and write the mapping under the + GLOBAL selected server, not the ?server_key tab the user is acting on. + {% endcomment %} + {% include "netbox_librenms_plugin/inc/_hidden_server_key.html" %} {% if librenms_id_serial_confirmed %}
action="{% url 'plugins:netbox_librenms_plugin:update_device_name' pk=object.pk %}" style="display: inline;"> {% csrf_token %} + {% comment %} + The view rebinds from the POSTed server_key; without it the sync runs + against the global selected server, not this tab's server. + {% endcomment %} + {% include "netbox_librenms_plugin/inc/_hidden_server_key.html" %}