Fix 500 error if boolean is_public is passed as string

create and update apis of 'volume-type' and 'group_type' returns
500 error if boolean 'is_public' value is passed in the form of
string. Internally, database api doesn't convert is_public from
string to boolean and hence it raises following error:

DBError: (exceptions.ValueError) invalid literal for int() with base
10

User can pass following valid boolean values to these APIs:
'0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on', 'y',
'yes'

Used cinder.utils.get_bool_param method to convert 'is_public'
parameter from string to boolean. For invalid boolean values it will
return exception.InvalidParameterValue.

APIImpact:
Now create and update apis of 'volume-type' and 'group_type' will not
return 500 if boolean 'is_public' passed in the form of string. For
boolean values other than the specified above these api's will return
400 HTTPBadRequest.

Closes-Bug: #1670260
Change-Id: I9337a9182d714d7e56958fd3fb340108110783a7

cinder/api/contrib/types_manage.py

@@ -19,8 +19,6 @@ import six
 from six.moves import http_client
 import webob
 
-from oslo_utils import strutils
-
 from cinder.api import extensions
 from cinder.api.openstack import wsgi
 from cinder.api.views import types as views_types
@@ -64,7 +62,8 @@ class VolumeTypesManageController(wsgi.Controller):
         description = vol_type.get('description')
         specs = vol_type.get('extra_specs', {})
         utils.validate_dictionary_string_length(specs)
-        is_public = vol_type.get('os-volume-type-access:is_public', True)
+        is_public = utils.get_bool_param('os-volume-type-access:is_public',
+                                         vol_type, True)
 
         if name is None or len(name.strip()) == 0:
             msg = _("Volume type name can not be empty.")
@@ -77,11 +76,6 @@ class VolumeTypesManageController(wsgi.Controller):
             utils.check_string_length(description, 'Type description',
                                       min_length=0, max_length=255)
 
-        if not strutils.is_valid_boolstr(is_public):
-            msg = _("Invalid value '%s' for is_public. Accepted values: "
-                    "True or False.") % is_public
-            raise webob.exc.HTTPBadRequest(explanation=msg)
-
         try:
             volume_types.create(context,
                                 name,
@@ -129,10 +123,8 @@ class VolumeTypesManageController(wsgi.Controller):
                     "a combination thereof.")
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
-        if is_public is not None and not strutils.is_valid_boolstr(is_public):
+        if is_public is not None:
-            msg = _("Invalid value '%s' for is_public. Accepted values: "
+            is_public = utils.get_bool_param('is_public', vol_type)
-                    "True or False.") % is_public
-            raise webob.exc.HTTPBadRequest(explanation=msg)
 
         if name:
             utils.check_string_length(name, 'Type name',

cinder/api/v3/group_types.py

@@ -68,7 +68,7 @@ class GroupTypesController(wsgi.Controller):
         name = grp_type.get('name', None)
         description = grp_type.get('description')
         specs = grp_type.get('group_specs', {})
-        is_public = grp_type.get('is_public', True)
+        is_public = utils.get_bool_param('is_public', grp_type, True)
 
         if name is None or len(name.strip()) == 0:
             msg = _("Group type name can not be empty.")
@@ -127,10 +127,8 @@ class GroupTypesController(wsgi.Controller):
                     "a combination thereof.")
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
-        if is_public is not None and not strutils.is_valid_boolstr(is_public):
+        if is_public is not None:
-            msg = _("Invalid value '%s' for is_public. Accepted values: "
+            is_public = utils.get_bool_param('is_public', grp_type)
-                    "True or False.") % is_public
-            raise webob.exc.HTTPBadRequest(explanation=msg)
 
         if name:
             utils.check_string_length(name, 'Type name',

cinder/tests/unit/api/contrib/test_types_manage.py

@@ -18,6 +18,7 @@ import six
 import webob
 
 import ddt
+from oslo_utils import strutils
 
 from cinder.api.contrib import types_manage
 from cinder import context
@@ -284,9 +285,31 @@ class VolumeTypesManageApiTest(test.TestCase):
                                 "description": "test description",
                                 "extra_specs": {"key1": "value1"}}}
         req = fakes.HTTPRequest.blank('/v2/%s/types' % fake.PROJECT_ID)
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.InvalidParameterValue,
                           self.controller._create, req, body)
 
