openstack
/
glance
Code Issues Proposed changes

Make is_public an argument rather than a filter. 

There is disagreement between the v1 api and v2 api about what an
'is_public' filter ought to mean. Version 1 uses 'is_public'
to filter whether or not image.is_public == True or False. Version 2
uses 'visibility' for this purpose. But in addition, it is theoretically
possible that 'is_public' is a valid property filter in v2.

With this change, v1 can pass is_public in as an argument to
image_get_all. V2 will continue to pass is_public through filters.

fixes bug: 1157988
fixes bug: 1154760

Change-Id: I716cb8b4f207f8526125ed360f5a24a6630c1e67
This commit is contained in:
Mark J. Washenberger 2013-03-22 10:59:28 -07:00
parent 0fc6972024
commit d8e3f873e0
6 changed files with 171 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
glance/db/simple/api.py
View File

@ -124,15 +124,13 @@ def _image_format(image_id, **values):
return image
def _filter_images(images, filters, context, status='accepted'):
def _filter_images(images, filters, context,
status='accepted', is_public=None):
filtered_images = []
if 'properties' in filters:
prop_filter = filters.pop('properties')
filters.update(prop_filter)
if 'is_public' in filters and filters['is_public'] is None:
filters.pop('is_public')
if status == 'all':
status = None
@ -150,16 +148,21 @@ def _filter_images(images, filters, context, status='accepted'):
if visibility:
if visibility == 'public':
filters['is_public'] = True
if not image['is_public']:
continue
elif visibility == 'private':
filters['is_public'] = False
if image['is_public']:
continue
if not (has_ownership or context.is_admin):
continue
elif visibility == 'shared':
if not is_member:
continue
if is_public is not None:
if not image['is_public'] == is_public:
continue
add = True
for k, value in filters.iteritems():
key = k
@ -175,7 +178,7 @@ def _filter_images(images, filters, context, status='accepted'):
add = image.get(key) >= value
elif k.endswith('_max'):
add = image.get(key) <= value
elif image.get(k) is not None:
elif k != 'is_public' and image.get(k) is not None:
add = image.get(key) == value
else:
properties = {}
@ -253,10 +256,10 @@ def image_get(context, image_id, session=None, force_show_deleted=False):
@log_call
def image_get_all(context, filters=None, marker=None, limit=None,
sort_key='created_at', sort_dir='desc',
member_status='accepted'):
member_status='accepted', is_public=None):
filters = filters or {}
images = DATA['images'].values()
images = _filter_images(images, filters, context, member_status)
images = _filter_images(images, filters, context, member_status, is_public)
images = _sort_images(images, sort_key, sort_dir)
images = _do_pagination(context, images, marker, limit,
filters.get('deleted'))

23
glance/db/sqlalchemy/api.py
View File

