V3 jsonschema validation: os-services

This patch adds jsonschema validation for below os-services API's
* PUT /v3/{project_id}/os-services/disable
* PUT /v3/{project_id}/os-services/disable-log-reason
* PUT /v3/{project_id}/os-services/enable
* PUT /v3/{project_id}/os-services/get-log
* PUT /v3/{project_id}/os-services/set-log
* PUT /v3/{project_id}/os-services/freeze
* PUT /v3/{project_id}/os-services/thaw
* PUT /v3/{project_id}/os-services/failover_host

Change-Id: I37365eb4d667263bb192f52d06135b2fd7298fef
Partial-Implements: bp json-schema-validation

cinder/api/contrib/services.py

@@ -24,6 +24,8 @@ from cinder.api import common
 from cinder.api import extensions
 from cinder.api import microversions as mv
 from cinder.api.openstack import wsgi
+from cinder.api.schemas import services as os_services
+from cinder.api import validation
 from cinder.backup import rpcapi as backup_rpcapi
 from cinder.common import constants
 from cinder import exception
@@ -43,9 +45,6 @@ LOG = logging.getLogger(__name__)
 
 
 class ServiceController(wsgi.Controller):
-    LOG_BINARIES = (constants.SCHEDULER_BINARY, constants.VOLUME_BINARY,
-                    constants.BACKUP_BINARY, constants.API_BINARY)
-
     def __init__(self, ext_mgr=None):
         self.ext_mgr = ext_mgr
         super(ServiceController, self).__init__()
@@ -123,36 +122,28 @@ class ServiceController(wsgi.Controller):
             svcs.append(ret_fields)
         return {'services': svcs}
 
-    def _is_valid_as_reason(self, reason):
-        if not reason:
-            return False
-        try:
-            utils.check_string_length(reason, 'Disabled reason', min_length=1,
-                                      max_length=255, allow_all_spaces=False)
-        except exception.InvalidInput:
-            return False
-
-        return True
-
     def _volume_api_proxy(self, fun, *args):
         try:
             return fun(*args)
         except exception.ServiceNotFound as ex:
             raise exception.InvalidInput(ex.msg)
 
-    def _freeze(self, context, req, body):
+    @validation.schema(os_services.freeze_and_thaw)
+    def _freeze(self, req, context, body):
         cluster_name, host = common.get_cluster_host(
             req, body, mv.REPLICATION_CLUSTER)
         return self._volume_api_proxy(self.volume_api.freeze_host, context,
                                       host, cluster_name)
 
-    def _thaw(self, context, req, body):
+    @validation.schema(os_services.freeze_and_thaw)
+    def _thaw(self, req, context, body):
         cluster_name, host = common.get_cluster_host(
             req, body, mv.REPLICATION_CLUSTER)
         return self._volume_api_proxy(self.volume_api.thaw_host, context,
                                       host, cluster_name)
 
-    def _failover(self, context, req, body, clustered):
+    @validation.schema(os_services.failover_host)
+    def _failover(self, req, context, clustered, body):
         # We set version to None to always get the cluster name from the body,
         # to False when we don't want to get it, and REPLICATION_CLUSTER  when
         # we only want it if the requested version is REPLICATION_CLUSTER  or
@@ -166,18 +157,15 @@ class ServiceController(wsgi.Controller):
     def _log_params_binaries_services(self, context, body):
         """Get binaries and services referred by given log set/get request."""
         query_filters = {'is_up': True}
-
         binary = body.get('binary')
+        binaries = []
         if binary in ('*', None, ''):
-            binaries = self.LOG_BINARIES
+            binaries = constants.LOG_BINARIES
         elif binary == constants.API_BINARY:
             return [binary], []
-        elif binary in self.LOG_BINARIES:
+        elif binary in constants.LOG_BINARIES:
             binaries = [binary]
             query_filters['binary'] = binary
-        else:
-            raise exception.InvalidInput(reason=_('%s is not a valid binary.')
-                                         % binary)
 
         server = body.get('server')
         if server:
@@ -186,12 +174,11 @@ class ServiceController(wsgi.Controller):
 
         return binaries, services
 
