From 7ee7fb98d343f6c6c44cac60ce6ca5ab8aeb9021 Mon Sep 17 00:00:00 2001 From: Ricardo Marques Date: Mon, 3 Aug 2020 16:38:10 +0100 Subject: [PATCH] mgr/dashboard: Unable to edit iSCSI logged-in client Fixes: https://tracker.ceph.com/issues/46818 Signed-off-by: Ricardo Marques --- src/pybind/mgr/dashboard/controllers/iscsi.py | 85 ++++++++++--- src/pybind/mgr/dashboard/tests/test_iscsi.py | 116 +++++++++++++++--- 2 files changed, 165 insertions(+), 36 deletions(-) diff --git a/src/pybind/mgr/dashboard/controllers/iscsi.py b/src/pybind/mgr/dashboard/controllers/iscsi.py index bcf94416a1e..81a54ce61e7 100644 --- a/src/pybind/mgr/dashboard/controllers/iscsi.py +++ b/src/pybind/mgr/dashboard/controllers/iscsi.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- # pylint: disable=too-many-branches +# pylint: disable=too-many-lines from __future__ import absolute_import from copy import deepcopy @@ -329,6 +330,8 @@ class IscsiTarget(RESTController): deleted_portal_names = list(old_portal_names - new_portal_names) validate_rest_api(deleted_portal_names) IscsiTarget._validate(new_target_iqn, target_controls, portals, disks, groups, settings) + IscsiTarget._validate_delete(gateway, target_iqn, config, new_target_iqn, target_controls, + disks, clients, groups) config = IscsiTarget._delete(target_iqn, config, 0, 50, new_target_iqn, target_controls, portals, disks, clients, groups) IscsiTarget._create(new_target_iqn, target_controls, acl_enabled, auth, portals, disks, @@ -369,13 +372,21 @@ class IscsiTarget(RESTController): group_id) TaskManager.current_task().inc_progress(task_progress_inc) deleted_clients = [] - for client_iqn in list(target_config['clients'].keys()): + deleted_client_luns = [] + for client_iqn, client_config in target_config['clients'].items(): if IscsiTarget._client_deletion_required(target, new_target_iqn, new_target_controls, new_clients, client_iqn, - new_groups, deleted_groups): + deleted_groups): deleted_clients.append(client_iqn) IscsiClient.instance(gateway_name=gateway_name).delete_client(target_iqn, client_iqn) + else: + for image_id in list(client_config.get('luns', {}).keys()): + if IscsiTarget._client_lun_deletion_required(target, client_iqn, image_id, + new_clients): + deleted_client_luns.append((client_iqn, image_id)) + IscsiClient.instance(gateway_name=gateway_name).delete_client_lun( + target_iqn, client_iqn, image_id) TaskManager.current_task().inc_progress(task_progress_inc) for image_id in target_config['disks']: if IscsiTarget._target_lun_deletion_required(target, new_target_iqn, @@ -386,7 +397,8 @@ class IscsiTarget(RESTController): for client_iqn in not_deleted_clients: client_image_ids = target_config['clients'][client_iqn]['luns'].keys() for client_image_id in client_image_ids: - if image_id == client_image_id: + if image_id == client_image_id and \ + (client_iqn, client_image_id) not in deleted_client_luns: IscsiClient.instance(gateway_name=gateway_name).delete_client_lun( target_iqn, client_iqn, client_image_id) IscsiClient.instance(gateway_name=gateway_name).delete_target_lun(target_iqn, @@ -429,7 +441,7 @@ class IscsiTarget(RESTController): for client_iqn in new_group['members']: if IscsiTarget._client_deletion_required(target, new_target_iqn, new_target_controls, new_clients, client_iqn, - new_groups, []): + []): return True # Check if any disk inside this group has changed for disk in new_group['disks']: @@ -449,25 +461,32 @@ class IscsiTarget(RESTController): @staticmethod def _client_deletion_required(target, new_target_iqn, new_target_controls, - new_clients, client_iqn, new_groups, deleted_groups): + new_clients, client_iqn, deleted_groups): if IscsiTarget._target_deletion_required(target, new_target_iqn, new_target_controls): return True - new_client = deepcopy(IscsiTarget._get_client(new_clients, client_iqn)) + new_client = IscsiTarget._get_client(new_clients, client_iqn) if not new_client: return True - # Disks inherited from groups must be considered - for group in new_groups: - if client_iqn in group['members']: - new_client['luns'] += group['disks'] - old_client = IscsiTarget._get_client(target['clients'], client_iqn) - if new_client != old_client: - return True # Check if client belongs to a groups that has been deleted for group in target['groups']: if group['group_id'] in deleted_groups and client_iqn in group['members']: return True return False + @staticmethod + def _client_lun_deletion_required(target, client_iqn, image_id, new_clients): + new_client = IscsiTarget._get_client(new_clients, client_iqn) + if not new_client: + return True + new_lun = IscsiTarget._get_disk(new_client.get('luns', []), image_id) + if not new_lun: + return True + old_client = IscsiTarget._get_client(target['clients'], client_iqn) + if not old_client: + return False + old_lun = IscsiTarget._get_disk(old_client.get('luns', []), image_id) + return new_lun != old_lun + @staticmethod def _get_disk(disks, image_id): for disk in disks: @@ -625,6 +644,33 @@ class IscsiTarget(RESTController): code='pool_does_not_exist', component='iscsi') + @staticmethod + def _validate_delete(gateway, target_iqn, config, new_target_iqn=None, new_target_controls=None, + new_disks=None, new_clients=None, new_groups=None): + new_target_controls = new_target_controls or {} + new_disks = new_disks or [] + new_clients = new_clients or [] + new_groups = new_groups or [] + + target_config = config['targets'][target_iqn] + target = IscsiTarget._config_to_target(target_iqn, config) + deleted_groups = [] + for group_id in list(target_config['groups'].keys()): + if IscsiTarget._group_deletion_required(target, new_target_iqn, new_target_controls, + new_groups, group_id, new_clients, + new_disks): + deleted_groups.append(group_id) + for client_iqn in list(target_config['clients'].keys()): + if IscsiTarget._client_deletion_required(target, new_target_iqn, new_target_controls, + new_clients, client_iqn, deleted_groups): + client_info = IscsiClient.instance(gateway_name=gateway).get_clientinfo(target_iqn, + client_iqn) + if client_info.get('state', {}).get('LOGGED_IN', []): + raise DashboardException(msg="Client '{}' cannot be deleted until it's logged " + "out".format(client_iqn), + code='client_logged_in', + component='iscsi') + @staticmethod def _update_targetauth(config, target_iqn, auth, gateway_name): # Target level authentication was introduced in ceph-iscsi config v11 @@ -647,11 +693,11 @@ class IscsiTarget(RESTController): targetauth_action) @staticmethod - def _is_auth_equal(target_auth, auth): - return auth['user'] == target_auth['username'] and \ - auth['password'] == target_auth['password'] and \ - auth['mutual_user'] == target_auth['mutual_username'] and \ - auth['mutual_password'] == target_auth['mutual_password'] + def _is_auth_equal(auth_config, auth): + return auth['user'] == auth_config['username'] and \ + auth['password'] == auth_config['password'] and \ + auth['mutual_user'] == auth_config['mutual_username'] and \ + auth['mutual_password'] == auth_config['mutual_password'] @staticmethod def _create(target_iqn, target_controls, acl_enabled, @@ -729,6 +775,9 @@ class IscsiTarget(RESTController): if not target_config or client_iqn not in target_config['clients']: IscsiClient.instance(gateway_name=gateway_name).create_client(target_iqn, client_iqn) + if not target_config or client_iqn not in target_config['clients'] or \ + not IscsiTarget._is_auth_equal(target_config['clients'][client_iqn]['auth'], + client['auth']): user = client['auth']['user'] password = client['auth']['password'] m_user = client['auth']['mutual_user'] diff --git a/src/pybind/mgr/dashboard/tests/test_iscsi.py b/src/pybind/mgr/dashboard/tests/test_iscsi.py index 6da706674cf..962ffeb6846 100644 --- a/src/pybind/mgr/dashboard/tests/test_iscsi.py +++ b/src/pybind/mgr/dashboard/tests/test_iscsi.py @@ -222,7 +222,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): "state": {} } }) - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_add_bad_client(self, _validate_image_mock): @@ -263,7 +263,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): response = copy.deepcopy(iscsi_target_response) response['target_iqn'] = target_iqn response['clients'][0]['auth']['password'] = 'MyNewPassword' - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_rename_client(self, _validate_image_mock): @@ -276,7 +276,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): response = copy.deepcopy(iscsi_target_response) response['target_iqn'] = target_iqn response['clients'][0]['client_iqn'] = 'iqn.1994-05.com.redhat:rh7-client0' - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_add_disk(self, _validate_image_mock): @@ -306,7 +306,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): }) response['clients'][0]['luns'].append({"image": "lun3", "pool": "rbd"}) - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_change_disk_image(self, _validate_image_mock): @@ -321,7 +321,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): response['target_iqn'] = target_iqn response['disks'][0]['image'] = 'lun0' response['clients'][0]['luns'][0]['image'] = 'lun0' - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_change_disk_controls(self, _validate_image_mock): @@ -334,7 +334,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): response = copy.deepcopy(iscsi_target_response) response['target_iqn'] = target_iqn response['disks'][0]['controls'] = {"qfull_timeout": 15} - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_rename_target(self, _validate_image_mock): @@ -346,7 +346,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): update_request['new_target_iqn'] = new_target_iqn response = copy.deepcopy(iscsi_target_response) response['target_iqn'] = new_target_iqn - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_rename_group(self, _validate_image_mock): @@ -359,7 +359,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): response = copy.deepcopy(iscsi_target_response) response['target_iqn'] = target_iqn response['groups'][0]['group_id'] = 'mygroup0' - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_add_client_to_group(self, _validate_image_mock): @@ -397,7 +397,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): } }) response['groups'][0]['members'].append('iqn.1994-05.com.redhat:rh7-client3') - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_remove_client_from_group(self, _validate_image_mock): @@ -410,7 +410,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): response = copy.deepcopy(iscsi_target_response) response['target_iqn'] = target_iqn response['groups'][0]['members'].remove('iqn.1994-05.com.redhat:rh7-client2') - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_remove_groups(self, _validate_image_mock): @@ -423,7 +423,7 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): response = copy.deepcopy(iscsi_target_response) response['target_iqn'] = target_iqn response['groups'] = [] - self._update_iscsi_target(create_request, update_request, response) + self._update_iscsi_target(create_request, update_request, 200, None, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_add_client_to_multiple_groups(self, _validate_image_mock): @@ -440,11 +440,90 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): 'component': 'iscsi' }) - def _update_iscsi_target(self, create_request, update_request, response): + @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') + def test_remove_client_lun(self, _validate_image_mock): + target_iqn = "iqn.2003-01.com.redhat.iscsi-gw:iscsi-igw17" + create_request = copy.deepcopy(iscsi_target_request) + create_request['target_iqn'] = target_iqn + create_request['clients'][0]['luns'] = [ + {"image": "lun1", "pool": "rbd"}, + {"image": "lun2", "pool": "rbd"}, + {"image": "lun3", "pool": "rbd"} + ] + update_request = copy.deepcopy(create_request) + update_request['new_target_iqn'] = target_iqn + update_request['clients'][0]['luns'] = [ + {"image": "lun1", "pool": "rbd"}, + {"image": "lun3", "pool": "rbd"} + ] + response = copy.deepcopy(iscsi_target_response) + response['target_iqn'] = target_iqn + response['clients'][0]['luns'] = [ + {"image": "lun1", "pool": "rbd"}, + {"image": "lun3", "pool": "rbd"} + ] + self._update_iscsi_target(create_request, update_request, 200, None, response) + + @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') + def test_change_client_auth(self, _validate_image_mock): + target_iqn = "iqn.2003-01.com.redhat.iscsi-gw:iscsi-igw18" + create_request = copy.deepcopy(iscsi_target_request) + create_request['target_iqn'] = target_iqn + update_request = copy.deepcopy(create_request) + update_request['new_target_iqn'] = target_iqn + update_request['clients'][0]['auth']['password'] = 'myiscsipasswordX' + response = copy.deepcopy(iscsi_target_response) + response['target_iqn'] = target_iqn + response['clients'][0]['auth']['password'] = 'myiscsipasswordX' + self._update_iscsi_target(create_request, update_request, 200, None, response) + + @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') + def test_remove_client_logged_in(self, _validate_image_mock): + client_info = { + 'alias': '', + 'ip_address': [], + 'state': {'LOGGED_IN': ['node1']} + } + # pylint: disable=protected-access + IscsiClientMock._instance.clientinfo = client_info + target_iqn = "iqn.2003-01.com.redhat.iscsi-gw:iscsi-igw19" + create_request = copy.deepcopy(iscsi_target_request) + create_request['target_iqn'] = target_iqn + update_request = copy.deepcopy(create_request) + update_request['new_target_iqn'] = target_iqn + update_request['clients'].pop(0) + response = copy.deepcopy(iscsi_target_response) + response['target_iqn'] = target_iqn + for client in response['clients']: + client['info'] = client_info + update_response = { + 'detail': "Client 'iqn.1994-05.com.redhat:rh7-client' cannot be deleted until it's " + "logged out", + 'code': 'client_logged_in', + 'component': 'iscsi' + } + self._update_iscsi_target(create_request, update_request, 400, update_response, response) + + @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') + def test_remove_client(self, _validate_image_mock): + target_iqn = "iqn.2003-01.com.redhat.iscsi-gw:iscsi-igw20" + create_request = copy.deepcopy(iscsi_target_request) + create_request['target_iqn'] = target_iqn + update_request = copy.deepcopy(create_request) + update_request['new_target_iqn'] = target_iqn + update_request['clients'].pop(0) + response = copy.deepcopy(iscsi_target_response) + response['target_iqn'] = target_iqn + response['clients'].pop(0) + self._update_iscsi_target(create_request, update_request, 200, None, response) + + def _update_iscsi_target(self, create_request, update_request, update_response_code, + update_response, response): self._task_post('/api/iscsi/target', create_request) self.assertStatus(201) self._task_put('/api/iscsi/target/{}'.format(create_request['target_iqn']), update_request) - self.assertStatus(200) + self.assertStatus(update_response_code) + self.assertJsonBody(update_response) self._get('/api/iscsi/target/{}'.format(update_request['new_target_iqn'])) self.assertStatus(200) self.assertJsonBody(response) @@ -591,6 +670,11 @@ class IscsiClientMock(object): "updated": "", "version": 11 } + self.clientinfo = { + 'alias': '', + 'ip_address': [], + 'state': {} + } @classmethod def instance(cls, gateway_name=None, service_url=None): @@ -817,8 +901,4 @@ class IscsiClientMock(object): def get_clientinfo(self, target_iqn, client_iqn): # pylint: disable=unused-argument - return { - 'alias': '', - 'ip_address': [], - 'state': {} - } + return self.clientinfo -- 2.39.5