+    @ddt.data('0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on',
+              'y', 'yes')
+    @mock.patch.object(volume_types, "get_volume_type_by_name")
+    @mock.patch.object(volume_types, "create")
+    @mock.patch("cinder.api.openstack.wsgi.Request.cache_resource")
+    @mock.patch("cinder.api.views.types.ViewBuilder.show")
+    def test_create_type_with_valid_is_public_in_string(
+            self, is_public, mock_show, mock_cache_resource,
+            mock_create, mock_get):
+        boolean_is_public = strutils.bool_from_string(is_public)
+        ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, True)
+        body = {"volume_type": {"name": "vol_type_1",
+                                "os-volume-type-access:is_public":
+                                    is_public,
+                                "extra_specs": {"key1": "value1"}}}
+        req = fakes.HTTPRequest.blank('/v2/%s/types' % fake.PROJECT_ID)
+        req.environ['cinder.context'] = ctxt
+        self.controller._create(req, body)
+        mock_create.assert_called_once_with(
+            ctxt, 'vol_type_1', {'key1': 'value1'},
+            boolean_is_public, description=None)
+
     def _create_volume_type_bad_body(self, body):
         req = fakes.HTTPRequest.blank('/v2/%s/types' % fake.PROJECT_ID)
         req.method = 'POST'
@@ -374,6 +397,28 @@ class VolumeTypesManageApiTest(test.TestCase):
                               (DEFAULT_VOLUME_TYPE, DEFAULT_VOLUME_TYPE),
              'is_public': False})
 
+    @ddt.data('0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on',
+              'y', 'yes')
+    @mock.patch('cinder.volume.volume_types.update')
+    @mock.patch('cinder.volume.volume_types.get_volume_type')
+    @mock.patch("cinder.api.openstack.wsgi.Request.cache_resource")
+    @mock.patch("cinder.api.views.types.ViewBuilder.show")
+    def test_update_with_valid_is_public_in_string(
+            self, is_public, mock_show, mock_cache_resource,
+            mock_get, mock_update):
+        body = {"volume_type": {"is_public": is_public}}
+        req = fakes.HTTPRequest.blank('/v2/%s/types/%s' % (
+            fake.PROJECT_ID, DEFAULT_VOLUME_TYPE))
+        req.method = 'PUT'
+        ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, True)
+        req.environ['cinder.context'] = ctxt
+        boolean_is_public = strutils.bool_from_string(is_public)
+
+        self.controller._update(req, DEFAULT_VOLUME_TYPE, body)
+        mock_update.assert_called_once_with(
+            ctxt, DEFAULT_VOLUME_TYPE, None, None,
+            is_public=boolean_is_public)
+
     @mock.patch('cinder.volume.volume_types.update')
     @mock.patch('cinder.volume.volume_types.get_volume_type')
     def test_update_type_with_description_having_length_zero(
@@ -550,7 +595,7 @@ class VolumeTypesManageApiTest(test.TestCase):
             fake.PROJECT_ID, DEFAULT_VOLUME_TYPE))
         req.method = 'PUT'
 
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.InvalidParameterValue,
                           self.controller._update, req,
                           DEFAULT_VOLUME_TYPE, body)
 

cinder/tests/unit/api/v3/test_group_types.py

@@ -15,7 +15,9 @@
 
 import uuid
 
+import ddt
 import mock
+from oslo_utils import strutils
 from oslo_utils import timeutils
 import six
 import webob
@@ -86,6 +88,7 @@ def return_group_types_get_default_not_found():
     return {}
 
 