-    def _set_log(self, context, body):
+    @validation.schema(os_services.set_log)
+    def _set_log(self, req, context, body):
         """Set log levels of services dynamically."""
         prefix = body.get('prefix')
         level = body.get('level')
-        # Validate log level
-        utils.get_log_method(level)
 
         binaries, services = self._log_params_binaries_services(context, body)
 
@@ -205,7 +192,8 @@ class ServiceController(wsgi.Controller):
 
         return webob.Response(status_int=http_client.ACCEPTED)
 
-    def _get_log(self, context, body):
+    @validation.schema(os_services.get_log)
+    def _get_log(self, req, context, body):
         """Get current log levels for services."""
         prefix = body.get('prefix')
         binaries, services = self._log_params_binaries_services(context, body)
@@ -229,6 +217,25 @@ class ServiceController(wsgi.Controller):
 
         return {'log_levels': result}
 
+    @validation.schema(os_services.disable_log_reason)
+    def _disabled_log_reason(self, req, body):
+        reason = body.get('disabled_reason')
+        disabled = True
+        status = "disabled"
+        return reason, disabled, status
+
+    @validation.schema(os_services.enable_and_disable)
+    def _enable(self, req, body):
+        disabled = False
+        status = "enabled"
+        return disabled, status
+
+    @validation.schema(os_services.enable_and_disable)
+    def _disable(self, req, body):
+        disabled = True
+        status = "disabled"
+        return disabled, status
+
     def update(self, req, id, body):
         """Enable/Disable scheduling for a service.
 
@@ -241,52 +248,40 @@ class ServiceController(wsgi.Controller):
         context.authorize(policy.UPDATE_POLICY)
 
         support_dynamic_log = req.api_version_request.matches(mv.LOG_LEVEL)
-
         ext_loaded = self.ext_mgr.is_loaded('os-extended-services')
         ret_val = {}
         if id == "enable":
-            disabled = False
+            disabled, status = self._enable(req, body=body)
-            status = "enabled"
+        elif id == "disable":
-            if ext_loaded:
+            disabled, status = self._disable(req, body=body)
-                ret_val['disabled_reason'] = None
+        elif id == "disable-log-reason" and ext_loaded:
-        elif (id == "disable" or
+            disabled_reason, disabled, status = (
-                (id == "disable-log-reason" and ext_loaded)):
+                self._disabled_log_reason(req, body=body))
-            disabled = True
+            ret_val['disabled_reason'] = disabled_reason
-            status = "disabled"
         elif id == "freeze":
-            return self._freeze(context, req, body)
+            return self._freeze(req, context, body=body)
         elif id == "thaw":
-            return self._thaw(context, req, body)
+            return self._thaw(req, context, body=body)
         elif id == "failover_host":
-            return self._failover(context, req, body, False)
+            return self._failover(req, context, False, body=body)
         elif (req.api_version_request.matches(mv.REPLICATION_CLUSTER) and
               id == 'failover'):
-            return self._failover(context, req, body, True)
+            return self._failover(req, context, True, body=body)
         elif support_dynamic_log and id == 'set-log':
-            return self._set_log(context, body)
+            return self._set_log(req, context, body=body)
         elif support_dynamic_log and id == 'get-log':
-            return self._get_log(context, body)
+            return self._get_log(req, context, body=body)
         else:
             raise exception.InvalidInput(reason=_("Unknown action"))
 
         host = common.get_cluster_host(req, body, False)[1]
-
         ret_val['disabled'] = disabled
-        if id == "disable-log-reason" and ext_loaded:
-            reason = body.get('disabled_reason')
-            if not self._is_valid_as_reason(reason):
-                msg = _('Disabled reason contains invalid characters '
-                        'or is too long')
-                raise webob.exc.HTTPBadRequest(explanation=msg)
-            ret_val['disabled_reason'] = reason
 
         # NOTE(uni): deprecating service request key, binary takes precedence
         # Still keeping service key here for API compatibility sake.
         service = body.get('service', '')
         binary = body.get('binary', '')
         binary_key = binary or service
-        if not binary_key:
-            raise webob.exc.HTTPBadRequest()
 
         # Not found exception will be handled at the wsgi level
         svc = objects.Service.get_by_args(context, host, binary_key)

cinder/api/schemas/services.py

@@ -0,0 +1,84 @@
+# Copyright 2018 NTT DATA
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License");
+#    you may not use this file except in compliance with the License.
+#    You may obtain a copy of the License at
+#
+#        http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS,
+#    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#    See the License for the specific language governing permissions and
+#    limitations under the License.
+
+import copy
+
+from cinder.api.validation import parameter_types
+
+
+enable_and_disable = {
+    'type': 'object',
+    'properties': {
+        'binary': {'type': 'string', 'minLength': 1, 'maxLength': 255},
+        'host': parameter_types.hostname,
+        'cluster': parameter_types.nullable_string,
+        'service': {'type': 'string', 'minLength': 1, 'maxLength': 255},
+    },
+    'anyOf': [
+        {'required': ['binary']},
+        {'required': ['service']}
+    ],
+    'additionalProperties': False,
+}
+
+
+disable_log_reason = copy.deepcopy(enable_and_disable)
+disable_log_reason['properties'][
+    'disabled_reason'] = {'type': 'string', 'minLength': 1, 'maxLength': 255,
+                          'format': 'disabled_reason'}
+
+
+set_log = {
+    'type': 'object',
+    'properties': {
+        'binary': parameter_types.binary,
+        'server': parameter_types.nullable_string,
+        'prefix': parameter_types.nullable_string,
+        'level': {'type': ['string', 'null'], 'format': 'level'}
+    },
+    'additionalProperties': False,
+}
+
+
+get_log = {
+    'type': 'object',
+    'properties': {
+        'binary': parameter_types.binary,
+        'server': parameter_types.nullable_string,
+        'prefix': parameter_types.nullable_string,
+    },
+    'additionalProperties': False,
+}
+
+
+freeze_and_thaw = {
+    'type': 'object',
+    'properties': {
+        'cluster': parameter_types.nullable_string,
+        'host': parameter_types.hostname,
+    },
+    'additionalProperties': False,
+}
+
+
+failover_host = {
+    'type': 'object',
+    'properties': {
+        'host': parameter_types.hostname,
+        'backend_id': parameter_types.nullable_string,
+        'cluster': parameter_types.nullable_string,
+    },
+    'additionalProperties': False,
+}

cinder/api/validation/parameter_types.py

@@ -249,5 +249,10 @@ quota_class_set = {
         }
     },
     'additionalProperties': False
-
+}
+
+
+binary = {
+    'type': 'string',
+    'enum': [binary for binary in constants.LOG_BINARIES + ('', '*')]
 }

cinder/api/validation/validators.py

@@ -209,7 +209,8 @@ def _validate_base64_format(instance):
     return True
 
 
-@jsonschema.FormatChecker.cls_checks('disabled_reason')
+@jsonschema.FormatChecker.cls_checks('disabled_reason',
+                                     exception.InvalidInput)
 def _validate_disabled_reason(param_value):
     _validate_string_length(param_value, 'disabled_reason',
                             mandatory=False, min_length=1, max_length=255,
@@ -289,6 +290,12 @@ def _validate_group_type(param_value):
     return True
 
 
+@jsonschema.FormatChecker.cls_checks('level')
+def _validate_log_level(level):
+    utils.get_log_method(level)
+    return True
+
+
 class FormatChecker(jsonschema.FormatChecker):
     """A FormatChecker can output the message from cause exception
 

