From 20419283cae4109adc3765016dfcb38aa78c12b8 Mon Sep 17 00:00:00 2001 From: wangxiyuan Date: Tue, 18 Jul 2017 18:52:46 +0800 Subject: [PATCH] Add input format check for qos_spec update If the input format for qos_spec is none string value in dict, Cinder will raise 500 error. This patch added the format check and raised 400 InvalidInput error to users. We also should update the creation process of qos_spec, to move the validation to qos_specs.py as Eric suggested. Co-Authored-By: wanghao Change-Id: I5cb68af74c68cd53bc3b84e705ca3472859c1ec3 Closes-bug: #1704988 Closes-Bug: #1710805 --- cinder/api/contrib/qos_specs_manage.py | 3 --- .../unit/api/contrib/test_qos_specs_manage.py | 16 +++++++++++++--- cinder/tests/unit/test_qos_specs.py | 2 +- .../unit/volume/drivers/ibm/test_storwize_svc.py | 4 ++-- cinder/volume/qos_specs.py | 7 +++++++ 5 files changed, 23 insertions(+), 9 deletions(-) diff --git a/cinder/api/contrib/qos_specs_manage.py b/cinder/api/contrib/qos_specs_manage.py index 8f97ed794c8..80025574726 100644 --- a/cinder/api/contrib/qos_specs_manage.py +++ b/cinder/api/contrib/qos_specs_manage.py @@ -98,9 +98,6 @@ class QoSSpecsController(wsgi.Controller): max_length=255, remove_whitespaces=True) name = name.strip() - # Validate the key-value pairs in the qos spec. - utils.validate_dictionary_string_length(specs) - try: spec = qos_specs.create(context, name, specs) notifier_info = dict(name=name, specs=specs) diff --git a/cinder/tests/unit/api/contrib/test_qos_specs_manage.py b/cinder/tests/unit/api/contrib/test_qos_specs_manage.py index 6e00e063d17..e2bfc819d5c 100644 --- a/cinder/tests/unit/api/contrib/test_qos_specs_manage.py +++ b/cinder/tests/unit/api/contrib/test_qos_specs_manage.py @@ -381,8 +381,7 @@ class QoSSpecManageApiTest(test.TestCase): @mock.patch('cinder.volume.qos_specs.create', side_effect=return_qos_specs_create) - @mock.patch('cinder.utils.validate_dictionary_string_length') - def test_create(self, mock_validate, mock_qos_spec_create): + def test_create(self, mock_qos_spec_create): body = {"qos_specs": {"name": "qos_specs_%s" % fake.QOS_SPEC_ID, "key1": "value1"}} @@ -395,7 +394,6 @@ class QoSSpecManageApiTest(test.TestCase): self.assertEqual(1, self.notifier.get_notification_count()) self.assertEqual('qos_specs_%s' % fake.QOS_SPEC_ID, res_dict['qos_specs']['name']) - self.assertTrue(mock_validate.called) @mock.patch('cinder.volume.qos_specs.create', side_effect=return_qos_specs_create) @@ -506,6 +504,18 @@ class QoSSpecManageApiTest(test.TestCase): req, fake.INVALID_ID, body) self.assertEqual(1, self.notifier.get_notification_count()) + @ddt.data({'qos_specs': {'key1': ['value1']}}, + {'qos_specs': {1: 'value1'}} + ) + def test_update_non_string_key_or_value(self, body): + req = fakes.HTTPRequest.blank('/v2/%s/qos-specs/%s' % + (fake.PROJECT_ID, fake.UUID1), + use_admin_context=True) + self.assertRaises(exception.InvalidQoSSpecs, + self.controller.update, + req, fake.UUID1, body) + self.assertEqual(1, self.notifier.get_notification_count()) + @mock.patch('cinder.volume.qos_specs.update', side_effect=return_qos_specs_update) def test_update_failed(self, mock_qos_update): diff --git a/cinder/tests/unit/test_qos_specs.py b/cinder/tests/unit/test_qos_specs.py index 100fccda492..596a8d656b5 100644 --- a/cinder/tests/unit/test_qos_specs.py +++ b/cinder/tests/unit/test_qos_specs.py @@ -115,7 +115,7 @@ class QoSSpecsTestCase(test.TestCase): # qos specs must exists self.assertRaises(exception.QoSSpecsNotFound, - qos_specs.update, self.ctxt, 'fake_id', qos) + qos_specs.update, self.ctxt, 'fake_id', qos['specs']) specs_id = self._create_qos_specs('Name', qos['consumer'], diff --git a/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py b/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py index 6fcd5305506..25acff1a499 100644 --- a/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py +++ b/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py @@ -4536,7 +4536,7 @@ class StorwizeSVCCommonDriverTestCase(test.TestCase): def test_storwize_svc_get_vdisk_params(self): self.driver.do_setup(None) - fake_qos = {'qos:IOThrottling': 5000} + fake_qos = {'qos:IOThrottling': '5000'} expected_qos = {'IOThrottling': 5000} fake_opts = self._get_default_opts() # The parameters retured should be the same to the default options, @@ -4622,7 +4622,7 @@ class StorwizeSVCCommonDriverTestCase(test.TestCase): # If the QoS is set both via the qos association and the # extra specs, the one from the qos association will take effect. - fake_qos_associate = {'qos:IOThrottling': 6000} + fake_qos_associate = {'qos:IOThrottling': '6000'} expected_qos_associate = {'IOThrottling': 6000} vol_type_qos = self._create_volume_type_qos_both(fake_qos, fake_qos_associate) diff --git a/cinder/volume/qos_specs.py b/cinder/volume/qos_specs.py index 33be69c29fc..1f5204d1a3d 100644 --- a/cinder/volume/qos_specs.py +++ b/cinder/volume/qos_specs.py @@ -24,6 +24,7 @@ from cinder import db from cinder import exception from cinder.i18n import _ from cinder import objects +from cinder import utils from cinder.volume import volume_types @@ -40,6 +41,9 @@ def create(context, name, specs=None): 'total_iops_sec': 1000, 'total_bytes_sec': 1024000} """ + # Validate the key-value pairs in the qos spec. + utils.validate_dictionary_string_length(specs) + consumer = specs.get('consumer') if consumer: # If we need to modify specs, copy so we don't cause unintended @@ -67,6 +71,7 @@ def update(context, qos_specs_id, specs): LOG.debug('qos_specs.update(): specs %s', specs) try: + utils.validate_dictionary_string_length(specs) qos_spec = objects.QualityOfServiceSpecs.get_by_id(context, qos_specs_id) @@ -81,6 +86,8 @@ def update(context, qos_specs_id, specs): qos_spec.specs.update(specs) qos_spec.save() + except exception.InvalidInput as e: + raise exception.InvalidQoSSpecs(reason=e) except db_exc.DBError: LOG.exception('DB error:') raise exception.QoSSpecsUpdateFailed(specs_id=qos_specs_id,