From 7933312374915f54f3dd8577d095fe340c30e9c6 Mon Sep 17 00:00:00 2001 From: Andy Norwood Date: Thu, 12 Mar 2026 10:23:02 +0000 Subject: [PATCH 01/22] fix: use get_librenms_sync_device() in verify views to prevent VC crash Replace inline VC resolution logic in SingleCableVerifyView and SingleInterfaceVerifyView with the centralized get_librenms_sync_device() utility. The inline logic only checked primary_ip, causing a 500 error (AttributeError: 'NoneType' object has no attribute '_meta') when no VC member has a primary_ip set. get_librenms_sync_device() handles this by also checking librenms_id and falling back to lowest vc_position. Added None guards for the edge case where no sync device can be resolved at all. Includes 8 new tests covering both views' VC resolution paths. --- .../tests/test_verify_views.py | 204 ++++++++++++++++++ .../views/base/cables_view.py | 13 +- .../views/object_sync/devices.py | 12 +- 3 files changed, 216 insertions(+), 13 deletions(-) create mode 100644 netbox_librenms_plugin/tests/test_verify_views.py diff --git a/netbox_librenms_plugin/tests/test_verify_views.py b/netbox_librenms_plugin/tests/test_verify_views.py new file mode 100644 index 0000000000..865a25e246 --- /dev/null +++ b/netbox_librenms_plugin/tests/test_verify_views.py @@ -0,0 +1,204 @@ +"""Tests for SingleCableVerifyView and SingleInterfaceVerifyView VC resolution. + +Covers the fix for the NoneType crash when a VC device has no primary_ip +on any member — both views must use get_librenms_sync_device() and guard +against None. +""" + +import json +from unittest.mock import MagicMock, patch + + +def _make_request(body: dict) -> MagicMock: + """Create a mock POST request with JSON body.""" + request = MagicMock() + request.body = json.dumps(body).encode() + request.user.has_perm.return_value = True + return request + + +def _make_device(pk=1, has_vc=False, name="test-device"): + """Create a mock Device with optional virtual_chassis.""" + device = MagicMock() + device.pk = pk + device.id = pk + device.name = name + device._meta.model_name = "device" + device.virtual_chassis = MagicMock() if has_vc else None + device.interfaces.filter.return_value.first.return_value = None + return device + + +# --------------------------------------------------------------------------- +# SingleCableVerifyView +# --------------------------------------------------------------------------- +class TestSingleCableVerifyView: + """SingleCableVerifyView.post() VC resolution and None guard.""" + + def _make_view(self): + from netbox_librenms_plugin.views.base.cables_view import SingleCableVerifyView + + view = object.__new__(SingleCableVerifyView) + view._librenms_api = MagicMock() + return view + + @patch("netbox_librenms_plugin.views.base.cables_view.get_object_or_404") + @patch("netbox_librenms_plugin.views.base.cables_view.get_librenms_sync_device") + @patch("netbox_librenms_plugin.views.base.cables_view.cache") + def test_vc_device_no_primary_ip_returns_empty_row(self, mock_cache, mock_sync, mock_get_obj): + """VC with no primary_ip: get_librenms_sync_device returns None → empty row, no crash.""" + device = _make_device(pk=1, has_vc=True) + mock_get_obj.return_value = device + mock_sync.return_value = None + + view = self._make_view() + request = _make_request({"device_id": 1, "local_port_id": "42"}) + response = view.post(request) + + data = json.loads(response.content) + assert data["status"] == "success" + assert data["formatted_row"]["cable_status"] == "Missing Ports" + mock_cache.get.assert_not_called() + + @patch("netbox_librenms_plugin.views.base.cables_view.get_object_or_404") + @patch("netbox_librenms_plugin.views.base.cables_view.get_librenms_sync_device") + @patch("netbox_librenms_plugin.views.base.cables_view.cache") + def test_vc_device_with_sync_device_uses_cache(self, mock_cache, mock_sync, mock_get_obj): + """VC with valid sync device: cache is queried with the sync device's key.""" + device = _make_device(pk=1, has_vc=True) + sync_device = _make_device(pk=2, name="sync-device") + mock_get_obj.return_value = device + mock_sync.return_value = sync_device + mock_cache.get.return_value = None # No cached data + + view = self._make_view() + request = _make_request({"device_id": 1, "local_port_id": "42"}) + view.post(request) + + mock_sync.assert_called_once_with(device) + mock_cache.get.assert_called_once() + cache_key = mock_cache.get.call_args[0][0] + assert "device" in cache_key + assert "2" in cache_key + + @patch("netbox_librenms_plugin.views.base.cables_view.get_object_or_404") + @patch("netbox_librenms_plugin.views.base.cables_view.get_librenms_sync_device") + @patch("netbox_librenms_plugin.views.base.cables_view.cache") + def test_non_vc_device_skips_sync_device_lookup(self, mock_cache, mock_sync, mock_get_obj): + """Non-VC device: get_librenms_sync_device is NOT called.""" + device = _make_device(pk=5, has_vc=False) + mock_get_obj.return_value = device + mock_cache.get.return_value = None + + view = self._make_view() + request = _make_request({"device_id": 5, "local_port_id": "10"}) + view.post(request) + + mock_sync.assert_not_called() + mock_cache.get.assert_called_once() + + def test_no_device_id_returns_empty_row(self): + """Missing device_id: returns default empty formatted_row.""" + view = self._make_view() + request = _make_request({"local_port_id": "42"}) + response = view.post(request) + + data = json.loads(response.content) + assert data["status"] == "success" + assert data["formatted_row"]["cable_status"] == "Missing Ports" + + +# --------------------------------------------------------------------------- +# SingleInterfaceVerifyView +# --------------------------------------------------------------------------- +class TestSingleInterfaceVerifyView: + """SingleInterfaceVerifyView.post() VC resolution and None guard.""" + + def _make_view(self): + from netbox_librenms_plugin.views.object_sync.devices import SingleInterfaceVerifyView + + view = object.__new__(SingleInterfaceVerifyView) + return view + + @patch("netbox_librenms_plugin.views.object_sync.devices.get_object_or_404") + @patch("netbox_librenms_plugin.views.object_sync.devices.get_librenms_sync_device") + @patch("netbox_librenms_plugin.views.object_sync.devices.cache") + def test_vc_device_no_sync_device_returns_404(self, mock_cache, mock_sync, mock_get_obj): + """VC with no sync device: returns 404 JSON error, no crash.""" + device = _make_device(pk=1, has_vc=True) + mock_get_obj.return_value = device + mock_sync.return_value = None + + view = self._make_view() + request = _make_request( + { + "device_id": 1, + "interface_name": "eth0", + "interface_name_field": "ifName", + } + ) + response = view.post(request) + + assert response.status_code == 404 + data = json.loads(response.content) + assert data["status"] == "error" + assert "sync device" in data["message"].lower() + mock_cache.get.assert_not_called() + + @patch("netbox_librenms_plugin.views.object_sync.devices.get_object_or_404") + @patch("netbox_librenms_plugin.views.object_sync.devices.get_librenms_sync_device") + @patch("netbox_librenms_plugin.views.object_sync.devices.cache") + def test_vc_device_with_sync_device_uses_cache(self, mock_cache, mock_sync, mock_get_obj): + """VC with valid sync device: cache is queried with the sync device's key.""" + device = _make_device(pk=1, has_vc=True) + sync_device = _make_device(pk=3, name="sync-member") + mock_get_obj.return_value = device + mock_sync.return_value = sync_device + mock_cache.get.return_value = None + + view = self._make_view() + request = _make_request( + { + "device_id": 1, + "interface_name": "eth0", + "interface_name_field": "ifName", + } + ) + view.post(request) + + mock_sync.assert_called_once_with(device) + mock_cache.get.assert_called_once() + cache_key = mock_cache.get.call_args[0][0] + assert "3" in cache_key + + @patch("netbox_librenms_plugin.views.object_sync.devices.get_object_or_404") + @patch("netbox_librenms_plugin.views.object_sync.devices.get_librenms_sync_device") + @patch("netbox_librenms_plugin.views.object_sync.devices.cache") + def test_non_vc_device_skips_sync_device_lookup(self, mock_cache, mock_sync, mock_get_obj): + """Non-VC device: get_librenms_sync_device is NOT called.""" + device = _make_device(pk=5, has_vc=False) + mock_get_obj.return_value = device + mock_cache.get.return_value = None + + view = self._make_view() + request = _make_request( + { + "device_id": 5, + "interface_name": "eth0", + "interface_name_field": "ifName", + } + ) + view.post(request) + + mock_sync.assert_not_called() + mock_cache.get.assert_called_once() + + def test_no_device_id_returns_400(self): + """Missing device_id: returns 400 error.""" + view = self._make_view() + request = _make_request({"interface_name": "eth0"}) + response = view.post(request) + + assert response.status_code == 400 + data = json.loads(response.content) + assert data["status"] == "error" diff --git a/netbox_librenms_plugin/views/base/cables_view.py b/netbox_librenms_plugin/views/base/cables_view.py index cff041c64e..f09a62561e 100644 --- a/netbox_librenms_plugin/views/base/cables_view.py +++ b/netbox_librenms_plugin/views/base/cables_view.py @@ -14,6 +14,7 @@ from netbox_librenms_plugin.utils import ( get_interface_name_field, + get_librenms_sync_device, get_virtual_chassis_member, ) from netbox_librenms_plugin.views.mixins import CacheMixin, LibreNMSAPIMixin, LibreNMSPermissionMixin @@ -363,17 +364,15 @@ def post(self, request): if selected_device_id: selected_device = get_object_or_404(Device, pk=selected_device_id) - # Get the primary device (master or first with IP) if part of virtual chassis + # Resolve the VC device that holds cached LibreNMS data if selected_device.virtual_chassis: - primary_device = selected_device.virtual_chassis.master - if not primary_device or not primary_device.primary_ip: - primary_device = next( - (member for member in selected_device.virtual_chassis.members.all() if member.primary_ip), - None, - ) + primary_device = get_librenms_sync_device(selected_device) else: primary_device = selected_device + if not primary_device: + return JsonResponse({"status": "success", "formatted_row": formatted_row}) + cached_links = cache.get(self.get_cache_key(primary_device, "links")) if cached_links: diff --git a/netbox_librenms_plugin/views/object_sync/devices.py b/netbox_librenms_plugin/views/object_sync/devices.py index 97584f8319..5859cc4a67 100644 --- a/netbox_librenms_plugin/views/object_sync/devices.py +++ b/netbox_librenms_plugin/views/object_sync/devices.py @@ -19,6 +19,7 @@ ) from netbox_librenms_plugin.utils import ( get_interface_name_field, + get_librenms_sync_device, get_missing_vlan_warning, get_tagged_vlan_css_class, get_untagged_vlan_css_class, @@ -106,16 +107,15 @@ def post(self, request): selected_device = get_object_or_404(Device, pk=selected_device_id) + # Resolve the VC device that holds cached LibreNMS data if selected_device.virtual_chassis: - primary_device = selected_device.virtual_chassis.master - if not primary_device or not primary_device.primary_ip: - primary_device = next( - (member for member in selected_device.virtual_chassis.members.all() if member.primary_ip), - None, - ) + primary_device = get_librenms_sync_device(selected_device) else: primary_device = selected_device + if not primary_device: + return JsonResponse({"status": "error", "message": "No sync device found for virtual chassis"}, status=404) + cached_data = cache.get(self.get_cache_key(primary_device, "ports")) if cached_data: From f5b9dedf9d8cbef58bc6f5f763d5868298aa0d84 Mon Sep 17 00:00:00 2001 From: Andy Norwood Date: Thu, 12 Mar 2026 13:17:46 +0000 Subject: [PATCH 02/22] =?UTF-8?q?refactor:=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20fix=20test=20docstrings,=20use=20conftest=20fixture?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename tests to reflect actual None scenario (no resolvable sync device, not 'no primary_ip') per reviewer feedback on comments 1 & 2 - Replace _make_device() with _make_vc_device() for VC-only cases; use mock_netbox_device conftest fixture for non-VC test cases per comment 4 --- .../tests/test_verify_views.py | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/netbox_librenms_plugin/tests/test_verify_views.py b/netbox_librenms_plugin/tests/test_verify_views.py index 865a25e246..4007f04f76 100644 --- a/netbox_librenms_plugin/tests/test_verify_views.py +++ b/netbox_librenms_plugin/tests/test_verify_views.py @@ -1,8 +1,8 @@ """Tests for SingleCableVerifyView and SingleInterfaceVerifyView VC resolution. -Covers the fix for the NoneType crash when a VC device has no primary_ip -on any member — both views must use get_librenms_sync_device() and guard -against None. +Verifies that both views delegate VC device resolution to +get_librenms_sync_device() and handle the None return gracefully +(e.g. empty VC members or vc_position type errors). """ import json @@ -17,14 +17,14 @@ def _make_request(body: dict) -> MagicMock: return request -def _make_device(pk=1, has_vc=False, name="test-device"): - """Create a mock Device with optional virtual_chassis.""" +def _make_vc_device(pk=1, name="vc-device"): + """Create a mock Device that belongs to a virtual chassis.""" device = MagicMock() device.pk = pk device.id = pk device.name = name device._meta.model_name = "device" - device.virtual_chassis = MagicMock() if has_vc else None + device.virtual_chassis = MagicMock() device.interfaces.filter.return_value.first.return_value = None return device @@ -45,9 +45,9 @@ def _make_view(self): @patch("netbox_librenms_plugin.views.base.cables_view.get_object_or_404") @patch("netbox_librenms_plugin.views.base.cables_view.get_librenms_sync_device") @patch("netbox_librenms_plugin.views.base.cables_view.cache") - def test_vc_device_no_primary_ip_returns_empty_row(self, mock_cache, mock_sync, mock_get_obj): - """VC with no primary_ip: get_librenms_sync_device returns None → empty row, no crash.""" - device = _make_device(pk=1, has_vc=True) + def test_vc_no_resolvable_sync_device_returns_empty_row(self, mock_cache, mock_sync, mock_get_obj): + """VC where get_librenms_sync_device returns None → empty row, no crash.""" + device = _make_vc_device(pk=1) mock_get_obj.return_value = device mock_sync.return_value = None @@ -63,10 +63,10 @@ def test_vc_device_no_primary_ip_returns_empty_row(self, mock_cache, mock_sync, @patch("netbox_librenms_plugin.views.base.cables_view.get_object_or_404") @patch("netbox_librenms_plugin.views.base.cables_view.get_librenms_sync_device") @patch("netbox_librenms_plugin.views.base.cables_view.cache") - def test_vc_device_with_sync_device_uses_cache(self, mock_cache, mock_sync, mock_get_obj): - """VC with valid sync device: cache is queried with the sync device's key.""" - device = _make_device(pk=1, has_vc=True) - sync_device = _make_device(pk=2, name="sync-device") + def test_vc_resolved_sync_device_uses_cache(self, mock_cache, mock_sync, mock_get_obj): + """VC with resolved sync device: cache is queried with that device's key.""" + device = _make_vc_device(pk=1) + sync_device = _make_vc_device(pk=2, name="sync-device") mock_get_obj.return_value = device mock_sync.return_value = sync_device mock_cache.get.return_value = None # No cached data @@ -84,10 +84,10 @@ def test_vc_device_with_sync_device_uses_cache(self, mock_cache, mock_sync, mock @patch("netbox_librenms_plugin.views.base.cables_view.get_object_or_404") @patch("netbox_librenms_plugin.views.base.cables_view.get_librenms_sync_device") @patch("netbox_librenms_plugin.views.base.cables_view.cache") - def test_non_vc_device_skips_sync_device_lookup(self, mock_cache, mock_sync, mock_get_obj): + def test_non_vc_device_skips_sync_device_lookup(self, mock_cache, mock_sync, mock_get_obj, mock_netbox_device): """Non-VC device: get_librenms_sync_device is NOT called.""" - device = _make_device(pk=5, has_vc=False) - mock_get_obj.return_value = device + mock_netbox_device.virtual_chassis = None + mock_get_obj.return_value = mock_netbox_device mock_cache.get.return_value = None view = self._make_view() @@ -123,9 +123,9 @@ def _make_view(self): @patch("netbox_librenms_plugin.views.object_sync.devices.get_object_or_404") @patch("netbox_librenms_plugin.views.object_sync.devices.get_librenms_sync_device") @patch("netbox_librenms_plugin.views.object_sync.devices.cache") - def test_vc_device_no_sync_device_returns_404(self, mock_cache, mock_sync, mock_get_obj): - """VC with no sync device: returns 404 JSON error, no crash.""" - device = _make_device(pk=1, has_vc=True) + def test_vc_no_resolvable_sync_device_returns_404(self, mock_cache, mock_sync, mock_get_obj): + """VC where get_librenms_sync_device returns None → 404 JSON error, no crash.""" + device = _make_vc_device(pk=1) mock_get_obj.return_value = device mock_sync.return_value = None @@ -148,10 +148,10 @@ def test_vc_device_no_sync_device_returns_404(self, mock_cache, mock_sync, mock_ @patch("netbox_librenms_plugin.views.object_sync.devices.get_object_or_404") @patch("netbox_librenms_plugin.views.object_sync.devices.get_librenms_sync_device") @patch("netbox_librenms_plugin.views.object_sync.devices.cache") - def test_vc_device_with_sync_device_uses_cache(self, mock_cache, mock_sync, mock_get_obj): - """VC with valid sync device: cache is queried with the sync device's key.""" - device = _make_device(pk=1, has_vc=True) - sync_device = _make_device(pk=3, name="sync-member") + def test_vc_resolved_sync_device_uses_cache(self, mock_cache, mock_sync, mock_get_obj): + """VC with resolved sync device: cache is queried with that device's key.""" + device = _make_vc_device(pk=1) + sync_device = _make_vc_device(pk=3, name="sync-member") mock_get_obj.return_value = device mock_sync.return_value = sync_device mock_cache.get.return_value = None @@ -174,10 +174,10 @@ def test_vc_device_with_sync_device_uses_cache(self, mock_cache, mock_sync, mock @patch("netbox_librenms_plugin.views.object_sync.devices.get_object_or_404") @patch("netbox_librenms_plugin.views.object_sync.devices.get_librenms_sync_device") @patch("netbox_librenms_plugin.views.object_sync.devices.cache") - def test_non_vc_device_skips_sync_device_lookup(self, mock_cache, mock_sync, mock_get_obj): + def test_non_vc_device_skips_sync_device_lookup(self, mock_cache, mock_sync, mock_get_obj, mock_netbox_device): """Non-VC device: get_librenms_sync_device is NOT called.""" - device = _make_device(pk=5, has_vc=False) - mock_get_obj.return_value = device + mock_netbox_device.virtual_chassis = None + mock_get_obj.return_value = mock_netbox_device mock_cache.get.return_value = None view = self._make_view() From 232f37d9fbdfc8cba665258a6bffb49930c8d26c Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Thu, 12 Mar 2026 08:18:36 +0000 Subject: [PATCH 03/22] feat(multi-server): JSON custom field librenms_id with server management and CR fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Multi-server librenms_id as JSON custom field (server_key → device_id map) - Server management UI, migration of legacy integer IDs - CR fixes: int conversion guard, VLAN dict guard, VC domain with server_key, forms has_option_only, JS server_key selector, _cancelled flag in bulk import, cache sort_keys, role recalculate, XSS escape, bool guards - Update existing tests for multi-server behavior - test_librenms_id.py: tests for new JSON CF format and migration --- .devcontainer/README.md | 23 +- .github/pull_request_template.md | 2 +- .pre-commit-config.yaml | 6 +- docs/development/testing.md | 23 +- docs/usage_tips/custom_field.md | 10 +- netbox_librenms_plugin/forms.py | 60 +- .../import_utils/__init__.py | 1 + .../import_utils/bulk_import.py | 321 +- netbox_librenms_plugin/import_utils/cache.py | 84 +- .../import_utils/device_operations.py | 114 +- .../import_utils/filters.py | 57 +- .../import_utils/virtual_chassis.py | 198 +- .../import_utils/vm_operations.py | 96 +- netbox_librenms_plugin/jobs.py | 31 +- netbox_librenms_plugin/librenms_api.py | 169 +- .../js/librenms_sync.js | 295 +- .../tables/device_status.py | 6 +- netbox_librenms_plugin/tables/interfaces.py | 10 +- netbox_librenms_plugin/tables/ipaddresses.py | 6 +- netbox_librenms_plugin/tables/vlans.py | 40 +- .../_cable_sync_content.html | 1 + .../_interface_sync_content.html | 1 + .../_ipaddress_sync_content.html | 1 + .../_vlan_sync_content.html | 1 + .../htmx/device_validation_details.html | 59 +- .../librenms_sync_base.html | 95 +- netbox_librenms_plugin/tests/conftest.py | 53 +- .../tests/mock_librenms_server.py | 40 + .../tests/test_background_jobs.py | 15 +- .../tests/test_import_utils.py | 3721 +++++++++++++++-- .../tests/test_integration_sync.py | 48 + .../tests/test_librenms_api.py | 123 +- .../tests/test_librenms_id.py | 505 +++ netbox_librenms_plugin/tests/test_mixins.py | 53 + .../tests/test_permissions.py | 134 +- .../tests/test_sync_devices.py | 107 + .../tests/test_sync_view_mismatch.py | 259 +- netbox_librenms_plugin/tests/test_utils.py | 197 +- .../tests/test_view_wiring.py | 174 +- .../tests/test_vlan_sync.py | 22 + netbox_librenms_plugin/urls.py | 12 + netbox_librenms_plugin/utils.py | 273 +- netbox_librenms_plugin/views/__init__.py | 2 + .../views/base/cables_view.py | 148 +- .../views/base/interfaces_view.py | 27 +- .../views/base/ip_addresses_view.py | 54 +- .../views/base/librenms_sync_view.py | 128 +- .../views/base/vlan_table_view.py | 16 +- .../views/imports/actions.py | 418 +- netbox_librenms_plugin/views/imports/list.py | 8 +- netbox_librenms_plugin/views/mixins.py | 37 +- .../views/object_sync/devices.py | 46 +- .../views/object_sync/vms.py | 16 +- netbox_librenms_plugin/views/sync/cables.py | 58 +- .../views/sync/device_fields.py | 239 +- netbox_librenms_plugin/views/sync/devices.py | 15 +- .../views/sync/interfaces.py | 40 +- .../views/sync/ip_addresses.py | 21 +- netbox_librenms_plugin/views/sync/vlans.py | 20 +- pyproject.toml | 6 + uv.lock | 8 + 61 files changed, 7564 insertions(+), 1189 deletions(-) create mode 100644 netbox_librenms_plugin/tests/test_librenms_id.py create mode 100644 uv.lock diff --git a/.devcontainer/README.md b/.devcontainer/README.md index 3560a93af3..7dc3ef967a 100644 --- a/.devcontainer/README.md +++ b/.devcontainer/README.md @@ -44,20 +44,20 @@ If you need to test with a LibreNMS instance on a private network (local lab, co 1. Fork and Clone: fork the plugin repo in Github and clone locally 2. Open in VS Code and choose "Reopen in Container" (or Ctrl+Shift+P → Dev Containers: Reopen in Container) -2. Wait for setup (~5min on first run or when new NetBox image is used). The container will install the plugin and prep NetBox -3. Set up GitHub access: `gh auth login` (for pushing/pulling code changes) -4. Create your plugin config — see [Plugin configuration](#plugin-configuration): +3. Wait for setup (~5min on first run or when new NetBox image is used). The container will install the plugin and prep NetBox +4. Set up GitHub access: `gh auth login` (for pushing/pulling code changes) +5. Create your plugin config — see [LibreNMS Server configuration](#librenms-server-configuration): - `cp .devcontainer/config/plugin-config.py.example .devcontainer/config/plugin-config.py` - Edit it with your server details (tokens/URLs) -5. Start NetBox with `netbox-run` (or `netbox-run-bg` in background) (see [Commands](#-commands-aliases)) -6. Access NetBox at http://localhost:8000 +6. Start NetBox with `netbox-run` (or `netbox-run-bg` in background) (see [Commands](#-commands-aliases)) +7. Access NetBox at http://localhost:8000 - Username: `admin` - Password: `admin` ### 🔄 Code changes and Committing 8. Edit code in the repo root. Check out [contributing docs](../docs/contributing.md) 9. Use `netbox-logs` to follow log output on screen -6. Commit changes and contribute as normal by submitting a PR on GitHub. +10. Commit changes and contribute as normal by submitting a PR on GitHub. ### Quick Tips - **Auto-reload**: Works for most code changes when `DEBUG=True` @@ -241,7 +241,7 @@ After any `.env` change, rebuild the dev container to apply environment updates. ### Additional packages (including other netbox plugins) - Create `.devcontainer/extra-requirements.txt` for extra Python packages. Example: `.devcontainer/extra-requirements.txt.example`. - - After changes: run `plugins-install` to install packages, then `netbox-restart` (see [Commands](#-commands-aliases)) + - After changes: run `plugin-install` to install packages, then `netbox-restart` (see [Commands](#-commands-aliases)) ## 🔧 Git Setup @@ -259,7 +259,7 @@ The dev container includes Git and GitHub CLI pre-installed. You'll need to conf git remote -v # If it shows git@github.com:..., convert to HTTPS -git remote set-url origin https://github.com/bonzo81/netbox-librenms-plugin.git +git remote set-url origin https://github.com//netbox-librenms-plugin.git ``` ### Recommended: GitHub CLI (Easiest) @@ -347,13 +347,14 @@ fatal: Could not read from remote repository. 1. **Check remote URL** - should use HTTPS, not SSH: ```bash git remote -v - # Should show: https://github.com/bonzo81/netbox-librenms-plugin.git - # NOT: git@github.com:bonzo81/netbox-librenms-plugin.git + # Should show: https://github.com//netbox-librenms-plugin.git + # NOT: git@github.com:/netbox-librenms-plugin.git ``` 2. **Fix SSH remote URL**: + ```bash - git remote set-url origin https://github.com/bonzo81/netbox-librenms-plugin.git + git remote set-url origin https://github.com//netbox-librenms-plugin.git ``` 3. **Authenticate with GitHub CLI**: diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index b641e68a71..99ff02374f 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -36,7 +36,7 @@ Delete items that don’t apply and describe briefly. 3. ## Risk Assessment -- Does this change affect existing users? +- Does this change affect existing users? - Could this cause unintended imports / updates? Explain briefly. diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6669696bbb..7346e658ed 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,6 +1,6 @@ repos: - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.15.4 # Use the latest version from https://github.com/astral-sh/ruff-pre-commit/releases + rev: v0.15.5 # Use the latest version from https://github.com/astral-sh/ruff-pre-commit/releases hooks: # Run the linter - id: ruff-check @@ -15,5 +15,9 @@ repos: - id: end-of-file-fixer - id: check-yaml exclude: ^mkdocs\.yml$ + - id: check-yaml + name: check-yaml (mkdocs.yml --unsafe) + args: [--unsafe] + files: ^mkdocs\.yml$ - id: check-added-large-files - id: check-merge-conflict diff --git a/docs/development/testing.md b/docs/development/testing.md index 05d27bd4ed..58f041c09e 100644 --- a/docs/development/testing.md +++ b/docs/development/testing.md @@ -35,8 +35,18 @@ The test suite covers all major plugin functionality. Tests are organized by the | [test_sync_interfaces.py](../../netbox_librenms_plugin/tests/test_sync_interfaces.py) | Interface sync—port matching, attribute updates, MAC handling, librenms_id assignment | | [test_virtual_chassis.py](../../netbox_librenms_plugin/tests/test_virtual_chassis.py) | Virtual chassis detection—VC member naming patterns and name generation | | [test_sync_view_mismatch.py](../../netbox_librenms_plugin/tests/test_sync_view_mismatch.py) | Sync page context—device type mismatch detection and badge rendering | +| [test_coverage_device_fields.py](../../netbox_librenms_plugin/tests/test_coverage_device_fields.py) | Device field sync view—field update logic and device field mapping | +| [test_coverage_list.py](../../netbox_librenms_plugin/tests/test_coverage_list.py) | Import list view—background job decision, job result loading, and GET handler | +| [test_coverage_api.py](../../netbox_librenms_plugin/tests/test_coverage_api.py) | LibreNMS API client—malformed payload guards, error paths, and edge cases | +| [test_coverage_sync_view.py](../../netbox_librenms_plugin/tests/test_coverage_sync_view.py) | Sync view base class—context preparation and tab rendering | +| [test_coverage_filters.py](../../netbox_librenms_plugin/tests/test_coverage_filters.py) | Import filter logic—filter form processing and device count helpers | +| [test_sync_modules.py](../../netbox_librenms_plugin/tests/test_sync_modules.py) | Module sync—inventory matching, module type resolution, and normalization rules | +| [test_modules_view.py](../../netbox_librenms_plugin/tests/test_modules_view.py) | Module sync view—context preparation, table rendering, and module bay mapping | +| [test_tables_modules.py](../../netbox_librenms_plugin/tests/test_tables_modules.py) | Module tables—column rendering, row formatting, and action buttons | | [test_permissions.py](../../netbox_librenms_plugin/tests/test_permissions.py) | Permission enforcement—mixin contracts, object-level permissions, and write guards | +| [test_vm_operations.py](../../netbox_librenms_plugin/tests/test_vm_operations.py) | VM operations—virtual machine sync, interface handling, and VM-specific views | | [test_integration_sync.py](../../netbox_librenms_plugin/tests/test_integration_sync.py) | Integration tests—API client against local mock HTTP server | +| [test_integration_virtual_chassis.py](../../netbox_librenms_plugin/tests/test_integration_virtual_chassis.py) | Integration tests—VC detection, negative cache, multi-server cache isolation | | [test_view_wiring.py](../../netbox_librenms_plugin/tests/test_view_wiring.py) | Smoke tests—view class MRO, mixin wiring, permission contracts, and template syntax | Supporting files: @@ -77,15 +87,15 @@ pytest netbox_librenms_plugin/tests/test_background_jobs.py -v # Multi-server librenms_id tests pytest netbox_librenms_plugin/tests/test_librenms_id.py -v -# Sync view tests (devices, interfaces) -pytest netbox_librenms_plugin/tests/test_sync_devices.py netbox_librenms_plugin/tests/test_sync_interfaces.py -v - -# Sync view mismatch detection and permission enforcement -pytest netbox_librenms_plugin/tests/test_sync_view_mismatch.py netbox_librenms_plugin/tests/test_permissions.py -v +# Sync view tests (devices, interfaces, modules) +pytest netbox_librenms_plugin/tests/test_sync_devices.py netbox_librenms_plugin/tests/test_sync_interfaces.py netbox_librenms_plugin/tests/test_sync_modules.py -v # Integration tests (API client against mock HTTP server) pytest netbox_librenms_plugin/tests/test_integration_sync.py -v +# Sync view mismatch detection and permission enforcement +pytest netbox_librenms_plugin/tests/test_sync_view_mismatch.py netbox_librenms_plugin/tests/test_permissions.py -v + # View wiring and template syntax smoke tests pytest netbox_librenms_plugin/tests/test_view_wiring.py -v ``` @@ -111,9 +121,10 @@ pytest netbox_librenms_plugin/tests/ -v --lf The test suite prioritizes speed and isolation so you can run tests frequently during development: - **Mock-based**: Unit tests use `MagicMock` instead of real database objects. No Django database setup required. -- **Fast execution**: The full suite runs in under 0.5 seconds. +- **Fast execution**: The full suite runs in approximately 15-20 seconds (varies by environment). - **Isolated**: Each test is independent with no shared state between tests. - **No external network access**: Tests never call external services. Integration tests use a local loopback HTTP server (`mock_librenms_server.py`) to exercise the real API client against realistic HTTP responses without requiring a running LibreNMS instance. +- **Coverage exclusions**: Test files themselves are excluded from coverage reports (see `[tool.coverage.run]` omit list in `pyproject.toml`). This approach means tests work identically in your local development environment, in the devcontainer, and in CI pipelines. diff --git a/docs/usage_tips/custom_field.md b/docs/usage_tips/custom_field.md index 032812ed82..0127d19efc 100644 --- a/docs/usage_tips/custom_field.md +++ b/docs/usage_tips/custom_field.md @@ -38,7 +38,15 @@ Follow these steps to create the `librenms_id` custom field in NetBox: - **Name:** `librenms_id` - **Label:** `LibreNMS ID` - **Description:** (Optional) Add a description like "LibreNMS Device ID for synchronization". - - **Type:** Integer + - **Type:** JSON (object) — stores a per-server mapping. + - Multi-server example: + ```json + {"production": 42, "staging": 17} + ``` + - Legacy single-server example (integer): + ``` + 42 + ``` - **Required:** Leave unchecked (optional). - **Default Value:** Leave blank. diff --git a/netbox_librenms_plugin/forms.py b/netbox_librenms_plugin/forms.py index f3e3d075e5..42c693a7bd 100644 --- a/netbox_librenms_plugin/forms.py +++ b/netbox_librenms_plugin/forms.py @@ -4,6 +4,7 @@ from dcim.choices import InterfaceTypeChoices from dcim.models import Device, DeviceRole, DeviceType, Location, Rack, Site from django import forms +from django.db.models import Case, IntegerField, Value, When from django.http import QueryDict from django.utils.translation import gettext_lazy as _ from netbox.forms import ( @@ -60,8 +61,8 @@ def _get_librenms_poller_group_choices(): api = LibreNMSAPI() success, poller_groups = api.get_poller_groups() - if success and poller_groups: - for group in poller_groups: + if success: + for group in poller_groups or []: group_id = str(group.get("id", "")) group_name = group.get("group_name", "") group_descr = group.get("descr", "") @@ -545,8 +546,21 @@ def __init__(self, *args, **kwargs): ] has_filters = any(data.get(field) for field in filter_fields) - # Apply default only on initial load (no filters, no job_id) - if "use_background_job" not in data and not data.get("job_id") and not has_filters: + # Option-only fields that don't constitute a real search submission + option_only_fields = [ + "show_disabled", + "enable_vc_detection", + "clear_cache", + "exclude_existing", + "apply_filters", + ] + non_option_fields = [ + f for f in data if f not in option_only_fields + ["csrfmiddlewaretoken", "use_background_job"] + ] + has_option_only = bool(data) and not bool(non_option_fields) and not has_filters + + # Apply default only on initial load (no filters, no job_id, no real submission) + if "use_background_job" not in data and not data.get("job_id") and not has_filters and not has_option_only: data["use_background_job"] = "on" args = (data,) + args[1:] @@ -577,19 +591,36 @@ def _populate_librenms_locations(self): """Fetch and populate LibreNMS locations in the dropdown.""" from django.core.cache import cache + from netbox_librenms_plugin.import_utils.cache import get_location_choices_cache_key from netbox_librenms_plugin.librenms_api import LibreNMSAPI try: - # Use caching to avoid repeated API calls - cache_key = "librenms_locations_choices" + # Determine server_key cheaply from settings to check cache before instantiating the API + try: + from netbox_librenms_plugin.models import LibreNMSSettings + + _settings = LibreNMSSettings.objects.first() + _server_key = (_settings.selected_server if _settings else None) or "default" + except Exception: + _server_key = "default" + + cache_key = get_location_choices_cache_key(_server_key) cached_choices = cache.get(cache_key) + if cached_choices is not None: + self.fields["librenms_location"].choices = cached_choices + return - if cached_choices: + # Cache miss — instantiate the API client and fetch + api = LibreNMSAPI() + # Recompute cache_key with the resolved server_key in case it differs from settings + cache_key = get_location_choices_cache_key(api.server_key) + # Second cache check: the resolved server_key may differ from the settings key + cached_choices = cache.get(cache_key) + if cached_choices is not None: self.fields["librenms_location"].choices = cached_choices return # Fetch locations from LibreNMS - api = LibreNMSAPI() success, locations = api.get_locations() if success and locations: @@ -752,8 +783,13 @@ def __init__(self, *args, **kwargs): if validation and validation.get("device_type", {}).get("suggestions"): suggestions = validation["device_type"]["suggestions"] if suggestions: - # Include suggested device types first, then all others + # Annotate with suggested_order so suggested types sort first suggested_ids = [s["device_type"].id for s in suggestions] - self.fields["device_type"].queryset = DeviceType.objects.filter( - id__in=suggested_ids - ) | DeviceType.objects.exclude(id__in=suggested_ids) + priority = Case( + *[When(id=pk, then=Value(i)) for i, pk in enumerate(suggested_ids)], + default=Value(len(suggested_ids)), + output_field=IntegerField(), + ) + self.fields["device_type"].queryset = DeviceType.objects.annotate(suggested_order=priority).order_by( + "suggested_order", "manufacturer__name", "model" + ) diff --git a/netbox_librenms_plugin/import_utils/__init__.py b/netbox_librenms_plugin/import_utils/__init__.py index b7a08fef59..81c24025ea 100644 --- a/netbox_librenms_plugin/import_utils/__init__.py +++ b/netbox_librenms_plugin/import_utils/__init__.py @@ -23,6 +23,7 @@ get_active_cached_searches, get_cache_metadata_key, get_import_device_cache_key, + get_import_search_cache_key, get_validated_device_cache_key, ) from .device_operations import ( # noqa: F401 diff --git a/netbox_librenms_plugin/import_utils/bulk_import.py b/netbox_librenms_plugin/import_utils/bulk_import.py index f24b56bf44..aea305cb69 100644 --- a/netbox_librenms_plugin/import_utils/bulk_import.py +++ b/netbox_librenms_plugin/import_utils/bulk_import.py @@ -1,16 +1,19 @@ -"""Bulk import orchestration and filter processing.""" +"""Bulk import orchestration for devices and filter processing.""" +import hashlib import logging from typing import List from core.choices import JobStatusChoices from django.core.cache import cache +from ..import_validation_helpers import apply_role_to_validation, recalculate_validation_status from ..librenms_api import LibreNMSAPI +from ..utils import find_by_librenms_id 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 .filters import get_librenms_devices_for_import -from .permissions import require_permissions +from .permissions import check_user_permissions, require_permissions from .virtual_chassis import ( create_virtual_chassis_with_members, empty_virtual_chassis_data, @@ -20,12 +23,36 @@ logger = logging.getLogger(__name__) +def _safe_disabled(device: dict) -> int: + """ + Return 1 if the device is disabled, 0 otherwise. + + Handles None, booleans, numeric strings, and common truthy/falsy tokens + (e.g. "true"/"yes"/"on" → 1, "false"/"no"/"off" → 0) without raising. + """ + val = device.get("disabled", 0) + if isinstance(val, bool): + return int(val) + if isinstance(val, str): + normalized = val.strip().lower() + if normalized in ("1", "true", "yes", "on"): + return 1 + if normalized in ("0", "false", "no", "off", ""): + return 0 + try: + int_val = int(val) + return 1 if int_val else 0 + except (TypeError, ValueError): + return 0 + + def bulk_import_devices_shared( device_ids: List[int], server_key: str = None, sync_options: dict = None, manual_mappings_per_device: dict = None, libre_devices_cache: dict = None, + vc_detection_enabled: bool = False, job=None, user=None, ) -> dict: @@ -43,6 +70,8 @@ def bulk_import_devices_shared( Example: {1179: {'device_role_id': 5}, 1180: {'device_role_id': 3}} libre_devices_cache: Optional dict mapping device_id to pre-fetched device data to avoid redundant API calls. Example: {123: {...device_data...}} + vc_detection_enabled: Whether to enable virtual chassis detection during import. + Should match the flag used during the filter/preview step for consistency. job: Optional JobRunner instance for progress logging and cancellation checks user: User performing the import (for permission checks). If job is provided, user is extracted from job.job.user if not explicitly passed. @@ -70,11 +99,13 @@ def bulk_import_devices_shared( if user is None and job is not None: user = getattr(job.job, "user", None) - # Check permissions at start of bulk operation + # Check permissions at start of bulk operation — both device and VM perms are + # required because any device may be flagged as import_as_vm during validation. required_perms = [ "dcim.add_device", - "dcim.add_interface", - "dcim.add_virtualchassis", + "dcim.change_device", + "virtualization.add_virtualmachine", + "virtualization.change_virtualmachine", ] require_permissions(user, required_perms, "import devices") @@ -84,26 +115,42 @@ def bulk_import_devices_shared( skipped_list = [] vc_created_count = 0 processed_vc_domains = set() # Track VCs already created by domain + _cancelled = False # Initialize API client once for all devices to avoid repeated config parsing api = LibreNMSAPI(server_key=server_key) for idx, device_id in enumerate(device_ids, start=1): - # Check for job cancellation every 5 devices - if job and idx % 5 == 0: - # Refresh job from DB to get current status - job.job.refresh_from_db() - job_status = job.job.status - status_value = job_status.value if hasattr(job_status, "value") else job_status - if status_value in (JobStatusChoices.STATUS_FAILED, "failed", "errored"): - if job.logger: - job.logger.warning(f"Import job cancelled at device {idx} of {total}") - else: - logger.warning(f"Import cancelled at device {idx} of {total}") - break - # Log progress - if job.logger: - job.logger.info(f"Imported device {idx} of {total}") + # Check for job cancellation on first iteration and every 5th thereafter. + # Check RQ/Redis state first (reflects stop API immediately); fall back to DB. + if job and (idx == 1 or idx % 5 == 0): + try: + from django_rq import get_queue + from rq.job import Job as RQJob + + queue = get_queue("default") + rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) + if rq_job.is_failed or rq_job.is_stopped: + if job.logger: + job.logger.warning( + f"Import job stopped at device {idx} of {total} (RQ status: {rq_job.get_status()})" + ) + else: + logger.warning(f"Import cancelled at device {idx} of {total}") + _cancelled = True + break + except Exception: + # Fall back to DB check if RQ is unavailable + job.job.refresh_from_db() + job_status = job.job.status + status_value = job_status.value if hasattr(job_status, "value") else job_status + if status_value in (JobStatusChoices.STATUS_FAILED, "failed", "errored"): + if job.logger: + job.logger.warning(f"Import job cancelled at device {idx} of {total}") + else: + logger.warning(f"Import cancelled at device {idx} of {total}") + _cancelled = True + break try: # Use cached device data if available to avoid redundant API calls @@ -129,6 +176,8 @@ def bulk_import_devices_shared( api=api, use_sysname=use_sysname_opt, strip_domain=strip_domain_opt, + server_key=api.server_key, + include_vc_detection=vc_detection_enabled, ) # Build manual mappings from validation + any provided overrides @@ -148,7 +197,8 @@ def bulk_import_devices_shared( result = import_single_device( device_id, - server_key=server_key, + server_key=api.server_key, # use resolved key, not raw parameter (may be None) + validation=validation, sync_options=sync_options, manual_mappings=device_mappings if device_mappings else None, libre_device=libre_device, @@ -162,6 +212,9 @@ def bulk_import_devices_shared( "message": result["message"], } ) + # Log progress after each successful import + if job and job.logger: + job.logger.info(f"Imported device {idx} of {total}") # Handle virtual chassis creation for stacks vc_data = validation.get("virtual_chassis", {}) @@ -176,19 +229,45 @@ def bulk_import_devices_shared( for m in vc_data.get("members", []) if (serial := str(m.get("serial") or "").strip()) and serial != "-" ) - vc_domain = ( - f"librenms-stack-{','.join(member_serials)}" if member_serials else f"librenms-{device_id}" - ) - + if member_serials: + vc_domain = f"librenms-stack-{','.join(member_serials)}" + else: + # No serials available — build a stable fingerprint from member name/model/position + # so all LibreNMS devices in the same physical stack share the same dedup key. + member_parts = sorted( + f"{m.get('name', '')}/{m.get('model', '')}:{m.get('position', 0)}" + for m in vc_data.get("members", []) + ) + if member_parts: + fingerprint = hashlib.md5((f"{device_id}," + ",".join(member_parts)).encode()).hexdigest()[ + :12 + ] + vc_domain = f"librenms-stack-{fingerprint}" + else: + vc_domain = f"librenms-{device_id}" + + # Guard VC creation with its own permission check — the upfront check + # only covers add_device/change_device; VirtualChassis needs a separate perm. + has_vc_perm, missing_vc_perms = check_user_permissions(user, ["dcim.add_virtualchassis"]) + if not has_vc_perm: + warn_msg = ( + f"Skipping VC creation for device {device_id}: " + f"missing permissions: {', '.join(missing_vc_perms)}" + ) + if job and job.logger: + job.logger.warning(warn_msg) + else: + logger.warning(warn_msg) # Only create VC if we haven't processed this stack yet # Add to set BEFORE attempting creation to prevent race condition - if vc_domain not in processed_vc_domains: + elif vc_domain not in processed_vc_domains: processed_vc_domains.add(vc_domain) try: vc = create_virtual_chassis_with_members( result["device"], vc_data["members"], libre_device, + server_key=api.server_key, ) vc_created_count += 1 log_msg = f"Created VC '{vc.name}' during bulk import for device {device_id}" @@ -227,6 +306,7 @@ def bulk_import_devices_shared( "failed": failed_list, "skipped": skipped_list, "virtual_chassis_created": vc_created_count, + "cancelled": _cancelled, } @@ -236,6 +316,7 @@ def bulk_import_devices( sync_options: dict = None, manual_mappings_per_device: dict = None, libre_devices_cache: dict = None, + vc_detection_enabled: bool = False, user=None, ) -> dict: """ @@ -252,6 +333,7 @@ def bulk_import_devices( Example: {1179: {'device_role_id': 5}, 1180: {'device_role_id': 3}} libre_devices_cache: Optional dict mapping device_id to pre-fetched device data to avoid redundant API calls. Example: {123: {...device_data...}} + vc_detection_enabled: Whether to enable virtual chassis detection during import. user: User performing the import (for permission checks) Returns: @@ -273,50 +355,127 @@ def bulk_import_devices( sync_options=sync_options, manual_mappings_per_device=manual_mappings_per_device, libre_devices_cache=libre_devices_cache, + vc_detection_enabled=vc_detection_enabled, job=None, # No job context for synchronous imports user=user, ) -def _refresh_existing_device(validation: dict) -> None: - """Refresh existing_device from DB to pick up changes made in NetBox since caching.""" +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. + + When existing_device is None (wasn't found at cache time), re-check if the device + was imported since caching by looking up librenms_id or hostname. + """ existing = validation.get("existing_device") - if not existing or not hasattr(existing, "pk"): + if existing and hasattr(existing, "pk"): + try: + from dcim.models import Device + from virtualization.models import VirtualMachine + + if validation.get("import_as_vm"): + refreshed = VirtualMachine.objects.filter(pk=existing.pk).first() + else: + refreshed = Device.objects.filter(pk=existing.pk).first() + + 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"): + apply_role_to_validation(validation, None) + recalculate_validation_status(validation, is_vm=bool(validation.get("import_as_vm"))) + 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 + # 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. + if not validation.get("import_as_vm"): + validation["device_role"] = {"found": False, "role": None} + 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 + if not libre_device: return try: from dcim.models import Device from virtualization.models import VirtualMachine - if validation.get("import_as_vm"): - refreshed = VirtualMachine.objects.filter(pk=existing.pk).first() - else: - refreshed = Device.objects.filter(pk=existing.pk).first() + import_as_vm = validation.get("import_as_vm", False) + Model = VirtualMachine if import_as_vm else Device + # Also check the opposite model — the LibreNMS object may have been + # imported as a VM even though import_as_vm=False (or vice versa). + CrossModel = Device if import_as_vm else VirtualMachine - if refreshed: - validation["existing_device"] = refreshed - if hasattr(refreshed, "role") and refreshed.role: - validation["device_role"]["found"] = True - validation["device_role"]["role"] = refreshed.role - else: - # Device was deleted since caching — recompute readiness - validation["existing_device"] = None - validation["existing_match_type"] = None - if validation.get("import_as_vm"): - required_found = ( - validation.get("site", {}).get("found") - and validation.get("cluster", {}).get("found") - and validation.get("device_role", {}).get("found") - ) - else: - required_found = ( - validation.get("site", {}).get("found") - and validation.get("device_type", {}).get("found") - and validation.get("device_role", {}).get("found") - ) - validation["can_import"] = validation["is_ready"] = bool(required_found and not validation.get("issues")) + librenms_id = libre_device.get("device_id") + hostname = libre_device.get("hostname", "") + sys_name = libre_device.get("sysName", "") + + new_device = None + match_type = None + found_as_cross_model = False + + def _lookup_in_model(m): + """Return (device, match_type) for model m, or (None, None).""" + if librenms_id is not None and not isinstance(librenms_id, bool): + try: + dev = find_by_librenms_id(m, int(librenms_id), server_key) + if dev: + return dev, "librenms_id" + except (ValueError, TypeError): + pass + 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 + + new_device, match_type = _lookup_in_model(Model) + + if not new_device: + # Try the opposite model: catches cross-model imports that happened + # after the cache was built (e.g. LibreNMS device imported as VM). + new_device, match_type = _lookup_in_model(CrossModel) + if new_device: + found_as_cross_model = True + + if new_device: + validation["existing_device"] = new_device + validation["existing_match_type"] = match_type + 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 + if not actual_is_vm and hasattr(new_device, "role") and new_device.role: + validation["device_role"] = {"found": True, "role": new_device.role} + elif not actual_is_vm: + validation.setdefault("device_role", {}).update({"found": False, "role": None}) 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}") + logger.error(f"Failed to check for newly imported device: {e}") + + +def _empty_return(return_cache_status: bool): + """Centralised empty-result return value for process_device_filters.""" + return ([], False) if return_cache_status else [] def process_device_filters( @@ -350,7 +509,7 @@ def process_device_filters( request: Optional Django request for client disconnect detection (synchronous only) return_cache_status: When True, returns (devices, from_cache) tuple use_sysname: If True, prefer sysName over hostname for device name resolution - strip_domain: If True, strip domain suffix from device names + strip_domain: If True, strip domain suffix from device name Returns: List[dict]: Validated devices with _validation key, or tuple of (devices, from_cache) @@ -372,9 +531,11 @@ def process_device_filters( return_cache_status=True, ) - # Filter out disabled devices if requested + # Filter out disabled devices if requested. LibreNMS's "disabled" field (1=disabled, + # 0=enabled) reflects manual device disablement; "status" reflects SNMP reachability. + # show_disabled controls the former: hidden when disabled==1, shown regardless of status. if not show_disabled: - libre_devices = [d for d in libre_devices if d.get("status") == 1] + libre_devices = [d for d in libre_devices if _safe_disabled(d) != 1] if job: job.logger.info(f"Found {len(libre_devices)} devices to process") @@ -398,13 +559,14 @@ def process_device_filters( except (BrokenPipeError, ConnectionError, IOError) as e: if request: logger.info(f"Client disconnected during VC prefetch: {e}") - return [] + return _empty_return(return_cache_status) raise # Validate each device validated_devices = [] total = len(libre_devices) - api_for_validation = api if vc_detection_enabled else None + # Always pass api so validate_device_for_import can run hardware/chassis lookups. + # vc_detection_enabled only gates VC-specific paths inside that function. if job: job.logger.info(f"Starting validation of {total} devices") @@ -418,13 +580,13 @@ def process_device_filters( if rq_job.is_failed or rq_job.is_stopped: job.logger.warning("Job was already stopped before validation started") - return [] + return _empty_return(return_cache_status) except Exception: # Fall back to DB check if RQ check fails job.job.refresh_from_db() - if job.job.status == JobStatusChoices.STATUS_FAILED: + if job.job.status in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED): job.logger.warning("Job was stopped before validation started") - return [] + return _empty_return(return_cache_status) else: logger.info(f"Validating {total} devices") @@ -447,21 +609,13 @@ def process_device_filters( job.logger.info( f"Job stopped at device {idx}/{total} (RQ status: {rq_job.get_status()}). Exiting gracefully." ) - return [] + return _empty_return(return_cache_status) except Exception: # If we can't check RQ status, fall back to DB status check job.job.refresh_from_db() - if job.job.status == JobStatusChoices.STATUS_FAILED: + if job.job.status in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED): job.logger.info(f"Job stopped at device {idx}/{total}. Exiting gracefully.") - return [] - elif request: - # Check for client disconnect - try: - if hasattr(request, "META") and request.META.get("wsgi.input"): - pass - except (BrokenPipeError, ConnectionError, IOError): - logger.info(f"Client disconnected during validation at device {idx}") - return [] + return _empty_return(return_cache_status) # Drop any cached validation/meta keys before recomputing device.pop("_validation", None) @@ -487,7 +641,7 @@ def process_device_filters( # Refresh existing_device from DB to avoid stale data # (user may have changed role, name, etc. in NetBox) - _refresh_existing_device(device["_validation"]) + _refresh_existing_device(device["_validation"], libre_device=device, server_key=api.server_key) # Apply exclude_existing filter if enabled if exclude_existing: @@ -502,16 +656,17 @@ def process_device_filters( try: validation = validate_device_for_import( device, - api=api_for_validation, + api=api, include_vc_detection=vc_detection_enabled, - force_vc_refresh=clear_cache, + force_vc_refresh=False, + server_key=api.server_key, use_sysname=use_sysname, strip_domain=strip_domain, ) except (BrokenPipeError, ConnectionError, IOError) as e: if request: logger.info(f"Client disconnected during device validation: {e}") - return [] + return _empty_return(return_cache_status) raise # Set VC detection metadata @@ -577,8 +732,10 @@ def process_device_filters( # Add this cache key if not already in index if cache_metadata_key not in cache_index: cache_index.append(cache_metadata_key) - # Store index with same timeout as the metadata - cache.set(cache_index_key, cache_index, timeout=api.cache_timeout) + # Always re-write the index so its TTL matches the freshly-written metadata. + # Without this the index can expire before the metadata and the active + # search entry disappears from the UI. + cache.set(cache_index_key, cache_index, timeout=api.cache_timeout) if job: if exclude_existing: diff --git a/netbox_librenms_plugin/import_utils/cache.py b/netbox_librenms_plugin/import_utils/cache.py index 81fb8f7e32..8de881f897 100644 --- a/netbox_librenms_plugin/import_utils/cache.py +++ b/netbox_librenms_plugin/import_utils/cache.py @@ -1,5 +1,7 @@ -"""Cache key generation and search management for device import operations.""" +"""Cache key generation and management for device import operations.""" +import hashlib +import json import logging from django.core.cache import cache @@ -7,6 +9,11 @@ logger = logging.getLogger(__name__) +def get_location_choices_cache_key(server_key: str) -> str: + """Return the cache key for LibreNMS location choices for a given server.""" + return f"librenms_locations_choices:{server_key}" + + def get_cache_metadata_key( server_key: str, filters: dict, vc_enabled: bool, use_sysname: bool = True, strip_domain: bool = False ) -> str: @@ -17,16 +24,22 @@ def get_cache_metadata_key( server_key: LibreNMS server identifier filters: Filter dictionary vc_enabled: Whether VC detection is enabled - use_sysname: Whether sysName is preferred for device name resolution + use_sysname: Whether sysName is preferred over hostname for device naming strip_domain: Whether domain suffix is stripped from device names Returns: str: Consistent cache key for metadata """ - # Sort filter items to ensure consistent key generation - filter_parts = "_".join(f"{k}={v}" for k, v in sorted(filters.items()) if v) - naming_part = f"sysname{int(use_sysname)}_strip{int(strip_domain)}" - return f"librenms_filter_cache_metadata_{server_key}_{filter_parts}_{vc_enabled}_{naming_part}" + # Sort filter items to ensure consistent key generation; use "is not None" to preserve + # valid falsy values like 0 and False (filtering only None/missing entries). + # Use JSON serialization for a stable, collision-free hash (avoids issues with + # values containing "=" or "_" that could collide with the key separators). + filter_hash = hashlib.sha256( + json.dumps( + {k: v for k, v in sorted(filters.items()) if v is not None}, sort_keys=True, separators=(",", ":") + ).encode() + ).hexdigest()[:16] + return f"librenms_filter_cache_metadata_{server_key}_{filter_hash}_{vc_enabled}_sysname={use_sysname}_strip={strip_domain}" def get_active_cached_searches(server_key: str) -> list[dict]: @@ -66,8 +79,9 @@ def get_active_cached_searches(server_key: str) -> list[dict]: "other": "Other", } - # Get cached location choices for enrichment - location_cache_key = "librenms_locations_choices" + # Get cached location choices for enrichment; scoped by server_key so labels + # from different LibreNMS servers don't bleed into each other's filter summaries. + location_cache_key = get_location_choices_cache_key(server_key) cached_locations = cache.get(location_cache_key) if cached_locations: location_choices = dict(cached_locations) @@ -76,9 +90,18 @@ def get_active_cached_searches(server_key: str) -> list[dict]: metadata = cache.get(cache_key) if metadata: # Cache still exists, calculate time remaining - cached_at = datetime.fromisoformat(metadata.get("cached_at")) cache_timeout = metadata.get("cache_timeout", 300) now = datetime.now(timezone.utc) + try: + cached_at_raw = metadata.get("cached_at") + cached_at = ( + datetime.fromisoformat(cached_at_raw) if cached_at_raw else datetime.fromtimestamp(0, timezone.utc) + ) + # Normalize naive datetimes (e.g., stored without tzinfo) to UTC + if cached_at.tzinfo is None: + cached_at = cached_at.replace(tzinfo=timezone.utc) + except (ValueError, TypeError): + cached_at = datetime.fromtimestamp(0, timezone.utc) age_seconds = (now - cached_at).total_seconds() remaining_seconds = max(0, cache_timeout - age_seconds) @@ -133,7 +156,7 @@ def get_validated_device_cache_key( filters: Filter dict with location, type, os, hostname, sysname, hardware keys device_id: LibreNMS device ID vc_enabled: Whether virtual chassis detection was enabled - use_sysname: Whether sysName is preferred for device name resolution + use_sysname: Whether sysName is preferred over hostname for device naming strip_domain: Whether domain suffix is stripped from device names Returns: @@ -142,16 +165,17 @@ def get_validated_device_cache_key( Example: >>> key = get_validated_device_cache_key('default', {'location': 'NYC'}, 123, True) >>> key - 'validated_device_default_-1234567890_123_vc_sysname1_strip0' + 'validated_device_default_e3b0c44298fc1c14_123_vc' """ - # Sort filters for consistent hashing - filter_hash = hash(str(sorted(filters.items()))) + # Sort filters for a deterministic, cross-process stable hash + filter_hash = hashlib.sha256(json.dumps(sorted(filters.items()), sort_keys=True).encode()).hexdigest()[:16] vc_part = "vc" if vc_enabled else "novc" - naming_part = f"sysname{int(use_sysname)}_strip{int(strip_domain)}" - return f"validated_device_{server_key}_{filter_hash}_{device_id}_{vc_part}_{naming_part}" + return ( + f"validated_device_{server_key}_{filter_hash}_{device_id}_{vc_part}_sysname={use_sysname}_strip={strip_domain}" + ) -def get_import_device_cache_key(device_id: int | str, server_key: str = "default") -> str: +def get_import_device_cache_key(device_id: int | str, server_key: str) -> str: """ Generate cache key for raw LibreNMS device data. @@ -161,7 +185,7 @@ def get_import_device_cache_key(device_id: int | str, server_key: str = "default Args: device_id: LibreNMS device ID - server_key: LibreNMS server identifier for multi-server setups + server_key: LibreNMS server identifier for multi-server setups (required) Returns: str: Cache key for the device data @@ -171,3 +195,29 @@ def get_import_device_cache_key(device_id: int | str, server_key: str = "default 'import_device_data_production_123' """ return f"import_device_data_{server_key}_{device_id}" + + +def get_import_search_cache_key(server_key: str, api_filters: dict, client_filters: dict) -> str: + """ + Generate a deterministic cache key for a LibreNMS device search result. + + The key encodes the server, API-side filters, and client-side filters so + that different filter combinations produce distinct cache entries. + + Args: + server_key: Resolved LibreNMS server key (use ``api.server_key``). + api_filters: Filters forwarded to the LibreNMS API. + client_filters: Filters applied client-side after the API response. + + Returns: + str: Cache key for the import search result. + """ + import hashlib + import json + + def _hash(d): + return hashlib.sha256( + json.dumps(sorted(d.items()) if isinstance(d, dict) else d, sort_keys=True).encode() + ).hexdigest()[:16] + + return f"librenms_devices_import_{server_key}_{_hash(api_filters)}_{_hash(client_filters)}" diff --git a/netbox_librenms_plugin/import_utils/device_operations.py b/netbox_librenms_plugin/import_utils/device_operations.py index 608cc42927..551449c334 100644 --- a/netbox_librenms_plugin/import_utils/device_operations.py +++ b/netbox_librenms_plugin/import_utils/device_operations.py @@ -14,6 +14,7 @@ find_matching_platform, find_matching_site, match_librenms_hardware_to_device_type, + set_librenms_device_id, ) from .cache import get_import_device_cache_key from .virtual_chassis import ( @@ -48,7 +49,6 @@ def _try_chassis_device_type_match(api, device_id): if not success or not inventory: return None - first_ambiguous_model = None for item in inventory: # Try entPhysicalName first (often the chassis part number like CHAS-BP-MX480-S) for field in ("entPhysicalName", "entPhysicalModelName"): @@ -56,9 +56,6 @@ def _try_chassis_device_type_match(api, device_id): if value and value not in skip_values: chassis_match = match_librenms_hardware_to_device_type(value) if chassis_match is None: - # MultipleObjectsReturned — ambiguous match; remember first - if first_ambiguous_model is None: - first_ambiguous_model = value continue if chassis_match["matched"]: chassis_match["match_type"] = "chassis" @@ -66,15 +63,7 @@ def _try_chassis_device_type_match(api, device_id): return chassis_match except Exception: logger.debug(f"Chassis inventory fallback failed for device {device_id}", exc_info=True) - return None - if first_ambiguous_model is not None: - return { - "matched": False, - "device_type": None, - "match_type": "chassis_ambiguous", - "chassis_model": first_ambiguous_model, - } return None @@ -143,6 +132,7 @@ def validate_device_for_import( force_vc_refresh: bool = False, use_sysname: bool = True, strip_domain: bool = False, + server_key: str = "default", ) -> dict: """ Validate if a LibreNMS device can be imported to NetBox. @@ -217,6 +207,7 @@ def validate_device_for_import( "serial_action": None, # None, "link", "conflict", "update_serial", "hostname_differs" "serial_confirmed": False, # True when librenms_id match and serial matches "serial_duplicate": False, # True when incoming serial is already on a different device + "librenms_id_needs_migration": False, # True when existing device has legacy bare-int ID "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 @@ -289,10 +280,13 @@ def validate_device_for_import( from virtualization.models import VirtualMachine + server_key = api.server_key if api is not None else server_key + # Check for existing VM first (by librenms_id custom field) - # Always query with int to match custom field type try: - existing_vm = VirtualMachine.objects.filter(custom_field_data__librenms_id=int(librenms_id)).first() + from netbox_librenms_plugin.utils import find_by_librenms_id + + existing_vm = find_by_librenms_id(VirtualMachine, librenms_id, server_key) except (ValueError, TypeError): # librenms_id is not convertible to int; no match will be found existing_vm = None @@ -304,7 +298,17 @@ def validate_device_for_import( result["import_as_vm"] = True # Force VM mode since VM exists result["can_import"] = False - # Check if name matches sysName + # 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 + # raw type check on custom_field_data reveals whether migration is needed. + _vm_cf_id = existing_vm.custom_field_data.get("librenms_id") + if (isinstance(_vm_cf_id, int) and not isinstance(_vm_cf_id, bool)) or ( + isinstance(_vm_cf_id, str) and _vm_cf_id.isdigit() + ): + result["librenms_id_needs_migration"] = True + + # Check if name matches resolved name (accounts for use_sysname/strip_domain) # Note: name_sync_available/suggested_name are intentionally not set for VMs # because UpdateDeviceNameView only supports Device objects; VM name-sync # would require a separate implementation. @@ -312,10 +316,11 @@ def validate_device_for_import( result["name_matches"] = True # Check for existing Device (by librenms_id custom field) - # Always query with int to match custom field type if not result["existing_device"]: try: - existing_device = Device.objects.filter(custom_field_data__librenms_id=int(librenms_id)).first() + from netbox_librenms_plugin.utils import find_by_librenms_id + + existing_device = find_by_librenms_id(Device, librenms_id, server_key) except (ValueError, TypeError): # librenms_id is not convertible to int; no match will be found existing_device = None @@ -326,6 +331,16 @@ def validate_device_for_import( result["existing_match_type"] = "librenms_id" result["can_import"] = False + # 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 + # raw type check on custom_field_data reveals whether migration is needed. + _dev_cf_id = existing_device.custom_field_data.get("librenms_id") + if (isinstance(_dev_cf_id, int) and not isinstance(_dev_cf_id, bool)) or ( + isinstance(_dev_cf_id, str) and _dev_cf_id.isdigit() + ): + result["librenms_id_needs_migration"] = True + # Check if name matches resolved name (VC-aware: compare against VC member name) if hostname and existing_device.virtual_chassis and existing_device.vc_position: incoming_serial = libre_device.get("serial") or "" @@ -481,11 +496,8 @@ def validate_device_for_import( # Validate based on import type (Device or VM) if import_as_vm: - # 2. For VMs: Validate Cluster (required) - Must be manually selected - result["cluster"]["found"] = False - if not result.get("existing_device"): - result["issues"].append("Cluster must be manually selected before importing as VM") - # Provide list of available clusters for user selection (cached) + # Always populate available clusters for all VMs (new or existing) so + # the cluster dropdown has options whether creating or updating a VM. cache_key = "librenms_import_all_clusters" all_clusters = cache.get(cache_key) if all_clusters is None: @@ -495,7 +507,13 @@ def validate_device_for_import( cache.set(cache_key, all_clusters, cache_timeout) result["cluster"]["available_clusters"] = all_clusters - # Skip device-specific validations for VMs + if import_as_vm: + if not result.get("existing_device"): + # 2. For NEW VMs: Validate Cluster (required) - Must be manually selected + result["cluster"]["found"] = False + result["issues"].append("Cluster must be manually selected before importing as VM") + + # Skip device-specific validations for all VMs (new and existing) result["site"]["found"] = True # Not required for VMs result["device_type"]["found"] = True # Not required for VMs result["device_role"]["found"] = True # Not required for VMs @@ -533,18 +551,6 @@ def validate_device_for_import( chassis_match = _try_chassis_device_type_match(api, device_id) if chassis_match and chassis_match["matched"]: dt_match = chassis_match - elif chassis_match and chassis_match.get("match_type") == "chassis_ambiguous": - # Chassis inventory returned multiple matches — propagate ambiguity - dt_match = { - "matched": False, - "device_type": None, - "match_type": "ambiguous", - } - result["issues"].append( - f"Multiple device types match chassis hardware" - f" '{chassis_match['chassis_model']}'" - " — resolve the ambiguity in NetBox." - ) # Update result keys individually to preserve the existing schema (especially "found") result["device_type"]["found"] = dt_match["matched"] @@ -552,6 +558,7 @@ def validate_device_for_import( result["device_type"]["match_type"] = dt_match.get("match_type") if not result["device_type"]["found"] and result["device_type"].get("match_type") != "ambiguous": + result["device_type"]["found"] = False result["issues"].append(f"No matching device type found for hardware: '{hardware}'") # Get some device types for user to choose from all_device_types = DeviceType.objects.all()[:10] @@ -564,12 +571,12 @@ def validate_device_for_import( for dt in all_device_types ] - # 4. DeviceRole (required) - Must be manually selected by user - logger.debug(f"[{hostname}] Issues BEFORE adding role issue: {result['issues']}") - result["device_role"]["found"] = False if not result.get("existing_device"): + # 4. DeviceRole (required for new devices) - Must be manually selected + logger.debug(f"[{hostname}] Issues BEFORE adding role issue: {result['issues']}") + result["device_role"]["found"] = False result["issues"].append("Device role must be manually selected before import") - logger.debug(f"[{hostname}] Issues AFTER adding role issue: {result['issues']}") + logger.debug(f"[{hostname}] Issues AFTER adding role issue: {result['issues']}") # Provide list of available roles for user selection (cached) cache_key = "librenms_import_all_roles" all_roles = cache.get(cache_key) @@ -761,6 +768,7 @@ def import_single_device( api=api, use_sysname=use_sysname_opt, strip_domain=strip_domain_opt, + server_key=api.server_key, ) # Check if device already exists @@ -795,8 +803,6 @@ def import_single_device( if rack_id: rack = Rack.objects.select_related("location", "site").filter(id=rack_id).first() or rack - rack = rack or validation.get("rack", {}).get("rack") - # Validate required fields if not site: return { @@ -825,16 +831,20 @@ def import_single_device( # Create device in NetBox with transaction.atomic(): - # Determine device name based on sync options - use_sysname = sync_options.get("use_sysname", True) if sync_options else True - strip_domain = sync_options.get("strip_domain", False) if sync_options else False - - device_name = _determine_device_name( - libre_device, - use_sysname=use_sysname, - strip_domain=strip_domain, - device_id=device_id, - ) + # Use pre-computed resolved_name from validation when available so the + # created device name matches exactly what was displayed in the import UI. + # Only fall back to recomputing from sync_options when no validation exists. + if validation and validation.get("resolved_name"): + device_name = validation["resolved_name"] + else: + use_sysname = sync_options.get("use_sysname", True) if sync_options else True + strip_domain = sync_options.get("strip_domain", False) if sync_options else False + device_name = _determine_device_name( + libre_device, + use_sysname=use_sysname, + strip_domain=strip_domain, + device_id=device_id, + ) # Generate import timestamp comment import_time = timezone.now().strftime("%Y-%m-%d %H:%M:%S %Z") @@ -846,7 +856,6 @@ def import_single_device( "role": device_role, "status": "active" if libre_device.get("status") == 1 else "offline", "comments": f"Imported from LibreNMS by netbox-librenms-plugin on {import_time}", - "custom_field_data": {"librenms_id": int(device_id)}, } # Add optional fields @@ -871,6 +880,7 @@ def import_single_device( # Create the device device = Device(**device_data) + set_librenms_device_id(device, device_id, api.server_key) device.full_clean() device.save() diff --git a/netbox_librenms_plugin/import_utils/filters.py b/netbox_librenms_plugin/import_utils/filters.py index 99aae78d5d..bdee6da79d 100644 --- a/netbox_librenms_plugin/import_utils/filters.py +++ b/netbox_librenms_plugin/import_utils/filters.py @@ -1,15 +1,40 @@ -"""Device filtering and API queries for LibreNMS devices.""" +"""Device filtering and retrieval from LibreNMS.""" import logging from typing import List from django.core.cache import cache +from .cache import get_import_search_cache_key + from ..librenms_api import LibreNMSAPI logger = logging.getLogger(__name__) +def _safe_disabled(device: dict) -> int: + """ + Return 1 if the device is disabled, 0 otherwise. + + Handles None, booleans, numeric strings, and common truthy/falsy tokens + (e.g. "true"/"yes"/"on" → 1, "false"/"no"/"off" → 0) without raising. + """ + val = device.get("disabled", 0) + if isinstance(val, bool): + return int(val) + if isinstance(val, str): + normalized = val.strip().lower() + if normalized in ("1", "true", "yes", "on"): + return 1 + if normalized in ("0", "false", "no", "off", ""): + return 0 + try: + int_val = int(val) + return 1 if int_val else 0 + except (TypeError, ValueError): + return 0 + + def get_device_count_for_filters( api: LibreNMSAPI, filters: dict, @@ -33,9 +58,11 @@ def get_device_count_for_filters( """ devices = get_librenms_devices_for_import(api, filters=filters, force_refresh=clear_cache) - # Filter out disabled devices if requested + # Filter out disabled devices if requested. LibreNMS's "disabled" field (1=disabled, + # 0=enabled) reflects manual device disablement; "status" reflects SNMP reachability. + # show_disabled controls the former: hidden when disabled==1, shown regardless of status. if not show_disabled: - devices = [d for d in devices if d.get("status") == 1] + devices = [d for d in devices if _safe_disabled(d) != 1] return len(devices) @@ -85,10 +112,15 @@ def get_librenms_devices_for_import( if filters: # Check for status filter first - it has special handling if filters.get("status") is not None: + # Normalize to int: form fields send strings ("1"/"0"), API may send ints + try: + status_val = int(filters["status"]) + except (ValueError, TypeError): + status_val = None # Status filter uses special types that don't need query param - if filters["status"] == 1: + if status_val == 1: api_filters["type"] = "up" - elif filters["status"] == 0: + elif status_val == 0: api_filters["type"] = "down" # Save ALL other filters for client-side filtering when status is used @@ -170,8 +202,9 @@ def get_librenms_devices_for_import( # We'll filter client-side if needed # Use caching to avoid repeated API calls - # Include both API and client filters in cache key - cache_key = f"librenms_devices_import_{server_key}_{hash(str(api_filters))}_{hash(str(client_filters))}" + # Include both API and client filters in cache key (deterministic, cross-process stable). + # Use api.server_key (always resolved) rather than the raw server_key arg (may differ). + cache_key = get_import_search_cache_key(api.server_key, api_filters, client_filters) from_cache = False if force_refresh: @@ -190,6 +223,8 @@ def get_librenms_devices_for_import( if not success: logger.error(f"Failed to retrieve devices from LibreNMS: {devices}") + # Cache a brief negative result to prevent hammering the API on repeated failures. + cache.set(cache_key, [], timeout=min(60, api.cache_timeout)) if return_cache_status: return [], False return [] @@ -232,19 +267,19 @@ def _apply_client_filters(devices: List[dict], filters: dict) -> List[dict]: if filters.get("type"): device_type = filters["type"].lower() - filtered = [d for d in filtered if d.get("type", "").lower() == device_type] + filtered = [d for d in filtered if (d.get("type") or "").lower() == device_type] if filters.get("os"): os_filter = filters["os"].lower() - filtered = [d for d in filtered if os_filter in d.get("os", "").lower()] + filtered = [d for d in filtered if os_filter in (d.get("os") or "").lower()] if filters.get("hostname"): hostname_filter = filters["hostname"].lower() - filtered = [d for d in filtered if hostname_filter in d.get("hostname", "").lower()] + filtered = [d for d in filtered if hostname_filter in (d.get("hostname") or "").lower()] if filters.get("sysname"): sysname_filter = filters["sysname"].lower() - filtered = [d for d in filtered if sysname_filter in d.get("sysName", "").lower()] + filtered = [d for d in filtered if sysname_filter in (d.get("sysName") or "").lower()] if filters.get("hardware"): hardware_filter = filters["hardware"].lower() diff --git a/netbox_librenms_plugin/import_utils/virtual_chassis.py b/netbox_librenms_plugin/import_utils/virtual_chassis.py index da1d0db897..f7a1a58b78 100644 --- a/netbox_librenms_plugin/import_utils/virtual_chassis.py +++ b/netbox_librenms_plugin/import_utils/virtual_chassis.py @@ -1,4 +1,4 @@ -"""Virtual chassis detection, creation, and caching.""" +"""Virtual chassis detection, creation, and management.""" import logging from typing import List @@ -32,11 +32,12 @@ def _clone_virtual_chassis_data(data: dict | None) -> dict: members = [] for idx, member in enumerate(data.get("members", [])): member_copy = member.copy() - raw_position = member_copy.get("position", idx) + raw_position = member_copy.get("position", idx + 1) try: - member_copy["position"] = int(raw_position) + pos = int(raw_position) + member_copy["position"] = pos if pos > 0 else idx + 1 except (TypeError, ValueError): - member_copy["position"] = idx + member_copy["position"] = idx + 1 # 1-based fallback; position 0 is invalid members.append(member_copy) member_count = data.get("member_count") or len(members) @@ -64,19 +65,29 @@ def get_virtual_chassis_data(api: LibreNMSAPI, device_id: int | str, *, force_re return empty_virtual_chassis_data() cache_key = _vc_cache_key(api, device_id) - if not force_refresh: + _cache_timeout = getattr(api, "cache_timeout", None) + cache_timeout = 300 if _cache_timeout is None else _cache_timeout + if not force_refresh and cache_timeout != 0: cached = cache.get(cache_key) if cached is not None: return _clone_virtual_chassis_data(cached) detection_data = detect_virtual_chassis_from_inventory(api, device_id) - if detection_data and "detection_error" not in detection_data: + if detection_data is None: + # Non-stack device or transient API failure — cache the negative result so + # prefetch_vc_data_for_devices() can skip these on subsequent renders. + # Use force_refresh=True to bypass the cache if needed. + empty = empty_virtual_chassis_data() + if cache_timeout != 0: + cache.set(cache_key, empty, timeout=cache_timeout) + return _clone_virtual_chassis_data(empty) + + if "detection_error" not in detection_data: detection_data["detection_error"] = None - cache_value = _clone_virtual_chassis_data(detection_data) if detection_data else empty_virtual_chassis_data() - - cache_timeout = getattr(api, "cache_timeout", 300) or 300 - cache.set(cache_key, cache_value, timeout=cache_timeout) + cache_value = _clone_virtual_chassis_data(detection_data) + if cache_timeout != 0: + cache.set(cache_key, cache_value, timeout=cache_timeout) return _clone_virtual_chassis_data(cache_value) @@ -117,7 +128,7 @@ def prefetch_vc_data_for_devices(api: LibreNMSAPI, device_ids: List[int], *, for logger.debug(f"VC cache warming complete for {len(device_ids)} devices") -def detect_virtual_chassis_from_inventory(api: LibreNMSAPI, device_id: int) -> dict: +def detect_virtual_chassis_from_inventory(api: LibreNMSAPI, device_id: int) -> dict | None: """ Detect if device is a stack/Virtual Chassis by analyzing ENTITY-MIB inventory. Vendor-agnostic using standard hierarchical structure. @@ -166,16 +177,21 @@ def detect_virtual_chassis_from_inventory(api: LibreNMSAPI, device_id: int) -> d return None # Step 2: Find parent container index - # Could be class="stack" or the main "chassis" + # Prefer "stack" over "chassis" for deterministic VC detection parent_index = None + stack_index = None + chassis_index = None for item in root_items: item_class = item.get("entPhysicalClass") - if item_class in ["stack", "chassis"]: - parent_index = item.get("entPhysicalIndex") - logger.debug(f"VC detection: Found parent container at index {parent_index} for device {device_id}") - break - - if not parent_index: + if item_class == "stack" and stack_index is None: + stack_index = item.get("entPhysicalIndex") + elif item_class == "chassis" and chassis_index is None: + chassis_index = item.get("entPhysicalIndex") + parent_index = stack_index if stack_index is not None else chassis_index + if parent_index is not None: + logger.debug(f"VC detection: Found parent container at index {parent_index} for device {device_id}") + + if parent_index is None: return None # Step 3: Get children chassis at next level @@ -200,11 +216,15 @@ def detect_virtual_chassis_from_inventory(api: LibreNMSAPI, device_id: int) -> d vc_name_pattern = _load_vc_member_name_pattern() if master_name else None members = [] for idx, chassis in enumerate(chassis_items): - raw_position = chassis.get("entPhysicalParentRelPos", idx) + # entPhysicalParentRelPos is 1-based; fall back to idx+1 (not idx) so + # position 0 is never produced — VC positions must be ≥ 1. + raw_position = chassis.get("entPhysicalParentRelPos", idx + 1) try: position = int(raw_position) + if position <= 0: + position = idx + 1 except (TypeError, ValueError): - position = idx + position = idx + 1 member_data = { "serial": chassis.get("entPhysicalSerialNum", ""), "position": position, @@ -214,13 +234,14 @@ def detect_virtual_chassis_from_inventory(api: LibreNMSAPI, device_id: int) -> d "description": chassis.get("entPhysicalDescr", ""), } - # Generate suggested name if we have master name + # Generate suggested name if we have master name. + # position is already 1-based, so pass it directly (no +1). if master_name: member_data["suggested_name"] = _generate_vc_member_name( - master_name, position + 1, serial=member_data.get("serial"), pattern=vc_name_pattern + master_name, position, serial=member_data.get("serial"), pattern=vc_name_pattern ) else: - member_data["suggested_name"] = f"Member-{position + 1}" + member_data["suggested_name"] = f"Member-{position}" members.append(member_data) @@ -260,8 +281,9 @@ def _generate_vc_member_name(master_name: str, position: int, serial: str = None master_name: Name of the master/primary device position: VC position number serial: Optional serial number of the member device - pattern: Optional pre-loaded pattern; if None, loaded from settings via - _load_vc_member_name_pattern() + pattern: Optional pre-loaded name pattern; if None, loaded from settings. + Pass a pre-loaded pattern when calling inside a loop to avoid + repeated DB queries. Returns: Generated member device name @@ -311,13 +333,16 @@ def update_vc_member_suggested_names(vc_data: dict, master_name: str) -> dict: # Load naming pattern once to avoid a DB query per member vc_pattern = _load_vc_member_name_pattern() for idx, member in enumerate(vc_data.get("members", [])): - raw_position = member.get("position", idx) + # Positions are stored as 1-based (from entPhysicalParentRelPos or idx+1 fallback). + # Use them directly for name generation; only replace 0/negative with 1-based fallback. + raw_position = member.get("position", idx + 1) try: - base_position = int(raw_position) + position = int(raw_position) + if position <= 0: + position = idx + 1 except (TypeError, ValueError): - base_position = idx - position = base_position + 1 # Convert to 1-based position - member["position"] = base_position + position = idx + 1 + member["position"] = position member["suggested_name"] = _generate_vc_member_name( master_name, position, serial=member.get("serial"), pattern=vc_pattern ) @@ -325,7 +350,23 @@ def update_vc_member_suggested_names(vc_data: dict, master_name: str) -> dict: return vc_data -def create_virtual_chassis_with_members(master_device: Device, members_info: list, libre_device: dict): +def _safe_pos(value) -> int | None: + """Return int position or None if not parseable.""" + try: + return int(value) + except (TypeError, ValueError): + return None + + +def _norm_serial(s) -> str: + """Normalize serial: strip whitespace; treat '-' as absent.""" + s = str(s or "").strip() + return "" if s == "-" else s + + +def create_virtual_chassis_with_members( + master_device: Device, members_info: list, libre_device: dict, server_key: str | None = None +) -> VirtualChassis: """ Create Virtual Chassis and member devices from detection info. @@ -352,10 +393,18 @@ def create_virtual_chassis_with_members(master_device: Device, members_info: lis ] """ - # Store original master device state for rollback + # original_master_name is still referenced in warning messages inside the atomic block. original_master_name = master_device.name - original_vc = master_device.virtual_chassis - original_vc_position = master_device.vc_position + + # Find master's actual VC position from members_info by serial match; default to 1 + _master_pos = 1 + if _norm_serial(master_device.serial): + for _m in members_info: + if _norm_serial(_m.get("serial")) == _norm_serial(master_device.serial): + _found_pos = _safe_pos(_m.get("position")) + if _found_pos and _found_pos >= 1: + _master_pos = _found_pos + break try: with transaction.atomic(): @@ -363,7 +412,7 @@ def create_virtual_chassis_with_members(master_device: Device, members_info: lis vc_pattern = _load_vc_member_name_pattern() # Rename master device to include position 1 pattern master_device_new_name = _generate_vc_member_name( - original_master_name, 1, serial=master_device.serial, pattern=vc_pattern + original_master_name, _master_pos, serial=master_device.serial, pattern=vc_pattern ) # Check if renamed master conflicts with existing device @@ -379,27 +428,44 @@ def create_virtual_chassis_with_members(master_device: Device, members_info: lis # Create VC using original base name vc_name = master_base_name + _device_id = libre_device.get("device_id") or master_device.pk + _domain_prefix = f"librenms-{server_key}" if server_key else "librenms" vc = VirtualChassis.objects.create( name=vc_name, master=master_device, - domain=f"librenms-{libre_device['device_id']}", + domain=f"{_domain_prefix}-{_device_id}", ) # Update master device master_device.virtual_chassis = vc - master_device.vc_position = 1 # Master is position 1 + master_device.vc_position = _master_pos master_device.save() # Create member devices for remaining positions - position = 2 # Start at 2 (master is 1) + position = _master_pos + 1 # Start after master position + used_positions = {_master_pos} # Master occupies its actual position members_created = 0 for member in members_info: - # Skip if this is the master's serial - if member.get("serial") == master_device.serial: + # Normalize serial and position up front so all skip-checks and + # downstream logic use consistent values (strips whitespace and + # treats the sentinel "-" as "no serial"). + serial = str(member.get("serial") or "").strip() + if serial == "-": + serial = "" + member_pos = _safe_pos(member.get("position")) + + # Skip if this is the master's serial (only when both serials are non-empty) + if serial and serial == (master_device.serial or "").strip(): + continue + # Skip blank-serial entries that represent the master slot by position + if ( + not serial + and member_pos is not None + and master_device.vc_position is not None + and member_pos == master_device.vc_position + ): continue - - serial = member.get("serial") member_rack = master_device.rack member_location = master_device.location or ( @@ -411,7 +477,25 @@ def create_virtual_chassis_with_members(master_device: Device, members_info: lis logger.warning(f"Device with serial '{serial}' already exists, skipping VC member creation") continue - member_name = _generate_vc_member_name(master_base_name, position, serial=serial, pattern=vc_pattern) + # Prefer the discovered SNMP position; fall back to sequential counter. + # member_pos was normalized via _safe_pos() above; 0 is not a valid vc_position. + discovered_pos = member_pos if (member_pos is not None and member_pos >= 1) else None + # If discovered_pos is already taken by another member, treat as absent. + if discovered_pos is not None and discovered_pos in used_positions: + discovered_pos = None + # Consume next free sequential slot when no valid discovered_pos. + if discovered_pos is None: + while position in used_positions: + position += 1 + chosen_pos = position + position += 1 + else: + chosen_pos = discovered_pos + # Advance sequential counter past chosen position. + position = max(position, chosen_pos + 1) + used_positions.add(chosen_pos) + + member_name = _generate_vc_member_name(master_base_name, chosen_pos, serial=serial, pattern=vc_pattern) # Check for duplicate name if Device.objects.filter(name=member_name).exists(): @@ -428,15 +512,27 @@ def create_virtual_chassis_with_members(master_device: Device, members_info: lis platform=master_device.platform, serial=serial, virtual_chassis=vc, - vc_position=position, + vc_position=chosen_pos, comments=f"VC member (LibreNMS: {member.get('name', 'Unknown')})\n" f"Auto-created from stack inventory", ) members_created += 1 - position += 1 # Validate member count - expected_members = len([m for m in members_info if m.get("serial") != master_device.serial]) + # Validate member count — exclude master-slot entries with blank serials + expected_members = len( + [ + m + for m in members_info + if not (_norm_serial(m.get("serial")) and _norm_serial(m.get("serial")) == master_device.serial) + and not ( + not _norm_serial(m.get("serial")) + and m.get("position") is not None + and master_device.vc_position is not None + and _safe_pos(m["position"]) == master_device.vc_position + ) + ] + ) if members_created < expected_members: logger.warning( f"Created {members_created} members but expected {expected_members}. " @@ -451,12 +547,10 @@ def create_virtual_chassis_with_members(master_device: Device, members_info: lis return vc except Exception as e: - # Rollback master device to original state + # The transaction.atomic() block above will roll back all DB changes automatically. + # Manual state restoration is redundant and the save() would fail in a broken transaction. logger.error( - f"Virtual Chassis creation failed for device {master_device.name}: {e}. Rolling back master device changes." + f"Virtual Chassis creation failed for device {master_device.name}: {e}", + exc_info=True, ) - master_device.name = original_master_name - master_device.virtual_chassis = original_vc - master_device.vc_position = original_vc_position - master_device.save() raise diff --git a/netbox_librenms_plugin/import_utils/vm_operations.py b/netbox_librenms_plugin/import_utils/vm_operations.py index b75b69f7a9..0190c069ba 100644 --- a/netbox_librenms_plugin/import_utils/vm_operations.py +++ b/netbox_librenms_plugin/import_utils/vm_operations.py @@ -1,8 +1,9 @@ -"""Virtual machine import operations.""" +"""Virtual machine creation and import operations.""" import logging from dcim.models import DeviceRole +from django.db import transaction from django.utils import timezone from virtualization.models import Cluster @@ -13,7 +14,13 @@ logger = logging.getLogger(__name__) -def create_vm_from_librenms(libre_device: dict, validation: dict, use_sysname: bool = True, role=None): +def create_vm_from_librenms( + libre_device: dict, + validation: dict, + server_key: str, + use_sysname: bool = True, + strip_domain: bool = False, +): """ Create a NetBox VirtualMachine from LibreNMS device data. @@ -21,7 +28,7 @@ def create_vm_from_librenms(libre_device: dict, validation: dict, use_sysname: b libre_device: Device data from LibreNMS validation: Validation result from validate_device_for_import with import_as_vm=True use_sysname: If True, prefer sysName; if False, use hostname - role: Optional DeviceRole to assign to the VM + server_key: LibreNMS server key used to store the librenms_id custom field Returns: Created VirtualMachine instance @@ -37,14 +44,16 @@ def create_vm_from_librenms(libre_device: dict, validation: dict, use_sysname: b # Extract matched objects from validation cluster = validation["cluster"]["cluster"] platform = validation["platform"].get("platform") + role = validation.get("device_role", {}).get("role") - # Determine VM name - use pre-computed name if available (handles strip_domain) - vm_name = libre_device.get("_computed_name") + # Determine VM name - use pre-computed name if available (handles strip_domain), + # falling back to the validated resolved_name before recomputing from raw fields. + vm_name = libre_device.get("_computed_name") or validation.get("resolved_name") if not vm_name: vm_name = _determine_device_name( libre_device, use_sysname=use_sysname, - strip_domain=False, + strip_domain=strip_domain, device_id=libre_device.get("device_id"), ) @@ -58,15 +67,20 @@ def create_vm_from_librenms(libre_device: dict, validation: dict, use_sysname: b raise ValueError(f"device_id is a boolean ({raw_device_id!r}); expected an integer") librenms_device_id = int(raw_device_id) - # Create the VM with librenms_id custom field - vm = VirtualMachine.objects.create( - name=vm_name, - cluster=cluster, - role=role, # Optional VM role - platform=platform, - comments=f"Imported from LibreNMS (device_id={librenms_device_id}) by netbox-librenms-plugin on {import_time}", - custom_field_data={"librenms_id": librenms_device_id}, - ) + from ..utils import set_librenms_device_id + + # Create the VM and assign its LibreNMS ID atomically so a failure in + # set_librenms_device_id never leaves a VM without a mapping. + with transaction.atomic(): + vm = VirtualMachine.objects.create( + name=vm_name, + cluster=cluster, + role=role, + platform=platform, + comments=f"Imported from LibreNMS (device_id={librenms_device_id}) by netbox-librenms-plugin on {import_time}", + ) + set_librenms_device_id(vm, librenms_device_id, server_key) + vm.save() logger.info(f"Created VM {vm.name} (ID: {vm.pk}) from LibreNMS device {libre_device['device_id']}") return vm @@ -136,15 +150,27 @@ def bulk_import_vms( log = job.logger if job else logger for idx, vm_id in enumerate(vm_ids, start=1): - # Check for job cancellation every 5 VMs - if job and idx % 5 == 0: - job.job.refresh_from_db() - job_status = job.job.status - status_value = job_status.value if hasattr(job_status, "value") else job_status - if status_value in ("failed", "errored"): + # Check for job cancellation before first VM and every 5 thereafter + if job and (idx == 1 or idx % 5 == 0): + cancelled = False + try: + from django_rq import get_queue + from rq.job import Job as RQJob + + queue = get_queue("default") + rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) + if rq_job.is_failed or rq_job.is_stopped: + cancelled = True + except Exception: + job.job.refresh_from_db() + job_status = job.job.status + status_value = job_status.value if hasattr(job_status, "value") else job_status + if status_value in ("failed", "errored", "stopped"): + cancelled = True + if cancelled: log.warning(f"Job cancelled at VM {idx} of {len(vm_ids)}") break - log.info(f"Imported VM {idx} of {len(vm_ids)}") + log.info(f"Processing VM {idx} of {len(vm_ids)}") try: # Fetch device data (uses cache helper) @@ -169,6 +195,7 @@ def bulk_import_vms( api=api, use_sysname=use_sysname_opt, strip_domain=strip_domain_opt, + server_key=api.server_key, ) # Check if VM already exists @@ -191,21 +218,28 @@ def bulk_import_vms( cluster = Cluster.objects.filter(id=cluster_id).first() if cluster: apply_cluster_to_validation(validation, cluster) + else: + result["failed"].append( + {"device_id": vm_id, "error": f"Selected cluster (id={cluster_id}) no longer exists"} + ) + continue role = None if role_id: role = DeviceRole.objects.filter(id=role_id).first() if role: apply_role_to_validation(validation, role, is_vm=True) + else: + result["failed"].append( + {"device_id": vm_id, "error": f"Selected role (id={role_id}) no longer exists"} + ) + continue # Determine VM name - use_sysname = sync_options.get("use_sysname", True) if sync_options else True - strip_domain = sync_options.get("strip_domain", False) if sync_options else False - vm_name = _determine_device_name( libre_device, - use_sysname=use_sysname, - strip_domain=strip_domain, + use_sysname=use_sysname_opt, + strip_domain=strip_domain_opt, device_id=vm_id, ) @@ -213,7 +247,13 @@ def bulk_import_vms( libre_device["_computed_name"] = vm_name # Create VM - vm = create_vm_from_librenms(libre_device, validation, use_sysname=use_sysname, role=role) + vm = create_vm_from_librenms( + libre_device, + validation, + use_sysname=use_sysname_opt, + strip_domain=strip_domain_opt, + server_key=api.server_key, + ) result["success"].append( { diff --git a/netbox_librenms_plugin/jobs.py b/netbox_librenms_plugin/jobs.py index f56d564e46..d8aa638c73 100644 --- a/netbox_librenms_plugin/jobs.py +++ b/netbox_librenms_plugin/jobs.py @@ -111,7 +111,7 @@ def run( "device_ids": device_ids, "total_processed": len(validated_devices), "filters": filters, - "server_key": server_key, + "server_key": api.server_key, "vc_detection_enabled": vc_detection_enabled, "use_sysname": use_sysname, "strip_domain": strip_domain, @@ -166,6 +166,7 @@ def run( vm_imports, server_key=None, sync_options=None, + vc_detection_enabled=False, manual_mappings_per_device=None, libre_devices_cache=None, **kwargs, @@ -178,6 +179,7 @@ def run( vm_imports: Dict mapping device_id to cluster/role info for VM imports server_key: Optional LibreNMS server key for multi-server setups sync_options: Dict with sync_interfaces, sync_cables, sync_ips, use_sysname, strip_domain + vc_detection_enabled: Whether VC detection was enabled during the filter step. manual_mappings_per_device: Dict mapping device_id to manual_mappings dict libre_devices_cache: Optional dict mapping device_id to pre-fetched device data **kwargs: Additional job parameters @@ -208,10 +210,11 @@ def run( self.logger.info(f"Importing {len(device_ids)} devices...") device_result = bulk_import_devices_shared( device_ids=device_ids, - server_key=server_key, + server_key=api.server_key, sync_options=sync_options, manual_mappings_per_device=manual_mappings_per_device, libre_devices_cache=libre_devices_cache, + vc_detection_enabled=vc_detection_enabled, job=self, # Pass job context for logging and cancellation user=self.job.user, # Pass user for permission checks ) @@ -226,13 +229,25 @@ def run( vm_imports, api, sync_options, libre_devices_cache, job=self, user=self.job.user ) - # Combine results - imported_device_pks = [item["device"].pk for item in device_result.get("success", []) if item.get("device")] - imported_vm_pks = [item["device"].pk for item in vm_result.get("success", []) if item.get("device")] + # Combine results — partition device_result successes by model type since + # bulk_import_devices_shared() may return VirtualMachine objects when import_as_vm=True. + device_successes = [] + vm_successes = list(vm_result.get("success", [])) + for item in device_result.get("success", []): + obj = item.get("device") + if not obj: + continue + if obj._meta.model_name == "virtualmachine": + vm_successes.append(item) + else: + device_successes.append(item) + + imported_device_pks = [item["device"].pk for item in device_successes] + imported_vm_pks = [item["device"].pk for item in vm_successes] # Also store LibreNMS device IDs for re-rendering table rows - imported_libre_device_ids = [item["device_id"] for item in device_result.get("success", [])] - imported_libre_vm_ids = [item["device_id"] for item in vm_result.get("success", [])] + imported_libre_device_ids = [item["device_id"] for item in device_successes] + imported_libre_vm_ids = [item["device_id"] for item in vm_successes] success_count = len(device_result.get("success", [])) + len(vm_result.get("success", [])) failed_count = len(device_result.get("failed", [])) + len(vm_result.get("failed", [])) @@ -246,7 +261,7 @@ def run( "imported_vm_pks": imported_vm_pks, "imported_libre_device_ids": imported_libre_device_ids, "imported_libre_vm_ids": imported_libre_vm_ids, - "server_key": server_key, + "server_key": api.server_key, "total": total_count, "success_count": success_count, "failed_count": failed_count, diff --git a/netbox_librenms_plugin/librenms_api.py b/netbox_librenms_plugin/librenms_api.py index 811cc367d3..100b81a349 100644 --- a/netbox_librenms_plugin/librenms_api.py +++ b/netbox_librenms_plugin/librenms_api.py @@ -190,21 +190,16 @@ def get_librenms_id(self, obj): If found via API, stores ID in custom field if available, otherwise caches the value. """ - librenms_id = obj.cf.get("librenms_id") + from netbox_librenms_plugin.utils import get_librenms_device_id + + librenms_id = get_librenms_device_id(obj, self.server_key, auto_save=False) if librenms_id is not None: - if isinstance(librenms_id, str): - try: - librenms_id = int(librenms_id) - self._store_librenms_id(obj, librenms_id) - except (ValueError, TypeError): - librenms_id = None # empty or invalid string — fall through to discovery - if librenms_id is not None: - return librenms_id + return librenms_id # Check cache cache_key = self._get_cache_key(obj) librenms_id = cache.get(cache_key) - if librenms_id: + if librenms_id is not None: return librenms_id # Determine dynamically from API @@ -215,23 +210,38 @@ def get_librenms_id(self, obj): # Try IP address if ip_address: librenms_id = self.get_device_id_by_ip(ip_address) - if librenms_id: - self._store_librenms_id(obj, librenms_id) - return librenms_id + if librenms_id is not None: + try: + librenms_id = int(librenms_id) + except (ValueError, TypeError): + librenms_id = None + if librenms_id is not None: + self._store_librenms_id(obj, librenms_id) + return librenms_id # Try primary IP's DNS name if dns_name: librenms_id = self.get_device_id_by_hostname(dns_name) - if librenms_id: - self._store_librenms_id(obj, librenms_id) - return librenms_id + if librenms_id is not None: + try: + librenms_id = int(librenms_id) + except (ValueError, TypeError): + librenms_id = None + if librenms_id is not None: + self._store_librenms_id(obj, librenms_id) + return librenms_id # Try hostname if FQDN if hostname: librenms_id = self.get_device_id_by_hostname(hostname) - if librenms_id: - self._store_librenms_id(obj, librenms_id) - return librenms_id + if librenms_id is not None: + try: + librenms_id = int(librenms_id) + except (ValueError, TypeError): + librenms_id = None + if librenms_id is not None: + self._store_librenms_id(obj, librenms_id) + return librenms_id return None @@ -261,7 +271,9 @@ def _store_librenms_id(self, obj, librenms_id): None """ if "librenms_id" in obj.cf: - obj.custom_field_data["librenms_id"] = librenms_id + from netbox_librenms_plugin.utils import set_librenms_device_id + + set_librenms_device_id(obj, librenms_id, self.server_key) obj.save(update_fields=["custom_field_data"]) else: # Use cache as fallback @@ -288,7 +300,7 @@ def get_device_id_by_ip(self, ip_address): response.raise_for_status() device_data = response.json()["devices"][0] return device_data["device_id"] - except (requests.exceptions.RequestException, IndexError, KeyError): + except (requests.exceptions.RequestException, IndexError, KeyError, TypeError): return None def get_device_id_by_hostname(self, hostname): @@ -311,7 +323,7 @@ def get_device_id_by_hostname(self, hostname): response.raise_for_status() device_data = response.json()["devices"][0] return device_data["device_id"] - except (requests.exceptions.RequestException, IndexError, KeyError): + except (requests.exceptions.RequestException, IndexError, KeyError, TypeError): return None def get_device_info(self, device_id): @@ -336,7 +348,7 @@ def get_device_info(self, device_id): device_data = response.json()["devices"][0] return True, device_data return False, None - except requests.exceptions.RequestException: + except (requests.exceptions.RequestException, IndexError, KeyError, TypeError): return False, None def get_ports(self, device_id, with_vlans=True): @@ -627,9 +639,12 @@ def get_device_ips(self, device_id): verify=self.verify_ssl, ) response.raise_for_status() - if response.status_code == 200: - ip_data = response.json()["addresses"] - return True, ip_data + data = response.json() + addresses = data.get("addresses") if isinstance(data, dict) else None + if not isinstance(addresses, list): + message = data.get("message") if isinstance(data, dict) else None + return False, message or "Unexpected response format: 'addresses' must be a list" + return True, addresses except requests.exceptions.RequestException as e: return False, str(e) @@ -685,11 +700,13 @@ def get_device_inventory(self, device_id): verify=self.verify_ssl, ) response.raise_for_status() - - if response.status_code == 200: - inventory_data = response.json() - return True, inventory_data.get("inventory", []) - return False, [] + inventory_data = response.json() + inventory = inventory_data.get("inventory") if isinstance(inventory_data, dict) else None + if not isinstance(inventory, list): + msg = inventory_data.get("message", "") if isinstance(inventory_data, dict) else "" + logger.warning(f"Unexpected inventory response for device {device_id}: {inventory_data}") + return False, msg or "Unexpected response format: missing 'inventory' list" + return True, inventory except requests.exceptions.RequestException as e: return False, str(e) @@ -717,12 +734,15 @@ def get_poller_groups(self): verify=self.verify_ssl, ) response.raise_for_status() - - if response.status_code == 200: - result = response.json() - if result.get("status") == "ok": - return True, result.get("get_poller_group", []) - return False, [] + result = response.json() + if isinstance(result, dict) and result.get("status") == "ok": + poller_groups = result.get("get_poller_group") + if not isinstance(poller_groups, list): + return False, result.get("message") or "Unexpected response format: missing 'get_poller_group' list" + return True, poller_groups + if isinstance(result, dict): + return False, result.get("message") or "Unexpected response format" + return False, "Unexpected response format: non-object JSON" except requests.exceptions.RequestException as e: return False, str(e) @@ -767,15 +787,17 @@ def get_inventory_filtered(self, device_id, ent_physical_class=None, ent_physica ) response.raise_for_status() - if response.status_code == 200: - data = response.json() - if data.get("status") == "ok": - inventory = data.get("inventory", []) - logger.debug(f"API returned {len(inventory)} items") + data = response.json() + if isinstance(data, dict) and data.get("status") == "ok": + inventory = data.get("inventory") if isinstance(data, dict) else None + if not isinstance(inventory, list): + msg = data.get("message") if isinstance(data, dict) else None + return False, msg or "Unexpected response format: missing 'inventory' list" + logger.debug(f"API returned {len(inventory)} items") - # If we got results or didn't specify filters, return - if inventory or not params: - return True, inventory + # If we got results or didn't specify filters, return + if inventory or not params: + return True, inventory # If filtered endpoint returned empty but we have filters, # try /all endpoint and filter client-side @@ -784,7 +806,7 @@ def get_inventory_filtered(self, device_id, ent_physical_class=None, ent_physica success, all_inventory = self.get_device_inventory(device_id) if not success: - return False, [] + return False, all_inventory # Apply client-side filters filtered = all_inventory @@ -799,11 +821,13 @@ def get_inventory_filtered(self, device_id, ent_physical_class=None, ent_physica return True, filtered - return False, [] + return False, data.get("message", "Unexpected response format") if isinstance( + data, dict + ) else "Unexpected response format" except requests.exceptions.RequestException as e: logger.warning(f"Failed to fetch filtered inventory: {e}") - return False, [] + return False, str(e) def list_devices(self, filters=None): """ @@ -863,12 +887,17 @@ def list_devices(self, filters=None): verify=self.verify_ssl, ) response.raise_for_status() - if response.status_code == 200: - result = response.json() - if result.get("status") == "ok": - return True, result.get("devices", []) - - return False, [] + result = response.json() + if isinstance(result, dict) and result.get("status") == "ok": + devices = result.get("devices") + if not isinstance(devices, list): + msg = result.get("message") + return False, msg or "Unexpected response format: missing 'devices' list" + return True, devices + + return False, result.get("message", "Unexpected response format") if isinstance( + result, dict + ) else "Unexpected response format" except requests.exceptions.RequestException as e: return False, str(e) @@ -911,16 +940,20 @@ def get_device_vlans(self, device_id: int) -> tuple[bool, list | str]: ) response.raise_for_status() - if response.status_code == 200: - result = response.json() - if result.get("status") == "ok": - # Filter VLANs by device_id since resources endpoint returns all VLANs - all_vlans = result.get("vlans", []) - device_vlans = [v for v in all_vlans if str(v.get("device_id")) == str(device_id)] - return True, device_vlans + result = response.json() + if isinstance(result, dict) and result.get("status") == "ok": + all_vlans = result.get("vlans") if isinstance(result, dict) else None + if not isinstance(all_vlans, list): + msg = result.get("message") if isinstance(result, dict) else None + return False, msg or "Unexpected response format: missing 'vlans' list" + # Filter VLANs by device_id since resources endpoint returns all VLANs + device_vlans = [ + v for v in all_vlans if isinstance(v, dict) and str(v.get("device_id")) == str(device_id) + ] + return True, device_vlans + if isinstance(result, dict): return False, result.get("message", "Unexpected response format") - - return False, f"HTTP {response.status_code}" + return False, "Unexpected response format" except requests.exceptions.HTTPError as e: if e.response.status_code == 404: return False, "VLANs resource not found" @@ -965,6 +998,8 @@ def get_port_vlan_details(self, port_id: int) -> tuple[bool, dict | str]: if response.status_code == 200: result = response.json() + if not isinstance(result, dict): + return False, "Unexpected response format" port_data = result.get("port", []) if port_data and len(port_data) > 0: return True, port_data[0] @@ -1019,7 +1054,15 @@ def parse_port_vlan_data(self, port_data: dict, interface_name_field: str = "ifN if vlans_data: # Parse from detailed vlans array for vlan_entry in vlans_data: + if not isinstance(vlan_entry, dict): + continue vlan_id = vlan_entry.get("vlan") + if vlan_id is None: + continue + try: + vlan_id = int(vlan_id) + except (ValueError, TypeError): + continue if vlan_entry.get("untagged") == 1: untagged_vlan = vlan_id else: diff --git a/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js b/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js index c56f9dee97..5b020bb366 100644 --- a/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js +++ b/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js @@ -544,74 +544,80 @@ function initializeVlanModalSave() { ? document.querySelectorAll('.vlan-edit-btn') : document.querySelectorAll(`.vlan-edit-btn[data-safe-name="${currentSafeName}"]`); - buttonsToUpdate.forEach(btn => { - try { - const btnVlans = JSON.parse(btn.dataset.vlans); - const groups = JSON.parse(btn.dataset.vlanGroups); - const btnSafeName = btn.dataset.safeName; - let changed = false; - - btnVlans.forEach(v => { - if (vidGroupMap.hasOwnProperty(String(v.vid))) { - const newGroupId = vidGroupMap[String(v.vid)]; - v.group_id = newGroupId; - if (v.missing) { - v.group_name = 'Not in NetBox'; - } else { - const matchedGroup = groups.find(g => String(g.id) === String(newGroupId)); - v.group_name = matchedGroup ? matchedGroup.name : '-- No Group (Global) --'; - } + // Apply DOM mutations (btn.dataset.vlans, hidden inputs, summary spans) + // Called only after a successful server response when persisting, or immediately otherwise. + function applyButtonUpdates() { + buttonsToUpdate.forEach(btn => { + try { + const btnVlans = JSON.parse(btn.dataset.vlans); + const groups = JSON.parse(btn.dataset.vlanGroups); + const btnSafeName = btn.dataset.safeName; + let changed = false; + + btnVlans.forEach(v => { + if (vidGroupMap.hasOwnProperty(String(v.vid))) { + const newGroupId = vidGroupMap[String(v.vid)]; + v.group_id = newGroupId; + + // Apply resolved missing/css state BEFORE computing group_name + // so group_name reflects the verified state from the server. + if (vidCssMap.hasOwnProperty(String(v.vid))) { + v.css = vidCssMap[String(v.vid)]; + v.missing = vidMissingMap[String(v.vid)] || false; + } - // Apply resolved CSS from verify endpoint if available - if (vidCssMap.hasOwnProperty(String(v.vid))) { - v.css = vidCssMap[String(v.vid)]; - v.missing = vidMissingMap[String(v.vid)] || false; - } + if (v.missing) { + v.group_name = 'Not in NetBox'; + } else { + const matchedGroup = groups.find(g => String(g.id) === String(newGroupId)); + v.group_name = matchedGroup ? matchedGroup.name : '-- No Group (Global) --'; + } - changed = true; + changed = true; - // Update the hidden input for this VID on this interface - const input = document.querySelector( - `input.vlan-group-hidden[name="vlan_group_${btnSafeName}_${v.vid}"]` - ); - if (input) { - input.value = newGroupId; + // Update the hidden input for this VID on this interface + const input = document.querySelector( + `input.vlan-group-hidden[name="vlan_group_${btnSafeName}_${v.vid}"]` + ); + if (input) { + input.value = newGroupId; + } } - } - }); + }); - if (changed) { - btn.dataset.vlans = JSON.stringify(btnVlans); - // Update the tooltip and re-render inline summary colors - const summarySpan = btn.previousElementSibling; - if (summarySpan && summarySpan.tagName === 'SPAN') { - const tooltipLines = btnVlans.map(v => - v.missing - ? `VLAN ${v.vid}(${v.type}) \u2192 \u26A0 Not in NetBox` - : `VLAN ${v.vid}(${v.type}) \u2192 ${v.group_name}` - ); - summarySpan.title = tooltipLines.join('\n'); - - // Re-render inline VLAN summary with correct colors - const MAX_INLINE = 3; - const inlineParts = btnVlans.slice(0, MAX_INLINE).map(v => { - const warning = v.missing - ? ' ' - : ''; - return `${v.vid}(${v.type})${warning}`; - }); - let html = inlineParts.join(', '); - if (btnVlans.length > MAX_INLINE) { - const extra = btnVlans.length - MAX_INLINE; - html += ` +${extra} more`; + if (changed) { + btn.dataset.vlans = JSON.stringify(btnVlans); + // Update the tooltip and re-render inline summary colors + const summarySpan = btn.previousElementSibling; + if (summarySpan && summarySpan.tagName === 'SPAN') { + const tooltipLines = btnVlans.map(v => + v.missing + ? `VLAN ${v.vid}(${v.type}) \u2192 \u26A0 Not in NetBox` + : `VLAN ${v.vid}(${v.type}) \u2192 ${v.group_name}` + ); + summarySpan.title = tooltipLines.join('\n'); + + // Re-render inline VLAN summary with correct colors + const MAX_INLINE = 3; + const inlineParts = btnVlans.slice(0, MAX_INLINE).map(v => { + const warning = v.missing + ? ' ' + : ''; + return `${v.vid}(${v.type})${warning}`; + }); + let html = inlineParts.join(', '); + if (btnVlans.length > MAX_INLINE) { + const extra = btnVlans.length - MAX_INLINE; + html += ` +${extra} more`; + } + summarySpan.innerHTML = html; } - summarySpan.innerHTML = html; } + } catch (e) { + // Skip buttons with invalid data } - } catch (e) { - // Skip buttons with invalid data - } - }); + }); + } // Persist overrides in server cache so other table pages pick them up if (applyToAll && Object.keys(vidGroupMap).length > 0) { @@ -624,7 +630,8 @@ function initializeVlanModalSave() { }, body: JSON.stringify({ device_id: deviceId, - vid_group_map: vidGroupMap + vid_group_map: vidGroupMap, + server_key: document.querySelector('input[name="server_key"]')?.value || null }) }).then(response => { if (!response.ok) { @@ -634,6 +641,8 @@ function initializeVlanModalSave() { throw new Error(msg); }); } + // Apply DOM mutations only after the server has persisted the overrides + applyButtonUpdates(); // Close modal only on success const closeBtn = modalEl.querySelector('[data-bs-dismiss="modal"]'); if (closeBtn) { @@ -650,7 +659,8 @@ function initializeVlanModalSave() { alertEl.textContent = 'Failed to save VLAN group overrides: ' + error.message; }); } else { - // No server persist needed — close immediately + // No server persist needed — apply DOM mutations and close immediately + applyButtonUpdates(); const closeBtn = modalEl.querySelector('[data-bs-dismiss="modal"]'); if (closeBtn) { closeBtn.click(); @@ -774,7 +784,8 @@ function handleVRFChange(select, value) { body: JSON.stringify({ device_id: deviceId, ip_address: fullIpAddress, // Use full IP address with prefix - vrf_id: value + vrf_id: value, + server_key: document.querySelector('input[name="server_key"]')?.value || null }) }) .then(response => { @@ -813,7 +824,8 @@ function handleInterfaceChange(select, value) { body: JSON.stringify({ device_id: value, interface_name: select.dataset.interface, - interface_name_field: document.querySelector('input[name="interface_name_field"]:checked')?.value || null + interface_name_field: document.querySelector('input[name="interface_name_field"]:checked')?.value || null, + server_key: document.querySelector('input[name="server_key"]')?.value || null }) }) .then(response => { @@ -859,7 +871,8 @@ function handleCableChange(select, value) { }, body: JSON.stringify({ device_id: value, - local_port_id: select.dataset.interface + local_port_id: select.dataset.interface, + server_key: document.querySelector('input[name="server_key"]')?.value || null }) }) .then(response => { @@ -1434,6 +1447,156 @@ function initializeSyncFormSpinners() { }); } + +/** + * Wire the "Install Selected" form to collect checked module-table rows before submit. + * The form is separate from the table (to avoid nested forms), so we copy the + * selected checkbox values into hidden inputs just before the form is submitted. + * Guard against duplicate listeners on repeated HTMX swaps via a data attribute. + */ +function handleInstallSelectedSubmit() { + // Remove any previously-injected hidden inputs to avoid duplicates + const form = document.getElementById('install-selected-form'); + if (!form) return; + form.querySelectorAll('input[data-injected-select]').forEach(el => { el.remove(); }); + + const table = document.getElementById('librenms-module-table'); + if (!table) return; + + table.querySelectorAll('input[name="select"]:checked').forEach(cb => { + const hidden = document.createElement('input'); + hidden.type = 'hidden'; + hidden.name = 'select'; + hidden.value = cb.value; + hidden.dataset.injectedSelect = '1'; + form.appendChild(hidden); + }); +} + +function initializeInstallSelectedForm() { + const form = document.getElementById('install-selected-form'); + if (!form) return; + if (form.dataset.installInit) return; + form.dataset.installInit = 'true'; + form.addEventListener('submit', handleInstallSelectedSubmit); +} + +/** + * Tracks the in-flight AbortController for the module replace preview fetch. + * Cancelled when a new Replace button is clicked before the previous fetch completes. + */ +let _activeReplaceController = null; + +/** + * Initialize Replace buttons on the module sync table. + * Each button carries module/ent_index/server_key as data attributes and opens + * the mismatch comparison modal by fetching the preview fragment from the server. + */ +function initializeModuleReplaceButtons() { + document.querySelectorAll('.module-replace-btn').forEach(btn => { + if (btn.dataset.replaceInitialized) return; + btn.dataset.replaceInitialized = 'true'; + + btn.addEventListener('click', function () { + // Cancel any in-flight preview request before starting a new one + if (_activeReplaceController) { + _activeReplaceController.abort(); + } + _activeReplaceController = new AbortController(); + const signal = _activeReplaceController.signal; + + const previewUrl = this.dataset.previewUrl; + const moduleId = this.dataset.moduleId; + const entIndex = this.dataset.entIndex; + const serverKey = this.dataset.serverKey; + + const params = new URLSearchParams({ + module_id: moduleId, + ent_index: entIndex, + server_key: serverKey, + }); + + // Show shared HTMX modal with loading state + const modalContent = document.getElementById('htmx-modal-content'); + if (modalContent) { + modalContent.innerHTML = + '' + + ''; + } + + // Show modal using direct class manipulation (consistent with openBulkVCModal) + const htmxModal = document.getElementById('htmx-modal'); + if (htmxModal) { + htmxModal.classList.add('show'); + htmxModal.style.display = 'block'; + htmxModal.setAttribute('aria-modal', 'true'); + htmxModal.removeAttribute('aria-hidden'); + let backdrop = document.querySelector('.modal-backdrop'); + if (!backdrop) { + backdrop = document.createElement('div'); + backdrop.className = 'modal-backdrop fade show'; + document.body.appendChild(backdrop); + } + document.body.classList.add('modal-open'); + } + + // Fetch preview content and inject into modal body + fetch(`${previewUrl}?${params.toString()}`, { + signal, + headers: { 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value }, + }) + .then(response => { + if (!response.ok) return response.text().then(t => { throw new Error(t); }); + return response.text(); + }) + .then(html => { + const modalBody = document.getElementById('htmx-modal-body'); + if (modalBody) { + modalBody.innerHTML = html; + } + }) + .catch(err => { + if (err.name === 'AbortError') return; // Superseded by a newer click — ignore + const modalBody = document.getElementById('htmx-modal-body'); + if (modalBody) { + const alert = document.createElement('div'); + alert.className = 'alert alert-danger'; + const icon = document.createElement('i'); + icon.className = 'mdi mdi-alert me-1'; + alert.appendChild(icon); + alert.appendChild(document.createTextNode(err.message || 'Failed to load preview.')); + modalBody.textContent = ''; + modalBody.appendChild(alert); + } + }); + }); + }); +} + +function closeHtmxModal() { + // Abort any in-flight module-replace preview request + if (typeof _activeReplaceController !== 'undefined' && _activeReplaceController) { + _activeReplaceController.abort(); + _activeReplaceController = null; + } + const htmxModal = document.getElementById('htmx-modal'); + if (htmxModal) { + htmxModal.classList.remove('show'); + htmxModal.style.display = 'none'; + htmxModal.setAttribute('aria-hidden', 'true'); + htmxModal.removeAttribute('aria-modal'); + const backdrop = document.querySelector('.modal-backdrop'); + if (backdrop) backdrop.remove(); + document.body.classList.remove('modal-open'); + } +} + // ============================================ // INITIALIZATION // ============================================ @@ -1458,6 +1621,8 @@ function initializeScripts() { initializeNetBoxOnlyInterfaces(); initializeSyncFormSpinners(); initializeVlanSyncGroupSelects(); + initializeInstallSelectedForm(); + initializeModuleReplaceButtons(); } diff --git a/netbox_librenms_plugin/tables/device_status.py b/netbox_librenms_plugin/tables/device_status.py index da5b02d0bf..995a623ec4 100644 --- a/netbox_librenms_plugin/tables/device_status.py +++ b/netbox_librenms_plugin/tables/device_status.py @@ -471,12 +471,12 @@ def render_actions(self, value, record): btn_class = "btn-outline-danger" btn_icon = "mdi-alert-circle" btn_label = " Conflict" - btn_title = "Resolve conflict" + btn_title = "View conflict details" elif has_actions: btn_class = "btn-outline-warning" btn_icon = "mdi-alert" btn_label = " Conflict" - btn_title = "Resolve conflict" + btn_title = "View conflict details" elif has_name_sync or has_sync_needed: btn_class = "btn-outline-warning" btn_icon = "mdi-information-outline" @@ -486,7 +486,7 @@ def render_actions(self, value, record): btn_class = "btn-outline-warning" btn_icon = "mdi-database-alert" btn_label = " Legacy ID" - btn_title = "Migrate Legacy ID" + btn_title = "View legacy ID migration details" else: btn_class = "btn-outline-success" btn_icon = "mdi-check-circle" diff --git a/netbox_librenms_plugin/tables/interfaces.py b/netbox_librenms_plugin/tables/interfaces.py index 5ceb30bf51..1216313da4 100644 --- a/netbox_librenms_plugin/tables/interfaces.py +++ b/netbox_librenms_plugin/tables/interfaces.py @@ -13,6 +13,7 @@ convert_speed_to_kbps, format_mac_address, get_interface_name_field, + get_librenms_device_id, get_missing_vlan_warning, get_table_paginate_count, get_tagged_vlan_css_class, @@ -46,11 +47,12 @@ class Meta: "id": "librenms-interface-table", } - def __init__(self, *args, device=None, interface_name_field=None, vlan_groups=None, **kwargs): + def __init__(self, *args, device=None, interface_name_field=None, vlan_groups=None, server_key="default", **kwargs): """Initialize table with device context and interface name field.""" self.device = device self.interface_name_field = interface_name_field or get_interface_name_field() self.vlan_groups = vlan_groups or [] + self.server_key = server_key # Update column accessors after initialization for column in ["selection", "name"]: @@ -149,7 +151,7 @@ def render_vlans(self, value, record): all_vlans.append(("T", vid)) if not all_vlans: - return format_html("—") + return mark_safe("—") interface_name = record.get(self.interface_name_field, "") safe_name = interface_name.replace("/", "_").replace(":", "_") @@ -360,7 +362,7 @@ def render_librenms_id(self, value, record): if not netbox_interface: return mark_safe(f'{value}') - netbox_librenms_id = netbox_interface.custom_field_data.get("librenms_id") + netbox_librenms_id = get_librenms_device_id(netbox_interface, self.server_key, auto_save=False) if netbox_librenms_id is None: return mark_safe( @@ -467,7 +469,7 @@ def render_mapping_tooltip(self, value, speed, mapping): ) else: display = value - icon = format_html('') + icon = mark_safe('') return display, icon def format_interface_data(self, port_data, device): diff --git a/netbox_librenms_plugin/tables/ipaddresses.py b/netbox_librenms_plugin/tables/ipaddresses.py index c076d54ba8..8c9e1bfe21 100644 --- a/netbox_librenms_plugin/tables/ipaddresses.py +++ b/netbox_librenms_plugin/tables/ipaddresses.py @@ -1,5 +1,5 @@ import django_tables2 as tables -from django.utils.html import format_html +from django.utils.html import format_html, mark_safe from netbox.tables.columns import ToggleColumn from utilities.paginator import EnhancedPaginator @@ -91,14 +91,14 @@ def render_status(self, value, record): record["ip_address"], ) elif value == "matched": - return format_html(' Synced') + return mark_safe(' Synced') elif record.get("interface_url"): return format_html( '', record["ip_address"], ) - return format_html('Missing NetBox Object') + return mark_safe('Missing NetBox Object') def render_device(self, value, record): """Render the device column with a link if available""" diff --git a/netbox_librenms_plugin/tables/vlans.py b/netbox_librenms_plugin/tables/vlans.py index 75c2f5e771..1b078b87b9 100644 --- a/netbox_librenms_plugin/tables/vlans.py +++ b/netbox_librenms_plugin/tables/vlans.py @@ -1,5 +1,5 @@ import django_tables2 as tables -from django.utils.html import format_html +from django.utils.html import format_html, format_html_join from django.utils.safestring import mark_safe from netbox.tables.columns import ToggleColumn from utilities.paginator import EnhancedPaginator @@ -124,12 +124,30 @@ def render_vlan_group_selection(self, value, record): # Priority 2: unique VID match selected_group_id = record["auto_selected_group_id"] - # Build the select element - options = [''] - for group in self.vlan_groups: - selected = "selected" if group.pk == selected_group_id else "" - scope_info = f" ({group.scope})" if group.scope else "" - options.append(f'') + # Build the select element using format_html_join to prevent XSS + options_html = format_html_join( + "", + '', + [ + ( + "", + "", + "", + "-- No Group (Global) --", + "", + ), + ] + + [ + ( + group.pk, + group.scope_id if group.scope_id else "", + " selected" if group.pk == selected_group_id else "", + group.name, + f" ({group.scope})" if group.scope else "", + ) + for group in self.vlan_groups + ], + ) select_html = format_html( '{% endif %}
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 3d7e1406cd..c8a51bdcad 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 @@ -9,6 +9,7 @@ action="{% url 'plugins:netbox_librenms_plugin:sync_selected_interfaces' object_type=model_name object_id=interface_sync.object.pk %}?interface_name_field={{ interface_name_field }}"> {% endwith %} {% csrf_token %} + {% if interface_sync.server_key %}{% endif %} {% block table_actions %}
diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.html index a31114de1f..e73bb83961 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.html @@ -7,6 +7,7 @@
{% endwith %} {% csrf_token %} + {% if ip_sync.server_key %}{% endif %}
diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html index 017f888ec1..8221f29e38 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html @@ -21,6 +21,7 @@ action="{% url 'plugins:netbox_librenms_plugin:sync_selected_vlans' object_type=model_name object_id=vlan_sync.object.pk %}"> {% endwith %} {% csrf_token %} + {% if vlan_sync.server_key %}{% endif %}
diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html index 8141c846d0..ba460b6bb0 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html @@ -27,7 +27,7 @@
{{ validation.existing_device.name }} - {% if validation.name_sync_available and existing_device_model_name != "virtualmachine" %} + {% if validation.name_sync_available %} {% csrf_token %} + + +
+ {% endif %} {% elif validation.existing_match_type == 'hostname' %}
@@ -432,6 +475,7 @@
hx-include="#use-sysname-toggle, #strip-domain-toggle"> {% csrf_token %} + {% if validation.device_type_mismatch %}
hx-include="#use-sysname-toggle, #strip-domain-toggle"> {% csrf_token %} + {% if validation.device_type_mismatch %}