cinder/common/constants.py

@@ -25,3 +25,4 @@ BACKUP_BINARY = "cinder-backup"
 SCHEDULER_TOPIC = SCHEDULER_BINARY
 VOLUME_TOPIC = VOLUME_BINARY
 BACKUP_TOPIC = BACKUP_BINARY
+LOG_BINARIES = (SCHEDULER_BINARY, VOLUME_BINARY, BACKUP_BINARY, API_BINARY)

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

@@ -21,11 +21,11 @@ import iso8601
 import mock
 from oslo_config import cfg
 from six.moves import http_client
-import webob.exc
 
 from cinder.api.contrib import services
 from cinder.api import extensions
 from cinder.api import microversions as mv
+from cinder.api.openstack import api_version_request as api_version
 from cinder.common import constants
 from cinder import context
 from cinder import exception
@@ -267,7 +267,8 @@ class ServicesTest(test.TestCase):
 
     def test_failover_no_values(self):
         req = FakeRequest(version=mv.REPLICATION_CLUSTER)
-        self.assertRaises(exception.InvalidInput, self.controller.update, req,
+        self.assertRaises(exception.InvalidInput,
+                          self.controller.update, req,
                           'failover', {'backend_id': 'replica1'})
 
     @ddt.data({'host': 'hostname'}, {'cluster': 'mycluster'})
@@ -703,42 +704,43 @@ class ServicesTest(test.TestCase):
                 'binary': 'cinder-scheduler',
                 'disabled_reason': None,
                 }
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.ValidationError,
                           self.controller.update,
                           req, "disable-log-reason", body)
 
