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
Comments on "Fix based on the work of Mike and Feilong" patch /bugs.launchpad .net/glance/ +bug/1546507/ +attachment/ 4788885/ +files/ 0001-Prevent- setting- locations- to-other- images. patch
https:/
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: api/v2/ images. py:
glance/
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: external_ url():
validate_
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: add_direct_ rbd_location( ): another_ image_id' for an image whose id is this_image_ id'
test_update_
The location is 'rbd://
'this_image_id', and the test correctly expects to get a 403.
We should also check that when location is 'rdb://
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( ): fsid/pool/ %s/another_ image_id' % image_id
Similar question, except this time, do we need a test to make
sure that a proper snap-style location, that is,
location = 'rbd://
returns a 2xx?
(same question about the v2 version)
By the way ... not sure if it makes sense to change this patch, but FORBIDDEN
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.