diff --git a/.mailmap b/.mailmap index 04abdc4e..1a348b92 100644 --- a/.mailmap +++ b/.mailmap @@ -1,5 +1,4 @@ -# Format is: -# -# +# "man git-shortlog" for reference +David Koo diff --git a/glanceclient/common/utils.py b/glanceclient/common/utils.py index 8993cba7..6cdc364a 100644 --- a/glanceclient/common/utils.py +++ b/glanceclient/common/utils.py @@ -325,3 +325,13 @@ def strip_version(endpoint): if re.match('v\d+\.?\d*', url_bits[-1]): endpoint = '/'.join(url_bits[:-1]) return endpoint + + +def print_image(image_obj, max_col_width=None): + ignore = ['self', 'access', 'file', 'schema'] + image = dict([item for item in six.iteritems(image_obj) + if item[0] not in ignore]) + if str(max_col_width).isdigit(): + print_dict(image, max_column_width=max_col_width) + else: + print_dict(image) diff --git a/glanceclient/v2/images.py b/glanceclient/v2/images.py index ab07a6be..cb57143c 100644 --- a/glanceclient/v2/images.py +++ b/glanceclient/v2/images.py @@ -13,12 +13,14 @@ # License for the specific language governing permissions and limitations # under the License. +import json import six from six.moves.urllib import parse import warlock from glanceclient.common import utils +from glanceclient import exc from glanceclient.openstack.common import strutils DEFAULT_PAGE_SIZE = 20 @@ -170,3 +172,94 @@ class Controller(object): # we need to fetch the image again to get a clean history. This is # an obvious optimization for warlock return self.get(image_id) + + def _get_image_with_locations_or_fail(self, image_id): + image = self.get(image_id) + if getattr(image, 'locations', None) is None: + raise exc.HTTPBadRequest('The administrator has disabled ' + 'API access to image locations') + return image + + def _send_image_update_request(self, image_id, patch_body): + url = '/v2/images/%s' % image_id + hdrs = {'Content-Type': 'application/openstack-images-v2.1-json-patch'} + self.http_client.raw_request('PATCH', url, + headers=hdrs, + body=json.dumps(patch_body)) + + def add_location(self, image_id, url, metadata): + """Add a new location entry to an image's list of locations. + + It is an error to add a URL that is already present in the list of + locations. + + :param image_id: ID of image to which the location is to be added. + :param url: URL of the location to add. + :param metadata: Metadata associated with the location. + :returns: The updated image + """ + image = self._get_image_with_locations_or_fail(image_id) + url_list = [l['url'] for l in image.locations] + if url in url_list: + err_str = 'A location entry at %s already exists' % url + raise exc.HTTPConflict(err_str) + + add_patch = [{'op': 'add', 'path': '/locations/-', + 'value': {'url': url, 'metadata': metadata}}] + self._send_image_update_request(image_id, add_patch) + return self.get(image_id) + + def delete_locations(self, image_id, url_set): + """Remove one or more location entries of an image. + + :param image_id: ID of image from which locations are to be removed. + :param url_set: set of URLs of location entries to remove. + :returns: None + """ + image = self._get_image_with_locations_or_fail(image_id) + current_urls = [l['url'] for l in image.locations] + + missing_locs = url_set.difference(set(current_urls)) + if missing_locs: + raise exc.HTTPNotFound('Unknown URL(s): %s' % list(missing_locs)) + + # NOTE: warlock doesn't generate the most efficient patch for remove + # operations (it shifts everything up and deletes the tail elements) so + # we do it ourselves. + url_indices = [current_urls.index(url) for url in url_set] + url_indices.sort(reverse=True) + patches = [{'op': 'remove', 'path': '/locations/%s' % url_idx} + for url_idx in url_indices] + self._send_image_update_request(image_id, patches) + + def update_location(self, image_id, url, metadata): + """Update an existing location entry in an image's list of locations. + + The URL specified must be already present in the image's list of + locations. + + :param image_id: ID of image whose location is to be updated. + :param url: URL of the location to update. + :param metadata: Metadata associated with the location. + :returns: The updated image + """ + image = self._get_image_with_locations_or_fail(image_id) + url_map = dict([(l['url'], l) for l in image.locations]) + if url not in url_map: + raise exc.HTTPNotFound('Unknown URL: %s' % url) + + if url_map[url]['metadata'] == metadata: + return image + + # NOTE: The server (as of now) doesn't support modifying individual + # location entries. So we must: + # 1. Empty existing list of locations. + # 2. Send another request to set 'locations' to the new list + # of locations. + url_map[url]['metadata'] = metadata + patches = [{'op': 'replace', + 'path': '/locations', + 'value': p} for p in ([], list(url_map.values()))] + self._send_image_update_request(image_id, patches) + + return self.get(image_id) diff --git a/glanceclient/v2/shell.py b/glanceclient/v2/shell.py index 05c266a2..4d0e3608 100644 --- a/glanceclient/v2/shell.py +++ b/glanceclient/v2/shell.py @@ -19,7 +19,6 @@ from glanceclient import exc import json import os from os.path import expanduser -import six IMAGE_SCHEMA = None @@ -54,14 +53,11 @@ def do_image_create(gc, args): fields[key] = value image = gc.images.create(**fields) - ignore = ['self', 'access', 'file', 'schema'] - image = dict([item for item in six.iteritems(image) - if item[0] not in ignore]) - utils.print_dict(image) + utils.print_image(image) @utils.arg('id', metavar='', help='ID of image to update.') -@utils.schema_args(get_image_schema, omit=['id']) +@utils.schema_args(get_image_schema, omit=['id', 'locations']) @utils.arg('--property', metavar="", action='append', default=[], help=('Arbitrary property to associate with image.' ' May be used multiple times.')) @@ -85,10 +81,7 @@ def do_image_update(gc, args): image_id = fields.pop('id') image = gc.images.update(image_id, remove_properties, **fields) - ignore = ['self', 'access', 'file', 'schema'] - image = dict([item for item in six.iteritems(image) - if item[0] not in ignore]) - utils.print_dict(image) + utils.print_image(image) @utils.arg('--page-size', metavar='', default=None, type=int, @@ -125,9 +118,7 @@ def do_image_show(gc, args): """Describe a specific image.""" image = gc.images.get(args.id) ignore = ['self', 'access', 'file', 'schema'] - image = dict([item for item in six.iteritems(image) if item[0] not in - ignore]) - utils.print_dict(image, max_column_width=int(args.max_column_width)) + utils.print_image(image, int(args.max_column_width)) @utils.arg('--image-id', metavar='', required=True, @@ -268,3 +259,48 @@ def do_image_tag_delete(gc, args): utils.exit('Unable to delete tag. Specify image_id and tag_value') else: gc.image_tags.delete(args.image_id, args.tag_value) + + +@utils.arg('--url', metavar='', required=True, + help='URL of location to add.') +@utils.arg('--metadata', metavar='', default='{}', + help=('Metadata associated with the location. ' + 'Must be a valid JSON object (default: %(default)s)')) +@utils.arg('id', metavar='', + help='ID of image to which the location is to be added.') +def do_location_add(gc, args): + """Add a location (and related metadata) to an image.""" + try: + metadata = json.loads(args.metadata) + except ValueError: + utils.exit('Metadata is not a valid JSON object.') + else: + image = gc.images.add_location(args.id, args.url, metadata) + utils.print_dict(image) + + +@utils.arg('--url', metavar='', action='append', required=True, + help='URL of location to remove. May be used multiple times.') +@utils.arg('id', metavar='', + help='ID of image whose locations are to be removed.') +def do_location_delete(gc, args): + """Remove locations (and related metadata) from an image.""" + image = gc.images.delete_locations(args.id, set(args.url)) + + +@utils.arg('--url', metavar='', required=True, + help='URL of location to update.') +@utils.arg('--metadata', metavar='', default='{}', + help=('Metadata associated with the location. ' + 'Must be a valid JSON object (default: %(default)s)')) +@utils.arg('id', metavar='', + help='ID of image whose location is to be updated.') +def do_location_update(gc, args): + """Update metadata of an image's location.""" + try: + metadata = json.loads(args.metadata) + except ValueError: + utils.exit('Metadata is not a valid JSON object.') + else: + image = gc.images.update_location(args.id, args.url, metadata) + utils.print_dict(image) diff --git a/tests/v2/test_images.py b/tests/v2/test_images.py index b76267c2..149e096b 100644 --- a/tests/v2/test_images.py +++ b/tests/v2/test_images.py @@ -14,11 +14,13 @@ # under the License. import errno +import json import testtools import six import warlock +from glanceclient import exc from glanceclient.v2 import images from tests import utils @@ -303,12 +305,43 @@ fixtures = { {'images': []}, ), }, + '/v2/images/a2b83adc-888e-11e3-8872-78acc0b951d8': { + 'GET': ( + {}, + { + 'id': 'a2b83adc-888e-11e3-8872-78acc0b951d8', + 'name': 'image-location-tests', + 'locations': [{u'url': u'http://foo.com/', + u'metadata': {u'foo': u'foometa'}}, + {u'url': u'http://bar.com/', + u'metadata': {u'bar': u'barmeta'}}], + }, + ), + 'PATCH': ( + {}, + '', + ) + }, } fake_schema = { 'name': 'image', - 'properties': {'id': {}, 'name': {}}, + 'properties': { + 'id': {}, + 'name': {}, + 'locations': { + 'type': 'array', + 'items': { + 'type': 'object', + 'properties': { + 'metadata': {'type': 'object'}, + 'url': {'type': 'string'}, + }, + 'required': ['url', 'metadata'], + }, + }, + }, 'additionalProperties': {'type': 'string'} } FakeModel = warlock.model_factory(fake_schema) @@ -626,3 +659,105 @@ class TestController(testtools.TestCase): params = {'name': 'pong', 'bad_prop': False} with testtools.ExpectedException(TypeError): self.controller.update(image_id, **params) + + def test_location_ops_when_server_disabled_location_ops(self): + # Location operations should not be allowed if server has not + # enabled location related operations + image_id = '3a4560a1-e585-443e-9b39-553b46ec92d1' + estr = 'The administrator has disabled API access to image locations' + url = 'http://bar.com/' + meta = {'bar': 'barmeta'} + + e = self.assertRaises(exc.HTTPBadRequest, + self.controller.add_location, + image_id, url, meta) + self.assertTrue(estr in str(e)) + + e = self.assertRaises(exc.HTTPBadRequest, + self.controller.delete_locations, + image_id, set([url])) + self.assertTrue(estr in str(e)) + + e = self.assertRaises(exc.HTTPBadRequest, + self.controller.update_location, + image_id, url, meta) + self.assertTrue(estr in str(e)) + + def _empty_get(self, image_id): + return ('GET', '/v2/images/%s' % image_id, {}, None) + + def _patch_req(self, image_id, patch_body): + c_type = 'application/openstack-images-v2.1-json-patch' + return ('PATCH', + '/v2/images/%s' % image_id, + {'Content-Type': c_type}, + json.dumps(patch_body)) + + def test_add_location(self): + image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8' + new_loc = {'url': 'http://spam.com/', 'metadata': {'spam': 'ham'}} + add_patch = {'path': '/locations/-', 'value': new_loc, 'op': 'add'} + image = self.controller.add_location(image_id, **new_loc) + self.assertEqual(self.api.calls, [ + self._empty_get(image_id), + self._patch_req(image_id, [add_patch]), + self._empty_get(image_id) + ]) + + def test_add_duplicate_location(self): + image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8' + new_loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'newfoo'}} + err_str = 'A location entry at %s already exists' % new_loc['url'] + + err = self.assertRaises(exc.HTTPConflict, + self.controller.add_location, + image_id, **new_loc) + self.assertIn(err_str, str(err)) + + def test_remove_location(self): + image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8' + url_set = set(['http://foo.com/', 'http://bar.com/']) + del_patches = [{'path': '/locations/1', 'op': 'remove'}, + {'path': '/locations/0', 'op': 'remove'}] + image = self.controller.delete_locations(image_id, url_set) + self.assertEqual(self.api.calls, [ + self._empty_get(image_id), + self._patch_req(image_id, del_patches) + ]) + + def test_remove_missing_location(self): + image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8' + url_set = set(['http://spam.ham/']) + err_str = 'Unknown URL(s): %s' % list(url_set) + + err = self.assertRaises(exc.HTTPNotFound, + self.controller.delete_locations, + image_id, url_set) + self.assertTrue(err_str in str(err)) + + def test_update_location(self): + image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8' + new_loc = {'url': 'http://foo.com/', 'metadata': {'spam': 'ham'}} + fixture_idx = '/v2/images/%s' % (image_id) + orig_locations = fixtures[fixture_idx]['GET'][1]['locations'] + loc_map = dict([(l['url'], l) for l in orig_locations]) + loc_map[new_loc['url']] = new_loc + mod_patch = [{'path': '/locations', 'op': 'replace', + 'value': []}, + {'path': '/locations', 'op': 'replace', + 'value': list(loc_map.values())}] + image = self.controller.update_location(image_id, **new_loc) + self.assertEqual(self.api.calls, [ + self._empty_get(image_id), + self._patch_req(image_id, mod_patch), + self._empty_get(image_id) + ]) + + def test_update_missing_location(self): + image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8' + new_loc = {'url': 'http://spam.com/', 'metadata': {'spam': 'ham'}} + err_str = 'Unknown URL: %s' % new_loc['url'] + err = self.assertRaises(exc.HTTPNotFound, + self.controller.update_location, + image_id, **new_loc) + self.assertTrue(err_str in str(err)) diff --git a/tests/v2/test_shell_v2.py b/tests/v2/test_shell_v2.py index fb541f84..e5d4095f 100644 --- a/tests/v2/test_shell_v2.py +++ b/tests/v2/test_shell_v2.py @@ -14,6 +14,7 @@ # License for the specific language governing permissions and limitations # under the License. +import json import mock import six import testtools @@ -53,7 +54,7 @@ class ShellV2Test(testtools.TestCase): utils.print_dict = mock.Mock() utils.save_image = mock.Mock() - def _test_with_few_arguments(self, func, func_args, err_msg): + def assert_exits_with_msg(self, func, func_args, err_msg): with mock.patch.object(utils, 'exit') as mocked_utils_exit: mocked_utils_exit.return_value = '%s' % err_msg @@ -207,6 +208,59 @@ class ShellV2Test(testtools.TestCase): utils.print_dict.assert_called_once_with({ 'id': 'pass', 'name': 'IMG-01', 'disk_format': 'vhd'}) + def test_do_location_add_update_with_invalid_json_metadata(self): + args = self._make_args({'id': 'pass', + 'url': 'http://foo/bar', + 'metadata': '{1, 2, 3}'}) + self.assert_exits_with_msg(test_shell.do_location_add, + args, + 'Metadata is not a valid JSON object.') + self.assert_exits_with_msg(test_shell.do_location_update, + args, + 'Metadata is not a valid JSON object.') + + def test_do_location_add(self): + gc = self.gc + loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'bar'}} + args = self._make_args({'id': 'pass', + 'url': loc['url'], + 'metadata': json.dumps(loc['metadata'])}) + with mock.patch.object(gc.images, 'add_location') as mocked_addloc: + expect_image = {'id': 'pass', 'locations': [loc]} + mocked_addloc.return_value = expect_image + + test_shell.do_location_add(self.gc, args) + mocked_addloc.assert_called_once_with('pass', + loc['url'], + loc['metadata']) + utils.print_dict.assert_called_once_with(expect_image) + + def test_do_location_delete(self): + gc = self.gc + loc_set = set(['http://foo/bar', 'http://spam/ham']) + args = self._make_args({'id': 'pass', 'url': loc_set}) + + with mock.patch.object(gc.images, 'delete_locations') as mocked_rmloc: + expect_image = {'id': 'pass', 'locations': []} + test_shell.do_location_delete(self.gc, args) + mocked_rmloc.assert_called_once_with('pass', loc_set) + + def test_do_location_update(self): + gc = self.gc + loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'bar'}} + args = self._make_args({'id': 'pass', + 'url': loc['url'], + 'metadata': json.dumps(loc['metadata'])}) + with mock.patch.object(gc.images, 'update_location') as mocked_modloc: + expect_image = {'id': 'pass', 'locations': [loc]} + mocked_modloc.return_value = expect_image + + test_shell.do_location_update(self.gc, args) + mocked_modloc.assert_called_once_with('pass', + loc['url'], + loc['metadata']) + utils.print_dict.assert_called_once_with(expect_image) + def test_do_explain(self): input = { 'page_size': 18, @@ -305,9 +359,9 @@ class ShellV2Test(testtools.TestCase): args = self._make_args({'image_id': None, 'member_id': 'MEM-01'}) msg = 'Unable to create member. Specify image_id and member_id' - self._test_with_few_arguments(func=test_shell.do_member_create, - func_args=args, - err_msg=msg) + self.assert_exits_with_msg(func=test_shell.do_member_create, + func_args=args, + err_msg=msg) def test_do_member_update(self): input = { @@ -335,9 +389,9 @@ class ShellV2Test(testtools.TestCase): msg = 'Unable to update member. Specify image_id, member_id' \ ' and member_status' - self._test_with_few_arguments(func=test_shell.do_member_update, - func_args=args, - err_msg=msg) + self.assert_exits_with_msg(func=test_shell.do_member_update, + func_args=args, + err_msg=msg) def test_do_member_delete(self): args = self._make_args({'image_id': 'IMG-01', 'member_id': 'MEM-01'}) @@ -350,9 +404,9 @@ class ShellV2Test(testtools.TestCase): args = self._make_args({'image_id': None, 'member_id': 'MEM-01'}) msg = 'Unable to delete member. Specify image_id and member_id' - self._test_with_few_arguments(func=test_shell.do_member_delete, - func_args=args, - err_msg=msg) + self.assert_exits_with_msg(func=test_shell.do_member_delete, + func_args=args, + err_msg=msg) def test_image_tag_update(self): args = self._make_args({'image_id': 'IMG-01', 'tag_value': 'tag01'}) @@ -368,9 +422,9 @@ class ShellV2Test(testtools.TestCase): args = self._make_args({'image_id': None, 'tag_value': 'tag01'}) msg = 'Unable to update tag. Specify image_id and tag_value' - self._test_with_few_arguments(func=test_shell.do_image_tag_update, - func_args=args, - err_msg=msg) + self.assert_exits_with_msg(func=test_shell.do_image_tag_update, + func_args=args, + err_msg=msg) def test_image_tag_delete(self): args = self._make_args({'image_id': 'IMG-01', 'tag_value': 'tag01'}) @@ -385,6 +439,6 @@ class ShellV2Test(testtools.TestCase): args = self._make_args({'image_id': 'IMG-01', 'tag_value': None}) msg = 'Unable to delete tag. Specify image_id and tag_value' - self._test_with_few_arguments(func=test_shell.do_image_tag_delete, - func_args=args, - err_msg=msg) + self.assert_exits_with_msg(func=test_shell.do_image_tag_delete, + func_args=args, + err_msg=msg)