Validate bool value using strutils.bool_from_string method

bool value is validated with different logic scattered all over the places
1. isinstance of six.string_type or bool
2. is_valid_boolstr method

Made changes to use strutils.bool_from_string method to validate bool
value at all places as this method does all things expected above.
Changes are not made in v1 api as it is going to be deprecated in
Liberty release.

Example error message:
Invalid value for force_host_copy: Unrecognized value 'xyz', acceptable
values are: '0', '1', 'f', 'false', 'n', 'no', 'off', 'on', 't', 'true',
'y', 'yes'

ApiImpact: 400 BadRequest response will be returned in case invalid force
value is provided during volume upload to image, earlier it was ignored.

Closes-Bug: 1460575
Change-Id: Ie64d429f5860a950cda9b363a65c5282f128f10b

diff --git a/cinder/api/contrib/admin_actions.py b/cinder/api/contrib/admin_actions.py
index 3ef3511c5803ce0c9f2ca91c2343378987860664..f11b1bd7e1626148992be485d352ecdd162accaf 100644 (file)
--- a/cinder/api/contrib/admin_actions.py
+++ b/cinder/api/contrib/admin_actions.py
@@ -15,7 +15,6 @@
 from oslo_log import log as logging
 import oslo_messaging as messaging
 from oslo_utils import strutils
-import six
 import webob
 from webob import exc
 
@@ -222,17 +221,13 @@ class VolumeAdminController(AdminController):
             host = params['host']
         except KeyError:
             raise exc.HTTPBadRequest(explanation=_("Must specify 'host'"))
-        force_host_copy = params.get('force_host_copy', False)
-        if isinstance(force_host_copy, six.string_types):
-            try:
-                force_host_copy = strutils.bool_from_string(force_host_copy,
-                                                            strict=True)
-            except ValueError:
-                raise exc.HTTPBadRequest(
-                    explanation=_("Bad value for 'force_host_copy'"))
-        elif not isinstance(force_host_copy, bool):
-            raise exc.HTTPBadRequest(
-                explanation=_("'force_host_copy' not string or bool"))
+        force_host_copy = params.get('force_host_copy', 'False')
+        try:
+            force_host_copy = strutils.bool_from_string(force_host_copy,
+                                                        strict=True)
+        except ValueError as e:
+            msg = (_("Invalid value for force_host_copy: '%s'") % e.message)
+            raise exc.HTTPBadRequest(explanation=msg)
         self.volume_api.migrate_volume(context, volume, host, force_host_copy)
         return webob.Response(status_int=202)
 

diff --git a/cinder/api/contrib/volume_actions.py b/cinder/api/contrib/volume_actions.py
index d6c88309df1bfdceb30aa2cb389d9e8366594351..b99f9e205f267793230329874e033919d74c18f0 100644 (file)
--- a/cinder/api/contrib/volume_actions.py
+++ b/cinder/api/contrib/volume_actions.py
@@ -265,15 +265,11 @@ class VolumeActionsController(wsgi.Controller):
             msg = _("No image_name was specified in request.")
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
-        force = params.get('force', False)
-        if isinstance(force, six.string_types):
-            try:
-                force = strutils.bool_from_string(force, strict=False)
-            except ValueError:
-                msg = _("Bad value for 'force' parameter.")
-                raise webob.exc.HTTPBadRequest(explanation=msg)
-        elif not isinstance(force, bool):
-            msg = _("'force' is not string or bool.")
+        force = params.get('force', 'False')
+        try:
+            force = strutils.bool_from_string(force, strict=True)
+        except ValueError as error:
+            msg = _("Invalid value for 'force': '%s'") % error.message
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
         try:
