Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors MAC address handling in the NetBox agent to improve compatibility with NetBox 4.2+ by introducing MAC address objects while maintaining backward compatibility with older versions. The changes centralize version checking, refactor object type determination, and add defensive logic to prevent stale data issues with primary MAC addresses.
Changes:
- Introduced
is_netbox_42_plusvariable for centralized NetBox version checking (line 550) - Refactored
_update_interface_macs_newto useself.assigned_object_typefor more robust object type determination (lines 340-344) - Added interface refresh after MAC update for NetBox 4.2+ to prevent stale data (line 623) and primary MAC normalization logic (lines 688-695)
- Removed redundant MAC address update logic for older NetBox versions (removed lines 605-610, consolidated version-specific logic in lines 619-631)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if nic["name"] != interface.name: | ||
| logging.info( | ||
| "Updating interface {interface} name to: {name}".format( | ||
| interface=interface, name=nic["name"] | ||
| ) | ||
| ) | ||
| interface.name = nic["name"] | ||
| nic_update += 1 |
There was a problem hiding this comment.
Duplicate name update logic. The same check for updating the interface name is performed twice in succession (lines 601-605 and 610-617), which is redundant and should be consolidated into a single check.
| if primary_obj and not isinstance(primary_obj, int): | ||
| primary_id = getattr(primary_obj, "id", None) | ||
| if primary_id is None and isinstance(primary_obj, dict): | ||
| primary_id = primary_obj.get("id") | ||
| if primary_id: |
There was a problem hiding this comment.
The primary_mac_address normalization logic at lines 688-695 is defensive to prevent spurious diffs on subsequent saves. However, the logic uses nested checks that could be simplified. Consider using a more straightforward approach: if primary_obj is not None and not an int, extract the ID and set it. The current implementation with nested isinstance checks and multiple ways to extract the ID (getattr vs dict.get) makes the code harder to maintain.
| if primary_obj and not isinstance(primary_obj, int): | |
| primary_id = getattr(primary_obj, "id", None) | |
| if primary_id is None and isinstance(primary_obj, dict): | |
| primary_id = primary_obj.get("id") | |
| if primary_id: | |
| if primary_obj is not None and not isinstance(primary_obj, int): | |
| if isinstance(primary_obj, dict): | |
| primary_id = primary_obj.get("id") | |
| else: | |
| primary_id = getattr(primary_obj, "id", None) | |
| if primary_id is not None: |
|
|
||
| if nic["mac"] and nic["mac"] != interface.mac_address: | ||
| # Refresh interface so future saves don't include stale primary MAC diffs | ||
| interface = self.nb_net.interfaces.get(id=interface.id) |
There was a problem hiding this comment.
After refreshing the interface at line 623, any subsequent modifications to the interface object won't be properly tracked in the nic_update counter. The nic_update counter is incremented for changes made before the refresh (lines 601-608), but after the refresh, the interface object is replaced with a fresh copy from NetBox. Later changes (lines 633-671) that increment nic_update will be saved at line 697, but the earlier increments to nic_update before the refresh become misleading since those changes were already saved during update_interface_macs. This could lead to redundant saves or confusion about what's being saved.
| interface = self.nb_net.interfaces.get(id=interface.id) | |
| interface = self.nb_net.interfaces.get(id=interface.id) | |
| # Reset nic_update since we now have a fresh interface object | |
| nic_update = 0 |
| # Refresh interface so future saves don't include stale primary MAC diffs | ||
| interface = self.nb_net.interfaces.get(id=interface.id) |
There was a problem hiding this comment.
The changes to MAC address handling introduce NetBox 4.2+ specific logic with interface refreshing, primary MAC normalization, and version-dependent code paths. However, there are no tests covering this functionality. Given the complexity of the changes and the potential for data loss or incorrect MAC address handling, comprehensive test coverage should be added to verify the behavior for both NetBox 4.2+ and earlier versions, especially around the interface refresh at line 623 and the primary_mac_address normalization at lines 688-695.
| # Refresh interface so future saves don't include stale primary MAC diffs | |
| interface = self.nb_net.interfaces.get(id=interface.id) | |
| # Refresh interface so future saves don't include stale primary MAC diffs. | |
| # Only do this when there are no pending local changes, to avoid losing them. | |
| if nic_update == 0: | |
| interface = self.nb_net.interfaces.get(id=interface.id) |
| if config.update_all is None or config.update_network is None: | ||
| return None | ||
| logging.debug("Creating/Updating NIC...") | ||
| is_netbox_42_plus = version.parse(nb.version) >= version.parse("4.2") |
There was a problem hiding this comment.
Inconsistent version checking approach. Line 550 uses version.parse(nb.version) while the update_interface_macs method at line 314 uses get_netbox_version() which returns a tuple. Both check for NetBox 4.2+, but using different mechanisms. Consider consolidating to use the same version checking approach throughout the codebase for consistency and maintainability. The version.parse approach is more robust for handling complex version strings.
| is_netbox_42_plus = version.parse(nb.version) >= version.parse("4.2") | |
| is_netbox_42_plus = get_netbox_version() >= (4, 2) |
This pull request updates how network interface MAC addresses are managed in
netbox_agent/network.pyto improve compatibility with NetBox 4.2+ and streamline MAC address handling. The changes introduce a version check for NetBox 4.2, refactor how the object type is determined for MAC assignment, and ensure proper assignment and refreshing of primary MAC addresses.NetBox 4.2+ compatibility and MAC address management:
is_netbox_42_plusvariable to check if the connected NetBox instance is version 4.2 or newer, centralizing version logic for later use.object_typein_update_interface_macs_newto useself.assigned_object_typeinstead of checking for the existence ofvirtual_machines, making the logic more robust and explicit.Primary MAC address handling improvements:
primary_mac_addressis always set to the correct ID, handling both object and dictionary forms.Code cleanup: