From 9bb85f24de6075f3fca08705659021d8e1c35fb5 Mon Sep 17 00:00:00 2001 From: TommyLike Date: Sat, 25 Mar 2017 22:37:40 +0800 Subject: [PATCH] [BugFix] Add 'all_tenants', 'project_id' in attachment-list There are some issues around new attach/detach API/CLI, fix them step by step. This patch adds 'all_tenants', 'project_id' support in attachment-list along with these modifications: 1. Don't throw errors when have invalid filters and don't check filters when is admin context. 2. Don't apply filters again when query already applied limit and offset. Change-Id: I5f469c809dfb16e52d6298c4aa13a30c27ee56f9 Closes-Bug: #1675974 --- cinder/api/v3/attachments.py | 24 +++++------ cinder/db/sqlalchemy/api.py | 16 ++++---- cinder/tests/unit/api/v3/test_attachments.py | 40 +++++++++++++++++-- ...t-in-attachment-list-3edd8g138a28s4r8.yaml | 4 ++ 4 files changed, 61 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/support-tenants-project-in-attachment-list-3edd8g138a28s4r8.yaml diff --git a/cinder/api/v3/attachments.py b/cinder/api/v3/attachments.py index cd44e93918e..093443adff0 100644 --- a/cinder/api/v3/attachments.py +++ b/cinder/api/v3/attachments.py @@ -69,23 +69,21 @@ class AttachmentsController(wsgi.Controller): search_opts = req.GET.copy() sort_keys, sort_dirs = common.get_sort_params(search_opts) marker, limit, offset = common.get_pagination_params(search_opts) - filters = search_opts - allowed = self.allowed_filters - if not allowed.issuperset(filters): - invalid_keys = set(filters).difference(allowed) - msg = _('Invalid filter keys: %s') % ', '.join(invalid_keys) - raise exception.InvalidInput(reason=msg) - # Filter out invalid options - allowed_search_options = ('status', 'volume_id', - 'instance_id') if search_opts.get('instance_id', None): search_opts['instance_uuid'] = search_opts.get('instance_id') utils.remove_invalid_filter_options(context, search_opts, - allowed_search_options) - return objects.VolumeAttachmentList.get_all( - context, search_opts=search_opts, marker=marker, limit=limit, - offset=offset, sort_keys=sort_keys, sort_direction=sort_dirs) + self.allowed_filters) + if context.is_admin and 'all_tenants' in search_opts: + del search_opts['all_tenants'] + return objects.VolumeAttachmentList.get_all( + context, search_opts=search_opts, marker=marker, limit=limit, + offset=offset, sort_keys=sort_keys, sort_direction=sort_dirs) + else: + return objects.VolumeAttachmentList.get_all_by_project( + context, context.project_id, search_opts=search_opts, + marker=marker, limit=limit, offset=offset, sort_keys=sort_keys, + sort_direction=sort_dirs) @wsgi.Controller.api_version(API_VERSION) @wsgi.response(202) diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 29533b80859..89db0a916b0 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -1778,9 +1778,9 @@ def _volume_get(context, volume_id, session=None, joined_load=True): def _attachment_get_all(context, filters=None, marker=None, limit=None, offset=None, sort_keys=None, sort_dirs=None): - project_id = filters.pop('project_id', None) if filters else None if filters and not is_valid_model_filters(models.VolumeAttachment, - filters): + filters, + exclude_list=['project_id']): return [] session = get_session() @@ -1791,11 +1791,6 @@ def _attachment_get_all(context, filters=None, marker=None, limit=None, offset, models.VolumeAttachment) if query is None: return [] - - query = query.options(joinedload('volume')) - if project_id: - query = query.filter(models.Volume.project_id == project_id) - return query.all() @@ -1820,9 +1815,16 @@ def _attachment_get_query(context, session=None, project_only=False): def _process_attachment_filters(query, filters): if filters: + project_id = filters.pop('project_id', None) # Ensure that filters' keys exist on the model if not is_valid_model_filters(models.VolumeAttachment, filters): return + if project_id: + volume = models.Volume + query = query.filter(volume.id == + models.VolumeAttachment.volume_id, + volume.project_id == project_id) + query = query.filter_by(**filters) return query diff --git a/cinder/tests/unit/api/v3/test_attachments.py b/cinder/tests/unit/api/v3/test_attachments.py index aa4cce73771..c3f06fd4e6a 100644 --- a/cinder/tests/unit/api/v3/test_attachments.py +++ b/cinder/tests/unit/api/v3/test_attachments.py @@ -42,18 +42,36 @@ class AttachmentsAPITestCase(test.TestCase): self.ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, auth_token=True, is_admin=True) + self.volume1 = self._create_volume(display_name='fake_volume_1', + project_id=fake.PROJECT_ID) + self.volume2 = self._create_volume(display_name='fake_volume_2', + project_id=fake.PROJECT2_ID) self.attachment1 = self._create_attachement( - volume_uuid=fake.VOLUME_ID, instance_uuid=fake.UUID1) + volume_uuid=self.volume1.id, instance_uuid=fake.UUID1) self.attachment2 = self._create_attachement( - volume_uuid=fake.VOLUME2_ID, instance_uuid=fake.UUID1) + volume_uuid=self.volume1.id, instance_uuid=fake.UUID1) self.attachment3 = self._create_attachement( - volume_uuid=fake.VOLUME3_ID, instance_uuid=fake.UUID2) + volume_uuid=self.volume1.id, instance_uuid=fake.UUID2) + self.attachment4 = self._create_attachement( + volume_uuid=self.volume2.id, instance_uuid=fake.UUID2) self.addCleanup(self._cleanup) def _cleanup(self): self.attachment1.destroy() self.attachment2.destroy() self.attachment3.destroy() + self.attachment4.destroy() + self.volume1.destroy() + self.volume2.destroy() + + def _create_volume(self, ctxt=None, display_name=None, project_id=None): + """Create a volume object.""" + ctxt = ctxt or self.ctxt + volume = objects.Volume(ctxt) + volume.display_name = display_name + volume.project_id = project_id + volume.create() + return volume def _create_attachement(self, ctxt=None, volume_uuid=None, instance_uuid=None, mountpoint=None, @@ -141,3 +159,19 @@ class AttachmentsAPITestCase(test.TestCase): expect_result = order_ids[2] if sort_dir == "desc" else order_ids[0] self.assertEqual(expect_result, res_dict['attachments'][0]['id']) + + @ddt.data({'admin': True, 'request_url': '?all_tenants=1', 'count': 4}, + {'admin': False, 'request_url': '?all_tenants=1', 'count': 3}, + {'admin': True, 'request_url': + '?all_tenants=1&project_id=%s' % fake.PROJECT2_ID, + 'count': 1}, + {'admin': False, 'request_url': '', 'count': 3}) + @ddt.unpack + def test_list_attachment_with_tenants(self, admin, request_url, count): + url = '/v3/%s/attachments%s' % (fake.PROJECT_ID, request_url) + req = fakes.HTTPRequest.blank(url, version=ATTACHMENTS_MICRO_VERSION, + use_admin_context=admin) + res_dict = self.controller.index(req) + + self.assertEqual(1, len(res_dict)) + self.assertEqual(count, len(res_dict['attachments'])) diff --git a/releasenotes/notes/support-tenants-project-in-attachment-list-3edd8g138a28s4r8.yaml b/releasenotes/notes/support-tenants-project-in-attachment-list-3edd8g138a28s4r8.yaml new file mode 100644 index 00000000000..920b10e6fdc --- /dev/null +++ b/releasenotes/notes/support-tenants-project-in-attachment-list-3edd8g138a28s4r8.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Add ``all_tenants``, ``project_id`` support in + attachment list&detail APIs.