diff --git a/manila/share/drivers/helpers.py b/manila/share/drivers/helpers.py index a134baf95f..ebd0fbb4fb 100644 --- a/manila/share/drivers/helpers.py +++ b/manila/share/drivers/helpers.py @@ -210,7 +210,7 @@ class NFSHelper(NASHelperBase): access_rules, ('ip',), (const.ACCESS_LEVEL_RO, const.ACCESS_LEVEL_RW)) - hosts = self._get_host_list(out, local_path) + hosts = self.get_host_list(out, local_path) for host in hosts: self._ssh_exec(server, ['sudo', 'exportfs', '-u', ':'.join((host, local_path))]) @@ -279,7 +279,8 @@ class NFSHelper(NASHelperBase): if add_rules: self._sync_nfs_temp_and_perm_files(server) - def _get_host_list(self, output, local_path): + @staticmethod + def get_host_list(output, local_path): entries = [] output = output.replace('\n\t\t', ' ') lines = output.split('\n') diff --git a/manila/share/drivers/ibm/gpfs.py b/manila/share/drivers/ibm/gpfs.py index 771aa58e69..621e767c09 100644 --- a/manila/share/drivers/ibm/gpfs.py +++ b/manila/share/drivers/ibm/gpfs.py @@ -46,6 +46,7 @@ from manila.common import constants from manila import exception from manila.i18n import _ from manila.share import driver +from manila.share.drivers.helpers import NFSHelper from manila.share import share_types from manila import utils @@ -678,7 +679,10 @@ class KNFSHelper(NASHelperBase): LOG.error(msg) raise exception.GPFSException(msg) - def _publish_access(self, *cmd): + def _publish_access(self, *cmd, **kwargs): + check_exit_code = kwargs.get('check_exit_code', True) + + outs = [] localserver_iplist = socket.gethostbyname_ex(socket.gethostname())[2] for server in self.configuration.gpfs_nfs_server_list: if server in localserver_iplist: @@ -690,11 +694,40 @@ class KNFSHelper(NASHelperBase): run_command = ['ssh', remote_login] + list(cmd) run_local = False try: - utils.execute(*run_command, - run_as_root=run_local, - check_exit_code=True) + out = utils.execute(*run_command, + run_as_root=run_local, + check_exit_code=check_exit_code) except exception.ProcessExecutionError: raise + outs.append(out) + return outs + + def _verify_denied_access(self, local_path, share, ip): + try: + cmd = ['exportfs'] + outs = self._publish_access(*cmd) + except exception.ProcessExecutionError: + msg = _('Failed to verify denied access for ' + 'share %s.') % share['name'] + LOG.exception(msg) + raise exception.GPFSException(msg) + + for stdout, stderr in outs: + if stderr and stderr.strip(): + msg = ('Log/ignore stderr during _validate_denied_access for ' + 'share %(sharename)s. Return code OK. ' + 'Stderr: %(stderr)s' % {'sharename': share['name'], + 'stderr': stderr}) + LOG.debug(msg) + + gpfs_ips = NFSHelper.get_host_list(stdout, local_path) + if ip in gpfs_ips: + msg = (_('Failed to deny access for share %(sharename)s. ' + 'IP %(ip)s still has access.') % + {'sharename': share['name'], + 'ip': ip}) + LOG.error(msg) + raise exception.GPFSException(msg) def remove_export(self, local_path, share): """Remove export.""" @@ -741,17 +774,23 @@ class KNFSHelper(NASHelperBase): def deny_access(self, local_path, share, access, force=False): """Remove access for one or more vm instances.""" - cmd = ['exportfs', '-u', ':'.join([access['access_to'], local_path])] + ip = access['access_to'] + cmd = ['exportfs', '-u', ':'.join([ip, local_path])] try: - self._publish_access(*cmd) - except exception.ProcessExecutionError as e: - msg = (_('Failed to deny access for share %(sharename)s. ' - 'Error: %(excmsg)s.') % - {'sharename': share['name'], - 'excmsg': e}) - LOG.error(msg) + # Can get exit code 0 for success or 1 for already gone (also + # potentially get 1 due to exportfs bug). So allow + # _publish_access to continue with [0, 1] and then verify after + # it is done. + self._publish_access(*cmd, check_exit_code=[0, 1]) + except exception.ProcessExecutionError: + msg = _('Failed to deny access for share %s.') % share['name'] + LOG.exception(msg) raise exception.GPFSException(msg) + # Error code (0 or 1) makes deny IP success indeterminate. + # So, verify that the IP access was completely removed. + self._verify_denied_access(local_path, share, ip) + class CESHelper(NASHelperBase): """Wrapper for NFS by Spectrum Scale CES""" diff --git a/manila/tests/share/drivers/ibm/test_gpfs.py b/manila/tests/share/drivers/ibm/test_gpfs.py index 1b11e7623f..0c4b1f429c 100644 --- a/manila/tests/share/drivers/ibm/test_gpfs.py +++ b/manila/tests/share/drivers/ibm/test_gpfs.py @@ -789,25 +789,95 @@ mmcesnfslsexport:nfsexports:HEADER:version:reserved:reserved:Path:Delegations:Cl self._knfs_helper._execute.assert_called_once_with('exportfs', run_as_root=True) + def test_knfs__verify_denied_access_pass(self): + local_path = self.fakesharepath + ip = self.access['access_to'] + fake_exportfs = ('/shares/share-1\n\t\t1.1.1.1\n' + '/shares/share-2\n\t\t2.2.2.2\n') + self._knfs_helper._publish_access = mock.Mock( + return_value=[(fake_exportfs, '')]) + + self._knfs_helper._verify_denied_access(local_path, self.share, ip) + + self._knfs_helper._publish_access.assert_called_once_with('exportfs') + + def test_knfs__verify_denied_access_fail(self): + local_path = self.fakesharepath + ip = self.access['access_to'] + data = {'path': local_path, 'ip': ip} + fake_exportfs = ('/shares/share-1\n\t\t1.1.1.1\n' + '%(path)s\n\t\t%(ip)s\n' + '/shares/share-2\n\t\t2.2.2.2\n') % data + self._knfs_helper._publish_access = mock.Mock( + return_value=[(fake_exportfs, '')]) + + self.assertRaises(exception.GPFSException, + self._knfs_helper._verify_denied_access, + local_path, + self.share, + ip) + + self._knfs_helper._publish_access.assert_called_once_with('exportfs') + + def test_knfs__verify_denied_access_exception(self): + self._knfs_helper._publish_access = mock.Mock( + side_effect=exception.ProcessExecutionError + ) + + ip = self.access['access_to'] + local_path = self.fakesharepath + + self.assertRaises(exception.GPFSException, + self._knfs_helper._verify_denied_access, + local_path, + self.share, + ip) + + self._knfs_helper._publish_access.assert_called_once_with('exportfs') + + @ddt.data((None, False), + ('', False), + (' ', False), + ('Some error to log', True)) + @ddt.unpack + def test_knfs__verify_denied_access_stderr(self, stderr, is_logged): + """Stderr debug logging should only happen when not empty.""" + outputs = [('', stderr)] + self._knfs_helper._publish_access = mock.Mock(return_value=outputs) + gpfs.LOG.debug = mock.Mock() + + self._knfs_helper._verify_denied_access( + self.fakesharepath, self.share, self.remote_ip) + + self._knfs_helper._publish_access.assert_called_once_with('exportfs') + self.assertEqual(is_logged, gpfs.LOG.debug.called) + def test_knfs_deny_access(self): - self._knfs_helper._publish_access = mock.Mock() + self._knfs_helper._publish_access = mock.Mock(return_value=[('', '')]) + access = self.access local_path = self.fakesharepath self._knfs_helper.deny_access(local_path, self.share, access) - cmd = ['exportfs', '-u', ':'.join([access['access_to'], local_path])] - self._knfs_helper._publish_access.assert_called_once_with(*cmd) + + deny = ['exportfs', '-u', ':'.join([access['access_to'], local_path])] + self._knfs_helper._publish_access.assert_has_calls([ + mock.call(*deny, check_exit_code=[0, 1]), + mock.call('exportfs')]) def test_knfs_deny_access_exception(self): self._knfs_helper._publish_access = mock.Mock( side_effect=exception.ProcessExecutionError ) + access = self.access local_path = self.fakesharepath cmd = ['exportfs', '-u', ':'.join([access['access_to'], local_path])] self.assertRaises(exception.GPFSException, self._knfs_helper.deny_access, local_path, self.share, access) - self._knfs_helper._publish_access.assert_called_once_with(*cmd) + + self._knfs_helper._publish_access.assert_called_once_with( + *cmd, check_exit_code=[0, 1]) def test_knfs__publish_access(self): self.mock_object(utils, 'execute') diff --git a/manila/tests/share/drivers/test_helpers.py b/manila/tests/share/drivers/test_helpers.py index 256242e650..0abeed7e6c 100644 --- a/manila/tests/share/drivers/test_helpers.py +++ b/manila/tests/share/drivers/test_helpers.py @@ -180,7 +180,7 @@ class NFSHelperTestCase(test.TestCase): '/shares/share-3\n\t\t30.0.0.4\n' '/shares/share-3\n\t\t30.0.0.7\n') expected = ['20.0.0.3', '20.0.0.6'] - result = self._helper._get_host_list(fake_exportfs, '/shares/share-1') + result = self._helper.get_host_list(fake_exportfs, '/shares/share-1') self.assertEqual(expected, result) @ddt.data(const.ACCESS_LEVEL_RW, const.ACCESS_LEVEL_RO) @@ -192,7 +192,7 @@ class NFSHelperTestCase(test.TestCase): access_rules = [test_generic.get_fake_access_rule( '1.1.1.1', access_level), ] self.mock_object(self._helper, '_sync_nfs_temp_and_perm_files') - self.mock_object(self._helper, '_get_host_list', + self.mock_object(self._helper, 'get_host_list', mock.Mock(return_value=['1.1.1.1'])) self._helper.update_access(self.server, self.share_name, access_rules, [], []) diff --git a/releasenotes/notes/bug-1651587-deny-access-verify-563ef2f3f6b8c13b.yaml b/releasenotes/notes/bug-1651587-deny-access-verify-563ef2f3f6b8c13b.yaml new file mode 100644 index 0000000000..227e9c4cab --- /dev/null +++ b/releasenotes/notes/bug-1651587-deny-access-verify-563ef2f3f6b8c13b.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Fixed GPFS KNFS deny access so that it will not fail when + the access can be verified to not exist.