Skip to content

Commit cb2996e

Browse files
committed
reject Palo Alto gateways that cannot be wired up front
1 parent 5876871 commit cb2996e

6 files changed

Lines changed: 202 additions & 0 deletions

File tree

‎python/neutron-understack/neutron_understack/ironic.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
from openstack import exceptions as sdk_exc
77
from openstack.baremetal.baremetal_service import BaremetalService
88
from openstack.baremetal.v1.node import Node as BaremetalNode
9+
from openstack.baremetal.v1.port import Port as BaremetalPort
910
from oslo_config import cfg
1011

1112
from neutron_understack import config
@@ -178,6 +179,11 @@ def node_vif_ids(self, node: str | BaremetalNode) -> list[str]:
178179
"""Return the Neutron port (VIF) ids currently attached to the node."""
179180
return self.irclient.list_node_vifs(node)
180181

182+
def node_ports(self, node: str | BaremetalNode) -> list[BaremetalPort]:
183+
"""Return the node's baremetal ports, including their network fields."""
184+
node_id = node.id if isinstance(node, BaremetalNode) else node
185+
return list(self.irclient.ports(details=True, node_id=node_id))
186+
181187
def adopt_node_for_router(
182188
self,
183189
node: str | BaremetalNode,

‎python/neutron-understack/neutron_understack/l3_router/palo_alto.py‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import json
22
import logging
3+
import time
34
import weakref
45

56
from neutron.objects import router as l3_obj
@@ -96,6 +97,14 @@ def __init__(self, l3_plugin):
9697
self._palo_alto_provider = f"{__name__}.{self.__class__.__name__}"
9798
self._interface_snapshots = weakref.WeakKeyDictionary()
9899
self._wiring = PaloAltoWiring(ironic=lambda: self._ironic)
100+
# Check the appliance can be wired on BEFORE_CREATE, before Neutron
101+
# creates the gateway port: rejecting there leaves nothing behind and
102+
# the request can be retried. BEFORE events always re-raise errors.
103+
registry.subscribe(
104+
self._validate_gateway_create,
105+
resources.ROUTER_GATEWAY,
106+
events.BEFORE_CREATE,
107+
)
99108
# Gateway attach must run on AFTER_CREATE (the gateway port does not
100109
# exist earlier) and must be cancellable so a wiring failure returns a
101110
# real API error instead of a swallowed 200. @registry.receives cannot
@@ -285,6 +294,24 @@ def _process_router_delete(self, resource, event, trigger, payload=None):
285294
router["id"],
286295
)
287296

297+
def _validate_gateway_create(self, resource, event, trigger, payload=None):
298+
"""ROUTER_GATEWAY / BEFORE_CREATE: check the gateway can be wired.
299+
300+
Read-only. Catches configuration problems (no adopted node, no NIC
301+
Ironic can bind) before Neutron creates anything for the gateway.
302+
"""
303+
router_id = payload.resource_id
304+
if self._palo_alto_router(payload.context, router_id) is None:
305+
return
306+
307+
started = time.monotonic()
308+
self._wiring.check_can_attach(router_id)
309+
LOG.debug(
310+
"Palo Alto router %s gateway pre-check passed in %.3fs",
311+
router_id,
312+
time.monotonic() - started,
313+
)
314+
288315
def _process_gateway_create(self, resource, event, trigger, payload=None):
289316
"""ROUTER_GATEWAY / AFTER_CREATE (cancellable): wire the gateway.
290317

‎python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,15 @@ class PaloAltoParentNotAnnotated(n_exc.BadRequest):
9494
)
9595

9696

97+
# BadRequest -> HTTP 400: no NIC on the node can carry the parent port's VIF.
98+
class PaloAltoNodeHasNoUsablePort(n_exc.BadRequest):
99+
message = (
100+
"Ironic node %(node_id)s for Palo Alto router %(router_id)s has no "
101+
"baremetal port with both physical_network and local_link_connection; "
102+
"check the node's port enrollment."
103+
)
104+
105+
97106
def _parent_port_name(router_id: str) -> str:
98107
"""Deterministic name for the router's anchor-network parent port."""
99108
return f"{ANCHOR_PARENT_PORT_NAME_PREFIX}-{router_id}"
@@ -148,6 +157,15 @@ def _device_id_cleared(port: dict) -> Generator[None, None, None]:
148157
)
149158

150159

