Port to MacOS

* Refactor Tunnel to support selecting port for remote sshd
* Refactor ssh tunnel to support MacOS version of ssh
* Refactor Tunnel.close() to find and kill off zombie siblings
* Add psutil dependency
* Add logging setup, letting library produce debugging records
* Clean up Tunnel API
* Fix test_runner.sh to propagate returncode to caller

Signed-off-by: Jhon Honce <jhonce@redhat.com>

Closes: #1199
Approved by: rhatdan

10 files changed, 189 insertions, 81 deletions

diff --git a/contrib/python/podman/podman/client.py b/contrib/python/podman/podman/client.py
index 5efd2be2b..fedd9310e 100644
--- a/contrib/python/podman/podman/client.py
+++ b/contrib/python/podman/podman/client.py
@@ -1,5 +1,6 @@
 """A client for communicating with a Podman varlink service."""
 import errno
+import logging
 import os
 from urllib.parse import urlparse
 
@@ -34,13 +35,17 @@ class BaseClient():
                 *args,
                 **kwargs):
         """Construct a Client based on input."""
+        log_level = os.environ.get('LOG_LEVEL')
+        if log_level is not None:
+            logging.basicConfig(level=logging.getLevelName(log_level.upper()))
+
         if uri is None:
             raise ValueError('uri is required and cannot be None')
         if interface is None:
             raise ValueError('interface is required and cannot be None')
 
         unsupported = set(kwargs.keys()).difference(
-            ('uri', 'interface', 'remote_uri', 'identity_file'))
+            ('uri', 'interface', 'remote_uri', 'port', 'identity_file'))
         if unsupported:
             raise ValueError('Unknown keyword arguments: {}'.format(
                 ', '.join(unsupported)))
@@ -50,38 +55,35 @@ class BaseClient():
             raise ValueError('path is required for uri,'
                              ' expected format "unix://path_to_socket"')
 
-        if kwargs.get('remote_uri'):
-            # Remote access requires the full tuple of information
-            if kwargs.get('remote_uri') is None:
-                raise ValueError(
-                    'remote_uri is required,'
-                    ' expected format "ssh://user@hostname/path_to_socket".')
-            remote = urlparse(kwargs['remote_uri'])
-            if remote.username is None:
-                raise ValueError(
-                    'username is required for remote_uri,'
-                    ' expected format "ssh://user@hostname/path_to_socket".')
-            if remote.path == '':
-                raise ValueError(
-                    'path is required for remote_uri,'
-                    ' expected format "ssh://user@hostname/path_to_socket".')
-            if remote.hostname is None:
-                raise ValueError(
-                    'hostname is required for remote_uri,'
-                    ' expected format "ssh://user@hostname/path_to_socket".')
-
-            return RemoteClient(
-                Context(
-                    uri,
-                    interface,
-                    local_path,
-                    remote.path,
-                    remote.username,
-                    remote.hostname,
-                    kwargs.get('identity_file'),
-                ))
-        return LocalClient(
-            Context(uri, interface, None, None, None, None, None))
+        if kwargs.get('remote_uri') is None:
+            return LocalClient(Context(uri, interface))
+
+        required = ('{} is required, expected format'
+                    ' "ssh://user@hostname[:port]/path_to_socket".')
+
+        # Remote access requires the full tuple of information
+        if kwargs.get('remote_uri') is None:
+            raise ValueError(required.format('remote_uri'))
+
+        remote = urlparse(kwargs['remote_uri'])
+        if remote.username is None:
+            raise ValueError(required.format('username'))
+        if remote.path == '':
+            raise ValueError(required.format('path'))
+        if remote.hostname is None:
+            raise ValueError(required.format('hostname'))
+
+        return RemoteClient(
+            Context(
+                uri,
+                interface,
+                local_path,
+                remote.path,
+                remote.username,
+                remote.hostname,
+                remote.port,
+                kwargs.get('identity_file'),
+            ))
 
 
 class LocalClient(BaseClient):
@@ -115,7 +117,7 @@ class RemoteClient(BaseClient):
         """Context manager for API workers to access varlink."""
         tunnel = self._portal.get(self._context.uri)
         if tunnel is None:
-            tunnel = Tunnel(self._context).bore(self._context.uri)
+            tunnel = Tunnel(self._context).bore()
             self._portal[self._context.uri] = tunnel
 
         try:
