From 47620961fe8eb9ec859b33bd0480a698e655af69 Mon Sep 17 00:00:00 2001 From: Jhon Honce Date: Fri, 27 Jul 2018 15:09:07 -0700 Subject: 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 Closes: #1199 Approved by: rhatdan --- contrib/python/podman/podman/client.py | 73 +++++++++---------- contrib/python/podman/podman/libs/tunnel.py | 97 ++++++++++++++++++++------ contrib/python/podman/requirements.txt | 1 + contrib/python/podman/test/test_client.py | 2 +- contrib/python/podman/test/test_runner.sh | 25 ++++--- contrib/python/podman/test/test_system.py | 4 +- contrib/python/podman/test/test_tunnel.py | 11 +-- contrib/python/pypodman/docs/man1/pypodman.1 | 5 ++ contrib/python/pypodman/pypodman/lib/config.py | 46 ++++++++++-- contrib/python/pypodman/pypodman/main.py | 6 +- contrib/spec/podman.spec.in | 1 + contrib/spec/python-podman.spec.in | 1 + 12 files changed, 191 insertions(+), 81 deletions(-) (limited to 'contrib') 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,10 +86,17 @@ 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', @@ -75,11 +104,14 @@ class PodmanArgumentParser(argparse.ArgumentParser): ' (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, diff --git a/contrib/spec/podman.spec.in b/contrib/spec/podman.spec.in index 1a850a55d..7284ddc71 100644 --- a/contrib/spec/podman.spec.in +++ b/contrib/spec/podman.spec.in @@ -203,6 +203,7 @@ BuildRequires: python3-varlink Requires: python3-setuptools Requires: python3-varlink Requires: python3-dateutil +Requires: python3-psutil Provides: python3-%{name} = %{version}-%{release} Summary: Python 3 bindings and client for %{name} diff --git a/contrib/spec/python-podman.spec.in b/contrib/spec/python-podman.spec.in index d7956d110..04b6039ad 100644 --- a/contrib/spec/python-podman.spec.in +++ b/contrib/spec/python-podman.spec.in @@ -42,6 +42,7 @@ Requires: python3-humanize Requires: python3-pytoml Requires: python3-setuptools Requires: python3-varlink +Requires: python3-psutil Requires: podman %if 0%{?fedora} -- cgit v1.2.3-54-g00ecf