160+
def _is_bindable_port(port) -> bool:
161+
"""Return True if Ironic can attach the parent VIF here and annotate it.
162+
163+
Ironic fills the parent port's binding profile from these two fields; a
164+
port without them leaves the parent unannotated and the wiring fails.
165+
"""
166+
return bool(port.physical_network) and bool(port.local_link_connection)
167+
168+
151169
def _missing_binding_fields(port: dict) -> list[str]:
152170
"""Return the binding fields Ironic should have set on the port but did not."""
153171
profile = port.get(portbindings.PROFILE) or {}
@@ -423,6 +441,19 @@ def _ensure_trunk(self, router_id: str, parent_port: dict) -> dict:
423441
return existing
424442
return self._create_trunk(router_id, parent_port)
425443

444+
def check_can_attach(self, router_id: str) -> None:
445+
"""Raise if the router's appliance cannot be wired; change nothing.
446+
447+
Meant to run before Neutron creates a gateway: rejecting the request
448+
then leaves nothing to clean up, and the same request can be retried
449+
once the node is fixed. Runtime failures are still possible later.
450+
"""
451+
node = self._ironic().node_by_instance_uuid(router_id)
452+
if node is None:
453+
raise PaloAltoNodeNotAdopted(router_id=router_id)
454+
if not any(_is_bindable_port(port) for port in self._ironic().node_ports(node)):
455+
raise PaloAltoNodeHasNoUsablePort(router_id=router_id, node_id=node.id)
456+
426457
def ensure_stack(self, router_id: str) -> AttachmentStack:
427458
"""Find or create the router's parent port, VIF attachment and trunk.
428459

‎python/neutron-understack/neutron_understack/tests/test_ironic.py‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,22 @@ def test_node_vif_ids(self, mocker):
245245
assert client.node_vif_ids(node) == ["p1", "p2"]
246246

247247

248+
class TestNodePorts:
249+
def test_lists_the_nodes_ports_with_details(self, mocker):
250+
client = _client(mocker)
251+
client.irclient.ports.return_value = iter(["port-a", "port-b"])
252+
253+
assert client.node_ports(_node()) == ["port-a", "port-b"]
254+
client.irclient.ports.assert_called_once_with(details=True, node_id="n1")
255+
256+
def test_accepts_a_node_id(self, mocker):
257+
client = _client(mocker)
258+
client.irclient.ports.return_value = iter([])
259+
260+
assert client.node_ports("n1") == []
261+
client.irclient.ports.assert_called_once_with(details=True, node_id="n1")
262+
263+
248264
class TestReleaseClearsOwnership:
249265
def test_active_node_is_undeployed_then_ownership_cleared(self, mocker):
250266
client = _client(mocker)

‎python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,71 @@ def test_gateway_wiring_error_reaches_the_caller(self, mocker):
534534
for exc in failure.value.inner_exceptions
535535
)
536536

537+
def _gateway_create_check(self):
538+
registry.publish(
539+
resources.ROUTER_GATEWAY,
540+
events.BEFORE_CREATE,
541+
self,
542+
payload=self._payload(metadata={"network_id": "ext-net"}),
543+
)
544+
545+
def test_gateway_check_rejects_before_neutron_creates_the_gateway(self, mocker):
546+
# BEFORE_CREATE runs before the gateway port exists, so rejecting here
547+
# leaves nothing to clean up and the same request can be retried.
548+
ironic = mocker.Mock()
549+
ironic.node_by_instance_uuid.return_value = None
550+
provider = _make_provider(
551+
mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic
552+
)
553+
provider.l3plugin.get_router.return_value = dict(self._ROUTER)
554+
555+
with pytest.raises(callback_exc.CallbackFailure) as failure:
556+
self._gateway_create_check()
557+
558+
[error] = failure.value.inner_exceptions
559+
assert isinstance(error, palo_alto_wiring.PaloAltoNodeNotAdopted)
560+
561+
def test_gateway_check_passes_without_creating_anything(self, mocker):
562+
ironic = mocker.Mock()
563+
ironic.node_by_instance_uuid.return_value = mocker.Mock(id="node-1")
564+
ironic.node_ports.return_value = [
565+
SimpleNamespace(
566+
physical_network="physnet-1",
567+
local_link_connection={"switch_id": "aa", "port_id": "Eth1/1"},
568+
)
569+
]
570+
core = mocker.Mock()
571+
trunk_plugin = mocker.Mock()
572+
mocker.patch(
573+
"neutron_understack.utils.fetch_trunk_plugin", return_value=trunk_plugin
574+
)
575+
provider = _make_provider(
576+
mocker,
577+
FakeFlavorPlugin(_palo_alto_driver()),
578+
ironic=ironic,
579+
core_plugin=core,
580+
)
581+
provider.l3plugin.get_router.return_value = dict(self._ROUTER)
582+
583+
self._gateway_create_check()
584+
585+
ironic.node_ports.assert_called_once()
586+
ironic.attach_vif_to_node.assert_not_called()
587+
core.create_network.assert_not_called()
588+
core.create_port.assert_not_called()
589+
trunk_plugin.create_trunk.assert_not_called()
590+
591+
def test_gateway_check_ignores_other_flavors(self, mocker):
592+
ironic = mocker.Mock()
593+
provider = _make_provider(
594+
mocker, FakeFlavorPlugin(_OTHER_DRIVER), ironic=ironic
595+
)
596+
provider.l3plugin.get_router.return_value = dict(self._ROUTER)
597+
598+
self._gateway_create_check()
599+
600+
ironic.node_by_instance_uuid.assert_not_called()
601+
537602
def test_gateway_teardown_runs_before_gateway_port_delete(self, mocker):
538603
# BEFORE_DELETE is the last point where the gateway port still exists.
539604
core = mocker.Mock()

‎python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,16 @@ def _snapshot(**changes):
6666
return palo_alto_wiring.AttachmentSnapshot(**{**fields, **changes})
6767

6868

69+
def _ironic_port(physical_network="physnet-1", local_link_connection=None):
70+
"""An Ironic baremetal port; bindable unless a field is overridden empty."""
71+
if local_link_connection is None:
72+
local_link_connection = {"switch_id": "aa", "port_id": "Eth1/1"}
73+
return SimpleNamespace(
74+
physical_network=physical_network,
75+
local_link_connection=local_link_connection,
76+
)
77+
78+
6979
@pytest.fixture
7080
def core_plugin(mocker):
7181
core = mocker.Mock()
@@ -131,6 +141,7 @@ def test_no_subport_vlan_available_is_conflict(self):
131141
[
132142
palo_alto_wiring.PaloAltoNodeNotAdopted,
133143
palo_alto_wiring.PaloAltoParentNotAnnotated,
144+
palo_alto_wiring.PaloAltoNodeHasNoUsablePort,
134145
],
135146
)
136147
def test_wiring_errors_are_bad_request(self, exc):
@@ -399,6 +410,52 @@ def test_reuses_existing_trunk(self, wiring, trunk_plugin):
399410
trunk_plugin.create_trunk.assert_not_called()
400411

401412

413+
class TestCheckCanAttach:
414+
"""The read-only check run before Neutron creates a gateway."""
415+
416+
def test_passes_when_node_has_a_bindable_port(
417+
self, wiring, ironic, node, core_plugin, trunk_plugin
418+
):
419+
ironic.node_ports.return_value = [
420+
_ironic_port(physical_network=None),
421+
_ironic_port(),
422+
]
423+
424+
wiring.check_can_attach("r1")
425+
426+
ironic.node_ports.assert_called_once_with(node)
427+
# Nothing is created or attached: the check must have no side effects.
428+
ironic.attach_vif_to_node.assert_not_called()
429+
core_plugin.create_network.assert_not_called()
430+
core_plugin.create_port.assert_not_called()
431+
trunk_plugin.create_trunk.assert_not_called()
432+
433+
def test_rejects_router_without_adopted_node(self, wiring, ironic):
434+
ironic.node_by_instance_uuid.return_value = None
435+
436+
with pytest.raises(palo_alto_wiring.PaloAltoNodeNotAdopted, match="router r1 "):
437+
wiring.check_can_attach("r1")
438+
ironic.node_ports.assert_not_called()
439+
440+
@pytest.mark.parametrize(
441+
"ports",
442+
[
443+
[],
444+
[_ironic_port(physical_network=None)],
445+
[_ironic_port(local_link_connection={})],
446+
],
447+
ids=["no-ports", "no-physical-network", "no-local-link-connection"],
448+
)
449+
def test_rejects_node_without_bindable_port(self, wiring, ironic, node, ports):
450+
ironic.node_ports.return_value = ports
451+
452+
with pytest.raises(
453+
palo_alto_wiring.PaloAltoNodeHasNoUsablePort,
454+
match="node node-1 for Palo Alto router r1",
455+
):
456+
wiring.check_can_attach("r1")
457+
458+
402459
class TestEnsureStack:
403460
def test_binds_parent_before_building_trunk(self, mocker, wiring):
404461
parent = {"id": "parent-1"}

0 commit comments

Comments
 (0)