@@ -337,16 +333,11 @@ class VolumeActionsController(wsgi.Controller):
             msg = _("Must specify readonly in request.")
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
-        if isinstance(readonly_flag, six.string_types):
-            try:
-                readonly_flag = strutils.bool_from_string(readonly_flag,
-                                                          strict=True)
-            except ValueError:
-                msg = _("Bad value for 'readonly'")
-                raise webob.exc.HTTPBadRequest(explanation=msg)
-
-        elif not isinstance(readonly_flag, bool):
-            msg = _("'readonly' not string or bool")
+        try:
+            readonly_flag = strutils.bool_from_string(readonly_flag,
+                                                      strict=True)
+        except ValueError as error:
+            msg = _("Invalid value for 'readonly': '%s'") % error.message
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
         self.volume_api.update_readonly_flag(context, volume, readonly_flag)
@@ -382,16 +373,11 @@ class VolumeActionsController(wsgi.Controller):
             msg = _("Must specify bootable in request.")
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
-        if isinstance(bootable, six.string_types):
-            try:
-                bootable = strutils.bool_from_string(bootable,
-                                                     strict=True)
-            except ValueError:
-                msg = _("Bad value for 'bootable'")
-                raise webob.exc.HTTPBadRequest(explanation=msg)
-
-        elif not isinstance(bootable, bool):
-            msg = _("'bootable' not string or bool")
+        try:
+            bootable = strutils.bool_from_string(bootable,
+                                                 strict=True)
+        except ValueError as error:
+            msg = _("Invalid value for 'bootable': '%s'") % error.message
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
         update_dict = {'bootable': bootable}

diff --git a/cinder/api/v2/snapshots.py b/cinder/api/v2/snapshots.py
index d3976629d72299a9ee5b741096edc71e2d977143..c3179771494968800fd464f752699de5f6d8c1b5 100644 (file)
--- a/cinder/api/v2/snapshots.py
+++ b/cinder/api/v2/snapshots.py
@@ -196,11 +196,13 @@ class SnapshotsController(wsgi.Controller):
             snapshot['display_name'] = snapshot.get('name')
             del snapshot['name']
 
-        if not utils.is_valid_boolstr(force):
-            msg = _("Invalid value '%s' for force. ") % force
+        try:
+            force = strutils.bool_from_string(force, strict=True)
+        except ValueError as error:
+            msg = _("Invalid value for 'force': '%s'") % error.message
             raise exception.InvalidParameterValue(err=msg)
 
-        if strutils.bool_from_string(force):
+        if force:
             new_snapshot = self.volume_api.create_snapshot_force(
                 context,
                 volume,

diff --git a/cinder/tests/unit/api/contrib/test_admin_actions.py b/cinder/tests/unit/api/contrib/test_admin_actions.py
index 0d6034c15284c324c652613c311afd44779bf058..bc6a892d97226264271b61703d6e7b9c85fdf14e 100644 (file)
--- a/cinder/tests/unit/api/contrib/test_admin_actions.py
+++ b/cinder/tests/unit/api/contrib/test_admin_actions.py
@@ -841,7 +841,7 @@ class AdminActionsTest(test.TestCase):
         db.snapshot_create(ctx, {'volume_id': volume['id']})
         self._migrate_volume_exec(ctx, volume, host, expected_status)
 
-    def test_migrate_volume_bad_force_host_copy1(self):
+    def test_migrate_volume_bad_force_host_copy(self):
         expected_status = 400
         host = 'test2'
         ctx = context.RequestContext('admin', 'fake', True)
@@ -849,14 +849,6 @@ class AdminActionsTest(test.TestCase):
         self._migrate_volume_exec(ctx, volume, host, expected_status,
                                   force_host_copy='foo')
 
-    def test_migrate_volume_bad_force_host_copy2(self):
-        expected_status = 400
-        host = 'test2'
-        ctx = context.RequestContext('admin', 'fake', True)
-        volume = self._migrate_volume_prep()
-        self._migrate_volume_exec(ctx, volume, host, expected_status,
-                                  force_host_copy=1)
-
     def _migrate_volume_comp_exec(self, ctx, volume, new_volume, error,
                                   expected_status, expected_id, no_body=False):
         req = webob.Request.blank('/v2/fake/volumes/%s/action' % volume['id'])