From aea5663c51e1154f697bdb32a73daac2b66f3c85 Mon Sep 17 00:00:00 2001 From: Aashish Sharma Date: Wed, 19 May 2021 11:27:13 +0530 Subject: [PATCH] mgr/dashboard: API Version changes doesnt appy to pre-defined methods(list, create etc.) Methods like list(), create(), get() etc doesn't get applied the version.Also for the endpoints that get the version changed, the docs and the request header has still the version v1.0+ in them. So with the version reduced it gives 415 error when trying to make the request. This PR fixes this issue. Fixes: https://tracker.ceph.com/issues/50855 Signed-off-by: Aashish Sharma (cherry picked from commit bb52092cd37855a3dfcfae9287e231865e9af3fa) Conflicts: src/pybind/mgr/dashboard/controllers/__init__.py(resolved conflicts due to https://github.com/ceph/ceph/pull/40063) src/pybind/mgr/dashboard/tests/test_docs.py(removed unused import DEFAULT_VERSION) --- doc/dev/developer_guide/dash-devel.rst | 22 ++++++- .../mgr/dashboard/controllers/__init__.py | 65 ++++++++++++------- src/pybind/mgr/dashboard/controllers/docs.py | 35 +++++++--- src/pybind/mgr/dashboard/tests/test_docs.py | 20 ++++-- .../mgr/dashboard/tests/test_versioning.py | 10 +++ 5 files changed, 112 insertions(+), 40 deletions(-) diff --git a/doc/dev/developer_guide/dash-devel.rst b/doc/dev/developer_guide/dash-devel.rst index 130a6b9a65d40..7f02e36cffa4c 100644 --- a/doc/dev/developer_guide/dash-devel.rst +++ b/doc/dev/developer_guide/dash-devel.rst @@ -1466,6 +1466,25 @@ same applies to other request types: | DELETE | Yes | delete | 204 | +--------------+------------+----------------+-------------+ +To use a custom endpoint for the above listed methods, you can +use ``@RESTController.MethodMap`` + +.. code-block:: python + + import cherrypy + from ..tools import ApiController, RESTController + + @RESTController.MethodMap(version='0.1') + def create(self): + return {"msg": "Hello"} + +This decorator supports three parameters to customize the +endpoint: + +* ``resource"``: resource id. +* ``status=200``: set the HTTP status response code +* ``version``: version + How to use a custom API endpoint in a RESTController? ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -1494,7 +1513,7 @@ used. To use a custom endpoint inside a restricted ``RESTController`` use def some_post_endpoint(self, **data): return {"msg": data} -Both decorators also support four parameters to customize the +Both decorators also support five parameters to customize the endpoint: * ``method="GET"``: the HTTP method allowed to access this endpoint. @@ -1503,6 +1522,7 @@ endpoint: * ``status=200``: set the HTTP status response code * ``query_params=[]``: list of method parameter names that correspond to URL query parameters. +* ``version``: version How to restrict access to a controller? ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/pybind/mgr/dashboard/controllers/__init__.py b/src/pybind/mgr/dashboard/controllers/__init__.py index 77b5b4747364d..7daf2301d351f 100644 --- a/src/pybind/mgr/dashboard/controllers/__init__.py +++ b/src/pybind/mgr/dashboard/controllers/__init__.py @@ -788,14 +788,14 @@ class RESTController(BaseController): } _method_mapping = collections.OrderedDict([ - ('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}), - ('singleton_set', {'method': 'PUT', 'resource': False, 'status': 200}) + ('list', {'method': 'GET', 'resource': False, 'status': 200, 'version': DEFAULT_VERSION}), + ('create', {'method': 'POST', 'resource': False, 'status': 201, 'version': DEFAULT_VERSION}), # noqa E501 #pylint: disable=line-too-long + ('bulk_set', {'method': 'PUT', 'resource': False, 'status': 200, 'version': DEFAULT_VERSION}), # noqa E501 #pylint: disable=line-too-long + ('bulk_delete', {'method': 'DELETE', 'resource': False, 'status': 204, 'version': DEFAULT_VERSION}), # noqa E501 #pylint: disable=line-too-long + ('get', {'method': 'GET', 'resource': True, 'status': 200, 'version': DEFAULT_VERSION}), + ('delete', {'method': 'DELETE', 'resource': True, 'status': 204, 'version': DEFAULT_VERSION}), # noqa E501 #pylint: disable=line-too-long + ('set', {'method': 'PUT', 'resource': True, 'status': 200, 'version': DEFAULT_VERSION}), + ('singleton_set', {'method': 'PUT', 'resource': False, 'status': 200, 'version': DEFAULT_VERSION}) # noqa E501 #pylint: disable=line-too-long ]) @classmethod @@ -840,35 +840,37 @@ class RESTController(BaseController): status = meth['status'] method = meth['method'] + if hasattr(func, "__method_map_method__"): + version = func.__method_map_method__['version'] if not sec_permissions: permission = cls._permission_map[method] - elif hasattr(func, "_collection_method_"): - if func._collection_method_['path']: - path = func._collection_method_['path'] + elif hasattr(func, "__collection_method__"): + if func.__collection_method__['path']: + path = func.__collection_method__['path'] else: path = "/{}".format(func.__name__) - status = func._collection_method_['status'] - method = func._collection_method_['method'] - query_params = func._collection_method_['query_params'] - version = func._collection_method_['version'] + status = func.__collection_method__['status'] + method = func.__collection_method__['method'] + query_params = func.__collection_method__['query_params'] + version = func.__collection_method__['version'] if not sec_permissions: permission = cls._permission_map[method] - elif hasattr(func, "_resource_method_"): + elif hasattr(func, "__resource_method__"): if not res_id_params: no_resource_id_params = True else: path_params = ["{{{}}}".format(p) for p in res_id_params] path += "/{}".format("/".join(path_params)) - if func._resource_method_['path']: - path += func._resource_method_['path'] + if func.__resource_method__['path']: + path += func.__resource_method__['path'] else: path += "/{}".format(func.__name__) - status = func._resource_method_['status'] - method = func._resource_method_['method'] - version = func._resource_method_['version'] - query_params = func._resource_method_['query_params'] + status = func.__resource_method__['status'] + method = func.__resource_method__['method'] + version = func.__resource_method__['version'] + query_params = func.__resource_method__['query_params'] if not sec_permissions: permission = cls._permission_map[method] @@ -918,7 +920,7 @@ class RESTController(BaseController): status = 200 def _wrapper(func): - func._resource_method_ = { + func.__resource_method__ = { 'method': method, 'path': path, 'status': status, @@ -928,6 +930,21 @@ class RESTController(BaseController): return func return _wrapper + @staticmethod + def MethodMap(resource=False, status=None, version=DEFAULT_VERSION): # noqa: N802 + + if status is None: + status = 200 + + def _wrapper(func): + func.__method_map_method__ = { + 'resource': resource, + 'status': status, + 'version': version + } + return func + return _wrapper + @staticmethod def Collection(method=None, path=None, status=None, query_params=None, # noqa: N802 version=DEFAULT_VERSION): @@ -938,7 +955,7 @@ class RESTController(BaseController): status = 200 def _wrapper(func): - func._collection_method_ = { + func.__collection_method__ = { 'method': method, 'path': path, 'status': status, diff --git a/src/pybind/mgr/dashboard/controllers/docs.py b/src/pybind/mgr/dashboard/controllers/docs.py index e7ed9742ab9d2..87c46e327bd60 100644 --- a/src/pybind/mgr/dashboard/controllers/docs.py +++ b/src/pybind/mgr/dashboard/controllers/docs.py @@ -185,7 +185,7 @@ class Docs(BaseController): return schema.as_dict() @classmethod - def _gen_responses(cls, method, resp_object=None): + def _gen_responses(cls, method, resp_object=None, version=None): resp: Dict[str, Dict[str, Union[str, Any]]] = { '400': { "description": "Operation exception. Please check the " @@ -203,26 +203,30 @@ class Docs(BaseController): "response body for the stack trace." } } + + if not version: + version = DEFAULT_VERSION + if method.lower() == 'get': resp['200'] = {'description': "OK", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if method.lower() == 'post': resp['201'] = {'description': "Resource created.", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if method.lower() == 'put': resp['200'] = {'description': "Resource updated.", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if method.lower() == 'delete': resp['204'] = {'description': "Resource deleted.", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if method.lower() in ['post', 'put', 'delete']: resp['202'] = {'description': "Operation is still executing." " Please check the task queue.", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if resp_object: @@ -230,7 +234,7 @@ class Docs(BaseController): if status_code in resp: resp[status_code].update({ 'content': { - 'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): { + 'application/vnd.ceph.api.v{}+json'.format(version): { 'schema': cls._gen_schema_for_content(response_body)}}}) return resp @@ -263,7 +267,7 @@ class Docs(BaseController): return parameters @classmethod - def _gen_paths(cls, all_endpoints): + def gen_paths(cls, all_endpoints): # pylint: disable=R0912 method_order = ['get', 'post', 'put', 'delete'] paths = {} @@ -283,8 +287,19 @@ class Docs(BaseController): func = endpoint.func summary = '' + version = '' resp = {} p_info = [] + + if hasattr(func, 'method_map_method'): + version = func.method_map_method['version'] + + elif hasattr(func, 'resource_method'): + version = func.resource_method['version'] + + elif hasattr(func, 'collection_method'): + version = func.collection_method['version'] + if hasattr(func, 'doc_info'): if func.doc_info['summary']: summary = func.doc_info['summary'] @@ -304,7 +319,7 @@ class Docs(BaseController): 'tags': [cls._get_tag(endpoint)], 'description': func.__doc__, 'parameters': params, - 'responses': cls._gen_responses(method, resp) + 'responses': cls._gen_responses(method, resp, version) } if summary: methods[method.lower()]['summary'] = summary @@ -340,7 +355,7 @@ class Docs(BaseController): host = cherrypy.request.base.split('://', 1)[1] if not offline else 'example.com' logger.debug("Host: %s", host) - paths = cls._gen_paths(all_endpoints) + paths = cls.gen_paths(all_endpoints) if not base_url: base_url = "/" diff --git a/src/pybind/mgr/dashboard/tests/test_docs.py b/src/pybind/mgr/dashboard/tests/test_docs.py index 7f4ea64003e13..623d5baf6efa1 100644 --- a/src/pybind/mgr/dashboard/tests/test_docs.py +++ b/src/pybind/mgr/dashboard/tests/test_docs.py @@ -30,10 +30,14 @@ class DecoratedController(RESTController): }, ) @Endpoint(json_response=False) - @RESTController.Resource('PUT') + @RESTController.Resource('PUT', version='0.1') def decorated_func(self, parameter): pass + @RESTController.MethodMap(version='0.1') + def list(self): + pass + # To assure functionality of @EndpointDoc, @GroupDoc class DocDecoratorsTest(ControllerTestCase): @@ -75,7 +79,7 @@ class DocsTest(ControllerTestCase): self.assertEqual(Docs()._type_to_str(None), str(SchemaType.OBJECT)) def test_gen_paths(self): - outcome = Docs()._gen_paths(False)['/api/doctest//{doctest}/decorated_func']['put'] + outcome = Docs().gen_paths(False)['/api/doctest//{doctest}/decorated_func']['put'] self.assertIn('tags', outcome) self.assertIn('summary', outcome) self.assertIn('parameters', outcome) @@ -83,7 +87,7 @@ class DocsTest(ControllerTestCase): expected_response_content = { '200': { - 'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): { + 'application/vnd.ceph.api.v0.1+json': { 'schema': {'type': 'array', 'items': {'type': 'object', 'properties': { 'my_prop': { @@ -91,7 +95,7 @@ class DocsTest(ControllerTestCase): 'description': '200 property desc.'}}}, 'required': ['my_prop']}}}, '202': { - 'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): { + 'application/vnd.ceph.api.v0.1+json': { 'schema': {'type': 'object', 'properties': {'my_prop': { 'type': 'string', @@ -104,8 +108,14 @@ class DocsTest(ControllerTestCase): # Check that a schema of type 'object' is received in the response. self.assertEqual(expected_response_content['202'], outcome['responses']['202']['content']) + def test_gen_method_paths(self): + outcome = Docs().gen_paths(False)['/api/doctest/']['get'] + + self.assertEqual({'application/vnd.ceph.api.v0.1+json': {'type': 'object'}}, + outcome['responses']['200']['content']) + def test_gen_paths_all(self): - paths = Docs()._gen_paths(False) + paths = Docs().gen_paths(False) for key in paths: self.assertTrue(any(base in key.split('/')[1] for base in ['api', 'ui-api'])) diff --git a/src/pybind/mgr/dashboard/tests/test_versioning.py b/src/pybind/mgr/dashboard/tests/test_versioning.py index d051e0550a488..666b388bf2884 100644 --- a/src/pybind/mgr/dashboard/tests/test_versioning.py +++ b/src/pybind/mgr/dashboard/tests/test_versioning.py @@ -11,6 +11,7 @@ from . import ControllerTestCase # pylint: disable=no-name-in-module class VTest(RESTController): RESOURCE_ID = "vid" + @RESTController.MethodMap(version="0.1") def list(self): return {'version': ""} @@ -31,6 +32,15 @@ class RESTVersioningTest(ControllerTestCase, unittest.TestCase): def setup_server(cls): cls.setup_controllers([VTest], "/test") + def test_list(self): + for (version, expected_status) in [ + ("0.1", 200), + ("2.0", 415) + ]: + with self.subTest(version=version): + self._get('/test/api/vtest', version=version) + self.assertStatus(expected_status) + def test_v1(self): for (version, expected_status) in [ ("1.0", 200), -- 2.39.5