Fix/keep undiscovered ips

module/sources/check_redfish/import_inventory.py

@@ -867,7 +867,9 @@ def update_network_interface(self):
 
                 port_data = data_to_update
 
-            self.add_update_interface(nic_object, self.device_object, port_data, nic_ips.get(port_name, list()))
+            # redfish only reliably reports the BMC IP, never the host NIC / bond / bridge IPs
+            self.add_update_interface(nic_object, self.device_object, port_data,
+                                      nic_ips.get(port_name, list()), keep_undiscovered_ips=True)
 
     def update_manager(self):
 

module/sources/common/source_base.py

@@ -292,7 +292,7 @@ def return_longest_matching_prefix_for_ip(self, ip_to_match=None, site_name=None
         return current_longest_matching_prefix
 
     def add_update_interface(self, interface_object, device_object, interface_data, interface_ips=None,
-                             vmware_object=None):
+                             vmware_object=None, keep_undiscovered_ips=False):
         """
         Adds/Updates an interface to/of a NBVM or NBDevice including IP addresses.
         Validates/enriches data in following order:
@@ -317,6 +317,8 @@ def add_update_interface(self, interface_object, device_object, interface_data,
             a list of ip addresses which are assigned to this interface
         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
 
         Returns
         -------
@@ -674,9 +676,13 @@ 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
+        skip_ip_removal = keep_undiscovered_ips is True and len(interface_ips or list()) == 0
+
         for current_ip in interface_object.get_ip_addresses():
 
-            if skip_ip_handling is True:
+            if skip_ip_handling is True or skip_ip_removal is True:
                 continue
 
             if grab(current_ip, "data.role.value") == "anycast":

tests/conftest.py

@@ -22,9 +22,16 @@
 
 import pytest
 
+from module.config.base import ConfigOptions
+from module.config.group import ConfigOptionGroup
+from module.config.option import ConfigOption
 from module.config.parser import ConfigParser
+from module.netbox.connection import NetBoxHandler
 from module.netbox.inventory import NetBoxInventory
+from module.netbox.object_classes import NBDevice, NBTag
 from module.sources import instantiate_sources
+from module.sources.check_redfish.config import CheckRedfishConfig
+from module.sources.check_redfish.import_inventory import CheckRedfish
 
 FIXTURE_DIR = Path(__file__).parent / "fixtures" / "vcsim"
 
@@ -194,3 +201,53 @@ def sdk(vcsim):
         yield instance.RetrieveContent()
     finally:
         connect.Disconnect(instance)
+
+
+@pytest.fixture
+def check_redfish_source(inventory):
+    """
+    Returns a function building a minimally initialized CheckRedfish source on the fresh
+    inventory, with a device to hang components off. The real add_necessary_base_objects()
+    runs, so the source tag and every custom field are registered as they are in production.
+
+    Settings start from the declared defaults of every CheckRedfishConfig option and are
+    overridden by keyword arguments, so a test states only what it cares about and an option
+    added to the config later reaches the tests with its real default.
+    """
+    def _make(**overrides: object) -> SimpleNamespace:
+        source = object.__new__(CheckRedfish)
+        source.inventory = inventory
+        source.name = "test"
+        source.source_tag = "Source: test"
+        source.settings = check_redfish_settings(**overrides)
+
+        source.add_necessary_base_objects()
+        # the primary tag is normally registered by the NetBox handler, not by the source
+        inventory.add_update_object(NBTag, data={"name": NetBoxHandler.primary_tag})
+
+        device = inventory.add_object(NBDevice, data={"name": "server01"}, source=source)
+        source.device_object = device
+
+        return SimpleNamespace(source=source, inventory=inventory, device=device)
+
+    return _make
+
+
+def check_redfish_settings(**overrides) -> ConfigOptions:
+    """
+    The settings a parsed check_redfish config produces: every declared option at its default,
+    with the given overrides applied. ConfigOptions is what ConfigBase.parse() returns, so an
+    option this source does not declare reads as None here exactly as it does in production.
+    """
+    values = {}
+    for entry in CheckRedfishConfig().options:
+        declared = entry.options if isinstance(entry, ConfigOptionGroup) else [entry]
+        for option in declared:
+            if isinstance(option, ConfigOption) and option.removed is not True:
+                values[option.key] = option.default_value
+
+    unknown = set(overrides) - set(values)
+    assert not unknown, f"not declared by CheckRedfishConfig: {sorted(unknown)}"
+
+    values.update(overrides)
+    return ConfigOptions(**values)

tests/test_check_redfish_interface_ips.py

@@ -0,0 +1,64 @@
+"""An interface the source discovered no IPs for must keep the IPs it already has.
+
+Drives the real add_update_interface() IP removal loop against real NBInterface and
+NBIPAddress objects.
+"""
+
+from module.netbox.object_classes import NBInterface, NBIPAddress
+
+
+def seed_interface_with_ip(context, name="pnet0", address="172.10.10.12/24"):
+    interface = context.inventory.add_object(
+        NBInterface, data={"name": name, "device": context.device}, source=context.source)
+    ip = context.inventory.add_object(
+        NBIPAddress, data={"address": address, "assigned_object_id": interface}, source=context.source)
+    assert ip in interface.get_ip_addresses()
+    return interface, ip
+
+
+def test_ip_is_kept_when_the_source_discovered_no_ips(check_redfish_source):
+    """The management IP on a bond or bridge matched only by a shared MAC must survive a sync."""
+
+    context = check_redfish_source()
+    interface, ip = seed_interface_with_ip(context)
+
+    context.source.add_update_interface(interface, context.device, {"name": "pnet0"}, [], keep_undiscovered_ips=True)
+
+    # unset_attribute() queues the de-assignment in unset_items, it does not mutate data
+    assert "assigned_object_id" not in ip.unset_items
+
+
+def test_ip_is_still_removed_by_default(check_redfish_source):
+    """Other sources are unchanged: an IP no longer reported is still removed."""
+
+    context = check_redfish_source()
+    interface, ip = seed_interface_with_ip(context)
+
+    context.source.add_update_interface(interface, context.device, {"name": "pnet0"}, [])
+
+    assert "assigned_object_id" in ip.unset_items
+
+
+def test_ip_is_still_removed_when_other_ips_are_discovered(check_redfish_source):
+    """The guard covers an empty discovery only. An IP dropped from a non-empty set still goes."""
+
+    context = check_redfish_source()
+    interface, ip = seed_interface_with_ip(context)
+
+    context.source.add_update_interface(interface, context.device, {"name": "pnet0"}, ["198.51.100.7/24"],
+                                keep_undiscovered_ips=True)
+
+    assert "assigned_object_id" in ip.unset_items
+
+
+def test_ip_is_still_removed_when_the_discovered_ips_are_unusable(check_redfish_source):
+    """A non-empty discovery is a statement about the interface even when none of the addresses
+    survive parsing, so the guard must not treat it as "discovered nothing"."""
+
+    context = check_redfish_source()
+    interface, ip = seed_interface_with_ip(context)
+
+    context.source.add_update_interface(interface, context.device, {"name": "pnet0"}, ["not-an-ip"],
+                                keep_undiscovered_ips=True)
+
+    assert "assigned_object_id" in ip.unset_items

tests/test_check_redfish_inventory_items.py

@@ -4,35 +4,7 @@
 classes. Only the NetBox REST API itself is out of scope.
 """
 
