diff --git a/components/ironic/cm-trait-based-networking.yaml b/components/ironic/cm-trait-based-networking.yaml index 76e267a5a..3bbed91d1 100644 --- a/components/ironic/cm-trait-based-networking.yaml +++ b/components/ironic/cm-trait-based-networking.yaml @@ -31,6 +31,17 @@ data: filter: port.category == 'network' max_count: 1 + # Match each parent VIF's role tag to its NIC category. + CUSTOM_NETDEV_ROLE_ATTACH: + order: 1 + actions: + - action: attach_port + filter: port.category == 'outside' && vif.tags == 'outside' + max_count: 1 + - action: attach_port + filter: port.category == 'inside' && vif.tags == 'inside' + max_count: 1 + # Attaches a single category=storage port for a storage VIF. Single-port # only for now (A/B grouping is deferred). Require it on a flavor with: # openstack flavor set --property trait:CUSTOM_STORAGE_ATTACH=required diff --git a/python/neutron-understack/neutron_understack/ironic.py b/python/neutron-understack/neutron_understack/ironic.py index 6ed8238cd..be13871fa 100644 --- a/python/neutron-understack/neutron_understack/ironic.py +++ b/python/neutron-understack/neutron_understack/ironic.py @@ -35,6 +35,24 @@ _RECONCILE_PROVISION_TIMEOUT = 60 _PANOS_DRIVER = "panos" _ROUTER_RELEASE_MARKER = "understack_router_release" # release is in progress +# Adoption sets it in instance_info.traits when a port has a role. +ROLE_ATTACH_TRAIT = "CUSTOM_NETDEV_ROLE_ATTACH" +_TENANT_VIF_KEY = "tenant_vif_port_id" +# Port categories that give a NIC a router role. +_ROLE_CATEGORIES = frozenset({"outside", "inside"}) + + +def uses_port_roles(node) -> bool: + """Return True if Ironic will attach this node's VIFs by port role. + + Reads instance_info, not node traits: Ironic applies a TBN trait only once + it is in instance_info.traits, which adoption sets. + """ + instance_info = getattr(node, "instance_info", None) + if not isinstance(instance_info, dict): + return False + traits = instance_info.get("traits") + return isinstance(traits, list) and ROLE_ATTACH_TRAIT in traits @dataclass(frozen=True) @@ -160,7 +178,7 @@ def router_id_for_release(node: BaremetalNode) -> str | None: return identities[0] def attach_vif_to_node(self, node: str | BaremetalNode, vif_id: str) -> None: - """Attach a Neutron port (VIF) to the node.""" + """Attach a VIF, forwarding optional role tags to Ironic's TBN rules.""" node_id = node.id if isinstance(node, BaremetalNode) else node LOG.info("Attaching VIF %s to Ironic node %s", vif_id, node_id) self.irclient.attach_vif_to_node(node, vif_id) @@ -184,6 +202,15 @@ def node_ports(self, node: str | BaremetalNode) -> list[BaremetalPort]: node_id = node.id if isinstance(node, BaremetalNode) else node return list(self.irclient.ports(details=True, node_id=node_id)) + def port_for_vif( + self, node: str | BaremetalNode, vif_id: str + ) -> BaremetalPort | None: + """Return the node's baremetal port that the VIF is attached to, or None.""" + for port in self.node_ports(node): + if (port.internal_info or {}).get(_TENANT_VIF_KEY) == vif_id: + return port + return None + def adopt_node_for_router( self, node: str | BaremetalNode, @@ -211,6 +238,8 @@ def adopt_node_for_router( router_name, project_id, ) + # Read before manage, so a failed lookup leaves the node untouched. + role_instance_info = self._role_attach_instance_info(node) try: # available -> manageable, required before the adopt verb is valid. # Attempt identity-checked rollback if manage or a later step fails. @@ -232,13 +261,19 @@ def adopt_node_for_router( router_id, router_name, ) - self.irclient.update_node( - node, - retry_on_conflict=False, - lessee=project_id, - instance_id=router_id, - instance_name=router_name, - ) + stamp = { + "lessee": project_id, + "instance_id": router_id, + "instance_name": router_name, + } + if role_instance_info is not None: + LOG.info( + "Setting node %s instance_info.traits to %s", + node_id, + role_instance_info["traits"], + ) + stamp["instance_info"] = role_instance_info + self.irclient.update_node(node, retry_on_conflict=False, **stamp) LOG.info("Node %s: adopt (manageable -> active)", node_id) # Use wait=False: the SDK waits for available after adopt, not active. # https://review.opendev.org/c/openstack/openstacksdk/+/999686 @@ -265,6 +300,31 @@ def adopt_node_for_router( adopted.instance_name, ) + def _role_attach_instance_info(self, node: str | BaremetalNode) -> dict | None: + """Return instance_info whose traits match the node's port roles, or None. + + A node with an outside or inside port gets exactly the role trait, so + no other attach rule can run first; a node without loses a stale one. + None means no change. + Undeploy clears instance_info, so release needs no matching step. + """ + current = ( + node if isinstance(node, BaremetalNode) else self.irclient.get_node(node) + ) + if current is None: + return None + instance_info = dict(current.instance_info or {}) + traits = list(instance_info.get("traits") or []) + ports = self.node_ports(current) + if any(port.category in _ROLE_CATEGORIES for port in ports): + wanted = [ROLE_ATTACH_TRAIT] + else: + wanted = [trait for trait in traits if trait != ROLE_ATTACH_TRAIT] + if wanted == traits: + return None + instance_info["traits"] = wanted + return instance_info + def _return_node_to_available( self, node: str | BaremetalNode, diff --git a/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py b/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py index 41bb35daf..8457cb992 100644 --- a/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py +++ b/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py @@ -2,8 +2,10 @@ A router's gateway and interfaces reach its adopted Ironic node through one stack: a parent port on the shared anchor network, VIF-attached to the node, -with a trunk whose VLAN subports are the router's own ports. This module holds -that wiring logic without any Neutron callback subscriptions. +with a trunk whose VLAN subports are the router's own ports. A node whose ports +carry roles gets one stack per side instead: the gateway on the outside NIC and +the subnets on the inside NIC. This module holds that wiring logic without any +Neutron callback subscriptions. """ import logging @@ -13,6 +15,7 @@ from dataclasses import dataclass from typing import Literal +from neutron.extensions import tagging from neutron_lib import constants as const from neutron_lib import context as n_context from neutron_lib import exceptions as n_exc @@ -23,6 +26,7 @@ from neutron_understack import utils from neutron_understack.ironic import IronicClient +from neutron_understack.ironic import uses_port_roles LOG = logging.getLogger(__name__) @@ -44,6 +48,12 @@ # Which kind of router port a trunk subport carries; used in log messages. PortLabel = Literal["gateway", "interface"] +# On a node whose ports carry roles, each side of the router has its own NIC, +# parent port and trunk: the gateway goes outside, the subnets inside. The +# side's name is also the tag on its parent port. +Side = Literal["outside", "inside"] +LABEL_SIDES: dict[PortLabel, Side] = {"gateway": "outside", "interface": "inside"} + @dataclass(frozen=True) class AttachmentSnapshot: @@ -60,6 +70,8 @@ class AttachmentSnapshot: trunk_id: str | None subport_present: bool parent_vif_attached: bool + # The stack the attach used; None on a node without port roles. + side: Side | None = None @dataclass(frozen=True) @@ -104,14 +116,34 @@ class PaloAltoNodeHasNoUsablePort(n_exc.BadRequest): ) -def _parent_port_name(router_id: str) -> str: - """Deterministic name for the router's anchor-network parent port.""" - return f"{ANCHOR_PARENT_PORT_NAME_PREFIX}-{router_id}" +# BadRequest -> HTTP 400: Ironic did not put a side's parent VIF on that side's NIC. +class PaloAltoParentNotOnSideNic(n_exc.BadRequest): + message = ( + "Palo Alto router %(router_id)s %(side)s parent port %(port_id)s was " + "attached to %(nic)s, not to the node's %(side)s NIC; check the node's " + "port categories and Ironic's CUSTOM_NETDEV_ROLE_ATTACH rule." + ) + + +# BadRequest -> HTTP 400: a node with port roles has no usable NIC for this side. +class PaloAltoNodeHasNoSideNic(n_exc.BadRequest): + message = ( + "Ironic node %(node_id)s for Palo Alto router %(router_id)s has no " + "baremetal port with category %(side)s, physical_network and " + "local_link_connection; check the node's port enrollment." + ) + + +def _parent_port_name(router_id: str, side: Side | None = None) -> str: + """Deterministic name for the router's (or a side's) anchor parent port.""" + name = f"{ANCHOR_PARENT_PORT_NAME_PREFIX}-{router_id}" + return f"{name}-{side}" if side else name -def _trunk_name(router_id: str) -> str: - """Deterministic name for the router's trunk.""" - return f"{TRUNK_NAME_PREFIX}-{router_id}" +def _trunk_name(router_id: str, side: Side | None = None) -> str: + """Deterministic name for the router's (or a side's) trunk.""" + name = f"{TRUNK_NAME_PREFIX}-{router_id}" + return f"{name}-{side}" if side else name def _has_subport(trunk: dict, port_id: str) -> bool: @@ -201,10 +233,33 @@ def _core_plugin(self): def _admin_context(self): return n_context.get_admin_context() + def _tag_plugin(self): + return directory.get_plugin(tagging.TAG_PLUGIN_TYPE) + def _node_for_router(self, router_id: str) -> BaremetalNode | None: """Return the Ironic node adopted for the router, or None.""" return self._ironic().node_by_instance_uuid(router_id) + def _attach_side(self, router_id: str, label: PortLabel) -> Side | None: + """Return the side a new attachment uses, or None without port roles.""" + node = self._node_for_router(router_id) + if node is None or not uses_port_roles(node): + return None + return LABEL_SIDES[label] + + def _existing_side(self, router_id: str, label: PortLabel) -> Side | None: + """Return the side whose stack exists for this label, or None. + + Decided from Neutron alone, so teardown still finds the stack after the + node has been released. + """ + side = LABEL_SIDES[label] + if self._trunk_for_router(router_id, side) is not None: + return side + if self._parent_port_for_router(router_id, side) is not None: + return side + return None + def ensure_anchor_network(self) -> dict: """Create the shared sentinel anchor network if it does not exist.""" core_plugin = self._core_plugin() @@ -265,7 +320,9 @@ def gateway_port_for_router(self, router_id: str) -> dict | None: ) return ports[0] - def _parent_port_for_router(self, router_id: str) -> dict | None: + def _parent_port_for_router( + self, router_id: str, side: Side | None = None + ) -> dict | None: """Return the router's existing anchor-network parent port, or None.""" core_plugin = self._core_plugin() admin_context = self._admin_context() @@ -273,13 +330,13 @@ def _parent_port_for_router(self, router_id: str) -> dict | None: ports = core_plugin.get_ports( admin_context, filters={ - "name": [_parent_port_name(router_id)], + "name": [_parent_port_name(router_id, side)], "network_id": [anchor_network["id"]], }, ) return ports[0] if ports else None - def _create_parent_port(self, router_id: str) -> dict: + def _create_parent_port(self, router_id: str, side: Side | None = None) -> dict: """Create the router's parent port on the anchor network. vnic_type=baremetal so Ironic can VIF-attach it to the adopted node. @@ -287,13 +344,13 @@ def _create_parent_port(self, router_id: str) -> dict: core_plugin = self._core_plugin() admin_context = self._admin_context() anchor_network = self.ensure_anchor_network() - port_name = _parent_port_name(router_id) + port_name = _parent_port_name(router_id, side) LOG.info( "Creating Palo Alto anchor parent port %s for router %s", port_name, router_id, ) - return core_plugin.create_port( + port = core_plugin.create_port( admin_context, { "port": { @@ -309,18 +366,26 @@ def _create_parent_port(self, router_id: str) -> dict: } }, ) + return port - def _ensure_parent_port(self, router_id: str) -> dict: + def _ensure_parent_port(self, router_id: str, side: Side | None = None) -> dict: """Find-or-create the router's anchor-network parent port (idempotent).""" - existing = self._parent_port_for_router(router_id) - if existing is not None: + port = self._parent_port_for_router(router_id, side) + if port is not None: LOG.debug( "Reusing Palo Alto anchor parent port %s for router %s", - existing["id"], + port["id"], router_id, ) - return existing - return self._create_parent_port(router_id) + else: + port = self._create_parent_port(router_id, side) + if side is not None and port.get("tags") != [side]: + # Ironic's TBN rule matches this tag (vif.tags) to the NIC's + # category, so a reused port that lost it is tagged again. + self._tag_plugin().update_tags( + self._admin_context(), "ports", port["id"], {"tags": [side]} + ) + return port def _fresh_port(self, port_id: str) -> dict: """Re-read a port so callers see its current binding profile.""" @@ -348,12 +413,32 @@ def _vif_landed_after_error( return False return parent_port_id in vif_ids - def _ensure_parent_vif_attached(self, router_id: str, parent_port: dict) -> dict: + def _verify_parent_on_side_nic( + self, router_id: str, node, parent_port_id: str, side: Side + ) -> None: + """Fail if Ironic did not put the side's parent VIF on that side's NIC. + + Ironic's TBN rule should place it, but if TBN is off or its rules + changed, the VIF can land on another NIC or none. + """ + nic = self._ironic().port_for_vif(node, parent_port_id) + if nic is None or nic.category != side: + raise PaloAltoParentNotOnSideNic( + router_id=router_id, + side=side, + port_id=parent_port_id, + nic=f"NIC {nic.name}" if nic is not None else "no NIC", + ) + + def _ensure_parent_vif_attached( + self, router_id: str, parent_port: dict, side: Side | None = None + ) -> dict: """VIF-attach the parent port to the router's node (idempotent). Attaching a single VIF; Ironic binds it to a free baremetal port on the node and annotates the Neutron port with local_link_information + - physical_network and host_id . + physical_network and host_id . With a side, Ironic must pick that + side's NIC. Returns a fresh copy of the parent port reflecting the new binding. """ @@ -383,6 +468,8 @@ def _ensure_parent_vif_attached(self, router_id: str, parent_port: dict) -> dict exc_info=True, ) + if side is not None: + self._verify_parent_on_side_nic(router_id, node, parent_port_id, side) fresh = self._fresh_port(parent_port_id) self._verify_parent_annotated(router_id, fresh) return fresh @@ -403,18 +490,22 @@ def _verify_parent_annotated(self, router_id: str, parent_port: dict) -> None: missing=", ".join(missing), ) - def _trunk_for_router(self, router_id: str) -> dict | None: + def _trunk_for_router( + self, router_id: str, side: Side | None = None + ) -> dict | None: """Return the router's existing trunk (by deterministic name), or None.""" admin_context = self._admin_context() trunks = self._trunk_plugin().get_trunks( - admin_context, filters={"name": [_trunk_name(router_id)]} + admin_context, filters={"name": [_trunk_name(router_id, side)]} ) return trunks[0] if trunks else None - def _create_trunk(self, router_id: str, parent_port: dict) -> dict: + def _create_trunk( + self, router_id: str, parent_port: dict, side: Side | None = None + ) -> dict: """Create the router's trunk with the parent port as its trunk parent.""" admin_context = self._admin_context() - trunk_name = _trunk_name(router_id) + trunk_name = _trunk_name(router_id, side) LOG.info( "Creating Palo Alto trunk %s on parent port %s for router %s", trunk_name, @@ -434,9 +525,11 @@ def _create_trunk(self, router_id: str, parent_port: dict) -> dict: }, ) - def _ensure_trunk(self, router_id: str, parent_port: dict) -> dict: + def _ensure_trunk( + self, router_id: str, parent_port: dict, side: Side | None = None + ) -> dict: """Find-or-create the router's trunk (idempotent).""" - existing = self._trunk_for_router(router_id) + existing = self._trunk_for_router(router_id, side) if existing is not None: LOG.debug( "Reusing Palo Alto trunk %s for router %s", @@ -444,26 +537,37 @@ def _ensure_trunk(self, router_id: str, parent_port: dict) -> dict: router_id, ) return existing - return self._create_trunk(router_id, parent_port) + return self._create_trunk(router_id, parent_port, side) def check_can_attach(self, router_id: str) -> None: - """Raise if the router's node cannot be wired. Read-only.""" + """Raise if the router's node cannot be wired for a gateway. Read-only. + + On a node with port roles, the NIC must be the outside one. + """ node = self._node_for_router(router_id) if node is None: raise PaloAltoNodeNotAdopted(router_id=router_id) - if not any(_is_bindable_port(port) for port in self._ironic().node_ports(node)): - raise PaloAltoNodeHasNoUsablePort(router_id=router_id, node_id=node.id) + ports = self._ironic().node_ports(node) + if not uses_port_roles(node): + if not any(_is_bindable_port(port) for port in ports): + raise PaloAltoNodeHasNoUsablePort(router_id=router_id, node_id=node.id) + return + side = LABEL_SIDES["gateway"] + if not any(_is_bindable_port(port) and port.category == side for port in ports): + raise PaloAltoNodeHasNoSideNic( + router_id=router_id, node_id=node.id, side=side + ) - def ensure_stack(self, router_id: str) -> AttachmentStack: + def ensure_stack(self, router_id: str, side: Side | None = None) -> AttachmentStack: """Find or create the router's parent port, VIF attachment and trunk. Builds them in this order so the parent is VIF-bound before any subport is added: the trunk driver only programs the switchport once the parent - is bound. + is bound. With a side, builds that side's stack. """ - parent = self._ensure_parent_port(router_id) - parent = self._ensure_parent_vif_attached(router_id, parent) - trunk = self._ensure_trunk(router_id, parent) + parent = self._ensure_parent_port(router_id, side) + parent = self._ensure_parent_vif_attached(router_id, parent, side) + trunk = self._ensure_trunk(router_id, parent, side) return AttachmentStack(parent=parent, trunk=trunk) def _next_available_subport_vlan( @@ -557,8 +661,8 @@ def add_interface_subport( def attach_port( self, router_id: str, port: dict, label: PortLabel ) -> AttachmentStack: - """Build the router's stack, then add the port as a subport.""" - stack = self.ensure_stack(router_id) + """Build the router's stack (the label's side's, if any), add the subport.""" + stack = self.ensure_stack(router_id, self._attach_side(router_id, label)) if label == "gateway": self.add_gateway_subport(router_id, stack.trunk, port) else: @@ -654,10 +758,12 @@ def cleanup_attachment( Handles a partially-built attach too: if a prior add failed after the parent port was created/VIF-attached but before the trunk existed, there is no trunk to key off, so find and tear down the orphan parent directly. + On a node with port roles, only the label's side's stack is touched. """ - trunk = self._trunk_for_router(router_id) + side = self._existing_side(router_id, label) + trunk = self._trunk_for_router(router_id, side) if trunk is None: - parent = self._parent_port_for_router(router_id) + parent = self._parent_port_for_router(router_id, side) if parent is not None: LOG.info( "No trunk for Palo Alto router %s; deleting orphan parent %s", @@ -678,8 +784,9 @@ def snapshot_attachment( self, router_id: str, port_id: str, label: PortLabel ) -> AttachmentSnapshot: """Record which wiring already exists before attaching a router port.""" - parent = self._parent_port_for_router(router_id) - trunk = self._trunk_for_router(router_id) + side = self._attach_side(router_id, label) + parent = self._parent_port_for_router(router_id, side) + trunk = self._trunk_for_router(router_id, side) parent_vif_attached = False if parent is not None: node = self._node_for_router(router_id) @@ -693,6 +800,7 @@ def snapshot_attachment( trunk_id=trunk["id"] if trunk else None, subport_present=trunk is not None and _has_subport(trunk, port_id), parent_vif_attached=parent_vif_attached, + side=side, ) def undo_attachment(self, snapshot: AttachmentSnapshot) -> None: @@ -701,7 +809,7 @@ def undo_attachment(self, snapshot: AttachmentSnapshot) -> None: Re-reads current state rather than replaying the calls that succeeded, because a remote call can apply its change and still raise. """ - trunk = self._trunk_for_router(snapshot.router_id) + trunk = self._trunk_for_router(snapshot.router_id, snapshot.side) if trunk is not None: if not snapshot.subport_present: self._remove_router_port_subport( @@ -715,7 +823,7 @@ def undo_attachment(self, snapshot: AttachmentSnapshot) -> None: if trunk["id"] != snapshot.trunk_id: self._trunk_plugin().delete_trunk(admin_context, trunk["id"]) - parent = self._parent_port_for_router(snapshot.router_id) + parent = self._parent_port_for_router(snapshot.router_id, snapshot.side) if parent is not None: if parent["id"] != snapshot.parent_id: self._detach_and_delete_parent(snapshot.router_id, parent["id"]) diff --git a/python/neutron-understack/neutron_understack/tests/test_ironic.py b/python/neutron-understack/neutron_understack/tests/test_ironic.py index abbe6a226..9bf74e695 100644 --- a/python/neutron-understack/neutron_understack/tests/test_ironic.py +++ b/python/neutron-understack/neutron_understack/tests/test_ironic.py @@ -4,11 +4,14 @@ inject a mock ``irclient`` (the openstacksdk baremetal proxy). """ +from types import SimpleNamespace + import pytest from openstack.baremetal.v1.node import Node from neutron_understack.ironic import IronicClient from neutron_understack.ironic import NodeReleaseResult +from neutron_understack.ironic import uses_port_roles _DEFAULT_EXTRA = object() @@ -51,6 +54,7 @@ def __init__( fail_manage_once=False, ): self.node = node + self.nics = [] self.fail_undeploy_once = fail_undeploy_once self.fail_cleanup_once = fail_cleanup_once self.fail_manage_once = fail_manage_once @@ -58,6 +62,9 @@ def __init__( def _copy(self): return Node(**self.node.to_dict()) + def ports(self, **filters): + return list(self.nics) + def get_node(self, node): node_id = node.id if isinstance(node, Node) else node if node_id != self.node.id: @@ -165,6 +172,7 @@ def test_manage_failure_is_rolled_back_and_reraised(self, mocker): # re-raise so the create aborts. The recovery leaves a manageable node # parked after clearing ownership. client = _client(mocker) + client.irclient.ports.return_value = [] # read before manage node = _node(state="available", router_id=None) owner = {"understack_router_id": "r"} marker = { @@ -715,3 +723,96 @@ def test_delete_path_keeps_the_api_timeout(self, mocker): client.release_node_for_router("router-1") assert self._timeout_used(client) == 300 + + +class TestUsesPortRoles: + @pytest.mark.parametrize( + ("instance_info", "expected"), + [ + ({"traits": ["CUSTOM_NETDEV_ROLE_ATTACH"]}, True), + ({"traits": ["CUSTOM_OTHER"]}, False), + ({}, False), + ("not-a-dict", False), + ], + ) + def test_reads_instance_info_traits(self, instance_info, expected): + node = SimpleNamespace(instance_info=instance_info) + + assert uses_port_roles(node) is expected + + +class TestRoleTraitOnAdoption: + """Adoption sets instance_info.traits to match the node's port roles.""" + + def _adopt(self, *, categories, instance_info=None, traits=()): + node = _node(state="available", router_id=None, lessee=None, instance_name=None) + node.traits = list(traits) + node.instance_info = instance_info or {} + client = IronicClient.__new__(IronicClient) + client.irclient = _FakeBaremetal(node) + client.irclient.nics = [SimpleNamespace(category=c) for c in categories] + client.adopt_node_for_router( + client.irclient.node, project_id="p", router_id="r-1", router_name="rtr" + ) + return client.irclient.node + + def test_node_without_port_roles_keeps_the_default_attach(self): + node = self._adopt(categories=["network"]) + + assert node.provision_state == "active" + assert node.instance_info == {} + + def test_node_with_port_roles_gets_only_the_role_trait(self): + node = self._adopt( + categories=["outside", "inside", "network"], + instance_info={"traits": ["CUSTOM_NETWORK_PORTGROUP"], "note": "kept"}, + ) + + # Only the role rule may run, or another rule could take a spare NIC. + assert node.provision_state == "active" + assert node.instance_info == { + "traits": ["CUSTOM_NETDEV_ROLE_ATTACH"], + "note": "kept", + } + + def test_a_failed_port_lookup_leaves_the_node_available(self): + node = _node(state="available", router_id=None, lessee=None, instance_name=None) + client = IronicClient.__new__(IronicClient) + client.irclient = _FakeBaremetal(node) + + def lookup_fails(**_): + raise RuntimeError("port lookup failed") + + client.irclient.ports = lookup_fails + + with pytest.raises(RuntimeError, match="port lookup failed"): + client.adopt_node_for_router( + client.irclient.node, project_id="p", router_id="r-1", router_name="rtr" + ) + + # Untouched: still available and unowned. + assert client.irclient.node.provision_state == "available" + assert client.irclient.node.instance_id is None + + def test_node_without_port_roles_loses_a_stale_role_trait(self): + # A leftover node trait is ignored. + node = self._adopt( + categories=["network"], + traits=["CUSTOM_NETDEV_ROLE_ATTACH"], + instance_info={"traits": ["CUSTOM_NETDEV_ROLE_ATTACH"]}, + ) + + assert node.instance_info == {"traits": []} + + +class TestPortForVif: + def test_finds_the_port_holding_the_vif(self, mocker): + client = _client(mocker) + holder = SimpleNamespace(internal_info={"tenant_vif_port_id": "port-1"}) + client.irclient.ports.return_value = [ + SimpleNamespace(internal_info=None), + holder, + ] + + assert client.port_for_vif(_node("n1"), "port-1") is holder + assert client.port_for_vif(_node("n1"), "port-2") is None diff --git a/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py b/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py index 8ae8753b6..f66ad1f12 100644 --- a/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py +++ b/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py @@ -594,6 +594,8 @@ def test_orchestrates_in_order(self, mocker, provider): provider._wiring, "_parent_port_for_router", return_value=None ) mocker.patch.object(provider._wiring, "_trunk_for_router", return_value=None) + # A node without port roles: one stack, no side. + mocker.patch.object(provider._wiring, "_attach_side", return_value=None) stack = palo_alto_wiring.AttachmentStack( parent={"id": "parent-1"}, trunk={"id": "trunk-1"} ) @@ -608,7 +610,7 @@ def test_orchestrates_in_order(self, mocker, provider): # The VIF-bound stack exists before the subport is added to its trunk. assert calls.mock_calls == [ - mocker.call.ensure_stack("r1"), + mocker.call.ensure_stack("r1", None), mocker.call.add_gateway_subport("r1", stack.trunk, {"id": "gw-1"}), ] @@ -634,6 +636,8 @@ def test_orchestrates_in_order(self, mocker, provider): provider._wiring, "_parent_port_for_router", return_value=None ) mocker.patch.object(provider._wiring, "_trunk_for_router", return_value=None) + # A node without port roles: one stack, no side. + mocker.patch.object(provider._wiring, "_attach_side", return_value=None) stack = palo_alto_wiring.AttachmentStack( parent={"id": "parent-1"}, trunk={"id": "trunk-1"} ) @@ -650,7 +654,7 @@ def test_orchestrates_in_order(self, mocker, provider): # The VIF-bound stack exists before the subport is added to its trunk. assert calls.mock_calls == [ - mocker.call.ensure_stack("r1"), + mocker.call.ensure_stack("r1", None), mocker.call.add_interface_subport("r1", stack.trunk, dict(_INTERFACE_PORT)), ] @@ -686,6 +690,7 @@ def test_abort_after_successful_rollback_does_nothing(self, mocker, provider): trunk_lookup = mocker.patch.object( provider._wiring, "_trunk_for_router", return_value=None ) + mocker.patch.object(provider._wiring, "_attach_side", return_value=None) mocker.patch.object( provider._wiring, "_ensure_parent_port", side_effect=RuntimeError("boom") ) diff --git a/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py b/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py index 493f24f67..b0b7dac13 100644 --- a/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py +++ b/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py @@ -382,11 +382,11 @@ def test_binds_parent_before_building_trunk(self, mocker, wiring): stack = wiring.ensure_stack("r1") - m_parent.assert_called_once_with("r1") + m_parent.assert_called_once_with("r1", None) # VIF-attach runs on the parent BEFORE the trunk/subport - m_vif.assert_called_once_with("r1", parent) + m_vif.assert_called_once_with("r1", parent, None) # trunk + subport use the BOUND parent - m_trunk.assert_called_once_with("r1", bound) + m_trunk.assert_called_once_with("r1", bound, None) assert stack == palo_alto_wiring.AttachmentStack(parent=bound, trunk=trunk) @@ -685,3 +685,237 @@ def test_skips_vif_detach_when_node_is_gone( ironic.detach_vif_from_node.assert_not_called() core_plugin.delete_port.assert_not_called() + + +_ROLE_INSTANCE_INFO = {"traits": ["CUSTOM_NETDEV_ROLE_ATTACH"]} + + +def _nic(category, name): + """A bindable Ironic NIC whose category is its port role.""" + return SimpleNamespace( + name=name, + category=category, + physical_network="physnet-1", + local_link_connection={"switch_id": "aa", "port_id": "Eth1/1"}, + ) + + +_OUTSIDE_NIC = _nic("outside", "fw:ethernet1/19") +_INSIDE_NIC = _nic("inside", "fw:ethernet1/20") + + +def _named(by_name): + """A get_ports/get_trunks fake returning what by_name holds for the name.""" + + def lookup(_ctx, filters): + return by_name.get(filters["name"][0], []) + + return lookup + + +class TestSingleStackWithoutPortRoles: + def test_attach_uses_one_unsuffixed_stack_and_no_tags( + self, mocker, wiring, ironic, node, core_plugin, trunk_plugin, device_id + ): + node.instance_info = {} + ironic.node_vif_ids.return_value = [] + core_plugin.get_ports.return_value = [] + core_plugin.create_port.return_value = {"id": "parent-1"} + core_plugin.get_port.return_value = _ANNOTATED_PARENT + trunk_plugin.get_trunks.return_value = [] + trunk_plugin.create_trunk.return_value = {"id": "trunk-1", "sub_ports": []} + tag_plugin = mocker.patch.object(wiring, "_tag_plugin") + + wiring.attach_port("r1", dict(_GATEWAY_PORT), "gateway") + + _ctx, body = core_plugin.create_port.call_args[0] + assert body["port"]["name"] == "palo-alto-router-anchor-r1" + ironic.attach_vif_to_node.assert_called_once_with(node, "parent-1") + ironic.port_for_vif.assert_not_called() + tag_plugin.assert_not_called() + _ctx, body = trunk_plugin.create_trunk.call_args[0] + assert body["trunk"]["name"] == "palo-alto-router-trunk-r1" + + +class TestSidedStacks: + """A node with port roles gets one parent port and trunk per side.""" + + @pytest.fixture(autouse=True) + def _roles_node(self, mocker, wiring, ironic, core_plugin, trunk_plugin, device_id): + self.node = mocker.Mock(id="node-1", instance_info=_ROLE_INSTANCE_INFO) + ironic.node_by_instance_uuid.return_value = self.node + ironic.node_vif_ids.return_value = [] + ironic.port_for_vif.side_effect = lambda _node, vif: { + "parent-outside": _OUTSIDE_NIC, + "parent-inside": _INSIDE_NIC, + }.get(vif) + core_plugin.get_ports.return_value = [] + core_plugin.create_port.side_effect = lambda _ctx, body: { + "id": "parent-" + body["port"]["name"].rsplit("-", 1)[1] + } + core_plugin.get_port.side_effect = lambda _ctx, port_id: { + **_ANNOTATED_PARENT, + "id": port_id, + } + trunk_plugin.get_trunks.return_value = [] + trunk_plugin.create_trunk.side_effect = lambda _ctx, body: { + "id": "trunk-" + body["trunk"]["name"].rsplit("-", 1)[1], + "sub_ports": [], + } + mocker.patch.object( + palo_alto_wiring.utils, + "allowed_tenant_vlan_id_ranges", + return_value=[(1, 1799)], + ) + self.tags = mocker.patch.object(wiring, "_tag_plugin").return_value + + def test_gateway_goes_on_the_outside_stack( + self, wiring, ironic, core_plugin, trunk_plugin + ): + wiring.attach_port("r1", dict(_GATEWAY_PORT), "gateway") + + _ctx, body = core_plugin.create_port.call_args[0] + assert body["port"]["name"] == "palo-alto-router-anchor-r1-outside" + self.tags.update_tags.assert_called_once_with( + ANY, "ports", "parent-outside", {"tags": ["outside"]} + ) + ironic.attach_vif_to_node.assert_called_once_with(self.node, "parent-outside") + _ctx, body = trunk_plugin.create_trunk.call_args[0] + assert body["trunk"]["name"] == "palo-alto-router-trunk-r1-outside" + assert body["trunk"]["port_id"] == "parent-outside" + _ctx, trunk_id, body = trunk_plugin.add_subports.call_args[0] + assert trunk_id == "trunk-outside" + assert ( + body["sub_ports"][0]["segmentation_id"] + == palo_alto_wiring.GATEWAY_SUBPORT_VLAN + ) + + def test_subnet_goes_on_the_inside_stack( + self, wiring, ironic, core_plugin, trunk_plugin + ): + wiring.attach_port("r1", dict(_INTERFACE_PORT), "interface") + + _ctx, body = core_plugin.create_port.call_args[0] + assert body["port"]["name"] == "palo-alto-router-anchor-r1-inside" + self.tags.update_tags.assert_called_once_with( + ANY, "ports", "parent-inside", {"tags": ["inside"]} + ) + ironic.attach_vif_to_node.assert_called_once_with(self.node, "parent-inside") + _ctx, trunk_id, body = trunk_plugin.add_subports.call_args[0] + assert trunk_id == "trunk-inside" + assert ( + body["sub_ports"][0]["segmentation_id"] + == palo_alto_wiring.INTERFACE_SUBPORT_VLAN_START + ) + + @pytest.mark.parametrize(("tags", "retagged"), [([], True), (["outside"], False)]) + def test_reused_parent_keeps_its_side_tag( + self, wiring, core_plugin, tags, retagged + ): + core_plugin.get_ports.return_value = [{"id": "parent-outside", "tags": tags}] + + wiring._ensure_parent_port("r1", "outside") + + core_plugin.create_port.assert_not_called() + assert self.tags.update_tags.called is retagged + + @pytest.mark.parametrize( + ("landed", "where"), + [(_INSIDE_NIC, "NIC fw:ethernet1/20"), (None, "no NIC")], + ids=["wrong-nic", "no-nic"], + ) + def test_rejects_a_vif_that_missed_its_sides_nic( + self, wiring, ironic, trunk_plugin, landed, where + ): + ironic.port_for_vif.side_effect = None + ironic.port_for_vif.return_value = landed + + with pytest.raises( + palo_alto_wiring.PaloAltoParentNotOnSideNic, + match=f"outside parent port parent-outside was attached to {where},", + ): + wiring.attach_port("r1", dict(_GATEWAY_PORT), "gateway") + trunk_plugin.create_trunk.assert_not_called() + + def test_check_can_attach_needs_the_sides_nic(self, wiring, ironic): + ironic.node_ports.return_value = [_INSIDE_NIC] + + with pytest.raises( + palo_alto_wiring.PaloAltoNodeHasNoSideNic, + match="no baremetal port with category outside", + ): + wiring.check_can_attach("r1") + + ironic.node_ports.return_value = [_INSIDE_NIC, _OUTSIDE_NIC] + wiring.check_can_attach("r1") + + def test_snapshot_records_the_side(self, wiring, core_plugin, trunk_plugin): + snapshot = wiring.snapshot_attachment("r1", "gw-1", "gateway") + + assert snapshot.side == "outside" + assert core_plugin.get_ports.call_args.kwargs["filters"]["name"] == [ + "palo-alto-router-anchor-r1-outside" + ] + assert trunk_plugin.get_trunks.call_args.kwargs["filters"]["name"] == [ + "palo-alto-router-trunk-r1-outside" + ] + + def test_cleanup_only_touches_the_sides_stack( + self, wiring, ironic, core_plugin, trunk_plugin + ): + outside = { + "id": "trunk-outside", + "port_id": "parent-outside", + "sub_ports": [{"port_id": "gw-1"}], + } + trunk_plugin.get_trunks.side_effect = _named( + {"palo-alto-router-trunk-r1-outside": [outside]} + ) + trunk_plugin.get_trunk.return_value = {**outside, "sub_ports": []} + + wiring.cleanup_attachment("r1", "gw-1", "gateway") + + _ctx, trunk_id, _body = trunk_plugin.remove_subports.call_args[0] + assert trunk_id == "trunk-outside" + trunk_plugin.delete_trunk.assert_called_once_with(ANY, "trunk-outside") + ironic.detach_vif_from_node.assert_called_once_with(self.node, "parent-outside") + core_plugin.delete_port.assert_called_once_with(ANY, "parent-outside") + + def test_cleanup_still_finds_a_single_stack_router(self, wiring, trunk_plugin): + # A router built before the node had port roles keeps its one stack. + single = { + "id": "trunk-1", + "port_id": "parent-1", + "sub_ports": [{"port_id": "gw-1"}], + } + trunk_plugin.get_trunks.side_effect = _named( + {"palo-alto-router-trunk-r1": [single]} + ) + trunk_plugin.get_trunk.return_value = {**single, "sub_ports": []} + + wiring.cleanup_attachment("r1", "gw-1", "gateway") + + _ctx, trunk_id, _body = trunk_plugin.remove_subports.call_args[0] + assert trunk_id == "trunk-1" + + def test_undo_removes_only_this_sides_new_stack( + self, wiring, core_plugin, trunk_plugin + ): + # The inside attach failed after building its stack; outside is untouched. + inside = { + "id": "trunk-inside", + "port_id": "parent-inside", + "sub_ports": [{"port_id": "intf-1"}], + } + trunk_plugin.get_trunks.side_effect = _named( + {"palo-alto-router-trunk-r1-inside": [inside]} + ) + trunk_plugin.get_trunk.return_value = {**inside, "sub_ports": []} + core_plugin.get_ports.side_effect = _named( + {"palo-alto-router-anchor-r1-inside": [{"id": "parent-inside"}]} + ) + + wiring.undo_attachment(_snapshot(side="inside")) + + trunk_plugin.delete_trunk.assert_called_once_with(ANY, "trunk-inside") + core_plugin.delete_port.assert_called_once_with(ANY, "parent-inside") diff --git a/python/understack-workflows/tests/test_enroll_fw.py b/python/understack-workflows/tests/test_enroll_fw.py index 89adefcc6..c01bbaa59 100644 --- a/python/understack-workflows/tests/test_enroll_fw.py +++ b/python/understack-workflows/tests/test_enroll_fw.py @@ -115,6 +115,7 @@ def test_enroll_fw_hands_metadata_to_the_engine(mocker): }, extra={"mate_serial": "026701010045"}, properties={}, + assign_roles=True, ) @@ -435,3 +436,17 @@ def test_argument_parser_requires_management_switch_and_port(): "pa1410", ] ) + + +def test_active_node_with_port_roles_still_gets_metadata_updates(mocker): + # The request carries no roles here, so an "outside" port is not drift. + labelled = _actual_port() + labelled.category = "outside" + client = _mock_client(mocker, node=_node(), node_ports=[labelled]) + engine = mocker.patch.object(enroll_fw.netdev_reconciler, "enroll") + + enroll_fw.enroll_fw(**BASE_ARGS, **FW_FIELDS) + + engine.assert_not_called() + client.update_port.assert_not_called() + client.update_node.assert_called_once() # metadata only diff --git a/python/understack-workflows/tests/test_netdev_reconciler.py b/python/understack-workflows/tests/test_netdev_reconciler.py index 809857a60..891c823ca 100644 --- a/python/understack-workflows/tests/test_netdev_reconciler.py +++ b/python/understack-workflows/tests/test_netdev_reconciler.py @@ -1172,3 +1172,199 @@ def test_enroll_does_not_touch_port_with_real_switch_id_and_matching_metadata( # update and no state churn on the available node. fake_ironic.port.update.assert_not_called() fake_ironic.node.set_provision_state.assert_not_called() + + +# --- port roles (outside / inside), assigned by enroll-fw ------------------- + + +def _port(label, bios_name=None): + return netdev_reconciler.NetdevPort( + label=label, + mac=f"00:00:00:00:00:{len(label):02x}", + switch="leaf-1", + interface="Ethernet1/1", + bios_name=bios_name or label, + ) + + +def _roles(ports): + return { + port.label: port.category for port in netdev_reconciler.assign_port_roles(ports) + } + + +def test_lowest_interface_is_outside_comparing_numbers(): + # Given in reverse; as plain text "ethernet1/10" would sort before "ethernet1/9". + ports = [_port("ethernet1/10"), _port("ethernet1/9")] + + assert _roles(ports) == {"ethernet1/9": "outside", "ethernet1/10": "inside"} + + +def test_more_than_two_ports_label_only_the_two_lowest(): + ports = [_port("ethernet1/3"), _port("ethernet1/1"), _port("ethernet1/2")] + + assert _roles(ports) == { + "ethernet1/1": "outside", + "ethernet1/2": "inside", + "ethernet1/3": None, + } + + +def test_a_single_port_is_refused(): + with pytest.raises(ValueError, match="at least two data ports"): + netdev_reconciler.assign_port_roles([_port("ethernet1/1")]) + + +def test_the_device_interface_name_wins_over_the_label(): + ports = [ + _port("port-a", bios_name="ethernet1/20"), + _port("port-b", bios_name="ethernet1/19"), + ] + + assert _roles(ports) == {"port-b": "outside", "port-a": "inside"} + + +_THIRD_PORT = { + "label": "port3", + "mac": "00:11:22:33:44:77", + "switch": "spine03.example.net", + "intf": "Ethernet1/3", +} +_THREE_PORTS = {**ENROLL_ARGS, "ports": [*ENROLL_ARGS["ports"], _THIRD_PORT]} + + +def test_firewall_enroll_sets_the_role_categories(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.port.create.side_effect = None + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + netdev_reconciler.enroll(**_THREE_PORTS, assign_roles=True) + + created = fake_ironic.port.create.call_args_list + # The third port gets no category. + assert [c.kwargs.get("category") for c in created] == ["outside", "inside", None] + + +def test_other_netdev_enroll_keeps_network_ports(mocker): + fake_ironic, _ = make_ironic_client() + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + netdev_reconciler.enroll(**ENROLL_ARGS) + + categories = [c.kwargs["category"] for c in fake_ironic.port.create.call_args_list] + assert categories == ["network", "network"] + + +def _available_node(): + return SimpleNamespace( + uuid="node-123", + driver="netdev", + provision_state="available", + resource_class="generic", + extra={}, + ) + + +def test_firewall_enroll_is_noop_when_roles_already_match(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = _available_node() + ports = matching_ports() + ports[0].category = "outside" + ports[1].category = "inside" + fake_ironic.port.list.return_value = ports + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + netdev_reconciler.enroll(**ENROLL_ARGS, assign_roles=True) + + fake_ironic.port.update.assert_not_called() + fake_ironic.node.set_provision_state.assert_not_called() + + +def test_firewall_enroll_labels_an_already_enrolled_available_node(mocker): + # Ports enrolled before roles existed are "network". + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = _available_node() + fake_ironic.port.list.return_value = [ + *matching_ports(), + existing_port( + "port3", "00:11:22:33:44:77", "spine03.example.net", "Ethernet1/3" + ), + ] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + netdev_reconciler.enroll(**_THREE_PORTS, assign_roles=True) + + fake_ironic.port.update.assert_has_calls( + [ + call( + "uuid-port1", + [{"op": "add", "path": "/category", "value": "outside"}], + ), + call( + "uuid-port2", + [{"op": "add", "path": "/category", "value": "inside"}], + ), + call("uuid-port3", [{"op": "remove", "path": "/category"}]), + ] + ) + + +@pytest.mark.parametrize("role", ["outside", "inside"]) +def test_firewall_enroll_refuses_a_left_out_port_that_still_has_a_role(mocker, role): + # A second NIC labelled for the same side could carry the router instead. + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = _available_node() + stale = existing_port( + "port3", + "00:11:22:33:44:77", + "spine03.example.net", + "Ethernet1/3", + category=role, + ) + fake_ironic.port.list.return_value = [*matching_ports(), stale] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + with pytest.raises(RuntimeError, match=f"leaf01:port3 .* still has role '{role}'"): + netdev_reconciler.enroll(**ENROLL_ARGS, assign_roles=True) + + # Refused before anything changed. + fake_ironic.port.update.assert_not_called() + fake_ironic.port.create.assert_not_called() + fake_ironic.node.set_provision_state.assert_not_called() + + +def test_firewall_enroll_still_allows_a_left_out_network_port(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = _available_node() + other = existing_port( + "port3", "00:11:22:33:44:77", "spine03.example.net", "Ethernet1/3" + ) + fake_ironic.port.list.return_value = [*matching_ports(), other] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + netdev_reconciler.enroll(**ENROLL_ARGS, assign_roles=True) + + # The left-out port is only warned about. + assert fake_ironic.port.update.call_count == 2 diff --git a/python/understack-workflows/understack_workflows/main/enroll_fw.py b/python/understack-workflows/understack_workflows/main/enroll_fw.py index 2a415c13e..9506e3b4f 100644 --- a/python/understack-workflows/understack_workflows/main/enroll_fw.py +++ b/python/understack-workflows/understack_workflows/main/enroll_fw.py @@ -148,6 +148,7 @@ def enroll_fw( driver_info=driver_info, extra=extra, properties=properties, + assign_roles=True, ) @@ -241,6 +242,9 @@ def _reject_structural_drift( port=port, existing=ports_by_name.get(f"{name}:{port.label}"), ports_by_mac=ports_by_mac, + # This path assigns no roles, so a live firewall's outside/inside + # labels must not count as drift. + manage_category=False, ) if plan is not None: raise RuntimeError( @@ -275,7 +279,9 @@ def argument_parser(): 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.", + "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).", ) parser.add_argument( "--resource-class", diff --git a/python/understack-workflows/understack_workflows/netdev_reconciler.py b/python/understack-workflows/understack_workflows/netdev_reconciler.py index 527dfe327..e05884c23 100644 --- a/python/understack-workflows/understack_workflows/netdev_reconciler.py +++ b/python/understack-workflows/understack_workflows/netdev_reconciler.py @@ -1,6 +1,8 @@ import json import logging +import re from dataclasses import dataclass +from dataclasses import replace from ironicclient.common.apiclient import exceptions as ironic_exceptions from ironicclient.common.utils import args_array_to_patch @@ -32,6 +34,13 @@ OPTIONAL_PORT_FIELDS = ("switch_id", "bios_name") ALLOWED_PORT_FIELDS = frozenset(REQUIRED_PORT_FIELDS + OPTIONAL_PORT_FIELDS) +# A port's role says which side of a router its NIC carries +# stored as the ironic port's category (other netdev ports keep "network"; +# extra firewall ports get none) +# enroll-fw assigns roles from the interface names: see assign_port_roles. +PORT_ROLES = ("outside", "inside") +DEFAULT_PORT_CATEGORY = "network" + @dataclass(frozen=True) class NetdevPort: @@ -44,6 +53,8 @@ class NetdevPort: # Ironic port's extra.bios_name. The Nautobot device sync uses it to name # the interface. Defaults to the label when not supplied. bios_name: str | None = None + # enroll-fw sets the role here, or None. + category: str | None = DEFAULT_PORT_CATEGORY def _has_cmdb_id(external_cmdb_id: int | str | None) -> bool: @@ -135,6 +146,34 @@ def build_netdev_ports(ports: list[dict]) -> list[NetdevPort]: return result +def _interface_sort_key(port: NetdevPort) -> tuple: + """Order ports by the device's interface name, number by number.""" + name = port.bios_name or port.label + pieces = re.split(r"(\d+)", name) + return ( + tuple((0, int(piece)) if piece.isdigit() else (1, piece) for piece in pieces), + port.label, + ) + + +def assign_port_roles(ports: list[NetdevPort]) -> list[NetdevPort]: + """Give a firewall's ports their router roles from the interface names. + + The lowest-numbered interface carries the router's outside (gateway), the + next one its inside (subnets), and any others get no category. Fewer than + two ports is refused: a firewall needs both. + Note: Inspection, once it discovers firewall ports, must apply the same rule. + """ + if len(ports) < len(PORT_ROLES): + raise ValueError( + "A firewall needs at least two data ports, one outside and one " + f"inside; got {len(ports)}" + ) + ordered = sorted(ports, key=_interface_sort_key) + roles = dict(zip((port.label for port in ordered), PORT_ROLES, strict=False)) + return [replace(port, category=roles.get(port.label)) for port in ports] + + def enroll( *, name: str, @@ -146,10 +185,18 @@ def enroll( driver_info: dict | None = None, extra: dict | None = None, properties: dict | None = None, + assign_roles: bool = False, ) -> str: + """Enroll a netdev node and its ports, then make it available. + + ``assign_roles`` (enroll-fw only) labels ports outside/inside; other netdev + devices are left as they were. + """ effective_resource_class = resource_class or DEFAULT_RESOURCE_CLASS effective_driver = driver or DEFAULT_DRIVER netdev_ports = build_netdev_ports(ports) + if assign_roles: + netdev_ports = assign_port_roles(netdev_ports) # driver_info/extra/properties are generic Ironic node metadata a caller may # want set (e.g. the firewall workflow records management access in @@ -211,6 +258,8 @@ def enroll( ] pending = [plan for plan in plans if plan is not None] + if assign_roles: + _reject_orphan_role_ports(node, name, node_ports, netdev_ports) warn_orphan_ports(node, name, node_ports, netdev_ports) state = node.provision_state @@ -298,6 +347,25 @@ def warn_orphan_ports( ) +def _reject_orphan_role_ports( + node: Node, node_name: str, node_ports: list, netdev_ports: list[NetdevPort] +) -> None: + """Refuse when a port left out of the request still has a role. + + Its old label would give the router two NICs for one side, and Ironic + could pick the stale one. + """ + requested = {f"{node_name}:{port.label}" for port in netdev_ports} + for existing in node_ports: + category = getattr(existing, "category", None) + if existing.name not in requested and category in PORT_ROLES: + raise RuntimeError( + f"[node:{node.uuid}] Port {existing.name} ({existing.uuid}) is " + f"not in the request but still has role {category!r}; delete " + "it, or include it in the ports list." + ) + + def find_or_create_netdev_node( *, client: IronicClient, @@ -438,12 +506,14 @@ def plan_netdev_port( port: NetdevPort, existing=None, ports_by_mac: dict | None = None, + manage_category: bool = True, ) -> dict | None: """Decide what a single port needs, without mutating anything. Returns a plan dict describing the action, or None if the port already matches the request. Raises on a MAC conflict that would imply an - automatic label move/rename. + automatic label move/rename. With ``manage_category`` False the port's + category is left as it is. Plan shapes: {"kind": "create", "port": NetdevPort} @@ -497,8 +567,11 @@ def plan_netdev_port( patch.append( {"op": "add", "path": "/physical_network", "value": physical_network} ) - if getattr(existing, "category", None) != "network": - patch.append({"op": "add", "path": "/category", "value": "network"}) + if manage_category and getattr(existing, "category", None) != port.category: + if port.category is None: + patch.append({"op": "remove", "path": "/category"}) + else: + patch.append({"op": "add", "path": "/category", "value": port.category}) # Converge extra.bios_name (the device's interface name) so re-runs update # it. Replace the whole extra object is unsafe (would drop other keys), so # patch the single nested key. @@ -574,7 +647,6 @@ def create_netdev_port( bios_name = port.bios_name or port.label port_data = { "address": port.mac, - "category": "network", "extra": {"bios_name": bios_name}, "local_link_connection": { "switch_id": switch_id, @@ -585,11 +657,13 @@ def create_netdev_port( "node_uuid": node.uuid, "physical_network": physical_network, } + if port.category is not None: + port_data["category"] = port.category logger.info( "[node:%s] Creating baremetal port name=%s mac=%s " "physical_network=%s switch_id=%s switch_info=%s port_id=%s " - "category=network", + "category=%s", node.uuid, port_name, port.mac, @@ -597,6 +671,7 @@ def create_netdev_port( switch_id, port.switch, port.interface, + port.category, ) created_port = client.create_port(port_data) logger.info(