-    def test_invalid_reason_field(self):
+    @ddt.data(' ' * 10, 'a' * 256, None)
-        # Check that empty strings are not allowed
+    def test_invalid_reason_field(self, reason):
-        reason = ' ' * 10
+        # # Check that empty strings are not allowed
-        self.assertFalse(self.controller._is_valid_as_reason(reason))
+        self.ext_mgr.extensions['os-extended-services'] = True
-        reason = 'a' * 256
+        self.controller = services.ServiceController(self.ext_mgr)
-        self.assertFalse(self.controller._is_valid_as_reason(reason))
+        req = (
-        # Check that spaces at the end are also counted
+            fakes.HTTPRequest.blank('v3/fake/os-services/disable-log-reason'))
-        reason = 'a' * 255 + ' '
+        body = {'host': 'host1',
-        self.assertFalse(self.controller._is_valid_as_reason(reason))
+                'binary': 'cinder-volume',
-        reason = 'it\'s a valid reason.'
+                'disabled_reason': reason,
-        self.assertTrue(self.controller._is_valid_as_reason(reason))
+                }
-        reason = None
+        self.assertRaises(exception.ValidationError,
-        self.assertFalse(self.controller._is_valid_as_reason(reason))
+                          self.controller.update,
+                          req, "disable-log-reason", body)
 
     def test_services_failover_host(self):
         url = '/v2/%s/os-services/failover_host' % fake.PROJECT_ID
         req = fakes.HTTPRequest.blank(url)
-        body = {'host': mock.sentinel.host,
+        body = {'host': 'fake_host',
-                'backend_id': mock.sentinel.backend_id}
+                'backend_id': 'fake_backend'}
         with mock.patch.object(self.controller.volume_api, 'failover') \
                 as failover_mock:
             res = self.controller.update(req, 'failover_host', body)
         failover_mock.assert_called_once_with(req.environ['cinder.context'],
-                                              mock.sentinel.host,
+                                              'fake_host',
                                               None,
-                                              mock.sentinel.backend_id)
+                                              'fake_backend')
         self.assertEqual(http_client.ACCEPTED, res.status_code)
 
