From a7fbaba34cb2157403591b55f9ee8c9deca058e2 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Wed, 5 Jun 2019 09:12:05 +0200 Subject: [PATCH] Use Resource layer in cloud for SecurityGroups of server - add fetch_server_security_groups to the compute proxy - fix add/remove security_group of server to properly pass SG name and not ID according to docs - force usage of resource layer in cloud for respective methods Change-Id: I5c46169de436b9fe4df31a12f3a6246063a7d342 --- doc/source/user/proxies/compute.rst | 1 + openstack/cloud/_compute.py | 21 +- openstack/compute/v2/_proxy.py | 26 ++- openstack/compute/v2/server.py | 33 ++- openstack/tests/unit/cloud/test_meta.py | 197 +++++++++++------- .../tests/unit/cloud/test_security_groups.py | 10 +- openstack/tests/unit/compute/v2/test_proxy.py | 21 ++ .../tests/unit/compute/v2/test_server.py | 45 ++++ 8 files changed, 253 insertions(+), 101 deletions(-) diff --git a/doc/source/user/proxies/compute.rst b/doc/source/user/proxies/compute.rst index 0d29784f2..4ec84d13f 100644 --- a/doc/source/user/proxies/compute.rst +++ b/doc/source/user/proxies/compute.rst @@ -40,6 +40,7 @@ Network Actions .. automethod:: openstack.compute.v2._proxy.Proxy.remove_fixed_ip_from_server .. automethod:: openstack.compute.v2._proxy.Proxy.add_floating_ip_to_server .. automethod:: openstack.compute.v2._proxy.Proxy.remove_floating_ip_from_server + .. automethod:: openstack.compute.v2._proxy.Proxy.fetch_server_security_groups .. automethod:: openstack.compute.v2._proxy.Proxy.add_security_group_to_server .. automethod:: openstack.compute.v2._proxy.Proxy.remove_security_group_from_server diff --git a/openstack/cloud/_compute.py b/openstack/cloud/_compute.py index a883ad3c7..9a0d355f3 100644 --- a/openstack/cloud/_compute.py +++ b/openstack/cloud/_compute.py @@ -198,12 +198,11 @@ class ComputeCloudMixin(_normalize.Normalizer): if not self._has_secgroups(): return [] - data = proxy._json_response( - self.compute.get( - '/servers/{server_id}/os-security-groups'.format( - server_id=server['id']))) - return self._normalize_secgroups( - self._get_and_munchify('security_groups', data)) + server = self.compute.get_server(server) + + server.fetch_security_groups(self.compute) + + return self._normalize_secgroups(server.security_groups) def _get_server_security_groups(self, server, security_groups): if not self._has_secgroups(): @@ -255,9 +254,7 @@ class ComputeCloudMixin(_normalize.Normalizer): return False for sg in security_groups: - proxy._json_response(self.compute.post( - '/servers/%s/action' % server['id'], - json={'addSecurityGroup': {'name': sg.name}})) + self.compute.add_security_group_to_server(server, sg) return True @@ -283,11 +280,9 @@ class ComputeCloudMixin(_normalize.Normalizer): for sg in security_groups: try: - proxy._json_response(self.compute.post( - '/servers/%s/action' % server['id'], - json={'removeSecurityGroup': {'name': sg.name}})) + self.compute.remove_security_group_from_server(server, sg) - except exc.OpenStackCloudURINotFound: + except exceptions.ResourceNotFound: # NOTE(jamielennox): Is this ok? If we remove something that # isn't present should we just conclude job done or is that an # error? Nova returns ok if you try to add a group twice. diff --git a/openstack/compute/v2/_proxy.py b/openstack/compute/v2/_proxy.py index 0e1434ea4..0fb493383 100644 --- a/openstack/compute/v2/_proxy.py +++ b/openstack/compute/v2/_proxy.py @@ -25,6 +25,7 @@ from openstack.compute.v2 import server_interface as _server_interface from openstack.compute.v2 import server_ip from openstack.compute.v2 import service as _service from openstack.compute.v2 import volume_attachment as _volume_attachment +from openstack.network.v2 import security_group as _sg from openstack import proxy from openstack import resource @@ -636,26 +637,37 @@ class Proxy(proxy.Proxy): server = self._get_resource(_server.Server, server) server.create_image(self, name, metadata) + def fetch_server_security_groups(self, server): + """Fetch security groups with details for a server. + + :param server: Either the ID of a server or a + :class:`~openstack.compute.v2.server.Server` instance. + + :returns: updated :class:`~openstack.compute.v2.server.Server` instance + """ + server = self._get_resource(_server.Server, server) + return server.fetch_security_groups(self) + def add_security_group_to_server(self, server, security_group): """Add a security group to a server :param server: Either the ID of a server or a - :class:`~openstack.compute.v2.server.Server` instance. - :param security_group: Either the ID of a security group or a + :class:`~openstack.compute.v2.server.Server` instance. + :param security_group: Either the ID, Name of a security group or a :class:`~openstack.network.v2.security_group.SecurityGroup` instance. :returns: None """ server = self._get_resource(_server.Server, server) - security_group_id = resource.Resource._get_id(security_group) - server.add_security_group(self, security_group_id) + security_group = self._get_resource(_sg.SecurityGroup, security_group) + server.add_security_group(self, security_group.name) def remove_security_group_from_server(self, server, security_group): """Remove a security group from a server :param server: Either the ID of a server or a - :class:`~openstack.compute.v2.server.Server` instance. + :class:`~openstack.compute.v2.server.Server` instance. :param security_group: Either the ID of a security group or a :class:`~openstack.network.v2.security_group.SecurityGroup` instance. @@ -663,8 +675,8 @@ class Proxy(proxy.Proxy): :returns: None """ server = self._get_resource(_server.Server, server) - security_group_id = resource.Resource._get_id(security_group) - server.remove_security_group(self, security_group_id) + security_group = self._get_resource(_sg.SecurityGroup, security_group) + server.remove_security_group(self, security_group.name) def add_fixed_ip_to_server(self, server, network_id): """Adds a fixed IP address to a server instance. diff --git a/openstack/compute/v2/server.py b/openstack/compute/v2/server.py index d7198f493..99001e587 100644 --- a/openstack/compute/v2/server.py +++ b/openstack/compute/v2/server.py @@ -12,6 +12,7 @@ from openstack.compute.v2 import metadata from openstack.image.v2 import image +from openstack import exceptions from openstack import resource from openstack import utils @@ -165,7 +166,8 @@ class Server(resource.Resource, metadata.MetadataMixin, resource.TagMixin): scheduler_hints = resource.Body('OS-SCH-HNT:scheduler_hints', type=dict) #: A list of applicable security groups. Each group contains keys for #: description, name, id, and rules. - security_groups = resource.Body('security_groups') + security_groups = resource.Body('security_groups', + type=list, list_type=dict) #: The UUIDs of the server groups to which the server belongs. #: Currently this can contain at most one entry. server_groups = resource.Body('server_groups', type=list, list_type=dict) @@ -305,12 +307,12 @@ class Server(resource.Resource, metadata.MetadataMixin, resource.TagMixin): body = {'createImage': action} self._action(session, body) - def add_security_group(self, session, security_group): - body = {"addSecurityGroup": {"name": security_group}} + def add_security_group(self, session, security_group_name): + body = {"addSecurityGroup": {"name": security_group_name}} self._action(session, body) - def remove_security_group(self, session, security_group): - body = {"removeSecurityGroup": {"name": security_group}} + def remove_security_group(self, session, security_group_name): + body = {"removeSecurityGroup": {"name": security_group_name}} self._action(session, body) def reset_state(self, session, state): @@ -494,5 +496,26 @@ class Server(resource.Resource, metadata.MetadataMixin, resource.TagMixin): self._action( session, {'os-migrateLive': body}, microversion=microversion) + def fetch_security_groups(self, session): + """Fetch security groups of a server. + + :returns: Updated Server instance. + + """ + url = utils.urljoin(Server.base_path, self.id, 'os-security-groups') + + response = session.get(url) + + exceptions.raise_from_response(response) + + try: + data = response.json() + if 'security_groups' in data: + self.security_groups = data['security_groups'] + except ValueError: + pass + + return self + ServerDetail = Server diff --git a/openstack/tests/unit/cloud/test_meta.py b/openstack/tests/unit/cloud/test_meta.py index c1ee49946..484767465 100644 --- a/openstack/tests/unit/cloud/test_meta.py +++ b/openstack/tests/unit/cloud/test_meta.py @@ -367,6 +367,20 @@ class TestMeta(base.TestCase): mock_get_volumes.return_value = [] mock_has_service.return_value = True + fake_server = fakes.make_fake_server( + server_id='test-id', name='test-name', status='ACTIVE', + flavor={u'id': u'1'}, + image={ + 'name': u'cirros-0.3.4-x86_64-uec', + u'id': u'f93d000b-7c29-4489-b375-3641a1758fe1'}, + addresses={u'test_pnztt_net': [{ + u'OS-EXT-IPS:type': u'fixed', + u'addr': PRIVATE_V4, + u'version': 4, + u'OS-EXT-IPS-MAC:mac_addr': u'fa:16:3e:ae:7d:42' + }]} + ) + self.register_uris([ dict(method='GET', uri=('https://network.example.com/v2.0/ports.json?' @@ -395,25 +409,19 @@ class TestMeta(base.TestCase): uri='https://network.example.com/v2.0/subnets.json', json={'subnets': SUBNETS_WITH_NAT}), + self.get_nova_discovery_mock_dict(), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', + append=['servers', fake_server['id']]), + json=fake_server), dict(method='GET', uri='{endpoint}/servers/test-id/os-security-groups'.format( endpoint=fakes.COMPUTE_ENDPOINT), json={'security_groups': []}) ]) - srv = self.cloud.get_openstack_vars(fakes.make_fake_server( - server_id='test-id', name='test-name', status='ACTIVE', - flavor={u'id': u'1'}, - image={ - 'name': u'cirros-0.3.4-x86_64-uec', - u'id': u'f93d000b-7c29-4489-b375-3641a1758fe1'}, - addresses={u'test_pnztt_net': [{ - u'OS-EXT-IPS:type': u'fixed', - u'addr': PRIVATE_V4, - u'version': 4, - u'OS-EXT-IPS-MAC:mac_addr': u'fa:16:3e:ae:7d:42' - }]} - )) + srv = self.cloud.get_openstack_vars(fake_server) self.assertEqual(PRIVATE_V4, srv['private_v4']) self.assert_calls() @@ -431,6 +439,20 @@ class TestMeta(base.TestCase): mock_get_flavor_name.return_value = 'm1.tiny' mock_get_volumes.return_value = [] + fake_server = fakes.make_fake_server( + server_id='test-id', name='test-name', status='ACTIVE', + flavor={u'id': u'1'}, + image={ + 'name': u'cirros-0.3.4-x86_64-uec', + u'id': u'f93d000b-7c29-4489-b375-3641a1758fe1'}, + addresses={u'test_pnztt_net': [{ + u'OS-EXT-IPS:type': u'fixed', + u'addr': PRIVATE_V4, + u'version': 4, + u'OS-EXT-IPS-MAC:mac_addr': u'fa:16:3e:ae:7d:42' + }]} + ) + self.register_uris([ dict(method='GET', uri='https://network.example.com/v2.0/networks.json', @@ -445,25 +467,19 @@ class TestMeta(base.TestCase): dict(method='GET', uri='https://network.example.com/v2.0/subnets.json', json={'subnets': SUBNETS_WITH_NAT}), + self.get_nova_discovery_mock_dict(), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', + append=['servers', fake_server['id']]), + json=fake_server), dict(method='GET', uri='{endpoint}/servers/test-id/os-security-groups'.format( endpoint=fakes.COMPUTE_ENDPOINT), json={'security_groups': []}) ]) - srv = self.cloud.get_openstack_vars(fakes.make_fake_server( - server_id='test-id', name='test-name', status='ACTIVE', - flavor={u'id': u'1'}, - image={ - 'name': u'cirros-0.3.4-x86_64-uec', - u'id': u'f93d000b-7c29-4489-b375-3641a1758fe1'}, - addresses={u'test_pnztt_net': [{ - u'OS-EXT-IPS:type': u'fixed', - u'addr': PRIVATE_V4, - u'version': 4, - u'OS-EXT-IPS-MAC:mac_addr': u'fa:16:3e:ae:7d:42' - }]} - )) + srv = self.cloud.get_openstack_vars(fake_server) self.assertEqual(PRIVATE_V4, srv['private_v4']) self.assert_calls() @@ -479,6 +495,19 @@ class TestMeta(base.TestCase): mock_get_image_name.return_value = 'cirros-0.3.4-x86_64-uec' mock_get_flavor_name.return_value = 'm1.tiny' mock_get_volumes.return_value = [] + + fake_server = fakes.make_fake_server( + server_id='test-id', name='test-name', status='ACTIVE', + flavor={u'id': u'1'}, + image={ + 'name': u'cirros-0.3.4-x86_64-uec', + u'id': u'f93d000b-7c29-4489-b375-3641a1758fe1'}, + addresses={u'test_pnztt_net': [{ + u'addr': PRIVATE_V4, + u'version': 4, + }]} + ) + self.register_uris([ dict(method='GET', uri='https://network.example.com/v2.0/networks.json', @@ -495,23 +524,19 @@ class TestMeta(base.TestCase): dict(method='GET', uri='https://network.example.com/v2.0/subnets.json', json={'subnets': SUBNETS_WITH_NAT}), + self.get_nova_discovery_mock_dict(), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', + append=['servers', fake_server['id']]), + json=fake_server), dict(method='GET', uri='{endpoint}/servers/test-id/os-security-groups'.format( endpoint=fakes.COMPUTE_ENDPOINT), json={'security_groups': []}) ]) - srv = self.cloud.get_openstack_vars(fakes.make_fake_server( - server_id='test-id', name='test-name', status='ACTIVE', - flavor={u'id': u'1'}, - image={ - 'name': u'cirros-0.3.4-x86_64-uec', - u'id': u'f93d000b-7c29-4489-b375-3641a1758fe1'}, - addresses={u'test_pnztt_net': [{ - u'addr': PRIVATE_V4, - u'version': 4, - }]} - )) + srv = self.cloud.get_openstack_vars(fake_server) self.assertEqual(PRIVATE_V4, srv['private_v4']) self.assert_calls() @@ -529,7 +554,21 @@ class TestMeta(base.TestCase): mock_get_volumes.return_value = [] mock_has_service.return_value = True + fake_server = fakes.make_fake_server( + server_id='test-id', name='test-name', status='ACTIVE', + flavor={u'id': u'1'}, + image={ + 'name': u'cirros-0.3.4-x86_64-uec', + u'id': u'f93d000b-7c29-4489-b375-3641a1758fe1'}, + addresses={u'test_pnztt_net': [{ + u'addr': PRIVATE_V4, + u'version': 4, + 'OS-EXT-IPS-MAC:mac_addr': 'fa:16:3e:ae:7d:42', + }]} + ) + self.register_uris([ + # self.get_nova_discovery_mock_dict(), dict(method='GET', uri=('https://network.example.com/v2.0/ports.json?' 'device_id=test-id'), @@ -563,24 +602,19 @@ class TestMeta(base.TestCase): dict(method='GET', uri='https://network.example.com/v2.0/subnets.json', json={'subnets': SUBNETS_WITH_NAT}), + self.get_nova_discovery_mock_dict(), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', + append=['servers', fake_server['id']]), + json=fake_server), dict(method='GET', uri='{endpoint}/servers/test-id/os-security-groups'.format( endpoint=fakes.COMPUTE_ENDPOINT), json={'security_groups': []}) ]) - srv = self.cloud.get_openstack_vars(fakes.make_fake_server( - server_id='test-id', name='test-name', status='ACTIVE', - flavor={u'id': u'1'}, - image={ - 'name': u'cirros-0.3.4-x86_64-uec', - u'id': u'f93d000b-7c29-4489-b375-3641a1758fe1'}, - addresses={u'test_pnztt_net': [{ - u'addr': PRIVATE_V4, - u'version': 4, - 'OS-EXT-IPS-MAC:mac_addr': 'fa:16:3e:ae:7d:42', - }]} - )) + srv = self.cloud.get_openstack_vars(fake_server) self.assertEqual(PUBLIC_V4, srv['public_v4']) self.assert_calls() @@ -598,15 +632,7 @@ class TestMeta(base.TestCase): mock_get_image_name.return_value = 'cirros-0.3.4-x86_64-uec' mock_get_flavor_name.return_value = 'm1.tiny' mock_get_volumes.return_value = [] - - self.register_uris([ - dict(method='GET', - uri='{endpoint}/servers/test-id/os-security-groups'.format( - endpoint=fakes.COMPUTE_ENDPOINT), - json={'security_groups': []}) - ]) - - srv = self.cloud.get_openstack_vars(fakes.make_fake_server( + fake_server = fakes.make_fake_server( server_id='test-id', name='test-name', status='ACTIVE', flavor={u'id': u'1'}, image={ @@ -625,7 +651,22 @@ class TestMeta(base.TestCase): 'version': 6 }] } - )) + ) + + self.register_uris([ + self.get_nova_discovery_mock_dict(), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', + append=['servers', fake_server['id']]), + json=fake_server), + dict(method='GET', + uri='{endpoint}/servers/test-id/os-security-groups'.format( + endpoint=fakes.COMPUTE_ENDPOINT), + json={'security_groups': []}) + ]) + + srv = self.cloud.get_openstack_vars(fake_server) self.assertEqual("10.223.160.141", srv['private_v4']) self.assertEqual("104.130.246.91", srv['public_v4']) @@ -652,20 +693,7 @@ class TestMeta(base.TestCase): mock_get_flavor_name.return_value = 'm1.tiny' mock_get_volumes.return_value = [] - self.register_uris([ - dict(method='GET', - uri='https://network.example.com/v2.0/networks.json', - json={'networks': OSIC_NETWORKS}), - dict(method='GET', - uri='https://network.example.com/v2.0/subnets.json', - json={'subnets': OSIC_SUBNETS}), - dict(method='GET', - uri='{endpoint}/servers/test-id/os-security-groups'.format( - endpoint=fakes.COMPUTE_ENDPOINT), - json={'security_groups': []}) - ]) - - srv = self.cloud.get_openstack_vars(fakes.make_fake_server( + fake_server = fakes.make_fake_server( server_id='test-id', name='test-name', status='ACTIVE', flavor={u'id': u'1'}, image={ @@ -684,7 +712,28 @@ class TestMeta(base.TestCase): 'version': 6 }] } - )) + ) + + self.register_uris([ + dict(method='GET', + uri='https://network.example.com/v2.0/networks.json', + json={'networks': OSIC_NETWORKS}), + dict(method='GET', + uri='https://network.example.com/v2.0/subnets.json', + json={'subnets': OSIC_SUBNETS}), + self.get_nova_discovery_mock_dict(), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', + append=['servers', fake_server['id']]), + json=fake_server), + dict(method='GET', + uri='{endpoint}/servers/test-id/os-security-groups'.format( + endpoint=fakes.COMPUTE_ENDPOINT), + json={'security_groups': []}) + ]) + + srv = self.cloud.get_openstack_vars(fake_server) self.assertEqual("10.223.160.141", srv['private_v4']) self.assertEqual("104.130.246.91", srv['public_v4']) diff --git a/openstack/tests/unit/cloud/test_security_groups.py b/openstack/tests/unit/cloud/test_security_groups.py index 3a27e70e8..d17214b38 100644 --- a/openstack/tests/unit/cloud/test_security_groups.py +++ b/openstack/tests/unit/cloud/test_security_groups.py @@ -548,14 +548,20 @@ class TestSecurityGroups(base.TestCase): def test_list_server_security_groups_nova(self): self.has_neutron = False - server = dict(id='server_id') + server = fakes.make_fake_server('1234', 'server-name', 'ACTIVE') self.register_uris([ + self.get_nova_discovery_mock_dict(), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', + append=['servers', server['id']]), + json=server), dict( method='GET', uri='{endpoint}/servers/{id}/os-security-groups'.format( endpoint=fakes.COMPUTE_ENDPOINT, - id='server_id'), + id=server['id']), json={'security_groups': [nova_grp_dict]}), ]) groups = self.cloud.list_server_security_groups(server) diff --git a/openstack/tests/unit/compute/v2/test_proxy.py b/openstack/tests/unit/compute/v2/test_proxy.py index b9b8694f3..17f282d83 100644 --- a/openstack/tests/unit/compute/v2/test_proxy.py +++ b/openstack/tests/unit/compute/v2/test_proxy.py @@ -515,3 +515,24 @@ class TestComputeProxy(test_proxy_base.TestProxyBase): method_args=["value", "host1", False], expected_args=["host1"], expected_kwargs={'force': False, 'block_migration': None}) + + def test_fetch_security_groups(self): + self._verify( + 'openstack.compute.v2.server.Server.fetch_security_groups', + self.proxy.fetch_server_security_groups, + method_args=["value"], + expected_args=[]) + + def test_add_security_groups(self): + self._verify( + 'openstack.compute.v2.server.Server.add_security_group', + self.proxy.add_security_group_to_server, + method_args=["value", {'id': 'id', 'name': 'sg'}], + expected_args=['sg']) + + def test_remove_security_groups(self): + self._verify( + 'openstack.compute.v2.server.Server.remove_security_group', + self.proxy.remove_security_group_from_server, + method_args=["value", {'id': 'id', 'name': 'sg'}], + expected_args=['sg']) diff --git a/openstack/tests/unit/compute/v2/test_server.py b/openstack/tests/unit/compute/v2/test_server.py index b00601cd5..854867c37 100644 --- a/openstack/tests/unit/compute/v2/test_server.py +++ b/openstack/tests/unit/compute/v2/test_server.py @@ -883,3 +883,48 @@ class TestServer(base.TestCase): headers = {'Accept': ''} self.sess.post.assert_called_with( url, json=body, headers=headers, microversion='2.30') + + def test_get_security_groups(self): + sot = server.Server(**EXAMPLE) + + response = mock.Mock() + + sgs = [{ + 'description': 'default', + 'id': 1, + 'name': 'default', + 'rules': [ + { + 'direction': 'egress', + 'ethertype': 'IPv6', + 'id': '3c0e45ff-adaf-4124-b083-bf390e5482ff', + 'port_range_max': None, + 'port_range_min': None, + 'protocol': None, + 'remote_group_id': None, + 'remote_ip_prefix': None, + 'security_group_id': '1', + 'project_id': 'e4f50856753b4dc6afee5fa6b9b6c550', + 'revision_number': 1, + 'tags': ['tag1,tag2'], + 'tenant_id': 'e4f50856753b4dc6afee5fa6b9b6c550', + 'created_at': '2018-03-19T19:16:56Z', + 'updated_at': '2018-03-19T19:16:56Z', + 'description': '' + } + ], + 'tenant_id': 'e4f50856753b4dc6afee5fa6b9b6c550' + }] + + response.status_code = 200 + response.json.return_value = { + 'security_groups': sgs + } + self.sess.get.return_value = response + + sot.fetch_security_groups(self.sess) + + url = 'servers/IDENTIFIER/os-security-groups' + self.sess.get.assert_called_with(url) + + self.assertEqual(sot.security_groups, sgs)