From 8e20f47633e71b0858f7eb310f1adc21b6d21c61 Mon Sep 17 00:00:00 2001 From: Panos Date: Sun, 20 Mar 2022 14:42:49 +0000 Subject: [PATCH] Conn refused (#339) * Handle connection refused better when there are multiple available addresses from DNS resolution * Updated tests, changelog --- Changelog.rst | 15 +++++++++++ pssh/clients/base/single.py | 18 +++++++++++--- pssh/exceptions.py | 8 +----- tests/native/test_parallel_client.py | 36 ++++++++++++--------------- tests/native/test_single_client.py | 37 +++++++++++++++++++++++++--- tests/ssh/test_parallel_client.py | 15 ++++++----- tests/ssh/test_single_client.py | 2 +- 7 files changed, 88 insertions(+), 43 deletions(-) diff --git a/Changelog.rst b/Changelog.rst index ee455c3f..1adf1f09 100644 --- a/Changelog.rst +++ b/Changelog.rst @@ -1,6 +1,21 @@ Change Log ============ +2.9.0 ++++++ + +Changes +-------- + +* ``pssh.exceptions.ConnectionError`` is now the same as built-in ``ConnectionError`` and deprecated - to be removed. +* Clients now continue connecting with all addresses in DNS list. In the case where an address refuses connection, + other available addresses are attempted without delay. + + For example where a host resolves to both IPv4 and v6 addresses while only one address is + accepting connections, or multiple v4/v6 addresses where only some are accepting connections. +* Connection actively refused error is no longer subject to retries. + + 2.8.0 +++++ diff --git a/pssh/clients/base/single.py b/pssh/clients/base/single.py index 33b6d2dd..343c1f99 100644 --- a/pssh/clients/base/single.py +++ b/pssh/clients/base/single.py @@ -207,8 +207,8 @@ def _auth_retry(self, retries=1): if retries < self.num_retries: sleep(self.retry_delay) return self._auth_retry(retries=retries+1) - msg = "Authentication error while connecting to %s:%s - %s" - raise AuthenticationError(msg, self.host, self.port, ex) + msg = "Authentication error while connecting to %s:%s - %s - retries %s/%s" + raise AuthenticationError(msg, self.host, self.port, ex, retries, self.num_retries) def disconnect(self): raise NotImplementedError @@ -284,13 +284,25 @@ def _connect(self, host, port, retries=1): host, str(ex.args[1]), retries, self.num_retries) raise unknown_ex from ex - family, _type, proto, _, sock_addr = addr_info[0] + for i, (family, _type, proto, _, sock_addr) in enumerate(addr_info): + try: + return self._connect_socket(family, _type, proto, sock_addr, host, port, retries) + except ConnectionRefusedError as ex: + if i+1 == len(addr_info): + logger.error("No available addresses from %s", [addr[4] for addr in addr_info]) + ex.args += (host, port) + raise + continue + + def _connect_socket(self, family, _type, proto, sock_addr, host, port, retries): self.sock = socket.socket(family, _type) if self.timeout: self.sock.settimeout(self.timeout) logger.debug("Connecting to %s:%s", host, port) try: self.sock.connect(sock_addr) + except ConnectionRefusedError: + raise except sock_error as ex: logger.error("Error connecting to host '%s:%s' - retry %s/%s", host, port, retries, self.num_retries) diff --git a/pssh/exceptions.py b/pssh/exceptions.py index 016300f3..3e6a3602 100644 --- a/pssh/exceptions.py +++ b/pssh/exceptions.py @@ -35,13 +35,7 @@ class UnknownHostError(Exception): UnknownHostException = UnknownHostError - - -class ConnectionError(Exception): - """Raised on error connecting (connection refused/timed out)""" - pass - - +ConnectionError = ConnectionError ConnectionErrorException = ConnectionError diff --git a/tests/native/test_parallel_client.py b/tests/native/test_parallel_client.py index 84fcf5e4..9f403224 100644 --- a/tests/native/test_parallel_client.py +++ b/tests/native/test_parallel_client.py @@ -35,7 +35,8 @@ from pssh.exceptions import UnknownHostException, \ AuthenticationException, ConnectionErrorException, \ HostArgumentException, SFTPError, SFTPIOError, Timeout, SCPError, \ - PKeyFileError, ShellError, HostArgumentError, NoIPv6AddressFoundError + PKeyFileError, ShellError, HostArgumentError, NoIPv6AddressFoundError, \ + AuthenticationError from pssh.output import HostOutput from .base_ssh2_case import PKEY_FILENAME, PUB_FILE @@ -276,7 +277,7 @@ def test_pssh_client_hosts_list_part_failure(self): self.assertTrue(client.finished(output)) self.assertEqual(len(hosts), len(output)) self.assertIsNotNone(output[1].exception) - self.assertEqual(output[1].exception.args[1], hosts[1]) + self.assertEqual(output[1].host, hosts[1]) self.assertIsInstance(output[1].exception, ConnectionErrorException) def test_pssh_client_timeout(self): @@ -350,23 +351,23 @@ def test_pssh_client_long_running_command_exit_codes_no_stdout(self): def test_pssh_client_retries(self): """Test connection error retries""" - listen_port = self.make_random_port() + # listen_port = self.make_random_port() expected_num_tries = 2 - client = ParallelSSHClient([self.host], port=listen_port, - pkey=self.user_key, + client = ParallelSSHClient([self.host], port=self.port, + pkey=b"fake", num_retries=expected_num_tries, retry_delay=.1, ) - self.assertRaises(ConnectionErrorException, client.run_command, 'blah') + self.assertRaises(AuthenticationError, client.run_command, 'blah') try: client.run_command('blah') - except ConnectionErrorException as ex: + except AuthenticationError as ex: + max_tries = ex.args[-2:][0] num_tries = ex.args[-1:][0] - self.assertEqual(expected_num_tries, num_tries, - msg="Got unexpected number of retries %s - " - "expected %s" % (num_tries, expected_num_tries,)) + self.assertEqual(expected_num_tries, max_tries) + self.assertEqual(expected_num_tries, num_tries) else: - raise Exception('No ConnectionErrorException') + raise Exception('No AuthenticationError') def test_sftp_exceptions(self): # Port with no server listening on it on separate ip @@ -380,7 +381,8 @@ def test_sftp_exceptions(self): try: cmd.get() except Exception as ex: - self.assertEqual(ex.args[1], self.host) + self.assertEqual(ex.args[2], self.host) + self.assertEqual(ex.args[3], port) self.assertIsInstance(ex, ConnectionErrorException) else: raise Exception("Expected ConnectionErrorException, got none") @@ -859,7 +861,7 @@ def test_identical_hosts_in_host_list(self): _host_stdout = list(host_out.stdout) self.assertListEqual(_host_stdout, expected_stdout) - def test_connection_error_exception(self): + def test_connection_error(self): """Test that we get connection error exception in output with correct arguments""" # Make port with no server listening on it on separate ip host = '127.0.0.3' @@ -874,13 +876,7 @@ def test_connection_error_exception(self): for host_output in output: exit_code = host_output.exit_code self.assertEqual(exit_code, None) - try: - raise output[0].exception - except ConnectionErrorException as ex: - self.assertEqual(ex.args[1], host) - self.assertEqual(ex.args[2], port) - else: - raise Exception("Expected ConnectionErrorException") + self.assertIsInstance(output[0].exception, ConnectionError) def test_bad_pkey_path(self): self.assertRaises(PKeyFileError, ParallelSSHClient, [self.host], port=self.port, diff --git a/tests/native/test_single_client.py b/tests/native/test_single_client.py index 0893b52e..5aa3bd6f 100644 --- a/tests/native/test_single_client.py +++ b/tests/native/test_single_client.py @@ -20,6 +20,8 @@ import shutil import tempfile from tempfile import NamedTemporaryFile + +import pytest from pytest import raises from unittest.mock import MagicMock, call, patch from hashlib import sha256 @@ -36,7 +38,7 @@ ) from pssh.exceptions import (AuthenticationException, ConnectionErrorException, SessionError, SFTPIOError, SFTPError, SCPError, PKeyFileError, Timeout, - AuthenticationError, NoIPv6AddressFoundError, + AuthenticationError, NoIPv6AddressFoundError, ConnectionError ) from .base_ssh2_case import SSH2TestCase @@ -89,6 +91,10 @@ def _sftp_exc(local_file, remote_file): self.assertRaises( SFTPIOError, client.copy_remote_file, 'fake_remote_file_not_exists', 'local') + def test_conn_refused(self): + with pytest.raises(ConnectionRefusedError): + SSHClient('127.0.0.99', port=self.port, num_retries=1, timeout=1) + @patch('pssh.clients.base.single.socket') def test_ipv6(self, gsocket): # As of Oct 2021, CircleCI does not support IPv6 in its containers. @@ -102,18 +108,41 @@ def test_ipv6(self, gsocket): _sock = MagicMock() gsocket.socket.return_value = _sock sock_con = MagicMock() + sock_con.side_effect = ConnectionRefusedError _sock.connect = sock_con getaddrinfo = MagicMock() gsocket.getaddrinfo = getaddrinfo getaddrinfo.return_value = [( socket.AF_INET6, socket.SocketKind.SOCK_STREAM, socket.IPPROTO_TCP, '', addr_info)] - with raises(TypeError): - # Mock object as a file descriptor will raise TypeError + with raises(ConnectionError): client = SSHClient(host, port=self.port, pkey=self.user_key, num_retries=1) getaddrinfo.assert_called_once_with(host, self.port, proto=socket.IPPROTO_TCP) sock_con.assert_called_once_with(addr_info) + @patch('pssh.clients.base.single.socket') + def test_multiple_available_addr(self, gsocket): + host = '127.0.0.1' + addr_info = (host, self.port) + gsocket.IPPROTO_TCP = socket.IPPROTO_TCP + gsocket.socket = MagicMock() + _sock = MagicMock() + gsocket.socket.return_value = _sock + sock_con = MagicMock() + sock_con.side_effect = ConnectionRefusedError + _sock.connect = sock_con + getaddrinfo = MagicMock() + gsocket.getaddrinfo = getaddrinfo + getaddrinfo.return_value = [ + (socket.AF_INET, socket.SocketKind.SOCK_STREAM, socket.IPPROTO_TCP, '', addr_info), + (socket.AF_INET, socket.SocketKind.SOCK_STREAM, socket.IPPROTO_TCP, '', addr_info), + ] + with raises(ConnectionError): + client = SSHClient(host, port=self.port, pkey=self.user_key, + num_retries=1) + getaddrinfo.assert_called_with(host, self.port, proto=socket.IPPROTO_TCP) + assert sock_con.call_count == len(getaddrinfo.return_value) + def test_no_ipv6(self): try: SSHClient(self.host, @@ -357,7 +386,7 @@ def test_password_auth_failure(self): raise AssertionError def test_retry_failure(self): - self.assertRaises(ConnectionErrorException, + self.assertRaises(ConnectionError, SSHClient, self.host, port=12345, num_retries=2, _auth_thread_pool=False, retry_delay=.1, diff --git a/tests/ssh/test_parallel_client.py b/tests/ssh/test_parallel_client.py index 0123be7e..3a9305e3 100644 --- a/tests/ssh/test_parallel_client.py +++ b/tests/ssh/test_parallel_client.py @@ -238,14 +238,14 @@ def test_pssh_client_hosts_list_part_failure(self): self.assertIsNotNone(output[1].exception, msg="Failed host %s has no exception in output - %s" % (hosts[1], output,)) self.assertTrue(output[1].exception is not None) - self.assertEqual(output[1].exception.args[1], hosts[1]) + self.assertEqual(output[1].host, hosts[1]) + self.assertEqual(output[1].exception.args[-2], hosts[1]) try: raise output[1].exception except ConnectionErrorException: pass else: - raise Exception("Expected ConnectionError, got %s instead" % ( - output[1].exception,)) + raise Exception("Expected ConnectionError, got %s instead" % (output[1].exception,)) def test_pssh_client_timeout(self): # 1ms timeout @@ -316,14 +316,13 @@ def test_connection_error_exception(self): num_retries=1) output = client.run_command(self.cmd, stop_on_errors=False) client.join(output) - self.assertIsNotNone(output[0].exception, - msg="Got no exception for host %s - expected connection error" % ( - host,)) + self.assertIsInstance(output[0].exception, ConnectionErrorException) + self.assertEqual(output[0].host, host) try: raise output[0].exception except ConnectionErrorException as ex: - self.assertEqual(ex.args[1], host) - self.assertEqual(ex.args[2], port) + self.assertEqual(ex.args[-2], host) + self.assertEqual(ex.args[-1], port) else: raise Exception("Expected ConnectionErrorException") diff --git a/tests/ssh/test_single_client.py b/tests/ssh/test_single_client.py index f37c5cb8..27738a4b 100644 --- a/tests/ssh/test_single_client.py +++ b/tests/ssh/test_single_client.py @@ -21,7 +21,7 @@ from datetime import datetime from gevent import sleep, Timeout as GTimeout, spawn -from ssh.session import Session +# from ssh.session import Session from ssh.exceptions import AuthenticationDenied from pssh.exceptions import AuthenticationException, ConnectionErrorException, \ SessionError, SFTPIOError, SFTPError, SCPError, PKeyFileError, Timeout, \