@@ -123,7 +125,7 @@ class RemoteClient(BaseClient):
             self._iface = self._client.open(self._context.interface)
             return self._iface
         except Exception:
-            tunnel.close(self._context.uri)
+            tunnel.close()
             raise
 
     def __exit__(self, e_type, e, e_traceback):
@@ -133,6 +135,7 @@ class RemoteClient(BaseClient):
         self._iface.close()
 
         # set timer to shutdown ssh tunnel
+        # self._portal.get(self._context.uri).close()
         if isinstance(e, VarlinkError):
             raise error_factory(e)
 
diff --git a/contrib/python/podman/podman/libs/tunnel.py b/contrib/python/podman/podman/libs/tunnel.py
index af02e690f..ac1dff594 100644
--- a/contrib/python/podman/podman/libs/tunnel.py
+++ b/contrib/python/podman/podman/libs/tunnel.py
@@ -1,11 +1,15 @@
 """Cache for SSH tunnels."""
 import collections
+import getpass
 import logging
 import os
 import subprocess
 import threading
 import time
 import weakref
+from contextlib import suppress
+
+import psutil
 
 Context = collections.namedtuple('Context', (
     'uri',
@@ -14,8 +18,10 @@ Context = collections.namedtuple('Context', (
     'remote_socket',
     'username',
     'hostname',
+    'port',
     'identity_file',
 ))
+Context.__new__.__defaults__ = (None, ) * len(Context._fields)
 
 
 class Portal(collections.MutableMapping):
@@ -51,7 +57,7 @@ class Portal(collections.MutableMapping):
         with self.lock:
             value, _ = self.data[key]
             del self.data[key]
-            value.close(key)
+            value.close()
             del value
 
     def __iter__(self):
@@ -93,47 +99,92 @@ class Tunnel():
     def __init__(self, context):
         """Construct Tunnel."""
         self.context = context
-        self._tunnel = None
+        self._closed = True
+
+    @property
+    def closed(self):
+        """Is tunnel closed."""
+        return self._closed
 
-    def bore(self, ident):
+    def bore(self):
         """Create SSH tunnel from given context."""
-        cmd = ['ssh']
+        cmd = ['ssh', '-fNT']
 
-        ssh_opts = '-fNT'
         if logging.getLogger().getEffectiveLevel() == logging.DEBUG:
-            ssh_opts += 'v'
+            cmd.append('-v')
         else:
-            ssh_opts += 'q'
-        cmd.append(ssh_opts)
+            cmd.append('-q')
+
+        if self.context.port:
+            cmd.extend(('-p', str(self.context.port)))
 
         cmd.extend(('-L', '{}:{}'.format(self.context.local_socket,
                                          self.context.remote_socket)))
         if self.context.identity_file:
             cmd.extend(('-i', self.context.identity_file))
 
-        cmd.append('ssh://{}@{}'.format(self.context.username,
-                                        self.context.hostname))
+        cmd.append('{}@{}'.format(self.context.username,
+                                  self.context.hostname))
 
-        logging.debug('Tunnel cmd "%s"', ' '.join(cmd))
+        logging.debug('Opening tunnel "%s", cmd "%s"', self.context.uri,
+                      ' '.join(cmd))
 
-        self._tunnel = subprocess.Popen(cmd, close_fds=True)
+        tunnel = subprocess.Popen(cmd, close_fds=True)
+        # The return value of Popen() has no long term value as that process
+        # has already exited by the time control is returned here. This is a
+        # side effect of the -f option. wait() will be called to clean up
+        # resources.
         for _ in range(300):
             # TODO: Make timeout configurable
-            if os.path.exists(self.context.local_socket):
+            if os.path.exists(self.context.local_socket) \
+                    or tunnel.returncode is not None:
                 break
-            time.sleep(0.5)
+            with suppress(subprocess.TimeoutExpired):
+                # waiting for either socket to be created
+                # or first child to exit
+                tunnel.wait(0.5)
         else:
-            raise TimeoutError('Failed to create tunnel using: {}'.format(
-                ' '.join(cmd)))
-        weakref.finalize(self, self.close, ident)
+            raise TimeoutError(
+                'Failed to create tunnel "{}", using: "{}"'.format(
+                    self.context.uri, ' '.join(cmd)))
+        if tunnel.returncode is not None and tunnel.returncode != 0:
+            raise subprocess.CalledProcessError(tunnel.returncode,
+                                                ' '.join(cmd))
+        tunnel.wait()
+
+        self._closed = False
+        weakref.finalize(self, self.close)
         return self
 
-    def close(self, ident):
+    def close(self):
         """Close SSH tunnel."""
-        if self._tunnel is None:
+        logging.debug('Closing tunnel "%s"', self.context.uri)
+
+        if self._closed:
             return
 
-        self._tunnel.kill()
-        self._tunnel.wait(300)
-        os.remove(self.context.local_socket)
-        self._tunnel = None
+        # Find all ssh instances for user with uri tunnel the hard way...
+        targets = [
+            p
+            for p in psutil.process_iter(attrs=['name', 'username', 'cmdline'])
+            if p.info['username'] == getpass.getuser()
+            and p.info['name'] == 'ssh'
+            and self.context.local_socket in ' '.join(p.info['cmdline'])
+        ]  # yapf: disable
+
+        # ask nicely for ssh to quit, reap results
+        for proc in targets:
+            proc.terminate()
+        _, alive = psutil.wait_procs(targets, timeout=300)
+
+        # kill off the uncooperative, then report any stragglers
+        for proc in alive:
+            proc.kill()
+        _, alive = psutil.wait_procs(targets, timeout=300)
+
+        for proc in alive:
+            logging.info('process %d survived SIGKILL, giving up.', proc.pid)
+
+        with suppress(OSError):
+            os.remove(self.context.local_socket)
+        self._closed = True
diff --git a/contrib/python/podman/requirements.txt b/contrib/python/podman/requirements.txt
index 5a936e7d5..1caf5e286 100644
--- a/contrib/python/podman/requirements.txt
+++ b/contrib/python/podman/requirements.txt
@@ -1,3 +1,4 @@
+psutil
 python-dateutil
 setuptools>=39
 varlink
diff --git a/contrib/python/podman/test/test_client.py b/contrib/python/podman/test/test_client.py
index 2abc60a24..d36cb1a43 100644
--- a/contrib/python/podman/test/test_client.py
+++ b/contrib/python/podman/test/test_client.py
@@ -28,7 +28,7 @@ class TestClient(unittest.TestCase):
         p = Client(
             uri='unix:/run/podman',
             interface='io.projectatomic.podman',
-            remote_uri='ssh://user@hostname/run/podmain/podman',
+            remote_uri='ssh://user@hostname/run/podman/podman',
             identity_file='~/.ssh/id_rsa')
 
         self.assertIsInstance(p._client, BaseClient)
diff --git a/contrib/python/podman/test/test_runner.sh b/contrib/python/podman/test/test_runner.sh
index b3d2ba15b..4771709ff 100755
--- a/contrib/python/podman/test/test_runner.sh
+++ b/contrib/python/podman/test/test_runner.sh
@@ -19,9 +19,9 @@ function usage {
 
 while getopts "vh" arg; do
   case $arg in
-    v ) VERBOSE='-v' ;;
-    h ) usage ; exit 0;;
-    \? ) usage ; exit 2;;
+    v ) VERBOSE='-v'; export LOG_LEVEL=debug ;;
+    h ) usage ; exit 0 ;;
+    \? ) usage ; exit 2 ;;
   esac
 done
 shift $((OPTIND -1))