@ -485,7 +485,7 @@ def paginate_query(query, model, limit, sort_keys, marker=None,
def image_get_all(context, filters=None, marker=None, limit=None,
sort_key='created_at', sort_dir='desc',
member_status='accepted'):
member_status='accepted', is_public=None):
"""
Get all images that match zero or more filters.
@ -496,6 +496,10 @@ def image_get_all(context, filters=None, marker=None, limit=None,
:param limit: maximum number of images to return
:param sort_key: image attribute by which results should be sorted
:param sort_dir: direction in which results should be sorted (asc, desc)
:param member_status: only return shared images that have this membership
status
:param is_public: If true, return only public images. If false, return
only private and shared images.
"""
filters = filters or {}
@ -504,10 +508,6 @@ def image_get_all(context, filters=None, marker=None, limit=None,
.options(sa_orm.joinedload(models.Image.properties))\
.options(sa_orm.joinedload(models.Image.locations))
# NOTE(markwash) treat is_public=None as if it weren't filtered
if 'is_public' in filters and filters['is_public'] is None:
del filters['is_public']
if not context.is_admin:
visibility_filters = [models.Image.is_public == True]
@ -532,9 +532,8 @@ def image_get_all(context, filters=None, marker=None, limit=None,
visibility = filters.pop('visibility')
if visibility == 'public':
query = query.filter(models.Image.is_public == True)
filters['is_public'] = True
elif visibility == 'private':
filters['is_public'] = False
query = query.filter(models.Image.is_public == False)
if (not context.is_admin) and context.owner is not None:
query = query.filter(
models.Image.owner == context.owner)
@ -543,6 +542,16 @@ def image_get_all(context, filters=None, marker=None, limit=None,
models.Image.members.any(member=context.owner,
deleted=False))
if is_public is not None:
query = query.filter(models.Image.is_public == is_public)
if 'is_public' in filters:
spec = models.Image.properties.any(
name='is_public',
value=filters.pop('is_public'),
deleted=False)
query = query.filter(spec)
showing_deleted = False
if 'changes-since' in filters:
# normalize timestamp to UTC, as sqlalchemy doesn't appear to

12
glance/registry/api/v1/images.py
View File

@ -64,9 +64,9 @@ class Controller(object):
# NOTE(markwash): for backwards compatibility, is_public=True for
# admins actually means "treat me as if I'm not an admin and show me
# all my images"
if context.is_admin and filters.get('is_public') is True:
if context.is_admin and params.get('is_public') is True:
context.is_admin = False
del filters['is_public']
del params['is_public']
try:
return self.db_api.image_get_all(context, filters=filters,
**params)
@ -141,6 +141,10 @@ class Controller(object):
'marker': self._get_marker(req),
}
if req.context.is_admin:
# Only admin gets to look for non-public images
params['is_public'] = self._get_is_public(req)
for key, value in params.items():
if value is None:
del params[key]
@ -160,10 +164,6 @@ class Controller(object):
filters = {}
properties = {}
if req.context.is_admin:
# Only admin gets to look for non-public images
filters['is_public'] = self._get_is_public(req)
for param in req.params:
if param in SUPPORTED_FILTERS:
filters[param] = req.params.get(param)

104
glance/tests/functional/db/base.py
View File

@ -763,21 +763,20 @@ class VisibilityTests(object):
def test_unknown_admin_is_public_true(self):
images = self.db_api.image_get_all(self.admin_none_context,
filters={'is_public': True})
is_public=True)
self.assertEquals(len(images), 4)
for i in images:
self.assertTrue(i['is_public'])
def test_unknown_admin_is_public_false(self):
images = self.db_api.image_get_all(self.admin_none_context,
filters={'is_public': False})
is_public=False)
self.assertEquals(len(images), 4)
for i in images:
self.assertFalse(i['is_public'])
def test_unknown_admin_is_public_none(self):
images = self.db_api.image_get_all(self.admin_none_context,
filters={'is_public': None})
images = self.db_api.image_get_all(self.admin_none_context)
self.assertEquals(len(images), 8)
def test_unknown_admin_visibility_public(self):
@ -799,22 +798,20 @@ class VisibilityTests(object):
self.assertEquals(len(images), 8)
def test_known_admin_is_public_true(self):
images = self.db_api.image_get_all(self.admin_context,
filters={'is_public': True})
images = self.db_api.image_get_all(self.admin_context, is_public=True)
self.assertEquals(len(images), 4)
for i in images:
self.assertTrue(i['is_public'])
def test_known_admin_is_public_false(self):
images = self.db_api.image_get_all(self.admin_context,
filters={'is_public': False})
is_public=False)
self.assertEquals(len(images), 4)
for i in images:
self.assertFalse(i['is_public'])
def test_known_admin_is_public_none(self):
images = self.db_api.image_get_all(self.admin_context,
filters={'is_public': None})
images = self.db_api.image_get_all(self.admin_context)
self.assertEquals(len(images), 8)
def test_known_admin_visibility_public(self):
@ -838,20 +835,17 @@ class VisibilityTests(object):
self.assertTrue(i['is_public'])
def test_unknown_user_is_public_true(self):
images = self.db_api.image_get_all(self.none_context,
filters={'is_public': True})
images = self.db_api.image_get_all(self.none_context, is_public=True)
self.assertEquals(len(images), 4)
for i in images:
self.assertTrue(i['is_public'])
def test_unknown_user_is_public_false(self):
images = self.db_api.image_get_all(self.none_context,
filters={'is_public': False})
images = self.db_api.image_get_all(self.none_context, is_public=False)
self.assertEquals(len(images), 0)
def test_unknown_user_is_public_none(self):
images = self.db_api.image_get_all(self.none_context,
filters={'is_public': None})
images = self.db_api.image_get_all(self.none_context)
self.assertEquals(len(images), 4)
for i in images:
self.assertTrue(i['is_public'])
@ -877,21 +871,20 @@ class VisibilityTests(object):
def test_tenant1_is_public_true(self):
images = self.db_api.image_get_all(self.tenant1_context,
filters={'is_public': True})
is_public=True)
self.assertEquals(len(images), 4)
for i in images:
self.assertTrue(i['is_public'])
def test_tenant1_is_public_false(self):
images = self.db_api.image_get_all(self.tenant1_context,
filters={'is_public': False})
is_public=False)
self.assertEquals(len(images), 1)
self.assertFalse(images[0]['is_public'])
self.assertEquals(images[0]['owner'], self.tenant1)
def test_tenant1_is_public_none(self):
images = self.db_api.image_get_all(self.tenant1_context,
filters={'is_public': None})
images = self.db_api.image_get_all(self.tenant1_context)
self.assertEquals(len(images), 5)
for i in images:
if not i['is_public']:
@ -911,6 +904,79 @@ class VisibilityTests(object):
self.assertFalse(images[0]['is_public'])
self.assertEquals(images[0]['owner'], self.tenant1)
def _setup_is_public_red_herring(self):
values = {
'name': 'Red Herring',
'owner': self.tenant1,
'is_public': False,
'properties': {'is_public': 'silly'}
}
fixture = build_image_fixture(**values)
self.db_api.image_create(self.admin_context, fixture)
def test_is_public_is_a_normal_filter_for_admin(self):
self._setup_is_public_red_herring()
images = self.db_api.image_get_all(self.admin_context,
filters={'is_public': 'silly'})
self.assertEqual(len(images), 1)
self.assertEqual(images[0]['name'], 'Red Herring')
def test_is_public_is_a_normal_filter_for_user(self):
self._setup_is_public_red_herring()
images = self.db_api.image_get_all(self.tenant1_context,
filters={'is_public': 'silly'})
self.assertEqual(len(images), 1)
self.assertEqual(images[0]['name'], 'Red Herring')
# NOTE(markwash): the following tests are sanity checks to make sure
# visibility filtering and is_public=(True|False) do not interact in
# unexpected ways. However, using both of the filtering techniques
# simultaneously is not an anticipated use case.
def test_admin_is_public_true_and_visibility_public(self):
images = self.db_api.image_get_all(self.admin_context, is_public=True,
filters={'visibility': 'public'})
self.assertEquals(len(images), 4)
def test_admin_is_public_false_and_visibility_public(self):
images = self.db_api.image_get_all(self.admin_context, is_public=False,
filters={'visibility': 'public'})
self.assertEquals(len(images), 0)
def test_admin_is_public_true_and_visibility_private(self):
images = self.db_api.image_get_all(self.admin_context, is_public=True,
filters={'visibility': 'private'})
self.assertEquals(len(images), 0)
def test_admin_is_public_false_and_visibility_private(self):
images = self.db_api.image_get_all(self.admin_context, is_public=False,
filters={'visibility': 'private'})
self.assertEquals(len(images), 4)
def test_tenant1_is_public_true_and_visibility_public(self):
images = self.db_api.image_get_all(self.tenant1_context,
is_public=True,
filters={'visibility': 'public'})
self.assertEquals(len(images), 4)
def test_tenant1_is_public_false_and_visibility_public(self):
images = self.db_api.image_get_all(self.tenant1_context,
is_public=False,
filters={'visibility': 'public'})
self.assertEquals(len(images), 0)
def test_tenant1_is_public_true_and_visibility_private(self):
images = self.db_api.image_get_all(self.tenant1_context,
is_public=True,
filters={'visibility': 'private'})
self.assertEquals(len(images), 0)
def test_tenant1_is_public_false_and_visibility_private(self):
images = self.db_api.image_get_all(self.tenant1_context,
is_public=False,
filters={'visibility': 'private'})
self.assertEquals(len(images), 1)
class TestMembershipVisibility(base.IsolatedUnitTest):
def setUp(self):

