From 0376185fb34a8cba3d8d705e579e08cddfb9316d Mon Sep 17 00:00:00 2001 From: Erno Kuvaja Date: Wed, 28 Mar 2018 19:49:48 +0100 Subject: [PATCH] Prevent taskflow creation in impossible import This change prevents taskflow creation during image import in cases we know for sure that the image activation won't be successful. The change follows the Glance principles of failing early and tries to provide as meaningful error messages to the end user as possible. Co-authored-by: Abhishek Kekane Closes-bug: 1758943 Change-Id: Iaee2781d07f4c0afd4f886f0b30b523bd47f1058 --- glance/api/v2/images.py | 27 +++++ glance/tests/unit/v2/test_images_resource.py | 112 +++++++++++++------ 2 files changed, 106 insertions(+), 33 deletions(-) diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index fedd53d82d..81510b0e39 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -90,6 +90,7 @@ class ImagesController(object): @utils.mutating def import_image(self, req, image_id, body): + image_repo = self.gateway.get_repo(req.context) task_factory = self.gateway.get_task_factory(req.context) executor_factory = self.gateway.get_task_executor_factory(req.context) task_repo = self.gateway.get_task_repo(req.context) @@ -99,6 +100,32 @@ class ImagesController(object): import_method = body.get('method').get('name') uri = body.get('method').get('uri') + + try: + image = image_repo.get(image_id) + if image.status == 'active': + msg = _("Image with status active cannot be target for import") + raise exception.Conflict(msg) + if image.status != 'queued' and import_method == 'web-download': + msg = _("Image needs to be in 'queued' state to use " + "'web-download' method") + raise exception.Conflict(msg) + if (image.status != 'uploading' and + import_method == 'glance-direct'): + msg = _("Image needs to be staged before 'glance-direct' " + "method can be used") + raise exception.Conflict(msg) + if not getattr(image, 'container_format', None): + msg = _("'container_format' needs to be set before import") + raise exception.Conflict(msg) + if not getattr(image, 'disk_format', None): + msg = _("'disk_format' needs to be set before import") + raise exception.Conflict(msg) + except exception.Conflict as e: + raise webob.exc.HTTPConflict(explanation=e.msg) + except exception.NotFound as e: + raise webob.exc.HTTPNotFound(explanation=e.msg) + if (import_method == 'web-download' and not utils.validate_import_uri(uri)): LOG.debug("URI for web-download does not pass filtering: %s", diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py index 2d6b8d0662..5d86907621 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -112,6 +112,15 @@ def _db_image_member_fixture(image_id, member_id, **kwargs): return obj +class FakeImage(object): + def __init__(self, status='active', container_format='ami', + disk_format='ami'): + self.id = UUID4 + self.status = status + self.container_format = container_format + self.disk_format = disk_format + + class TestImagesController(base.IsolatedUnitTest): def setUp(self): @@ -600,49 +609,80 @@ class TestImagesController(base.IsolatedUnitTest): self.assertRaises(webob.exc.HTTPNotFound, self.controller.show, request, UUID4) - def test_image_import_raises_conflict(self): + def test_image_import_raises_conflict_if_container_format_is_none(self): request = unit_test_utils.get_fake_request() - # NOTE(abhishekk): Due to - # https://bugs.launchpad.net/glance/+bug/1712463 taskflow is not - # executing. Once it is fixed instead of mocking spawn_n method - # we should mock execute method of _ImportToStore task. - with mock.patch.object(eventlet.GreenPool, 'spawn_n', - side_effect=exception.Conflict): + + with mock.patch.object( + glance.api.authorization.ImageRepoProxy, 'get') as mock_get: + mock_get.return_value = FakeImage(container_format=None) self.assertRaises(webob.exc.HTTPConflict, self.controller.import_image, request, UUID4, {'method': {'name': 'glance-direct'}}) + def test_image_import_raises_conflict_if_disk_format_is_none(self): + request = unit_test_utils.get_fake_request() + + with mock.patch.object( + glance.api.authorization.ImageRepoProxy, 'get') as mock_get: + mock_get.return_value = FakeImage(disk_format=None) + self.assertRaises(webob.exc.HTTPConflict, + self.controller.import_image, request, UUID4, + {'method': {'name': 'glance-direct'}}) + + def test_image_import_raises_conflict(self): + request = unit_test_utils.get_fake_request() + + with mock.patch.object( + glance.api.authorization.ImageRepoProxy, 'get') as mock_get: + mock_get.return_value = FakeImage(status='queued') + self.assertRaises(webob.exc.HTTPConflict, + self.controller.import_image, request, UUID4, + {'method': {'name': 'glance-direct'}}) + + def test_image_import_raises_conflict_for_web_download(self): + request = unit_test_utils.get_fake_request() + + with mock.patch.object( + glance.api.authorization.ImageRepoProxy, 'get') as mock_get: + mock_get.return_value = FakeImage() + self.assertRaises(webob.exc.HTTPConflict, + self.controller.import_image, request, UUID4, + {'method': {'name': 'web-download'}}) + def test_image_import_raises_conflict_for_invalid_status_change(self): request = unit_test_utils.get_fake_request() - # NOTE(abhishekk): Due to - # https://bugs.launchpad.net/glance/+bug/1712463 taskflow is not - # executing. Once it is fixed instead of mocking spawn_n method - # we should mock execute method of _ImportToStore task. + with mock.patch.object( - eventlet.GreenPool, 'spawn_n', - side_effect=exception.InvalidImageStatusTransition): + glance.api.authorization.ImageRepoProxy, 'get') as mock_get: + mock_get.return_value = FakeImage() self.assertRaises(webob.exc.HTTPConflict, self.controller.import_image, request, UUID4, {'method': {'name': 'glance-direct'}}) def test_image_import_raises_bad_request(self): request = unit_test_utils.get_fake_request() - # NOTE(abhishekk): Due to - # https://bugs.launchpad.net/glance/+bug/1712463 taskflow is not - # executing. Once it is fixed instead of mocking spawn_n method - # we should mock execute method of _ImportToStore task. - with mock.patch.object(eventlet.GreenPool, 'spawn_n', - side_effect=ValueError): - self.assertRaises(webob.exc.HTTPBadRequest, - self.controller.import_image, request, UUID4, - {'method': {'name': 'glance-direct'}}) + with mock.patch.object( + glance.api.authorization.ImageRepoProxy, 'get') as mock_get: + mock_get.return_value = FakeImage(status='uploading') + # NOTE(abhishekk): Due to + # https://bugs.launchpad.net/glance/+bug/1712463 taskflow is not + # executing. Once it is fixed instead of mocking spawn_n method + # we should mock execute method of _ImportToStore task. + with mock.patch.object(eventlet.GreenPool, 'spawn_n', + side_effect=ValueError): + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.import_image, request, UUID4, + {'method': {'name': 'glance-direct'}}) def test_image_import_invalid_uri_filtering(self): request = unit_test_utils.get_fake_request() - self.assertRaises(webob.exc.HTTPBadRequest, - self.controller.import_image, request, UUID4, - {'method': {'name': 'web-download', - 'uri': 'fake_uri'}}) + with mock.patch.object( + glance.api.authorization.ImageRepoProxy, 'get') as mock_get: + mock_get.return_value = FakeImage(status='queued') + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.import_image, request, UUID4, + {'method': {'name': 'web-download', + 'uri': 'fake_uri'}}) def test_create(self): request = unit_test_utils.get_fake_request() @@ -2251,9 +2291,12 @@ class TestImagesController(base.IsolatedUnitTest): def test_image_import(self): request = unit_test_utils.get_fake_request() - output = self.controller.import_image(request, UUID4, - {'method': {'name': - 'glance-direct'}}) + with mock.patch.object( + glance.api.authorization.ImageRepoProxy, 'get') as mock_get: + mock_get.return_value = FakeImage(status='uploading') + output = self.controller.import_image( + request, UUID4, {'method': {'name': 'glance-direct'}}) + self.assertEqual(UUID4, output) def test_image_import_not_allowed(self): @@ -2261,10 +2304,13 @@ class TestImagesController(base.IsolatedUnitTest): # NOTE(abhishekk): For coverage purpose setting tenant to # None. It is not expected to do in normal scenarios. request.context.tenant = None - self.assertRaises(webob.exc.HTTPForbidden, - self.controller.import_image, - request, UUID4, {'method': {'name': - 'glance-direct'}}) + with mock.patch.object( + glance.api.authorization.ImageRepoProxy, 'get') as mock_get: + mock_get.return_value = FakeImage(status='uploading') + self.assertRaises(webob.exc.HTTPForbidden, + self.controller.import_image, + request, UUID4, {'method': {'name': + 'glance-direct'}}) class TestImagesControllerPolicies(base.IsolatedUnitTest):