@@ -113,29 +113,36 @@ PODMAN_ARGS="--storage-driver=vfs \
   --cni-config-dir=$CNI_CONFIG_PATH \
   "
 if [[ -n $VERBOSE ]]; then
-  PODMAN_ARGS="$PODMAN_ARGS --log-level=debug"
+  PODMAN_ARGS="$PODMAN_ARGS --log-level=$LOG_LEVEL"
 fi
 PODMAN="podman $PODMAN_ARGS"
 
-# document what we're about to do...
-$PODMAN --version
+# Run podman in background without systemd for test purposes
+cat >/tmp/test_podman.output <<-EOT
+$($PODMAN --version)
+$PODMAN varlink --timeout=0 ${PODMAN_HOST}
+==========================================
+EOT
 
 set -x
-# Run podman in background without systemd for test purposes
-$PODMAN varlink --timeout=0 ${PODMAN_HOST} >/tmp/test_runner.output 2>&1 &
+$PODMAN varlink --timeout=0 ${PODMAN_HOST} >>/tmp/test_podman.output 2>&1 &
 
 if [[ -z $1 ]]; then
   export PYTHONPATH=.
   python3 -m unittest discover -s . $VERBOSE
+  RETURNCODE=$?
 else
   export PYTHONPATH=.:./test
   python3 -m unittest $1 $VERBOSE
