openstack
/
glance
Code Issues Proposed changes

Restrict location updates to active, queued images 

Currently image location updates (removing, replacing) are permitted for
images even if their state is not ``active``.

This need is to:
 * Prevent the replacement of image locations of images that are not
   ``active`` or ``queued`` by returning a Conflict Error
    (409 response code).
 * Prevent removal of image locations of images that are not ``active``
   by returning a Conflict Error (409 response code).

Changing of image locations when the image state is not ``active`` can
result in bad experiences for the users.
 *  If one tries to change or remove the location for an image while it
    is in ``saving`` state, Glance would be trying to write data to a
    previously saved location while the user updates the custom
    location. This results in a race condition.
 *  For images that are in ``queued`` state and no image data has been
    uploaded yet, there is no need for an image location to be
    removed and permitting users to remove the image
    location can result in a bad experience. However users can be
    allowed to replace the image location to maintain backward
    compatibility and also because replacing could mean replacing an
    empty location by a non-empty image location.
*   For images in ``deactivated`` state, it is essential that image
    locations are not updated as it does not abide with the purpose of
    the image state being set to ``deactivated`` and may cause security
    concerns.

This commit introduces the following change in behavior:
  * If an image is in ``active`` state, removing/replacing the custom
    locations on that image will be allowed so there is no change in
    behavior for this case.
  * If an image is in ``saving`` or ``deactivated``, the status of
    that image will be checked while trying to replace/remove the custom
    location and a HTTP 409 Conflict status will be returned in response
    to the user.
  * If an image is in ``queued`` state, removing the custom
    image location will not be permitted as an image in queued status
    should not have any location associated with it. Replacing of location
    may be permitted here though.
  * If an image is in ``deleted`` or ``pending_delete`` state, a HTTP
    409 Conflict status will be returned, if that image is visible to
    the user (in case of admins). Otherwise, the location cannot be
    removed/replaced anyway.  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 returned.

