aurora-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From s...@apache.org
Subject aurora git commit: Fix command escaping when using the Mesos containerizer.
Date Mon, 23 Jan 2017 07:39:31 GMT
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 <serb@apache.org>
Authored: Mon Jan 23 08:38:52 2017 +0100
Committer: Stephan Erb <serb@apache.org>
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]}}'),
   ]


Mime
View raw message