+  RETURNCODE=$?
 fi
 
 set +x
 pkill -9 podman
 pkill -9 conmon
 
-showlog /tmp/test_runner.output
+showlog /tmp/test_podman.output
 showlog /tmp/alpine.log
 showlog /tmp/busybox.log
+
+exit $RETURNCODE
diff --git a/contrib/python/podman/test/test_system.py b/contrib/python/podman/test/test_system.py
index 3f6ca57a2..2b1342f8a 100644
--- a/contrib/python/podman/test/test_system.py
+++ b/contrib/python/podman/test/test_system.py
@@ -25,7 +25,7 @@ class TestSystem(unittest.TestCase):
 
     def test_remote_ping(self):
         host = urlparse(self.host)
-        remote_uri = 'ssh://root@localhost/{}'.format(host.path)
+        remote_uri = 'ssh://root@localhost{}'.format(host.path)
 
         local_uri = 'unix:{}/tunnel/podman.sock'.format(self.tmpdir)
         with podman.Client(
@@ -33,7 +33,7 @@ class TestSystem(unittest.TestCase):
                 remote_uri=remote_uri,
                 identity_file=os.path.expanduser('~/.ssh/id_rsa'),
         ) as remote_client:
-            remote_client.system.ping()
+            self.assertTrue(remote_client.system.ping())
 
     def test_versions(self):
         with podman.Client(self.host) as pclient:
diff --git a/contrib/python/podman/test/test_tunnel.py b/contrib/python/podman/test/test_tunnel.py
index 719a2f9a4..ed23fd905 100644
--- a/contrib/python/podman/test/test_tunnel.py
+++ b/contrib/python/podman/test/test_tunnel.py
@@ -4,7 +4,6 @@ import time
 import unittest
 from unittest.mock import MagicMock, patch
 
-import podman
 from podman.libs.tunnel import Context, Portal, Tunnel
 
 
@@ -60,20 +59,22 @@ class TestTunnel(unittest.TestCase):
             '/run/podman/socket',
             'user',
             'hostname',
+            None,
             '~/.ssh/id_rsa',
         )
-        tunnel = Tunnel(context).bore('unix:/01')
+        tunnel = Tunnel(context).bore()
 
         cmd = [
             'ssh',
-            '-fNTq',
+            '-fNT',
+            '-q',
             '-L',
             '{}:{}'.format(context.local_socket, context.remote_socket),
             '-i',
             context.identity_file,
-            'ssh://{}@{}'.format(context.username, context.hostname),
+            '{}@{}'.format(context.username, context.hostname),
         ]
 
-        mock_finalize.assert_called_once_with(tunnel, tunnel.close, 'unix:/01')
+        mock_finalize.assert_called_once_with(tunnel, tunnel.close)
         mock_exists.assert_called_once_with(context.local_socket)
         mock_Popen.assert_called_once_with(cmd, close_fds=True)
diff --git a/contrib/python/pypodman/docs/man1/pypodman.1 b/contrib/python/pypodman/docs/man1/pypodman.1
index 50d88f84d..7c4816062 100644
--- a/contrib/python/pypodman/docs/man1/pypodman.1
+++ b/contrib/python/pypodman/docs/man1/pypodman.1
@@ -46,6 +46,11 @@ user.
 Name of remote host. There is no default, if not given \f[C]pypodman\f[]
 attempts to connect to \f[C]\-\-remote\-socket\-path\f[] on local host.
 .PP
+\f[B]\[en]port\f[]
+.PP
+The optional port for \f[C]ssh\f[] to connect tunnel to on remote host.
+Default is None and will allow \f[C]ssh\f[] to follow it's default configuration.
+.PP
 \f[B]\[en]remote\-socket\-path\f[]
 .PP
 Path on remote host for podman service's \f[C]AF_UNIX\f[] socket. The default is
diff --git a/contrib/python/pypodman/pypodman/lib/config.py b/contrib/python/pypodman/pypodman/lib/config.py
index 75059fc15..137ffdb34 100644
--- a/contrib/python/pypodman/pypodman/lib/config.py
+++ b/contrib/python/pypodman/pypodman/lib/config.py
@@ -33,6 +33,28 @@ class HelpFormatter(argparse.RawDescriptionHelpFormatter):
         super().__init__(*args, **kwargs)
 
 
