From 909aa8c7faa99b00ad540fac5a60fe32d7fa13f7 Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Thu, 10 Apr 2025 13:43:30 +0100 Subject: [PATCH 1/2] Add reproducer test for bug 2105896 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we added support for neutron security groups that were shared via rbac policy in epoxy the way we detected duplicate groups was implemented incorrectly. NoUniqueMatchova should reject requests by name if there are two security groups with the same name however we also reject requests when you use the uuid. That is a bug. Related-Bug: #2105896 Co-Authored-By: René Ribaud Change-Id: I0e1dda07110a99daee1137d7692689a6dd274af8 Signed-off-by: René Ribaud (cherry picked from commit 47ed32399aabc2f55082c78377a55cabfef337bd) --- nova/tests/unit/network/test_neutron.py | 94 +++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/nova/tests/unit/network/test_neutron.py b/nova/tests/unit/network/test_neutron.py index 8fd55ee867b..e2261140115 100644 --- a/nova/tests/unit/network/test_neutron.py +++ b/nova/tests/unit/network/test_neutron.py @@ -9432,6 +9432,100 @@ def test__process_security_groups_non_unique_match(self): [mock.call(fields=['id', 'name'], tenant_id=uuids.project_id), mock.call(fields=['id', 'name'], shared=True)]) + def test__process_security_groups_unique_uuids(self): + instance = objects.Instance(project_id=uuids.project_id) + mock_neutron = mock.Mock(spec=client.Client) + mock_neutron.list_security_groups.side_effect = [ + { + 'security_groups': [ + { + 'id': uuids.sg1, + 'name': 'nonunique-name', + } + ] + }, + { + 'security_groups': [ + { + 'id': uuids.sg2, + 'name': 'nonunique-name', + } + ] + } + ] + mock_neutron.list_extensions.return_value = { + 'extensions': [{'alias': constants.SG_SHARED_FILTER}]} + api = neutronapi.API() + + # FIXME(sean-k-mooney): this is bug 2105896 + # it is ok for security groups to have the same name if we + # request them by uuid. + ex = self.assertRaises( + exception.NoUniqueMatch, api._process_security_groups, + instance, mock_neutron, [uuids.sg1, uuids.sg2]) + + self.assertIn("nonunique-name", str(ex)) + mock_neutron.list_security_groups.assert_has_calls( + [mock.call(fields=['id', 'name'], tenant_id=uuids.project_id), + mock.call(fields=['id', 'name'], shared=True)]) + + def test__process_security_groups_non_unique_match_same_tenant(self): + """Test that duplicate names within the same tenant raise + NoUniqueMatch when requested by name. + """ + instance = objects.Instance(project_id=uuids.project_id) + mock_neutron = mock.Mock(spec=client.Client) + mock_neutron.list_security_groups.return_value = { + 'security_groups': [ + { + 'id': uuids.sg1, + 'name': 'nonunique-name', + }, + { + 'id': uuids.sg2, + 'name': 'nonunique-name', + } + ] + } + mock_neutron.list_extensions.return_value = { + 'extensions': [{'alias': constants.SG_SHARED_FILTER}]} + api = neutronapi.API() + + ex = self.assertRaises( + exception.NoUniqueMatch, api._process_security_groups, + instance, mock_neutron, ["nonunique-name"]) + self.assertIn("nonunique-name", str(ex)) + + def test__process_security_groups_unique_uuids_same_tenant(self): + """Test that duplicate names within the same tenant are handled + when requested by UUID. + """ + instance = objects.Instance(project_id=uuids.project_id) + mock_neutron = mock.Mock(spec=client.Client) + mock_neutron.list_security_groups.return_value = { + 'security_groups': [ + { + 'id': uuids.sg1, + 'name': 'nonunique-name', + }, + { + 'id': uuids.sg2, + 'name': 'nonunique-name', + } + ] + } + mock_neutron.list_extensions.return_value = { + 'extensions': [{'alias': constants.SG_SHARED_FILTER}]} + api = neutronapi.API() + + # FIXME(sean-k-mooney): this is bug 2105896 + # it is ok for security groups to have the same name if we + # request them by uuid. + ex = self.assertRaises( + exception.NoUniqueMatch, api._process_security_groups, + instance, mock_neutron, [uuids.sg1, uuids.sg2]) + self.assertIn("nonunique-name", str(ex)) + @mock.patch.object(neutronapi.API, 'get_instance_nw_info') @mock.patch.object(neutronapi.API, '_update_port_dns_name') @mock.patch.object(neutronapi.API, '_create_port_minimal') From 556ba4c9ccef03216ae465d5c4eb85ce2255a4c9 Mon Sep 17 00:00:00 2001 From: cw0306-lee Date: Wed, 10 Dec 2025 14:07:24 +0900 Subject: [PATCH 2/2] Fix error when multiple security group in a project with same name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server build fail when user give any security group and there are same name security exists in project. I fixed it occurs error when user give security group with duplicated name. Closes-Bug: #2105896 Change-Id: Ie061550c6eecb51951cebd9c323f31b93b748ff5 Co-Authored-By: René Ribaud Signed-off-by: cw0306-lee (cherry picked from commit 96a0b17c7cdd9e64c558b6f005ff2b260020cbe6) --- nova/network/neutron.py | 41 +++++++++---------- nova/tests/unit/network/test_neutron.py | 33 +++++++-------- .../notes/bug-2105896-2bebad3d9eacd346.yaml | 11 +++++ 3 files changed, 48 insertions(+), 37 deletions(-) create mode 100644 releasenotes/notes/bug-2105896-2bebad3d9eacd346.yaml diff --git a/nova/network/neutron.py b/nova/network/neutron.py index 557b71ee2b5..aac8a7cc7b2 100644 --- a/nova/network/neutron.py +++ b/nova/network/neutron.py @@ -24,6 +24,8 @@ import time import typing as ty +from collections import defaultdict + from neutronclient.common import exceptions as neutron_client_exc from neutronclient.v2_0 import client as clientv20 from oslo_concurrency import lockutils @@ -810,12 +812,12 @@ def _get_security_group_ids(self, security_groups, user_security_groups): :raises nova.exception.SecurityGroupNotFound: If a given security group is not found. """ - # Initialize two dictionaries to map security group names and IDs to - # their corresponding IDs - name_to_id = {} + # Map security group names to their IDs (a name can have + # multiple IDs since Neutron does not enforce uniqueness) + name_to_id = defaultdict(list) # NOTE(sean-k-mooney): using a dict here instead of a set is faster # probably due to l1 code cache misses due to the introduction - # of set lookup in addition to dict lookups making the branch + # of set lookups in addition to dict lookups making the branch # prediction for the second for loop less reliable. id_to_id = {} @@ -823,14 +825,8 @@ def _get_security_group_ids(self, security_groups, user_security_groups): for user_security_group in user_security_groups: name = user_security_group['name'] sg_id = user_security_group['id'] - - # Check for duplicate names and raise an exception if found - if name in name_to_id: - raise exception.NoUniqueMatch( - _("Multiple security groups found matching" - " '%s'. Use an ID to be more specific.") % name) - # Map the name to its corresponding ID - name_to_id[name] = sg_id + # Append the ID to the list for this name + name_to_id[name].append(sg_id) # Map the ID to itself for easy lookup id_to_id[sg_id] = sg_id @@ -839,16 +835,19 @@ def _get_security_group_ids(self, security_groups, user_security_groups): # Iterate over the requested security groups for security_group in security_groups: - # Check if the security group is in the name-to-ID dictionary - # as if a user names the security group the same as - # another's security groups uuid, the name takes priority. - if security_group in name_to_id: - security_group_ids.append(name_to_id[security_group]) - # Check if the security group is in the ID-to-ID dictionary - elif security_group in id_to_id: + # Check UUID first since it is always unique + if security_group in id_to_id: security_group_ids.append(id_to_id[security_group]) - # Raise an exception if the security group is not found in - # either dictionary + # Then check by name + elif security_group in name_to_id: + # If there are multiple IDs for this name, raise exception + if len(name_to_id[security_group]) > 1: + raise exception.NoUniqueMatch( + _("Multiple security groups found matching" + " '%s'. Use an ID to be more specific.") + % security_group) + security_group_ids.append(name_to_id[security_group][0]) + # Raise an exception if the security group is not found else: raise exception.SecurityGroupNotFound( security_group_id=security_group) diff --git a/nova/tests/unit/network/test_neutron.py b/nova/tests/unit/network/test_neutron.py index e2261140115..046b39c38c0 100644 --- a/nova/tests/unit/network/test_neutron.py +++ b/nova/tests/unit/network/test_neutron.py @@ -9457,21 +9457,22 @@ def test__process_security_groups_unique_uuids(self): 'extensions': [{'alias': constants.SG_SHARED_FILTER}]} api = neutronapi.API() - # FIXME(sean-k-mooney): this is bug 2105896 - # it is ok for security groups to have the same name if we - # request them by uuid. - ex = self.assertRaises( - exception.NoUniqueMatch, api._process_security_groups, + # Bug 2105896: it is ok for security groups to have the same + # name if we request them by uuid. + result = api._process_security_groups( instance, mock_neutron, [uuids.sg1, uuids.sg2]) - self.assertIn("nonunique-name", str(ex)) + self.assertEqual([uuids.sg1, uuids.sg2], result) mock_neutron.list_security_groups.assert_has_calls( [mock.call(fields=['id', 'name'], tenant_id=uuids.project_id), mock.call(fields=['id', 'name'], shared=True)]) def test__process_security_groups_non_unique_match_same_tenant(self): - """Test that duplicate names within the same tenant raise - NoUniqueMatch when requested by name. + """Test that duplicate names within the same tenant are handled. + + When two SGs in the same tenant have the same name, requesting + by name should raise NoUniqueMatch, but requesting by UUID + should succeed. """ instance = objects.Instance(project_id=uuids.project_id) mock_neutron = mock.Mock(spec=client.Client) @@ -9491,14 +9492,15 @@ def test__process_security_groups_non_unique_match_same_tenant(self): 'extensions': [{'alias': constants.SG_SHARED_FILTER}]} api = neutronapi.API() + # Requesting by name should raise NoUniqueMatch ex = self.assertRaises( exception.NoUniqueMatch, api._process_security_groups, instance, mock_neutron, ["nonunique-name"]) self.assertIn("nonunique-name", str(ex)) def test__process_security_groups_unique_uuids_same_tenant(self): - """Test that duplicate names within the same tenant are handled - when requested by UUID. + """Test that duplicate names within the same tenant are accepted + when requested by UUID (bug 2105896). """ instance = objects.Instance(project_id=uuids.project_id) mock_neutron = mock.Mock(spec=client.Client) @@ -9518,13 +9520,12 @@ def test__process_security_groups_unique_uuids_same_tenant(self): 'extensions': [{'alias': constants.SG_SHARED_FILTER}]} api = neutronapi.API() - # FIXME(sean-k-mooney): this is bug 2105896 - # it is ok for security groups to have the same name if we - # request them by uuid. - ex = self.assertRaises( - exception.NoUniqueMatch, api._process_security_groups, + result = api._process_security_groups( instance, mock_neutron, [uuids.sg1, uuids.sg2]) - self.assertIn("nonunique-name", str(ex)) + self.assertEqual([uuids.sg1, uuids.sg2], result) + # Only one call since both SGs are in the tenant list + mock_neutron.list_security_groups.assert_called_once_with( + fields=['id', 'name'], tenant_id=uuids.project_id) @mock.patch.object(neutronapi.API, 'get_instance_nw_info') @mock.patch.object(neutronapi.API, '_update_port_dns_name') diff --git a/releasenotes/notes/bug-2105896-2bebad3d9eacd346.yaml b/releasenotes/notes/bug-2105896-2bebad3d9eacd346.yaml new file mode 100644 index 00000000000..8d74f6df1cc --- /dev/null +++ b/releasenotes/notes/bug-2105896-2bebad3d9eacd346.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + `Bug #2105896`_: Fix error when multiple security groups in a project + share the same name. The security group resolution now checks UUIDs + before names, ensuring that a request using a specific UUID is never + blocked by a naming collision. When a security group is requested by + name and multiple groups share that name, a ``NoUniqueMatch`` error is + raised prompting the user to use a UUID instead. + + .. _Bug #2105896: https://launchpad.net/bugs/2105896