From d9366cb8188725f979cd54271ce257dfc9e58169 Mon Sep 17 00:00:00 2001 From: Nathaniel Case Date: Tue, 16 Jun 2020 23:35:37 -0400 Subject: [PATCH] [stable-2.9] Allow the use of paramiko_conn before connection (#61570) (#69642) * [stable-2.9] Allow the use of _paramiko_conn even if the connection hasn't been started. (#61570) * Allow the use of _paramiko_conn even if the connection hasn't been started. I'm not sure what the benefit is of Noneing paramiko_conn on close, but will keep for now * Fix test * Try to fix up net_put & net_get * Add changelog. (cherry picked from commit 50e09be14f0b055440a3b7df7ed916c8c24bdae2) Co-authored-by: Nathaniel Case * Restore check_prompt and task_uuid --- .../fragments/61570-netcli-put-get.yaml | 2 + lib/ansible/plugins/action/net_get.py | 41 +++++++------ lib/ansible/plugins/action/net_put.py | 57 ++++++++----------- lib/ansible/plugins/cliconf/__init__.py | 8 ++- lib/ansible/plugins/connection/network_cli.py | 11 +++- .../plugins/connection/test_network_cli.py | 4 +- 6 files changed, 67 insertions(+), 56 deletions(-) create mode 100644 changelogs/fragments/61570-netcli-put-get.yaml diff --git a/changelogs/fragments/61570-netcli-put-get.yaml b/changelogs/fragments/61570-netcli-put-get.yaml new file mode 100644 index 00000000000..8d835cf8766 --- /dev/null +++ b/changelogs/fragments/61570-netcli-put-get.yaml @@ -0,0 +1,2 @@ +bugfixes: +- "fixed issues when using net_get & net_put before the persistent connection has been started" diff --git a/lib/ansible/plugins/action/net_get.py b/lib/ansible/plugins/action/net_get.py index 5ad646068d7..b19c44824cf 100644 --- a/lib/ansible/plugins/action/net_get.py +++ b/lib/ansible/plugins/action/net_get.py @@ -25,7 +25,7 @@ import hashlib from ansible.errors import AnsibleError from ansible.module_utils._text import to_text, to_bytes -from ansible.module_utils.connection import Connection +from ansible.module_utils.connection import Connection, ConnectionError from ansible.plugins.action import ActionBase from ansible.module_utils.six.moves.urllib.parse import urlsplit from ansible.utils.display import Display @@ -67,33 +67,31 @@ class ActionModule(ActionBase): if proto is None: proto = 'scp' - sock_timeout = self._play_context.timeout - if socket_path is None: socket_path = self._connection.socket_path conn = Connection(socket_path) + sock_timeout = conn.get_option('persistent_command_timeout') try: changed = self._handle_existing_file(conn, src, dest, proto, sock_timeout) if changed is False: - result['changed'] = False + result['changed'] = changed result['destination'] = dest return result except Exception as exc: - result['msg'] = ('Warning: exception %s idempotency check failed. Check ' - 'dest' % exc) + result['msg'] = ('Warning: %s idempotency check failed. Check dest' % exc) try: - out = conn.get_file( + conn.get_file( source=src, destination=dest, proto=proto, timeout=sock_timeout ) except Exception as exc: result['failed'] = True - result['msg'] = ('Exception received : %s' % exc) + result['msg'] = 'Exception received: %s' % exc - result['changed'] = True + result['changed'] = changed result['destination'] = dest return result @@ -118,27 +116,37 @@ class ActionModule(ActionBase): return filename def _handle_existing_file(self, conn, source, dest, proto, timeout): + """ + Determines whether the source and destination file match. + + :return: False if source and dest both exist and have matching sha1 sums, True otherwise. + """ if not os.path.exists(dest): return True + cwd = self._loader.get_basedir() filename = str(uuid.uuid4()) tmp_dest_file = os.path.join(cwd, filename) try: - out = conn.get_file( + conn.get_file( source=source, destination=tmp_dest_file, proto=proto, timeout=timeout ) - except Exception as exc: - os.remove(tmp_dest_file) - raise Exception(exc) + except ConnectionError as exc: + error = to_text(exc) + if error.endswith("No such file or directory"): + if os.path.exists(tmp_dest_file): + os.remove(tmp_dest_file) + return True try: with open(tmp_dest_file, 'r') as f: new_content = f.read() with open(dest, 'r') as f: old_content = f.read() - except (IOError, OSError) as ioexc: - raise IOError(ioexc) + except (IOError, OSError): + os.remove(tmp_dest_file) + raise sha1 = hashlib.sha1() old_content_b = to_bytes(old_content, errors='surrogate_or_strict') @@ -152,8 +160,7 @@ class ActionModule(ActionBase): os.remove(tmp_dest_file) if checksum_old == checksum_new: return False - else: - return True + return True def _get_working_path(self): cwd = self._loader.get_basedir() diff --git a/lib/ansible/plugins/action/net_put.py b/lib/ansible/plugins/action/net_put.py index 55ecb1c3980..bd844a12166 100644 --- a/lib/ansible/plugins/action/net_put.py +++ b/lib/ansible/plugins/action/net_put.py @@ -19,15 +19,12 @@ __metaclass__ = type import copy import os -import time import uuid import hashlib -import sys -import re from ansible.errors import AnsibleError from ansible.module_utils._text import to_text, to_bytes -from ansible.module_utils.connection import Connection +from ansible.module_utils.connection import Connection, ConnectionError from ansible.plugins.action import ActionBase from ansible.module_utils.six.moves.urllib.parse import urlsplit from ansible.utils.display import Display @@ -38,7 +35,6 @@ display = Display() class ActionModule(ActionBase): def run(self, tmp=None, task_vars=None): - changed = True socket_path = None network_os = self._get_network_os(task_vars).split('.')[-1] persistent_connection = self._play_context.connection.split('.')[-1] @@ -53,7 +49,7 @@ class ActionModule(ActionBase): return result try: - src = self._task.args.get('src') + src = self._task.args['src'] except KeyError as exc: return {'failed': True, 'msg': 'missing required argument: %s' % exc} @@ -107,15 +103,14 @@ class ActionModule(ActionBase): try: changed = self._handle_existing_file(conn, output_file, dest, proto, sock_timeout) if changed is False: - result['changed'] = False + result['changed'] = changed result['destination'] = dest return result except Exception as exc: - result['msg'] = ('Warning: Exc %s idempotency check failed. Check' - 'dest' % exc) + result['msg'] = ('Warning: %s idempotency check failed. Check dest' % exc) try: - out = conn.copy_file( + conn.copy_file( source=output_file, destination=dest, proto=proto, timeout=sock_timeout ) @@ -127,7 +122,7 @@ class ActionModule(ActionBase): result['msg'] = 'Warning: iosxr scp server pre close issue. Please check dest' else: result['failed'] = True - result['msg'] = ('Exception received : %s' % exc) + result['msg'] = 'Exception received: %s' % exc if mode == 'text': # Cleanup tmp file expanded wih ansible vars @@ -138,35 +133,34 @@ class ActionModule(ActionBase): return result def _handle_existing_file(self, conn, source, dest, proto, timeout): + """ + Determines whether the source and destination file match. + + :return: False if source and dest both exist and have matching sha1 sums, True otherwise. + """ cwd = self._loader.get_basedir() filename = str(uuid.uuid4()) - source_file = os.path.join(cwd, filename) + tmp_source_file = os.path.join(cwd, filename) try: - out = conn.get_file( - source=dest, destination=source_file, + conn.get_file( + source=dest, destination=tmp_source_file, proto=proto, timeout=timeout ) - except Exception as exc: - pattern = to_text(exc) - not_found_exc = "No such file or directory" - if re.search(not_found_exc, pattern, re.I): - if os.path.exists(source_file): - os.remove(source_file) + except ConnectionError as exc: + error = to_text(exc) + if error.endswith("No such file or directory"): + if os.path.exists(tmp_source_file): + os.remove(tmp_source_file) return True - else: - try: - os.remove(source_file) - except OSError as osex: - raise Exception(osex) try: with open(source, 'r') as f: new_content = f.read() - with open(source_file, 'r') as f: + with open(tmp_source_file, 'r') as f: old_content = f.read() - except (IOError, OSError) as ioexc: - os.remove(source_file) - raise IOError(ioexc) + except (IOError, OSError): + os.remove(tmp_source_file) + raise sha1 = hashlib.sha1() old_content_b = to_bytes(old_content, errors='surrogate_or_strict') @@ -177,11 +171,10 @@ class ActionModule(ActionBase): new_content_b = to_bytes(new_content, errors='surrogate_or_strict') sha1.update(new_content_b) checksum_new = sha1.digest() - os.remove(source_file) + os.remove(tmp_source_file) if checksum_old == checksum_new: return False - else: - return True + return True def _get_binary_src_file(self, src): working_path = self._get_working_path() diff --git a/lib/ansible/plugins/cliconf/__init__.py b/lib/ansible/plugins/cliconf/__init__.py index 94e2b75b966..be2df78daf6 100644 --- a/lib/ansible/plugins/cliconf/__init__.py +++ b/lib/ansible/plugins/cliconf/__init__.py @@ -365,8 +365,12 @@ class CliconfBase(AnsiblePlugin): if proto == 'scp': if not HAS_SCP: raise AnsibleError("Required library scp is not installed. Please install it using `pip install scp`") - with SCPClient(ssh.get_transport(), socket_timeout=timeout) as scp: - scp.get(source, destination) + try: + with SCPClient(ssh.get_transport(), socket_timeout=timeout) as scp: + scp.get(source, destination) + except EOFError: + # This appears to be benign. + pass elif proto == 'sftp': with ssh.open_sftp() as sftp: sftp.get(source, destination) diff --git a/lib/ansible/plugins/connection/network_cli.py b/lib/ansible/plugins/connection/network_cli.py index 22cb6b86692..52add31c972 100644 --- a/lib/ansible/plugins/connection/network_cli.py +++ b/lib/ansible/plugins/connection/network_cli.py @@ -358,6 +358,13 @@ class Connection(NetworkConnectionBase): ) self.queue_message('log', 'network_os is set to %s' % self._network_os) + @property + def paramiko_conn(self): + if self._paramiko_conn is None: + self._paramiko_conn = connection_loader.get('paramiko', self._play_context, '/dev/null') + self._paramiko_conn.set_options(direct={'look_for_keys': not bool(self._play_context.password and not self._play_context.private_key_file)}) + return self._paramiko_conn + def _get_log_channel(self): name = "p=%s u=%s | " % (os.getpid(), getpass.getuser()) name += "paramiko [%s]" % self._play_context.remote_addr @@ -431,9 +438,7 @@ class Connection(NetworkConnectionBase): Connects to the remote device and starts the terminal ''' if not self.connected: - self.paramiko_conn = connection_loader.get('paramiko', self._play_context, '/dev/null') self.paramiko_conn._set_log_channel(self._get_log_channel()) - self.paramiko_conn.set_options(direct={'look_for_keys': not bool(self._play_context.password and not self._play_context.private_key_file)}) self.paramiko_conn.force_persistence = self.force_persistence command_timeout = self.get_option('persistent_command_timeout') @@ -500,7 +505,7 @@ class Connection(NetworkConnectionBase): self.queue_message('debug', "cli session is now closed") self.paramiko_conn.close() - self.paramiko_conn = None + self._paramiko_conn = None self.queue_message('debug', "ssh connection has been closed successfully") super(Connection, self).close() diff --git a/test/units/plugins/connection/test_network_cli.py b/test/units/plugins/connection/test_network_cli.py index a625449a72e..2268721bf77 100644 --- a/test/units/plugins/connection/test_network_cli.py +++ b/test/units/plugins/connection/test_network_cli.py @@ -77,13 +77,13 @@ class TestConnectionClass(unittest.TestCase): terminal = MagicMock(supports_multiplexing=False) conn._terminal = terminal conn._ssh_shell = MagicMock() - conn.paramiko_conn = MagicMock() + conn._paramiko_conn = MagicMock() conn._connected = True conn.close() self.assertTrue(terminal.on_close_shell.called) self.assertIsNone(conn._ssh_shell) - self.assertIsNone(conn.paramiko_conn) + self.assertIsNone(conn._paramiko_conn) @patch("ansible.plugins.connection.paramiko_ssh.Connection._connect") def test_network_cli_exec_command(self, mocked_super):