Skip to content

Fix/macaddress #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Charlie-Root merged 3 commits into from
Feb 25, 2026
Merged

Fix/macaddress #6

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff7bd04
fix
Charlie-Root Feb 25, 2026
72f1975
fix
Charlie-Root Feb 25, 2026
8ed4857
fix
Charlie-Root Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 21 additions & 19 deletions netbox_agent/network.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,10 +337,11 @@ def _update_interface_macs_legacy(self, nic, macs):
)

def _update_interface_macs_new(self, nic, macs):
if hasattr(self.nb_net, 'virtual_machines'):
object_type = 'virtualization.vminterface'
else:
object_type = 'dcim.interface'
object_type = (
"virtualization.vminterface"
if self.assigned_object_type == "virtualization.vminterface"
else "dcim.interface"
)
try:
nb_macs = list(self.nb.dcim.mac_addresses.filter(
assigned_object_type=object_type,
Expand Down Expand Up @@ -546,6 +547,7 @@ def create_or_update_netbox_network_cards(self):
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")
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
is_netbox_42_plus = version.parse(nb.version) >= version.parse("4.2")
is_netbox_42_plus = get_netbox_version() >= (4, 2)

Copilot uses AI. Check for mistakes.

# delete unknown interface
nb_nics = list(self.get_netbox_network_cards())
Expand Down Expand Up @@ -602,12 +604,6 @@ def batched(it, n):
interface.name = nic['name']
nic_update += 1

if nic['mac'] != interface.mac_address:
logging.info('Updating interface {interface} mac_address to: {mac}'.format(
interface=interface, mac=nic['mac']))
interface.mac_address = nic['mac']
nic_update += 1

ret, interface = self.reset_vlan_on_interface(nic, interface)
nic_update += ret

Expand All @@ -620,21 +616,18 @@ def batched(it, n):
interface.name = nic["name"]
nic_update += 1

if version.parse(nb.version) >= version.parse("4.2"):
# Create MAC objects
if is_netbox_42_plus:
if nic["mac"]:
self.update_interface_macs(interface, [nic["mac"]])

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)
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Comment on lines +622 to +623
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# 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)

Copilot uses AI. Check for mistakes.
elif nic["mac"] and nic["mac"] != interface.mac_address:
logging.info(
"Updating interface {interface} mac to: {mac}".format(
"Updating interface {interface} mac_address to: {mac}".format(
interface=interface, mac=nic["mac"]
)
)
if version.parse(nb.version) < version.parse("4.2"):
interface.mac_address = nic["mac"]
else:
interface.primary_mac_address = {"mac_address": nic["mac"]}
interface.mac_address = nic["mac"]
nic_update += 1

if hasattr(interface, "mtu"):
Expand Down Expand Up @@ -691,6 +684,15 @@ def batched(it, n):
# sync local IPs
for ip in nic["ip"]:
self.create_or_update_netbox_ip_on_interface(ip, interface)

if is_netbox_42_plus:
primary_obj = getattr(interface, "primary_mac_address", None)
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:
Comment on lines +690 to +694
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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:

Copilot uses AI. Check for mistakes.
interface.primary_mac_address = primary_id
if nic_update > 0:
interface.save()

Expand Down
Loading