-    @ddt.data(('failover_host', {'host': mock.sentinel.host,
+    @ddt.data(('failover_host', {'host': 'fake_host',
-                                 'backend_id': mock.sentinel.backend_id}),
+                                 'backend_id': 'fake_backend'}),
-              ('freeze', {'host': mock.sentinel.host}),
+              ('freeze', {'host': 'fake_host'}),
-              ('thaw', {'host': mock.sentinel.host}))
+              ('thaw', {'host': 'fake_host'}))
     @ddt.unpack
     @mock.patch('cinder.objects.ServiceList.get_all')
     def test_services_action_host_not_found(self, method, body,
@@ -746,16 +748,16 @@ class ServicesTest(test.TestCase):
         url = '/v2/%s/os-services/%s' % (fake.PROJECT_ID, method)
         req = fakes.HTTPRequest.blank(url)
         mock_get_all_services.return_value = []
-        msg = 'No service found with host=%s' % mock.sentinel.host
+        msg = 'No service found with host=%s' % 'fake_host'
         result = self.assertRaises(exception.InvalidInput,
                                    self.controller.update,
                                    req, method, body)
         self.assertEqual(msg, result.msg)
 
-    @ddt.data(('failover', {'cluster': mock.sentinel.cluster,
+    @ddt.data(('failover', {'cluster': 'fake_cluster',
-                            'backend_id': mock.sentinel.backend_id}),
+                            'backend_id': 'fake_backend'}),
-              ('freeze', {'cluster': mock.sentinel.cluster}),
+              ('freeze', {'cluster': 'fake_cluster'}),
-              ('thaw', {'cluster': mock.sentinel.cluster}))
+              ('thaw', {'cluster': 'fake_cluster'}))
     @ddt.unpack
     @mock.patch('cinder.objects.ServiceList.get_all')
     def test_services_action_cluster_not_found(self, method, body,
@@ -763,7 +765,7 @@ class ServicesTest(test.TestCase):
         url = '/v3/%s/os-services/%s' % (fake.PROJECT_ID, method)
         req = fakes.HTTPRequest.blank(url, version=mv.REPLICATION_CLUSTER)
         mock_get_all_services.return_value = []
-        msg = 'No service found with cluster=%s' % mock.sentinel.cluster
+        msg = "No service found with cluster=fake_cluster"
         result = self.assertRaises(exception.InvalidInput,
                                    self.controller.update, req,
                                    method, body)
@@ -772,23 +774,23 @@ class ServicesTest(test.TestCase):
     def test_services_freeze(self):
         url = '/v2/%s/os-services/freeze' % fake.PROJECT_ID
         req = fakes.HTTPRequest.blank(url)
-        body = {'host': mock.sentinel.host}
+        body = {'host': 'fake_host'}
         with mock.patch.object(self.controller.volume_api, 'freeze_host') \
                 as freeze_mock:
             res = self.controller.update(req, 'freeze', body)
         freeze_mock.assert_called_once_with(req.environ['cinder.context'],
-                                            mock.sentinel.host, None)
+                                            'fake_host', None)
         self.assertEqual(freeze_mock.return_value, res)
 
     def test_services_thaw(self):
         url = '/v2/%s/os-services/thaw' % fake.PROJECT_ID
         req = fakes.HTTPRequest.blank(url)
-        body = {'host': mock.sentinel.host}
+        body = {'host': 'fake_host'}
         with mock.patch.object(self.controller.volume_api, 'thaw_host') \
                 as thaw_mock:
             res = self.controller.update(req, 'thaw', body)
         thaw_mock.assert_called_once_with(req.environ['cinder.context'],
-                                          mock.sentinel.host, None)
+                                          'fake_host', None)
         self.assertEqual(thaw_mock.return_value, res)
 
     @ddt.data('freeze', 'thaw', 'failover_host')
@@ -805,7 +807,7 @@ class ServicesTest(test.TestCase):
         body = mock.sentinel.body
         res = self.controller.update(req, 'set-log', body)
         self.assertEqual(set_log_mock.return_value, res)
-        set_log_mock.assert_called_once_with(mock.ANY, body)
+        set_log_mock.assert_called_once_with(req, mock.ANY, body=body)
 
     @mock.patch('cinder.api.contrib.services.ServiceController._get_log')
     def test_get_log(self, get_log_mock):
@@ -814,13 +816,14 @@ class ServicesTest(test.TestCase):
         body = mock.sentinel.body
         res = self.controller.update(req, 'get-log', body)
         self.assertEqual(get_log_mock.return_value, res)
-        get_log_mock.assert_called_once_with(mock.ANY, body)
+        get_log_mock.assert_called_once_with(req, mock.ANY, body=body)
 
-    def test__log_params_binaries_services_wrong_binary(self):
+    def test_get_log_wrong_binary(self):
+        req = FakeRequest(version=mv.LOG_LEVEL)
         body = {'binary': 'wrong-binary'}
