From 62e1f6a29bbd3c924d4cc0fda83ffda9f2d66f88 Mon Sep 17 00:00:00 2001 From: Panos Date: Fri, 10 Dec 2021 23:48:57 +0000 Subject: [PATCH 1/7] Handle conn refused --- pssh/clients/base/single.py | 13 ++++++++++++- tests/native/test_single_client.py | 6 ++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/pssh/clients/base/single.py b/pssh/clients/base/single.py index 33b6d2dd..7cb7be9a 100644 --- a/pssh/clients/base/single.py +++ b/pssh/clients/base/single.py @@ -284,13 +284,24 @@ 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: + self._connect_socket(family, _type, proto, sock_addr, host, port, retries) + except ConnectionRefusedError: + if i+1 == len(addr_info): + logger.error("No available addresses from %s", addr_info) + 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/tests/native/test_single_client.py b/tests/native/test_single_client.py index 0893b52e..82f22881 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 @@ -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. From 67109587b1a507347f35345640576c5f955d8920 Mon Sep 17 00:00:00 2001 From: Panos Date: Sat, 11 Dec 2021 11:41:49 +0000 Subject: [PATCH 2/7] Updated tests, changelog --- Changelog.rst | 12 ++++++++++ pssh/clients/base/single.py | 9 +++---- pssh/exceptions.py | 8 +------ tests/native/test_parallel_client.py | 36 +++++++++++++--------------- tests/native/test_single_client.py | 4 ++-- 5 files changed, 36 insertions(+), 33 deletions(-) diff --git a/Changelog.rst b/Changelog.rst index ee455c3f..7058943b 100644 --- a/Changelog.rst +++ b/Changelog.rst @@ -1,6 +1,18 @@ Change Log ============ +2.9.0 ++++++ + +Changes +-------- + +* ``pssh.exceptions.ConnectionError`` is now the same as built-in ``ConnectionError`` and deprecated - to be removed. +* Clients now attempt to continue connecting with all addresses in DNS list for host in the case where an address + refuses connection. 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 7cb7be9a..535e0c6b 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 @@ -287,9 +287,10 @@ def _connect(self, host, port, retries=1): for i, (family, _type, proto, _, sock_addr) in enumerate(addr_info): try: self._connect_socket(family, _type, proto, sock_addr, host, port, retries) - except ConnectionRefusedError: + except ConnectionRefusedError as ex: if i+1 == len(addr_info): - logger.error("No available addresses from %s", addr_info) + logger.error("No available addresses from %s", [addr[4] for addr in addr_info]) + ex.args += (host, port) raise continue 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 82f22881..27c6aa9b 100644 --- a/tests/native/test_single_client.py +++ b/tests/native/test_single_client.py @@ -38,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 @@ -363,7 +363,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, From ff636df999883b657d94fbe453a00f22495bea4e Mon Sep 17 00:00:00 2001 From: Panos Date: Sat, 11 Dec 2021 11:48:26 +0000 Subject: [PATCH 3/7] Updated tests --- tests/ssh/test_parallel_client.py | 15 +++++++-------- tests/ssh/test_single_client.py | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) 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, \ From 45b3173f74a12026132d15a5b3b0c48edc960947 Mon Sep 17 00:00:00 2001 From: Panos Date: Sat, 11 Dec 2021 12:08:10 +0000 Subject: [PATCH 4/7] WIP --- Changelog.rst | 5 +++-- pssh/clients/base/single.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Changelog.rst b/Changelog.rst index 7058943b..b94806e5 100644 --- a/Changelog.rst +++ b/Changelog.rst @@ -8,8 +8,9 @@ Changes -------- * ``pssh.exceptions.ConnectionError`` is now the same as built-in ``ConnectionError`` and deprecated - to be removed. -* Clients now attempt to continue connecting with all addresses in DNS list for host in the case where an address - refuses connection. For example where a host resolves to both IPv4 and v6 addresses while only one address is +* 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. diff --git a/pssh/clients/base/single.py b/pssh/clients/base/single.py index 535e0c6b..343c1f99 100644 --- a/pssh/clients/base/single.py +++ b/pssh/clients/base/single.py @@ -286,7 +286,7 @@ def _connect(self, host, port, retries=1): raise unknown_ex from ex for i, (family, _type, proto, _, sock_addr) in enumerate(addr_info): try: - self._connect_socket(family, _type, proto, sock_addr, host, port, retries) + 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]) From 4785bce8cd9de422803fa6ce063cab6b85cba64a Mon Sep 17 00:00:00 2001 From: Panos Date: Sat, 11 Dec 2021 20:07:17 +0000 Subject: [PATCH 5/7] Changelog --- Changelog.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/Changelog.rst b/Changelog.rst index b94806e5..1ee5169e 100644 --- a/Changelog.rst +++ b/Changelog.rst @@ -14,6 +14,7 @@ Changes 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 +++++ From d4f646e4e74710e06a6f8438b679a5de08c0808e Mon Sep 17 00:00:00 2001 From: Panos Date: Sun, 20 Mar 2022 13:42:21 +0000 Subject: [PATCH 6/7] Changelog --- Changelog.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/Changelog.rst b/Changelog.rst index 1ee5169e..1adf1f09 100644 --- a/Changelog.rst +++ b/Changelog.rst @@ -10,6 +10,7 @@ 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. From 85ec56cab6e0baf5bf00c49907a72121fcd1453d Mon Sep 17 00:00:00 2001 From: Panos Date: Sun, 20 Mar 2022 14:29:12 +0000 Subject: [PATCH 7/7] Added unit test for attempting to connect to multiple available addresses from DNS resolution. --- tests/native/test_single_client.py | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/tests/native/test_single_client.py b/tests/native/test_single_client.py index 27c6aa9b..5aa3bd6f 100644 --- a/tests/native/test_single_client.py +++ b/tests/native/test_single_client.py @@ -108,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,