From 945f3fe95a5a00993dfa1a549b57279696d1606e Mon Sep 17 00:00:00 2001 From: huangtianhua Date: Thu, 24 Jul 2014 15:52:11 +0800 Subject: [PATCH] Match tenant_id when name ambiguity in get_secgroup_uuids() Neutron allows securityGroup creation with same name between one/more users, and the admin roles can get/use the other users' securityGroup. So if there are several securityGroups return, we should match the one which is belongs to the stack owner, and then to raise 'PhysicalResourceNameAmbiguity' exception if can't match. Change-Id: Icb7eae35ba40a142a4d398b7c9d58c8483b9588e Closes-Bug: #1348064 --- heat/engine/clients/os/neutron.py | 35 +++++++++ heat/engine/resources/instance.py | 5 +- heat/engine/resources/network_interface.py | 5 +- heat/engine/resources/neutron/neutron.py | 26 +++++-- heat/engine/resources/neutron/port.py | 4 +- heat/tests/test_instance.py | 6 +- heat/tests/test_neutron.py | 90 ++++++++++++++++++++++ heat/tests/test_neutron_client.py | 73 +++++++++++++++++- 8 files changed, 229 insertions(+), 15 deletions(-) diff --git a/heat/engine/clients/os/neutron.py b/heat/engine/clients/os/neutron.py index 0f6e77daf..9f0e188b8 100644 --- a/heat/engine/clients/os/neutron.py +++ b/heat/engine/clients/os/neutron.py @@ -15,8 +15,10 @@ from neutronclient.common import exceptions from neutronclient.neutron import v2_0 as neutronV20 from neutronclient.v2_0 import client as nc +from heat.common import exception from heat.engine.clients import client_plugin from heat.engine import constraints +from heat.openstack.common import uuidutils class NeutronClientPlugin(client_plugin.ClientPlugin): @@ -81,6 +83,39 @@ class NeutronClientPlugin(client_plugin.ClientPlugin): subnet_info = self.client().show_subnet(subnet_id) return subnet_info['subnet']['network_id'] + def get_secgroup_uuids(self, security_groups): + ''' + Returns a list of security group UUIDs. + Args: + security_groups: List of security group names or UUIDs + ''' + seclist = [] + all_groups = None + for sg in security_groups: + if uuidutils.is_uuid_like(sg): + seclist.append(sg) + else: + if not all_groups: + response = self.client().list_security_groups() + all_groups = response['security_groups'] + same_name_groups = [g for g in all_groups if g['name'] == sg] + groups = [g['id'] for g in same_name_groups] + if len(groups) == 0: + raise exception.PhysicalResourceNotFound(resource_id=sg) + elif len(groups) == 1: + seclist.append(groups[0]) + else: + # for admin roles, can get the other users' + # securityGroups, so we should match the tenant_id with + # the groups, and return the own one + own_groups = [g['id'] for g in same_name_groups + if g['tenant_id'] == self.context.tenant_id] + if len(own_groups) == 1: + seclist.append(own_groups[0]) + else: + raise exception.PhysicalResourceNameAmbiguity(name=sg) + return seclist + class NetworkConstraint(constraints.BaseCustomConstraint): diff --git a/heat/engine/resources/instance.py b/heat/engine/resources/instance.py index 5db2f4351..d7be322dd 100644 --- a/heat/engine/resources/instance.py +++ b/heat/engine/resources/instance.py @@ -21,7 +21,6 @@ from heat.engine import attributes from heat.engine import constraints from heat.engine import properties from heat.engine import resource -from heat.engine.resources.neutron import neutron from heat.engine.resources import volume from heat.engine import scheduler from heat.engine import signal_responder @@ -483,8 +482,8 @@ class Instance(resource.Resource): if security_groups: props['security_groups'] = \ - neutron.NeutronResource.get_secgroup_uuids( - security_groups, self.neutron()) + self.client_plugin('neutron').get_secgroup_uuids( + security_groups) port = neutronclient.create_port({'port': props})['port'] nics = [{'port-id': port['id']}] diff --git a/heat/engine/resources/network_interface.py b/heat/engine/resources/network_interface.py index 5e71ab98c..4618e84dc 100644 --- a/heat/engine/resources/network_interface.py +++ b/heat/engine/resources/network_interface.py @@ -14,7 +14,6 @@ from heat.engine import attributes from heat.engine import properties from heat.engine import resource -from heat.engine.resources.neutron import neutron class NetworkInterface(resource.Resource): @@ -118,8 +117,8 @@ class NetworkInterface(resource.Resource): } if self.properties[self.GROUP_SET]: - sgs = neutron.NeutronResource.get_secgroup_uuids( - self.properties.get(self.GROUP_SET), self.neutron()) + sgs = self.client_plugin().get_secgroup_uuids( + self.properties.get(self.GROUP_SET)) props['security_groups'] = sgs port = client.create_port({'port': props})['port'] self.resource_id_set(port['id']) diff --git a/heat/engine/resources/neutron/neutron.py b/heat/engine/resources/neutron/neutron.py index 40168133b..e46e2ff73 100644 --- a/heat/engine/resources/neutron/neutron.py +++ b/heat/engine/resources/neutron/neutron.py @@ -11,6 +11,8 @@ # License for the specific language governing permissions and limitations # under the License. +import warnings + from heat.common import exception from heat.engine import resource from heat.engine import scheduler @@ -140,13 +142,17 @@ class NeutronResource(resource.Resource): return unicode(self.resource_id) @staticmethod - def get_secgroup_uuids(security_groups, client): + def get_secgroup_uuids(security_groups, client, tenant_id): ''' Returns a list of security group UUIDs. Args: security_groups: List of security group names or UUIDs client: reference to neutronclient + tenant_id: the tenant id to match the security_groups ''' + warnings.warn('neutron.NeutronResource.get_secgroup_uuids is ' + 'deprecated. Use ' + 'self.client_plugin("neutron").get_secgroup_uuids') seclist = [] all_groups = None for sg in security_groups: @@ -156,12 +162,22 @@ class NeutronResource(resource.Resource): if not all_groups: response = client.list_security_groups() all_groups = response['security_groups'] - groups = [g['id'] for g in all_groups if g['name'] == sg] + same_name_groups = [g for g in all_groups if g['name'] == sg] + groups = [g['id'] for g in same_name_groups] if len(groups) == 0: raise exception.PhysicalResourceNotFound(resource_id=sg) - if len(groups) > 1: - raise exception.PhysicalResourceNameAmbiguity(name=sg) - seclist.append(groups[0]) + elif len(groups) == 1: + seclist.append(groups[0]) + else: + # for admin roles, can get the other users' + # securityGroups, so we should match the tenant_id with + # the groups, and return the own one + own_groups = [g['id'] for g in same_name_groups + if g['tenant_id'] == tenant_id] + if len(own_groups) == 1: + seclist.append(own_groups[0]) + else: + raise exception.PhysicalResourceNameAmbiguity(name=sg) return seclist def _delete_task(self): diff --git a/heat/engine/resources/neutron/port.py b/heat/engine/resources/neutron/port.py index f2363c185..d9fe74092 100644 --- a/heat/engine/resources/neutron/port.py +++ b/heat/engine/resources/neutron/port.py @@ -247,8 +247,8 @@ class Port(neutron.NeutronResource): del pair[self.ALLOWED_ADDRESS_PAIR_MAC_ADDRESS] if props.get(self.SECURITY_GROUPS): - props[self.SECURITY_GROUPS] = self.get_secgroup_uuids( - props.get(self.SECURITY_GROUPS), self.neutron()) + props[self.SECURITY_GROUPS] = self.client_plugin().\ + get_secgroup_uuids(props.get(self.SECURITY_GROUPS)) else: props.pop(self.SECURITY_GROUPS, None) diff --git a/heat/tests/test_instance.py b/heat/tests/test_instance.py index 3b5da0aed..bfaa48b8f 100644 --- a/heat/tests/test_instance.py +++ b/heat/tests/test_instance.py @@ -1281,7 +1281,7 @@ class InstancesTest(HeatTestCase): neutronclient.Client.create_port( {'port': props}).MultipleTimes().AndReturn( {'port': {'id': 'fake_port_id'}}) - + self.stub_keystoneclient() self.m.ReplayAll() if get_secgroup_raises: @@ -1302,24 +1302,28 @@ class InstancesTest(HeatTestCase): fake_groups_list = { 'security_groups': [ { + 'tenant_id': 'test_tenant_id', 'id': '0389f747-7785-4757-b7bb-2ab07e4b09c3', 'name': 'security_group_1', 'security_group_rules': [], 'description': 'no protocol' }, { + 'tenant_id': 'test_tenant_id', 'id': '384ccd91-447c-4d83-832c-06974a7d3d05', 'name': 'security_group_2', 'security_group_rules': [], 'description': 'no protocol' }, { + 'tenant_id': 'test_tenant_id', 'id': 'e91a0007-06a6-4a4a-8edb-1d91315eb0ef', 'name': 'duplicate_group_name', 'security_group_rules': [], 'description': 'no protocol' }, { + 'tenant_id': 'test_tenant_id', 'id': '8be37f3c-176d-4826-aa17-77d1d9df7b2e', 'name': 'duplicate_group_name', 'security_group_rules': [], diff --git a/heat/tests/test_neutron.py b/heat/tests/test_neutron.py index aaa5ade52..53235c69a 100644 --- a/heat/tests/test_neutron.py +++ b/heat/tests/test_neutron.py @@ -537,6 +537,96 @@ class NeutronTest(HeatTestCase): self.assertRaises(KeyError, res._resolve_attribute, 'attr3') self.assertIsNone(res._resolve_attribute('attr2')) + def test_get_secgroup_uuids(self): + # test get_secgroup_uuids with uuid + security_groups = ['b62c3079-6946-44f5-a67b-6b9091884d4f', + '9887157c-d092-40f5-b547-6361915fce7d'] + self.assertEqual(security_groups, + qr.get_secgroup_uuids(security_groups, None, None)) + # test get_secgroup_uuids with name + secgroups = ['security_group_1'] + expected_groups = ['0389f747-7785-4757-b7bb-2ab07e4b09c3'] + ctx = utils.dummy_context( + tenant_id='dc4b074874244f7693dd65583733a758') + fake_groups_list = { + 'security_groups': [ + { + 'tenant_id': 'dc4b074874244f7693dd65583733a758', + 'id': '0389f747-7785-4757-b7bb-2ab07e4b09c3', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + } + ] + } + nclient = neutronclient.Client() + self.m.StubOutWithMock(neutronclient.Client, 'list_security_groups') + neutronclient.Client.list_security_groups().AndReturn( + fake_groups_list) + self.m.ReplayAll() + self.assertEqual(expected_groups, + qr.get_secgroup_uuids(secgroups, nclient, + ctx.tenant_id)) + self.m.VerifyAll() + self.m.UnsetStubs() + # test there are two securityGroups with same name, but there is + # one belongs to the tenant + fake_groups_list = { + 'security_groups': [ + { + 'tenant_id': 'dc4b074874244f7693dd65583733a758', + 'id': '0389f747-7785-4757-b7bb-2ab07e4b09c3', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + }, + { + 'tenant_id': '64395a8e5beb4930a18245f76a5b1570', + 'id': '384ccd91-447c-4d83-832c-06974a7d3d05', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + } + ] + } + self.m.StubOutWithMock(neutronclient.Client, 'list_security_groups') + neutronclient.Client.list_security_groups().AndReturn( + fake_groups_list) + self.m.ReplayAll() + self.assertEqual(expected_groups, + qr.get_secgroup_uuids(secgroups, nclient, + ctx.tenant_id)) + self.m.VerifyAll() + self.m.UnsetStubs() + # test there are two securityGroups with same name, and the two + # all belong to the tenant + fake_groups_list = { + 'security_groups': [ + { + 'tenant_id': 'dc4b074874244f7693dd65583733a758', + 'id': '0389f747-7785-4757-b7bb-2ab07e4b09c3', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + }, + { + 'tenant_id': 'dc4b074874244f7693dd65583733a758', + 'id': '384ccd91-447c-4d83-832c-06974a7d3d05', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + } + ] + } + self.m.StubOutWithMock(neutronclient.Client, 'list_security_groups') + neutronclient.Client.list_security_groups().AndReturn(fake_groups_list) + self.m.ReplayAll() + self.assertRaises(exception.PhysicalResourceNameAmbiguity, + qr.get_secgroup_uuids, + secgroups, nclient, ctx.tenant_id) + self.m.VerifyAll() + self.m.UnsetStubs() + class NeutronNetTest(HeatTestCase): diff --git a/heat/tests/test_neutron_client.py b/heat/tests/test_neutron_client.py index 8beebcc74..0effedd2b 100644 --- a/heat/tests/test_neutron_client.py +++ b/heat/tests/test_neutron_client.py @@ -13,7 +13,7 @@ import mock - +from heat.common import exception from heat.engine.clients.os import neutron from heat.tests.common import HeatTestCase from heat.tests import utils @@ -77,3 +77,74 @@ class NeutronClientPluginTests(NeutronClientPluginTestCase): # in this case find_resourceid_by_name_or_id is not called self.mock_find.assert_called_once_with(self.neutron_client, 'subnet', 'test_subnet') + + def test_get_secgroup_uuids(self): + # test get from uuids + sgs_uuid = ['b62c3079-6946-44f5-a67b-6b9091884d4f', + '9887157c-d092-40f5-b547-6361915fce7d'] + + sgs_list = self.neutron_plugin.get_secgroup_uuids(sgs_uuid) + self.assertEqual(sgs_list, sgs_uuid) + # test get from name, return only one + sgs_non_uuid = ['security_group_1'] + expected_groups = ['0389f747-7785-4757-b7bb-2ab07e4b09c3'] + fake_list = { + 'security_groups': [ + { + 'tenant_id': 'test_tenant_id', + 'id': '0389f747-7785-4757-b7bb-2ab07e4b09c3', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + } + ] + } + self.neutron_client.list_security_groups.return_value = fake_list + self.assertEqual(expected_groups, + self.neutron_plugin.get_secgroup_uuids(sgs_non_uuid)) + # test only one belong to the tenant + fake_list = { + 'security_groups': [ + { + 'tenant_id': 'test_tenant_id', + 'id': '0389f747-7785-4757-b7bb-2ab07e4b09c3', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + }, + { + 'tenant_id': 'not_test_tenant_id', + 'id': '384ccd91-447c-4d83-832c-06974a7d3d05', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + } + ] + } + self.neutron_client.list_security_groups.return_value = fake_list + self.assertEqual(expected_groups, + self.neutron_plugin.get_secgroup_uuids(sgs_non_uuid)) + # test there are two securityGroups with same name, and the two + # all belong to the tenant + fake_list = { + 'security_groups': [ + { + 'tenant_id': 'test_tenant_id', + 'id': '0389f747-7785-4757-b7bb-2ab07e4b09c3', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + }, + { + 'tenant_id': 'test_tenant_id', + 'id': '384ccd91-447c-4d83-832c-06974a7d3d05', + 'name': 'security_group_1', + 'security_group_rules': [], + 'description': 'no protocol' + } + ] + } + self.neutron_client.list_security_groups.return_value = fake_list + self.assertRaises(exception.PhysicalResourceNameAmbiguity, + self.neutron_plugin.get_secgroup_uuids, + sgs_non_uuid)