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<sxmatch1986@gmail.com>

Change-Id: I5cb68af74c68cd53bc3b84e705ca3472859c1ec3
Closes-bug: #1704988
Closes-Bug: #1710805

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)

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):

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'],

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)

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,