Merge lp:~smoser/curtin/trunk.unshare-chroot into lp:~curtin-dev/curtin/trunk

[Scott Moser]

use unshare to put chroot commands in own pid namespace.

Bug 1645680 showed problems when a process started detached and
was left running. This starts processes in a chroot in their own
pid namespace, so that the process is pid 1. Then, its children
are killed when it exits.

Also, backed out the work around changes made for 1645680.

[Scott Moser (smoser)]

I started a jenkins vmtest run at https://jenkins.ubuntu.com/server/view/Curtin/job/curtin-vmtest-devel-debug/39/console

[Scott Moser (smoser)]

This does need unit test additions fixes, and probably even fails unit tests at this point.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:499
https://jenkins.ubuntu.com/server/job/curtin-ci/488/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/488/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/488/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/488/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/488/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/488/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

Most of the complexity here is in trying to default to unshare_pid=None,
and having that do the right thing, which means
  a.) if i'm root
  b.) if target is not /
  c.) i have the unshare command with --fork and --pid, which does not exist on older versions o f ubuntu

Then there is some pain to avoid recursion in the subp call for 'unshare --help' to have it avoid recursing.

That complexity seems necessary, otherwise we have to change all the callers to call with unshare_pid=True only when they can.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:500
https://jenkins.ubuntu.com/server/job/curtin-ci/500/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/500
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/500
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/500
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/500

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/500/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Much nicer now, but still needs some unit tests.
I've started vmtest run at
 https://jenkins.ubuntu.com/server/view/Curtin/job/curtin-vmtest-devel-debug/45/

I have high hopes at the moment.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:502
https://jenkins.ubuntu.com/server/job/curtin-ci/501/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/501/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/501/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/501/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/501/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/501/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:503
https://jenkins.ubuntu.com/server/job/curtin-ci/503/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/503/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/503/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/503/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/503/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/503/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:504
https://jenkins.ubuntu.com/server/job/curtin-ci/508/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/508
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/508
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/508
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/508

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/508/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

replies inline.

[Scott Moser (smoser)]

I guess the issue with only calling it in specific cases is it is just not easy to do there.
as we'd have to either
a.) have the caller determine "can i use unshare?" and then pass it in if they want it.
b.) default to False and provide some way to indicate "use if you can".

I guess 'b' would equate to the current branch but with the default to 'false' and the 'apt' case explciitly passing in 'None'. or we could make some nicer name. util.UNSHARE_AUTO or something.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:505
https://jenkins.ubuntu.com/server/job/curtin-ci/509/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/509
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/509
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/509
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/509

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/509/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

This looks clearer with the separate methods for detecting and then unshare arg building.

w.r.t the policy; I think this is similar to the strict mode we use. Ideally we always use unshare pid for chroot, if available; we have a few cases where we *know* we require it and we have a workaround if it's not present.

I wonder if we could detect if it's available, and if not (and we're in strict=True) then apply the fallback?

OTOH, do we know if any supported release where we won't have unshare pid support available and that we also require the workarounds (apt is the only case I'm aware of)?

[Scott Moser (smoser)]

