Repository: aurora Updated Branches: refs/heads/master 6ad4c8728 -> dc6f27ed3
Fix command escaping when using the Mesos containerizer. The important bit is the change to call the Mesos containerizer with `shell=False`. Getting rid of manual json encoding and eliminating shlex might have helped as well, but was more motivated by clarity rather than correctness. Bugs closed: AURORA-1782 Reviewed at https://reviews.apache.org/r/55684/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/dc6f27ed Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/dc6f27ed Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/dc6f27ed Branch: refs/heads/master Commit: dc6f27ed3addd7c8bca1d089abb7d9fd120ee13f Parents: 6ad4c87 Author: Stephan Erb <[email protected]> Authored: Mon Jan 23 08:38:52 2017 +0100 Committer: Stephan Erb <[email protected]> Committed: Mon Jan 23 08:38:52 2017 +0100 ---------------------------------------------------------------------- .../apache/aurora/common/health_check/shell.py | 23 ++++++----- .../aurora/executor/common/health_checker.py | 20 +++++----- .../apache/thermos/common/process_util.py | 41 ++++++++------------ src/main/python/apache/thermos/core/process.py | 6 +-- .../aurora/common/health_check/test_shell.py | 37 ++++++------------ .../executor/common/test_health_checker.py | 9 ++++- .../python/apache/thermos/core/test_process.py | 4 +- .../apache/aurora/e2e/http/http_example.aurora | 21 +++++++++- 8 files changed, 82 insertions(+), 79 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/dc6f27ed/src/main/python/apache/aurora/common/health_check/shell.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/aurora/common/health_check/shell.py b/src/main/python/apache/aurora/common/health_check/shell.py index 6ac9021..58470a4 100644 --- a/src/main/python/apache/aurora/common/health_check/shell.py +++ b/src/main/python/apache/aurora/common/health_check/shell.py @@ -40,24 +40,24 @@ class ShellHealthCheck(object): def __init__( self, - cmd, + raw_cmd, + wrapped_cmd, preexec_fn=None, - timeout_secs=None, - wrapper_fn=None): + timeout_secs=None): """ Initialize with the command we would like to call. - :param cmd: Command to execute that is expected to have a 0 return code on success. - :type cmd: str + :param raw_cmd: Command to execute as passed by the user. Supposed to 0 return code on success. + :type raw_cmd: str + :param wrapped_cmd: Wrapped form of the user command including executing shell. + :type wrapped_cmd: list(str) :param preexec_fn: Callable to invoke just before the child shell process is executed. :type preexec_fn: callable :param timeout_secs: Timeout in seconds. :type timeout_secs: int - :param wrapper_fn: Callable to invoke that wraps the shell command for filesystem isolation. - :type wrapper_fn: callable """ - self._original_cmd = cmd - self._cmd = cmd if wrapper_fn is None else wrapper_fn(cmd) + self._raw_cmd = raw_cmd + self._wrapped_cmd = wrapped_cmd self._preexec_fn = preexec_fn self._timeout_secs = timeout_secs @@ -70,14 +70,13 @@ class ShellHealthCheck(object): """ try: subprocess.check_call( - self._cmd, + self._wrapped_cmd, timeout=self._timeout_secs, - shell=True, preexec_fn=self._preexec_fn) return True, None except subprocess.CalledProcessError as reason: # The command didn't return a 0 so provide reason for failure. - return False, str(WrappedCalledProcessError(self._original_cmd, reason)) + return False, str(WrappedCalledProcessError(self._raw_cmd, reason)) except subprocess.TimeoutExpired: return False, 'Health check timed out.' except OSError as e: http://git-wip-us.apache.org/repos/asf/aurora/blob/dc6f27ed/src/main/python/apache/aurora/executor/common/health_checker.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/aurora/executor/common/health_checker.py b/src/main/python/apache/aurora/executor/common/health_checker.py index a5fb18f..5bb4768 100644 --- a/src/main/python/apache/aurora/executor/common/health_checker.py +++ b/src/main/python/apache/aurora/executor/common/health_checker.py @@ -366,22 +366,22 @@ class HealthCheckerProvider(StatusCheckerProvider): # If the task is executing in an isolated filesystem we'll want to wrap the health check # command within a mesos-containerizer invocation so that it's executed within that # filesystem. - wrapper = None if sandbox.is_filesystem_image: health_check_user = (os.getusername() if self._nosetuid_health_checks else assigned_task.task.job.role) - def wrapper(cmd): - return wrap_with_mesos_containerizer( - cmd, - health_check_user, - sandbox.container_root, - self._mesos_containerizer_path) + wrapped_cmd = wrap_with_mesos_containerizer( + interpolated_command, + health_check_user, + sandbox.container_root, + self._mesos_containerizer_path) + else: + wrapped_cmd = ['/bin/bash', '-c', interpolated_command] shell_signaler = ShellHealthCheck( - cmd=interpolated_command, + raw_cmd=interpolated_command, + wrapped_cmd=wrapped_cmd, preexec_fn=demote_to_job_role_user, - timeout_secs=timeout_secs, - wrapper_fn=wrapper) + timeout_secs=timeout_secs) a_health_checker = lambda: shell_signaler() else: portmap = resolve_ports(mesos_task, assigned_task.assignedPorts) http://git-wip-us.apache.org/repos/asf/aurora/blob/dc6f27ed/src/main/python/apache/thermos/common/process_util.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/thermos/common/process_util.py b/src/main/python/apache/thermos/common/process_util.py index 637b025..54e716b 100644 --- a/src/main/python/apache/thermos/common/process_util.py +++ b/src/main/python/apache/thermos/common/process_util.py @@ -13,6 +13,7 @@ # import ctypes +import json import os from twitter.common import log @@ -21,30 +22,22 @@ from gen.apache.aurora.api.constants import TASK_FILESYSTEM_MOUNT_POINT def wrap_with_mesos_containerizer(cmdline, user, cwd, mesos_containerizer_path): - # We're going to embed this in JSON, so we must escape quotes and newlines. - cmdline = cmdline.replace('"', '\\"').replace('\n', '\\n') - - # We must wrap the command in single quotes otherwise the shell that executes - # mesos-containerizer will expand any bash variables in the cmdline. Escaping single quotes in - # bash is hard: https://github.com/koalaman/shellcheck/wiki/SC1003. - bash_wrapper = "/bin/bash -c '\\''%s'\\''" - - # The shell: true below shouldn't be necessary. Since we're just invoking bash anyway, using it - # results in a process like: `sh -c /bin/bash -c ...`, however in my testing no combination of - # shell: false and splitting the bash/cmdline args across value/arguments produced an invocation - # that actually worked. That said, it *should* be possbie. - # TODO(jcohen): Investigate setting shell:false further. - return ('%s launch ' - '--unshare_namespace_mnt ' - '--working_directory=%s ' - '--rootfs=%s ' - '--user=%s ' - '--command=\'{"shell":true,"value":"%s"}\'' % ( - mesos_containerizer_path, - cwd, - os.path.join(os.environ['MESOS_DIRECTORY'], TASK_FILESYSTEM_MOUNT_POINT), - user, - bash_wrapper % cmdline)) + command = json.dumps({ + 'shell': False, + 'value': '/bin/bash', # the binary to executed + 'arguments': [ + '/bin/bash', # the name of the called binary as passed to the launched process + '-c', + cmdline + ] + }) + return [mesos_containerizer_path, + 'launch', + '--unshare_namespace_mnt', + '--working_directory=%s' % cwd, + '--rootfs=%s' % os.path.join(os.environ['MESOS_DIRECTORY'], TASK_FILESYSTEM_MOUNT_POINT), + '--user=%s' % user, + '--command=%s' % command] def setup_child_subreaping(): http://git-wip-us.apache.org/repos/asf/aurora/blob/dc6f27ed/src/main/python/apache/thermos/core/process.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/thermos/core/process.py b/src/main/python/apache/thermos/core/process.py index 496b540..4a4678f 100644 --- a/src/main/python/apache/thermos/core/process.py +++ b/src/main/python/apache/thermos/core/process.py @@ -25,7 +25,6 @@ import grp import os import pwd import select -import shlex import signal import subprocess import sys @@ -391,9 +390,8 @@ class Process(ProcessBase): # If mesos-containerizer is not set, we only need to wrap the cmdline in a bash invocation. if self._mesos_containerizer_path is None: return ['/bin/bash', '-c', cmdline] - - return shlex.split( - wrap_with_mesos_containerizer(cmdline, self._user, cwd, self._mesos_containerizer_path)) + else: + return wrap_with_mesos_containerizer(cmdline, self._user, cwd, self._mesos_containerizer_path) def execute(self): """Perform final initialization and launch target process commandline in a subprocess.""" http://git-wip-us.apache.org/repos/asf/aurora/blob/dc6f27ed/src/test/python/apache/aurora/common/health_check/test_shell.py ---------------------------------------------------------------------- diff --git a/src/test/python/apache/aurora/common/health_check/test_shell.py b/src/test/python/apache/aurora/common/health_check/test_shell.py index 4f02878..792ef40 100644 --- a/src/test/python/apache/aurora/common/health_check/test_shell.py +++ b/src/test/python/apache/aurora/common/health_check/test_shell.py @@ -33,35 +33,22 @@ class TestHealthChecker(unittest.TestCase): @mock.patch('subprocess32.check_call', autospec=True) def test_health_check_ok(self, mock_popen): - shell = ShellHealthCheck('cmd', timeout_secs=30) + shell = ShellHealthCheck(raw_cmd='cmd', wrapped_cmd='wrapped-cmd', timeout_secs=30) success, msg = shell() self.assertTrue(success) self.assertIsNone(msg) - mock_popen.assert_called_once_with('cmd', shell=True, timeout=30, preexec_fn=mock.ANY) + mock_popen.assert_called_once_with('wrapped-cmd', timeout=30, preexec_fn=mock.ANY) @mock.patch('subprocess32.check_call', autospec=True) def test_health_check_failed(self, mock_popen): cmd = 'failed' + wrapped_cmd = 'wrapped-failed' # Fail due to command returning a non-0 exit status. - mock_popen.side_effect = subprocess.CalledProcessError(1, cmd) + mock_popen.side_effect = subprocess.CalledProcessError(1, wrapped_cmd) - shell = ShellHealthCheck(cmd, timeout_secs=30) + shell = ShellHealthCheck(raw_cmd=cmd, wrapped_cmd=wrapped_cmd, timeout_secs=30) success, msg = shell() - mock_popen.assert_called_once_with(cmd, shell=True, timeout=30, preexec_fn=mock.ANY) - - self.assertFalse(success) - self.assertEqual(msg, "Command 'failed' returned non-zero exit status 1") - - @mock.patch('subprocess32.check_call', autospec=True) - def test_health_check_failed_with_wrapper(self, mock_popen): - cmd = 'failed' - mock_popen.side_effect = subprocess.CalledProcessError(1, cmd) - - shell = ShellHealthCheck(cmd, timeout_secs=30, wrapper_fn=lambda c: 'wrapped: %s' % c) - success, msg = shell() - self.assertEqual( - mock_popen.mock_calls, - [mock.call('wrapped: %s' % cmd, shell=True, timeout=30, preexec_fn=mock.ANY)]) + mock_popen.assert_called_once_with(wrapped_cmd, timeout=30, preexec_fn=mock.ANY) self.assertFalse(success) self.assertEqual(msg, "Command 'failed' returned non-zero exit status 1") @@ -71,9 +58,9 @@ class TestHealthChecker(unittest.TestCase): # Fail due to command returning a non-0 exit status. mock_popen.side_effect = subprocess.TimeoutExpired('failed', timeout=30) - shell = ShellHealthCheck('cmd', timeout_secs=30) + shell = ShellHealthCheck(raw_cmd='cmd', wrapped_cmd='wrapped-cmd', timeout_secs=30) success, msg = shell() - mock_popen.assert_called_once_with('cmd', shell=True, timeout=30, preexec_fn=mock.ANY) + mock_popen.assert_called_once_with('wrapped-cmd', timeout=30, preexec_fn=mock.ANY) self.assertFalse(success) self.assertEqual(msg, 'Health check timed out.') @@ -83,9 +70,9 @@ class TestHealthChecker(unittest.TestCase): # Fail due to command not existing. mock_popen.side_effect = OSError(1, 'failed') - shell = ShellHealthCheck('cmd', timeout_secs=30) + shell = ShellHealthCheck(raw_cmd='cmd', wrapped_cmd='wrapped-cmd', timeout_secs=30) success, msg = shell() - mock_popen.assert_called_once_with('cmd', shell=True, timeout=30, preexec_fn=mock.ANY) + mock_popen.assert_called_once_with('wrapped-cmd', timeout=30, preexec_fn=mock.ANY) self.assertFalse(success) self.assertEqual(msg, 'OSError: failed') @@ -94,8 +81,8 @@ class TestHealthChecker(unittest.TestCase): # Invalid commmand passed in raises ValueError. mock_popen.side_effect = ValueError('Could not read command.') timeout = 10 - shell = ShellHealthCheck('cmd', timeout_secs=timeout) + shell = ShellHealthCheck(raw_cmd='cmd', wrapped_cmd='wrapped-cmd', timeout_secs=timeout) success, msg = shell() - mock_popen.assert_called_once_with('cmd', shell=True, timeout=10, preexec_fn=mock.ANY) + mock_popen.assert_called_once_with('wrapped-cmd', timeout=timeout, preexec_fn=mock.ANY) self.assertFalse(success) self.assertEqual(msg, 'Invalid commmand.') http://git-wip-us.apache.org/repos/asf/aurora/blob/dc6f27ed/src/test/python/apache/aurora/executor/common/test_health_checker.py ---------------------------------------------------------------------- diff --git a/src/test/python/apache/aurora/executor/common/test_health_checker.py b/src/test/python/apache/aurora/executor/common/test_health_checker.py index 86a71c7..a3b5a22 100644 --- a/src/test/python/apache/aurora/executor/common/test_health_checker.py +++ b/src/test/python/apache/aurora/executor/common/test_health_checker.py @@ -563,6 +563,7 @@ class TestHealthCheckerProvider(unittest.TestCase): # Should not be trying to access role's user info. assert not mock_getpwnam.called + @mock.patch.dict(os.environ, {'MESOS_DIRECTORY': '/some/path'}) @mock.patch('pwd.getpwnam') def test_from_assigned_task_shell_filesystem_image(self, mock_getpwnam): interval_secs = 17 @@ -609,7 +610,13 @@ class TestHealthCheckerProvider(unittest.TestCase): return other is not None assert mock_shell.mock_calls == [ - mock.call(cmd='failed command', wrapper_fn=NotNone(), preexec_fn=None, timeout_secs=5.0)] + mock.call( + raw_cmd='failed command', + wrapped_cmd=NotNone(), + preexec_fn=None, + timeout_secs=5.0 + ) + ] def test_interpolate_cmd(self): """Making sure thermos.ports[foo] gets correctly substituted with assignedPorts info.""" http://git-wip-us.apache.org/repos/asf/aurora/blob/dc6f27ed/src/test/python/apache/thermos/core/test_process.py ---------------------------------------------------------------------- diff --git a/src/test/python/apache/thermos/core/test_process.py b/src/test/python/apache/thermos/core/test_process.py index 2fd3d96..5203902 100644 --- a/src/test/python/apache/thermos/core/test_process.py +++ b/src/test/python/apache/thermos/core/test_process.py @@ -135,8 +135,8 @@ def test_simple_process_filesystem_isolator(): taskpath, 'stdout', 'launch --unshare_namespace_mnt --working_directory=%s --rootfs=/some/path/taskfs ' - '--user=None --command={"shell":true,"value":"/bin/bash -c \'echo hello world\'"}\n' % ( - sandbox)) + '--user=None --command={"shell": false, "arguments": ["/bin/bash", "-c", ' + '"echo hello world"], "value": "/bin/bash"}\n' % (sandbox)) @mock.patch('os.chown') http://git-wip-us.apache.org/repos/asf/aurora/blob/dc6f27ed/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora ---------------------------------------------------------------------- diff --git a/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora b/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora index 1985f89..b2b977b 100644 --- a/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora +++ b/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora @@ -59,10 +59,28 @@ verify_file_mount = Process( cmdline = 'cat /home/vagrant/aurora/.auroraversion' ) +# Regression test for quotation mark usage (AURORA-1782) +verify_command_escaping = Process( + name = 'verify_command_escaping', + cmdline = """ +python -c 'import sys + +if __name__ == "__main__": + sys.exit(0)' +""" +) + test_task = SequentialTask( name = 'http_example', resources = Resources(cpu=0.5, ram=32*MB, disk=64*MB, gpu='{{profile.gpu}}'), - processes = [setup_env, read_env, echo_ports, stage_server, run_server] + processes = [ + setup_env, + read_env, + echo_ports, + verify_command_escaping, + stage_server, + run_server + ] ) no_python_task = SequentialTask( @@ -72,6 +90,7 @@ no_python_task = SequentialTask( setup_env, read_env, echo_ports, + verify_command_escaping, verify_file_mount, Process(name='run_server', cmdline='run-server.sh {{thermos.ports[http]}}'), ]
