From fff161f2f69280a9db86a3ba18c9ef54ee2639d9 Mon Sep 17 00:00:00 2001 From: jctanner Date: Thu, 29 Sep 2016 17:44:54 -0400 Subject: [PATCH] Smart mode for sftp+scp (#17813) If the sftp fails, roll over to scp by default. This saves users from having to know about the scp_if_ssh method when sftp is broken on the remote host. --- examples/ansible.cfg | 8 ++- lib/ansible/constants.py | 2 +- lib/ansible/plugins/connection/ssh.py | 63 ++++++++++++++----- .../connections/test_connection_ssh.py | 8 +-- 4 files changed, 59 insertions(+), 22 deletions(-) diff --git a/examples/ansible.cfg b/examples/ansible.cfg index 54a10082951..fac5181f11b 100644 --- a/examples/ansible.cfg +++ b/examples/ansible.cfg @@ -306,9 +306,11 @@ # #pipelining = False -# if True, make ansible use scp if the connection type is ssh -# (default is sftp) -#scp_if_ssh = True +# Control the mechanism for transfering files +# * smart = try sftp and then try scp [default] +# * True = use scp only +# * False = use sftp only +#scp_if_ssh = smart # if False, sftp will not use batch mode to transfer files. This may cause some # types of file transfer failures impossible to catch however, and should diff --git a/lib/ansible/constants.py b/lib/ansible/constants.py index 333c8215dca..dd232fc8963 100644 --- a/lib/ansible/constants.py +++ b/lib/ansible/constants.py @@ -172,7 +172,7 @@ DEFAULT_REMOTE_PORT = get_config(p, DEFAULTS, 'remote_port', 'ANSIBLE DEFAULT_ASK_VAULT_PASS = get_config(p, DEFAULTS, 'ask_vault_pass', 'ANSIBLE_ASK_VAULT_PASS', False, boolean=True) DEFAULT_VAULT_PASSWORD_FILE = get_config(p, DEFAULTS, 'vault_password_file', 'ANSIBLE_VAULT_PASSWORD_FILE', None, ispath=True) DEFAULT_TRANSPORT = get_config(p, DEFAULTS, 'transport', 'ANSIBLE_TRANSPORT', 'smart') -DEFAULT_SCP_IF_SSH = get_config(p, 'ssh_connection', 'scp_if_ssh', 'ANSIBLE_SCP_IF_SSH', False, boolean=True) +DEFAULT_SCP_IF_SSH = get_config(p, 'ssh_connection', 'scp_if_ssh', 'ANSIBLE_SCP_IF_SSH', 'smart') DEFAULT_SFTP_BATCH_MODE = get_config(p, 'ssh_connection', 'sftp_batch_mode', 'ANSIBLE_SFTP_BATCH_MODE', True, boolean=True) DEFAULT_MANAGED_STR = get_config(p, DEFAULTS, 'ansible_managed', None, 'Ansible managed: {file} modified on %Y-%m-%d %H:%M:%S by {uid} on {host}') DEFAULT_SYSLOG_FACILITY = get_config(p, DEFAULTS, 'syslog_facility', 'ANSIBLE_SYSLOG_FACILITY', 'LOG_USER') diff --git a/lib/ansible/plugins/connection/ssh.py b/lib/ansible/plugins/connection/ssh.py index c24752e1825..98ab6b08fd4 100644 --- a/lib/ansible/plugins/connection/ssh.py +++ b/lib/ansible/plugins/connection/ssh.py @@ -31,10 +31,14 @@ import time from ansible import constants as C from ansible.compat.six import text_type, binary_type from ansible.errors import AnsibleError, AnsibleConnectionFailure, AnsibleFileNotFound +from ansible.errors import AnsibleOptionsError +from ansible.module_utils.basic import BOOLEANS from ansible.module_utils._text import to_bytes, to_native, to_text from ansible.plugins.connection import ConnectionBase +from ansible.utils.boolean import boolean from ansible.utils.path import unfrackpath, makedirs_safe + try: from __main__ import display except ImportError: @@ -309,7 +313,7 @@ class Connection(ConnectionBase): return b''.join(output), remainder - def _run(self, cmd, in_data, sudoable=True): + def _run(self, cmd, in_data, sudoable=True, checkrc=True): ''' Starts the command and communicates with it until it ends. ''' @@ -551,7 +555,7 @@ class Connection(ConnectionBase): if p.returncode != 0 and controlpersisterror: raise AnsibleError('using -c ssh on certain older ssh versions may not support ControlPersist, set ANSIBLE_SSH_ARGS="" (or ssh_args in [ssh_connection] section of the config file) before running again') - if p.returncode == 255 and in_data: + if p.returncode == 255 and in_data and checkrc: raise AnsibleConnectionFailure('SSH Error: data could not be sent to the remote host. Make sure this host can be reached over ssh') return (p.returncode, b_stdout, b_stderr) @@ -641,18 +645,49 @@ class Connection(ConnectionBase): # accept them for hostnames and IPv4 addresses too. host = '[%s]' % self.host - if C.DEFAULT_SCP_IF_SSH: - cmd = self._build_command('scp', in_path, u'{0}:{1}'.format(host, pipes.quote(out_path))) - in_data = None - else: - cmd = self._build_command('sftp', to_bytes(host)) - in_data = u"put {0} {1}\n".format(pipes.quote(in_path), pipes.quote(out_path)) - - in_data = to_bytes(in_data, nonstring='passthru') - (returncode, stdout, stderr) = self._run(cmd, in_data) - - if returncode != 0: - raise AnsibleError("failed to transfer file to {0}:\n{1}\n{2}".format(to_native(out_path), to_native(stdout), to_native(stderr))) + # since this can be a non-bool now, we need to handle it correctly + scp_if_ssh = C.DEFAULT_SCP_IF_SSH + if not isinstance(scp_if_ssh, bool): + scp_if_ssh = scp_if_ssh.lower() + if scp_if_ssh in BOOLEANS: + scp_if_ssh = boolean(scp_if_ssh) + elif scp_if_ssh != 'smart': + raise AnsibleOptionsError('scp_if_ssh needs to be one of [smart|True|False]') + + # create a list of commands to use based on config options + methods = ['sftp'] + if scp_if_ssh == 'smart': + methods.append('scp') + elif scp_if_ssh: + methods = ['scp'] + + success = False + res = None + for method in methods: + if method == 'sftp': + cmd = self._build_command('sftp', to_bytes(host)) + in_data = u"put {0} {1}\n".format(pipes.quote(in_path), pipes.quote(out_path)) + elif method == 'scp': + cmd = self._build_command('scp', in_path, u'{0}:{1}'.format(host, pipes.quote(out_path))) + in_data = None + + in_data = to_bytes(in_data, nonstring='passthru') + (returncode, stdout, stderr) = self._run(cmd, in_data, checkrc=False) + # Check the return code and rollover to next method if failed + if returncode == 0: + success = True + break + else: + # If not in smart mode, the data will be printed by the raise below + if scp_if_ssh == 'smart': + display.warning(msg='%s transfer mechanism failed on %s. Use ANSIBLE_DEBUG=1 to see detailed information' % (method, host)) + display.debug(msg='%s' % to_native(stdout)) + display.debug(msg='%s' % to_native(stderr)) + res = (returncode, stdout, stderr) + + if not success: + raise AnsibleError("failed to transfer file to {0}:\n{1}\n{2}"\ + .format(to_native(out_path), to_native(res[1]), to_native(res[2]))) def fetch_file(self, in_path, out_path): ''' fetch a file from remote to local ''' diff --git a/test/units/plugins/connections/test_connection_ssh.py b/test/units/plugins/connections/test_connection_ssh.py index 85c363450b4..735f1b915f0 100644 --- a/test/units/plugins/connections/test_connection_ssh.py +++ b/test/units/plugins/connections/test_connection_ssh.py @@ -311,22 +311,22 @@ class TestConnectionBaseClass(unittest.TestCase): # test with C.DEFAULT_SCP_IF_SSH enabled C.DEFAULT_SCP_IF_SSH = True conn.put_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', None) + conn._run.assert_called_with('some command to run', None, checkrc=False) conn.put_file(u'/path/to/in/file/with/unicode-fö〩', u'/path/to/dest/file/with/unicode-fö〩') - conn._run.assert_called_with('some command to run', None) + conn._run.assert_called_with('some command to run', None, checkrc=False) # test with C.DEFAULT_SCP_IF_SSH disabled C.DEFAULT_SCP_IF_SSH = False expected_in_data = b' '.join((b'put', to_bytes(pipes.quote('/path/to/in/file')), to_bytes(pipes.quote('/path/to/dest/file')))) + b'\n' conn.put_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', expected_in_data) + conn._run.assert_called_with('some command to run', expected_in_data, checkrc=False) expected_in_data = b' '.join((b'put', to_bytes(pipes.quote('/path/to/in/file/with/unicode-fö〩')), to_bytes(pipes.quote('/path/to/dest/file/with/unicode-fö〩')))) + b'\n' conn.put_file(u'/path/to/in/file/with/unicode-fö〩', u'/path/to/dest/file/with/unicode-fö〩') - conn._run.assert_called_with('some command to run', expected_in_data) + conn._run.assert_called_with('some command to run', expected_in_data, checkrc=False) # test that a non-zero rc raises an error conn._run.return_value = (1, 'stdout', 'some errors')