> OTOH, do we know if any supported release where we won't have unshare pid
> support available and that we also require the workarounds (apt is the only
> case I'm aware of)?

'unshare --fork --pid' should work in xenial and beyond.
yakkety+ is where we explicitly need the apt fix.

So we end up using unshare on xenial and newer whenever we have target != /.
And we do *not* use it wherever target == / or precise or trusty.

[lp:~smoser/curtin/trunk.unshare-chroot]

500. By Scott Moser on 2017-06-04

merge with trunk

501. By Scott Moser on 2017-06-05

merge with unshare-chroot branch

502. By Scott Moser on 2017-06-05

add unit tests

503. By Scott Moser on 2017-06-05

pep8

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:503
https://jenkins.ubuntu.com/server/job/curtin-ci/528/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/528
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/528
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/528
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/528
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/528

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/528/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

https://jenkins.ubuntu.com/server/view/Curtin/job/curtin-vmtest-devel-debug/78/

vmtest run at ^.

I would like more review on this, i think its sane at this point.

[Chad Smith (chad.smith)]

Some minor nits inline. Looks good learned a bit about unshare today.

[lp:~smoser/curtin/trunk.unshare-chroot]

504. By Scott Moser on 2017-06-09

address feedback

- push subp through taking a string as input.
- push subp through taking a tuple as input.
- add a docstring

505. By Scott Moser on 2017-06-09

address feedback wrt self.expected_on and self.expected_off

506. By Scott Moser on 2017-06-09

address feedback drop some code in test_unshare_pid_true_and_not_root_raises

507. By Scott Moser on 2017-06-09

test_euid0_target_not_slash: use only by kwargs

[Scott Moser (smoser)]

Chad,
I think I addressed all your feedback. I did leave the assignment to False.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:507
https://jenkins.ubuntu.com/server/job/curtin-ci/530/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/530
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/530
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/530
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/530
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/530

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/530/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

+1 thanks for addressing the review comments.

review: Approve

[Ryan Harper (raharper)]

I've started another vmtest run, we don't have a successful run of this branch due to the port issue which is now fixed in trunk,

https://jenkins.ubuntu.com/server/view/Curtin/job/curtin-vmtest-devel-debug/80/

If that passes, I'll push that merge branch to trunk.

[Chad Smith (chad.smith)]

Looks all green per the jenkins vmtest run.

[Scott Moser (smoser)]

I merged and pushed.
revno 506.

=== modified file 'curtin/commands/apt_config.py'
--- curtin/commands/apt_config.py	2017-02-09 17:55:33 +0000
+++ curtin/commands/apt_config.py	2017-06-09 20:54:37 +0000
@@ -24,7 +24,6 @@
 import os
 import re
 import sys
-import time
 import yaml
 
 from curtin.log import LOG
@@ -406,20 +405,12 @@
         if aa_repo_match(source):
             with util.ChrootableTarget(
                     target, sys_resolvconf=True) as in_chroot:
-                time_entered = time.time()
                 try:
                     in_chroot.subp(["add-apt-repository", source],
                                    retries=(1, 2, 5, 10))
                 except util.ProcessExecutionError:
                     LOG.exception("add-apt-repository failed.")
                     raise
-                finally:
-                    # workaround to gnupg >=2.x spawning daemons (LP: #1645680)
-                    seconds_since = time.time() - time_entered + 1
-                    in_chroot.subp(['killall', '--wait', '--quiet',
-                                    '--younger-than', '%ds' % seconds_since,
-                                    '--regexp', '(dirmngr|gpg-agent)'],
-                                   rcs=[0, 1])
             continue
 
         sourcefn = util.target_path(target, ent['filename'])

=== modified file 'curtin/util.py'
--- curtin/util.py	2017-05-26 18:27:22 +0000
+++ curtin/util.py	2017-06-09 20:54:37 +0000
@@ -57,6 +57,7 @@
 _INSTALLED_MAIN = '/usr/bin/curtin'
 
 _LSB_RELEASE = {}
+_HAS_UNSHARE_PID = None
 
 _DNS_REDIRECT_IP = None
 
@@ -66,21 +67,31 @@
 
 def _subp(args, data=None, rcs=None, env=None, capture=False,
           shell=False, logstring=False, decode="replace",
-          target=None, cwd=None, log_captured=False):
+          target=None, cwd=None, log_captured=False, unshare_pid=None):
     if rcs is None:
         rcs = [0]
-
     devnull_fp = None
-    try:
-        if target_path(target) != "/":
-            args = ['chroot', target] + list(args)
-
-        if not logstring:
-            LOG.debug(("Running command %s with allowed return codes %s"
-                       " (shell=%s, capture=%s)"), args, rcs, shell, capture)
-        else:
-            LOG.debug(("Running hidden command to protect sensitive "
-                       "input/output logstring: %s"), logstring)
+
+    tpath = target_path(target)
+    chroot_args = [] if tpath == "/" else ['chroot', target]
+    sh_args = ['sh', '-c'] if shell else []
+    if isinstance(args, string_types):
+        args = [args]
+
+    try:
+        unshare_args = _get_unshare_pid_args(unshare_pid, tpath)
+    except RuntimeError as e:
+        raise RuntimeError("Unable to unshare pid (cmd=%s): %s" % (args, e))
+
+    args = unshare_args + chroot_args + sh_args + list(args)
+
+    if not logstring:
+        LOG.debug(("Running command %s with allowed return codes %s"
+                   " (capture=%s)"), args, rcs, capture)
+    else:
+        LOG.debug(("Running hidden command to protect sensitive "
+                   "input/output logstring: %s"), logstring)
+    try:
         stdin = None
         stdout = None
         stderr = None
@@ -94,7 +105,7 @@
             stdin = subprocess.PIPE
         sp = subprocess.Popen(args, stdout=stdout,
                               stderr=stderr, stdin=stdin,
-                              env=env, shell=shell, cwd=cwd)
+                              env=env, shell=False, cwd=cwd)
         # communicate in python2 returns str, python3 returns bytes
         (out, err) = sp.communicate(data)
 
@@ -128,6 +139,63 @@
     return (out, err)
 
 
