From c4f7a2564814b034e3f45464528c7818e5edcf3d Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Tue, 14 Apr 2020 12:51:25 +0200 Subject: [PATCH 01/27] add support for a configurable ssh connect timeout --- st2common/st2common/config.py | 5 +++- st2common/st2common/runners/paramiko_ssh.py | 30 ++++++++++++--------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/st2common/st2common/config.py b/st2common/st2common/config.py index e0cbb92278..18da4c594b 100644 --- a/st2common/st2common/config.py +++ b/st2common/st2common/config.py @@ -421,7 +421,10 @@ def register_opts(ignore_errors=False): help='Use the .ssh/config file. Useful to override ports etc.'), cfg.StrOpt( 'ssh_config_file_path', default='~/.ssh/config', - help='Path to the ssh config file.') + help='Path to the ssh config file.'), + cfg.IntOpt( + 'ssh_connect_timeout', default=60, + help='Max time in seconds to establish the SSH connection.') ] do_register_opts(ssh_runner_opts, group='ssh_runner') diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index 2e5154db9f..377320f5eb 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -49,7 +49,7 @@ class SSHCommandTimeoutError(Exception): Exception which is raised when an SSH command times out. """ - def __init__(self, cmd, timeout, stdout=None, stderr=None): + def __init__(self, cmd, timeout, ssh_connect_timeout, stdout=None, stderr=None): """ :param stdout: Stdout which was consumed until the timeout occured. :type stdout: ``str`` @@ -59,14 +59,16 @@ def __init__(self, cmd, timeout, stdout=None, stderr=None): """ self.cmd = cmd self.timeout = timeout + self.ssh_connect_timeout = ssh_connect_timeout self.stdout = stdout self.stderr = stderr - self.message = 'Command didn\'t finish in %s seconds' % (timeout) + self.message = 'Command didn\'t finish in %s seconds or the SSH connection did not succeed in %s seconds' % \ + (timeout, ssh_connect_timeout) super(SSHCommandTimeoutError, self).__init__(self.message) def __repr__(self): - return ('' % - (self.cmd, self.timeout)) + return ('' % + (self.cmd, self.timeout, self.ssh_connect_timeout)) def __str__(self): return self.message @@ -83,9 +85,6 @@ class ParamikoSSHClient(object): # How long to sleep while waiting for command to finish to prevent busy waiting SLEEP_DELAY = 0.2 - # Connect socket timeout - CONNECT_TIMEOUT = 60 - def __init__(self, hostname, port=DEFAULT_SSH_PORT, username=None, password=None, bastion_host=None, key_files=None, key_material=None, timeout=None, passphrase=None, handle_stdout_line_func=None, handle_stderr_line_func=None): @@ -105,10 +104,11 @@ def __init__(self, hostname, port=DEFAULT_SSH_PORT, username=None, password=None self.username = username self.password = password self.key_files = key_files - self.timeout = timeout or ParamikoSSHClient.CONNECT_TIMEOUT + self.timeout = timeout self.key_material = key_material self.bastion_host = bastion_host self.passphrase = passphrase + self.ssh_connect_timeout = cfg.CONF.ssh_runner.ssh_connect_timeout self._handle_stdout_line_func = handle_stdout_line_func self._handle_stderr_line_func = handle_stderr_line_func @@ -116,6 +116,11 @@ def __init__(self, hostname, port=DEFAULT_SSH_PORT, username=None, password=None cfg.CONF.ssh_runner.ssh_config_file_path or '~/.ssh/config' ) + + if self.timeout and self.ssh_connect_timeout > self.timeout - 2: + # the connect timeout should not be greater than the action timeout + self.ssh_connect_timeout = self.timeout - 2 + self.logger = logging.getLogger(__name__) self.client = None @@ -415,8 +420,9 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): stdout = sanitize_output(stdout.getvalue(), uses_pty=uses_pty) stderr = sanitize_output(stderr.getvalue(), uses_pty=uses_pty) - raise SSHCommandTimeoutError(cmd=cmd, timeout=timeout, stdout=stdout, - stderr=stderr) + raise SSHCommandTimeoutError(cmd=cmd, timeout=timeout, + ssh_connect_timeout=self.ssh_connect_timeout, + stdout=stdout, stderr=stderr) stdout_data = self._consume_stdout(chan=chan, call_line_handler_func=call_line_handler_func) @@ -633,7 +639,7 @@ def _connect(self, host, socket=None): conninfo = {'hostname': host, 'allow_agent': False, 'look_for_keys': False, - 'timeout': self.timeout} + 'timeout': self.ssh_connect_timeout} ssh_config_file_info = {} if cfg.CONF.ssh_runner.use_ssh_config: @@ -702,7 +708,7 @@ def _connect(self, host, socket=None): conninfo['look_for_keys'] = True extra = {'_hostname': host, '_port': self.port, - '_username': self.username, '_timeout': self.timeout} + '_username': self.username, '_timeout': self.ssh_connect_timeout} self.logger.debug('Connecting to server', extra=extra) self.socket = socket or ssh_config_file_info.get('sock', None) From d314b1b4e5904b989cafe327492349042baf28fd Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 19 Apr 2020 00:38:39 +0200 Subject: [PATCH 02/27] add changelog entry adding the support of ssh_connect_timout --- CHANGELOG.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b377f33597..5c83c91d42 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -6,7 +6,9 @@ in development Added ~~~~~ - +* Add support for a configurable connect timeout for SSH connections as requested in #4715 + by adding the new configuration parameter ``ssh_connect_timeout`` to the ``ssh_runner`` + group in st2.conf. * Add support for blacklisting / whitelisting hosts to the HTTP runner by adding new ``url_hosts_blacklist`` and ``url_hosts_whitelist`` runner attribute. (new feature) #4757 From c5bee89a44ac3201cbf5ef816cb5602e3a8f2fb0 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Wed, 3 Jun 2020 20:23:42 +0200 Subject: [PATCH 03/27] Fix failure when substracting int from string (timeout) --- st2common/st2common/runners/paramiko_ssh.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index 377320f5eb..8a4c4c4869 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -104,11 +104,11 @@ def __init__(self, hostname, port=DEFAULT_SSH_PORT, username=None, password=None self.username = username self.password = password self.key_files = key_files - self.timeout = timeout + self.timeout = int(timeout) self.key_material = key_material self.bastion_host = bastion_host self.passphrase = passphrase - self.ssh_connect_timeout = cfg.CONF.ssh_runner.ssh_connect_timeout + self.ssh_connect_timeout = int(cfg.CONF.ssh_runner.ssh_connect_timeout) self._handle_stdout_line_func = handle_stdout_line_func self._handle_stderr_line_func = handle_stderr_line_func From c5906fb12decc75cd96afd9d67fc7c1598fca3db Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Wed, 3 Jun 2020 21:03:32 +0200 Subject: [PATCH 04/27] Convert timeout and ssh_connect_timout only if needed --- st2common/st2common/runners/paramiko_ssh.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index 8a4c4c4869..9767cb9c3c 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -104,11 +104,11 @@ def __init__(self, hostname, port=DEFAULT_SSH_PORT, username=None, password=None self.username = username self.password = password self.key_files = key_files - self.timeout = int(timeout) + self.timeout = timeout self.key_material = key_material self.bastion_host = bastion_host self.passphrase = passphrase - self.ssh_connect_timeout = int(cfg.CONF.ssh_runner.ssh_connect_timeout) + self.ssh_connect_timeout = cfg.CONF.ssh_runner.ssh_connect_timeout self._handle_stdout_line_func = handle_stdout_line_func self._handle_stderr_line_func = handle_stderr_line_func @@ -117,9 +117,9 @@ def __init__(self, hostname, port=DEFAULT_SSH_PORT, username=None, password=None '~/.ssh/config' ) - if self.timeout and self.ssh_connect_timeout > self.timeout - 2: + if self.timeout and int(self.ssh_connect_timeout) > int(self.timeout) - 2: # the connect timeout should not be greater than the action timeout - self.ssh_connect_timeout = self.timeout - 2 + self.ssh_connect_timeout = int(self.timeout) - 2 self.logger = logging.getLogger(__name__) From 209f0764e6ce7130dee293160c240f9e25ff3647 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Wed, 3 Jun 2020 21:33:51 +0200 Subject: [PATCH 05/27] Add ssh_connect_timeout to the unit test ssh_connect_timeout --- st2actions/tests/unit/test_parallel_ssh.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/st2actions/tests/unit/test_parallel_ssh.py b/st2actions/tests/unit/test_parallel_ssh.py index 89fb4e75bd..24faa849d6 100644 --- a/st2actions/tests/unit/test_parallel_ssh.py +++ b/st2actions/tests/unit/test_parallel_ssh.py @@ -152,7 +152,8 @@ def test_run_command_timeout(self): connect=True) mock_run = Mock(side_effect=SSHCommandTimeoutError(cmd='pwd', timeout=10, stdout='a', - stderr='b')) + stderr='b', + ssh_connect_timeout=30)) for host in hosts: hostname, _ = client._get_host_port_info(host) host_client = client._hosts_client[host] From eff1720b306768aba2fa42cb65eded1f226b0776 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 6 Jun 2020 23:25:52 +0200 Subject: [PATCH 06/27] Updathe paramiko_ssh tests to be aligned with the new connection timeout default of 60s --- st2actions/tests/unit/test_paramiko_ssh.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index c6da6e3058..6b27817853 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -48,7 +48,7 @@ def setUp(self): 'port': 8822, 'username': 'ubuntu', 'key_files': '~/.ssh/ubuntu_ssh', - 'timeout': '600'} + 'timeout': '60'} self.ssh_cli = ParamikoSSHClient(**conn_params) @patch('paramiko.SSHClient', Mock) @@ -446,7 +446,7 @@ def test_basic_usage_absolute_path(self): 'allow_agent': False, 'hostname': 'dummy.host.org', 'look_for_keys': False, - 'timeout': '600', + 'timeout': '60', 'port': 8822} mock_cli.connect.assert_called_once_with(**expected_conn) From 0b6c0c0c3875d15629b813e4369e60c619b74600 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 7 Jun 2020 00:23:07 +0200 Subject: [PATCH 07/27] reduce the connect timeout to 30 seconds to be lower than the default of 60 seconds --- st2actions/tests/unit/test_parallel_ssh.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2actions/tests/unit/test_parallel_ssh.py b/st2actions/tests/unit/test_parallel_ssh.py index 24faa849d6..43c7b5f83c 100644 --- a/st2actions/tests/unit/test_parallel_ssh.py +++ b/st2actions/tests/unit/test_parallel_ssh.py @@ -50,7 +50,7 @@ def test_connect_with_password(self): 'look_for_keys': False, 'password': 'ubuntu', 'username': 'ubuntu', - 'timeout': 60, + 'timeout': 30, 'port': 22 } for host in hosts: @@ -72,7 +72,7 @@ def test_connect_with_random_ports(self): 'look_for_keys': False, 'password': 'ubuntu', 'username': 'ubuntu', - 'timeout': 60, + 'timeout': 30, 'port': 22 } for host in hosts: From 4cb122dc4c69dec65940be58e4160da9e527aa65 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 7 Jun 2020 01:31:44 +0200 Subject: [PATCH 08/27] Add timeout to the ParallelSSHCLient parameters --- st2actions/tests/unit/test_parallel_ssh.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/st2actions/tests/unit/test_parallel_ssh.py b/st2actions/tests/unit/test_parallel_ssh.py index 43c7b5f83c..c72edfd81a 100644 --- a/st2actions/tests/unit/test_parallel_ssh.py +++ b/st2actions/tests/unit/test_parallel_ssh.py @@ -22,6 +22,7 @@ from st2common.runners.parallel_ssh import ParallelSSHClient from st2common.runners.paramiko_ssh import ParamikoSSHClient from st2common.runners.paramiko_ssh import SSHCommandTimeoutError + import st2tests.config as tests_config tests_config.parse_args() @@ -43,7 +44,8 @@ def test_connect_with_password(self): client = ParallelSSHClient(hosts=hosts, user='ubuntu', password='ubuntu', - connect=False) + connect=False, + timeout=30) client.connect() expected_conn = { 'allow_agent': False, @@ -65,7 +67,8 @@ def test_connect_with_random_ports(self): client = ParallelSSHClient(hosts=hosts, user='ubuntu', password='ubuntu', - connect=False) + connect=False, + timeout=30) client.connect() expected_conn = { 'allow_agent': False, From c8ce0dd22917cec7c346a8bd003882044a665326 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 7 Jun 2020 01:52:45 +0200 Subject: [PATCH 09/27] Revert "Add timeout to the ParallelSSHCLient parameters" This reverts commit 4cb122dc4c69dec65940be58e4160da9e527aa65 which passed the timeout parameter to the wrong method --- st2actions/tests/unit/test_parallel_ssh.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/st2actions/tests/unit/test_parallel_ssh.py b/st2actions/tests/unit/test_parallel_ssh.py index c72edfd81a..43c7b5f83c 100644 --- a/st2actions/tests/unit/test_parallel_ssh.py +++ b/st2actions/tests/unit/test_parallel_ssh.py @@ -22,7 +22,6 @@ from st2common.runners.parallel_ssh import ParallelSSHClient from st2common.runners.paramiko_ssh import ParamikoSSHClient from st2common.runners.paramiko_ssh import SSHCommandTimeoutError - import st2tests.config as tests_config tests_config.parse_args() @@ -44,8 +43,7 @@ def test_connect_with_password(self): client = ParallelSSHClient(hosts=hosts, user='ubuntu', password='ubuntu', - connect=False, - timeout=30) + connect=False) client.connect() expected_conn = { 'allow_agent': False, @@ -67,8 +65,7 @@ def test_connect_with_random_ports(self): client = ParallelSSHClient(hosts=hosts, user='ubuntu', password='ubuntu', - connect=False, - timeout=30) + connect=False) client.connect() expected_conn = { 'allow_agent': False, From ff289afd350069b1f1505e5a5b845db4bf9fd220 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 7 Jun 2020 02:12:20 +0200 Subject: [PATCH 10/27] Add override ssh_connect_timeout = 30 for the ParamiSSHClientTestCase --- st2actions/tests/unit/test_paramiko_ssh.py | 1 + 1 file changed, 1 insertion(+) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index 6b27817853..81cc4d50dc 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -43,6 +43,7 @@ def setUp(self): """ cfg.CONF.set_override(name='ssh_key_file', override=None, group='system_user') cfg.CONF.set_override(name='use_ssh_config', override=False, group='ssh_runner') + cfg.CONF.set_override(name='ssh_connect_timeout', override='30', group='ssh_runnter') conn_params = {'hostname': 'dummy.host.org', 'port': 8822, From c82f19359a50d64bc548d011543d4fd28904818d Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 7 Jun 2020 02:26:49 +0200 Subject: [PATCH 11/27] set the parallelSSHTests timeout to 60 seconds --- st2actions/tests/unit/test_parallel_ssh.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2actions/tests/unit/test_parallel_ssh.py b/st2actions/tests/unit/test_parallel_ssh.py index 43c7b5f83c..24faa849d6 100644 --- a/st2actions/tests/unit/test_parallel_ssh.py +++ b/st2actions/tests/unit/test_parallel_ssh.py @@ -50,7 +50,7 @@ def test_connect_with_password(self): 'look_for_keys': False, 'password': 'ubuntu', 'username': 'ubuntu', - 'timeout': 30, + 'timeout': 60, 'port': 22 } for host in hosts: @@ -72,7 +72,7 @@ def test_connect_with_random_ports(self): 'look_for_keys': False, 'password': 'ubuntu', 'username': 'ubuntu', - 'timeout': 30, + 'timeout': 60, 'port': 22 } for host in hosts: From 658e8b5a6887be4c4ec11940b54c4356ca3eaffb Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 7 Jun 2020 02:29:11 +0200 Subject: [PATCH 12/27] fix typo ssh_runnter -> ssh_runnter --- st2actions/tests/unit/test_paramiko_ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index 81cc4d50dc..883e4905d2 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -43,7 +43,7 @@ def setUp(self): """ cfg.CONF.set_override(name='ssh_key_file', override=None, group='system_user') cfg.CONF.set_override(name='use_ssh_config', override=False, group='ssh_runner') - cfg.CONF.set_override(name='ssh_connect_timeout', override='30', group='ssh_runnter') + cfg.CONF.set_override(name='ssh_connect_timeout', override='30', group='ssh_runner') conn_params = {'hostname': 'dummy.host.org', 'port': 8822, From 03608dd8ee9ec85ec0d6d1e71d4f7c6a94d012ae Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 7 Jun 2020 02:38:04 +0200 Subject: [PATCH 13/27] set timeouts on all expected_conn objects to 30s to match the overridden ssh_connect_timeout --- st2actions/tests/unit/test_paramiko_ssh.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index 883e4905d2..57692b4218 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -49,7 +49,7 @@ def setUp(self): 'port': 8822, 'username': 'ubuntu', 'key_files': '~/.ssh/ubuntu_ssh', - 'timeout': '60'} + 'timeout': '30'} self.ssh_cli = ParamikoSSHClient(**conn_params) @patch('paramiko.SSHClient', Mock) @@ -109,7 +109,7 @@ def test_create_with_password(self): 'allow_agent': False, 'hostname': 'dummy.host.org', 'look_for_keys': False, - 'timeout': 60, + 'timeout': 30, 'port': 22} mock.client.connect.assert_called_once_with(**expected_conn) @@ -128,7 +128,7 @@ def test_deprecated_key_argument(self): 'hostname': 'dummy.host.org', 'look_for_keys': False, 'key_filename': 'id_rsa', - 'timeout': 60, + 'timeout': 30, 'port': 22} mock.client.connect.assert_called_once_with(**expected_conn) @@ -168,7 +168,7 @@ def test_key_material_argument(self): 'hostname': 'dummy.host.org', 'look_for_keys': False, 'pkey': pkey, - 'timeout': 60, + 'timeout': 30, 'port': 22} mock.client.connect.assert_called_once_with(**expected_conn) @@ -232,7 +232,7 @@ def test_key_with_passphrase_success(self): 'hostname': 'dummy.host.org', 'look_for_keys': False, 'pkey': pkey, - 'timeout': 60, + 'timeout': 30, 'port': 22} mock.client.connect.assert_called_once_with(**expected_conn) @@ -326,7 +326,7 @@ def test_create_with_key(self): 'hostname': 'dummy.host.org', 'look_for_keys': False, 'key_filename': 'id_rsa', - 'timeout': 60, + 'timeout': 30, 'port': 22} mock.client.connect.assert_called_once_with(**expected_conn) @@ -355,7 +355,7 @@ def test_create_with_key_via_bastion(self): 'hostname': 'dummy.host.org', 'look_for_keys': False, 'key_filename': 'id_rsa', - 'timeout': 60, + 'timeout': 30, 'port': 22, 'sock': mock.bastion_socket} mock.client.connect.assert_called_once_with(**expected_conn) @@ -377,7 +377,7 @@ def test_create_with_password_and_key(self): 'hostname': 'dummy.host.org', 'look_for_keys': False, 'key_filename': 'id_rsa', - 'timeout': 60, + 'timeout': 30, 'port': 22} mock.client.connect.assert_called_once_with(**expected_conn) @@ -418,7 +418,7 @@ def test_create_without_credentials_use_default_key(self): 'key_filename': 'stanley_rsa', 'allow_agent': False, 'look_for_keys': False, - 'timeout': 60, + 'timeout': 30, 'port': 22} mock.client.connect.assert_called_once_with(**expected_conn) From 6e1d784181219118645b3a6ae97a3f4340195ed9 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 7 Jun 2020 02:49:24 +0200 Subject: [PATCH 14/27] override the ssh_connect_timeout with an int, not a string --- st2actions/tests/unit/test_paramiko_ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index 57692b4218..67722f67b4 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -43,7 +43,7 @@ def setUp(self): """ cfg.CONF.set_override(name='ssh_key_file', override=None, group='system_user') cfg.CONF.set_override(name='use_ssh_config', override=False, group='ssh_runner') - cfg.CONF.set_override(name='ssh_connect_timeout', override='30', group='ssh_runner') + cfg.CONF.set_override(name='ssh_connect_timeout', override=30, group='ssh_runner') conn_params = {'hostname': 'dummy.host.org', 'port': 8822, From fb4e13d9842e31adc62e1da4b70256fa826326ea Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 00:39:18 +0200 Subject: [PATCH 15/27] fix remaining test cases that were still expecting the old threshold --- st2actions/tests/unit/test_paramiko_ssh.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index 67722f67b4..cec6f679da 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -49,7 +49,7 @@ def setUp(self): 'port': 8822, 'username': 'ubuntu', 'key_files': '~/.ssh/ubuntu_ssh', - 'timeout': '30'} + 'timeout': 28} self.ssh_cli = ParamikoSSHClient(**conn_params) @patch('paramiko.SSHClient', Mock) @@ -346,7 +346,7 @@ def test_create_with_key_via_bastion(self): 'hostname': 'bastion.host.org', 'look_for_keys': False, 'key_filename': 'id_rsa', - 'timeout': 60, + 'timeout': 30, 'port': 22} mock.bastion_client.connect.assert_called_once_with(**expected_bastion_conn) From 0ce8c06d394184ac32718a95cdbf66e90f970436 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 00:40:59 +0200 Subject: [PATCH 16/27] move the changelog entry into the correct section and add credits for the requester and contributor --- CHANGELOG.rst | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ef8446ecb0..504b34c6db 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -3,6 +3,12 @@ Changelog in development -------------- +Added +~~~~~ +* Add support for a configurable connect timeout for SSH connections as requested in #4715 + by adding the new configuration parameter ``ssh_connect_timeout`` to the ``ssh_runner`` + group in st2.conf. + This option was requested by Harry Lee (@tclh123) and contributed by Marcel Weinberg (@winem). Fixed ~~~~~ @@ -22,9 +28,6 @@ Fixed Added ~~~~~ -* Add support for a configurable connect timeout for SSH connections as requested in #4715 - by adding the new configuration parameter ``ssh_connect_timeout`` to the ``ssh_runner`` - group in st2.conf. * Add support for blacklisting / whitelisting hosts to the HTTP runner by adding new ``url_hosts_blacklist`` and ``url_hosts_whitelist`` runner attribute. (new feature) #4757 From ecaf77942ead1bb21e4f941f64a922a9150450a4 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 01:29:29 +0200 Subject: [PATCH 17/27] add test to see the behaviour when connecting to a random port via ssh --- st2common/tests/unit/test_paramiko_command_action_model.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/st2common/tests/unit/test_paramiko_command_action_model.py b/st2common/tests/unit/test_paramiko_command_action_model.py index 5cc0499d8c..5089bae656 100644 --- a/st2common/tests/unit/test_paramiko_command_action_model.py +++ b/st2common/tests/unit/test_paramiko_command_action_model.py @@ -104,6 +104,13 @@ def test_get_command_string_no_user_env_vars(self): ex = 'export FOO=BAR && cd /tmp && echo boo bah baz' self.assertEqual(cmd_action.get_full_command_string(), ex) + def test_get_command_error_after_ssh_connect_timeout(self): + cmd_action = ParamikoRemoteCommandActionTestCase._get_test_command_action( + 'echo foo bar') + cmd_action.port = 22222 + ex = 'cd /tmp && echo foo bar' + self.assertEqual(cmd_action.get_full_command_string(), ex) + @staticmethod def _get_test_command_action(command): cmd_action = ParamikoRemoteCommandAction('fixtures.remote_command', From b6b9ed481a0acbc1170738ec45e396a62c28aa76 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 01:41:31 +0200 Subject: [PATCH 18/27] fix test_key_with_passphrase_success test with new timeout --- st2actions/tests/unit/test_paramiko_ssh.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index cec6f679da..a5c295ef04 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -49,7 +49,7 @@ def setUp(self): 'port': 8822, 'username': 'ubuntu', 'key_files': '~/.ssh/ubuntu_ssh', - 'timeout': 28} + 'timeout': 30} self.ssh_cli = ParamikoSSHClient(**conn_params) @patch('paramiko.SSHClient', Mock) @@ -250,7 +250,7 @@ def test_key_with_passphrase_success(self): 'look_for_keys': False, 'key_filename': path, 'password': 'testphrase', - 'timeout': 60, + 'timeout': 30, 'port': 22} mock.client.connect.assert_called_once_with(**expected_conn) From a30dea7e3c091f7c8e72204d1c1a2becc45092b7 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 01:49:40 +0200 Subject: [PATCH 19/27] fix basic setup test in test_paramiko_ssh --- st2actions/tests/unit/test_paramiko_ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index a5c295ef04..22f3d5886e 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -447,7 +447,7 @@ def test_basic_usage_absolute_path(self): 'allow_agent': False, 'hostname': 'dummy.host.org', 'look_for_keys': False, - 'timeout': '60', + 'timeout': 30, 'port': 8822} mock_cli.connect.assert_called_once_with(**expected_conn) From 644ee49f45254d3a6c37ddeaacc946e0384c5267 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 01:49:59 +0200 Subject: [PATCH 20/27] add newly generated st2.conf.sample as suggested by the lint checker --- conf/st2.conf.sample | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/conf/st2.conf.sample b/conf/st2.conf.sample index bd99093c24..32c3e60148 100644 --- a/conf/st2.conf.sample +++ b/conf/st2.conf.sample @@ -299,14 +299,16 @@ logging = /etc/st2/logging.sensorcontainer.conf sensor_node_name = sensornode1 [ssh_runner] +# Path to the ssh config file. +ssh_config_file_path = ~/.ssh/config # Max number of parallel remote SSH actions that should be run. Works only with Paramiko SSH runner. max_parallel_actions = 50 +# Max time in seconds to establish the SSH connection. +ssh_connect_timeout = 60 # Location of the script on the remote filesystem. remote_dir = /tmp # Use the .ssh/config file. Useful to override ports etc. use_ssh_config = False -# Path to the ssh config file. -ssh_config_file_path = ~/.ssh/config # How partial success of actions run on multiple nodes should be treated. allow_partial_failure = False From ccca5e28c5f9ecd8d41ba0175596939be4252782 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 01:52:44 +0200 Subject: [PATCH 21/27] Revert "add test to see the behaviour when connecting to a random port via ssh" This reverts commit ecaf77942ead1bb21e4f941f64a922a9150450a4. --- st2common/tests/unit/test_paramiko_command_action_model.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/st2common/tests/unit/test_paramiko_command_action_model.py b/st2common/tests/unit/test_paramiko_command_action_model.py index 5089bae656..5cc0499d8c 100644 --- a/st2common/tests/unit/test_paramiko_command_action_model.py +++ b/st2common/tests/unit/test_paramiko_command_action_model.py @@ -104,13 +104,6 @@ def test_get_command_string_no_user_env_vars(self): ex = 'export FOO=BAR && cd /tmp && echo boo bah baz' self.assertEqual(cmd_action.get_full_command_string(), ex) - def test_get_command_error_after_ssh_connect_timeout(self): - cmd_action = ParamikoRemoteCommandActionTestCase._get_test_command_action( - 'echo foo bar') - cmd_action.port = 22222 - ex = 'cd /tmp && echo foo bar' - self.assertEqual(cmd_action.get_full_command_string(), ex) - @staticmethod def _get_test_command_action(command): cmd_action = ParamikoRemoteCommandAction('fixtures.remote_command', From d295df97472888c238ec568d6b43d769425a0d4b Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 02:05:11 +0200 Subject: [PATCH 22/27] fix basic setup test in test_paramiko_ssh for real this time --- st2actions/tests/unit/test_paramiko_ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index 22f3d5886e..841b23b04b 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -447,7 +447,7 @@ def test_basic_usage_absolute_path(self): 'allow_agent': False, 'hostname': 'dummy.host.org', 'look_for_keys': False, - 'timeout': 30, + 'timeout': 28, 'port': 8822} mock_cli.connect.assert_called_once_with(**expected_conn) From 7c00fea3253841033b330dec171e3197652a5b85 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 21:23:26 +0200 Subject: [PATCH 23/27] fix lint issues --- st2common/st2common/runners/paramiko_ssh.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index 9767cb9c3c..02da51e7e6 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -62,8 +62,8 @@ def __init__(self, cmd, timeout, ssh_connect_timeout, stdout=None, stderr=None): self.ssh_connect_timeout = ssh_connect_timeout self.stdout = stdout self.stderr = stderr - self.message = 'Command didn\'t finish in %s seconds or the SSH connection did not succeed in %s seconds' % \ - (timeout, ssh_connect_timeout) + self.message = ('Command didn\'t finish in %s seconds or the SSH connection ' + + 'did not succeed in %s seconds' % (timeout, ssh_connect_timeout)) super(SSHCommandTimeoutError, self).__init__(self.message) def __repr__(self): @@ -420,7 +420,7 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): stdout = sanitize_output(stdout.getvalue(), uses_pty=uses_pty) stderr = sanitize_output(stderr.getvalue(), uses_pty=uses_pty) - raise SSHCommandTimeoutError(cmd=cmd, timeout=timeout, + raise SSHCommandTimeoutError(cmd=cmd, timeout=timeout, ssh_connect_timeout=self.ssh_connect_timeout, stdout=stdout, stderr=stderr) From 869e99a97bfcfb8fe02b52fd1d5ce6bda787c503 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 20 Jun 2020 21:41:49 +0200 Subject: [PATCH 24/27] fix string formatting issue to replace all variables --- st2common/st2common/runners/paramiko_ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index 02da51e7e6..b50522a90f 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -62,7 +62,7 @@ def __init__(self, cmd, timeout, ssh_connect_timeout, stdout=None, stderr=None): self.ssh_connect_timeout = ssh_connect_timeout self.stdout = stdout self.stderr = stderr - self.message = ('Command didn\'t finish in %s seconds or the SSH connection ' + + self.message = ('Command didn\'t finish in %s seconds or the SSH connection ' 'did not succeed in %s seconds' % (timeout, ssh_connect_timeout)) super(SSHCommandTimeoutError, self).__init__(self.message) From ee0b9b89f3a173dfc1d4e849d48fd70a7f8903f2 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sat, 18 Jul 2020 22:00:00 +0200 Subject: [PATCH 25/27] Update Changelog and remove duplicate Added section --- CHANGELOG.rst | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 91014926a3..29c9070aa9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -3,18 +3,16 @@ Changelog in development -------------- + Added ~~~~~ +* Add make command to autogen JSON schema from the models of action, rule, etc. Add check + to ensure update to the models require schema to be regenerated. (new feature) * Add support for a configurable connect timeout for SSH connections as requested in #4715 by adding the new configuration parameter ``ssh_connect_timeout`` to the ``ssh_runner`` group in st2.conf. This option was requested by Harry Lee (@tclh123) and contributed by Marcel Weinberg (@winem). -Added -~~~~~ -* Add make command to autogen JSON schema from the models of action, rule, etc. Add check - to ensure update to the models require schema to be regenerated. (new feature) - Fixed ~~~~~ * Fixed a bug where `type` attribute was missing for netstat action in linux pack. Fixes #4946 From 9d06c2b63e784462dd9ef4464e4333774ca9c072 Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 19 Jul 2020 18:09:19 +0200 Subject: [PATCH 26/27] Add newline to changelog message to be consistent with others --- CHANGELOG.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2c11193f19..199b4a1de3 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,7 @@ Added * Add support for a configurable connect timeout for SSH connections as requested in #4715 by adding the new configuration parameter ``ssh_connect_timeout`` to the ``ssh_runner`` group in st2.conf. + This option was requested by Harry Lee (@tclh123) and contributed by Marcel Weinberg (@winem). Fixed From 75bbc3879e2117a91089d731e06e623f3426947b Mon Sep 17 00:00:00 2001 From: Marcel Weinberg Date: Sun, 19 Jul 2020 18:21:27 +0200 Subject: [PATCH 27/27] add PR reference --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 199b4a1de3..5dfbc6ed12 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,7 +12,7 @@ Added different partition (@punkrokk) * Add support for a configurable connect timeout for SSH connections as requested in #4715 by adding the new configuration parameter ``ssh_connect_timeout`` to the ``ssh_runner`` - group in st2.conf. + group in st2.conf. (new feature) #4914 This option was requested by Harry Lee (@tclh123) and contributed by Marcel Weinberg (@winem).