diff --git a/module/sources/check_redfish/import_inventory.py b/module/sources/check_redfish/import_inventory.py index 91add4e1..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 @@ -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): 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..68b378d9 --- /dev/null +++ b/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