diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 1a1befb7d9..d2ec88264b 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -602,21 +602,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. @@ -858,10 +843,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']) @@ -880,10 +867,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 24fbcee0a9..bdef56d1f3 100644 --- a/nova/block_device.py +++ b/nova/block_device.py @@ -22,6 +22,8 @@ from oslo.config import cfg from nova import exception from nova.openstack.common.gettextutils import _ 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 @@ -73,6 +75,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__() @@ -88,10 +92,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): @@ -134,7 +167,8 @@ class BlockDeviceDict(dict): pass else: - raise exception.InvalidBDMFormat() + raise exception.InvalidBDMFormat( + details="Unrecognized legacy format.") return cls(new_bdm, non_computable_fields) @@ -150,10 +184,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) @@ -257,6 +293,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 89b1c31db9..0068fe7f8e 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -231,8 +231,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 491c951c1a..8a79bf5e63 100644 --- a/nova/tests/api/openstack/compute/test_servers.py +++ b/nova/tests/api/openstack/compute/test_servers.py @@ -2668,8 +2668,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 @@ -2754,6 +2756,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):