diff --git a/manila/share/drivers/generic.py b/manila/share/drivers/generic.py index b5c12ac0..b3e48461 100644 --- a/manila/share/drivers/generic.py +++ b/manila/share/drivers/generic.py @@ -160,6 +160,11 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): ssh_pool.remove(ssh) ssh = ssh_pool.create() self.ssh_connections[server['instance_id']] = (ssh_pool, ssh) + + # (aovchinnikov): ssh_execute does not behave well when passed + # parameters with spaces. + wrap = lambda token: "\"" + token + "\"" + command = [wrap(tkn) if tkn.count(' ') else tkn for tkn in command] return processutils.ssh_execute(ssh, ' '.join(command), check_exit_code=check_exit_code) @@ -335,10 +340,11 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): volume): LOG.debug("Mounting '%(dev)s' to path '%(path)s' on " "server '%(server)s'.", log_data) - mount_cmd = ['sudo mkdir -p', mount_path, '&&'] - mount_cmd.extend(['sudo mount', volume['mountpoint'], + mount_cmd = ['sudo', 'mkdir', '-p', mount_path, '&&'] + mount_cmd.extend(['sudo', 'mount', volume['mountpoint'], + mount_path]) + mount_cmd.extend(['&&', 'sudo', 'chmod', '777', mount_path]) - mount_cmd.extend(['&& sudo chmod 777', mount_path]) self._ssh_exec(server_details, mount_cmd) # Add mount permanently @@ -365,8 +371,8 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): if self._is_device_mounted(mount_path, server_details): LOG.debug("Unmounting path '%(path)s' on server " "'%(server)s'.", log_data) - unmount_cmd = ['sudo umount', mount_path, '&& sudo rmdir', - mount_path] + unmount_cmd = ['sudo', 'umount', mount_path, '&&', 'sudo', + 'rmdir', mount_path] self._ssh_exec(server_details, unmount_cmd) # Remove mount permanently self._sync_mount_temp_and_perm_files(server_details) diff --git a/manila/share/drivers/helpers.py b/manila/share/drivers/helpers.py index 47894829..068f990f 100644 --- a/manila/share/drivers/helpers.py +++ b/manila/share/drivers/helpers.py @@ -289,8 +289,8 @@ class NFSHelper(NASHelperBase): maintenance_file = self._get_maintenance_file_path(share_name) backup_exports = [ 'cat', const.NFS_EXPORTS_FILE, - '| grep', share_name, - '| sudo tee', maintenance_file + '|', 'grep', share_name, + '|', 'sudo', 'tee', maintenance_file ] self._ssh_exec(server, backup_exports) @@ -304,9 +304,9 @@ class NFSHelper(NASHelperBase): maintenance_file = self._get_maintenance_file_path(share_name) restore_exports = [ 'cat', maintenance_file, - '| sudo tee -a', const.NFS_EXPORTS_FILE, - '&& sudo exportfs -r', - '&& sudo rm -f', maintenance_file + '|', 'sudo', 'tee', '-a', const.NFS_EXPORTS_FILE, + '&&', 'sudo', 'exportfs', '-r', + '&&', 'sudo', 'rm', '-f', maintenance_file ] self._ssh_exec(server, restore_exports) @@ -324,10 +324,10 @@ class CIFSHelperIPAccess(NASHelperBase): self.export_format = '\\\\%s\\%s' self.parameters = { 'browseable': 'yes', - '\"create mask\"': '0755', - '\"hosts deny\"': '0.0.0.0/0', # deny all by default - '\"hosts allow\"': '127.0.0.1', - '\"read only\"': 'no', + 'create mask': '0755', + 'hosts deny': '0.0.0.0/0', # deny all by default + 'hosts allow': '127.0.0.1', + 'read only': 'no', } def init_helper(self, server): @@ -406,13 +406,13 @@ class CIFSHelperIPAccess(NASHelperBase): def _get_allow_hosts(self, server, share_name): (out, _) = self._ssh_exec(server, ['sudo', 'net', 'conf', 'getparm', - share_name, '\"hosts allow\"']) + share_name, 'hosts allow']) return out.split() def _set_allow_hosts(self, server, hosts, share_name): - value = "\"" + ' '.join(hosts) + "\"" + value = ' '.join(hosts) or ' ' self._ssh_exec(server, ['sudo', 'net', 'conf', 'setparm', share_name, - '\"hosts allow\"', value]) + 'hosts allow', value]) @staticmethod def _get_share_group_name_from_export_location(export_location): @@ -450,7 +450,8 @@ class CIFSHelperIPAccess(NASHelperBase): allowed_hosts = " ".join(self._get_allow_hosts(server, share_name)) backup_exports = [ - 'echo', "'%s'" % allowed_hosts, '| sudo tee', maintenance_file + 'echo', "'%s'" % allowed_hosts, '|', 'sudo', 'tee', + maintenance_file ] self._ssh_exec(server, backup_exports) self._set_allow_hosts(server, [], share_name) @@ -459,7 +460,7 @@ class CIFSHelperIPAccess(NASHelperBase): maintenance_file = self._get_maintenance_file_path(share_name) (exports, __) = self._ssh_exec(server, ['cat', maintenance_file]) self._set_allow_hosts(server, exports.split(), share_name) - self._ssh_exec(server, ['sudo rm -f', maintenance_file]) + self._ssh_exec(server, ['sudo', 'rm', '-f', maintenance_file]) class CIFSHelperUserAccess(CIFSHelperIPAccess): @@ -512,7 +513,7 @@ class CIFSHelperUserAccess(CIFSHelperIPAccess): return 'read list' def _set_valid_users(self, server, users, share_name, access_level): - value = "\"" + ' '.join(users) + "\"" + value = ' '.join(users) param = self._get_conf_param(access_level) self._ssh_exec(server, ['sudo', 'net', 'conf', 'setparm', share_name, param, value]) diff --git a/manila/tests/share/drivers/test_generic.py b/manila/tests/share/drivers/test_generic.py index bc679325..82d46640 100644 --- a/manila/tests/share/drivers/test_generic.py +++ b/manila/tests/share/drivers/test_generic.py @@ -397,9 +397,9 @@ class GenericShareDriverTestCase(test.TestCase): server) self._driver._ssh_exec.assert_called_once_with( server, - ['sudo mkdir -p', mount_path, - '&&', 'sudo mount', volume['mountpoint'], mount_path, - '&& sudo chmod 777', mount_path], + ['sudo', 'mkdir', '-p', mount_path, + '&&', 'sudo', 'mount', volume['mountpoint'], mount_path, + '&&', 'sudo', 'chmod', '777', mount_path], ) def test_mount_device_present(self): @@ -454,7 +454,7 @@ class GenericShareDriverTestCase(test.TestCase): self.server) self._driver._ssh_exec.assert_called_once_with( self.server, - ['sudo umount', mount_path, '&& sudo rmdir', mount_path], + ['sudo', 'umount', mount_path, '&&', 'sudo', 'rmdir', mount_path], ) def test_unmount_device_retry_once(self): @@ -485,8 +485,8 @@ class GenericShareDriverTestCase(test.TestCase): self._driver._sync_mount_temp_and_perm_files.assert_called_once_with( self.server) self.assertEqual( - [mock.call(self.server, ['sudo umount', mount_path, - '&& sudo rmdir', mount_path]) + [mock.call(self.server, ['sudo', 'umount', mount_path, + '&&', 'sudo', 'rmdir', mount_path]) for i in moves.range(2)], self._driver._ssh_exec.mock_calls, ) @@ -1366,6 +1366,24 @@ class GenericShareDriverTestCase(test.TestCase): ) self.assertEqual(ssh_output, result) + def test__ssh_exec_check_list_comprehensions_still_work(self): + ssh_output = 'fake_ssh_output' + cmd = ['fake', 'command spaced'] + ssh = mock.Mock() + ssh_pool = mock.Mock() + ssh_pool.create = mock.Mock(side_effect=lambda: ssh) + ssh_pool.remove = mock.Mock() + self.mock_object(processutils, 'ssh_execute', + mock.Mock(return_value=ssh_output)) + self._driver.ssh_connections = { + self.server['instance_id']: (ssh_pool, ssh) + } + + self._driver._ssh_exec(self.server, cmd) + + processutils.ssh_execute.assert_called_once_with( + ssh, 'fake "command spaced"', check_exit_code=True) + def test_get_share_stats_refresh_false(self): self._driver._stats = {'fake_key': 'fake_value'} diff --git a/manila/tests/share/drivers/test_helpers.py b/manila/tests/share/drivers/test_helpers.py index 087312e6..e0a4b0f9 100644 --- a/manila/tests/share/drivers/test_helpers.py +++ b/manila/tests/share/drivers/test_helpers.py @@ -241,8 +241,8 @@ class NFSHelperTestCase(test.TestCase): self._helper._ssh_exec.assert_any_call( self.server, ['cat', const.NFS_EXPORTS_FILE, - '| grep', self.share_name, - '| sudo tee', fake_maintenance_path] + '|', 'grep', self.share_name, + '|', 'sudo', 'tee', fake_maintenance_path] ) self._helper._ssh_exec.assert_any_call( self.server, @@ -264,8 +264,8 @@ class NFSHelperTestCase(test.TestCase): self._helper._ssh_exec.assert_called_once_with( self.server, ['cat', fake_maintenance_path, - '| sudo tee -a', const.NFS_EXPORTS_FILE, - '&& sudo exportfs -r', '&& sudo rm -f', + '|', 'sudo', 'tee', '-a', const.NFS_EXPORTS_FILE, + '&&', 'sudo', 'exportfs', '-r', '&&', 'sudo', 'rm', '-f', fake_maintenance_path] ) @@ -448,8 +448,8 @@ class CIFSHelperIPAccessTestCase(test.TestCase): access_rules, [], []) self._helper._ssh_exec.assert_called_once_with( self.server_details, ['sudo', 'net', 'conf', 'setparm', - self.share_name, '"hosts allow"', - '"1.1.1.1"']) + self.share_name, 'hosts allow', + '1.1.1.1']) def test_get_allow_hosts(self): self.mock_object(self._helper, '_ssh_exec', @@ -460,7 +460,7 @@ class CIFSHelperIPAccessTestCase(test.TestCase): self.server_details, self.share_name) self.assertEqual(expected, result) cmd = ['sudo', 'net', 'conf', 'getparm', self.share_name, - '\"hosts allow\"'] + 'hosts allow'] self._helper._ssh_exec.assert_called_once_with( self.server_details, cmd) @@ -537,7 +537,8 @@ class CIFSHelperIPAccessTestCase(test.TestCase): self.server_details, self.share_name) self._helper._set_allow_hosts.assert_called_once_with( self.server_details, [], self.share_name) - valid_cmd = ['echo', "'test test2'", '| sudo tee', maintenance_path] + valid_cmd = ['echo', "'test test2'", '|', 'sudo', 'tee', + maintenance_path] self._helper._ssh_exec.assert_called_once_with( self.server_details, valid_cmd) @@ -557,7 +558,7 @@ class CIFSHelperIPAccessTestCase(test.TestCase): self._helper._ssh_exec.assert_any_call( self.server_details, ['cat', fake_maintenance_path]) self._helper._ssh_exec.assert_any_call( - self.server_details, ['sudo rm -f', fake_maintenance_path]) + self.server_details, ['sudo', 'rm', '-f', fake_maintenance_path]) @ddt.ddt @@ -601,10 +602,10 @@ class CIFSHelperUserAccessTestCase(test.TestCase): self._helper._ssh_exec.assert_has_calls([ mock.call(self.server_details, ['sudo', 'net', 'conf', 'setparm', self.share_name, - 'valid users', '"user1"']), + 'valid users', 'user1']), mock.call(self.server_details, ['sudo', 'net', 'conf', 'setparm', self.share_name, - 'read list', '"user2"']) + 'read list', 'user2']) ]) def test_update_access_exception_level(self):