-        self.assertRaises(exception.InvalidInput,
+        self.assertRaises(exception.ValidationError,
-                          self.controller._log_params_binaries_services,
+                          self.controller._get_log, req, self.context,
-                          'get-log', body)
+                          body=body)
 
     @ddt.data(None, '', '*')
     @mock.patch('cinder.objects.ServiceList.get_all')
@@ -828,7 +831,7 @@ class ServicesTest(test.TestCase):
         body = {'binary': binary, 'server': 'host1'}
         binaries, services = self.controller._log_params_binaries_services(
             mock.sentinel.context, body)
-        self.assertEqual(self.controller.LOG_BINARIES, binaries)
+        self.assertEqual(constants.LOG_BINARIES, binaries)
         self.assertEqual(service_list_mock.return_value, services)
         service_list_mock.assert_called_once_with(
             mock.sentinel.context, filters={'host_or_cluster': body['server'],
@@ -853,11 +856,18 @@ class ServicesTest(test.TestCase):
                 filters={'host_or_cluster': body['server'], 'binary': binary,
                          'is_up': True})
 
-    @ddt.data(None, '', 'wronglevel')
+    @ddt.data((None, exception.InvalidInput),
-    def test__set_log_invalid_level(self, level):
+              ('', exception.InvalidInput),
+              ('wronglevel', exception.InvalidInput))
+    @ddt.unpack
+    def test__set_log_invalid_level(self, level, exceptions):
         body = {'level': level}
-        self.assertRaises(exception.InvalidInput,
+        url = '/v3/%s/os-services/set-log' % fake.PROJECT_ID
-                          self.controller._set_log, self.context, body)
+        req = fakes.HTTPRequest.blank(url)
+        req.api_version_request = api_version.APIVersionRequest("3.32")
+        self.assertRaises(exceptions,
+                          self.controller._set_log, req, self.context,
+                          body=body)
 
     @mock.patch('cinder.utils.get_log_method')
     @mock.patch('cinder.objects.ServiceList.get_all')
@@ -873,12 +883,15 @@ class ServicesTest(test.TestCase):
             objects.Service(self.context, binary=constants.BACKUP_BINARY),
         ]
         get_all_mock.return_value = services
+        url = '/v3/%s/os-services/set-log' % fake.PROJECT_ID
+        req = fakes.HTTPRequest.blank(url)
         body = {'binary': '*', 'prefix': 'eventlet.', 'level': 'debug'}
         log_level = objects.LogLevel(prefix=body['prefix'],
                                      level=body['level'])
         with mock.patch('cinder.objects.LogLevel') as log_level_mock:
             log_level_mock.return_value = log_level
-            res = self.controller._set_log(mock.sentinel.context, body)
+            res = self.controller._set_log(req, mock.sentinel.context,
+                                           body=body)
             log_level_mock.assert_called_once_with(mock.sentinel.context,
                                                    prefix=body['prefix'],
                                                    level=body['level'])
@@ -924,12 +937,15 @@ class ServicesTest(test.TestCase):
                             host='host'),
         ]
         get_all_mock.return_value = services
+        url = '/v3/%s/os-services/get-log' % fake.PROJECT_ID
+        req = fakes.HTTPRequest.blank(url)
         body = {'binary': '*', 'prefix': 'eventlet.'}
 
         log_level = objects.LogLevel(prefix=body['prefix'])
         with mock.patch('cinder.objects.LogLevel') as log_level_mock:
             log_level_mock.return_value = log_level
-            res = self.controller._get_log(mock.sentinel.context, body)
+            res = self.controller._get_log(req, mock.sentinel.context,
+                                           body=body)
             log_level_mock.assert_called_once_with(mock.sentinel.context,
                                                    prefix=body['prefix'])
 

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

@@ -310,7 +310,7 @@ class ClustersTestCase(test.TestCase):
     @ddt.data('a' * 256, '   ')
     def test_update_wrong_disabled_reason(self, disabled_reason):
         req = FakeRequest()
-        self.assertRaises(exception.InvalidInput, self.controller.update,
+        self.assertRaises(exception.ValidationError, self.controller.update,
                           req, 'disable',
                           body={'name': 'cluster_name',
                                 'disabled_reason': disabled_reason})