From 8d118ccedc7e0544ec21e1fbb7f1b8b3a4f03715 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Fri, 13 Mar 2015 17:47:09 +0100 Subject: [PATCH] Require disk and container format on image-create Currently glanceclient doesn't enforce disk-format or container-format presence in the command line on image-create when providing image data (with --file, --location, --copy-from), which means that the POST request is made with the whole image and error is reported by Glance API. This post enforces presence of those arguments when image data is provided so we can get the error quicker and avoid sending unnecessary data over the network. Change-Id: I5914fa9cfef190a028b374005adbf3a804b1b677 Closes-Bug: #1309272 --- glanceclient/common/utils.py | 45 ++++++++ glanceclient/tests/unit/v1/test_shell.py | 44 ++++++++ glanceclient/tests/unit/v2/test_shell_v2.py | 107 +++++++++++++++++++- glanceclient/v1/shell.py | 2 + glanceclient/v2/shell.py | 2 + 5 files changed, 198 insertions(+), 2 deletions(-) diff --git a/glanceclient/common/utils.py b/glanceclient/common/utils.py index 8daaffff..304fcd46 100644 --- a/glanceclient/common/utils.py +++ b/glanceclient/common/utils.py @@ -36,11 +36,15 @@ from oslo_utils import encodeutils from oslo_utils import strutils import prettytable +from glanceclient import _i18n from glanceclient import exc +_ = _i18n._ + _memoized_property_lock = threading.Lock() SENSITIVE_HEADERS = ('X-Auth-Token', ) +REQUIRED_FIELDS_ON_DATA = ('disk_format', 'container_format') # Decorator for cli-args @@ -53,6 +57,47 @@ def arg(*args, **kwargs): return _decorator +def on_data_require_fields(data_fields, required=REQUIRED_FIELDS_ON_DATA): + """Decorator to check commands' validity + + This decorator checks that required fields are present when image + data has been supplied via command line arguments or via stdin + + On error throws CommandError exception with meaningful message. + + :param data_fields: Which fields' presence imply image data + :type data_fields: iter + :param required: Required fields + :type required: iter + :return: function decorator + """ + + def args_decorator(func): + def prepare_fields(fields): + args = ('--' + x.replace('_', '-') for x in fields) + return ', '.join(args) + + def func_wrapper(gc, args): + # Set of arguments with data + fields = set(a[0] for a in vars(args).items() if a[1]) + + # Fields the conditional requirements depend on + present = fields.intersection(data_fields) + + # How many conditional requirements are missing + missing = set(required) - fields + + # We use get_data_file to check if data is provided in stdin + if (present or get_data_file(args)) and missing: + msg = (_("error: Must provide %(req)s when using %(opt)s.") % + {'req': prepare_fields(missing), + 'opt': prepare_fields(present) or 'stdin'}) + raise exc.CommandError(msg) + return func(gc, args) + return func_wrapper + return args_decorator + + def schema_args(schema_getter, omit=None): omit = omit or [] typemap = { diff --git a/glanceclient/tests/unit/v1/test_shell.py b/glanceclient/tests/unit/v1/test_shell.py index a3a41c24..dd3e3cf1 100644 --- a/glanceclient/tests/unit/v1/test_shell.py +++ b/glanceclient/tests/unit/v1/test_shell.py @@ -232,6 +232,9 @@ class ShellInvalidEndpointandParameterTest(utils.TestCase): 'OS_IMAGE_URL': 'http://is.invalid'} self.shell = shell.OpenStackImagesShell() + self.patched = mock.patch('glanceclient.common.utils.get_data_file', + autospec=True, return_value=None) + self.mock_get_data_file = self.patched.start() self.gc = self._mock_glance_client() @@ -254,6 +257,7 @@ class ShellInvalidEndpointandParameterTest(utils.TestCase): def tearDown(self): super(ShellInvalidEndpointandParameterTest, self).tearDown() os.environ = self.old_environment + self.patched.stop() def run_command(self, cmd): self.shell.main(cmd.split()) @@ -323,6 +327,46 @@ class ShellInvalidEndpointandParameterTest(utils.TestCase): SystemExit, self.run_command, 'image-create --min-disk 10gb') + @mock.patch('sys.stderr') + def test_image_create_missing_disk_format(self, __): + # We test for all possible sources + for origin in ('--file', '--location', '--copy-from'): + e = self.assertRaises(exc.CommandError, self.run_command, + '--os-image-api-version 1 image-create ' + + origin + ' fake_src --container-format bare') + self.assertEqual('error: Must provide --disk-format when using ' + + origin + '.', e.message) + + @mock.patch('sys.stderr') + def test_image_create_missing_container_format(self, __): + # We test for all possible sources + for origin in ('--file', '--location', '--copy-from'): + e = self.assertRaises(exc.CommandError, self.run_command, + '--os-image-api-version 1 image-create ' + + origin + ' fake_src --disk-format qcow2') + self.assertEqual('error: Must provide --container-format when ' + 'using ' + origin + '.', e.message) + + @mock.patch('sys.stderr') + def test_image_create_missing_container_format_stdin_data(self, __): + # Fake that get_data_file method returns data + self.mock_get_data_file.return_value = six.StringIO() + e = self.assertRaises(exc.CommandError, self.run_command, + '--os-image-api-version 1 image-create' + ' --disk-format qcow2') + self.assertEqual('error: Must provide --container-format when ' + 'using stdin.', e.message) + + @mock.patch('sys.stderr') + def test_image_create_missing_disk_format_stdin_data(self, __): + # Fake that get_data_file method returns data + self.mock_get_data_file.return_value = six.StringIO() + e = self.assertRaises(exc.CommandError, self.run_command, + '--os-image-api-version 1 image-create' + ' --container-format bare') + self.assertEqual('error: Must provide --disk-format when using stdin.', + e.message) + @mock.patch('sys.stderr') def test_image_update_invalid_size_parameter(self, __): self.assertRaises( diff --git a/glanceclient/tests/unit/v2/test_shell_v2.py b/glanceclient/tests/unit/v2/test_shell_v2.py index faa12a76..e2678af6 100644 --- a/glanceclient/tests/unit/v2/test_shell_v2.py +++ b/glanceclient/tests/unit/v2/test_shell_v2.py @@ -16,18 +16,73 @@ import json import mock import os +import six import tempfile import testtools from glanceclient.common import utils +from glanceclient import exc +from glanceclient import shell + +# NOTE(geguileo): This is very nasty, but I can't find a better way to set +# command line arguments in glanceclient.v2.shell.do_image_create that are +# set by decorator utils.schema_args while preserving the spirits of the test + +# Backup original decorator +original_schema_args = utils.schema_args + + +# Set our own decorator that calls the original but with simulated schema +def schema_args(schema_getter, omit=None): + global original_schema_args + # We only add the 2 arguments that are required by image-create + my_schema_getter = lambda: { + 'properties': { + 'container_format': { + 'enum': [None, 'ami', 'ari', 'aki', 'bare', 'ovf', 'ova'], + 'type': 'string', + 'description': 'Format of the container'}, + 'disk_format': { + 'enum': [None, 'ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', + 'qcow2', 'vdi', 'iso'], + 'type': 'string', + 'description': 'Format of the disk'}, + 'location': {'type': 'string'}, + 'locations': {'type': 'string'}, + 'copy_from': {'type': 'string'}}} + return original_schema_args(my_schema_getter, omit) +utils.schema_args = schema_args + from glanceclient.v2 import shell as test_shell +# Return original decorator. +utils.schema_args = original_schema_args + class ShellV2Test(testtools.TestCase): def setUp(self): super(ShellV2Test, self).setUp() self._mock_utils() self.gc = self._mock_glance_client() + self.shell = shell.OpenStackImagesShell() + os.environ = { + 'OS_USERNAME': 'username', + 'OS_PASSWORD': 'password', + 'OS_TENANT_ID': 'tenant_id', + 'OS_TOKEN_ID': 'test', + 'OS_AUTH_URL': 'http://127.0.0.1:5000/v2.0/', + 'OS_AUTH_TOKEN': 'pass', + 'OS_IMAGE_API_VERSION': '1', + 'OS_REGION_NAME': 'test', + 'OS_IMAGE_URL': 'http://is.invalid'} + self.shell = shell.OpenStackImagesShell() + self.patched = mock.patch('glanceclient.common.utils.get_data_file', + autospec=True, return_value=None) + self.mock_get_data_file = self.patched.start() + + def tearDown(self): + super(ShellV2Test, self).tearDown() + self.patched.stop() def _make_args(self, args): # NOTE(venkatesh): this conversion from a dict to an object @@ -59,6 +114,49 @@ class ShellV2Test(testtools.TestCase): mocked_utils_exit.assert_called_once_with(err_msg) + def _run_command(self, cmd): + self.shell.main(cmd.split()) + + @mock.patch('sys.stderr') + def test_image_create_missing_disk_format(self, __): + # We test for all possible sources + for origin in ('--file', '--location', '--copy-from'): + e = self.assertRaises(exc.CommandError, self._run_command, + '--os-image-api-version 2 image-create ' + + origin + ' fake_src --container-format bare') + self.assertEqual('error: Must provide --disk-format when using ' + + origin + '.', e.message) + + @mock.patch('sys.stderr') + def test_image_create_missing_container_format(self, __): + # We test for all possible sources + for origin in ('--file', '--location', '--copy-from'): + e = self.assertRaises(exc.CommandError, self._run_command, + '--os-image-api-version 2 image-create ' + + origin + ' fake_src --disk-format qcow2') + self.assertEqual('error: Must provide --container-format when ' + 'using ' + origin + '.', e.message) + + @mock.patch('sys.stderr') + def test_image_create_missing_container_format_stdin_data(self, __): + # Fake that get_data_file method returns data + self.mock_get_data_file.return_value = six.StringIO() + e = self.assertRaises(exc.CommandError, self._run_command, + '--os-image-api-version 2 image-create' + ' --disk-format qcow2') + self.assertEqual('error: Must provide --container-format when ' + 'using stdin.', e.message) + + @mock.patch('sys.stderr') + def test_image_create_missing_disk_format_stdin_data(self, __): + # Fake that get_data_file method returns data + self.mock_get_data_file.return_value = six.StringIO() + e = self.assertRaises(exc.CommandError, self._run_command, + '--os-image-api-version 2 image-create' + ' --container-format bare') + self.assertEqual('error: Must provide --disk-format when using stdin.', + e.message) + def test_do_image_list(self): input = { 'limit': None, @@ -261,6 +359,7 @@ class ShellV2Test(testtools.TestCase): 'container_format': 'bare'}) def test_do_image_create_with_file(self): + self.mock_get_data_file.return_value = six.StringIO() try: file_name = None with open(tempfile.mktemp(), 'w+') as f: @@ -305,7 +404,9 @@ class ShellV2Test(testtools.TestCase): def test_do_image_create_with_user_props(self, mock_stdin): args = self._make_args({'name': 'IMG-01', 'property': ['myprop=myval'], - 'file': None}) + 'file': None, + 'container_format': 'bare', + 'disk_format': 'qcow2'}) with mock.patch.object(self.gc.images, 'create') as mocked_create: ignore_fields = ['self', 'access', 'file', 'schema'] expect_image = dict([(field, field) for field in ignore_fields]) @@ -320,7 +421,9 @@ class ShellV2Test(testtools.TestCase): test_shell.do_image_create(self.gc, args) mocked_create.assert_called_once_with(name='IMG-01', - myprop='myval') + myprop='myval', + container_format='bare', + disk_format='qcow2') utils.print_dict.assert_called_once_with({ 'id': 'pass', 'name': 'IMG-01', 'myprop': 'myval'}) diff --git a/glanceclient/v1/shell.py b/glanceclient/v1/shell.py index 83b85762..c28dc23f 100644 --- a/glanceclient/v1/shell.py +++ b/glanceclient/v1/shell.py @@ -32,6 +32,7 @@ import glanceclient.v1.images CONTAINER_FORMATS = 'Acceptable formats: ami, ari, aki, bare, and ovf.' DISK_FORMATS = ('Acceptable formats: ami, ari, aki, vhd, vmdk, raw, ' 'qcow2, vdi, and iso.') +DATA_FIELDS = ('location', 'copy_from', 'file') _bool_strict = functools.partial(strutils.bool_from_string, strict=True) @@ -221,6 +222,7 @@ def do_image_download(gc, args): help='Print image size in a human-friendly format.') @utils.arg('--progress', action='store_true', default=False, help='Show upload progress bar.') +@utils.on_data_require_fields(DATA_FIELDS) def do_image_create(gc, args): """Create a new image.""" # Filter out None values diff --git a/glanceclient/v2/shell.py b/glanceclient/v2/shell.py index 2f6122a0..deea24b4 100644 --- a/glanceclient/v2/shell.py +++ b/glanceclient/v2/shell.py @@ -27,6 +27,7 @@ import os MEMBER_STATUS_VALUES = image_members.MEMBER_STATUS_VALUES IMAGE_SCHEMA = None +DATA_FIELDS = ('location', 'copy_from', 'file') def get_image_schema(): @@ -55,6 +56,7 @@ def get_image_schema(): 'to the client via stdin.') @utils.arg('--progress', action='store_true', default=False, help='Show upload progress bar.') +@utils.on_data_require_fields(DATA_FIELDS) def do_image_create(gc, args): """Create a new image.""" schema = gc.schemas.get("image")