From 35f7aacceb28e4c806829759a30df1e4d7d2d012 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Mon, 7 Sep 2026 14:05:43 +0200 Subject: [PATCH 1/2] moves the check_redfish test source builder into a fixture Both check_redfish test modules carried their own copy of the same setup: reset the inventory singleton, build a CheckRedfish through object.__new__, run the real add_necessary_base_objects(), and add a device to hang components off. Two more copies were about to arrive with the interface IP and device serial tests. conftest.py gains a check_redfish_source fixture which returns a builder. It reuses the existing inventory fixture instead of resetting the singleton again, so the reset lives in one place, and it takes the source settings as keyword arguments because each entry point of the source reads a different subset of them. It returns the source, the inventory and the device as a namespace. The primary tag is now always registered. Only the orphan tagging tests needed it, but the NetBox handler registers it in production regardless, so making it unconditional brings the fixture closer to a real run rather than further from it. No test assertion changed. Reverting either of the two fixes these modules cover still fails them, so the shared setup does not weaken what they check. --- tests/conftest.py | 57 ++++++++++++++ tests/test_check_redfish_inventory_items.py | 60 ++++----------- tests/test_check_redfish_orphan_tagging.py | 83 +++++++-------------- 3 files changed, 101 insertions(+), 99 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 5f10e929..0594903b 100644 --- a/tests/conftest.py +++ b/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) diff --git a/tests/test_check_redfish_inventory_items.py b/tests/test_check_redfish_inventory_items.py index 0b6b425b..2299d534 100644 --- a/tests/test_check_redfish_inventory_items.py +++ b/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"] diff --git a/tests/test_check_redfish_orphan_tagging.py b/tests/test_check_redfish_orphan_tagging.py index aa45879d..288a4be9 100644 --- a/tests/test_check_redfish_orphan_tagging.py +++ b/tests/test_check_redfish_orphan_tagging.py @@ -7,34 +7,7 @@ import types from module.netbox.connection import NetBoxHandler -from module.netbox.inventory import NetBoxInventory -from module.netbox.object_classes import NBDevice, NBInventoryItem, NBTag -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() - # 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 source, inventory, device +from module.netbox.object_classes import NBInventoryItem def make_netbox_handler(): @@ -59,63 +32,63 @@ def existing_item(source, device, name, inventory_type, health="OK"): return item -def test_components_are_marked_absent_when_the_scan_reports_none_of_their_type(): +def test_components_are_marked_absent_when_the_scan_reports_none_of_their_type(check_redfish_source): """A scan reporting no fan says the fans are gone, so they must be marked absent.""" - source, inventory, device = make_source() - source.inventory.source_list.append(source) - fan = existing_item(source, device, "Fan 1 (ID: 1)", "Fan") + context = check_redfish_source() + context.inventory.source_list.append(context.source) + fan = existing_item(context.source, context.device, "Fan 1 (ID: 1)", "Fan") - source.inventory_file_content = {"inventory": {"fan": []}} - source.update_fan() + context.source.inventory_file_content = {"inventory": {"fan": []}} + context.source.update_fan() assert fan.data["custom_fields"]["health"] == "Absent" - assert fan.source is source, "the run must claim the item, or it is tagged orphaned" + assert fan.source is context.source, "the run must claim the item, or it is tagged orphaned" - inventory.tag_all_the_things(make_netbox_handler()) + context.inventory.tag_all_the_things(make_netbox_handler()) assert NetBoxHandler.orphaned_tag not in fan.get_tags() -def test_components_already_absent_are_claimed_again_on_every_run(): +def test_components_already_absent_are_claimed_again_on_every_run(check_redfish_source): """An item already at absent must still be claimed, or it is tagged orphaned.""" - source, inventory, device = make_source() - source.inventory.source_list.append(source) - fan = existing_item(source, device, "Fan 1 (ID: 1)", "Fan", health="Absent") + context = check_redfish_source() + context.inventory.source_list.append(context.source) + fan = existing_item(context.source, context.device, "Fan 1 (ID: 1)", "Fan", health="Absent") - source.inventory_file_content = {"inventory": {"fan": []}} - source.update_fan() + context.source.inventory_file_content = {"inventory": {"fan": []}} + context.source.update_fan() - assert fan.source is source, "an already absent item must still be claimed by the run" + assert fan.source is context.source, "an already absent item must still be claimed by the run" - inventory.tag_all_the_things(make_netbox_handler()) + context.inventory.tag_all_the_things(make_netbox_handler()) assert NetBoxHandler.orphaned_tag not in fan.get_tags() -def test_components_of_another_type_are_left_alone(): +def test_components_of_another_type_are_left_alone(check_redfish_source): """An empty fan batch says nothing about the CPUs, which must not be marked absent.""" - source, inventory, device = make_source() - source.inventory.source_list.append(source) - cpu = existing_item(source, device, "Socket 1", "CPU") + context = check_redfish_source() + context.inventory.source_list.append(context.source) + cpu = existing_item(context.source, context.device, "Socket 1", "CPU") - source.inventory_file_content = {"inventory": {"fan": []}} - source.update_fan() + context.source.inventory_file_content = {"inventory": {"fan": []}} + context.source.update_fan() assert cpu.data["custom_fields"]["health"] == "OK" -def test_reported_components_are_still_updated_normally(): +def test_reported_components_are_still_updated_normally(check_redfish_source): """The empty batch handling must not disturb the ordinary path.""" - source, inventory, device = make_source() + context = check_redfish_source() - source.inventory_file_content = {"inventory": {"fan": [ + context.source.inventory_file_content = {"inventory": {"fan": [ {"name": "Fan 1", "id": "1", "health_status": "OK", "operation_status": "Enabled", "physical_context": "CPU", "reading": 4200, "reading_unit": "RPM"}]}} - source.update_fan() + context.source.update_fan() - items = inventory.get_all_items(NBInventoryItem) + items = context.inventory.get_all_items(NBInventoryItem) assert len(items) == 1 assert items[0].data["custom_fields"]["health"] == "OK" assert items[0].data["custom_fields"]["inventory_type"] == "Fan" From 1c1ce9879a07c524de2199ef75dd308a59b24dc3 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Thu, 3 Sep 2026 17:40:14 +0200 Subject: [PATCH 2/2] keeps management IPs on interfaces the source discovered no IPs for add_update_interface() removes every IP on an interface which the source did not report. The removal is gated only on skip_ip_handling, so it runs whenever a source hands over an empty IP list, treating "I found nothing" as "there is nothing". For check_redfish that assumption is wrong. Redfish reports the BMC IP and little else, while map_object_interfaces_to_current_interfaces() matches Redfish NIC ports to existing NetBox interfaces by MAC. An OS bond or bridge (pnet0, bond0, sometimes eth0) inherits the physical NIC's MAC, so a Redfish port matches the OS interface that carries the management IP. That IP is not in the Redfish data, so it was unassigned on every sync, and unassigning the device's primary IP also clears primary_ip4 on the device. permitted_subnets does not protect against this: it filters which IPs are added, not which are removed. add_update_interface() gains keep_undiscovered_ips, default False, so vmware and every other source are unchanged. When it is set, the removal loop is skipped for an interface the source discovered no IPs for. An interface which reports a different set of IPs is untouched, so a genuine IP removal is still synced. The check_redfish source passes it. The condition reads interface_ips, what the source reported, not ip_address_objects, what survived parsing and prefix matching. An address which fails to parse is dropped with a continue, so keying on the result would treat a non-empty discovery holding one bad address as "discovered nothing" and would keep stale IPs. The tests drive the real add_update_interface() removal loop and assert on the queued de-assignment, covering the flag, the default, a non-empty discovery, and a discovery whose only address is unusable. --- .../sources/check_redfish/import_inventory.py | 4 +- module/sources/common/source_base.py | 10 ++- tests/test_check_redfish_interface_ips.py | 64 +++++++++++++++++++ 3 files changed, 75 insertions(+), 3 deletions(-) create mode 100644 tests/test_check_redfish_interface_ips.py diff --git a/module/sources/check_redfish/import_inventory.py b/module/sources/check_redfish/import_inventory.py index e4a05426..a5eefd76 100644 --- a/module/sources/check_redfish/import_inventory.py +++ b/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): diff --git a/module/sources/common/source_base.py b/module/sources/common/source_base.py index d1934d16..a5d79f38 100644 --- a/module/sources/common/source_base.py +++ b/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": diff --git a/tests/test_check_redfish_interface_ips.py b/tests/test_check_redfish_interface_ips.py new file mode 100644 index 00000000..34d9125e --- /dev/null +++ b/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