diff --git a/nova/api/openstack/compute/security_groups.py b/nova/api/openstack/compute/security_groups.py index 038fded957e6..38f8096b2d3c 100644 --- a/nova/api/openstack/compute/security_groups.py +++ b/nova/api/openstack/compute/security_groups.py @@ -159,8 +159,7 @@ class SecurityGroupController(SecurityGroupControllerBase, wsgi.Controller): try: id = security_group_api.validate_id(id) - security_group = security_group_api.get( - context, None, id, map_exception=True) + security_group = security_group_api.get(context, id) except exception.SecurityGroupNotFound as exp: raise exc.HTTPNotFound(explanation=exp.format_message()) except exception.Invalid as exp: @@ -178,8 +177,7 @@ class SecurityGroupController(SecurityGroupControllerBase, wsgi.Controller): try: id = security_group_api.validate_id(id) - security_group = security_group_api.get( - context, None, id, map_exception=True) + security_group = security_group_api.get(context, id) security_group_api.destroy(context, security_group) except exception.SecurityGroupNotFound as exp: raise exc.HTTPNotFound(explanation=exp.format_message()) @@ -222,7 +220,7 @@ class SecurityGroupController(SecurityGroupControllerBase, wsgi.Controller): try: security_group_api.validate_property(group_name, 'name', None) security_group_api.validate_property(group_description, - 'description', None) + 'description', None) group_ref = security_group_api.create_security_group( context, group_name, group_description) except exception.Invalid as exp: @@ -241,8 +239,7 @@ class SecurityGroupController(SecurityGroupControllerBase, wsgi.Controller): try: id = security_group_api.validate_id(id) - security_group = security_group_api.get( - context, None, id, map_exception=True) + security_group = security_group_api.get(context, id) except exception.SecurityGroupNotFound as exp: raise exc.HTTPNotFound(explanation=exp.format_message()) except exception.Invalid as exp: @@ -283,7 +280,7 @@ class SecurityGroupRulesController(SecurityGroupControllerBase, parent_group_id = security_group_api.validate_id( sg_rule.get('parent_group_id')) security_group = security_group_api.get( - context, None, parent_group_id, map_exception=True) + context, parent_group_id) if group_id is not None: group_id = security_group_api.validate_id(group_id) @@ -354,8 +351,7 @@ class SecurityGroupRulesController(SecurityGroupControllerBase, id = security_group_api.validate_id(id) rule = security_group_api.get_rule(context, id) group_id = rule['parent_group_id'] - security_group = security_group_api.get( - context, None, group_id, map_exception=True) + security_group = security_group_api.get(context, group_id) security_group_api.remove_rules( context, security_group, [rule['id']]) except exception.SecurityGroupNotFound as exp: diff --git a/nova/compute/api.py b/nova/compute/api.py index bb1c59524399..aa54c0ddc659 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -385,13 +385,10 @@ class API(base.Base): :param context: The nova request context. :type context: nova.context.RequestContext - :param secgroups: list of requested security group names, or uuids in - the case of Neutron. + :param secgroups: list of requested security group names :type secgroups: list - :returns: list of requested security group names unmodified if using - nova-network. If using Neutron, the list returned is all uuids. - Note that 'default' is a special case and will be unmodified if - it's requested. + :returns: list of requested security group UUIDs; note that 'default' + is a special case and will be unmodified if it's requested. """ security_groups = [] for secgroup in secgroups: @@ -399,12 +396,8 @@ class API(base.Base): if secgroup == "default": security_groups.append(secgroup) continue - secgroup_dict = security_group_api.get(context, secgroup) - if not secgroup_dict: - raise exception.SecurityGroupNotFoundForProject( - project_id=context.project_id, security_group_id=secgroup) - - security_groups.append(secgroup_dict['id']) + secgroup_uuid = security_group_api.validate_name(context, secgroup) + security_groups.append(secgroup_uuid) return security_groups @@ -922,17 +915,17 @@ class API(base.Base): # When using Neutron, _check_requested_secgroups will translate and # return any requested security group names to uuids. - security_groups = ( - self._check_requested_secgroups(context, security_groups)) + security_groups = self._check_requested_secgroups( + context, security_groups) # Note: max_count is the number of instances requested by the user, # max_network_count is the maximum number of instances taking into # account any network quotas - max_network_count = self._check_requested_networks(context, - requested_networks, max_count) + max_network_count = self._check_requested_networks( + context, requested_networks, max_count) kernel_id, ramdisk_id = self._handle_kernel_and_ramdisk( - context, kernel_id, ramdisk_id, boot_meta) + context, kernel_id, ramdisk_id, boot_meta) config_drive = self._check_config_drive(config_drive) diff --git a/nova/network/security_group_api.py b/nova/network/security_group_api.py index 8861b4439182..e07dba162eb0 100644 --- a/nova/network/security_group_api.py +++ b/nova/network/security_group_api.py @@ -31,6 +31,7 @@ import six from six.moves import urllib from webob import exc +from nova import context as nova_context from nova import exception from nova.i18n import _ from nova.network import neutron as neutronapi @@ -53,6 +54,33 @@ def validate_id(id): return id +def validate_name( + context: nova_context.RequestContext, + name: str): + """Validate a security group name and return the corresponding UUID. + + :param context: The nova request context. + :param name: The name of the security group. + :raises NoUniqueMatch: If there is no unique match for the provided name. + :raises SecurityGroupNotFound: If there's no match for the provided name. + :raises NeutronClientException: For all other exceptions. + """ + neutron = neutronapi.get_client(context) + try: + return neutronv20.find_resourceid_by_name_or_id( + neutron, 'security_group', name, context.project_id) + except n_exc.NeutronClientNoUniqueMatch as e: + raise exception.NoUniqueMatch(six.text_type(e)) + except n_exc.NeutronClientException as e: + exc_info = sys.exc_info() + if e.status_code == 404: + LOG.debug('Neutron security group %s not found', name) + raise exception.SecurityGroupNotFound(six.text_type(e)) + else: + LOG.error('Neutron Error: %s', e) + six.reraise(*exc_info) + + def parse_cidr(cidr): if not cidr: return '0.0.0.0/0' @@ -280,44 +308,26 @@ def _convert_to_nova_security_group_rule_format(rule): return nova_rule -def get(context, name=None, id=None, map_exception=False): +def get(context, id): neutron = neutronapi.get_client(context) try: - if not id and name: - # NOTE(flwang): The project id should be honoured so as to get - # the correct security group id when user(with admin role but - # non-admin project) try to query by name, so as to avoid - # getting more than duplicated records with the same name. - id = neutronv20.find_resourceid_by_name_or_id( - neutron, 'security_group', name, context.project_id) group = neutron.show_security_group(id).get('security_group') return _convert_to_nova_security_group_format(group) - except n_exc.NeutronClientNoUniqueMatch as e: - raise exception.NoUniqueMatch(six.text_type(e)) except n_exc.NeutronClientException as e: exc_info = sys.exc_info() if e.status_code == 404: - LOG.debug("Neutron security group %s not found", name) + LOG.debug('Neutron security group %s not found', id) raise exception.SecurityGroupNotFound(six.text_type(e)) else: LOG.error("Neutron Error: %s", e) six.reraise(*exc_info) - except TypeError as e: - LOG.error("Neutron Error: %s", e) - msg = _("Invalid security group name: %(name)s.") % {"name": name} - raise exception.SecurityGroupNotFound(six.text_type(msg)) -def list(context, names=None, ids=None, project=None, - search_opts=None): +def list(context, project, search_opts=None): """Returns list of security group rules owned by tenant.""" neutron = neutronapi.get_client(context) params = {} search_opts = search_opts if search_opts else {} - if names: - params['name'] = names - if ids: - params['id'] = ids # NOTE(jeffrey4l): list all the security groups when following # conditions are met @@ -325,23 +335,25 @@ def list(context, names=None, ids=None, project=None, # * it is admin context and all_tenants exist in search_opts. # * project is not specified. list_all_tenants = (context.is_admin and - 'all_tenants' in search_opts and - not any([names, ids])) - # NOTE(jeffrey4l): The neutron doesn't have `all-tenants` concept. + 'all_tenants' in search_opts) + # NOTE(jeffrey4l): neutron doesn't have `all-tenants` concept. # All the security group will be returned if the project/tenant # id is not passed. - if project and not list_all_tenants: + if not list_all_tenants: params['tenant_id'] = project + try: security_groups = neutron.list_security_groups(**params).get( 'security_groups') except n_exc.NeutronClientException: with excutils.save_and_reraise_exception(): LOG.exception("Neutron Error getting security groups") + converted_rules = [] for security_group in security_groups: converted_rules.append( _convert_to_nova_security_group_format(security_group)) + return converted_rules diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index d16a752f027f..a39152c6e727 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -8704,12 +8704,11 @@ class ComputeAPITestCase(BaseTestCase): def test_create_instance_associates_security_groups(self): # Make sure create associates security groups. - group = {'id': uuids.secgroup_id, 'name': 'testgroup'} with test.nested( mock.patch.object(self.compute_api.compute_task_api, 'schedule_and_build_instances'), - mock.patch('nova.network.security_group_api.get', - return_value=group), + mock.patch('nova.network.security_group_api.validate_name', + return_value=uuids.secgroup_id), ) as (mock_sbi, mock_secgroups): self.compute_api.create( self.context, @@ -8721,14 +8720,16 @@ class ComputeAPITestCase(BaseTestCase): reqspec = build_call[1]['request_spec'][0] self.assertEqual(1, len(reqspec.security_groups)) - self.assertEqual(group['id'], reqspec.security_groups[0].uuid) + self.assertEqual(uuids.secgroup_id, reqspec.security_groups[0].uuid) mock_secgroups.assert_called_once_with(mock.ANY, 'testgroup') def test_create_instance_with_invalid_security_group_raises(self): pre_build_len = len(db.instance_get_all(self.context)) - with mock.patch('nova.network.security_group_api.get', - return_value=None) as mock_secgroups: - self.assertRaises(exception.SecurityGroupNotFoundForProject, + with mock.patch( + 'nova.network.security_group_api.validate_name', + side_effect=exception.SecurityGroupNotFound('foo'), + ) as mock_secgroups: + self.assertRaises(exception.SecurityGroupNotFound, self.compute_api.create, self.context, instance_type=self.default_flavor, diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 9ff320b50af2..30da0fdcd723 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -6708,7 +6708,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): access_ip_v4 = access_ip_v6 = config_drive = \ auto_disk_config = reservation_id = None # This tests that 'default' is unchanged, but 'fake-security-group' - # will be translated to a uuid for Neutron. + # will be translated to a UUID for Neutron. requested_secgroups = ['default', 'fake-security-group'] # This will short-circuit _check_requested_networks requested_networks = objects.NetworkRequestList(objects=[ @@ -6716,8 +6716,8 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): max_count = 1 supports_port_resource_request = False with mock.patch( - 'nova.network.security_group_api.get', - return_value={'id': uuids.secgroup_uuid}) as scget: + 'nova.network.security_group_api.validate_name', + return_value=uuids.secgroup_uuid) as scget: base_options, max_network_count, key_pair, security_groups, \ network_metadata = ( self.compute_api._validate_and_build_base_options( diff --git a/nova/tests/unit/network/test_security_group.py b/nova/tests/unit/network/test_security_group.py index 08e8761c87fb..c2b6c6e6f957 100644 --- a/nova/tests/unit/network/test_security_group.py +++ b/nova/tests/unit/network/test_security_group.py @@ -78,47 +78,6 @@ class TestNeutronDriver(test.NoDBTestCase): mock_list_secgroup.assert_called_once_with(tenant_id=project_id) - def test_list_with_all_tenants_sec_name_and_admin_context(self): - project_id = '0af70a4d22cf4652824ddc1f2435dd85' - search_opts = {'all_tenants': 1} - security_group_names = ['secgroup_ssh'] - security_groups_list = {'security_groups': []} - admin_context = context.RequestContext('user1', project_id, True) - - with mock.patch.object( - self.mocked_client, - 'list_security_groups', - return_value=security_groups_list) as mock_list_secgroup: - sg_api.list(admin_context, project=project_id, - names=security_group_names, - search_opts=search_opts) - - mock_list_secgroup.assert_called_once_with( - name=security_group_names, - tenant_id=project_id) - - def test_list_with_all_tenants_sec_name_ids_and_admin_context(self): - project_id = '0af70a4d22cf4652824ddc1f2435dd85' - search_opts = {'all_tenants': 1} - security_group_names = ['secgroup_ssh'] - security_group_ids = ['id1'] - security_groups_list = {'security_groups': []} - admin_context = context.RequestContext('user1', project_id, True) - - with mock.patch.object( - self.mocked_client, - 'list_security_groups', - return_value=security_groups_list) as mock_list_secgroup: - sg_api.list(admin_context, project=project_id, - names=security_group_names, - ids=security_group_ids, - search_opts=search_opts) - - mock_list_secgroup.assert_called_once_with( - name=security_group_names, - id=security_group_ids, - tenant_id=project_id) - def test_list_with_all_tenants_not_admin(self): search_opts = {'all_tenants': 1} security_groups_list = {'security_groups': []} @@ -133,36 +92,6 @@ class TestNeutronDriver(test.NoDBTestCase): mock_list_secgroup.assert_called_once_with( tenant_id=self.context.project_id) - def test_get_with_name_duplicated(self): - sg_name = 'web_server' - expected_sg_id = '85cc3048-abc3-43cc-89b3-377341426ac5' - expected_sg = {'security_group': {'name': sg_name, - 'id': expected_sg_id, - 'tenant_id': self.context.project_id, - 'description': 'server', 'rules': []}} - self.mocked_client.show_security_group.return_value = expected_sg - - with mock.patch.object(neutronv20, 'find_resourceid_by_name_or_id', - return_value=expected_sg_id): - observed_sg = sg_api.get(self.context, name=sg_name) - expected_sg['security_group']['project_id'] = self.context.project_id - del expected_sg['security_group']['tenant_id'] - self.assertEqual(expected_sg['security_group'], observed_sg) - self.mocked_client.show_security_group.assert_called_once_with( - expected_sg_id) - - def test_get_with_invalid_name(self): - sg_name = 'invalid_name' - expected_sg_id = '85cc3048-abc3-43cc-89b3-377341426ac5' - self.mocked_client.show_security_group.side_effect = TypeError - - with mock.patch.object(neutronv20, 'find_resourceid_by_name_or_id', - return_value=expected_sg_id): - self.assertRaises(exception.SecurityGroupNotFound, - sg_api.get, self.context, name=sg_name) - self.mocked_client.show_security_group.assert_called_once_with( - expected_sg_id) - def test_create_security_group_with_bad_request(self): name = 'test-security-group' description = None @@ -228,6 +157,7 @@ class TestNeutronDriver(test.NoDBTestCase): body) def test_list_security_group_with_no_port_range_and_not_tcp_udp_icmp(self): + project_id = '0af70a4d22cf4652824ddc1f2435dd85' sg1 = {'description': 'default', 'id': '07f1362f-34f6-4136-819a-2dcde112269e', 'name': 'default', @@ -247,7 +177,7 @@ class TestNeutronDriver(test.NoDBTestCase): self.mocked_client.list_security_groups.return_value = ( {'security_groups': [sg1]}) - result = sg_api.list(self.context) + result = sg_api.list(self.context, project=project_id) expected = [{'rules': [{'from_port': -1, 'protocol': '51', 'to_port': -1, 'parent_group_id': '07f1362f-34f6-4136-819a-2dcde112269e', @@ -257,7 +187,8 @@ class TestNeutronDriver(test.NoDBTestCase): 'id': '07f1362f-34f6-4136-819a-2dcde112269e', 'name': 'default', 'description': 'default'}] self.assertEqual(expected, result) - self.mocked_client.list_security_groups.assert_called_once_with() + self.mocked_client.list_security_groups.assert_called_once_with( + tenant_id=project_id) def test_instances_security_group_bindings(self, detailed=False): server_id = 'c5a20e8d-c4b0-47cf-9dca-ebe4f758acb1'