From 8dcfc4cb4ca5be1d1b088e80f3a0f3bf01fc277a Mon Sep 17 00:00:00 2001 From: Steve Keay Date: Tue, 6 Oct 2026 14:10:20 +0100 Subject: [PATCH 1/7] Relax dot1q VLAN ID tag restrictions on trunk ports The check was added to prevent the user from claiming a segmentation ID that happens to be the same as the parent port of this vlan. We don't want to allow that situation because the switch (stupidly) rejects that configuration. However, the way the check was implemented was too restrictive - it banned a range of thousands of VLAN IDs. It did successfully prevent all theoretical clashes present and future, but it also prevented legitimate configurations and caused an upset customer. We update this check so that it now compares the VLAN ID of the actual native VLAN on this trunk interface, and only raises an error if the user is configuring a number that would currently clash. The theoretical valid range is 1-4094 according to 802.1Q. However we would avoid VLAN 1, and we must avoid our nxos "reserved" vlan range 3872-3999. On Nexus 9000 NX-OS, Cisco treats VLANs 4048-4093 as their own. These might work, but even if they work today, they might randomly stop working at some future point. So this puts the realistic useable VLAN range at 2-3871 inclusive. --- .../tests/scenarios/SCENARIOS.md | 11 ++- .../tests/scenarios/test_trunk_operations.py | 37 ++++++-- .../neutron_understack/tests/test_trunk.py | 84 +++++++++++++------ .../neutron_understack/trunk.py | 39 ++++++--- 4 files changed, 125 insertions(+), 46 deletions(-) diff --git a/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md b/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md index e1a6fc633..abb4573cb 100644 --- a/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md +++ b/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md @@ -293,13 +293,20 @@ 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-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-NONNATIVE-01 — any non-native subport seg_id is allowed +- given: a bound baremetal parent with a trunk +- when: a subport is added with a valid VLAN segmentation_id outside the old + broad allowed range but different from the parent's native VLAN +- then: the subport is added and gets its dynamic segment normally + ### 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..ac673d319 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 @@ -274,20 +274,26 @@ 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=4000) - assert "Segmentation ID" in str(exc_info.value) + self._add_subport( + trunk_id, + subport_id, + seg_id=native_segment[segments_db.SEGMENTATION_ID], + ) + assert "matches the native VLAN" 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( @@ -295,3 +301,20 @@ def test_subport_segid_out_of_range_rejected(self): ) is None ) + + @pytest.mark.scenario("TRUNK-SEGID-NONNATIVE-01") + def test_subport_segid_outside_old_range_is_allowed(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 + ) + native_vlan = native_segment[segments_db.SEGMENTATION_ID] + subport_vlan = 4000 if native_vlan != 4000 else 4001 + + self._add_subport(trunk_id, subport_id, seg_id=subport_vlan) + + self._assert_subport_bound(trunk_id, subport_id, "host-a", seg_id=subport_vlan) diff --git a/python/neutron-understack/neutron_understack/tests/test_trunk.py b/python/neutron-understack/neutron_understack/tests/test_trunk.py index d7555e7a3..02fd149f3 100644 --- a/python/neutron-understack/neutron_understack/tests/test_trunk.py +++ b/python/neutron-understack/neutron_understack/tests/test_trunk.py @@ -55,9 +55,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 +68,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, @@ -316,65 +318,97 @@ def test_when_trunk_id_is_network_node_trunk_id( mocker, understack_trunk_driver, trunk_id, + 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" ) + result = understack_trunk_driver._check_subports_segmentation_id( - [], str(trunk_id) + [], 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 = 4000 + 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( + def test_when_segmentation_id_matches_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", ) mocker.patch( - "neutron_understack.utils.allowed_tenant_vlan_id_ranges", - return_value=[(1, 1500)], + "neutron_understack.utils.network_segment_by_physnet", + return_value=vlan_network_segment, ) - subport.segmentation_id = 1600 - with pytest.raises(SubportSegmentationIDError): - understack_trunk_driver._check_subports_segmentation_id([subport], trunk_id) + port_object.network_id = str(network_id) + subport.segmentation_id = vlan_network_segment.segmentation_id + + with pytest.raises(SubportSegmentationIDError, match="matches the native VLAN"): + 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 + 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/trunk.py b/python/neutron-understack/neutron_understack/trunk.py index 6b7e7fd40..d2011056a 100644 --- a/python/neutron-understack/neutron_understack/trunk.py +++ b/python/neutron-understack/neutron_understack/trunk.py @@ -24,8 +24,8 @@ 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." + "%(subport_id)s because it matches the native VLAN on physical " + "network: %(physical_network)s. Please use a different Segmentation ID." ) @@ -87,8 +87,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,15 +96,14 @@ 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. + """Reject a subport tag that matches the parent's native VLAN. 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. + 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 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 @@ -114,14 +113,30 @@ def _check_subports_segmentation_id( if trunk_id == utils.fetch_network_node_trunk_id(): return - ns_ranges = utils.allowed_tenant_vlan_id_ranges() + 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 + + native_vlan_id = int(native_segment.segmentation_id) for subport in subports: seg_id = subport.segmentation_id - if not utils.segmentation_id_in_ranges(seg_id, ns_ranges): + 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), + physical_network=physical_network, ) def configure_trunk(self, trunk_details: dict, port_id: str) -> None: From 1250c5933a73d3a17befc2c7e8fb225b9fb03ee2 Mon Sep 17 00:00:00 2001 From: Steve Keay Date: Tue, 6 Oct 2026 14:45:01 +0100 Subject: [PATCH 2/7] Extend to check the native vlan setting as well as the subport setting --- .../tests/scenarios/test_trunk_operations.py | 2 +- .../neutron_understack/tests/test_trunk.py | 19 ++- .../tests/test_understack_mech.py | 128 +++++++++++++++++- .../neutron_understack/trunk.py | 60 ++++++-- .../neutron_understack/understack_mech.py | 45 +++++- 5 files changed, 237 insertions(+), 17 deletions(-) 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 ac673d319..776f2838f 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 @@ -293,7 +293,7 @@ def test_subport_segid_matching_native_vlan_rejected(self): subport_id, seg_id=native_segment[segments_db.SEGMENTATION_ID], ) - assert "matches the native VLAN" in str(exc_info.value) + 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( diff --git a/python/neutron-understack/neutron_understack/tests/test_trunk.py b/python/neutron-understack/neutron_understack/tests/test_trunk.py index 02fd149f3..8c8bd179c 100644 --- a/python/neutron-understack/neutron_understack/tests/test_trunk.py +++ b/python/neutron-understack/neutron_understack/tests/test_trunk.py @@ -282,8 +282,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=[], @@ -386,7 +401,7 @@ def test_when_segmentation_id_matches_native_vlan( port_object.network_id = str(network_id) subport.segmentation_id = vlan_network_segment.segmentation_id - with pytest.raises(SubportSegmentationIDError, match="matches the native VLAN"): + with pytest.raises(SubportSegmentationIDError, match="used as both"): understack_trunk_driver._check_subports_segmentation_id( [subport], trunk_id, port_object ) 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..9224b4b12 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( @@ -22,6 +24,100 @@ def _port_update_context(original, current): class TestUpdatePortPreCommit: + def test_rejects_parent_native_vlan_used_by_existing_subport( + self, mocker, understack_driver, understack_trunk_driver + ): + trunk_details = { + "trunk_id": "trunk-a", + "sub_ports": [ + { + "port_id": "subport-a", + "segmentation_id": 2222, + } + ], + } + original = { + "id": "parent-a", + "network_id": "network-a", + "device_owner": "", + portbindings.PROFILE: {"physical_network": "physnet-a"}, + "trunk_details": trunk_details, + } + current = { + **original, + portbindings.PROFILE: {"physical_network": "physnet-b"}, + } + native_segment = mocker.Mock(segmentation_id=2222) + find_segment = mocker.patch( + f"{MECH_UTILS}.network_segment_by_physnet", + return_value=native_segment, + ) + mocker.patch( + "neutron_understack.utils.fetch_network_node_trunk_id", + return_value="network-node-trunk", + ) + understack_driver.trunk_driver = understack_trunk_driver + + with pytest.raises(trunk.SubportSegmentationIDError, match="used as both"): + understack_driver.update_port_precommit( + _port_update_context(original, current) + ) + + find_segment.assert_called_once_with( + network_id="network-a", physnet="physnet-b" + ) + + def test_defers_parent_native_vlan_check_until_binding_when_segment_missing( + self, mocker, understack_driver, understack_trunk_driver + ): + trunk_details = { + "trunk_id": "trunk-a", + "sub_ports": [ + { + "port_id": "subport-a", + "segmentation_id": 2222, + } + ], + } + original = { + "id": "parent-a", + "network_id": "network-a", + "device_owner": "", + portbindings.PROFILE: {"physical_network": "physnet-a"}, + "trunk_details": trunk_details, + } + current = { + **original, + portbindings.PROFILE: {"physical_network": "physnet-b"}, + } + mocker.patch(f"{MECH_UTILS}.network_segment_by_physnet", return_value=None) + understack_driver.trunk_driver = understack_trunk_driver + check_native_vlan = mocker.patch.object( + understack_driver.trunk_driver, "check_trunk_native_vlan" + ) + + understack_driver.update_port_precommit(_port_update_context(original, current)) + + check_native_vlan.assert_not_called() + + def test_skips_native_vlan_check_when_parent_physnet_is_unchanged( + self, mocker, understack_driver, understack_trunk_driver + ): + port = { + "id": "parent-a", + "network_id": "network-a", + "device_owner": "", + portbindings.PROFILE: {"physical_network": "physnet-a"}, + "trunk_details": {"trunk_id": "trunk-a", "sub_ports": []}, + } + current = {**port, "name": "new-name"} + find_segment = mocker.patch(f"{MECH_UTILS}.network_segment_by_physnet") + understack_driver.trunk_driver = understack_trunk_driver + + understack_driver.update_port_precommit(_port_update_context(port, current)) + + find_segment.assert_not_called() + def test_validates_when_port_becomes_router_interface( self, mocker, understack_driver ): @@ -171,9 +267,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 @@ -381,11 +474,36 @@ 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", + ) + 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() + 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 d2011056a..b85f9dff5 100644 --- a/python/neutron-understack/neutron_understack/trunk.py +++ b/python/neutron-understack/neutron_understack/trunk.py @@ -23,9 +23,9 @@ class SubportSegmentationIDError(exc.NeutronException): message = ( - "Segmentation ID: %(seg_id)s cannot be set to the Subport: " - "%(subport_id)s because it matches the native VLAN on physical " - "network: %(physical_network)s. Please use a 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." ) @@ -129,19 +129,63 @@ def _check_subports_segmentation_id( if native_segment is None: return - native_vlan_id = int(native_segment.segmentation_id) + 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 = subport.segmentation_id + seg_id = int(subport["segmentation_id"]) if seg_id == native_vlan_id: raise SubportSegmentationIDError( seg_id=seg_id, - subport_id=subport.port_id, + 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..b6bd8e599 100644 --- a/python/neutron-understack/neutron_understack/understack_mech.py +++ b/python/neutron-understack/neutron_understack/understack_mech.py @@ -189,6 +189,8 @@ def create_port_postcommit(self, context: PortContext) -> None: raise def update_port_precommit(self, context: PortContext): + self._check_trunk_native_vlan(context) + if not utils.is_router_interface(context): return @@ -246,6 +248,42 @@ def update_port_precommit(self, context: PortContext): }, ) + def _check_trunk_native_vlan(self, context: PortContext) -> None: + """Reject a parent-port update that would collide with a subport tag.""" + port = context.current + trunk_details = port.get("trunk_details") + if not trunk_details: + return + + physical_network = (port.get(portbindings.PROFILE) or {}).get( + "physical_network" + ) + if not physical_network: + return + + original_profile = context.original.get(portbindings.PROFILE) or {} + original_physical_network = original_profile.get("physical_network") + if physical_network == original_physical_network: + # The native VLAN for a (network, physnet) pair is stable. If a + # rebind has to allocate a replacement segment, the binding-path + # candidate check below remains authoritative. + return + + native_segment = utils.network_segment_by_physnet( + network_id=port["network_id"], physnet=physical_network + ) + if native_segment is None: + # A first binding to this (network, physnet) allocates the segment + # later. _bind_port_segment validates that candidate before doing + # any trunk configuration. + return + + self.trunk_driver.check_trunk_native_vlan( + trunk_details=trunk_details, + physical_network=physical_network, + native_vlan_id=native_segment.segmentation_id, + ) + def update_port_postcommit(self, context: PortContext) -> None: if utils.is_baremetal_port(context) and utils.is_port_unbinding(context): self._tenant_network_port_cleanup(context) @@ -383,7 +421,12 @@ def _bind_port_segment(self, context: PortContext, 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) + self.trunk_driver.configure_trunk( + trunk_details=trunk_details, + port_id=port_id, + physical_network=physnet, + native_vlan_id=dynamic_segment[api.SEGMENTATION_ID], + ) LOG.debug("continue_binding for segment: %s", segment) context.continue_binding( From 8498de889f9ac8fa2c43c18a94ca3b949312d51b Mon Sep 17 00:00:00 2001 From: Steve Keay Date: Tue, 6 Oct 2026 15:46:44 +0100 Subject: [PATCH 3/7] Restrict VLAN IDs to within a reasonably reliable range Avoid VLANs that may be reserved/used by Cisco for their own purposes. --- .../neutron_understack/config.py | 6 +- .../tests/scenarios/SCENARIOS.md | 9 +- .../tests/scenarios/test_trunk_operations.py | 23 ++--- .../neutron_understack/tests/test_trunk.py | 83 ++++++++++++++++++- .../neutron_understack/trunk.py | 28 +++++-- 5 files changed, 125 insertions(+), 24 deletions(-) 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/scenarios/SCENARIOS.md b/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md index abb4573cb..8adabe6b8 100644 --- a/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md +++ b/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md @@ -301,11 +301,12 @@ adds/removes must reconcile the parent's switch (VLAN group). callback, surfaced as `CallbackFailure`), with no binding level or dynamic segment allocated -### TRUNK-SEGID-NONNATIVE-01 — any non-native subport seg_id is allowed +### 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 valid VLAN segmentation_id outside the old - broad allowed range but different from the parent's native VLAN -- then: the subport is added and gets its dynamic segment normally +- 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 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 776f2838f..9884d9a2d 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 @@ -302,19 +302,22 @@ def test_subport_segid_matching_native_vlan_rejected(self): is None ) - @pytest.mark.scenario("TRUNK-SEGID-NONNATIVE-01") - def test_subport_segid_outside_old_range_is_allowed(self): + @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) - native_segment = segments_db.get_dynamic_segment( - self.context, parent_net, physical_network=DEFAULT_PHYSNET + with pytest.raises(cb_exc.CallbackFailure) as exc_info: + self._add_subport(trunk_id, subport_id, seg_id=4000) + 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( + self.context, subport_net, physical_network=DEFAULT_PHYSNET + ) + is None ) - native_vlan = native_segment[segments_db.SEGMENTATION_ID] - subport_vlan = 4000 if native_vlan != 4000 else 4001 - - self._add_subport(trunk_id, subport_id, seg_id=subport_vlan) - - self._assert_subport_bound(trunk_id, subport_id, "host-a", seg_id=subport_vlan) diff --git a/python/neutron-understack/neutron_understack/tests/test_trunk.py b/python/neutron-understack/neutron_understack/tests/test_trunk.py index 8c8bd179c..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: @@ -327,12 +328,14 @@ 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, ): mocker.patch( @@ -342,9 +345,10 @@ def test_when_trunk_id_is_network_node_trunk_id( 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), port_object + [subport], str(trunk_id), port_object ) find_segment.assert_not_called() @@ -369,7 +373,7 @@ def test_when_segmentation_id_differs_from_native_vlan( return_value=vlan_network_segment, ) port_object.network_id = str(network_id) - subport.segmentation_id = 4000 + subport.segmentation_id = 2000 result = understack_trunk_driver._check_subports_segmentation_id( [subport], trunk_id, port_object @@ -380,6 +384,80 @@ def test_when_segmentation_id_differs_from_native_vlan( ) assert result is None + @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, + ): + 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.fetch_network_node_trunk_id", + return_value="different-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, @@ -414,6 +492,7 @@ def test_when_parent_port_is_unbound( 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" ) diff --git a/python/neutron-understack/neutron_understack/trunk.py b/python/neutron-understack/neutron_understack/trunk.py index b85f9dff5..5f8fbe82d 100644 --- a/python/neutron-understack/neutron_understack/trunk.py +++ b/python/neutron-understack/neutron_understack/trunk.py @@ -29,6 +29,13 @@ class SubportSegmentationIDError(exc.NeutronException): ) +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." + ) + + def _missing_physnet_msg(port_id: str) -> str: """physical_network names the segment range a subport allocates from.""" return ( @@ -98,21 +105,32 @@ def _handle_tenant_vlan_id_and_switchport_config( def _check_subports_segmentation_id( self, subports: list[SubPort], trunk_id: str, parent_port: Port ) -> None: - """Reject a subport tag that matches the parent's native VLAN. + """Validate tenant tags and reject a parent native VLAN collision. A switchport cannot have a mapped VLAN ID equal to the native VLAN ID. 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 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. + 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 + minimum, maximum = cfg.CONF.ml2_understack.default_tenant_vlan_id_range + for subport in subports: + 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 From 7fed28eed1e2805fbe777ef2f154c2adb97b0bb4 Mon Sep 17 00:00:00 2001 From: Steve Keay Date: Fri, 9 Oct 2026 12:20:59 +0100 Subject: [PATCH 4/7] drop! testing neutron pr-2456 in dev --- components/images-openstack.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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" From 1a6d11729b3685b95af7b4e949bd378fd527a880 Mon Sep 17 00:00:00 2001 From: Doug Goldstein Date: Fri, 9 Oct 2026 14:23:05 -0500 Subject: [PATCH 5/7] Release native VLAN segment when subport collision fails binding The native VLAN is only known once the dynamic segment has been allocated, so on a first bind the subport collision check in configure_trunk() can only run afterwards. If it raised, the freshly allocated segment was left on the network. ML2 swallows exceptions from bind_port, so nothing gave it back until the port was deleted, and every retry reused it and failed the same way. Release the segment on failure, but only if this bind allocated it and no other port is bound to it. A segment that already existed is not ours to release. --- .../tests/test_understack_mech.py | 104 ++++++++++++++++++ .../neutron_understack/understack_mech.py | 27 +++-- 2 files changed, 123 insertions(+), 8 deletions(-) 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 9224b4b12..2405b9b09 100644 --- a/python/neutron-understack/neutron_understack/tests/test_understack_mech.py +++ b/python/neutron-understack/neutron_understack/tests/test_understack_mech.py @@ -431,6 +431,110 @@ 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, + vlan_network_segment, + ): + """A native VLAN that collides with a subport must not leak its segment.""" + 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.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, + vlan_network_segment, + ): + """allocate_dynamic_segment can return a segment another port is using.""" + 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.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, + vlan_network_segment, + ): + """A segment that already existed is not ours to release.""" + mocker.patch( + f"{MECH_UTILS}.vlan_segment_for_physnet", + return_value=vlan_network_segment, + ) + 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 ): diff --git a/python/neutron-understack/neutron_understack/understack_mech.py b/python/neutron-understack/neutron_understack/understack_mech.py index b6bd8e599..e5bf107b1 100644 --- a/python/neutron-understack/neutron_understack/understack_mech.py +++ b/python/neutron-understack/neutron_understack/understack_mech.py @@ -401,7 +401,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", @@ -409,24 +411,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=trunk_details, - port_id=port_id, - physical_network=physnet, - native_vlan_id=dynamic_segment[api.SEGMENTATION_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( From 1e28e5f03391a86d9cfe993c72895fb90bba187e Mon Sep 17 00:00:00 2001 From: Steve Keay Date: Mon, 12 Oct 2026 12:54:38 +0100 Subject: [PATCH 6/7] Remove redundant check - port update can't cause native vlan clash The parent port is not allowed to be updated after the trunk has been created so this extra check will never catch any problems. --- .../tests/test_understack_mech.py | 130 ++++-------------- .../neutron_understack/understack_mech.py | 38 ----- 2 files changed, 27 insertions(+), 141 deletions(-) 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 2405b9b09..7d06a5518 100644 --- a/python/neutron-understack/neutron_understack/tests/test_understack_mech.py +++ b/python/neutron-understack/neutron_understack/tests/test_understack_mech.py @@ -24,100 +24,6 @@ def _port_update_context(original, current): class TestUpdatePortPreCommit: - def test_rejects_parent_native_vlan_used_by_existing_subport( - self, mocker, understack_driver, understack_trunk_driver - ): - trunk_details = { - "trunk_id": "trunk-a", - "sub_ports": [ - { - "port_id": "subport-a", - "segmentation_id": 2222, - } - ], - } - original = { - "id": "parent-a", - "network_id": "network-a", - "device_owner": "", - portbindings.PROFILE: {"physical_network": "physnet-a"}, - "trunk_details": trunk_details, - } - current = { - **original, - portbindings.PROFILE: {"physical_network": "physnet-b"}, - } - native_segment = mocker.Mock(segmentation_id=2222) - find_segment = mocker.patch( - f"{MECH_UTILS}.network_segment_by_physnet", - return_value=native_segment, - ) - mocker.patch( - "neutron_understack.utils.fetch_network_node_trunk_id", - return_value="network-node-trunk", - ) - understack_driver.trunk_driver = understack_trunk_driver - - with pytest.raises(trunk.SubportSegmentationIDError, match="used as both"): - understack_driver.update_port_precommit( - _port_update_context(original, current) - ) - - find_segment.assert_called_once_with( - network_id="network-a", physnet="physnet-b" - ) - - def test_defers_parent_native_vlan_check_until_binding_when_segment_missing( - self, mocker, understack_driver, understack_trunk_driver - ): - trunk_details = { - "trunk_id": "trunk-a", - "sub_ports": [ - { - "port_id": "subport-a", - "segmentation_id": 2222, - } - ], - } - original = { - "id": "parent-a", - "network_id": "network-a", - "device_owner": "", - portbindings.PROFILE: {"physical_network": "physnet-a"}, - "trunk_details": trunk_details, - } - current = { - **original, - portbindings.PROFILE: {"physical_network": "physnet-b"}, - } - mocker.patch(f"{MECH_UTILS}.network_segment_by_physnet", return_value=None) - understack_driver.trunk_driver = understack_trunk_driver - check_native_vlan = mocker.patch.object( - understack_driver.trunk_driver, "check_trunk_native_vlan" - ) - - understack_driver.update_port_precommit(_port_update_context(original, current)) - - check_native_vlan.assert_not_called() - - def test_skips_native_vlan_check_when_parent_physnet_is_unchanged( - self, mocker, understack_driver, understack_trunk_driver - ): - port = { - "id": "parent-a", - "network_id": "network-a", - "device_owner": "", - portbindings.PROFILE: {"physical_network": "physnet-a"}, - "trunk_details": {"trunk_id": "trunk-a", "sub_ports": []}, - } - current = {**port, "name": "new-name"} - find_segment = mocker.patch(f"{MECH_UTILS}.network_segment_by_physnet") - understack_driver.trunk_driver = understack_trunk_driver - - understack_driver.update_port_precommit(_port_update_context(port, current)) - - find_segment.assert_not_called() - def test_validates_when_port_becomes_router_interface( self, mocker, understack_driver ): @@ -436,19 +342,24 @@ def test_releases_allocated_segment_when_trunk_check_fails( 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.configure_trunk.side_effect = ( - trunk.SubportSegmentationIDError( + 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) @@ -470,21 +381,26 @@ def test_keeps_allocated_segment_when_other_ports_are_bound( 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.configure_trunk.side_effect = ( - trunk.SubportSegmentationIDError( + 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) @@ -506,19 +422,24 @@ def test_keeps_reused_segment_when_trunk_check_fails( 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.configure_trunk.side_effect = ( - trunk.SubportSegmentationIDError( + 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) @@ -601,12 +522,15 @@ def test_rejects_new_native_vlan_used_by_existing_subport_before_binding( "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: diff --git a/python/neutron-understack/neutron_understack/understack_mech.py b/python/neutron-understack/neutron_understack/understack_mech.py index e5bf107b1..49d77dd5f 100644 --- a/python/neutron-understack/neutron_understack/understack_mech.py +++ b/python/neutron-understack/neutron_understack/understack_mech.py @@ -189,8 +189,6 @@ def create_port_postcommit(self, context: PortContext) -> None: raise def update_port_precommit(self, context: PortContext): - self._check_trunk_native_vlan(context) - if not utils.is_router_interface(context): return @@ -248,42 +246,6 @@ def update_port_precommit(self, context: PortContext): }, ) - def _check_trunk_native_vlan(self, context: PortContext) -> None: - """Reject a parent-port update that would collide with a subport tag.""" - port = context.current - trunk_details = port.get("trunk_details") - if not trunk_details: - return - - physical_network = (port.get(portbindings.PROFILE) or {}).get( - "physical_network" - ) - if not physical_network: - return - - original_profile = context.original.get(portbindings.PROFILE) or {} - original_physical_network = original_profile.get("physical_network") - if physical_network == original_physical_network: - # The native VLAN for a (network, physnet) pair is stable. If a - # rebind has to allocate a replacement segment, the binding-path - # candidate check below remains authoritative. - return - - native_segment = utils.network_segment_by_physnet( - network_id=port["network_id"], physnet=physical_network - ) - if native_segment is None: - # A first binding to this (network, physnet) allocates the segment - # later. _bind_port_segment validates that candidate before doing - # any trunk configuration. - return - - self.trunk_driver.check_trunk_native_vlan( - trunk_details=trunk_details, - physical_network=physical_network, - native_vlan_id=native_segment.segmentation_id, - ) - def update_port_postcommit(self, context: PortContext) -> None: if utils.is_baremetal_port(context) and utils.is_port_unbinding(context): self._tenant_network_port_cleanup(context) From b7bdc290c858fd7df6e22bf4b44a35a6a50b529c Mon Sep 17 00:00:00 2001 From: Doug Goldstein Date: Fri, 9 Oct 2026 14:45:20 -0500 Subject: [PATCH 7/7] Test trunk configuration when everything exists before parent bind Cover the case where a parent port, its trunk and all subports are created before the parent is bound. Subport adds are no-ops while the parent has no physical network, so configure_trunk() has to allocate the subport segments and binding levels when the parent later binds. Also mark the existing native VLAN collision scenario as a strict xfail. The collision is detected in bind_port, but ML2 cannot abort a bind: it falls through to the next mechanism driver and the port still binds (vif_type=other) rather than binding_failed. This needs the upstream Neutron fix backported. --- .../neutron_understack/tests/conftest.py | 8 +- .../tests/scenarios/SCENARIOS.md | 18 ++++ .../tests/scenarios/test_trunk_operations.py | 99 +++++++++++++++++++ 3 files changed, 123 insertions(+), 2 deletions(-) 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 8adabe6b8..d636ede38 100644 --- a/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md +++ b/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md @@ -293,6 +293,24 @@ 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-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 equal to the VLAN segment of 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 9884d9a2d..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"]