-import types
-
-from module.netbox.inventory import NetBoxInventory
-from module.netbox.object_classes import NBDevice, NBInventoryItem
-from module.sources.check_redfish.import_inventory import CheckRedfish
-
-
-def make_source():
-    """Build a CheckRedfish source on a fresh inventory, with the real base objects registered."""
-
-    inventory = NetBoxInventory()
-    # reset the singleton state so each test starts from an empty inventory
-    inventory.init()
-    inventory.source_list = list()
-    inventory.netbox_api_version = "4.3.0"
-
-    source = object.__new__(CheckRedfish)
-    source.inventory = inventory
-    source.name = "test"
-    source.source_tag = "Source: test"
-    source.settings = types.SimpleNamespace()
-
-    source.add_necessary_base_objects()
-
-    device = inventory.add_object(NBDevice, data={"name": "server01"}, source=source)
-    source.device_object = device
-
-    return source, inventory, device
-
+from module.netbox.object_classes import NBInventoryItem
 
 # a Dell `location` as check_redfish can hand it back: a nested Oem object, not a string
 DELL_LOCATION = {
@@ -53,15 +25,15 @@ def enclosure(name, location, serial="ENC-AAA"):
          "operation_status": "Enabled"}]}}
 
 
-def test_structured_location_is_not_stringified_into_the_item_name():
+def test_structured_location_is_not_stringified_into_the_item_name(check_redfish_source):
     """A structured location must not reach the name as its Python repr."""
 
-    source, inventory, _ = make_source()
+    context = check_redfish_source()
 
-    source.inventory_file_content = enclosure("BP_PSV 0:1", DELL_LOCATION)
-    source.update_storage_enclosure()
+    context.source.inventory_file_content = enclosure("BP_PSV 0:1", DELL_LOCATION)
+    context.source.update_storage_enclosure()
 
-    items = inventory.get_all_items(NBInventoryItem)
+    items = context.inventory.get_all_items(NBInventoryItem)
     assert len(items) == 1
 
     name = items[0].data["name"]
@@ -72,29 +44,29 @@ def test_structured_location_is_not_stringified_into_the_item_name():
     assert name == "BP_PSV 0:1"
 
 
-def test_plain_string_location_is_kept_in_the_item_name():
+def test_plain_string_location_is_kept_in_the_item_name(check_redfish_source):
     """A location that really is a string is still used."""
 
-    source, inventory, _ = make_source()
+    context = check_redfish_source()
 
-    source.inventory_file_content = enclosure("BP_PSV 0:1", "Slot 3")
-    source.update_storage_enclosure()
+    context.source.inventory_file_content = enclosure("BP_PSV 0:1", "Slot 3")
+    context.source.update_storage_enclosure()
 
-    items = inventory.get_all_items(NBInventoryItem)
+    items = context.inventory.get_all_items(NBInventoryItem)
     assert len(items) == 1
     assert items[0].data["name"] == "BP_PSV 0:1 Slot 3"
 
 
-def test_two_enclosures_with_structured_locations_stay_distinct():
+def test_two_enclosures_with_structured_locations_stay_distinct(check_redfish_source):
     """Dropping the unusable location must not merge two enclosures onto one name."""
 
-    source, inventory, _ = make_source()
+    context = check_redfish_source()
 
-    source.inventory_file_content = {"inventory": {"storage_enclosure": [
+    context.source.inventory_file_content = {"inventory": {"storage_enclosure": [
         enclosure("BP_PSV 0:1", DELL_LOCATION, "ENC-AAA")["inventory"]["storage_enclosure"][0],
         enclosure("BP_PSV 0:2", DELL_LOCATION, "ENC-BBB")["inventory"]["storage_enclosure"][0],
     ]}}
-    source.update_storage_enclosure()
+    context.source.update_storage_enclosure()
 
-    names = sorted(item.data["name"] for item in inventory.get_all_items(NBInventoryItem))
+    names = sorted(item.data["name"] for item in context.inventory.get_all_items(NBInventoryItem))
     assert names == ["BP_PSV 0:1", "BP_PSV 0:2"]