+@ddt.ddt
 class GroupTypesApiTest(test.TestCase):
 
     def _create_group_type(self, group_type_name, group_specs=None,
@@ -112,6 +115,27 @@ class GroupTypesApiTest(test.TestCase):
                                                 [fake.PROJECT_ID])
         self.type_id0 = group_types.get_default_cgsnapshot_type()['id']
 
+    @ddt.data('0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on',
+              'y', 'yes')
+    @mock.patch.object(group_types, "get_group_type_by_name")
+    @mock.patch.object(group_types, "create")
+    @mock.patch("cinder.api.openstack.wsgi.Request.cache_resource")
+    @mock.patch("cinder.api.views.types.ViewBuilder.show")
+    def test_create_group_type_with_valid_is_public_in_string(
+            self, is_public, mock_show, mock_cache_resource,
+            mock_create, mock_get):
+        boolean_is_public = strutils.bool_from_string(is_public)
+        req = fakes.HTTPRequest.blank('/v3/%s/types' % fake.PROJECT_ID,
+                                      version=GROUP_TYPE_MICRO_VERSION)
+        req.environ['cinder.context'] = self.ctxt
+
+        body = {"group_type": {"is_public": is_public, "name": "group_type1",
+                               "description": None}}
+        self.controller.create(req, body)
+        mock_create.assert_called_once_with(
+            self.ctxt, 'group_type1', {},
+            boolean_is_public, description=None)
+
     def test_group_types_index(self):
         self.mock_object(group_types, 'get_all_group_types',
                          return_group_types_get_all_types)
@@ -260,6 +284,27 @@ class GroupTypesApiTest(test.TestCase):
         self.assertEqual(expect_result[2], res['group_types'][2]['id'])
         self.assertEqual(expect_result[3], res['group_types'][3]['id'])
 
+    @ddt.data('0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on',
+              'y', 'yes')
+    @mock.patch.object(group_types, "get_group_type")
+    @mock.patch.object(group_types, "update")
+    @mock.patch("cinder.api.openstack.wsgi.Request.cache_resource")
+    @mock.patch("cinder.api.views.types.ViewBuilder.show")
+    def test_update_group_type_with_valid_is_public_in_string(
+            self, is_public, mock_show, mock_cache_resource,
+            mock_update, mock_get):
+        boolean_is_public = strutils.bool_from_string(is_public)
+        type_id = six.text_type(uuid.uuid4())
+        req = fakes.HTTPRequest.blank(
+            '/v3/%s/types/%s' % (fake.PROJECT_ID, type_id),
+            version=GROUP_TYPE_MICRO_VERSION)
+        req.environ['cinder.context'] = self.ctxt
+        body = {"group_type": {"is_public": is_public, "name": "group_type1"}}
+        self.controller.update(req, type_id, body)
+        mock_update.assert_called_once_with(
+            self.ctxt, type_id, 'group_type1', None,
+            is_public=boolean_is_public)
+
     def test_group_types_show(self):
         self.mock_object(group_types, 'get_group_type',
                          return_group_types_get_group_type)

cinder/utils.py

@@ -592,11 +592,11 @@ def _get_disk_of_partition(devpath, st=None):
     return (devpath, st)
 
 
-def get_bool_param(param_string, params):
+def get_bool_param(param_string, params, default=False):
-    param = params.get(param_string, False)
+    param = params.get(param_string, default)
     if not strutils.is_valid_boolstr(param):
-        msg = _('Value %(param)s for %(param_string)s is not a '
+        msg = _("Value '%(param)s' for '%(param_string)s' is not "
-                'boolean.') % {'param': param, 'param_string': param_string}
+                "a boolean.") % {'param': param, 'param_string': param_string}
         raise exception.InvalidParameterValue(err=msg)
 
     return strutils.bool_from_string(param, strict=True)

releasenotes/notes/bug-1670260-fix-boolean-is_public-d16e1957c0f09d65.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fixed issue where ``create`` and ``update`` api's of ``volume-type`` and
+    ``group_type`` were returning 500 error if boolean 'is_public' value
+    passed in the form of string. Now user can pass following valid boolean
+    values to these api's:
+    '0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on', 'y', 'yes'