TODO:
Atomicity is required such that glance permits removal/replacement of
image locations on certain permissible image transition states handling
the race conditions like:
  * In case where the status of the image is ``saving`` and it
    has just moved to ```active`` status, ideally removing/replacing
    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
    removing/replacing custom location should be allowed. Again, due
    to lack of atomicity in setting image status glance will ignore
    the request 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 remove/replace
    the 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 replace the location for
    that image.
  * In case where the status of the image is ``active`` and location
    is attempted to be set on it, and at that point if the image goes
    into ``deleted``, ``pending_delete`` or ``killed`` status, then the
    user must get a HTTP 409 Conflict status. However due to lack of
    atomicity in setting the image status, the location may get updated.

NOTE:
This commit ensures that removal of image locations
is not allowed for an image in any state except ``active`` and
replacement of an image location is not allowed except for ``active``
and ``queued`` images.
Further work is required on the atomicity of glance to accommodate
location updates in cases where the images would be undergoing state
changes.

Impacts:
APIImpact
DocImpact

Credits:
This commit message contains text and information from the commit
message for: https://review.openstack.org/#/c/324012/
co authored by Nikhil Komawar <nik.komawar@gmail.com>.

Co-Authored-By: Nikhil Komawar <nik.komawar@gmail.com>
Co-Authored-By: Dharini Chandrasekar <dharini.chandrasekar@intel.com>

Lite-Spec: https://review.openstack.org/#/c/368192/
Closes-Bug: 1622016

Change-Id: Ic24cf8234599a32add1cb9f294f902e497d885e0
This commit is contained in:
Nikhil Komawar 2016-09-07 17:29:41 -04:00 committed by Dharini Chandrasekar
parent e822301130
commit 4ac8adbccc
3 changed files with 124 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
glance/api/v2/images.py
View File

@ -274,12 +274,15 @@ class ImagesController(object):
"invisible.")
raise webob.exc.HTTPForbidden(explanation=msg)
if image.status not in ('active', 'queued'):
msg = _("It's not allowed to replace locations if image status is "
"%s.") % image.status
raise webob.exc.HTTPConflict(explanation=msg)
try:
# NOTE(flwang): _locations_proxy's setattr method will check if
# the update is acceptable.
image.locations = value
if image.status == 'queued':
image.status = 'active'
except (exception.BadStoreUri, exception.DuplicateLocation) as e:
raise webob.exc.HTTPBadRequest(explanation=e.msg)
except ValueError as ve: # update image status failed.
@ -318,6 +321,11 @@ class ImagesController(object):
"invisible.")
raise webob.exc.HTTPForbidden(explanation=msg)
if image.status not in ('active'):
msg = _("It's not allowed to remove locations if image status is "
"%s.") % image.status
raise webob.exc.HTTPConflict(explanation=msg)
if len(image.locations) == 1:
LOG.debug("User forbidden to remove last location of image %s",
image.image_id)

161
glance/tests/unit/v2/test_images_resource.py
View File

@ -1571,90 +1571,113 @@ 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):
def test_replace_location_possible_on_queued(self):
self.config(show_multiple_locations=True)
self.images = [
_db_fixture('1', owner=TENANT1, checksum=CHKSUM,
name='1',
is_public=True,
disk_format='raw',
container_format='bare',
status='queued'),
]
self.db.image_create(None, self.images[0])
request = unit_test_utils.get_fake_request()
new_location = {'url': '%s/fake_location_1' % BASE_URI, 'metadata': {}}
changes = [{'op': 'replace', 'path': ['locations'],
'value': [new_location]}]
output = self.controller.update(request, '1', changes)
self.assertEqual('1', output.image_id)
self.assertEqual(1, len(output.locations))
self.assertEqual(new_location, output.locations[0])
def test_add_location_possible_on_queued(self):
self.config(show_multiple_locations=True)
self.images = [
_db_fixture('1', owner=TENANT1, checksum=CHKSUM,
name='1',
is_public=True,
disk_format='raw',
container_format='bare',
status='queued'),
]
self.db.image_create(None, self.images[0])
request = unit_test_utils.get_fake_request()
new_location = {'url': '%s/fake_location_1' % BASE_URI, 'metadata': {}}
changes = [{'op': 'add', 'path': ['locations', '-'],
'value': new_location}]
output = self.controller.update(request, '1', changes)
self.assertEqual('1', output.image_id)
self.assertEqual(1, len(output.locations))
self.assertEqual(new_location, output.locations[0])
def _test_update_locations_status(self, image_status, update):
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'),
status=image_status),
]
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', '-'],
if image_status == 'deactivated':
self.db.image_create(request.context, self.images[0])
else:
self.db.image_create(None, self.images[0])
new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}
changes = [{'op': update, '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_location_add_not_permitted_status_saving(self):
self._test_update_locations_status('saving', 'add')
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_location_add_not_permitted_status_deactivated(self):
self._test_update_locations_status('deactivated', 'add')
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_location_add_not_permitted_status_deleted(self):
self._test_update_locations_status('deleted', 'add')
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_location_add_not_permitted_status_pending_delete(self):
self._test_update_locations_status('pending_delete', 'add')
def test_location_add_not_permitted_status_killed(self):
self._test_update_locations_status('killed', 'add')
def test_location_remove_not_permitted_status_saving(self):
self._test_update_locations_status('saving', 'remove')
def test_location_remove_not_permitted_status_deactivated(self):
self._test_update_locations_status('deactivated', 'remove')
def test_location_remove_not_permitted_status_deleted(self):
self._test_update_locations_status('deleted', 'remove')
def test_location_remove_not_permitted_status_pending_delete(self):
self._test_update_locations_status('pending_delete', 'remove')
def test_location_remove_not_permitted_status_killed(self):
self._test_update_locations_status('killed', 'remove')
def test_location_remove_not_permitted_status_queued(self):
self._test_update_locations_status('queued', 'remove')
def test_location_replace_not_permitted_status_saving(self):
self._test_update_locations_status('saving', 'replace')
def test_location_replace_not_permitted_status_deactivated(self):
self._test_update_locations_status('deactivated', 'replace')
def test_location_replace_not_permitted_status_deleted(self):
self._test_update_locations_status('deleted', 'replace')
def test_location_replace_not_permitted_status_pending_delete(self):
self._test_update_locations_status('pending_delete', 'replace')
def test_location_replace_not_permitted_status_killed(self):
self._test_update_locations_status('killed', 'replace')
def test_update_add_locations_insertion(self):
self.config(show_multiple_locations=True)

22
releasenotes/notes/restrict_location_updates-05454bb765a8c92c.yaml Normal file
View File

@ -0,0 +1,22 @@
---
prelude: >
Location updates for images are now restricted to
images in ``active`` or ``queued`` status. Please
refer to the "Fixes" section for more information.
fixes:
- |
Image location updates to an image not in status
``active`` or ``queued`` can introduce race
conditions and security issues and hence a bad
experience for users and operators. As a result,
we have restricted image location updates in this
release. Users will now observe the following:
* HTTP Response Code 409 (Conflict) will be returned
in response to an attempt to remove an image
location when the image status is not ``active``
* HTTP Response Code 409 (Conflict) will be returned
in response to an attempt to replace an image
location when the image status is not ``active`` or
``queued``