+def _has_unshare_pid():
+    global _HAS_UNSHARE_PID
+    if _HAS_UNSHARE_PID is not None:
+        return _HAS_UNSHARE_PID
+
+    if not which('unshare'):
+        _HAS_UNSHARE_PID = False
+        return False
+    out, err = subp(["unshare", "--help"], capture=True, decode=False,
+                    unshare_pid=False)
+    joined = b'\n'.join([out, err])
+    _HAS_UNSHARE_PID = b'--fork' in joined and b'--pid' in joined
+    return _HAS_UNSHARE_PID
+
+
+def _get_unshare_pid_args(unshare_pid=None, target=None, euid=None):
+    """Get args for calling unshare for a pid.
+
+    If unshare_pid is False, return empty list.
+    If unshare_pid is True, check if it is usable.  If not, raise exception.
+    if unshare_pid is None, then unshare if
+       * euid is 0
+       * 'unshare' with '--fork' and '--pid' is available.
+       * target != /
+    """
+    if unshare_pid is not None and not unshare_pid:
+        # given a false-ish other than None means no.
+        return []
+
+    if euid is None:
+        euid = os.geteuid()
+
+    tpath = target_path(target)
+
+    unshare_pid_in = unshare_pid
+    if unshare_pid is None:
+        unshare_pid = False
+        if tpath != "/" and euid == 0:
+            if _has_unshare_pid():
+                unshare_pid = True
+
+    if not unshare_pid:
+        return []
+
+    # either unshare was passed in as True, or None and turned to True.
+    if euid != 0:
+        raise RuntimeError(
+            "given unshare_pid=%s but euid (%s) != 0." %
+            (unshare_pid_in, euid))
+
+    if not _has_unshare_pid():
+        raise RuntimeError(
+            "given unshare_pid=%s but no unshare command." % unshare_pid_in)
+
+    return ['unshare', '--fork', '--pid', '--']
+
+
 def subp(*args, **kwargs):
     """Run a subprocess.
 
@@ -160,6 +228,10 @@
         means to run, sleep 1, run, sleep 3, run and then return exit code.
     :param target:
         run the command as 'chroot target <args>'
+    :param unshare_pid:
+        unshare the pid namespace.
+        default value (None) is to unshare pid namespace if possible
+        and target != /
 
     :return
         if not capturing, return is (None, None)

=== modified file 'tests/unittests/test_util.py'
--- tests/unittests/test_util.py	2017-05-12 14:35:53 +0000
+++ tests/unittests/test_util.py	2017-06-09 20:54:37 +0000
@@ -160,6 +160,13 @@
         decode_type = str
         nodecode_type = bytes
 
+    def setUp(self):
+        super(TestSubp, self).setUp()
+        mock_getunsh = mock.patch(
+            "curtin.util._get_unshare_pid_args", return_value=[])
+        self.mock_get_unshare_pid_args = mock_getunsh.start()
+        self.addCleanup(mock_getunsh.stop)
+
     def printf_cmd(self, *args):
         # bash's printf supports \xaa.  So does /usr/bin/printf
         # but by using bash, we remove dependency on another program.
@@ -296,12 +303,29 @@
         calls = m_popen.call_args_list
         popen_args, popen_kwargs = calls[-1]
         target = util.target_path(kwargs.get('target', None))
+        unshcmd = self.mock_get_unshare_pid_args.return_value
         if target == "/":
-            self.assertEqual(cmd, popen_args[0])
+            self.assertEqual(unshcmd + list(cmd), popen_args[0])
         else:
-            self.assertEqual(['chroot', target] + list(cmd), popen_args[0])
+            self.assertEqual(unshcmd + ['chroot', target] + list(cmd),
+                             popen_args[0])
         return calls
 
+    def test_args_can_be_a_tuple(self):
+        """subp can take a tuple for cmd rather than a list."""
+        my_cmd = tuple(['echo', 'hi', 'mom'])
+        calls = self._subp_wrap_popen(my_cmd, {})
+        args, kwargs = calls[0]
+        # subp was called with cmd as a tuple.  That may get converted to
+        # a list before subprocess.popen.  So only compare as lists.
+        self.assertEqual(1, len(calls))
+        self.assertEqual(list(my_cmd), list(args[0]))
+
+    def test_args_can_be_a_string(self):
+        """subp("cat") is acceptable, as suprocess.call("cat") works fine."""
+        out, err = util.subp("cat", data=b'hi mom', capture=True, decode=False)
+        self.assertEqual(b'hi mom', out)
+
     def test_with_target_gets_chroot(self):
         args, kwargs = self._subp_wrap_popen(["my-command"],
                                              {'target': "/mytarget"})[0]
@@ -342,6 +366,94 @@
         # since we fail a few times, it needs to have been called again.
         self.assertEqual(len(r), len(rcs))
 
+    def test_unshare_pid_return_is_used(self):
+        """The return of _get_unshare_pid_return needs to be in command."""
+        my_unshare_cmd = ['do-unshare-command', 'arg0', 'arg1', '--']
+        self.mock_get_unshare_pid_args.return_value = my_unshare_cmd
+        my_kwargs = {'target': '/target', 'unshare_pid': True}
+        r = self._subp_wrap_popen(['apt-get', 'install'], my_kwargs)
+        self.assertEqual(1, len(r))
+        args, kwargs = r[0]
+        self.assertEqual(
+            [mock.call(my_kwargs['unshare_pid'], my_kwargs['target'])],
+            self.mock_get_unshare_pid_args.call_args_list)
+        expected = (my_unshare_cmd + ['chroot', '/target'] +
+                    ['apt-get', 'install'])
+        self.assertEqual(expected, args[0])
+
+
+class TestGetUnsharePidArgs(TestCase):
+    """Test the internal implementation for when to unshare."""
+
+    def setUp(self):
+        super(TestGetUnsharePidArgs, self).setUp()
+        mock_hasunsh = mock.patch(
+            "curtin.util._has_unshare_pid", return_value=True)
+        self.mock_has_unshare_pid = mock_hasunsh.start()
+        self.addCleanup(mock_hasunsh.stop)
+        mock_geteuid = mock.patch(
+            "curtin.util.os.geteuid", return_value=0)
+        self.mock_geteuid = mock_geteuid.start()
+        self.addCleanup(mock_geteuid.stop)
+
+    def assertOff(self, result):
+        self.assertEqual([], result)
+
+    def assertOn(self, result):
+        self.assertEqual(['unshare', '--fork', '--pid', '--'], result)
+
+    def test_unshare_pid_none_and_not_root_means_off(self):
+        """If not root, then expect off."""
+        self.assertOff(util._get_unshare_pid_args(None, "/foo", 500))
+        self.assertOff(util._get_unshare_pid_args(None, "/", 500))
+
+        self.mock_geteuid.return_value = 500
+        self.assertOff(util._get_unshare_pid_args(None, "/"))
+        self.assertOff(
+            util._get_unshare_pid_args(unshare_pid=None, target="/foo"))
+
+    def test_unshare_pid_none_and_no_unshare_pid_means_off(self):
+        """No unshare support and unshare_pid is None means off."""
+        self.mock_has_unshare_pid.return_value = False
+        self.assertOff(util._get_unshare_pid_args(None, "/target", 0))
+
+    def test_unshare_pid_true_and_no_unshare_pid_raises(self):
+        """Passing unshare_pid in as True and no command should raise."""
+        self.mock_has_unshare_pid.return_value = False
+        expected_msg = 'no unshare command'
+        with self.assertRaisesRegexp(RuntimeError, expected_msg):
+            util._get_unshare_pid_args(True)
+
+        with self.assertRaisesRegexp(RuntimeError, expected_msg):
+            util._get_unshare_pid_args(True, "/foo", 0)
+
+    def test_unshare_pid_true_and_not_root_raises(self):
+        """When unshare_pid is True for non-root an error is raised."""
+        expected_msg = 'euid.* != 0'
+        with self.assertRaisesRegexp(RuntimeError, expected_msg):
+            util._get_unshare_pid_args(True, "/foo", 500)
+
+        self.mock_geteuid.return_value = 500
+        with self.assertRaisesRegexp(RuntimeError, expected_msg):
+            util._get_unshare_pid_args(True)
+
+    def test_euid0_target_not_slash(self):
+        """If root and target is not /, then expect on."""
+        self.assertOn(util._get_unshare_pid_args(None, target="/foo", euid=0))
+
+    def test_euid0_target_slash(self):
+        """If root and target is /, then expect off."""
+        self.assertOff(util._get_unshare_pid_args(None, "/", 0))
+        self.assertOff(util._get_unshare_pid_args(None, target=None, euid=0))
+
+    def test_unshare_pid_of_false_means_off(self):
+        """Any unshare_pid value false-ish other than None means no unshare."""
+        self.assertOff(
+            util._get_unshare_pid_args(unshare_pid=False, target=None))
+        self.assertOff(util._get_unshare_pid_args(False, "/target", 1))
+        self.assertOff(util._get_unshare_pid_args(False, "/", 0))
+        self.assertOff(util._get_unshare_pid_args("", "/target", 0))
+
 
 class TestHuman2Bytes(TestCase):
     GB = 1024 * 1024 * 1024