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 8fd55ee867b..046b39c38c0 100644 --- a/nova/tests/unit/network/test_neutron.py +++ b/nova/tests/unit/network/test_neutron.py @@ -9432,6 +9432,101 @@ 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() + + # 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.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 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) + 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() + + # 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 accepted + when requested by UUID (bug 2105896). + """ + 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() + + result = api._process_security_groups( + instance, mock_neutron, [uuids.sg1, uuids.sg2]) + 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') @mock.patch.object(neutronapi.API, '_create_port_minimal') 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