feat(enroll-fw): enroll a firewall without ports and let inspection label them PUC-2128

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"

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()
@@ -441,7 +442,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}

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]

python/understack-workflows/tests/test_enroll_fw.py

@@ -450,3 +450,112 @@ 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, physnet=None):
+    """Mock a new node whose inspection leaves ports with these 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",
+        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 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):
+    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):
+    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):
+    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()
+
+
+@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": " ", "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",
+            ]
+        )

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 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
@@ -135,6 +138,19 @@ def enroll_fw(
         )
         return node.uuid
 
+    if not ports:
+        return _enroll_by_inspection(
+            client=client,
+            name=name,
+            physical_network=physical_network,
+            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 +176,85 @@ def _get_node_or_none(client: IronicClient, name: str):
         return None
 
 
+def _enroll_by_inspection(
+    *,
+    client: IronicClient,
+    name: str,
+    physical_network: 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.
+
+    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}
+    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")
+    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
+
+
 def _update_active_firewall(
     *,
     client: IronicClient,
@@ -196,14 +291,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, ""):
@@ -271,21 +367,18 @@ def argument_parser():
     parser.add_argument(
         "--physical-network",
         required=True,
-        help="Port physical_network",
+        help="Port physical_network, the VLAN group in Nautobot (e.g. f20-3-network)",
     )
     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",

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,12 @@ spec:
   arguments:
     parameters:
       - name: name
+      # VLAN group in Nautobot, e.g. f20-3-network.
       - name: physical_network
-      # JSON array of ports (same format as enroll-netdev).
+      # 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
@@ -94,7 +98,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: