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

@@ -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
 

openstack/cloud/_compute.py

@@ -198,12 +198,11 @@ class ComputeCloudMixin(_normalize.Normalizer):
         if not self._has_secgroups():
             return []
 
-        data = proxy._json_response(
+        server = self.compute.get_server(server)
-            self.compute.get(
+
-                '/servers/{server_id}/os-security-groups'.format(
+        server.fetch_security_groups(self.compute)
-                    server_id=server['id'])))
+
-        return self._normalize_secgroups(
+        return self._normalize_secgroups(server.security_groups)
-            self._get_and_munchify('security_groups', data))
 
     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(
+            self.compute.add_security_group_to_server(server, sg)
-                '/servers/%s/action' % server['id'],
-                json={'addSecurityGroup': {'name': sg.name}}))
 
         return True
 
@@ -283,11 +280,9 @@ class ComputeCloudMixin(_normalize.Normalizer):
 
         for sg in security_groups:
             try:
-                proxy._json_response(self.compute.post(
+                self.compute.remove_security_group_from_server(server, sg)
-                    '/servers/%s/action' % server['id'],
-                    json={'removeSecurityGroup': {'name': sg.name}}))
 
-            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.

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.
+            :class:`~openstack.compute.v2.server.Server` instance.
-        :param security_group: Either the ID of a security group or a
+        :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)
+        security_group = self._get_resource(_sg.SecurityGroup, security_group)
-        server.add_security_group(self, security_group_id)
+        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)
+        security_group = self._get_resource(_sg.SecurityGroup, security_group)
-        server.remove_security_group(self, security_group_id)
+        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.

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):
+    def add_security_group(self, session, security_group_name):
-        body = {"addSecurityGroup": {"name": security_group}}
+        body = {"addSecurityGroup": {"name": security_group_name}}
         self._action(session, body)
 
-    def remove_security_group(self, session, security_group):
+    def remove_security_group(self, session, security_group_name):
-        body = {"removeSecurityGroup": {"name": security_group}}
+        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

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(
+        srv = self.cloud.get_openstack_vars(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.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(
+        srv = self.cloud.get_openstack_vars(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.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(
+        srv = self.cloud.get_openstack_vars(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.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(
+        srv = self.cloud.get_openstack_vars(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.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 = []
-
+        fake_server = fakes.make_fake_server(
-        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(
             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([
+        fake_server = fakes.make_fake_server(
-            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(
             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'])

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)

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'])

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)