From 455c9b1a2a9a10f4be11c76ff2ac5ac3cb2040b1 Mon Sep 17 00:00:00 2001 From: Ricardo Dias Date: Thu, 3 May 2018 15:24:45 +0100 Subject: [PATCH] mgr/dashboard: controller infrastructure refactor Signed-off-by: Ricardo Dias --- .../mgr/dashboard/controllers/__init__.py | 393 ++++++++++++------ src/pybind/mgr/dashboard/module.py | 9 +- src/pybind/mgr/dashboard/tests/helper.py | 2 +- .../mgr/dashboard/tests/test_exceptions.py | 4 +- .../mgr/dashboard/tests/test_rbd_mirroring.py | 6 +- .../mgr/dashboard/tests/test_rest_tasks.py | 5 +- .../mgr/dashboard/tests/test_tcmu_iscsi.py | 4 +- src/pybind/mgr/dashboard/tests/test_tools.py | 14 +- 8 files changed, 297 insertions(+), 140 deletions(-) diff --git a/src/pybind/mgr/dashboard/controllers/__init__.py b/src/pybind/mgr/dashboard/controllers/__init__.py index db6b52defb1..6dabe363dc5 100644 --- a/src/pybind/mgr/dashboard/controllers/__init__.py +++ b/src/pybind/mgr/dashboard/controllers/__init__.py @@ -20,15 +20,22 @@ from ..exceptions import ViewCacheNoDataException, DashboardException from ..services.exception import serialize_dashboard_exception -def ApiController(path): - def decorate(cls): +class Controller(object): + def __init__(self, path, base_url=""): + self.path = path + self.base_url = base_url + + def __call__(self, cls): cls._cp_controller_ = True - cls._cp_path_ = path + if self.base_url: + cls._cp_path_ = "{}/{}".format(self.base_url, self.path) + else: + cls._cp_path_ = self.path config = { 'tools.sessions.on': True, 'tools.sessions.name': Session.NAME, 'tools.session_expire_at_browser_close.on': True, - 'tools.dashboard_exception_handler.on': True, + 'tools.dashboard_exception_handler.on': True } if not hasattr(cls, '_cp_config'): cls._cp_config = {} @@ -36,7 +43,21 @@ def ApiController(path): config['tools.authenticate.on'] = False cls._cp_config.update(config) return cls - return decorate + + +class ApiController(Controller): + def __init__(self, path, version=1): + if version == 1: + base_url = "api" + else: + base_url = "api/v" + str(version) + super(ApiController, self).__init__(path, base_url) + self.version = version + + def __call__(self, cls): + cls = super(ApiController, self).__call__(cls) + cls._api_version = self.version + return cls def AuthRequired(enabled=True): @@ -82,41 +103,52 @@ def load_controllers(): return controllers +ENDPOINT_MAP = collections.defaultdict(list) + + def generate_controller_routes(ctrl_class, mapper, base_url): inst = ctrl_class() - for methods, url_suffix, action, params in ctrl_class.endpoints(): - if not url_suffix: - name = ctrl_class.__name__ - url = "{}/{}".format(base_url, ctrl_class._cp_path_) + endp_base_urls = set() + + for endpoint in ctrl_class.endpoints(): + conditions = dict(method=endpoint.methods) if endpoint.methods else None + endp_url = endpoint.url + if '/' in endp_url: + endp_base_urls.add(endp_url[:endp_url.find('/')]) else: - name = "{}:{}".format(ctrl_class.__name__, url_suffix) - url = "{}/{}/{}".format(base_url, ctrl_class._cp_path_, url_suffix) + endp_base_urls.add(endp_url) + url = "{}/{}".format(base_url, endp_url) - if params: - for param in params: - url = "{}/:{}".format(url, param) + logger.debug("Mapped [%s] to %s:%s restricted to %s", + url, ctrl_class.__name__, endpoint.action, + endpoint.methods) - conditions = dict(method=methods) if methods else None + ENDPOINT_MAP[endpoint.url].append(endpoint) - logger.debug("Mapping [%s] to %s:%s restricted to %s", - url, ctrl_class.__name__, action, methods) - mapper.connect(name, url, controller=inst, action=action, + name = ctrl_class.__name__ + ":" + endpoint.action + mapper.connect(name, url, controller=inst, action=endpoint.action, conditions=conditions) # adding route with trailing slash name += "/" url += "/" - mapper.connect(name, url, controller=inst, action=action, + mapper.connect(name, url, controller=inst, action=endpoint.action, conditions=conditions) + return endp_base_urls + def generate_routes(url_prefix): mapper = cherrypy.dispatch.RoutesDispatcher() ctrls = load_controllers() + + parent_urls = set() for ctrl in ctrls: - generate_controller_routes(ctrl, mapper, "{}/api".format(url_prefix)) + parent_urls.update(generate_controller_routes(ctrl, mapper, + "{}".format(url_prefix))) - return mapper + logger.debug("list of parent paths: %s", parent_urls) + return mapper, parent_urls def json_error_page(status, message, traceback, version): @@ -193,65 +225,137 @@ class Task(object): return wrapper +def _get_function_params(func): + """ + Retrieves the list of parameters declared in function. + Each parameter is represented as dict with keys: + * name (str): the name of the parameter + * required (bool): whether the parameter is required or not + * default (obj): the parameter's default value + """ + fspec = getargspec(func) + + func_params = [] + nd = len(fspec.args) if not fspec.defaults else -len(fspec.defaults) + for param in fspec.args[1:nd]: + func_params.append({'name': param, 'required': True}) + + if fspec.defaults: + for param, val in zip(fspec.args[nd:], fspec.defaults): + func_params.append({ + 'name': param, + 'required': False, + 'default': val + }) + + return func_params + + class BaseController(object): """ Base class for all controllers providing API endpoints. """ + class Endpoint(object): + """ + An instance of this class represents an endpoint. + """ + def __init__(self, ctrl, func, methods=None): + self.ctrl = ctrl + self.func = self._unwrap(func) + if methods is None: + methods = [] + self.methods = methods + + @classmethod + def _unwrap(cls, func): + while hasattr(func, "__wrapped__"): + func = func.__wrapped__ + return func + + @property + def url(self): + ctrl_path_params = self.ctrl.get_path_param_names() + if self.func.__name__ != '__call__': + url = "{}/{}".format(self.ctrl.get_path(), self.func.__name__) + else: + url = self.ctrl.get_path() + path_params = [ + p['name'] for p in _get_function_params(self.func) + if p['required'] and p['name'] not in ctrl_path_params] + path_params = ["{{{}}}".format(p) for p in path_params] + if path_params: + url += "/{}".format("/".join(path_params)) + return url + + @property + def action(self): + return self.func.__name__ + + @property + def path_params(self): + return [p for p in _get_function_params(self.func) if p['required']] + + @property + def query_params(self): + return [p for p in _get_function_params(self.func) + if not p['required']] + + @property + def body_params(self): + return [] + + @property + def group(self): + return self.ctrl.__name__ + + @property + def is_api(self): + return hasattr(self.ctrl, '_api_version') + + @property + def is_secure(self): + return self.ctrl._cp_config['tools.authenticate.on'] + + def __repr__(self): + return "Endpoint({}, {}, {})".format(self.url, self.methods, + self.action) + def __init__(self): - logger.info('Initializing controller: %s -> /api/%s', + logger.info('Initializing controller: %s -> /%s', self.__class__.__name__, self._cp_path_) @classmethod - def _parse_function_args(cls, func): - args = getargspec(func) - nd = len(args.args) if not args.defaults else -len(args.defaults) - cargs = args.args[1:nd] - - # filter out controller path params - for idx, step in enumerate(cls._cp_path_.split('/')): + def get_path_param_names(cls): + path_params = [] + for step in cls._cp_path_.split('/'): param = None if step[0] == ':': param = step[1:] - if step[0] == '{' and step[-1] == '}' and ':' in step[1:-1]: - param, _, _regex = step[1:-1].partition(':') - + elif step[0] == '{' and step[-1] == '}': + param, _, _ = step[1:-1].partition(':') if param: - if param not in cargs: - raise Exception("function '{}' does not have the" - " positional argument '{}' in the {} " - "position".format(func, param, idx)) - cargs.remove(param) - return cargs + path_params.append(param) + return path_params + + @classmethod + def get_path(cls): + return cls._cp_path_ @classmethod def endpoints(cls): """ - The endpoints method returns a list of endpoints. Each endpoint - consists of a tuple with methods, URL suffix, an action and its - arguments. - - By default, endpoints will be methods of the BaseController class, - which have been decorated by the @cherrpy.expose decorator. A method - will also be considered an endpoint if the `exposed` attribute has been - set on the method to a value which evaluates to True, which is - basically what @cherrpy.expose does, too. - - :return: A tuple of methods, url_suffix, action and arguments of the - function - :rtype: list[tuple] + This method iterates over all the methods decorated with ``@endpoint`` + and creates an Endpoint object for each one of the methods. + + :return: A list of endpoint objects + :rtype: list[BaseController.Endpoint] """ result = [] - for name, func in inspect.getmembers(cls, predicate=callable): + for _, func in inspect.getmembers(cls, predicate=callable): if hasattr(func, 'exposed') and func.exposed: - args = cls._parse_function_args(func) - methods = [] - url_suffix = name - action = name - if name == '__call__': - url_suffix = None - result.append((methods, url_suffix, action, args)) + result.append(cls.Endpoint(cls, func)) return result @@ -282,78 +386,123 @@ class RESTController(BaseController): # resource id parameter for using in get, set, and delete methods # should be overriden by subclasses. # to specify a composite id (two parameters) use '/'. e.g., "param1/param2". - # If subclasses don't override this property we try to infer the structure of - # the resourse ID. + # If subclasses don't override this property we try to infer the structure + # of the resourse ID. RESOURCE_ID = None _method_mapping = collections.OrderedDict([ - (('GET', False), ('list', 200)), - (('PUT', False), ('bulk_set', 200)), - (('PATCH', False), ('bulk_set', 200)), - (('POST', False), ('create', 201)), - (('DELETE', False), ('bulk_delete', 204)), - (('GET', True), ('get', 200)), - (('DELETE', True), ('delete', 204)), - (('PUT', True), ('set', 200)), - (('PATCH', True), ('set', 200)) + ('list', {'method': 'GET', 'resource': False, 'status': 200}), + ('create', {'method': 'POST', 'resource': False, 'status': 201}), + ('bulk_set', {'method': 'PUT', 'resource': False, 'status': 200}), + ('bulk_delete', {'method': 'DELETE', 'resource': False, 'status': 204}), + ('get', {'method': 'GET', 'resource': True, 'status': 200}), + ('delete', {'method': 'DELETE', 'resource': True, 'status': 204}), + ('set', {'method': 'PUT', 'resource': True, 'status': 200}) ]) - @classmethod - def endpoints(cls): - # pylint: disable=too-many-branches - - result = [] - for attr, val in inspect.getmembers(cls, predicate=callable): - if hasattr(val, 'exposed') and val.exposed and \ - attr != '_collection' and attr != '_element': - result.append(([], attr, attr, cls._parse_function_args(val))) - - for k, v in cls._method_mapping.items(): - func = getattr(cls, v[0], None) - if not k[1] and func: - if k[0] != 'PATCH': # we already wrapped in PUT - wrapper = cls._rest_request_wrapper(func, v[1]) - setattr(cls, v[0], wrapper) - else: - wrapper = func - result.append(([k[0]], None, v[0], [])) + class RESTEndpoint(BaseController.Endpoint): + def __init__(self, ctrl, func): + if func.__name__ in ctrl._method_mapping: + methods = [ctrl._method_mapping[func.__name__]['method']] + status = ctrl._method_mapping[func.__name__]['status'] + elif hasattr(func, "_resource_method_"): + methods = func._resource_method_ + status = 200 + elif hasattr(func, "_collection_method_"): + methods = func._collection_method_ + status = 200 + else: + assert False + + wrapper = ctrl._rest_request_wrapper(func, status) + setattr(ctrl, func.__name__, wrapper) + + super(RESTController.RESTEndpoint, self).__init__( + ctrl, func, methods) + + def get_resource_id_params(self): + if self.func.__name__ in self.ctrl._method_mapping: + if self.ctrl._method_mapping[self.func.__name__]['resource']: + resource_id_params = self.ctrl.infer_resource_id() + if resource_id_params: + return resource_id_params + + if hasattr(self.func, '_resource_method_'): + resource_id_params = self.ctrl.infer_resource_id() + if resource_id_params: + return resource_id_params + + return [] + + @property + def url(self): + url = self.ctrl.get_path() + + res_id_params = self.get_resource_id_params() + if res_id_params: + res_id_params = ["{{{}}}".format(p) for p in res_id_params] + url += "/{}".format("/".join(res_id_params)) + + if hasattr(self.func, "_collection_method_") \ + or hasattr(self.func, "_resource_method_"): + url += "/{}".format(self.func.__name__) + return url + + @property + def path_params(self): + params = [{'name': p, 'required': True} + for p in self.ctrl.get_path_param_names()] + params.extend([{'name': p, 'required': True} + for p in self.get_resource_id_params()]) + return params + + @property + def query_params(self): + path_params_names = [p['name'] for p in self.path_params] + if 'GET' in self.methods or 'DELETE' in self.methods: + return [p for p in _get_function_params(self.func) + if p['name'] not in path_params_names] + return [] + + @property + def body_params(self): + path_params_names = [p['name'] for p in self.path_params] + if 'POST' in self.methods or 'PUT' in self.methods: + return [p for p in _get_function_params(self.func) + if p['name'] not in path_params_names] + return [] - args = [] + @classmethod + def infer_resource_id(cls): + if cls.RESOURCE_ID is not None: + return cls.RESOURCE_ID.split('/') for k, v in cls._method_mapping.items(): - func = getattr(cls, v[0], None) - if k[1] and func: - if k[0] != 'PATCH': # we already wrapped in PUT - wrapper = cls._rest_request_wrapper(func, v[1]) - setattr(cls, v[0], wrapper) - else: - wrapper = func - if not args: - if cls.RESOURCE_ID is None: - args = cls._parse_function_args(func) - else: - args = cls.RESOURCE_ID.split('/') - result.append(([k[0]], None, v[0], args)) - - for attr, val in inspect.getmembers(cls, predicate=callable): - if hasattr(val, '_collection_method_'): - wrapper = cls._rest_request_wrapper(val, 200) - setattr(cls, attr, wrapper) - result.append( - (val._collection_method_, attr, attr, [])) - - for attr, val in inspect.getmembers(cls, predicate=callable): - if hasattr(val, '_resource_method_'): - wrapper = cls._rest_request_wrapper(val, 200) - setattr(cls, attr, wrapper) - res_params = [":{}".format(arg) for arg in args] - url_suffix = "{}/{}".format("/".join(res_params), attr) - result.append( - (val._resource_method_, url_suffix, attr, [])) + func = getattr(cls, k, None) + while hasattr(func, "__wrapped__"): + func = func.__wrapped__ + if v['resource'] and func: + path_params = cls.get_path_param_names() + params = _get_function_params(func) + return [p['name'] for p in params + if p['required'] and p['name'] not in path_params] + return None + @classmethod + def endpoints(cls): + result = [] + for _, val in inspect.getmembers(cls, predicate=callable): + if val.__name__ in cls._method_mapping: + result.append(cls.RESTEndpoint(cls, val)) + elif hasattr(val, "_collection_method_") \ + or hasattr(val, "_resource_method_"): + result.append(cls.RESTEndpoint(cls, val)) + elif hasattr(val, 'exposed') and val.exposed: + result.append(cls.Endpoint(cls, val)) return result @classmethod def _rest_request_wrapper(cls, func, status_code): + @wraps(func) def wrapper(*vpath, **params): method = func if cherrypy.request.method not in ['GET', 'DELETE']: @@ -364,6 +513,8 @@ class RESTController(BaseController): cherrypy.response.status = status_code return method(*vpath, **params) + if not hasattr(wrapper, '__wrapped__'): + wrapper.__wrapped__ = func return wrapper @staticmethod @@ -373,8 +524,8 @@ class RESTController(BaseController): @staticmethod def _takes_json(func): def inner(*args, **kwargs): - if cherrypy.request.headers.get('Content-Type', - '') == 'application/x-www-form-urlencoded': + if cherrypy.request.headers.get('Content-Type', '') == \ + 'application/x-www-form-urlencoded': return func(*args, **kwargs) content_length = int(cherrypy.request.headers['Content-Length']) diff --git a/src/pybind/mgr/dashboard/module.py b/src/pybind/mgr/dashboard/module.py index 55bf33d8029..5ed9525f3e2 100644 --- a/src/pybind/mgr/dashboard/module.py +++ b/src/pybind/mgr/dashboard/module.py @@ -276,16 +276,19 @@ class Module(MgrModule, SSLCherryPyConfig): # about to start serving self.set_uri(uri) - mapper = generate_routes(self.url_prefix) + mapper, parent_urls = generate_routes(self.url_prefix) config = { '{}/'.format(self.url_prefix): { 'tools.staticdir.on': True, 'tools.staticdir.dir': self.get_frontend_path(), 'tools.staticdir.index': 'index.html' - }, - '{}/api'.format(self.url_prefix): {'request.dispatch': mapper} + } } + for purl in parent_urls: + config['{}/{}'.format(self.url_prefix, purl)] = { + 'request.dispatch': mapper + } cherrypy.tree.mount(None, config=config) cherrypy.engine.start() diff --git a/src/pybind/mgr/dashboard/tests/helper.py b/src/pybind/mgr/dashboard/tests/helper.py index 76702867682..e98a770c621 100644 --- a/src/pybind/mgr/dashboard/tests/helper.py +++ b/src/pybind/mgr/dashboard/tests/helper.py @@ -91,7 +91,7 @@ class ControllerTestCase(helper.CPWebCase): logger.info("task (%s, %s) is still executing", self.task_name, self.task_metadata) time.sleep(1) - self.tc._get('/task?name={}'.format(self.task_name)) + self.tc._get('/api/task?name={}'.format(self.task_name)) res = self.tc.jsonBody() for task in res['finished_tasks']: if task['metadata'] == self.task_metadata: diff --git a/src/pybind/mgr/dashboard/tests/test_exceptions.py b/src/pybind/mgr/dashboard/tests/test_exceptions.py index 4fc9ef7f8fc..98f80265ef8 100644 --- a/src/pybind/mgr/dashboard/tests/test_exceptions.py +++ b/src/pybind/mgr/dashboard/tests/test_exceptions.py @@ -7,7 +7,7 @@ import cherrypy import rados from ..services.ceph_service import SendCommandError -from ..controllers import RESTController, ApiController, Task +from ..controllers import RESTController, Controller, Task from .helper import ControllerTestCase from ..services.exception import handle_rados_error, handle_send_command_error, \ serialize_dashboard_exception @@ -15,7 +15,7 @@ from ..tools import ViewCache, TaskManager, NotificationQueue # pylint: disable=W0613 -@ApiController('foo') +@Controller('foo') class FooResource(RESTController): @cherrypy.expose diff --git a/src/pybind/mgr/dashboard/tests/test_rbd_mirroring.py b/src/pybind/mgr/dashboard/tests/test_rbd_mirroring.py index c4d04c22449..479b133e047 100644 --- a/src/pybind/mgr/dashboard/tests/test_rbd_mirroring.py +++ b/src/pybind/mgr/dashboard/tests/test_rbd_mirroring.py @@ -64,11 +64,11 @@ class RbdMirroringControllerTest(ControllerTestCase): Summary._cp_config['tools.authenticate.on'] = False # pylint: disable=protected-access - cls.setup_controllers([RbdMirror, Summary], '/api/test') + cls.setup_controllers([RbdMirror, Summary], '/test') @mock.patch('dashboard.controllers.rbd_mirroring.rbd') def test_default(self, rbd_mock): # pylint: disable=W0613 - self._get('/api/test/rbdmirror') + self._get('/test/api/rbdmirror') result = self.jsonBody() self.assertStatus(200) self.assertEqual(result['status'], 0) @@ -78,7 +78,7 @@ class RbdMirroringControllerTest(ControllerTestCase): @mock.patch('dashboard.controllers.rbd_mirroring.rbd') def test_summary(self, rbd_mock): # pylint: disable=W0613 """We're also testing `summary`, as it also uses code from `rbd_mirroring.py`""" - self._get('/api/test/summary') + self._get('/test/api/summary') self.assertStatus(200) summary = self.jsonBody()['rbd_mirroring'] diff --git a/src/pybind/mgr/dashboard/tests/test_rest_tasks.py b/src/pybind/mgr/dashboard/tests/test_rest_tasks.py index e5c8b068a60..3a23c115566 100644 --- a/src/pybind/mgr/dashboard/tests/test_rest_tasks.py +++ b/src/pybind/mgr/dashboard/tests/test_rest_tasks.py @@ -4,12 +4,12 @@ import time from .helper import ControllerTestCase -from ..controllers import ApiController, RESTController, Task +from ..controllers import Controller, RESTController, Task from ..controllers.task import Task as TaskController from ..tools import NotificationQueue, TaskManager -@ApiController('test/task') +@Controller('test/task') class TaskTest(RESTController): sleep_time = 0.0 @@ -45,6 +45,7 @@ class TaskControllerTest(ControllerTestCase): # pylint: disable=protected-access NotificationQueue.start_queue() TaskManager.init() + TaskTest._cp_config['tools.authenticate.on'] = False TaskController._cp_config['tools.authenticate.on'] = False cls.setup_controllers([TaskTest, TaskController]) diff --git a/src/pybind/mgr/dashboard/tests/test_tcmu_iscsi.py b/src/pybind/mgr/dashboard/tests/test_tcmu_iscsi.py index f7b3ff08f85..d189c12ba29 100644 --- a/src/pybind/mgr/dashboard/tests/test_tcmu_iscsi.py +++ b/src/pybind/mgr/dashboard/tests/test_tcmu_iscsi.py @@ -71,10 +71,10 @@ class TcmuIscsiControllerTest(ControllerTestCase): mgr.url_prefix = '' TcmuIscsi._cp_config['tools.authenticate.on'] = False # pylint: disable=protected-access - cls.setup_controllers(TcmuIscsi, "/api/test") + cls.setup_controllers(TcmuIscsi, "/test") def test_list(self): - self._get('/api/test/tcmuiscsi') + self._get('/test/api/tcmuiscsi') self.assertStatus(200) self.assertJsonBody({ 'daemons': [ diff --git a/src/pybind/mgr/dashboard/tests/test_tools.py b/src/pybind/mgr/dashboard/tests/test_tools.py index 0972c51008c..69c8b697b14 100644 --- a/src/pybind/mgr/dashboard/tests/test_tools.py +++ b/src/pybind/mgr/dashboard/tests/test_tools.py @@ -9,11 +9,13 @@ from mock import patch from ..services.exception import handle_rados_error from .helper import ControllerTestCase -from ..controllers import RESTController, ApiController, BaseController +from ..controllers import RESTController, ApiController, Controller, \ + BaseController from ..tools import is_valid_ipv6_address, dict_contains_path -@ApiController('foo') +# pylint: disable=W0613 +@Controller('foo') class FooResource(RESTController): elems = [] @@ -38,7 +40,7 @@ class FooResource(RESTController): return dict(key=key, newdata=newdata) -@ApiController('foo/:key/:method') +@Controller('foo/:key/:method') class FooResourceDetail(RESTController): def list(self, key, method): return {'detail': (key, [method])} @@ -115,13 +117,13 @@ class RESTControllerTest(ControllerTestCase): assert 'traceback' in body def test_args_from_json(self): - self._put("/fooargs/hello", {'name': 'world'}) + self._put("/api/fooargs/hello", {'name': 'world'}) self.assertJsonBody({'code': 'hello', 'name': 'world', 'opt1': None, 'opt2': None}) - self._put("/fooargs/hello", {'name': 'world', 'opt1': 'opt1'}) + self._put("/api/fooargs/hello", {'name': 'world', 'opt1': 'opt1'}) self.assertJsonBody({'code': 'hello', 'name': 'world', 'opt1': 'opt1', 'opt2': None}) - self._put("/fooargs/hello", {'name': 'world', 'opt2': 'opt2'}) + self._put("/api/fooargs/hello", {'name': 'world', 'opt2': 'opt2'}) self.assertJsonBody({'code': 'hello', 'name': 'world', 'opt1': None, 'opt2': 'opt2'}) def test_detail_route(self): -- 2.39.5