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 ++-- 7 files changed, 138 insertions(+), 75 deletions(-) (limited to 'contrib/python/podman') 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) -- cgit v1.2.3-54-g00ecf