From e9fd70332e67810a7d89fbb1ddd73d8832090f61 Mon Sep 17 00:00:00 2001 From: Nikola Dipanov Date: Thu, 30 May 2013 12:49:56 +0200 Subject: [PATCH] Add basic BDM format validation in the API layer Adds the basic sanity checking for the new BDM data format when it's passed in to the servers create method. Also make block_device routines that raise IvalidBDMFormat error add a brief explanation to it that can be reused to send more informative validation errors to the user. More comprehensive validation that will take a look at the request as a whole, and take into consideration all the metadata, will be done in the compute API create method in the subsequent patches. blueprint: improve-block-device-handling Change-Id: Ie4c2c9a219be91faa07106f820a728ca9ac42e1b --- nova/api/openstack/compute/servers.py | 43 +++++------ nova/block_device.py | 74 +++++++++++++++++-- nova/exception.py | 3 +- .../api/openstack/compute/test_servers.py | 38 ++++++++++ nova/tests/test_block_device.py | 54 +++++++++++++- 5 files changed, 177 insertions(+), 35 deletions(-) diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 687ded751f..8074a10c9f 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -601,21 +601,6 @@ class Controller(wsgi.Controller): def _validate_server_name(self, value): self._check_string_length(value, 'Server name', max_length=255) - def _validate_block_device(self, bd): - self._check_string_length(bd['device_name'], - 'Device name', max_length=255) - - if ' ' in bd['device_name']: - msg = _("Device name cannot include spaces.") - raise exc.HTTPBadRequest(explanation=msg) - - if 'volume_size' in bd: - try: - bd['volume_size'] = utils.validate_integer( - bd['volume_size'], 'volume_size', min_value=0) - except exception.InvalidInput as e: - raise exc.HTTPBadRequest(explanation=e.format_message()) - def _get_injected_files(self, personality): """Create a list of injected files from the personality attribute. @@ -857,10 +842,12 @@ class Controller(wsgi.Controller): if self.ext_mgr.is_loaded('os-volumes'): block_device_mapping = server_dict.get('block_device_mapping', []) for bdm in block_device_mapping: - # Ignore empty volume size - if 'volume_size' in bdm and not bdm['volume_size']: - del bdm['volume_size'] - self._validate_block_device(bdm) + try: + block_device.validate_device_name(bdm.get("device_name")) + block_device.validate_and_default_volume_size(bdm) + except exception.InvalidBDMFormat as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + if 'delete_on_termination' in bdm: bdm['delete_on_termination'] = strutils.bool_from_string( bdm['delete_on_termination']) @@ -879,10 +866,20 @@ class Controller(wsgi.Controller): # Assume legacy format legacy_bdm = not bool(block_device_mapping_v2) - # This will also do basic validation of block_devices - block_device_mapping_v2 = [ - block_device.BlockDeviceDict.from_api(bdm_dict) - for bdm_dict in block_device_mapping_v2] + # NOTE (ndipanov) Don't allow empty device_name values + # for now until we can handle it on the + # compute side. + if any('device_name' not in bdm + for bdm in block_device_mapping_v2): + expl = _('Missing device_name in some block devices.') + raise exc.HTTPBadRequest(explanation=expl) + + try: + block_device_mapping_v2 = [ + block_device.BlockDeviceDict.from_api(bdm_dict) + for bdm_dict in block_device_mapping_v2] + except exception.InvalidBDMFormat as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) block_device_mapping = (block_device_mapping or block_device_mapping_v2) diff --git a/nova/block_device.py b/nova/block_device.py index 8ae396756b..4fbcc1b351 100644 --- a/nova/block_device.py +++ b/nova/block_device.py @@ -21,6 +21,8 @@ from oslo.config import cfg from nova import exception from nova.openstack.common import log as logging +from nova.openstack.common import strutils +from nova import utils from nova.virt import driver CONF = cfg.CONF @@ -72,6 +74,8 @@ class BlockDeviceDict(dict): _db_only_fields = (bdm_db_only_fields | bdm_db_inherited_fields) + _required_fields = set(['source_type']) + def __init__(self, bdm_dict=None, do_not_default=None): super(BlockDeviceDict, self).__init__() @@ -87,10 +91,39 @@ class BlockDeviceDict(dict): def _validate(self, bdm_dict): """Basic data format validations.""" - if (not set(key for key, _ in bdm_dict.iteritems()) <= + dict_fields = set(key for key, _ in bdm_dict.iteritems()) + + # Check that there are no bogus fields + if not (dict_fields <= (self._fields | self._db_only_fields)): - raise exception.InvalidBDMFormat() - # TODO(ndipanov): Validate must-have fields! + raise exception.InvalidBDMFormat( + details="Some fields are invalid.") + + if bdm_dict.get('no_device'): + return + + # Check that all required fields are there + if (self._required_fields and + not ((dict_fields & self._required_fields) == + self._required_fields)): + raise exception.InvalidBDMFormat( + details="Some required fields are missing") + + if 'delete_on_termination' in bdm_dict: + bdm_dict['delete_on_termination'] = strutils.bool_from_string( + bdm_dict['delete_on_termination']) + + if bdm_dict.get('device_name') is not None: + validate_device_name(bdm_dict['device_name']) + + validate_and_default_volume_size(bdm_dict) + + if bdm_dict.get('boot_index'): + try: + bdm_dict['boot_index'] = int(bdm_dict['boot_index']) + except ValueError: + raise exception.InvalidBDMFormat( + details="Boot index is invalid.") @classmethod def from_legacy(cls, legacy_bdm): @@ -133,7 +166,8 @@ class BlockDeviceDict(dict): pass else: - raise exception.InvalidBDMFormat() + raise exception.InvalidBDMFormat( + details="Unrecognized legacy format.") return cls(new_bdm, non_computable_fields) @@ -149,10 +183,12 @@ class BlockDeviceDict(dict): device_uuid = api_dict.get('uuid') if source_type not in ('volume', 'image', 'snapshot', 'blank'): - raise exception.InvalidBDMFormat() + raise exception.InvalidBDMFormat( + details="Invalid source_type field.") elif source_type != 'blank': if not device_uuid: - raise exception.InvalidBDMFormat() + raise exception.InvalidBDMFormat( + details="Missing device UUID.") api_dict[source_type + '_id'] = device_uuid api_dict.pop('uuid', None) @@ -256,6 +292,32 @@ def properties_root_device_name(properties): return root_device_name +def validate_device_name(value): + try: + # NOTE (ndipanov): Do not allow empty device names + # until assigning default values + # is supported by nova.compute + utils.check_string_length(value, 'Device name', + min_length=1, max_length=255) + except exception.InvalidInput as e: + raise exception.InvalidBDMFormat( + details="Device name empty or too long.") + + if ' ' in value: + raise exception.InvalidBDMFormat( + details="Device name contains spaces.") + + +def validate_and_default_volume_size(bdm): + if bdm.get('volume_size'): + try: + bdm['volume_size'] = utils.validate_integer( + bdm['volume_size'], 'volume_size', min_value=0) + except exception.InvalidInput as e: + raise exception.InvalidBDMFormat( + details="Invalid volume_size.") + + _ephemeral = re.compile('^ephemeral(\d|[1-9]\d+)$') diff --git a/nova/exception.py b/nova/exception.py index 063980fc31..9e48aeae7e 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -230,8 +230,7 @@ class InvalidBDMVolume(InvalidBDM): class InvalidBDMFormat(InvalidBDM): msg_fmt = _("Block Device Mapping is Invalid: " - "some fields are not recognized, " - "or have invalid values.") + "%(details)s") class InvalidBDMForLegacy(InvalidBDM): diff --git a/nova/tests/api/openstack/compute/test_servers.py b/nova/tests/api/openstack/compute/test_servers.py index ed15f43075..fb35704049 100644 --- a/nova/tests/api/openstack/compute/test_servers.py +++ b/nova/tests/api/openstack/compute/test_servers.py @@ -2667,8 +2667,10 @@ class ServersControllerCreateTest(test.TestCase): self.ext_mgr.extensions = {'os-volumes': 'fake', 'os-block-device-mapping-v2-boot': 'fake'} bdm_v2 = [{'source_type': 'volume', + 'device_name': 'fake_dev', 'uuid': 'fake_vol'}] bdm_v2_expected = [{'source_type': 'volume', + 'device_name': 'fake_dev', 'volume_id': 'fake_vol'}] params = {'block_device_mapping_v2': bdm_v2} old_create = compute_api.API.create @@ -2753,6 +2755,42 @@ class ServersControllerCreateTest(test.TestCase): self.assertRaises(webob.exc.HTTPBadRequest, self._test_create_extra, params) + def test_create_instance_bdm_v2_missing_device_name(self): + self.ext_mgr.extensions = {'os-volumes': 'fake', + 'os-block-device-mapping-v2-boot': 'fake'} + bdm_v2 = [{'source_type': 'volume', + 'uuid': 'fake_vol'}] + params = {'block_device_mapping_v2': bdm_v2} + + def _validate(*args, **kwargs): + pass + + def _validate_bdm(*args, **kwargs): + pass + + self.stubs.Set(block_device.BlockDeviceDict, + '_validate', _validate) + self.stubs.Set(compute_api.API, '_validate_bdm', + _validate_bdm) + + self.assertRaises(webob.exc.HTTPBadRequest, + self._test_create_extra, params) + + def test_create_instance_bdm_v2_validation_error(self): + self.ext_mgr.extensions = {'os-volumes': 'fake', + 'os-block-device-mapping-v2-boot': 'fake'} + bdm_v2 = [{'device_name': 'bogus device'}] + params = {'block_device_mapping_v2': bdm_v2} + + def _validate(*args, **kwargs): + raise exception.InvalidBDMFormat() + + self.stubs.Set(block_device.BlockDeviceDict, + '_validate', _validate) + + self.assertRaises(webob.exc.HTTPBadRequest, + self._test_create_extra, params) + def test_create_instance_with_user_data_enabled(self): self.ext_mgr.extensions = {'os-user-data': 'fake'} user_data = 'fake' diff --git a/nova/tests/test_block_device.py b/nova/tests/test_block_device.py index 669d197bd9..f2765c07e2 100644 --- a/nova/tests/test_block_device.py +++ b/nova/tests/test_block_device.py @@ -225,10 +225,15 @@ class TestBlockDeviceDict(test.TestCase): ] def test_init(self): + def fake_validate(obj, dct): + pass + self.stubs.Set(block_device.BlockDeviceDict, '_fields', set(['field1', 'field2'])) self.stubs.Set(block_device.BlockDeviceDict, '_db_only_fields', set(['db_field1', 'db_field2'])) + self.stubs.Set(block_device.BlockDeviceDict, '_validate', + fake_validate) # Make sure db fields are not picked up if they are not # in the original dict @@ -256,12 +261,53 @@ class TestBlockDeviceDict(test.TestCase): self.assertFalse('db_field1' in dev_dict) self.assertFalse('db_field2'in dev_dict) - # Assert basic validation works - # NOTE (ndipanov): Move to separate test once we have - # more complex validations in place + def test_validate(self): self.assertRaises(exception.InvalidBDMFormat, block_device.BlockDeviceDict, - {'field1': 'foo', 'bogus_field': 'lame_val'}) + {'bogus_field': 'lame_val'}) + + lame_bdm = dict(self.new_mapping[2]) + del lame_bdm['source_type'] + self.assertRaises(exception.InvalidBDMFormat, + block_device.BlockDeviceDict, + lame_bdm) + + lame_bdm['no_device'] = True + block_device.BlockDeviceDict(lame_bdm) + + lame_dev_bdm = dict(self.new_mapping[2]) + lame_dev_bdm['device_name'] = "not a valid name" + self.assertRaises(exception.InvalidBDMFormat, + block_device.BlockDeviceDict, + lame_dev_bdm) + + lame_dev_bdm['device_name'] = "" + self.assertRaises(exception.InvalidBDMFormat, + block_device.BlockDeviceDict, + lame_dev_bdm) + + cool_volume_size_bdm = dict(self.new_mapping[2]) + cool_volume_size_bdm['volume_size'] = '42' + cool_volume_size_bdm = block_device.BlockDeviceDict( + cool_volume_size_bdm) + self.assertEquals(cool_volume_size_bdm['volume_size'], 42) + + lame_volume_size_bdm = dict(self.new_mapping[2]) + lame_volume_size_bdm['volume_size'] = 'some_non_int_string' + self.assertRaises(exception.InvalidBDMFormat, + block_device.BlockDeviceDict, + lame_volume_size_bdm) + + truthy_bdm = dict(self.new_mapping[2]) + truthy_bdm['delete_on_termination'] = '1' + truthy_bdm = block_device.BlockDeviceDict(truthy_bdm) + self.assertEquals(truthy_bdm['delete_on_termination'], True) + + verbose_bdm = dict(self.new_mapping[2]) + verbose_bdm['boot_index'] = 'first' + self.assertRaises(exception.InvalidBDMFormat, + block_device.BlockDeviceDict, + verbose_bdm) def test_from_legacy(self): for legacy, new in zip(self.legacy_mapping, self.new_mapping):