From fff2f245667c9798871b2395c9ffb6628d427510 Mon Sep 17 00:00:00 2001 From: Eric Edgar Date: Mon, 9 Mar 2020 13:25:34 -0500 Subject: [PATCH 1/9] Close proxycommand socket when its not needed anymore --- st2common/st2common/runners/paramiko_ssh.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index c489629fb3..d70b477872 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -123,6 +123,7 @@ def __init__(self, hostname, port=DEFAULT_SSH_PORT, username=None, password=None self.bastion_client = None self.bastion_socket = None + self.socket = None def connect(self): """ @@ -455,6 +456,12 @@ def close(self): self.client.close() + if self.socket: + self.logger.debug('Closing proxycommand socket connection') + #https://github.com/paramiko/paramiko/issues/789 Avoid zombie ssh processes + self.socket.process.kill() + self.socket.process.poll() + if self.sftp_client: self.sftp_client.close() @@ -698,8 +705,8 @@ def _connect(self, host, socket=None): '_username': self.username, '_timeout': self.timeout} self.logger.debug('Connecting to server', extra=extra) - socket = socket or ssh_config_file_info.get('sock', None) - if socket: + self.socket = self.socket or ssh_config_file_info.get('sock', None) + if self.socket: conninfo['sock'] = socket client = paramiko.SSHClient() From 12612b33e5c93a3f6e44851db5ea32037a2df2e8 Mon Sep 17 00:00:00 2001 From: JP Bourget Date: Wed, 1 Apr 2020 20:59:21 -0400 Subject: [PATCH 2/9] Update st2common/st2common/runners/paramiko_ssh.py Co-Authored-By: blag --- 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 d70b477872..dab48112ef 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -458,7 +458,7 @@ def close(self): if self.socket: self.logger.debug('Closing proxycommand socket connection') - #https://github.com/paramiko/paramiko/issues/789 Avoid zombie ssh processes + # https://github.com/paramiko/paramiko/issues/789 Avoid zombie ssh processes self.socket.process.kill() self.socket.process.poll() From b18722777d7abf52ba2c746ddbc2e675a05e7f86 Mon Sep 17 00:00:00 2001 From: Eric Edgar Date: Fri, 3 Apr 2020 12:08:57 -0500 Subject: [PATCH 3/9] Update CHANGELOG referring to 4881 --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 79ea9b0362..798c012cfb 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -20,7 +20,7 @@ Added Changed ~~~~~~~ - +* Fix ssh zombies when using ProxyCommand from ssh config #4881 * Install pack with the latest tag version if it exists when branch is not specialized. (improvement) #4743 * Implement "continue" engine command to orquesta workflow. (improvement) #4740 From 76236e50c89bd8de33e1498a0541be3403f764e4 Mon Sep 17 00:00:00 2001 From: Eric Edgar Date: Fri, 3 Apr 2020 12:14:01 -0500 Subject: [PATCH 4/9] use socket from the entry to _connect --- 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 dab48112ef..2e5154db9f 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -705,7 +705,7 @@ def _connect(self, host, socket=None): '_username': self.username, '_timeout': self.timeout} self.logger.debug('Connecting to server', extra=extra) - self.socket = self.socket or ssh_config_file_info.get('sock', None) + self.socket = socket or ssh_config_file_info.get('sock', None) if self.socket: conninfo['sock'] = socket From 3393409fec11069b6d733a1cdf611bfd5c15d974 Mon Sep 17 00:00:00 2001 From: Eric Edgar Date: Fri, 3 Apr 2020 15:33:34 -0500 Subject: [PATCH 5/9] add tests for paramiko socket closing --- st2actions/tests/unit/test_paramiko_ssh.py | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index e0697664a3..8047f8fce5 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -802,3 +802,35 @@ def test_use_ssh_config_port_value_provided_in_the_config(self, mock_sshclient): call_kwargs = mock_client.connect.call_args[1] self.assertEqual(call_kwargs['port'], 9999) + + @patch('paramiko.SSHClient', Mock) + @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase', + MagicMock(return_value=False)) + + def test_socket_closed(self): + conn_params = {'hostname': 'dummy.host.org', + 'username': 'ubuntu', + 'password': 'pass', + 'timeout': '600'} + ssh_client = ParamikoSSHClient(**conn_params) + ssh_client.connect() + + # Make sure .close() doesn't actually call anything real + ssh_client.client = Mock() + ssh_client.sftp_client = None + ssh_client.bastion_client = None + + ssh_client.socket = Mock() + + # Make sure we havent called any close methods at this point + self.assertEqual(ssh_client.socket.process.kill.call_count, 0) + self.assertEqual(ssh_client.socket.process.poll.call_count, 0) + + # Call the function that has changed + ssh_client.close() + + # Make sure we have called kill and poll + self.assertEqual(ssh_client.socket.process.kill.call_count, 1) + self.assertEqual(ssh_client.socket.process.poll.call_count, 1) + + From bb2c988705e0a683ad7e42e36a78dba51e592b31 Mon Sep 17 00:00:00 2001 From: Eric Edgar Date: Fri, 3 Apr 2020 15:58:50 -0500 Subject: [PATCH 6/9] Add todos for tests --- st2actions/tests/unit/test_paramiko_ssh.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index 8047f8fce5..5b02c9542e 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -803,10 +803,8 @@ def test_use_ssh_config_port_value_provided_in_the_config(self, mock_sshclient): call_kwargs = mock_client.connect.call_args[1] self.assertEqual(call_kwargs['port'], 9999) - @patch('paramiko.SSHClient', Mock) @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase', MagicMock(return_value=False)) - def test_socket_closed(self): conn_params = {'hostname': 'dummy.host.org', 'username': 'ubuntu', @@ -823,6 +821,7 @@ def test_socket_closed(self): ssh_client.socket = Mock() # Make sure we havent called any close methods at this point + # TODO: Replace these with .assert_not_called() once it's Python 3.6+ only self.assertEqual(ssh_client.socket.process.kill.call_count, 0) self.assertEqual(ssh_client.socket.process.poll.call_count, 0) @@ -830,7 +829,6 @@ def test_socket_closed(self): ssh_client.close() # Make sure we have called kill and poll + # TODO: Replace these with .assert_called_once() once it's Python 3.6+ only self.assertEqual(ssh_client.socket.process.kill.call_count, 1) self.assertEqual(ssh_client.socket.process.poll.call_count, 1) - - From cd3220b820e51819b8e84d63f0327e879ed29d08 Mon Sep 17 00:00:00 2001 From: Eric Edgar Date: Fri, 3 Apr 2020 17:04:43 -0500 Subject: [PATCH 7/9] move to bugs --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 803d699432..be3d18e7fe 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -20,7 +20,6 @@ Added Changed ~~~~~~~ -* Fix ssh zombies when using ProxyCommand from ssh config #4881 * Install pack with the latest tag version if it exists when branch is not specialized. (improvement) #4743 * Implement "continue" engine command to orquesta workflow. (improvement) #4740 @@ -2428,6 +2427,7 @@ Deprecated Fixed ~~~~~ +* Fix ssh zombies when using ProxyCommand from ssh config #4881 [Eric Edgar] * Bug fixes to allow Sensors to have their own log files. #2487 [Andrew Regan] * Make sure that the ``filename``, ``module``, ``funcName`` and ``lineno`` attributes which are available in the log formatter string contain the correct values. (bug-fix) From c49f97a9fb700af6374e72cf963af9d23a050248 Mon Sep 17 00:00:00 2001 From: Eric Edgar Date: Fri, 3 Apr 2020 17:05:21 -0500 Subject: [PATCH 8/9] remove unneeded test call --- st2actions/tests/unit/test_paramiko_ssh.py | 1 - 1 file changed, 1 deletion(-) diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py index 5b02c9542e..c6da6e3058 100644 --- a/st2actions/tests/unit/test_paramiko_ssh.py +++ b/st2actions/tests/unit/test_paramiko_ssh.py @@ -811,7 +811,6 @@ def test_socket_closed(self): 'password': 'pass', 'timeout': '600'} ssh_client = ParamikoSSHClient(**conn_params) - ssh_client.connect() # Make sure .close() doesn't actually call anything real ssh_client.client = Mock() From 9bd10f7089ca2b50e8e9f83d2733c1fefd049b43 Mon Sep 17 00:00:00 2001 From: Eric Edgar Date: Mon, 6 Apr 2020 13:57:44 -0500 Subject: [PATCH 9/9] Move the changes to the correct place in the changelog --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index be3d18e7fe..d89577336a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -64,6 +64,7 @@ Changed Fixed ~~~~~ +* Fix ssh zombies when using ProxyCommand from ssh config #4881 [Eric Edgar] * Fix rbac with execution view where the rbac is unable to verify the pack or uid of the execution because it was not returned from the action execution db. This would result in an internal server error when trying to view the results of a single execution. @@ -2427,7 +2428,6 @@ Deprecated Fixed ~~~~~ -* Fix ssh zombies when using ProxyCommand from ssh config #4881 [Eric Edgar] * Bug fixes to allow Sensors to have their own log files. #2487 [Andrew Regan] * Make sure that the ``filename``, ``module``, ``funcName`` and ``lineno`` attributes which are available in the log formatter string contain the correct values. (bug-fix)