From ee14ef7c6095d0572e90b29580f2d8d6982dcbda Mon Sep 17 00:00:00 2001 From: Kiefer Chang Date: Wed, 9 Sep 2020 16:20:32 +0800 Subject: [PATCH] mgr/dashboard: fix error when typing existing paths in the Ganesha form - The `CephFS.ls_dir()` implementation in the backend had changed, the code in the UI endpoint `/ui-api/nfs-ganesha/lsdir` needs to adapt. - Add fs_name as resource_id in `/ui-api/nfs-ganesha/lsdir/` to distinguish FS. - Add more checks and unit tests. Fixes: https://tracker.ceph.com/issues/47372 Signed-off-by: Kiefer Chang --- qa/tasks/mgr/dashboard/test_ganesha.py | 9 +- .../mgr/dashboard/controllers/nfsganesha.py | 41 ++++--- .../ceph/nfs/nfs-form/nfs-form.component.ts | 3 +- .../src/app/shared/api/nfs.service.spec.ts | 4 +- .../src/app/shared/api/nfs.service.ts | 4 +- .../mgr/dashboard/tests/test_ganesha.py | 101 +++++++++++++++++- 6 files changed, 140 insertions(+), 22 deletions(-) diff --git a/qa/tasks/mgr/dashboard/test_ganesha.py b/qa/tasks/mgr/dashboard/test_ganesha.py index c99e3651ff144..581cf4464d236 100644 --- a/qa/tasks/mgr/dashboard/test_ganesha.py +++ b/qa/tasks/mgr/dashboard/test_ganesha.py @@ -181,8 +181,13 @@ class GaneshaTest(DashboardTestCase): }))) def test_ganesha_lsdir(self): - self._get('/ui-api/nfs-ganesha/lsdir') - self.assertStatus(500) + fss = self._get('/ui-api/nfs-ganesha/cephfs/filesystems') + self.assertStatus(200) + for fs in fss: + data = self._get('/ui-api/nfs-ganesha/lsdir/{}'.format(fs['name'])) + self.assertStatus(200) + self.assertSchema(data, JObj({'paths': JList(str)})) + self.assertEqual(data['paths'][0], '/') def test_ganesha_buckets(self): data = self._get('/ui-api/nfs-ganesha/rgw/buckets') diff --git a/src/pybind/mgr/dashboard/controllers/nfsganesha.py b/src/pybind/mgr/dashboard/controllers/nfsganesha.py index fae04e5e1b81d..1b803906947cf 100644 --- a/src/pybind/mgr/dashboard/controllers/nfsganesha.py +++ b/src/pybind/mgr/dashboard/controllers/nfsganesha.py @@ -2,6 +2,7 @@ from __future__ import absolute_import import logging +import os from functools import partial import cephfs @@ -10,7 +11,7 @@ import cherrypy from ..security import Scope from ..services.cephfs import CephFS from ..services.cephx import CephX -from ..services.exception import serialize_dashboard_exception +from ..services.exception import DashboardException, serialize_dashboard_exception from ..services.ganesha import Ganesha, GaneshaConf, NFSException from ..services.rgw_client import RgwClient from . import ApiController, BaseController, ControllerDoc, Endpoint, \ @@ -281,21 +282,35 @@ class NFSGaneshaUi(BaseController): @Endpoint('GET', '/lsdir') @ReadPermission - def lsdir(self, root_dir=None, depth=1): # pragma: no cover + def lsdir(self, fs_name, root_dir=None, depth=1): # pragma: no cover if root_dir is None: root_dir = "/" - depth = int(depth) - if depth > 5: - logger.warning("Limiting depth to maximum value of 5: " - "input depth=%s", depth) - depth = 5 - root_dir = '{}{}'.format(root_dir.rstrip('/'), '/') + if not root_dir.startswith('/'): + root_dir = '/{}'.format(root_dir) + root_dir = os.path.normpath(root_dir) + + try: + depth = int(depth) + error_msg = '' + if depth < 0: + error_msg = '`depth` must be greater or equal to 0.' + if depth > 5: + logger.warning("Limiting depth to maximum value of 5: " + "input depth=%s", depth) + depth = 5 + except ValueError: + error_msg = '`depth` must be an integer.' + finally: + if error_msg: + raise DashboardException(code=400, + component='nfsganesha', + msg=error_msg) + try: - cfs = CephFS() - root_dir = root_dir.encode() - paths = cfs.ls_dir(root_dir, depth) - # Convert (bytes => string) and prettify paths (strip slashes). - paths = [p.decode().rstrip('/') for p in paths if p != root_dir] + cfs = CephFS(fs_name) + paths = [root_dir] + paths.extend([p['path'].rstrip('/') + for p in cfs.ls_dir(root_dir, depth)]) except (cephfs.ObjectNotFound, cephfs.PermissionError): paths = [] return {'paths': paths} diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-form/nfs-form.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-form/nfs-form.component.ts index b07ecf4a6d34b..52568696f91a5 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-form/nfs-form.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/nfs/nfs-form/nfs-form.component.ts @@ -384,7 +384,8 @@ export class NfsFormComponent extends CdForm implements OnInit { return of([]); } - return this.nfsService.lsDir(path); + const fsName = this.nfsForm.getValue('fsal').fs_name; + return this.nfsService.lsDir(fsName, path); } pathChangeHandler() { diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/nfs.service.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/nfs.service.spec.ts index b90e816d97927..247be05b234ec 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/nfs.service.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/nfs.service.spec.ts @@ -59,8 +59,8 @@ describe('NfsService', () => { }); it('should call lsDir', () => { - service.lsDir('foo_dir').subscribe(); - const req = httpTesting.expectOne('ui-api/nfs-ganesha/lsdir?root_dir=foo_dir'); + service.lsDir('a', 'foo_dir').subscribe(); + const req = httpTesting.expectOne('ui-api/nfs-ganesha/lsdir/a?root_dir=foo_dir'); expect(req.request.method).toBe('GET'); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/nfs.service.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/nfs.service.ts index e9858416919a3..997faab9344e4 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/nfs.service.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/nfs.service.ts @@ -68,8 +68,8 @@ export class NfsService { }); } - lsDir(root_dir: string) { - return this.http.get(`${this.uiApiPath}/lsdir?root_dir=${root_dir}`); + lsDir(fs_name: string, root_dir: string) { + return this.http.get(`${this.uiApiPath}/lsdir/${fs_name}?root_dir=${root_dir}`); } buckets(user_id: string) { diff --git a/src/pybind/mgr/dashboard/tests/test_ganesha.py b/src/pybind/mgr/dashboard/tests/test_ganesha.py index 8e2d5804395d8..2151d5269a0b2 100644 --- a/src/pybind/mgr/dashboard/tests/test_ganesha.py +++ b/src/pybind/mgr/dashboard/tests/test_ganesha.py @@ -2,18 +2,21 @@ from __future__ import absolute_import import unittest +from urllib.parse import urlencode try: - from mock import MagicMock, Mock + from mock import MagicMock, Mock, patch except ImportError: - from unittest.mock import MagicMock, Mock + from unittest.mock import MagicMock, Mock, patch import orchestrator from .. import mgr +from ..controllers.nfsganesha import NFSGaneshaUi from ..services import ganesha from ..services.ganesha import Export, GaneshaConf, GaneshaConfParser from ..settings import Settings +from . import ControllerTestCase # pylint: disable=no-name-in-module from . import KVStoreMockMixin # pylint: disable=no-name-in-module @@ -644,3 +647,97 @@ EXPORT self.assertEqual(export.cluster_id, '_default_') self.assertEqual(export.attr_expiration_time, 0) self.assertEqual(export.security_label, True) + + +class NFSGaneshaUiControllerTest(ControllerTestCase): + @classmethod + def setup_server(cls): + # pylint: disable=protected-access + NFSGaneshaUi._cp_config['tools.authenticate.on'] = False + cls.setup_controllers([NFSGaneshaUi]) + + @classmethod + def _create_ls_dir_url(cls, fs_name, query_params): + api_url = '/ui-api/nfs-ganesha/lsdir/{}'.format(fs_name) + if query_params is not None: + return '{}?{}'.format(api_url, urlencode(query_params)) + return api_url + + @patch('dashboard.controllers.nfsganesha.CephFS') + def test_lsdir(self, cephfs_class): + cephfs_class.return_value.ls_dir.return_value = [ + {'path': '/foo'}, + {'path': '/foo/bar'} + ] + mocked_ls_dir = cephfs_class.return_value.ls_dir + + reqs = [ + { + 'params': None, + 'cephfs_ls_dir_args': ['/', 1], + 'path0': '/', + 'status': 200 + }, + { + 'params': {'root_dir': '/', 'depth': '1'}, + 'cephfs_ls_dir_args': ['/', 1], + 'path0': '/', + 'status': 200 + }, + { + 'params': {'root_dir': '', 'depth': '1'}, + 'cephfs_ls_dir_args': ['/', 1], + 'path0': '/', + 'status': 200 + }, + { + 'params': {'root_dir': '/foo', 'depth': '3'}, + 'cephfs_ls_dir_args': ['/foo', 3], + 'path0': '/foo', + 'status': 200 + }, + { + 'params': {'root_dir': 'foo', 'depth': '6'}, + 'cephfs_ls_dir_args': ['/foo', 5], + 'path0': '/foo', + 'status': 200 + }, + { + 'params': {'root_dir': '/', 'depth': '-1'}, + 'status': 400 + }, + { + 'params': {'root_dir': '/', 'depth': 'abc'}, + 'status': 400 + } + ] + + for req in reqs: + self._get(self._create_ls_dir_url('a', req['params'])) + self.assertStatus(req['status']) + + # Returned paths should contain root_dir as first element + if req['status'] == 200: + paths = self.json_body()['paths'] + self.assertEqual(paths[0], req['path0']) + cephfs_class.assert_called_once_with('a') + + # Check the arguments passed to `CephFS.ls_dir`. + if req.get('cephfs_ls_dir_args'): + mocked_ls_dir.assert_called_once_with(*req['cephfs_ls_dir_args']) + else: + mocked_ls_dir.assert_not_called() + mocked_ls_dir.reset_mock() + cephfs_class.reset_mock() + + @patch('dashboard.controllers.nfsganesha.cephfs') + @patch('dashboard.controllers.nfsganesha.CephFS') + def test_lsdir_non_existed_dir(self, cephfs_class, cephfs): + cephfs.ObjectNotFound = Exception + cephfs.PermissionError = Exception + cephfs_class.return_value.ls_dir.side_effect = cephfs.ObjectNotFound() + self._get(self._create_ls_dir_url('a', {'root_dir': '/foo', 'depth': '3'})) + cephfs_class.assert_called_once_with('a') + cephfs_class.return_value.ls_dir.assert_called_once_with('/foo', 3) + self.assertStatus(200) + self.assertJsonBody({'paths': []}) -- 2.39.5