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

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()

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",

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