diff --git a/module/sources/check_redfish/import_inventory.py b/module/sources/check_redfish/import_inventory.py index e4a0542..a5eefd7 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 d1934d1..a5d79f3 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/conftest.py b/tests/conftest.py index 5f10e92..0594903 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_interface_ips.py b/tests/test_check_redfish_interface_ips.py new file mode 100644 index 0000000..34d9125 --- /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 diff --git a/tests/test_check_redfish_inventory_items.py b/tests/test_check_redfish_inventory_items.py index 0b6b425..2299d53 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 aa45879..288a4be 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"