diff --git a/nova/api/openstack/compute/routes.py b/nova/api/openstack/compute/routes.py index 835b4099a5..7d30fd8836 100644 --- a/nova/api/openstack/compute/routes.py +++ b/nova/api/openstack/compute/routes.py @@ -269,7 +269,6 @@ server_controller = functools.partial(_create_controller, extended_status.ExtendedStatusController, extended_volumes.ExtendedVolumesController, hide_server_addresses.Controller, - security_groups.SecurityGroupsOutputController, ], [ admin_actions.AdminActionsController, diff --git a/nova/api/openstack/compute/security_groups.py b/nova/api/openstack/compute/security_groups.py index 12c3588887..aea0cb872f 100644 --- a/nova/api/openstack/compute/security_groups.py +++ b/nova/api/openstack/compute/security_groups.py @@ -16,7 +16,6 @@ """The security groups extension.""" from oslo_log import log as logging -from oslo_serialization import jsonutils from webob import exc from nova.api.openstack.api_version_request \ @@ -35,7 +34,6 @@ from nova.virt import netutils LOG = logging.getLogger(__name__) -ATTRIBUTE_NAME = 'security_groups' SG_NOT_FOUND = object() @@ -474,66 +472,3 @@ class SecurityGroupActionController(wsgi.Controller): raise exc.HTTPConflict(explanation=exp.format_message()) except exception.SecurityGroupNotExistsForInstance as exp: raise exc.HTTPBadRequest(explanation=exp.format_message()) - - -class SecurityGroupsOutputController(wsgi.Controller): - def __init__(self, *args, **kwargs): - super(SecurityGroupsOutputController, self).__init__(*args, **kwargs) - self.compute_api = compute.API() - self.security_group_api = ( - openstack_driver.get_openstack_security_group_driver()) - - def _extend_servers(self, req, servers): - # TODO(arosen) this function should be refactored to reduce duplicate - # code and use get_instance_security_groups instead of get_db_instance. - if not len(servers): - return - key = "security_groups" - context = req.environ['nova.context'] - if not openstack_driver.is_neutron_security_groups(): - for server in servers: - instance = req.get_db_instance(server['id']) - groups = instance.get(key) - if groups: - server[ATTRIBUTE_NAME] = [{"name": group.name} - for group in groups] - else: - # If method is a POST we get the security groups intended for an - # instance from the request. The reason for this is if using - # neutron security groups the requested security groups for the - # instance are not in the db and have not been sent to neutron yet. - if req.method != 'POST': - sg_instance_bindings = ( - self.security_group_api - .get_instances_security_groups_bindings(context, - servers)) - for server in servers: - groups = sg_instance_bindings.get(server['id']) - if groups: - server[ATTRIBUTE_NAME] = groups - - # In this section of code len(servers) == 1 as you can only POST - # one server in an API request. - else: - # try converting to json - req_obj = jsonutils.loads(req.body) - # Add security group to server, if no security group was in - # request add default since that is the group it is part of - servers[0][ATTRIBUTE_NAME] = req_obj['server'].get( - ATTRIBUTE_NAME, [{'name': 'default'}]) - - def _show(self, req, resp_obj): - if 'server' in resp_obj.obj: - self._extend_servers(req, [resp_obj.obj['server']]) - - @wsgi.extends - def show(self, req, resp_obj, id): - return self._show(req, resp_obj) - - @wsgi.extends - def create(self, req, resp_obj, body): - return self._show(req, resp_obj) - - @wsgi.extends - def detail(self, req, resp_obj): - self._extend_servers(req, list(resp_obj.obj['servers'])) diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 8736e00651..64e8c3818a 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -723,7 +723,8 @@ class ServersController(wsgi.Controller): show_extended_attr=False, show_host_status=False, show_keypair=False, - show_srv_usg=False) + show_srv_usg=False, + show_sec_grp=False) except exception.InstanceNotFound: msg = _("Instance could not be found") raise exc.HTTPNotFound(explanation=msg) @@ -997,7 +998,8 @@ class ServersController(wsgi.Controller): show_extended_attr=False, show_host_status=False, show_keypair=show_keypair, - show_srv_usg=False) + show_srv_usg=False, + show_sec_grp=False) # Add on the admin_password attribute since the view doesn't do it # unless instance passwords are disabled diff --git a/nova/api/openstack/compute/views/servers.py b/nova/api/openstack/compute/views/servers.py index 89a14137df..f779b30fba 100644 --- a/nova/api/openstack/compute/views/servers.py +++ b/nova/api/openstack/compute/views/servers.py @@ -15,6 +15,7 @@ # under the License. from oslo_log import log as logging +from oslo_serialization import jsonutils from nova.api.openstack import api_version_request from nova.api.openstack import common @@ -25,6 +26,7 @@ from nova import availability_zones as avail_zone from nova import compute from nova import context as nova_context from nova import exception +from nova.network.security_group import openstack_driver from nova import objects from nova.policies import extended_server_attributes as esa_policies from nova.policies import flavor_extra_specs as fes_policies @@ -65,10 +67,13 @@ class ViewBuilder(common.ViewBuilder): self._image_builder = views_images.ViewBuilder() self._flavor_builder = views_flavors.ViewBuilder() self.compute_api = compute.API() + self.security_group_api = ( + openstack_driver.get_openstack_security_group_driver()) def create(self, request, instance): """View that should be returned when an instance is created.""" - return { + + server = { "server": { "id": instance["uuid"], "links": self._get_links(request, @@ -81,9 +86,13 @@ class ViewBuilder(common.ViewBuilder): 'AUTO' if instance.get('auto_disk_config') else 'MANUAL'), }, } + self._add_security_grps(request, [server["server"]], [instance]) + + return server def basic(self, request, instance, show_extra_specs=False, - show_extended_attr=None, show_host_status=None): + show_extended_attr=None, show_host_status=None, + show_sec_grp=None): """Generic, non-detailed view of an instance.""" return { "server": { @@ -118,7 +127,7 @@ class ViewBuilder(common.ViewBuilder): def show(self, request, instance, extend_address=True, show_extra_specs=None, show_AZ=True, show_config_drive=True, show_extended_attr=None, show_host_status=None, - show_keypair=True, show_srv_usg=True): + show_keypair=True, show_srv_usg=True, show_sec_grp=True): """Detailed view of a single instance.""" ip_v4 = instance.get('access_ip_v4') ip_v6 = instance.get('access_ip_v6') @@ -191,6 +200,8 @@ class ViewBuilder(common.ViewBuilder): # the tzinfo from the stamp and str() it. server["server"][key] = (instance[k].replace(tzinfo=None) if instance[k] else None) + if show_sec_grp: + self._add_security_grps(request, [server["server"]], [instance]) if show_extended_attr is None: show_extended_attr = context.can( @@ -270,13 +281,23 @@ class ViewBuilder(common.ViewBuilder): if (api_version_request.is_supported(request, min_version='2.16')): show_host_status = context.can( servers_policies.SERVERS % 'show:host_status', fatal=False) - return self._list_view(self.show, request, instances, coll_name, - show_extra_specs, - show_extended_attr=show_extended_attr, - show_host_status=show_host_status) + # NOTE(gmann): pass show_sec_grp=False in _list_view() because + # security groups for detail method will be added by separate + # call to self._add_security_grps by passing the all servers + # together. That help to avoid multiple neutron call for each server. + servers_dict = self._list_view(self.show, request, instances, + coll_name, show_extra_specs, + show_extended_attr=show_extended_attr, + show_host_status=show_host_status, + show_sec_grp=False) + + self._add_security_grps(request, list(servers_dict["servers"]), + instances) + return servers_dict def _list_view(self, func, request, servers, coll_name, show_extra_specs, - show_extended_attr=None, show_host_status=None): + show_extended_attr=None, show_host_status=None, + show_sec_grp=False): """Provide a view for a list of servers. :param func: Function used to format the server data @@ -288,12 +309,15 @@ class ViewBuilder(common.ViewBuilder): included in the response dict. :param show_host_status: If the host status should be included in the response dict. + :param show_sec_grp: If the security group should be included in + the response dict. :returns: Server data in dictionary format """ server_list = [func(request, server, show_extra_specs=show_extra_specs, show_extended_attr=show_extended_attr, - show_host_status=show_host_status)["server"] + show_host_status=show_host_status, + show_sec_grp=show_sec_grp)["server"] for server in servers] servers_links = self._get_collection_links(request, servers, @@ -422,3 +446,42 @@ class ViewBuilder(common.ViewBuilder): fault_dict['details'] = fault["details"] return fault_dict + + def _add_security_grps(self, req, servers, instances): + # TODO(arosen) this function should be refactored to reduce duplicate + # code and use get_instance_security_groups instead of get_db_instance. + if not len(servers): + return + if not openstack_driver.is_neutron_security_groups(): + instances = {inst['uuid']: inst for inst in instances} + for server in servers: + instance = instances[server['id']] + groups = instance.get('security_groups') + if groups: + server['security_groups'] = [{"name": group.name} + for group in groups] + else: + # If method is a POST we get the security groups intended for an + # instance from the request. The reason for this is if using + # neutron security groups the requested security groups for the + # instance are not in the db and have not been sent to neutron yet. + if req.method != 'POST': + context = req.environ['nova.context'] + sg_instance_bindings = ( + self.security_group_api + .get_instances_security_groups_bindings(context, + servers)) + for server in servers: + groups = sg_instance_bindings.get(server['id']) + if groups: + server['security_groups'] = groups + + # This section is for POST request. There can be only one security + # group for POST request. + else: + # try converting to json + req_obj = jsonutils.loads(req.body) + # Add security group to server, if no security group was in + # request add default since that is the group it is part of + servers[0]['security_groups'] = req_obj['server'].get( + 'security_groups', [{'name': 'default'}]) diff --git a/nova/tests/unit/api/openstack/compute/test_access_ips.py b/nova/tests/unit/api/openstack/compute/test_access_ips.py index 0d70119588..936bb8cbf2 100644 --- a/nova/tests/unit/api/openstack/compute/test_access_ips.py +++ b/nova/tests/unit/api/openstack/compute/test_access_ips.py @@ -33,7 +33,8 @@ class AccessIPsAPIValidationTestV21(test.TestCase): def fake_rebuild(*args, **kwargs): pass - + # Neutron security groups are tested in test_neutron_security_groups.py + self.flags(use_neutron=False) fakes.stub_out_nw_api(self) self._set_up_controller() fake.stub_out_image_service(self) diff --git a/nova/tests/unit/api/openstack/compute/test_availability_zone.py b/nova/tests/unit/api/openstack/compute/test_availability_zone.py index 8c7ac56cb0..982c4b1fdb 100644 --- a/nova/tests/unit/api/openstack/compute/test_availability_zone.py +++ b/nova/tests/unit/api/openstack/compute/test_availability_zone.py @@ -168,6 +168,8 @@ class ServersControllerCreateTestV21(test.TestCase): super(ServersControllerCreateTestV21, self).setUp() self.instance_cache_num = 0 + # Neutron security groups are tested in test_neutron_security_groups.py + self.flags(use_neutron=False) fakes.stub_out_nw_api(self) self._set_up_controller() diff --git a/nova/tests/unit/api/openstack/compute/test_serversV21.py b/nova/tests/unit/api/openstack/compute/test_serversV21.py index d8e0723a3c..0755959f15 100644 --- a/nova/tests/unit/api/openstack/compute/test_serversV21.py +++ b/nova/tests/unit/api/openstack/compute/test_serversV21.py @@ -162,11 +162,22 @@ class ControllerTest(test.TestCase): def setUp(self): super(ControllerTest, self).setUp() self.flags(use_ipv6=False) + # Neutron security groups are tested in test_neutron_security_groups.py + self.flags(use_neutron=False) + fakes.stub_out_nw_api(self) fakes.stub_out_key_pair_funcs(self) fake.stub_out_image_service(self) + security_groups = [ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}] return_server = fakes.fake_compute_get(id=2, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=security_groups) return_servers = fakes.fake_compute_get_all() # Server sort keys extension is enabled in v21 so sort data is passed # to the instance API and the sorted DB API is invoked @@ -391,7 +402,9 @@ class ServersControllerTest(ControllerTest): "OS-EXT-SRV-ATTR:instance_name": "instance-00000002", "key_name": '', "OS-SRV-USG:launched_at": None, - "OS-SRV-USG:terminated_at": None + "OS-SRV-USG:terminated_at": None, + "security_groups": [{'name': 'fake-0-0'}, + {'name': 'fake-0-1'}] } } @@ -1509,7 +1522,14 @@ class ServersControllerTestV23(ServersControllerTest): metadata={"seq": "2"}, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}]) def _get_server_data_dict(self, uuid, image_bookmark, flavor_bookmark, status="ACTIVE", progress=100): @@ -1560,7 +1580,18 @@ class ServersControllerTestV23(ServersControllerTest): metadata={"seq": "2"}, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, + 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', + 'deleted': False, 'deleted_at': None, + 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, + 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', + 'deleted': False, 'deleted_at': None, + 'updated_at': None, 'created_at': None}]) obj_list.append(server) return objects.InstanceList(objects=obj_list) @@ -1595,7 +1626,14 @@ class ServersControllerTestV29(ServersControllerTest): metadata={"seq": "2"}, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}]) def _get_server_data_dict(self, uuid, image_bookmark, flavor_bookmark, status="ACTIVE", progress=100): @@ -1631,7 +1669,14 @@ class ServersControllerTestV29(ServersControllerTest): metadata={"seq": "2"}, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}]) req = self.req('/fake/servers/%s' % FAKE_UUID) res_dict = self.controller.show(req, FAKE_UUID) @@ -1671,7 +1716,14 @@ class ServersControllerTestV29(ServersControllerTest): metadata={"seq": "2"}, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}]) req = self.req('/fake/servers/detail') servers_list = self.controller.detail(req) @@ -1729,7 +1781,14 @@ class ServersControllerTestV216(ServersControllerTest): metadata={"seq": "2"}, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}]) self.useFixture(fixtures.MockPatchObject( compute_api.API, 'get_instance_host_status', return_value='UP')).mock @@ -1784,7 +1843,18 @@ class ServersControllerTestV216(ServersControllerTest): metadata={"seq": "2"}, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, + 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', + 'deleted': False, 'deleted_at': None, + 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, + 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', + 'deleted': False, 'deleted_at': None, + 'updated_at': None, 'created_at': None}]) obj_list.append(server) return objects.InstanceList(objects=obj_list) @@ -1819,7 +1889,14 @@ class ServersControllerTestV219(ServersControllerTest): metadata={"seq": "2"}, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}]) self.useFixture(fixtures.MockPatchObject( compute_api.API, 'get_instance_host_status', return_value='UP')).mock @@ -1861,7 +1938,14 @@ class ServersControllerTestV219(ServersControllerTest): metadata={"seq": "2"}, availability_zone='nova', launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}]) req = self.req('/fake/servers/%s' % FAKE_UUID) res_dict = self.controller.show(req, FAKE_UUID) @@ -3271,6 +3355,7 @@ class ServerStatusTest(test.TestCase): def setUp(self): super(ServerStatusTest, self).setUp() + self.flags(use_neutron=False) fakes.stub_out_nw_api(self) self.controller = servers.ServersController() @@ -3392,6 +3477,15 @@ class ServersControllerCreateTest(test.TestCase): "task_state": "", "vm_state": "", "root_device_name": inst.get('root_device_name', 'vda'), + "security_groups": [ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, + 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, + 'created_at': None}] }) self.instance_cache_by_id[instance['id']] = instance @@ -3462,6 +3556,9 @@ class ServersControllerCreateTest(test.TestCase): self.req = fakes.HTTPRequest.blank('/fake/servers') self.req.method = 'POST' self.req.headers["content-type"] = "application/json" + server = dict(name='server_test', imageRef=FAKE_UUID, flavorRef=2) + body = {'server': server} + self.req.body = encodeutils.safe_encode(jsonutils.dumps(body)) def _check_admin_password_len(self, server_dict): """utility function - check server_dict for admin_password length.""" @@ -6159,6 +6256,9 @@ class ServersViewBuilderTest(test.TestCase): def setUp(self): super(ServersViewBuilderTest, self).setUp() self.flags(use_ipv6=True) + # Neutron security groups are tested in test_neutron_security_groups.py + self.flags(use_neutron=False) + fakes.stub_out_nw_api(self) self.flags(group='glance', api_servers=['http://localhost:9292']) nw_cache_info = self._generate_nw_cache_info() db_inst = fakes.stub_instance( @@ -6170,7 +6270,14 @@ class ServersViewBuilderTest(test.TestCase): availability_zone='nova', nw_cache=nw_cache_info, launched_at=None, - terminated_at=None) + terminated_at=None, + security_groups=[ + {'name': 'fake-0-0', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}, + {'name': 'fake-0-1', 'id': 1, 'description': 'foo', + 'user_id': 'bar', 'project_id': 'baz', 'deleted': False, + 'deleted_at': None, 'updated_at': None, 'created_at': None}]) privates = ['172.19.0.1'] publics = ['192.168.0.3'] @@ -6351,7 +6458,9 @@ class ServersViewBuilderTest(test.TestCase): "OS-EXT-SRV-ATTR:instance_name": "instance-00000001", "key_name": '', "OS-SRV-USG:launched_at": None, - "OS-SRV-USG:terminated_at": None + "OS-SRV-USG:terminated_at": None, + "security_groups": [{'name': 'fake-0-0'}, + {'name': 'fake-0-1'}] } } @@ -6438,7 +6547,9 @@ class ServersViewBuilderTest(test.TestCase): "OS-EXT-SRV-ATTR:instance_name": "instance-00000001", "key_name": '', "OS-SRV-USG:launched_at": None, - "OS-SRV-USG:terminated_at": None + "OS-SRV-USG:terminated_at": None, + "security_groups": [{'name': 'fake-0-0'}, + {'name': 'fake-0-1'}] } } @@ -6625,7 +6736,9 @@ class ServersViewBuilderTest(test.TestCase): "OS-EXT-SRV-ATTR:instance_name": "instance-00000001", "key_name": '', "OS-SRV-USG:launched_at": None, - "OS-SRV-USG:terminated_at": None + "OS-SRV-USG:terminated_at": None, + "security_groups": [{'name': 'fake-0-0'}, + {'name': 'fake-0-1'}] } } @@ -6709,7 +6822,9 @@ class ServersViewBuilderTest(test.TestCase): "OS-EXT-SRV-ATTR:instance_name": "instance-00000001", "key_name": '', "OS-SRV-USG:launched_at": None, - "OS-SRV-USG:terminated_at": None + "OS-SRV-USG:terminated_at": None, + "security_groups": [{'name': 'fake-0-0'}, + {'name': 'fake-0-1'}] } }