fix: ip discovery before subnet filter

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
@@ -874,6 +884,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 +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([
+                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:
@@ -1042,9 +1058,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):
 

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

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

tests/test_check_redfish_interface_ip_discovery.py

@@ -0,0 +1,146 @@
+"""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
+
+
+@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