diff --git a/components/images-openstack.yaml b/components/images-openstack.yaml index 04c023280..6aedfb51d 100644 --- a/components/images-openstack.yaml +++ b/components/images-openstack.yaml @@ -43,7 +43,7 @@ images: neutron_metadata: "ghcr.io/rackerlabs/understack/neutron:2026.1" neutron_ovn_metadata: "ghcr.io/rackerlabs/understack/neutron:2026.1" neutron_openvswitch_agent: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_server: "ghcr.io/rackerlabs/understack/neutron:2026.1" + neutron_server: "ghcr.io/rackerlabs/understack/neutron:pr-2456" neutron_rpc_server: "ghcr.io/rackerlabs/understack/neutron:2026.1" neutron_ovn_maintenance_worker: "ghcr.io/rackerlabs/understack/neutron:2026.1" neutron_ironic_agent: "ghcr.io/rackerlabs/understack/neutron:2026.1" diff --git a/python/neutron-understack/neutron_understack/config.py b/python/neutron-understack/neutron_understack/config.py index ea8f80cd9..851a71de6 100644 --- a/python/neutron-understack/neutron_understack/config.py +++ b/python/neutron-understack/neutron_understack/config.py @@ -25,11 +25,11 @@ ), cfg.ListOpt( "default_tenant_vlan_id_range", - default=[1, 3799], + default=[2, 3871], item_type=cfg.types.Integer(min=1, max=4094), help=( - "List of 2 comma separated integers, that represents a VLAN range, that" - "will be used for mapped VLANs on the switches." + "Two comma-separated integers defining the inclusive VLAN range used " + "for mapped VLANs on switches and accepted for tenant trunk subports." ), ), ] diff --git a/python/neutron-understack/neutron_understack/tests/conftest.py b/python/neutron-understack/neutron_understack/tests/conftest.py index a49db540b..00ebcad3b 100644 --- a/python/neutron-understack/neutron_understack/tests/conftest.py +++ b/python/neutron-understack/neutron_understack/tests/conftest.py @@ -279,13 +279,17 @@ def port_context(network_context, port_dict, port_binding, ml2_plugin) -> PortCo @pytest.fixture -def understack_driver(oslo_config) -> UnderstackDriver: +def understack_driver(mocker, oslo_config) -> UnderstackDriver: """The understack driver, which must never reach out to Undersync. No ``undersync`` attribute is injected on purpose, so a reintroduced ``self.undersync.sync(...)`` fails loudly instead of passing against a mock. + The trunk collaborator is mocked because these unit tests do not call the + driver's production ``initialize()`` hook. """ - return UnderstackDriver() + driver = UnderstackDriver() + driver.trunk_driver = mocker.create_autospec(UnderstackTrunkDriver, instance=True) + return driver @pytest.fixture diff --git a/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md b/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md index e1a6fc633..d636ede38 100644 --- a/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md +++ b/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md @@ -293,13 +293,39 @@ adds/removes must reconcile the parent's switch (VLAN group). parent binding there is no physnet to carry the subport on and nothing to reconcile -### TRUNK-SEGID-RANGE-01 — subport seg_id outside the allowed range is rejected +### TRUNK-PARENT-BIND-NATIVE-01 — binding rejects an existing native VLAN collision +- given: an unbound baremetal parent with a trunk whose existing subport tag + will equal the parent's native VLAN on its eventual physical network +- when: the parent is vif-attached and that native VLAN is selected +- then: the parent binding fails with no parent or subport binding levels and + no `undersync.sync`; the collision could not be checked when the trunk or + subport was created because the unbound parent had no physical network or + native VLAN yet + +### TRUNK-PARENT-BIND-NATIVE-02 — trunk and subports created before bind are configured at bind +- given: an unbound baremetal parent with a trunk and several subports added + before the parent has any host or physical network +- when: the parent is vif-attached +- then: the parent binds, each subport gets a level-0 binding row on its + network's dynamic VLAN segment for the parent's physnet, and + `undersync.sync` fires for that physnet; none of this existed before the + bind because the subport add had no physnet to allocate on + +### TRUNK-SEGID-NATIVE-01 — subport seg_id matching the native VLAN is rejected - given: a bound baremetal parent with a trunk -- when: a subport is added with a segmentation_id outside `[1, 3799]` +- when: a subport is added with a segmentation_id equal to the VLAN segment of + the parent's network on the parent's physical network - then: `SubportSegmentationIDError` (raised in the SUBPORTS PRECOMMIT_CREATE callback, surfaced as `CallbackFailure`), with no binding level or dynamic segment allocated +### TRUNK-SEGID-RANGE-01 — subport seg_id outside the tenant range is rejected +- given: a bound baremetal parent with a trunk +- when: a subport is added with a VLAN segmentation_id outside the configured + `default_tenant_vlan_id_range` +- then: the request is rejected before a binding level or dynamic segment is + allocated + ### TRUNK-ORDER-01 — subport removal deallocates before it notifies Undersync - given: a bound baremetal parent with a trunk and an attached subport - when: the subport is removed diff --git a/python/neutron-understack/neutron_understack/tests/scenarios/test_trunk_operations.py b/python/neutron-understack/neutron_understack/tests/scenarios/test_trunk_operations.py index 0de428899..c97bd4e84 100644 --- a/python/neutron-understack/neutron_understack/tests/scenarios/test_trunk_operations.py +++ b/python/neutron-understack/neutron_understack/tests/scenarios/test_trunk_operations.py @@ -12,6 +12,7 @@ from neutron.plugins.ml2 import db as ml2_db from neutron.services.trunk import exceptions as trunk_exc from neutron_lib import constants as p_const +from neutron_lib.api.definitions import portbindings from neutron_lib.callbacks import exceptions as cb_exc from neutron_understack.tests.scenarios.base import DEFAULT_PHYSNET @@ -32,6 +33,33 @@ def _plain_port(self, net_id): assert res.status_int == 201, res.body return self.deserialize(self.fmt, res)["port"]["id"] + def _unbound_baremetal_port(self, net_id): + res = self._create_port( + self.fmt, + net_id, + arg_list=(portbindings.VNIC_TYPE,), + is_admin=True, + **{portbindings.VNIC_TYPE: portbindings.VNIC_BAREMETAL}, + ) + assert res.status_int == 201, res.body + return self.deserialize(self.fmt, res)["port"] + + def _vif_attach(self, port_id, physnet=DEFAULT_PHYSNET, host="host-a"): + data = { + "port": { + portbindings.HOST_ID: host, + portbindings.PROFILE: self.baremetal_binding_profile(physnet=physnet), + } + } + req = self.new_update_request("ports", data, port_id, as_service=True) + with mock.patch( + "neutron_understack.utils.fetch_network_node_trunk_id", + return_value=_FAKE_NN_TRUNK, + ): + res = req.get_response(self.api) + assert res.status_int == 200, res.body + return self.deserialize(self.fmt, res)["port"] + def _make_trunk(self, parent_id): trunk = self.trunk_plugin.create_trunk( self.context, @@ -230,6 +258,77 @@ def test_subport_add_unbound_parent_no_sync(self): ) self.undersync_mock.sync.assert_not_called() + # The understack driver raises SubportSegmentationIDError in bind_port, but + # ML2 has no way to abort a bind: it logs the failure and tries the next + # mechanism driver, which binds the port. This asserts the desired behavior; + # strict=True flips an unexpected pass into a failure, prompting removal of + # the xfail once the upstream Neutron fix is backported. + @pytest.mark.xfail( + strict=True, + reason=( + "native VLAN collision on parent bind is detected but ML2 cannot " + "abort a bind; it falls through to the next mechanism driver and the " + "port binds (vif_type=other). Needs the upstream Neutron fix backported" + ), + ) + @pytest.mark.scenario("TRUNK-PARENT-BIND-NATIVE-01") + def test_unbound_trunk_rejects_native_vlan_collision_on_parent_bind(self): + """Binding must revalidate subports once the native VLAN is known.""" + parent_net = self._make_network(self.fmt, "parent-net", True)["network"]["id"] + parent = self._unbound_baremetal_port(parent_net) + subport_net = self._make_network(self.fmt, "subport-net", True)["network"]["id"] + subport_id = self._plain_port(subport_net) + trunk_id = self._make_trunk(parent["id"]) + + # This is valid while the parent is unbound because its native VLAN is + # not known yet. Pin the segment that vif-attach will select so the + # eventual collision is deterministic. + self._add_subport(trunk_id, subport_id, seg_id=SUBPORT_VLAN) + native_segment = { + segments_db.NETWORK_TYPE: p_const.TYPE_VLAN, + segments_db.PHYSICAL_NETWORK: DEFAULT_PHYSNET, + segments_db.SEGMENTATION_ID: SUBPORT_VLAN, + } + segments_db.add_network_segment( + self.context, parent_net, native_segment, is_dynamic=True + ) + + self.undersync_mock.reset_mock() + updated = self._vif_attach(parent["id"]) + + assert updated[portbindings.VIF_TYPE] == portbindings.VIF_TYPE_BINDING_FAILED + assert not ml2_db.get_binding_level_objs(self.context, parent["id"], "host-a") + assert not ml2_db.get_binding_level_objs(self.context, subport_id, "host-a") + self.undersync_mock.sync.assert_not_called() + + @pytest.mark.scenario("TRUNK-PARENT-BIND-NATIVE-02") + def test_unbound_trunk_with_subports_configures_on_parent_bind(self): + """Everything created before the parent binds is configured at bind.""" + parent_net = self._make_network(self.fmt, "parent-net", True)["network"]["id"] + parent = self._unbound_baremetal_port(parent_net) + sub_net_a = self._make_network(self.fmt, "sub-a", True)["network"]["id"] + sub_net_b = self._make_network(self.fmt, "sub-b", True)["network"]["id"] + subport_a = self._plain_port(sub_net_a) + subport_b = self._plain_port(sub_net_b) + trunk_id = self._make_trunk(parent["id"]) + self._add_subport(trunk_id, subport_a, seg_id=1500) + self._add_subport(trunk_id, subport_b, seg_id=1600) + for net_id in (sub_net_a, sub_net_b): + assert ( + segments_db.get_dynamic_segment( + self.context, net_id, physical_network=DEFAULT_PHYSNET + ) + is None + ) + + self.undersync_mock.reset_mock() + updated = self._vif_attach(parent["id"]) + + assert updated[portbindings.VIF_TYPE] == portbindings.VIF_TYPE_OTHER + self._assert_subport_bound(trunk_id, subport_a, "host-a", seg_id=1500) + self._assert_subport_bound(trunk_id, subport_b, "host-a", seg_id=1600) + self.undersync_mock.sync.assert_any_call(DEFAULT_PHYSNET) + @pytest.mark.scenario("TRUNK-MULTI-01") def test_multiple_subports_add_syncs(self): parent_net = self._make_network(self.fmt, "parent-net", True)["network"]["id"] @@ -274,20 +373,46 @@ def test_multiple_subports_add_syncs(self): assert segment_a != segment_b self.undersync_mock.sync.assert_any_call(DEFAULT_PHYSNET) - @pytest.mark.scenario("TRUNK-SEGID-RANGE-01") - def test_subport_segid_out_of_range_rejected(self): + @pytest.mark.scenario("TRUNK-SEGID-NATIVE-01") + def test_subport_segid_matching_native_vlan_rejected(self): parent_net = self._make_network(self.fmt, "parent-net", True)["network"]["id"] parent_id = self._bind_baremetal_port(parent_net, DEFAULT_PHYSNET, "host-a") subport_net = self._make_network(self.fmt, "subport-net", True)["network"]["id"] subport_id = self._plain_port(subport_net) trunk_id = self._make_trunk(parent_id) + native_segment = segments_db.get_dynamic_segment( + self.context, parent_net, physical_network=DEFAULT_PHYSNET + ) - # 4000 is a valid VLAN but outside the default tenant range [1, 3799]. - # subports_added raises SubportSegmentationIDError, which the SUBPORTS - # PRECOMMIT_CREATE callback machinery re-raises as CallbackFailure. + # subports_added raises SubportSegmentationIDError when the requested + # tag is the native VLAN, and the callback machinery wraps it. + with pytest.raises(cb_exc.CallbackFailure) as exc_info: + self._add_subport( + trunk_id, + subport_id, + seg_id=native_segment[segments_db.SEGMENTATION_ID], + ) + assert "used as both" in str(exc_info.value) + assert not ml2_db.get_binding_level_objs(self.context, subport_id, "host-a") + assert ( + segments_db.get_dynamic_segment( + self.context, subport_net, physical_network=DEFAULT_PHYSNET + ) + is None + ) + + @pytest.mark.scenario("TRUNK-SEGID-RANGE-01") + def test_subport_segid_outside_configured_range_is_rejected(self): + parent_net = self._make_network(self.fmt, "parent-net", True)["network"]["id"] + parent_id = self._bind_baremetal_port(parent_net, DEFAULT_PHYSNET, "host-a") + subport_net = self._make_network(self.fmt, "subport-net", True)["network"]["id"] + subport_id = self._plain_port(subport_net) + trunk_id = self._make_trunk(parent_id) with pytest.raises(cb_exc.CallbackFailure) as exc_info: self._add_subport(trunk_id, subport_id, seg_id=4000) - assert "Segmentation ID" in str(exc_info.value) + assert "outside the configured tenant trunk VLAN range 2-3871" in str( + exc_info.value + ) assert not ml2_db.get_binding_level_objs(self.context, subport_id, "host-a") assert ( segments_db.get_dynamic_segment( diff --git a/python/neutron-understack/neutron_understack/tests/test_trunk.py b/python/neutron-understack/neutron_understack/tests/test_trunk.py index d7555e7a3..0f7839123 100644 --- a/python/neutron-understack/neutron_understack/tests/test_trunk.py +++ b/python/neutron-understack/neutron_understack/tests/test_trunk.py @@ -4,6 +4,7 @@ from neutron_understack import utils from neutron_understack.trunk import SubportSegmentationIDError +from neutron_understack.trunk import SubportSegmentationIDRangeError class TestSubportsAdded: @@ -55,9 +56,9 @@ def test_when_subports_are_not_present( @pytest.mark.usefixtures("_utils_fetch_subport_network_id_patch") class Test_HandleTenantVlanIDAndSwitchportConfig: def test_that_check_subports_segmentation_id_is_called( - self, mocker, understack_trunk_driver, trunk, subport, network_id, vlan_num + self, mocker, understack_trunk_driver, trunk, subport ): - mocker.patch("neutron_understack.utils.fetch_port_object") + parent_port = mocker.patch("neutron_understack.utils.fetch_port_object") mocker.patch( "neutron_understack.utils.parent_port_is_bound", return_value=False ) @@ -68,7 +69,9 @@ def test_that_check_subports_segmentation_id_is_called( [subport], trunk ) - subport_seg_id_check.assert_called_once() + subport_seg_id_check.assert_called_once_with( + [subport], trunk.id, parent_port.return_value + ) def test_when_parent_port_is_bound( self, @@ -280,8 +283,23 @@ def test_that_add_subports_networks_is_called( mocker.patch.object( understack_trunk_driver, "_add_subports_networks_to_parent_port_switchport" ) - understack_trunk_driver.configure_trunk({}, port_id) + check_native_vlan = mocker.patch.object( + understack_trunk_driver, "check_trunk_native_vlan" + ) + trunk_details = {"trunk_id": "trunk-a", "sub_ports": []} + + understack_trunk_driver.configure_trunk( + trunk_details=trunk_details, + port_id=port_id, + physical_network="physnet-a", + native_vlan_id=1800, + ) + check_native_vlan.assert_called_once_with( + trunk_details=trunk_details, + physical_network="physnet-a", + native_vlan_id=1800, + ) understack_trunk_driver._add_subports_networks_to_parent_port_switchport.assert_called_once_with( parent_port=port_object, subports=[], @@ -310,71 +328,181 @@ def test_without_subports(self, mocker, understack_trunk_driver): deallocate.assert_called_once_with([], "host-a") +@pytest.mark.usefixtures("oslo_config") class TestCheckSubportsSegmentationId: def test_when_trunk_id_is_network_node_trunk_id( self, mocker, understack_trunk_driver, trunk_id, + subport, + port_object, ): - # Mock fetch_network_node_trunk_id to return the trunk_id mocker.patch( "neutron_understack.utils.fetch_network_node_trunk_id", return_value=str(trunk_id), ) - # Mock to ensure the function returns early and doesn't call this - allowed_ranges_mock = mocker.patch( - "neutron_understack.utils.allowed_tenant_vlan_id_ranges" + find_segment = mocker.patch( + "neutron_understack.utils.network_segment_by_physnet" ) + subport.segmentation_id = 4000 + result = understack_trunk_driver._check_subports_segmentation_id( - [], str(trunk_id) + [subport], str(trunk_id), port_object ) - # Should not call allowed_tenant_vlan_id_ranges because it returns early - allowed_ranges_mock.assert_not_called() + + find_segment.assert_not_called() assert result is None - def test_when_segmentation_id_is_in_allowed_range( + def test_when_segmentation_id_differs_from_native_vlan( self, mocker, understack_trunk_driver, trunk_id, subport, + port_object, + network_id, + vlan_network_segment, ): - # Mock fetch_network_node_trunk_id to return a different trunk ID mocker.patch( "neutron_understack.utils.fetch_network_node_trunk_id", return_value="different-trunk-id", ) - allowed_ranges = mocker.patch( - "neutron_understack.utils.allowed_tenant_vlan_id_ranges", - return_value=[(1, 1500)], + find_segment = mocker.patch( + "neutron_understack.utils.network_segment_by_physnet", + return_value=vlan_network_segment, ) - subport.segmentation_id = 500 + port_object.network_id = str(network_id) + subport.segmentation_id = 2000 + result = understack_trunk_driver._check_subports_segmentation_id( - [subport], trunk_id + [subport], trunk_id, port_object + ) + + find_segment.assert_called_once_with( + network_id=str(network_id), physnet="physnet" ) - allowed_ranges.assert_called_once() assert result is None - def test_when_segmentation_id_is_not_in_allowed_range( + @pytest.mark.parametrize("segmentation_id", [2, 3871]) + def test_when_segmentation_id_is_on_configured_range_boundary( self, mocker, understack_trunk_driver, trunk_id, subport, + port_object, + segmentation_id, ): - # Mock fetch_network_node_trunk_id to return a different trunk ID mocker.patch( "neutron_understack.utils.fetch_network_node_trunk_id", return_value="different-trunk-id", ) + port_object.bindings[0].vif_type = portbindings.VIF_TYPE_UNBOUND + subport.segmentation_id = segmentation_id + + result = understack_trunk_driver._check_subports_segmentation_id( + [subport], trunk_id, port_object + ) + + assert result is None + + @pytest.mark.parametrize("segmentation_id", [1, 3872, 4094]) + def test_when_segmentation_id_is_outside_configured_range( + self, + mocker, + understack_trunk_driver, + trunk_id, + subport, + port_object, + segmentation_id, + ): mocker.patch( - "neutron_understack.utils.allowed_tenant_vlan_id_ranges", - return_value=[(1, 1500)], + "neutron_understack.utils.fetch_network_node_trunk_id", + return_value="different-trunk-id", ) - subport.segmentation_id = 1600 - with pytest.raises(SubportSegmentationIDError): - understack_trunk_driver._check_subports_segmentation_id([subport], trunk_id) + find_segment = mocker.patch( + "neutron_understack.utils.network_segment_by_physnet" + ) + subport.segmentation_id = segmentation_id + + with pytest.raises( + SubportSegmentationIDRangeError, match="outside the configured" + ): + understack_trunk_driver._check_subports_segmentation_id( + [subport], trunk_id, port_object + ) + + find_segment.assert_not_called() + + def test_uses_configured_tenant_vlan_range( + self, + mocker, + oslo_config, + understack_trunk_driver, + trunk_id, + subport, + port_object, + ): + oslo_config.config( + default_tenant_vlan_id_range=[100, 200], group="ml2_understack" + ) + mocker.patch( + "neutron_understack.utils.fetch_network_node_trunk_id", + return_value="different-trunk-id", + ) + subport.segmentation_id = 99 + + with pytest.raises(SubportSegmentationIDRangeError, match="100-200"): + understack_trunk_driver._check_subports_segmentation_id( + [subport], trunk_id, port_object + ) + + def test_when_segmentation_id_matches_native_vlan( + self, + mocker, + understack_trunk_driver, + trunk_id, + subport, + port_object, + network_id, + vlan_network_segment, + ): + mocker.patch( + "neutron_understack.utils.fetch_network_node_trunk_id", + return_value="different-trunk-id", + ) + mocker.patch( + "neutron_understack.utils.network_segment_by_physnet", + return_value=vlan_network_segment, + ) + port_object.network_id = str(network_id) + subport.segmentation_id = vlan_network_segment.segmentation_id + + with pytest.raises(SubportSegmentationIDError, match="used as both"): + understack_trunk_driver._check_subports_segmentation_id( + [subport], trunk_id, port_object + ) + + def test_when_parent_port_is_unbound( + self, mocker, understack_trunk_driver, trunk_id, subport, port_object + ): + mocker.patch( + "neutron_understack.utils.fetch_network_node_trunk_id", + return_value="different-trunk-id", + ) + port_object.bindings[0].vif_type = portbindings.VIF_TYPE_UNBOUND + subport.segmentation_id = 2000 + find_segment = mocker.patch( + "neutron_understack.utils.network_segment_by_physnet" + ) + + result = understack_trunk_driver._check_subports_segmentation_id( + [subport], trunk_id, port_object + ) + + find_segment.assert_not_called() + assert result is None @pytest.mark.parametrize("binding_profile", [{"physical_network": None}], indirect=True) diff --git a/python/neutron-understack/neutron_understack/tests/test_understack_mech.py b/python/neutron-understack/neutron_understack/tests/test_understack_mech.py index 5eedca7f0..7d06a5518 100644 --- a/python/neutron-understack/neutron_understack/tests/test_understack_mech.py +++ b/python/neutron-understack/neutron_understack/tests/test_understack_mech.py @@ -12,6 +12,8 @@ from neutron_understack import trunk from neutron_understack import understack_mech +MECH_UTILS = "neutron_understack.understack_mech.utils" + def _port_update_context(original, current): return SimpleNamespace( @@ -171,9 +173,6 @@ def test_skips_non_baremetal_port(self, mocker, understack_driver, port_context) cleanup.assert_not_called() -MECH_UTILS = "neutron_understack.understack_mech.utils" - - class TestDeletePortPostCommit: def test_skips_non_baremetal_port(self, mocker, understack_driver, port_context): port_context.current[portbindings.VNIC_TYPE] = portbindings.VNIC_NORMAL @@ -338,6 +337,125 @@ def test_with_no_trunk( next_segments_to_bind=[vlan_network_segment], ) + def test_releases_allocated_segment_when_trunk_check_fails( + self, + mocker, + port_context, + understack_driver, + understack_trunk_driver, + vlan_network_segment, + ): + """A native VLAN that collides with a subport must not leak its segment.""" + port_context.current["trunk_details"] = {"trunk_id": "trunk-a"} + mocker.patch.object( + port_context, "allocate_dynamic_segment", return_value=vlan_network_segment + ) + mocker.patch(f"{MECH_UTILS}.ports_bound_to_segment", return_value=[]) + understack_driver.trunk_driver = understack_trunk_driver + mocker.patch.object( + understack_driver.trunk_driver, + "configure_trunk", + side_effect=trunk.SubportSegmentationIDError( + seg_id=vlan_network_segment[api.SEGMENTATION_ID], + subport_id="subport-a", + physical_network=vlan_network_segment[api.PHYSICAL_NETWORK], + ), + ) + release = mocker.patch(f"{MECH_UTILS}.release_dynamic_segment") + port_context._prepare_to_bind(port_context.network.network_segments) + + with pytest.raises(trunk.SubportSegmentationIDError): + understack_driver._bind_port_segment( + port_context, + next( + s + for s in port_context.network.network_segments + if s[api.NETWORK_TYPE] == "vxlan" + ), + ) + + release.assert_called_once_with(vlan_network_segment[api.ID]) + + def test_keeps_allocated_segment_when_other_ports_are_bound( + self, + mocker, + port_context, + understack_driver, + understack_trunk_driver, + vlan_network_segment, + ): + """allocate_dynamic_segment can return a segment another port is using.""" + port_context.current["trunk_details"] = {"trunk_id": "trunk-a"} + mocker.patch.object( + port_context, "allocate_dynamic_segment", return_value=vlan_network_segment + ) + mocker.patch( + f"{MECH_UTILS}.ports_bound_to_segment", return_value=["other-port"] + ) + understack_driver.trunk_driver = understack_trunk_driver + mocker.patch.object( + understack_driver.trunk_driver, + "configure_trunk", + side_effect=trunk.SubportSegmentationIDError( + seg_id=vlan_network_segment[api.SEGMENTATION_ID], + subport_id="subport-a", + physical_network=vlan_network_segment[api.PHYSICAL_NETWORK], + ), + ) + release = mocker.patch(f"{MECH_UTILS}.release_dynamic_segment") + port_context._prepare_to_bind(port_context.network.network_segments) + + with pytest.raises(trunk.SubportSegmentationIDError): + understack_driver._bind_port_segment( + port_context, + next( + s + for s in port_context.network.network_segments + if s[api.NETWORK_TYPE] == "vxlan" + ), + ) + + release.assert_not_called() + + def test_keeps_reused_segment_when_trunk_check_fails( + self, + mocker, + port_context, + understack_driver, + understack_trunk_driver, + vlan_network_segment, + ): + """A segment that already existed is not ours to release.""" + port_context.current["trunk_details"] = {"trunk_id": "trunk-a"} + mocker.patch( + f"{MECH_UTILS}.vlan_segment_for_physnet", + return_value=vlan_network_segment, + ) + understack_driver.trunk_driver = understack_trunk_driver + mocker.patch.object( + understack_driver.trunk_driver, + "configure_trunk", + side_effect=trunk.SubportSegmentationIDError( + seg_id=vlan_network_segment[api.SEGMENTATION_ID], + subport_id="subport-a", + physical_network=vlan_network_segment[api.PHYSICAL_NETWORK], + ), + ) + release = mocker.patch(f"{MECH_UTILS}.release_dynamic_segment") + port_context._prepare_to_bind(port_context.network.network_segments) + + with pytest.raises(trunk.SubportSegmentationIDError): + understack_driver._bind_port_segment( + port_context, + next( + s + for s in port_context.network.network_segments + if s[api.NETWORK_TYPE] == "vxlan" + ), + ) + + release.assert_not_called() + def test_refuses_unsupported_vnic_type( self, mocker, port_context, understack_driver ): @@ -381,11 +499,39 @@ def test_with_trunk_details( port_context._prepare_to_bind(port_context.network.network_segments) understack_driver.trunk_driver = understack_trunk_driver - mocker.patch.object(understack_driver.trunk_driver, "configure_trunk") + configure_trunk = mocker.patch.object( + understack_driver.trunk_driver, "configure_trunk" + ) understack_driver.bind_port(port_context) - understack_driver.trunk_driver.configure_trunk.assert_called_once() + configure_trunk.assert_called_once_with( + trunk_details=port_context.current["trunk_details"], + port_id=port_context.current["id"], + physical_network="physnet", + native_vlan_id=666, + ) port_context.continue_binding.assert_called_once() + @pytest.mark.parametrize("port_dict", [{"trunk": True}], indirect=True) + def test_rejects_new_native_vlan_used_by_existing_subport_before_binding( + self, mocker, understack_driver, port_context, understack_trunk_driver + ): + port_context.current["trunk_details"]["sub_ports"][0]["segmentation_id"] = 666 + mocker.patch.object(port_context, "continue_binding") + port_context._prepare_to_bind(port_context.network.network_segments) + mocker.patch( + "neutron_understack.utils.fetch_network_node_trunk_id", + return_value="network-node-trunk", + ) + mocker.patch(f"{MECH_UTILS}.ports_bound_to_segment", return_value=[]) + release = mocker.patch(f"{MECH_UTILS}.release_dynamic_segment") + understack_driver.trunk_driver = understack_trunk_driver + + with pytest.raises(trunk.SubportSegmentationIDError, match="used as both"): + understack_driver.bind_port(port_context) + + port_context.continue_binding.assert_not_called() + release.assert_called_once() + class TestCreateNetworkPostCommit: @pytest.mark.usefixtures("_ml2_understack_conf") diff --git a/python/neutron-understack/neutron_understack/trunk.py b/python/neutron-understack/neutron_understack/trunk.py index 6b7e7fd40..5f8fbe82d 100644 --- a/python/neutron-understack/neutron_understack/trunk.py +++ b/python/neutron-understack/neutron_understack/trunk.py @@ -23,9 +23,16 @@ class SubportSegmentationIDError(exc.NeutronException): message = ( - "Segmentation ID: %(seg_id)s cannot be set to the Subport: " - "%(subport_id)s as it falls outside of allowed ranges: " - "%(network_segment_ranges)s. Please use different Segmentation ID." + "VLAN %(seg_id)s on physical network %(physical_network)s cannot be " + "used as both the native VLAN for trunk %(trunk_id)s and the " + "segmentation ID for subport %(subport_id)s." + ) + + +class SubportSegmentationIDRangeError(exc.NeutronException): + message = ( + "VLAN %(seg_id)s for subport %(subport_id)s is outside the configured " + "tenant trunk VLAN range %(minimum)s-%(maximum)s." ) @@ -87,8 +94,8 @@ def register(self, resource, event, trigger, payload=None): def _handle_tenant_vlan_id_and_switchport_config( self, subports: list[SubPort], trunk: Trunk ) -> None: - self._check_subports_segmentation_id(subports, trunk.id) parent_port_obj = utils.fetch_port_object(trunk.port_id) + self._check_subports_segmentation_id(subports, trunk.id, parent_port_obj) if utils.parent_port_is_bound(parent_port_obj): self._add_subports_networks_to_parent_port_switchport( @@ -96,37 +103,107 @@ def _handle_tenant_vlan_id_and_switchport_config( ) def _check_subports_segmentation_id( - self, subports: list[SubPort], trunk_id: str + self, subports: list[SubPort], trunk_id: str, parent_port: Port ) -> None: - """Checks if a subport's segmentation_id is within the allowed range. + """Validate tenant tags and reject a parent native VLAN collision. A switchport cannot have a mapped VLAN ID equal to the native VLAN ID. - Since the user specifies the VLAN ID (segmentation_id) when adding a - subport, an error is raised if it falls within any VLAN network segment - range, as these ranges are used to allocate VLAN tags for all VLAN - segments, including native VLANs. - - The only case where this check is not required is for a network node - trunk, since its subport segmentation_ids are the same as the network - segment VLAN tags allocated to the subports. Therefore, there is no - possibility of conflict with the native VLAN. + Resolve the native VLAN from the parent port's network and physical + network so that multi-segment networks are checked against the segment + used on this particular switch. + + The network-node trunk is exempt because its segmentation IDs are + internally allocated fabric VLANs, not tenant-selected mapped VLANs. + They are the same tags as the subports' network segments, so they also + cannot conflict with the parent through VLAN mapping. """ if trunk_id == utils.fetch_network_node_trunk_id(): return - ns_ranges = utils.allowed_tenant_vlan_id_ranges() + minimum, maximum = cfg.CONF.ml2_understack.default_tenant_vlan_id_range for subport in subports: - seg_id = subport.segmentation_id - if not utils.segmentation_id_in_ranges(seg_id, ns_ranges): + seg_id = int(subport["segmentation_id"]) + if not minimum <= seg_id <= maximum: + raise SubportSegmentationIDRangeError( + seg_id=seg_id, + subport_id=subport["port_id"], + minimum=minimum, + maximum=maximum, + ) + + if not utils.parent_port_is_bound(parent_port): + return + + physical_network = parent_port.bindings[0].profile.get("physical_network") + if not physical_network: + # The create path reports the more specific missing-physnet error + # when it attempts to configure the parent switchport. + return + + native_segment = utils.network_segment_by_physnet( + network_id=parent_port.network_id, + physnet=physical_network, + ) + if native_segment is None: + return + + self._check_native_vlan_conflict( + subports=subports, + trunk_id=trunk_id, + physical_network=physical_network, + native_vlan_id=native_segment.segmentation_id, + ) + + def _check_native_vlan_conflict( + self, + subports: list[SubPort] | list[dict], + trunk_id: str, + physical_network: str, + native_vlan_id: int, + ) -> None: + """Enforce the trunk's native/tagged VLAN uniqueness invariant.""" + native_vlan_id = int(native_vlan_id) + for subport in subports: + seg_id = int(subport["segmentation_id"]) + if seg_id == native_vlan_id: raise SubportSegmentationIDError( seg_id=seg_id, - subport_id=subport.port_id, - network_segment_ranges=utils.printable_ranges(ns_ranges), + subport_id=subport["port_id"], + trunk_id=trunk_id, + physical_network=physical_network, ) - def configure_trunk(self, trunk_details: dict, port_id: str) -> None: - parent_port_obj = utils.fetch_port_object(port_id) + def check_trunk_native_vlan( + self, + trunk_details: dict, + physical_network: str, + native_vlan_id: int, + ) -> None: + """Check a candidate parent native VLAN against existing subports.""" + if trunk_details["trunk_id"] == utils.fetch_network_node_trunk_id(): + return + + self._check_native_vlan_conflict( + subports=trunk_details.get("sub_ports", []), + trunk_id=trunk_details["trunk_id"], + physical_network=physical_network, + native_vlan_id=native_vlan_id, + ) + + def configure_trunk( + self, + trunk_details: dict, + port_id: str, + physical_network: str, + native_vlan_id: int, + ) -> None: + self.check_trunk_native_vlan( + trunk_details=trunk_details, + physical_network=physical_network, + native_vlan_id=native_vlan_id, + ) subports = trunk_details.get("sub_ports", []) + parent_port_obj = utils.fetch_port_object(port_id) self._add_subports_networks_to_parent_port_switchport( parent_port=parent_port_obj, subports=subports ) diff --git a/python/neutron-understack/neutron_understack/understack_mech.py b/python/neutron-understack/neutron_understack/understack_mech.py index 7f0c59194..49d77dd5f 100644 --- a/python/neutron-understack/neutron_understack/understack_mech.py +++ b/python/neutron-understack/neutron_understack/understack_mech.py @@ -363,7 +363,9 @@ def _bind_port_segment(self, context: PortContext, segment): physnet, ) + freshly_allocated_segment = None current_vlan_segment = utils.vlan_segment_for_physnet(context, physnet) + if current_vlan_segment: LOG.info( "vlan segment: %(segment)s already preset for physnet: %(physnet)s", @@ -371,19 +373,33 @@ def _bind_port_segment(self, context: PortContext, segment): ) dynamic_segment = current_vlan_segment else: - dynamic_segment = context.allocate_dynamic_segment( + freshly_allocated_segment = context.allocate_dynamic_segment( segment={ "network_type": p_const.TYPE_VLAN, "physical_network": physnet, }, ) + dynamic_segment = freshly_allocated_segment LOG.debug("bind_port_segment: Native VLAN segment %s", dynamic_segment) trunk_details = context.current.get("trunk_details") or {} port_id = context.current["id"] - if trunk_details: - self.trunk_driver.configure_trunk(trunk_details, port_id) + try: + if trunk_details: + self.trunk_driver.configure_trunk( + trunk_details=trunk_details, + port_id=port_id, + physical_network=physnet, + native_vlan_id=dynamic_segment[api.SEGMENTATION_ID], + ) + except Exception: + # Don't leak the dynamic segment we just allocated + if freshly_allocated_segment and not utils.ports_bound_to_segment( + freshly_allocated_segment[api.ID] + ): + utils.release_dynamic_segment(freshly_allocated_segment[api.ID]) + raise LOG.debug("continue_binding for segment: %s", segment) context.continue_binding(