Comment 100 for bug 1546507

Revision history for this message
Brian Rosmaita (brian-rosmaita) wrote :

Comments on "Fix based on the work of Mike and Feilong" patch
https://bugs.launchpad.net/glance/+bug/1546507/+attachment/4788885/+files/0001-Prevent-setting-locations-to-other-images.patch

I think the basic logic is OK, these are just a few things I noticed.

glance/location.py:
Two questions:
(1) Should we add a log message if we don't delete the backend data,
something like
"Data at %s not being removed as image %s does not appear to own it"
 % (location, image_id)

(2)if we do decide it's ok to delete the data, but len(locs) > 1,
should we provide a list of the locations that are now invalid (along
with the image ids that own those locations?

glance/api/v1/images.py:
glance/api/v2/images.py:
The exception message isn't quite accurate. Something like
"The location '%s' is forbidden for the configured store"
would be better.

glance/common/utils.py:
validate_external_url():
The NOTE is misleading. The function only checks to make sure the URL
contains the image_id of the image that owns the location being set.
It doesn't actually check to see if the location is in use by another
image.

glance/tests/unit/v1/test_api.py:
test_update_add_direct_rbd_location():
The location is 'rbd://another_image_id' for an image whose id is
'this_image_id', and the test correctly expects to get a 403.
We should also check that when location is 'rdb://this_image_id'
we get a 403, since that's supposed to be a disallowed URL.
(same thing in the v2 version of the test)

test_update_add_snap_styled_rbd_location():
Similar question, except this time, do we need a test to make
sure that a proper snap-style location, that is,
location = 'rbd://fsid/pool/%s/another_image_id' % image_id
returns a 2xx?
(same question about the v2 version)

By the way ... not sure if it makes sense to change this patch, but
someone went through the codebase recently and changed all the
numeric response codes to symbolic constants:
  from six.moves import http_client
and then use, for example, http_client.FORBIDDEN