From d8928c20671a23b26ae9d8e76e95d62a174b8300 Mon Sep 17 00:00:00 2001 From: Dinesh Bhor Date: Mon, 13 Feb 2017 20:41:55 +0530 Subject: [PATCH] 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 | 16 ++---- cinder/api/v3/group_types.py | 8 ++- .../unit/api/contrib/test_types_manage.py | 49 ++++++++++++++++++- cinder/tests/unit/api/v3/test_group_types.py | 45 +++++++++++++++++ cinder/utils.py | 8 +-- ...ix-boolean-is_public-d16e1957c0f09d65.yaml | 8 +++ 6 files changed, 111 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/bug-1670260-fix-boolean-is_public-d16e1957c0f09d65.yaml diff --git a/cinder/api/contrib/types_manage.py b/cinder/api/contrib/types_manage.py index 97ac8d20a6f..756f9dc2731 100644 --- a/cinder/api/contrib/types_manage.py +++ b/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): - msg = _("Invalid value '%s' for is_public. Accepted values: " - "True or False.") % is_public - raise webob.exc.HTTPBadRequest(explanation=msg) + if is_public is not None: + is_public = utils.get_bool_param('is_public', vol_type) if name: utils.check_string_length(name, 'Type name', diff --git a/cinder/api/v3/group_types.py b/cinder/api/v3/group_types.py index 2728106c4d3..1163f5197f9 100644 --- a/cinder/api/v3/group_types.py +++ b/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): - msg = _("Invalid value '%s' for is_public. Accepted values: " - "True or False.") % is_public - raise webob.exc.HTTPBadRequest(explanation=msg) + if is_public is not None: + is_public = utils.get_bool_param('is_public', grp_type) if name: utils.check_string_length(name, 'Type name', diff --git a/cinder/tests/unit/api/contrib/test_types_manage.py b/cinder/tests/unit/api/contrib/test_types_manage.py index 6e3553d61f3..5f7b77080b7 100644 --- a/cinder/tests/unit/api/contrib/test_types_manage.py +++ b/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) diff --git a/cinder/tests/unit/api/v3/test_group_types.py b/cinder/tests/unit/api/v3/test_group_types.py index 5574c6b76ed..859f78179bc 100644 --- a/cinder/tests/unit/api/v3/test_group_types.py +++ b/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) diff --git a/cinder/utils.py b/cinder/utils.py index 8ef6f2467d5..16614a503db 100644 --- a/cinder/utils.py +++ b/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): - param = params.get(param_string, False) +def get_bool_param(param_string, params, default=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 ' - 'boolean.') % {'param': param, 'param_string': param_string} + msg = _("Value '%(param)s' for '%(param_string)s' is not " + "a boolean.") % {'param': param, 'param_string': param_string} raise exception.InvalidParameterValue(err=msg) return strutils.bool_from_string(param, strict=True) diff --git a/releasenotes/notes/bug-1670260-fix-boolean-is_public-d16e1957c0f09d65.yaml b/releasenotes/notes/bug-1670260-fix-boolean-is_public-d16e1957c0f09d65.yaml new file mode 100644 index 00000000000..a40fa7cb902 --- /dev/null +++ b/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' \ No newline at end of file