From f0f2905f5b54c01941dde9c18fcac0746ab3e9c5 Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Fri, 30 Oct 2026 20:07:58 +0530 Subject: [PATCH 1/6] feat(enroll-fw): enroll a firewall without ports and let inspection label them --- .../tests/test_enroll_fw.py | 57 +++++++++++ .../understack_workflows/main/enroll_fw.py | 97 ++++++++++++++++--- .../workflowtemplates/enroll-fw.yaml | 9 +- 3 files changed, 145 insertions(+), 18 deletions(-) diff --git a/python/understack-workflows/tests/test_enroll_fw.py b/python/understack-workflows/tests/test_enroll_fw.py index c01bbaa59..97b4bae6f 100644 --- a/python/understack-workflows/tests/test_enroll_fw.py +++ b/python/understack-workflows/tests/test_enroll_fw.py @@ -450,3 +450,60 @@ def test_active_node_with_port_roles_still_gets_metadata_updates(mocker): engine.assert_not_called() client.update_port.assert_not_called() client.update_node.assert_called_once() # metadata only + + +# --- enrollment without ports (inspection labels them) ---------------------- + + +def _enroll_without_ports(mocker, categories): + """Mock a new node whose inspection leaves ports with these categories.""" + _mock_client(mocker, node_ports=[SimpleNamespace(category=c) for c in categories]) + mocker.patch.object( + enroll_fw.netdev_reconciler, + "find_or_create_netdev_node", + return_value=(_node(provision_state="enroll"), True), + ) + engine = mocker.patch.object(enroll_fw.netdev_reconciler, "enroll") + transition = mocker.patch.object(enroll_fw.ironic_node, "transition") + return engine, transition + + +def _steps(transition): + return [call.kwargs["target_state"] for call in transition.call_args_list] + + +def test_enroll_without_ports_inspects_before_providing(mocker): + engine, transition = _enroll_without_ports(mocker, ["outside", "inside", None]) + + node_uuid = enroll_fw.enroll_fw(**{**BASE_ARGS, "ports": []}, **FW_FIELDS) + + assert node_uuid == "node-uuid" + engine.assert_not_called() + assert _steps(transition) == ["manage", "inspect", "provide"] + + +def test_enroll_without_ports_is_not_provided_without_both_roles(mocker): + _engine, transition = _enroll_without_ports(mocker, ["outside", None]) + + with pytest.raises(RuntimeError, match="an outside and an inside port"): + enroll_fw.enroll_fw(**{**BASE_ARGS, "ports": []}, **FW_FIELDS) + + assert _steps(transition) == ["manage", "inspect"] + + +def test_active_node_without_ports_updates_metadata_only(mocker): + client = _mock_client(mocker, node=_node()) + + enroll_fw.enroll_fw(**{**BASE_ARGS, "ports": []}, **FW_FIELDS) + + client.list_ports.assert_not_called() # nothing to compare + client.update_node.assert_called_once() + + +def test_ports_need_a_physical_network(mocker): + client = _mock_client(mocker) + + with pytest.raises(ValueError, match="--physical-network"): + enroll_fw.enroll_fw(**{**BASE_ARGS, "physical_network": " "}, **FW_FIELDS) + + client.get_node.assert_not_called() diff --git a/python/understack-workflows/understack_workflows/main/enroll_fw.py b/python/understack-workflows/understack_workflows/main/enroll_fw.py index 8a9edeff0..4b48d139f 100644 --- a/python/understack-workflows/understack_workflows/main/enroll_fw.py +++ b/python/understack-workflows/understack_workflows/main/enroll_fw.py @@ -7,6 +7,7 @@ from understack_workflows import firewall from understack_workflows import helpers +from understack_workflows import ironic_node from understack_workflows import netdev_reconciler from understack_workflows.ironic.client import IronicClient @@ -98,6 +99,8 @@ def enroll_fw( driver = (driver or "").strip() if not driver: raise ValueError("enroll-fw requires a non-empty --driver") + if ports and not (physical_network or "").strip(): + raise ValueError("enroll-fw requires --physical-network with --ports") resource_class = _require_specific_resource_class(resource_class) management_switch, management_switch_port = _require_management_location( management_switch, management_switch_port @@ -135,6 +138,18 @@ def enroll_fw( ) return node.uuid + if not ports: + return _enroll_by_inspection( + client=client, + name=name, + resource_class=resource_class, + driver=driver, + external_cmdb_id=external_cmdb_id, + driver_info=driver_info, + extra=extra, + properties=properties, + ) + # New or re-enrollable node: the engine writes the metadata inside the # enrollment lifecycle (at create, or patched before the node becomes # available), so the node is never allocatable in an under-described state. @@ -160,6 +175,58 @@ def _get_node_or_none(client: IronicClient, name: str): return None +def _enroll_by_inspection( + *, + client: IronicClient, + name: str, + resource_class: str, + driver: str, + external_cmdb_id: int | str | None, + driver_info: dict, + extra: dict, + properties: dict, +) -> str: + """Enroll a firewall without ports and let inspection find and label them. + + The node is provided only once it has an outside and an inside port, so a + router can never adopt it unlabelled. + """ + if external_cmdb_id not in (None, ""): + extra = {**extra, "external_cmdb_id": external_cmdb_id} + node, created = netdev_reconciler.find_or_create_netdev_node( + client=client, + name=name, + resource_class=resource_class, + driver=driver, + driver_info=driver_info, + extra=extra, + properties=properties, + ) + if node.provision_state != "manageable": + ironic_node.transition(node, target_state="manage", expected_state="manageable") + if not created: + netdev_reconciler.update_node_metadata( + client=client, + node=node, + resource_class=resource_class, + driver_info=driver_info, + extra=extra, + properties=properties, + ) + + logger.info("[node:%s] Inspecting to discover and label the ports", node.uuid) + ironic_node.transition(node, target_state="inspect", expected_state="manageable") + found = {getattr(port, "category", None) for port in client.list_ports(node.uuid)} + if not found.issuperset(netdev_reconciler.PORT_ROLES): + raise RuntimeError( + f"[node:{node.uuid}] Inspection did not give the node an outside and " + "an inside port; leaving it manageable" + ) + + ironic_node.transition(node, target_state="provide", expected_state="available") + return node.uuid + + def _update_active_firewall( *, client: IronicClient, @@ -196,14 +263,15 @@ def _update_active_firewall( f"{resource_class!r} on an in-service firewall" ) - netdev_ports = netdev_reconciler.build_netdev_ports(ports) - _reject_structural_drift( - client=client, - node=node, - name=name, - physical_network=physical_network, - netdev_ports=netdev_ports, - ) + # Without ports there is nothing to compare: inspection owns them. + if ports: + _reject_structural_drift( + client=client, + node=node, + name=name, + physical_network=physical_network, + netdev_ports=netdev_reconciler.build_netdev_ports(ports), + ) active_extra = dict(extra) if external_cmdb_id not in (None, ""): @@ -270,22 +338,19 @@ def argument_parser(): parser.add_argument("--name", required=True, help="Ironic node name") parser.add_argument( "--physical-network", - required=True, - help="Port physical_network", + default="", + help="Port physical_network. Required with --ports.", ) parser.add_argument( "--ports", - required=False, default="[]", - help="Optional JSON array of ports to override LLDP discovery (same format " - "as enroll-netdev). By default, ports are discovered automatically via LLDP " - "during inspection. Use this option to manually specify ports that may not " - "be discovered properly or to override auto-discovered values. 'switch' must " + help="JSON array of ports (same format as enroll-netdev). 'switch' must " "be the switch FQDN (e.g. n11-22-1.dfw3.rackspace.net) so the Nautobot " "sync can resolve the cable. Optional per-port 'bios_name' (the device " "interface name); defaults to the label. At least two ports: the " "lowest-numbered interface becomes the router's outside NIC (gateway) " - "and the next one its inside NIC (subnets).", + "and the next one its inside NIC (subnets). Leave it out to have " + "inspection discover the ports over LLDP and label them.", ) parser.add_argument( "--resource-class", diff --git a/workflows/argo-events/workflowtemplates/enroll-fw.yaml b/workflows/argo-events/workflowtemplates/enroll-fw.yaml index 9fd615360..ea0da18f7 100644 --- a/workflows/argo-events/workflowtemplates/enroll-fw.yaml +++ b/workflows/argo-events/workflowtemplates/enroll-fw.yaml @@ -8,7 +8,8 @@ metadata: Defined in `workflows/argo-events/workflowtemplates/enroll-fw.yaml`. Reuses enroll-netdev for the node/port lifecycle and additionally records firewall management details in driver_info and the HA mate serial in extra. - Defaults to the panos hardware type. + Defaults to the panos hardware type. Without ports, it inspects the + firewall to discover and label them, and only then makes it available. The workflow reads site-specific configuration from the `enroll-fw-config` ConfigMap. After enrollment, it runs the templates @@ -35,9 +36,13 @@ spec: arguments: parameters: - name: name + # Needed only with ports. - name: physical_network - # JSON array of ports (same format as enroll-netdev). + value: "" + # JSON array of ports (same format as enroll-netdev). Leave it as [] to + # let inspection discover and label the ports. - name: ports + value: "[]" - name: external_cmdb_id value: "" - name: resource_class From 40d567516a872c941c8d38ee19e53a8ff4492cfc Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Mon, 2 Nov 2026 19:42:39 +0530 Subject: [PATCH 2/6] dev only: run enroll-fw from the pr-2619 image --- workflows/argo-events/workflowtemplates/enroll-fw.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/workflows/argo-events/workflowtemplates/enroll-fw.yaml b/workflows/argo-events/workflowtemplates/enroll-fw.yaml index ea0da18f7..99ff85492 100644 --- a/workflows/argo-events/workflowtemplates/enroll-fw.yaml +++ b/workflows/argo-events/workflowtemplates/enroll-fw.yaml @@ -99,7 +99,8 @@ spec: valueFrom: path: /run/enrollment/node-uuid container: - image: ghcr.io/rackerlabs/understack/ironic-nautobot-client:latest + image: ghcr.io/rackerlabs/understack/ironic-nautobot-client:pr-2619 + imagePullPolicy: Always command: - enroll-fw args: From f8383ee869f35fd6b686cff455f1c3c59b60946a Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Tue, 3 Nov 2026 00:17:43 +0530 Subject: [PATCH 3/6] fix of Unexpected exception of type TypeError: 'list' object is not callable --- .../ironic_understack/drivers/panos_inspect.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py index d01e107e1..dced0c1c2 100644 --- a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py +++ b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py @@ -441,7 +441,7 @@ def _validate_firewall_readiness(task) -> None: :raises HardwareInspectionFailure: if requirements not met """ node = task.node - ports = list(task.ports()) + ports = list(task.ports) # Check port categories (should have been assigned during port creation) categories = {getattr(p, "category", None) for p in ports} From 5cbb8c5c1d2aaebc200128470fc86b8b3d38e76f Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Tue, 3 Nov 2026 00:41:14 +0530 Subject: [PATCH 4/6] physnet gets None during in inspection --- .../ironic_understack/drivers/panos_inspect.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py index dced0c1c2..b526ac665 100644 --- a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py +++ b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py @@ -402,7 +402,8 @@ def _interface_sort_key(iface_name: str) -> tuple: existing_port.address = mac_address existing_port.local_link_connection = inspected_port.local_link_connection existing_port.extra = {"bios_name": local_interface} - existing_port.physical_network = physical_network + if physical_network and not existing_port.physical_network: + existing_port.physical_network = physical_network existing_port.category = category existing_port.name = port_name existing_port.save() From 63d28ea4aeb4cfb35aec407ca8461457370d7bb8 Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Tue, 3 Nov 2026 01:04:57 +0530 Subject: [PATCH 5/6] take physnet from enroll --- .../tests/test_panos_inspect.py | 35 ++++++++++ .../tests/test_enroll_fw.py | 66 +++++++++++++++++-- .../understack_workflows/main/enroll_fw.py | 44 ++++++++++--- .../workflowtemplates/enroll-fw.yaml | 3 +- 4 files changed, 131 insertions(+), 17 deletions(-) diff --git a/python/ironic-understack/ironic_understack/tests/test_panos_inspect.py b/python/ironic-understack/ironic_understack/tests/test_panos_inspect.py index b19bfa965..150c037b8 100644 --- a/python/ironic-understack/ironic_understack/tests/test_panos_inspect.py +++ b/python/ironic-understack/ironic_understack/tests/test_panos_inspect.py @@ -85,3 +85,38 @@ def test_apply_system_info_ignores_empty_fields(task): panos_inspect._apply_system_info(task.node, {"model": "", "serial": ""}) assert task.node.properties == {} assert task.node.extra == {} + + +@pytest.mark.parametrize( + ("current", "derived", "expected"), + [ + ("f20-3-network", None, "f20-3-network"), + ("f20-3-network", "f20-3/f20-4-network", "f20-3-network"), + (None, "f20-3-network", "f20-3-network"), + ], + ids=["not-wiped", "not-replaced", "filled-in"], +) +def test_existing_port_keeps_its_physical_network( + task, mocker, current, derived, expected +): + task.node.name = "fw1" + macs = {"ethernet1/1": "60:15:2b:54:1a:10", "ethernet1/2": "60:15:2b:54:1a:11"} + task.ports = [Mock(address=mac, physical_network=current) for mac in macs.values()] + neighbors = [ + { + "local_interface": interface, + "system_name": "f20-3-1.iad3", + "port_id": f"Ethernet1/{n}", + "chassis_id": "c4:7e:e0:e4:55:3f", + } + for n, interface in enumerate(macs, start=43) + ] + mocker.patch.object( + panos_inspect.vlan_group_name_convention, + "vlan_group_names", + return_value={"f20-3-1.iad3": derived}, + ) + + panos_inspect._update_ports_from_neighbors(task, neighbors, macs) + + assert [p.physical_network for p in task.ports] == [expected, expected] diff --git a/python/understack-workflows/tests/test_enroll_fw.py b/python/understack-workflows/tests/test_enroll_fw.py index 97b4bae6f..5c4d4e4dd 100644 --- a/python/understack-workflows/tests/test_enroll_fw.py +++ b/python/understack-workflows/tests/test_enroll_fw.py @@ -455,9 +455,20 @@ def test_active_node_with_port_roles_still_gets_metadata_updates(mocker): # --- enrollment without ports (inspection labels them) ---------------------- -def _enroll_without_ports(mocker, categories): +def _enroll_without_ports(mocker, categories, physnet=None): """Mock a new node whose inspection leaves ports with these categories.""" - _mock_client(mocker, node_ports=[SimpleNamespace(category=c) for c in categories]) + client = _mock_client( + mocker, + node_ports=[ + SimpleNamespace( + uuid=f"port-{i}", + name=f"fw:ethernet1/{i}", + category=c, + physical_network=physnet, + ) + for i, c in enumerate(categories, start=1) + ], + ) mocker.patch.object( enroll_fw.netdev_reconciler, "find_or_create_netdev_node", @@ -465,30 +476,51 @@ def _enroll_without_ports(mocker, categories): ) engine = mocker.patch.object(enroll_fw.netdev_reconciler, "enroll") transition = mocker.patch.object(enroll_fw.ironic_node, "transition") - return engine, transition + return client, engine, transition def _steps(transition): return [call.kwargs["target_state"] for call in transition.call_args_list] +def _physnet_updates(client): + return {call.args[0]: call.args[1] for call in client.update_port.call_args_list} + + def test_enroll_without_ports_inspects_before_providing(mocker): - engine, transition = _enroll_without_ports(mocker, ["outside", "inside", None]) + client, engine, transition = _enroll_without_ports( + mocker, ["outside", "inside", None] + ) node_uuid = enroll_fw.enroll_fw(**{**BASE_ARGS, "ports": []}, **FW_FIELDS) assert node_uuid == "node-uuid" engine.assert_not_called() assert _steps(transition) == ["manage", "inspect", "provide"] + # Only the role ports get the physical network. + patch = [{"op": "add", "path": "/physical_network", "value": "f20-1-network"}] + assert _physnet_updates(client) == {"port-1": patch, "port-2": patch} + + +def test_enroll_without_ports_keeps_a_matching_physical_network(mocker): + client, _engine, transition = _enroll_without_ports( + mocker, ["outside", "inside"], physnet="f20-1-network" + ) + + enroll_fw.enroll_fw(**{**BASE_ARGS, "ports": []}, **FW_FIELDS) + + client.update_port.assert_not_called() + assert _steps(transition) == ["manage", "inspect", "provide"] def test_enroll_without_ports_is_not_provided_without_both_roles(mocker): - _engine, transition = _enroll_without_ports(mocker, ["outside", None]) + client, _engine, transition = _enroll_without_ports(mocker, ["outside", None]) with pytest.raises(RuntimeError, match="an outside and an inside port"): enroll_fw.enroll_fw(**{**BASE_ARGS, "ports": []}, **FW_FIELDS) assert _steps(transition) == ["manage", "inspect"] + client.update_port.assert_not_called() def test_active_node_without_ports_updates_metadata_only(mocker): @@ -500,10 +532,30 @@ def test_active_node_without_ports_updates_metadata_only(mocker): client.update_node.assert_called_once() -def test_ports_need_a_physical_network(mocker): +@pytest.mark.parametrize("ports", [BASE_ARGS["ports"], []], ids=["ports", "no-ports"]) +def test_enroll_fw_requires_a_physical_network(mocker, ports): client = _mock_client(mocker) with pytest.raises(ValueError, match="--physical-network"): - enroll_fw.enroll_fw(**{**BASE_ARGS, "physical_network": " "}, **FW_FIELDS) + enroll_fw.enroll_fw( + **{**BASE_ARGS, "physical_network": " ", "ports": ports}, **FW_FIELDS + ) client.get_node.assert_not_called() + + +def test_argument_parser_requires_physical_network(): + parser = enroll_fw.argument_parser() + with pytest.raises(SystemExit): + parser.parse_args( + [ + "--name", + "fw1", + "--resource-class", + "pa1410", + "--management-switch", + "switch1", + "--management-switch-port", + "eth1", + ] + ) diff --git a/python/understack-workflows/understack_workflows/main/enroll_fw.py b/python/understack-workflows/understack_workflows/main/enroll_fw.py index 4b48d139f..9265f0ac7 100644 --- a/python/understack-workflows/understack_workflows/main/enroll_fw.py +++ b/python/understack-workflows/understack_workflows/main/enroll_fw.py @@ -99,8 +99,8 @@ def enroll_fw( driver = (driver or "").strip() if not driver: raise ValueError("enroll-fw requires a non-empty --driver") - if ports and not (physical_network or "").strip(): - raise ValueError("enroll-fw requires --physical-network with --ports") + if not (physical_network or "").strip(): + raise ValueError("enroll-fw requires --physical-network") resource_class = _require_specific_resource_class(resource_class) management_switch, management_switch_port = _require_management_location( management_switch, management_switch_port @@ -142,6 +142,7 @@ def enroll_fw( return _enroll_by_inspection( client=client, name=name, + physical_network=physical_network, resource_class=resource_class, driver=driver, external_cmdb_id=external_cmdb_id, @@ -179,6 +180,7 @@ def _enroll_by_inspection( *, client: IronicClient, name: str, + physical_network: str, resource_class: str, driver: str, external_cmdb_id: int | str | None, @@ -188,8 +190,8 @@ def _enroll_by_inspection( ) -> str: """Enroll a firewall without ports and let inspection find and label them. - The node is provided only once it has an outside and an inside port, so a - router can never adopt it unlabelled. + Provided only once it has an outside and an inside port, which get the + given physical_network. """ if external_cmdb_id not in (None, ""): extra = {**extra, "external_cmdb_id": external_cmdb_id} @@ -216,13 +218,39 @@ def _enroll_by_inspection( logger.info("[node:%s] Inspecting to discover and label the ports", node.uuid) ironic_node.transition(node, target_state="inspect", expected_state="manageable") - found = {getattr(port, "category", None) for port in client.list_ports(node.uuid)} - if not found.issuperset(netdev_reconciler.PORT_ROLES): + role_ports = [ + port + for port in client.list_ports(node.uuid) + if getattr(port, "category", None) in netdev_reconciler.PORT_ROLES + ] + if not {port.category for port in role_ports}.issuperset( + netdev_reconciler.PORT_ROLES + ): raise RuntimeError( f"[node:{node.uuid}] Inspection did not give the node an outside and " "an inside port; leaving it manageable" ) + for port in role_ports: + if port.physical_network != physical_network: + logger.info( + "[node:%s] Setting physical_network=%s on port %s (was %s)", + node.uuid, + physical_network, + port.name, + port.physical_network, + ) + client.update_port( + port.uuid, + [ + { + "op": "add", + "path": "/physical_network", + "value": physical_network, + } + ], + ) + ironic_node.transition(node, target_state="provide", expected_state="available") return node.uuid @@ -338,8 +366,8 @@ def argument_parser(): parser.add_argument("--name", required=True, help="Ironic node name") parser.add_argument( "--physical-network", - default="", - help="Port physical_network. Required with --ports.", + required=True, + help="Port physical_network, the VLAN group in Nautobot (e.g. f20-3-network)", ) parser.add_argument( "--ports", diff --git a/workflows/argo-events/workflowtemplates/enroll-fw.yaml b/workflows/argo-events/workflowtemplates/enroll-fw.yaml index 99ff85492..68cea7db5 100644 --- a/workflows/argo-events/workflowtemplates/enroll-fw.yaml +++ b/workflows/argo-events/workflowtemplates/enroll-fw.yaml @@ -36,9 +36,8 @@ spec: arguments: parameters: - name: name - # Needed only with ports. + # VLAN group in Nautobot, e.g. f20-3-network. - name: physical_network - value: "" # JSON array of ports (same format as enroll-netdev). Leave it as [] to # let inspection discover and label the ports. - name: ports From c758ab49c43f7fabe7b9e0b3849669a3127f9c34 Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Tue, 3 Nov 2026 01:08:44 +0530 Subject: [PATCH 6/6] dev only: run the Ironic conductor from the pr-2619 image --- components/images-openstack.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/images-openstack.yaml b/components/images-openstack.yaml index ab24e6d1e..7466b7e20 100644 --- a/components/images-openstack.yaml +++ b/components/images-openstack.yaml @@ -25,7 +25,7 @@ images: # ironic ironic_api: "ghcr.io/rackerlabs/understack/ironic:2026.1" - ironic_conductor: "ghcr.io/rackerlabs/understack/ironic:2026.1" + ironic_conductor: "ghcr.io/rackerlabs/understack/ironic:pr-2619" ironic_novncproxy: "ghcr.io/rackerlabs/understack/ironic:2026.1" ironic_pxe: "ghcr.io/rackerlabs/understack/ironic:2026.1" ironic_pxe_init: "ghcr.io/rackerlabs/understack/ironic:2026.1"