From 273f7252cd97f0105c064c2602823371859ea279 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Mon, 2 Nov 2026 12:36:25 +0100 Subject: [PATCH 1/2] decides whether the source discovered IPs before permitted_subnets filtering keep_undiscovered_ips exists so an interface Redfish reports no address for keeps the IPs it already has: Redfish does not report host NIC, bond or bridge addresses, and such an interface is matched only by a shared MAC. Whether anything was discovered was read from interface_ips, the list the check_redfish source hands over. That list is already filtered: update_network_interface() drops any address permitted_subnets rejects before building it. So a port where Redfish reported an address the operator excluded looked identical to a port Redfish said nothing about, and its existing IPs were kept instead of being cleaned up. The two cases are different. In the first the source could see the interface, so an address in NetBox which is no longer reported is gone and should be removed. Only the second means the source is blind and must not be trusted to prune. The source now decides, because it is the only place that sees its own discovery before filtering: update_network_interface() counts the addresses Redfish reported per port and passes keep_undiscovered_ips only for a port which reported none. add_update_interface() is unchanged apart from saying so, since interface_ips is a surviving subset and cannot answer the question on its own. The test drives the real update_network_interface() with a real PermittedSubnets which excludes the reported address, and asserts the stale address is still removed. It also asserts PermittedSubnets parsed the exclusion, because an unparseable subnet permits everything and would have made the test pass without exercising anything. The shared check_redfish test fixture now calls reset_inventory_state(), which sets up the per inventory file state __init__ would have, so a test can drive the network interface path at all. A port skipped by skip_os_reported_ips is not counted. The operator told the source to ignore the addresses of that port, so it did not see them and must keep what NetBox has. The test seeds the interface with a primary MAC address object, because on NetBox 4.2 and later an interface is matched by that object and not by the legacy mac_address field. --- .../sources/check_redfish/import_inventory.py | 11 +- module/sources/common/source_base.py | 8 +- tests/conftest.py | 2 + ...st_check_redfish_interface_ip_discovery.py | 123 ++++++++++++++++++ 4 files changed, 139 insertions(+), 5 deletions(-) create mode 100644 tests/test_check_redfish_interface_ip_discovery.py diff --git a/module/sources/check_redfish/import_inventory.py b/module/sources/check_redfish/import_inventory.py index 91add4e1..432ef524 100644 --- a/module/sources/check_redfish/import_inventory.py +++ b/module/sources/check_redfish/import_inventory.py @@ -874,6 +874,8 @@ def update_network_interface(self): port_data_dict = dict() nic_ips = dict() + # addresses redfish reported per port, counted before permitted_subnets filtering + nic_ips_reported = dict() discovered_int_list = list() for nic_port in grab(self.inventory_file_content, "inventory.network_port", fallback=list()): @@ -983,6 +985,8 @@ def update_network_interface(self): nic_ips[port_name] = list() if self.settings.skip_os_reported_ips is True and mgmt_only is False: continue + nic_ips_reported[port_name] = len(grab(nic_port, "ipv4_addresses", fallback=list())) + \ + len(grab(nic_port, "ipv6_addresses", fallback=list())) for ipv4_address in grab(nic_port, "ipv4_addresses", fallback=list()): if self.settings.permitted_subnets.permitted(ipv4_address, interface_name=port_name) is False: @@ -1042,9 +1046,12 @@ def update_network_interface(self): port_data = data_to_update - # redfish only reliably reports the BMC IP, never the host NIC / bond / bridge IPs + # redfish reports no host NIC / bond / bridge address, so an interface it said + # nothing about keeps the IPs it has. An address it did report but the operator + # excluded still counts as having seen the interface. self.add_update_interface(nic_object, self.device_object, port_data, - nic_ips.get(port_name, list()), keep_undiscovered_ips=True) + nic_ips.get(port_name, list()), + keep_undiscovered_ips=nic_ips_reported.get(port_name, 0) == 0) def update_manager(self): diff --git a/module/sources/common/source_base.py b/module/sources/common/source_base.py index 7538e5de..5061e68c 100644 --- a/module/sources/common/source_base.py +++ b/module/sources/common/source_base.py @@ -402,7 +402,9 @@ def add_update_interface(self, interface_object, device_object, interface_data, vmware_object: vim.HostSystem | vim.VirtualMachine object to add to list of objects to reevaluate keep_undiscovered_ips: bool - if True, keep the existing IPs of an interface the source discovered no IPs for + if True the source reported no addresses at all for this interface, so its + existing IPs are kept. The caller decides this before filtering its own + discovery, because an address it dropped is still one it saw Returns ------- @@ -770,8 +772,8 @@ def add_update_interface(self, interface_object, device_object, interface_data, ip_address_objects.append(this_ip_object) - # keyed on what the source reported, not on what survived parsing: an unusable address - # is still a statement that the interface was seen + # the caller states whether it discovered anything; interface_ips is only its + # surviving subset, so it cannot answer that on its own skip_ip_removal = keep_undiscovered_ips is True and len(interface_ips or list()) == 0 # guest tools which report as running but hand back no interface at all are broken diff --git a/tests/conftest.py b/tests/conftest.py index b1335bf9..b6450e4c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -230,6 +230,8 @@ def _make(**overrides: object) -> SimpleNamespace: source.name = "test" source.source_tag = "Source: test" source.settings = check_redfish_settings(**overrides) + # the per inventory file state __init__ would have set up + source.reset_inventory_state() source.add_necessary_base_objects() # the primary tag is normally registered by the NetBox handler, not by the source diff --git a/tests/test_check_redfish_interface_ip_discovery.py b/tests/test_check_redfish_interface_ip_discovery.py new file mode 100644 index 00000000..5e539091 --- /dev/null +++ b/tests/test_check_redfish_interface_ip_discovery.py @@ -0,0 +1,123 @@ +"""Whether the source discovered addresses is decided before permitted_subnets filtering. + +keep_undiscovered_ips exists because Redfish does not report host NIC, bond or bridge +addresses, so an interface matched only by a shared MAC must keep the management IP it +already has. An address that Redfish did report but the operator excluded is a different +case: the source was not blind, so a stale address on that interface is still cleaned up. + +Drives the real update_network_interface() with a real PermittedSubnets. +""" + +import pytest + +from module.netbox.object_classes import NBInterface, NBIPAddress, NBMACAddress +from module.sources.common.permitted_subnets import PermittedSubnets + +MAC = "AA:BB:CC:DD:EE:01" +STALE = "192.0.2.50/24" +REPORTED = "198.51.100.7/24" +REPORTED_SUBNET = "198.51.100.0/24" + + +def nic_inventory(ipv4_addresses): + """One adapter with one port, reporting the given addresses.""" + + return {"inventory": { + "network_adapter": [ + {"id": "NIC.Integrated.1", "name": "Integrated NIC 1", "model": "X710", + "manufacturer": "Intel", "serial": "NIC-AAA", "firmware": "1.0", + "health_status": "OK", "operation_status": "Enabled"}], + "network_port": [ + {"id": "NIC.Integrated.1-1", "name": "Integrated NIC 1 Port 1", + "adapter_id": "NIC.Integrated.1", "addresses": [MAC], "link_status": "Up", + "health_status": "OK", "operation_status": "Enabled", "manager_ids": [], + "ipv4_addresses": ipv4_addresses, "ipv6_addresses": [], + "link_speed": 10000, "hostname": None, "port_id": "1"}], + }} + + +def seed_interface_with_stale_ip(context, name="NIC.Integrated.1-1"): + interface = context.inventory.add_object( + NBInterface, data={"name": name, "device": context.device}, source=context.source) + # NetBox >= 4.2 matches an interface by its primary MAC address object + mac = context.inventory.add_object( + NBMACAddress, data={"mac_address": MAC, "assigned_object_id": interface, + "assigned_object_type": NBInterface}, source=context.source) + interface.update(data={"primary_mac_address": mac}, source=context.source) + ip = context.inventory.add_object( + NBIPAddress, data={"address": STALE, "assigned_object_id": interface}, source=context.source) + assert ip in interface.get_ip_addresses() + return interface, ip + + +def run(context, ipv4_addresses, permitted): + subnets = PermittedSubnets(permitted) + # a subnet string that fails to parse would silently permit everything and let these + # tests pass for the wrong reason + assert subnets.validation_failed is False + context.source.settings.permitted_subnets = subnets + context.source.inventory_file_content = nic_inventory(ipv4_addresses) + context.source.update_network_adapter() + context.source.update_network_interface() + + +def test_stale_ip_is_removed_when_the_reported_address_is_excluded(check_redfish_source): + """The regression: Redfish reported an address, so the interface was seen. The operator + excluding that subnet must not be read as "the source discovered nothing".""" + + context = check_redfish_source() + _, stale = seed_interface_with_stale_ip(context) + + run(context, [REPORTED], permitted=f"!{REPORTED_SUBNET}, 0.0.0.0/0") + + assert "assigned_object_id" in stale.unset_items + + +def test_stale_ip_is_kept_when_redfish_reported_nothing(check_redfish_source): + """The case the option exists for, which must keep working.""" + + context = check_redfish_source() + _, stale = seed_interface_with_stale_ip(context) + + run(context, [], permitted="0.0.0.0/0") + + assert "assigned_object_id" not in stale.unset_items + + +def test_stale_ip_is_removed_when_a_different_address_is_reported(check_redfish_source): + """A reported and permitted address that is not the stale one still replaces it.""" + + context = check_redfish_source() + _, stale = seed_interface_with_stale_ip(context) + + run(context, [REPORTED], permitted="0.0.0.0/0") + + assert "assigned_object_id" in stale.unset_items + + +@pytest.mark.parametrize("permitted", ["0.0.0.0/0", f"!{REPORTED_SUBNET}, 0.0.0.0/0"]) +def test_reported_address_is_only_synced_when_permitted(check_redfish_source, permitted): + """permitted_subnets still governs what gets added, either way.""" + + context = check_redfish_source() + seed_interface_with_stale_ip(context) + + run(context, [REPORTED], permitted=permitted) + + synced = [ip.data["address"] for ip in context.inventory.get_all_items(NBIPAddress)] + if permitted == "0.0.0.0/0": + assert REPORTED in synced + else: + assert REPORTED not in synced + + +def test_stale_ip_is_kept_when_os_reported_ips_are_skipped(check_redfish_source): + """skip_os_reported_ips means the source ignores this port's addresses, so it did not see them.""" + + context = check_redfish_source() + context.source.settings.skip_os_reported_ips = True + _, stale = seed_interface_with_stale_ip(context) + + run(context, [REPORTED], permitted="0.0.0.0/0") + + assert "assigned_object_id" not in stale.unset_items From 707dcfcccd057620b39fd1f3a368c55d9b8ceeed Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Mon, 2 Nov 2026 12:47:38 +0100 Subject: [PATCH 2/2] counts only real IP addresses as discovered by check_redfish An inventory entry which is not an IP address (null, "N/A") made a port count as discovered. keep_undiscovered_ips was then false, and the existing IPs of the interface were removed although the port reported no usable address. Count only entries which parse as an IP address. An excluded valid address still counts, so permitted_subnets filtering does not hide a real discovery. --- .../sources/check_redfish/import_inventory.py | 16 +++++++++++-- ...st_check_redfish_interface_ip_discovery.py | 23 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/module/sources/check_redfish/import_inventory.py b/module/sources/check_redfish/import_inventory.py index 432ef524..97282e65 100644 --- a/module/sources/check_redfish/import_inventory.py +++ b/module/sources/check_redfish/import_inventory.py @@ -11,6 +11,7 @@ import glob import hashlib import json +from ipaddress import ip_interface from packaging import version @@ -30,6 +31,15 @@ log = get_logger() + +def is_ip_address(value) -> bool: + """True if value parses as an IP address, with or without a prefix length.""" + try: + ip_interface(value) + except ValueError: + return False + return True + class CheckRedfish(SourceBase): """ Source class to import check_redfish inventory files @@ -985,8 +995,10 @@ def update_network_interface(self): nic_ips[port_name] = list() if self.settings.skip_os_reported_ips is True and mgmt_only is False: continue - nic_ips_reported[port_name] = len(grab(nic_port, "ipv4_addresses", fallback=list())) + \ - len(grab(nic_port, "ipv6_addresses", fallback=list())) + nic_ips_reported[port_name] = len([ + address for address in [*grab(nic_port, "ipv4_addresses", fallback=list()), + *grab(nic_port, "ipv6_addresses", fallback=list())] + if is_ip_address(address)]) for ipv4_address in grab(nic_port, "ipv4_addresses", fallback=list()): if self.settings.permitted_subnets.permitted(ipv4_address, interface_name=port_name) is False: diff --git a/tests/test_check_redfish_interface_ip_discovery.py b/tests/test_check_redfish_interface_ip_discovery.py index 5e539091..68b378d9 100644 --- a/tests/test_check_redfish_interface_ip_discovery.py +++ b/tests/test_check_redfish_interface_ip_discovery.py @@ -121,3 +121,26 @@ def test_stale_ip_is_kept_when_os_reported_ips_are_skipped(check_redfish_source) run(context, [REPORTED], permitted="0.0.0.0/0") assert "assigned_object_id" not in stale.unset_items + + +@pytest.mark.parametrize("reported", [[None], ["not-an-ip"], [None, "N/A"]]) +def test_stale_ip_is_kept_when_no_reported_entry_is_an_address(check_redfish_source, reported): + """An entry which is not an IP address is not a discovery, so it must not trigger pruning.""" + + context = check_redfish_source() + _, stale = seed_interface_with_stale_ip(context) + + run(context, reported, permitted="0.0.0.0/0") + + assert "assigned_object_id" not in stale.unset_items + + +def test_stale_ip_is_removed_when_an_excluded_address_is_reported_with_junk(check_redfish_source): + """A valid address next to an invalid entry still counts, even when permitted_subnets excludes it.""" + + context = check_redfish_source() + _, stale = seed_interface_with_stale_ip(context) + + run(context, [None, REPORTED], permitted=f"!{REPORTED_SUBNET}, 0.0.0.0/0") + + assert "assigned_object_id" in stale.unset_items