23
glance/tests/unit/test_clients.py
View File

@ -15,6 +15,7 @@
# License for the specific language governing permissions and limitations
# under the License.
import copy
import datetime
from glance.common import config
@ -819,6 +820,28 @@ class TestRegistryClient(base.IsolatedUnitTest):
for image in images:
self.assertEquals('v a', image['properties']['p a'])
def test_get_image_is_public_v1(self):
"""Tests that a detailed call can be filtered by a property"""
extra_fixture = {'id': _gen_uuid(),
'status': 'saving',
'is_public': True,
'disk_format': 'vhd',
'container_format': 'ovf',
'name': 'new name! #123',
'size': 19,
'checksum': None,
'properties': {'is_public': 'avalue'}}
context = copy.copy(self.context)
db_api.image_create(context, extra_fixture)
filters = {'property-is_public': 'avalue'}
images = self.client.get_images_detailed(filters=filters)
self.assertEquals(len(images), 1)
for image in images:
self.assertEquals('avalue', image['properties']['is_public'])
def test_get_image_details_sort_disk_format_asc(self):
"""
Tests that a detailed call returns list of

29
glance/tests/unit/v1/test_api.py
View File

@ -1453,6 +1453,35 @@ class TestRegistryAPI(base.IsolatedUnitTest):
for image in images:
self.assertEqual('v a', image['properties']['prop_123'])
def test_get_details_filter_is_public_property(self):
"""
Tests that the /images/detail registry API returns list of
public images that have a specific custom property
"""
extra_fixture = {'id': _gen_uuid(),
'status': 'active',
'is_public': True,
'disk_format': 'vhd',
'container_format': 'ovf',
'name': 'fake image #3',
'size': 19,
'checksum': None,
'properties': {'is_public': 'avalue'}}
db_api.image_create(self.context, extra_fixture)
req = webob.Request.blank('/images/detail?property-is_public=avalue')
res = req.get_response(self.api)
res_dict = json.loads(res.body)
self.assertEquals(res.status_int, 200)
images = res_dict['images']
self.assertEquals(len(images), 1)
for image in images:
self.assertEqual('avalue', image['properties']['is_public'])
self.assertTrue(image['is_public'])
def test_get_details_filter_public_none(self):
"""
Tests that the /images/detail registry API returns list of