Feat/oob sync#295
Closed
marcinpsk wants to merge 120 commits into
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds OOB detection and linkage handling, bulk import collision detection, OOB reconciliation actions, and stage-2 move-to-winner migration endpoints. Sync views are now server-key scoped and merge OOB controller data into rendered results. Templates, tables, and JavaScript render OOB states and nested modal flows. Utilities, routes, docs, and tests were expanded to support dict-form Sequence Diagram(s)sequenceDiagram
participant User
participant ValidationView
participant ImportActions
participant NetBoxDB
participant LibreNMSAPI
User->>ValidationView: open validation details
ValidationView->>LibreNMSAPI: fetch device / interface data
ValidationView->>NetBoxDB: resolve librenms_id / OOB linkage
ValidationView-->>User: show OOB, promote, or merge actions
User->>ImportActions: POST add-as-OOB or promote-to-host
ImportActions->>NetBoxDB: update librenms_id and device linkage
ImportActions-->>User: HTMX response / toast
User->>ImportActions: POST merge devices
ImportActions->>NetBoxDB: merge links and stamp migration marker
ImportActions-->>User: refresh response
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
docs/usage_tips/mapping_rules.md (2)
3-3: 💤 Low valueConsider adding a comma for clarity.
The sentence "with them you can cover vendor naming variations..." would read more clearly as "with them, you can cover vendor naming variations...".
✍️ Suggested improvement
-Mapping rules are the configuration layer that connects LibreNMS identifiers to NetBox objects. Without them the plugin relies on exact-string matching; with them you can cover vendor naming variations, OS string aliases, model-number differences, and bay naming schemes across your entire fleet. +Mapping rules are the configuration layer that connects LibreNMS identifiers to NetBox objects. Without them the plugin relies on exact-string matching; with them, you can cover vendor naming variations, OS string aliases, model-number differences, and bay naming schemes across your entire fleet.
195-195: 💤 Low valueConsider adding article for clarity.
The phrase "to have bay to live in" would be clearer as "to have a bay to live in".
✍️ Suggested improvement
-Some chassis report child components (CPMs, MDAs, mezzanines) without the intermediate carrier/holder module that must first exist in NetBox for those children to have bay to live in. A Carrier Auto-Install Rule tells the plugin: "for this manufacturer / device type, when you see an orphan component of this class and name pattern, suggest installing this ModuleType into the matching empty bay." +Some chassis report child components (CPMs, MDAs, mezzanines) without the intermediate carrier/holder module that must first exist in NetBox for those children to have a bay to live in. A Carrier Auto-Install Rule tells the plugin: "for this manufacturer / device type, when you see an orphan component of this class and name pattern, suggest installing this ModuleType into the matching empty bay."netbox_librenms_plugin/tests/test_coverage_device_operations.py (1)
1692-1720: ⚡ Quick winPatch VC detection in the shared OOB test fixture to keep these tests deterministic.
validate_device_for_import()will still execute VC detection in these tests (api is set, vc detection defaults on), which makes OOB assertions depend on unrelated logic. Add a defaultget_virtual_chassis_datapatch in_base_patches()so these tests stay scoped and stable.Proposed diff
def _base_patches(self, mock_device_cls, mock_vm_cls=None): @@ return [ patch("netbox_librenms_plugin.import_utils.device_operations.Site"), patch("netbox_librenms_plugin.import_utils.device_operations.DeviceType"), patch("netbox_librenms_plugin.import_utils.device_operations.DeviceRole"), patch("netbox_librenms_plugin.import_utils.device_operations.cache"), patch("ipam.models.IPAddress"), patch("virtualization.models.VirtualMachine", new=mock_vm_cls), + patch( + "netbox_librenms_plugin.import_utils.device_operations.get_virtual_chassis_data", + return_value={"is_stack": False, "member_count": 0, "members": [], "detection_error": None}, + ), patch( "netbox_librenms_plugin.import_utils.device_operations.match_librenms_hardware_to_device_type", return_value={"matched": False}, ),netbox_librenms_plugin/tests/test_migrate_views.py (1)
275-277: ⚡ Quick winTighten save-call assertions for partial-field writes.
On Line 276 and Line 277,
assert_called_once()is too broad; this path should explicitly enforceupdate_fields=["oob_ip"]on both saves, matching the view’s concurrency/validation-safe contract.Proposed test tightening
- winner.save.assert_called_once() - donor.save.assert_called_once() + winner.save.assert_called_once_with(update_fields=["oob_ip"]) + donor.save.assert_called_once_with(update_fields=["oob_ip"])
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76d85bcd-7195-40c2-a3ac-ed4764a00939
⛔ Files ignored due to path filters (13)
docs/img/Netbox-librenms-plugin-device-sync-fields.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-import-page.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-module-sync-tab.pngis excluded by!**/*.pngdocs/img/carrier_auto_install_rules/list.pngis excluded by!**/*.pngdocs/img/device_type_mappings/list.pngis excluded by!**/*.pngdocs/img/inventory_ignore_rules/list.pngis excluded by!**/*.pngdocs/img/module_bay_mappings/list.pngis excluded by!**/*.pngdocs/img/module_type_mappings/add.pngis excluded by!**/*.pngdocs/img/module_type_mappings/list.pngis excluded by!**/*.pngdocs/img/normalization_rules/add.pngis excluded by!**/*.pngdocs/img/normalization_rules/list.pngis excluded by!**/*.pngdocs/img/platform_mappings/add.pngis excluded by!**/*.pngdocs/img/platform_mappings/list.pngis excluded by!**/*.png
📒 Files selected for processing (49)
docs/README.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/README.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/migrate.py
Contributor
Author
|
hi @bonzo81 - this probably could be split in 2 - as it has some UI improvements that could be separate - I will do that as I see CR has a lot to say on this one - will switch to draft for now. I'd expect to rebase that on top of your changes around module/interface sync, when that would be ready/in develop. |
Owner
|
Thanks @marcinpsk sounds fair enough. CR certainly was busy! Just created PR #296 as a module sync follow up |
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
May 25, 2026
- utils.py: use _type_normalized in OOB_TYPE_PATTERN.search to avoid TypeError when oob_type is None - device_status.py: coerce paired_oob_id/paired_host_id to int before comparison to handle mixed str/int values - device_operations.py: add created_ips:[] to all failure return paths in import_single_device for consistent response schema - cables_view.py: resolve OOB mapping from VC sync device instead of raw obj so VC pages load controller LLDP rows correctly - interfaces_view.py: guard chassis_member None for OOB port names that don't resolve to a VC member position - modules_view.py: use sync_device instead of obj when reading OOB mapping so VC pages find the mapping on the correct member - actions.py: only re-check change permission for concurrent PlatformMapping creation when the row actually needs updating (not a no-op) - list.py: replace inline auto_create_ipam fallback with resolve_auto_create_ipam() to honour request POST/GET toggle - librenms_import.js: align Bootstrap global detection order in validationRefresh handler to match rest of plugin - templates: log error.message in fetch catch blocks for diagnosability - test_collisions.py: use token-based set assertion to avoid substring false positives on role strings
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
May 25, 2026
- utils.py: use _type_normalized in OOB_TYPE_PATTERN.search to avoid TypeError when oob_type is None - device_status.py: coerce paired_oob_id/paired_host_id to int before comparison to handle mixed str/int values - device_operations.py: add created_ips:[] to all failure return paths in import_single_device for consistent response schema - cables_view.py: resolve OOB mapping from VC sync device instead of raw obj so VC pages load controller LLDP rows correctly - interfaces_view.py: guard chassis_member None for OOB port names that don't resolve to a VC member position - modules_view.py: use sync_device instead of obj when reading OOB mapping so VC pages find the mapping on the correct member - actions.py: only re-check change permission for concurrent PlatformMapping creation when the row actually needs updating (not a no-op) - list.py: replace inline auto_create_ipam fallback with resolve_auto_create_ipam() to honour request POST/GET toggle - librenms_import.js: align Bootstrap global detection order in validationRefresh handler to match rest of plugin - templates: log error.message in fetch catch blocks for diagnosability - test_collisions.py: use token-based set assertion to avoid substring false positives on role strings
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
May 27, 2026
- utils.py: use _type_normalized in OOB_TYPE_PATTERN.search to avoid TypeError when oob_type is None - device_status.py: coerce paired_oob_id/paired_host_id to int before comparison to handle mixed str/int values - device_operations.py: add created_ips:[] to all failure return paths in import_single_device for consistent response schema - cables_view.py: resolve OOB mapping from VC sync device instead of raw obj so VC pages load controller LLDP rows correctly - interfaces_view.py: guard chassis_member None for OOB port names that don't resolve to a VC member position - modules_view.py: use sync_device instead of obj when reading OOB mapping so VC pages find the mapping on the correct member - actions.py: only re-check change permission for concurrent PlatformMapping creation when the row actually needs updating (not a no-op) - list.py: replace inline auto_create_ipam fallback with resolve_auto_create_ipam() to honour request POST/GET toggle - librenms_import.js: align Bootstrap global detection order in validationRefresh handler to match rest of plugin - templates: log error.message in fetch catch blocks for diagnosability - test_collisions.py: use token-based set assertion to avoid substring false positives on role strings
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
May 29, 2026
- utils.py: use _type_normalized in OOB_TYPE_PATTERN.search to avoid TypeError when oob_type is None - device_status.py: coerce paired_oob_id/paired_host_id to int before comparison to handle mixed str/int values - device_operations.py: add created_ips:[] to all failure return paths in import_single_device for consistent response schema - cables_view.py: resolve OOB mapping from VC sync device instead of raw obj so VC pages load controller LLDP rows correctly - interfaces_view.py: guard chassis_member None for OOB port names that don't resolve to a VC member position - modules_view.py: use sync_device instead of obj when reading OOB mapping so VC pages find the mapping on the correct member - actions.py: only re-check change permission for concurrent PlatformMapping creation when the row actually needs updating (not a no-op) - list.py: replace inline auto_create_ipam fallback with resolve_auto_create_ipam() to honour request POST/GET toggle - librenms_import.js: align Bootstrap global detection order in validationRefresh handler to match rest of plugin - templates: log error.message in fetch catch blocks for diagnosability - test_collisions.py: use token-based set assertion to avoid substring false positives on role strings
marcinpsk
force-pushed
the
feat/oob-sync
branch
5 times, most recently
from
0521e1f to
7826081
Compare
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
May 31, 2026
- utils.py: use _type_normalized in OOB_TYPE_PATTERN.search to avoid TypeError when oob_type is None - device_status.py: coerce paired_oob_id/paired_host_id to int before comparison to handle mixed str/int values - device_operations.py: add created_ips:[] to all failure return paths in import_single_device for consistent response schema - cables_view.py: resolve OOB mapping from VC sync device instead of raw obj so VC pages load controller LLDP rows correctly - interfaces_view.py: guard chassis_member None for OOB port names that don't resolve to a VC member position - modules_view.py: use sync_device instead of obj when reading OOB mapping so VC pages find the mapping on the correct member - actions.py: only re-check change permission for concurrent PlatformMapping creation when the row actually needs updating (not a no-op) - list.py: replace inline auto_create_ipam fallback with resolve_auto_create_ipam() to honour request POST/GET toggle - librenms_import.js: align Bootstrap global detection order in validationRefresh handler to match rest of plugin - templates: log error.message in fetch catch blocks for diagnosability - test_collisions.py: use token-based set assertion to avoid substring false positives on role strings
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
May 31, 2026
- utils.py: use _type_normalized in OOB_TYPE_PATTERN.search to avoid TypeError when oob_type is None - device_status.py: coerce paired_oob_id/paired_host_id to int before comparison to handle mixed str/int values - device_operations.py: add created_ips:[] to all failure return paths in import_single_device for consistent response schema - cables_view.py: resolve OOB mapping from VC sync device instead of raw obj so VC pages load controller LLDP rows correctly - interfaces_view.py: guard chassis_member None for OOB port names that don't resolve to a VC member position - modules_view.py: use sync_device instead of obj when reading OOB mapping so VC pages find the mapping on the correct member - actions.py: only re-check change permission for concurrent PlatformMapping creation when the row actually needs updating (not a no-op) - list.py: replace inline auto_create_ipam fallback with resolve_auto_create_ipam() to honour request POST/GET toggle - librenms_import.js: align Bootstrap global detection order in validationRefresh handler to match rest of plugin - templates: log error.message in fetch catch blocks for diagnosability - test_collisions.py: use token-based set assertion to avoid substring false positives on role strings
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
Jun 1, 2026
- utils.py: use _type_normalized in OOB_TYPE_PATTERN.search to avoid TypeError when oob_type is None - device_status.py: coerce paired_oob_id/paired_host_id to int before comparison to handle mixed str/int values - device_operations.py: add created_ips:[] to all failure return paths in import_single_device for consistent response schema - cables_view.py: resolve OOB mapping from VC sync device instead of raw obj so VC pages load controller LLDP rows correctly - interfaces_view.py: guard chassis_member None for OOB port names that don't resolve to a VC member position - modules_view.py: use sync_device instead of obj when reading OOB mapping so VC pages find the mapping on the correct member - actions.py: only re-check change permission for concurrent PlatformMapping creation when the row actually needs updating (not a no-op) - list.py: replace inline auto_create_ipam fallback with resolve_auto_create_ipam() to honour request POST/GET toggle - librenms_import.js: align Bootstrap global detection order in validationRefresh handler to match rest of plugin - templates: log error.message in fetch catch blocks for diagnosability - test_collisions.py: use token-based set assertion to avoid substring false positives on role strings
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
Jun 1, 2026
- utils.py: use _type_normalized in OOB_TYPE_PATTERN.search to avoid TypeError when oob_type is None - device_status.py: coerce paired_oob_id/paired_host_id to int before comparison to handle mixed str/int values - device_operations.py: add created_ips:[] to all failure return paths in import_single_device for consistent response schema - cables_view.py: resolve OOB mapping from VC sync device instead of raw obj so VC pages load controller LLDP rows correctly - interfaces_view.py: guard chassis_member None for OOB port names that don't resolve to a VC member position - modules_view.py: use sync_device instead of obj when reading OOB mapping so VC pages find the mapping on the correct member - actions.py: only re-check change permission for concurrent PlatformMapping creation when the row actually needs updating (not a no-op) - list.py: replace inline auto_create_ipam fallback with resolve_auto_create_ipam() to honour request POST/GET toggle - librenms_import.js: align Bootstrap global detection order in validationRefresh handler to match rest of plugin - templates: log error.message in fetch catch blocks for diagnosability - test_collisions.py: use token-based set assertion to avoid substring false positives on role strings
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
Jun 1, 2026
- utils.py: use _type_normalized in OOB_TYPE_PATTERN.search to avoid TypeError when oob_type is None - device_status.py: coerce paired_oob_id/paired_host_id to int before comparison to handle mixed str/int values - device_operations.py: add created_ips:[] to all failure return paths in import_single_device for consistent response schema - cables_view.py: resolve OOB mapping from VC sync device instead of raw obj so VC pages load controller LLDP rows correctly - interfaces_view.py: guard chassis_member None for OOB port names that don't resolve to a VC member position - modules_view.py: use sync_device instead of obj when reading OOB mapping so VC pages find the mapping on the correct member - actions.py: only re-check change permission for concurrent PlatformMapping creation when the row actually needs updating (not a no-op) - list.py: replace inline auto_create_ipam fallback with resolve_auto_create_ipam() to honour request POST/GET toggle - librenms_import.js: align Bootstrap global detection order in validationRefresh handler to match rest of plugin - templates: log error.message in fetch catch blocks for diagnosability - test_collisions.py: use token-based set assertion to avoid substring false positives on role strings
marcinpsk
force-pushed
the
feat/oob-sync
branch
3 times, most recently
from
9161826 to
0cc8fea
Compare
…esh succeed A host LLDP failure no longer aborts the refresh (OOB/serial rows can still surface it), so the success path now warns when _links_fetch_error is set and a host id was queried — otherwise host cables are silently omitted under a success banner. Skipped for OOB-only devices (librenms_id is None), where a host fetch failure is expected.
… overwrite the host OOB-controller rows are merged into the host interface list only for context (shared-LOM) and are never routed to a real target device. sync_selected_interfaces matched by name only, so a main+OOB 'eth0' collision processed both and the OOB row could overwrite the host interface with the wrong port_id/attrs. Skip _source=='oob' rows in the sync loop.
…directs Two module-sync handlers compute server_key = POST or self.librenms_api.server_key but called _modules_redirect_response(request, sync_url) without it on several return sites, so the helper fell back to POST/GET only and dropped the active-server context when the POST field was absent. Pass the computed server_key at every return site in those handlers.
…in a nested modal Bootstrap can strip .show from a nested modal before the keydown listener runs, so the '.modal.show' guard missed and Escape tore down the outer validation modal. Also gate on the event origin (event.target.closest of a nested modal), which is unaffected by that timing.
The test used oob_type='idrac' (no chars needing escaping), so it would pass even if render_actions() emitted the value unescaped. Use '<idrac>' and assert the escaped form is present and the raw value absent.
…failure An OOB-only mapping has no host librenms_id, so the host get_device_links() call always records _links_fetch_error even though no host fetch was meaningfully attempted. When the OOB controller validly returned no links, the empty-result guard mislabeled that as a failure and returned None, so _prepare_context() skipped caching the empty snapshot and stale OOB cable rows lingered after a genuine empty refresh. Return [] when the only reason for the recorded error is the absent host mapping on an OOB-scoped device.
…s to existing
When a previously-unmatched bulk-import row later resolves to an existing object,
its create-time blockers ("Device role must be..." / "Cluster must be...") are
stale — validate_device_for_import() only adds them when there's no existing_device.
The VM path cleared neither (both branches were gated on `not actual_is_vm`), and a
cross-model match can carry the other model's blocker, so a stale message lingered
in the UI. Clear both unconditionally once existing_device is set, before recalc.
Also docs: list `idrac` alongside `drac` in the OOB-type list (constants.OOB_TYPES
treats them as distinct canonical tokens).
Test: test_fresh_lookup_vm_clears_stale_cluster_blocker (real-DB) — a VM row resolves
by name match (actual_is_vm=True) and the cluster blocker is gone; verified red→green.
…in cache refresh; require OOB fetch success before caching empty OOB-only links - _refresh_existing_device now mirrors validate_device_for_import's cross-model collision check: raise AmbiguousLibreNMSIdError when the same (server_key, librenms_id) resolves on both a Device and a VirtualMachine, instead of binding to whichever model is queried first. - get_links_data OOB-only exemption now also requires the OOB fetch to have succeeded; a failed/malformed OOB fetch returns None so a transient failure doesn't overwrite the cache with an empty snapshot and drop stale OOB rows. - Harden the open-redirect-barrier test: pin get_librenms_sync_device so lookup_device is deterministic (matches sibling post() tests).
…move-to-winner swap and test contracts - validate_device_for_import merge block: re-validate the CURRENT side (hostname/ serial) with the same unique [:2] guard as the peer, so a duplicate-name/serial .first() match can't pair the user with an arbitrary merge target; warn + skip instead. - _interface_sync_content.html: add hx-swap="none" to the move-to-winner button so an OOB-only/empty move response can't clear the action button. - test_librenms_id _qs_returning: enforce the [:2] slice contract so a regression to [:1] fails loudly. - test_vlan_sync: encode server_key in the cache-key mocks so the eviction asserts actually prove the POSTed 'prod' scope (not a constant that passes regardless).
…etBox-only link copy - _cable_sync_content.html: migrated donor mode rendered a bare <div> dropping the CSRF token and server_key, but handleCableChange()'s verify-cable fetch reads both from the DOM — emit standalone hidden inputs (mirrors _interface_sync_content.html) so those JS requests don't hit a null token (TypeError/403) or the wrong server. - _interface_sync_content.html: the NetBox-only modal is transfer-only in migrated mode, so branch the trigger's title copy to 'view and move' (vs 'view and delete') to match.
…vice; harden device-field tests - import_single_device: an ambiguous librenms_id has existing_device=None, so the existing-device guard didn't catch it and a manual_mappings import could still create a duplicate Device under the ambiguous id. Add an explicit ambiguous_librenms_id guard that blocks the create. Real-DB test asserts no Device is created. - test_coverage_device_fields: pin three real behaviours the mocks otherwise let slip — full_clean() before save in the reused-platform mapping path, >=2 nested atomic() calls in the IntegrityError retry, and that the write-site add_platformmapping permission check ran.
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/views/sync/interfaces.py (1)
69-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve and validate the posted
server_keybefore using it for cache reads and custom-field writes.This sync action trusts the raw POST value, then passes it into cache lookup,
find_by_librenms_id(), andset_librenms_device_id(). If a stale/tampered key still has cached data, the sync can persist interface mappings under an unconfigured server namespace instead of failing closed like the other server-scoped paths.Proposed fix
- # Read server_key from POST so we use the exact server the user was viewing - server_key = request.POST.get("server_key") or self.librenms_api.server_key - self._post_server_key = server_key - interface_name_field = get_interface_name_field(request) self.interface_name_field = interface_name_field + + # Resolve the POSTed server through the LibreNMS settings so stale or + # tampered server keys cannot write custom-field mappings under an + # unconfigured namespace. + server_key = self.rebind_api_for_server(request.POST.get("server_key")) + if server_key is None: + messages.error(request, "Selected LibreNMS server is no longer configured.") + redirect_url = ( + reverse(url_name, kwargs={"pk": object_id}) + + f"?tab=interfaces&interface_name_field={interface_name_field}" + ) + return redirect(redirect_url) + self._post_server_key = server_key
🧹 Nitpick comments (1)
netbox_librenms_plugin/tests/test_ip_verify.py (1)
154-181: ⚡ Quick winCover boolean
vrf_idrejection too.The production guard rejects JSON booleans for
vrf_id, but these regressions only cover booleandevice_id. Add one VRF boolean case sotrue/falsecannot silently regress to1/0.Proposed regression test
def test_boolean_true_object_id_rejected_as_invalid(self): # object_id=True would otherwise int() to 1 and validate as device `#1`. view = _make_view() request = _make_request({"device_id": True, "ip_address": "10.0.0.1/24", "object_type": "device"}) response = view.post(request) assert response.status_code == 400 payload = json.loads(response.content) assert payload["message"] == "Invalid object ID" + + def test_boolean_vrf_id_rejected_as_invalid(self): + # vrf_id=True would otherwise int() to 1 and validate as VRF `#1`. + view = _make_view() + request = _make_request( + {"device_id": 5, "vrf_id": True, "ip_address": "10.0.0.1/24", "object_type": "device"} + ) + response = view.post(request) + assert response.status_code == 400 + payload = json.loads(response.content) + assert payload["message"] == "Invalid VRF ID"
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dfe1bc52-7e6f-4a86-b17d-1ac917361d91
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.gitignoredocs/SUMMARY.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/custom_field.mddocs/usage_tips/oob_management.mdmkdocs.ymlnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_oob_interface_select.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/create_platform_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_cable_sync_content_template.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
…-mode block
- _refresh_existing_device: the vanished-link and deleted-device branches recomputed
readiness but deferred re-asserting the create-time role/cluster blocker to the fresh
lookup, which early-returns when libre_device is None (or its except swallows) — so a
dropped-match row could stay importable with no role/cluster. Re-assert the blocker in
both drop branches before recompute. Real-DB regression test.
- apply_merge_candidates: set is_ready=False alongside can_import=False so a stale
is_ready=True (from hostname-first processing) can't leave contradictory state.
- test_coverage_base_views: seed POST server_key + assert rebind_api_for_server('prod') so
the open-redirect regression proves post() actually reads the submitted key.
…pstream review)
- cables_view: malformed OOB links no longer early-return links_data (which bypassed the
final None classification and cleared cached rows on an OOB-only device); fall through.
- interfaces_view: validate the main ports payload (dict + list of dict rows) before
enrichment, mirroring the OOB branch — a malformed 200 now fails closed, not 500.
- modules_view: run the transceiver type-check before the emptiness check so an empty
non-list ({}) is treated as malformed, not a successful 'no transceivers' response.
- librenms_api: legacy single-server mode now rejects an EXPLICIT non-default server_key
(fail closed) instead of using the default URL/token under a bogus cache/CF scope.
- device_operations: route _describe_existing_librenms_link host-id through the canonical
get_librenms_device_id accessor (read-only), keeping only OOB-subobject parsing local.
- device_status: use coerce_librenms_id (rejects bool/float) for paired host/OOB IDs.
- tests: malformed-payload + legacy-key red/green coverage, boolean vrf_id rejection,
sync_views2 docstring correction.
…ols; test hardening - device_status: the OOB-linked title used a raw int() for the paired host id, bypassing the strict _coerce_pair_id() the host-half branch uses — a bool/float id could render a bogus 'LibreNMS #1'. Reuse _coerce_pair_id for a single id contract. - librenms_import.js: the no-Bootstrap modal fallback called preventDefault() on every nested dismiss control, cancelling submit/hx-* actions on dismiss buttons that also act. Only suppress default for inert dismiss controls. - tests: whitespace-tolerant full-sync server_key assertion; pin request.htmx explicitly in the migrate request builders so a truthy MagicMock can't mis-route the non-HTMX path.
_build_interface_maps hand-rolled obj.virtual_chassis.members.values_list(...). Add a shared get_virtual_chassis_members(device) helper (returns all member Devices, or [device] when not in a VC) and use it, so the IP-sync member set can't drift from the rest of the plugin's VC handling. Behavior-preserving: added real-VC characterization tests (cross-member port-id/name resolution, cross-member name ambiguity, non-VC isolation) that pass identically before and after the refactor, plus a direct helper test. (interfaces.py's two member-expansion sites are develop-owned and left for a develop-side pass to route through the same helper.)
…de CSRF/transfer gating - collisions.detect_bulk_collisions: skip non-dict rows / non-dict validation instead of crashing the whole bulk-confirm flow on .get(). - device_status actions render: type-check existing_librenms_link before reading pairing keys so a malformed payload can't break the table render. - _vlan_sync_content.html: render CSRF + server_key in migrated mode too (the VLAN verify JS reads csrfmiddlewaretoken and posts server_key); only the form-submit action input stays gated. - librenms_sync_base.html: gate the donor transfer-IP buttons to Device pages (object|meta model_name == device) so a VM can't drive device_transfer_ip on a same-pk Device. - tests: red->green coverage for each; add non-dict main-ports row case; pin OOB-only fixture librenms_id=None; drop brittle '?tab=' >=5 sanity threshold.
… migrated-mode CSRF
- bulk_import: a missing scanned device_id no longer clears a still-live cached
librenms_id match; only drop it when the DB linkage is genuinely gone.
- bulk_import: clear stale site/device-type create-time blockers (not just
role/cluster) when a row resolves to an existing match.
- ip verify: reject JSON float device_id/vrf_id before int() truncation.
- sync redirects: on the no-server_key fallback, prefer the already-bound
_librenms_api; only when nothing is bound (failed rebind) resolve the default
via the librenms_api property so the redirect still carries the resolved
server_key, guarded so a misconfigured default degrades gracefully instead of
500ing.
- modules read path: mirror the post-side fail-closed inventory-shape guard so a
stale malformed cache entry can't crash _build_context.
- templates: convert multiline {# #} comments to {% comment %} (CI lint); emit a
standalone CSRF + server_key in migrated mode for the module fragment so the
verify-module JS POST keeps working.
… fallback, VC primary-IP guard - librenms_api: drop the legacy single-server-mode KeyError for an explicit non-default key. The strict unknown-key contract is multi-server-only; legacy mode binds the single configured server (view-layer validation handles cache/CF scoping). Unbreaks build_librenms_api() in legacy mode. - device_fields._sync_url: resolve the requested server_key against the trusted config first; a stale/unconfigured POST key no longer short-circuits the active/default fallback, so the redirect keeps its server_key (re-validated through the allowlist, open-redirect safe). - ip_addresses.process_ip_sync: don't set obj.primary_ip from an address bound to a sibling VC member's interface (the interface maps span all members); record it as primary_no_interface instead of persisting an invalid primary. - librenms_import.js: guard event.target with instanceof Element before .closest() (document keydown target has no .closest()). - test(modules): pin the librenms_id/OOB cache fingerprint in the malformed- inventory guard test so it actually exercises the inventory check (was passing via an id-mismatch invalidation; now a true red->green guard).
The migrated-mode Move button emitted hx-vals='{"server_key": "..."}'
unconditionally, so a marker without a server_key POSTed an empty
{"server_key": ""} — overriding the active/default server with a blank
key on non-default installs. Wrap it in {% if migrated_to_marker.server_key %}
to match the form-mode guard (the conditional server_key hidden input).
Tests render the real template: with a key the Move button carries
hx-vals; without one it omits the payload entirely (red->green).
…aces
NetBox's ComponentModel.clean() hard-blocks any device change on an existing
component ("Components cannot be moved to a different device"), keyed on the
device_id cached into _original_device at load time. MoveInterfaceToWinnerView's
full_clean()+save() therefore rejected EVERY interface move with a 409 — the
feature could never move an interface on NetBox 4.2+.
Re-seed _original_device to the winner so only that one blanket guard is
defeated; the real cross-device parent/lag/bridge checks, name uniqueness, and
save()'s _site/_location/_rack denormalization all still run (a bare
.update(device=...) would skip them and leave stale denormalized location).
Convert the migrate view tests from MagicMock queryset-chain stubs to real-DB
end-to-end (donor/winner devices, interfaces, IPs via the conftest builders +
mark_librenms_migrated): the move, the cross-device LAG rejection, the
unique-constraint FK transfer ordering, and the reject paths now exercise the
real ORM. The mock suite masked this bug entirely — its no-op full_clean()
'moved' the interface in-memory and asserted success. Concurrency-only paths
(marker repointed under lock, save IntegrityError race) and pure guards stay
mock-based with a note.
Device primary_ip4/primary_ip6/oob_ip are persisted via save(update_fields=...) in the OOB-link, merge, and move-to-winner flows to avoid full_clean() rejecting the write over unrelated pre-existing inconsistencies (e.g. face without rack). That also skips the NetBox invariant that the address must be assigned to one of the device's OWN interfaces — so the ownership check lived by convention at each call site, and a future site that forgot it could silently persist an off-device FK. Add utils.set_device_ip_fk(device, field, ip, *, save=True) as the single guarded chokepoint: it raises ValueError for a non-None address not assigned to an interface on the device, and (by default) saves only that column. Route all four write sites through it — migrate.py _reconcile_donor_device_ip_fks + TransferDeviceIPView (save=True), actions.py OOB-link set + merge OOB transfer (save=False, batched into the existing update_fields). Callers still order release-before-claim for the UNIQUE FK. Test: TestSetDeviceIpFk (real DB) — sets/clears on an owned address, refuses an off-device address without persisting, save=False defers the write, bad field rejected. Red->green verified.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Builds on #303 (feat/ipam). Lets a single NetBox device represent a physical server while pulling data from two LibreNMS devices — the in-band host (the OS, polled via SNMP/agent on the production NIC) and the out-of-band management controller (iDRAC / iLO / IPMI / BMC, polled via its dedicated management IP). Sync makes it obvious which LibreNMS source each row came from, and pairing / merging an existing host+OOB pair (or promoting an already-imported OOB-only device to a full host) becomes an import action instead of manual cleanup.
Motivation / Problem
Feature.
Servers typically appear in LibreNMS as two separate devices: the in-band host and the out-of-band management controller. They share one physical chassis but expose different inventories, interfaces and IPs. NetBox models them as one device with an OOB IP and management interface — there is no separate iDRAC device. Today the plugin doesn't bridge that mismatch:
Goal: let one NetBox device represent the physical box and pull data from both LibreNMS devices:
Would close #289.
Scope of Change
Highlights (delta over feat/ipam):
_sourcebadge showing whether the row came from the host or the OOB device.idrac0, with confirm/override). This satisfies NetBox's requirement thatoob_ipbe interface-assigned; no unassigned/global IP addresses are created.librenms_idlinkage — the OOB sub-object stores only{id, type}; the controller's IP/version are read live from LibreNMS via the id rather than denormalised into the custom field.How Was This Tested?
librenms_idcoercion + OOB helpers, and import-action contracts (test_collisions.py,test_migrate_views.py,test_coverage_*,test_import_validation_helpers.py,test_librenms_id.py).Manual Test Steps
_sourcebadge.Risk Assessment
Backwards Compatibility
librenms_idcustom field.Other Notes
views/imports/actions.py, device migration inviews/sync/migrate.py, and the OOB linkage helpers inutils.py.