+class PortAction(argparse.Action):
+    """Validate port number given is positive integer."""
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        """Validate input."""
+        if values > 0:
+            setattr(namespace, self.dest, values)
+            return
+
+        msg = 'port numbers must be a positive integer.'
+        raise argparse.ArgumentError(self, msg)
+
+
+class PathAction(argparse.Action):
+    """Expand user- and relative-paths."""
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        """Resolve full path value."""
+        setattr(namespace, self.dest,
+                os.path.abspath(os.path.expanduser(values)))
+
+
 class PodmanArgumentParser(argparse.ArgumentParser):
     """Default remote podman configuration."""
 
@@ -64,22 +86,32 @@ class PodmanArgumentParser(argparse.ArgumentParser):
                   ' (default: XDG_RUNTIME_DIR/pypodman'))
         self.add_argument(
             '--user',
+            '-l',
             default=getpass.getuser(),
             help='Authenicating user on remote host. (default: %(default)s)')
         self.add_argument(
             '--host', help='name of remote host. (default: None)')
         self.add_argument(
+            '--port',
+            '-p',
+            type=int,
+            action=PortAction,
+            help='port for ssh tunnel to remote host. (default: 22)')
+        self.add_argument(
             '--remote-socket-path',
             metavar='PATH',
             help=('path of podman socket on remote host'
                   ' (default: /run/podman/io.projectatomic.podman)'))
         self.add_argument(
             '--identity-file',
+            '-i',
             metavar='PATH',
+            action=PathAction,
             help=('path to ssh identity file. (default: ~user/.ssh/id_dsa)'))
         self.add_argument(
             '--config-home',
             metavar='DIRECTORY',
+            action=PathAction,
             help=('home of configuration "pypodman.conf".'
                   ' (default: XDG_CONFIG_HOME/pypodman'))
 
@@ -125,8 +157,8 @@ class PodmanArgumentParser(argparse.ArgumentParser):
             if dir_ is None:
                 continue
             with suppress(OSError):
-                with open(os.path.join(dir_, 'pypodman/pypodman.conf'),
-                          'r') as stream:
+                with open(os.path.join(dir_,
+                                       'pypodman/pypodman.conf')) as stream:
                     config.update(pytoml.load(stream))
 
         def reqattr(name, value):
@@ -156,6 +188,7 @@ class PodmanArgumentParser(argparse.ArgumentParser):
             'user',
             getattr(args, 'user')
             or os.environ.get('USER')
+            or os.environ.get('LOGNAME')
             or config['default'].get('user')
             or getpass.getuser()
         )   # yapf:disable
@@ -195,8 +228,13 @@ class PodmanArgumentParser(argparse.ArgumentParser):
             args.local_socket_path = args.remote_socket_path
 
         args.local_uri = "unix:{}".format(args.local_socket_path)
-        args.remote_uri = "ssh://{}@{}{}".format(args.user, args.host,
-                                                 args.remote_socket_path)
+
+        components = ['ssh://', args.user, '@', args.host]
+        if args.port:
+            components.extend((':', str(args.port)))
+        components.append(args.remote_socket_path)
+
+        args.remote_uri = ''.join(components)
         return args
 
     def exit(self, status=0, message=None):
diff --git a/contrib/python/pypodman/pypodman/main.py b/contrib/python/pypodman/pypodman/main.py
index e7055d611..5e0ef0750 100755
--- a/contrib/python/pypodman/pypodman/main.py
+++ b/contrib/python/pypodman/pypodman/main.py
@@ -2,6 +2,7 @@
 import logging
 import os
 import sys
+from subprocess import CalledProcessError
 
 from pypodman.lib import PodmanArgumentParser
 
@@ -49,14 +50,15 @@ def main():
 
     try:
         returncode = getattr(obj, args.method)()
+    except KeyboardInterrupt:
+        pass
     except AttributeError as e:
         logging.critical(e, exc_info=want_tb())
         logging.warning('See subparser "%s" configuration.',
                         args.subparser_name)
         returncode = 3
-    except KeyboardInterrupt:
-        pass
     except (
+            CalledProcessError,
             ConnectionRefusedError,
             ConnectionResetError,
             TimeoutError,