From 7c7dd626896d732d75c6b802a33b9462aee885fd Mon Sep 17 00:00:00 2001 From: Mike Fedosin Date: Wed, 1 Jun 2016 19:38:48 +0300 Subject: [PATCH] Attempt to not set location on non active or queued image Currently, an authorized user can set custom location string to an image that is in any non-deleted status. If a custom location is set to an image in ``saving`` status, it may result in a race condition between the data stream that is trying to save the image data to the backend and the custom location user is attempting to set on the image record. This will result in a bad experience for the user who is streaming the image data to Glance as it is a better experiece to set the location after the image data has been saved and image is in ``active`` status, in this case. If a custom location is set to an image in ``deactivated`` status, the purpose of setting the image to ``deactivated`` is void. This will result in a worse experience for the security team that is trying to evaluate the validity of the image data of the deactivated image. Avoiding setting custom location in this case will ensure data consistency until image is pulled out of the deactivation process. This commit introduces the following change in behavior: * If an image is in ``saving`` or ``deactivated`` status, the staus of that image will be checked while trying to set the custom location and a HTTP 409 Conflict status will be retured in response to the user. * If an image is in ``active`` or ``queued`` status, setting custom locations on that image will be allowed so there is no change in behavior for this case. * If an image is in ``deleted`` or ``pending_delete`` status, a HTTP 409 Conflict status will be retured, if that image is visible to the user (in case of admins). Otherwise, the location cannot be set anyway. Setting a location to a ``deleted`` image is fruitless as the image cannot be used. Please note ``pending_delete`` is another form of the ``deleted`` status and behavior in either case should be expected to be same. * If an image is in ``killed`` status, a HTTP 409 Conflict status will be retured. Nevertheless, it is again fruitless to attempt to set a location on such images as they are unusable. * This operation still involves the following race conditions: * In case where the status of the image is ``saving`` and it has just moved to ```active`` status, ideally setting custom location should be allowed however, due to lack of atomicity in setting image status glance will ignore setting the location and a 409 will be returned. * In case where the status of the image is ``deactivated`` and it has just been moved to ``active`` status, ideally setting custom location should be allowed however, due to lack of atomicity in setting image status glance will ignore setting the location and a 409 will be returned. * In case where the status of the image is ``active`` and it has just been moved to ``deactivated`` status, due to lack of atomicity in setting image status, glance may set another location on that image. * In case where the status of the image is ``queued`` and it has just been moved to ``saving`` status, due to lack of atomicity in setting image status, glance may set another location on that image. * In case where the status of the image is ``queued`` or ``active`` and location is attempted to be set on it, if the image first goes into ``deleted``, ``pending_delete`` or ``killed`` status then the user will get a HTTP 409 Conflict status back. This occurs again due to lack of atomicity in setting image status. Please note: * We will plan to add further granularity in setting locations to the images atomically in subsequent commits. * Fow now, though, this commit does resolve the issue of setting locations on an image incorrectly (in unexpected circumstances) to a significant degree. So, this is good progress in ensuring rightful use of the image locations feature. APIImpact DocImpact Co-Authored-By: Mike Fedosin Co-Authored-By: Nikhil Komawar Change-Id: Ie463e2f30db94cde7716c83a94ec2fb0c0658c91 Lite-Spec: https://review.openstack.org/#/c/324780/ Partial-Bug: 1587985 --- glance/api/v2/images.py | 5 ++ glance/tests/unit/v2/test_images_resource.py | 85 ++++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index 36a524230f..72cc39fa2b 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -292,6 +292,11 @@ class ImagesController(object): "invisible.") raise webob.exc.HTTPForbidden(explanation=msg) + if image.status not in ('active', 'queued'): + msg = _("It's not allowed to add locations if image status is " + "%s.") % image.status + raise webob.exc.HTTPConflict(explanation=msg) + pos = self._get_locations_op_pos(path_pos, len(image.locations), True) if pos is None: diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py index 3cfc4ddf19..86f102215f 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -1571,6 +1571,91 @@ class TestImagesController(base.IsolatedUnitTest): self.assertEqual(2, len(output.locations)) self.assertEqual(new_location, output.locations[1]) + def test_update_add_locations_status_saving(self): + self.config(show_multiple_locations=True) + self.images = [ + _db_fixture('1', owner=TENANT1, checksum=CHKSUM, + name='1', + disk_format='raw', + container_format='bare', + status='saving'), + ] + self.db.image_create(None, self.images[0]) + new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} + request = unit_test_utils.get_fake_request() + changes = [{'op': 'add', 'path': ['locations', '-'], + 'value': new_location}] + self.assertRaises(webob.exc.HTTPConflict, + self.controller.update, request, '1', changes) + + def test_update_add_locations_status_deactivated(self): + self.config(show_multiple_locations=True) + self.images = [ + _db_fixture('1', owner=TENANT1, checksum=CHKSUM, + name='1', + disk_format='raw', + container_format='bare', + status='deactivated'), + ] + request = unit_test_utils.get_fake_request() + self.db.image_create(request.context, self.images[0]) + new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} + changes = [{'op': 'add', 'path': ['locations', '-'], + 'value': new_location}] + self.assertRaises(webob.exc.HTTPConflict, + self.controller.update, request, '1', changes) + + def test_update_add_locations_status_deleted(self): + self.config(show_multiple_locations=True) + self.images = [ + _db_fixture('1', owner=TENANT1, checksum=CHKSUM, + name='1', + disk_format='raw', + container_format='bare', + status='deleted'), + ] + self.db.image_create(None, self.images[0]) + new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} + request = unit_test_utils.get_fake_request() + changes = [{'op': 'add', 'path': ['locations', '-'], + 'value': new_location}] + self.assertRaises(webob.exc.HTTPConflict, + self.controller.update, request, '1', changes) + + def test_update_add_locations_status_pending_delete(self): + self.config(show_multiple_locations=True) + self.images = [ + _db_fixture('1', owner=TENANT1, checksum=CHKSUM, + name='1', + disk_format='raw', + container_format='bare', + status='pending_delete'), + ] + self.db.image_create(None, self.images[0]) + new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} + request = unit_test_utils.get_fake_request() + changes = [{'op': 'add', 'path': ['locations', '-'], + 'value': new_location}] + self.assertRaises(webob.exc.HTTPConflict, + self.controller.update, request, '1', changes) + + def test_update_add_locations_status_killed(self): + self.config(show_multiple_locations=True) + self.images = [ + _db_fixture('1', owner=TENANT1, checksum=CHKSUM, + name='1', + disk_format='raw', + container_format='bare', + status='killed'), + ] + self.db.image_create(None, self.images[0]) + new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} + request = unit_test_utils.get_fake_request() + changes = [{'op': 'add', 'path': ['locations', '-'], + 'value': new_location}] + self.assertRaises(webob.exc.HTTPConflict, + self.controller.update, request, '1', changes) + def test_update_add_locations_insertion(self): self.config(show_multiple_locations=True) new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}