From d86ad77d6f6486465eade3b3654b05fdc76ff448 Mon Sep 17 00:00:00 2001 From: Abhijeet Kasurde Date: Mon, 25 Mar 2024 07:50:37 -0700 Subject: [PATCH] Remove deprecated scp_if_ssh from SSH connection plugin (#82072) * removed deprecated scp_if_ssh feature from SSH connection plugin Fixes: #81715 Signed-off-by: Abhijeet Kasurde --- changelogs/fragments/scp_if_ssh.yml | 3 + lib/ansible/plugins/connection/ssh.py | 47 ++------------ .../targets/connection_ssh/runme.sh | 2 +- test/units/plugins/connection/test_ssh.py | 65 +------------------ 4 files changed, 13 insertions(+), 104 deletions(-) create mode 100644 changelogs/fragments/scp_if_ssh.yml diff --git a/changelogs/fragments/scp_if_ssh.yml b/changelogs/fragments/scp_if_ssh.yml new file mode 100644 index 00000000000..98ce86af914 --- /dev/null +++ b/changelogs/fragments/scp_if_ssh.yml @@ -0,0 +1,3 @@ +--- +removed_features: + - Remove deprecated ``scp_if_ssh`` from ssh connection plugin (https://github.com/ansible/ansible/issues/81715). diff --git a/lib/ansible/plugins/connection/ssh.py b/lib/ansible/plugins/connection/ssh.py index eca906e5be5..24e5fb3d070 100644 --- a/lib/ansible/plugins/connection/ssh.py +++ b/lib/ansible/plugins/connection/ssh.py @@ -306,10 +306,10 @@ DOCUMENTATION = ''' version_added: '2.7' ssh_transfer_method: description: - - "Preferred method to use when transferring files over ssh" - - Setting to 'smart' (default) will try them in order, until one succeeds or they all fail - - For OpenSSH >=9.0 you must add an additional option to enable scp (scp_extra_args="-O") - - Using 'piped' creates an ssh pipe with C(dd) on either side to copy the data + - Preferred method to use when transferring files over SSH. + - Setting to 'smart' (default) will try them in order until one succeeds or they all fail. + - For OpenSSH >=9.0 you must add an additional option to enable scp (scp_extra_args="-O"). + - Using 'piped' creates an SSH pipe with C(dd) on either side to copy the data. choices: ['sftp', 'scp', 'piped', 'smart'] type: string env: [{name: ANSIBLE_SSH_TRANSFER_METHOD}] @@ -318,24 +318,6 @@ DOCUMENTATION = ''' vars: - name: ansible_ssh_transfer_method version_added: '2.12' - scp_if_ssh: - deprecated: - why: In favor of the O(ssh_transfer_method) option. - version: "2.17" - alternatives: O(ssh_transfer_method) - default: smart - description: - - "Preferred method to use when transferring files over SSH." - - When set to V(smart), Ansible will try them until one succeeds or they all fail. - - If set to V(True), it will force 'scp', if V(False) it will use 'sftp'. - - For OpenSSH >=9.0 you must add an additional option to enable scp (C(scp_extra_args="-O")) - - This setting will overridden by O(ssh_transfer_method) if set. - env: [{name: ANSIBLE_SCP_IF_SSH}] - ini: - - {key: scp_if_ssh, section: ssh_connection} - vars: - - name: ansible_scp_if_ssh - version_added: '2.7' use_tty: version_added: '2.5' default: true @@ -403,10 +385,8 @@ from ansible.errors import ( AnsibleError, AnsibleFileNotFound, ) -from ansible.errors import AnsibleOptionsError from ansible.module_utils.six import PY3, text_type, binary_type from ansible.module_utils.common.text.converters import to_bytes, to_native, to_text -from ansible.module_utils.parsing.convert_bool import BOOLEANS, boolean from ansible.plugins.connection import ConnectionBase, BUFSIZE from ansible.plugins.shell.powershell import _parse_clixml from ansible.utils.display import Display @@ -1241,10 +1221,9 @@ class Connection(ConnectionBase): # Transfer methods to try methods = [] - # Use the transfer_method option if set, otherwise use scp_if_ssh + # Use the transfer_method option if set ssh_transfer_method = self.get_option('ssh_transfer_method') - scp_if_ssh = self.get_option('scp_if_ssh') - if ssh_transfer_method is None and scp_if_ssh == 'smart': + if ssh_transfer_method is None: ssh_transfer_method = 'smart' if ssh_transfer_method is not None: @@ -1252,20 +1231,6 @@ class Connection(ConnectionBase): methods = smart_methods else: methods = [ssh_transfer_method] - else: - # since this can be a non-bool now, we need to handle it correctly - 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, strict=False) - elif scp_if_ssh != 'smart': - raise AnsibleOptionsError('scp_if_ssh needs to be one of [smart|True|False]') - if scp_if_ssh == 'smart': - methods = smart_methods - elif scp_if_ssh is True: - methods = ['scp'] - else: - methods = ['sftp'] for method in methods: returncode = stdout = stderr = None diff --git a/test/integration/targets/connection_ssh/runme.sh b/test/integration/targets/connection_ssh/runme.sh index ad817c83011..5fee4317c1f 100755 --- a/test/integration/targets/connection_ssh/runme.sh +++ b/test/integration/targets/connection_ssh/runme.sh @@ -66,7 +66,7 @@ fi # sftp ./posix.sh "$@" # scp -ANSIBLE_SCP_IF_SSH=true ./posix.sh "$@" "${scp_args[@]}" +ANSIBLE_SSH_TRANSFER_METHOD=scp ./posix.sh "$@" "${scp_args[@]}" # piped ANSIBLE_SSH_TRANSFER_METHOD=piped ./posix.sh "$@" diff --git a/test/units/plugins/connection/test_ssh.py b/test/units/plugins/connection/test_ssh.py index 3ea02d050f1..54486a7e92a 100644 --- a/test/units/plugins/connection/test_ssh.py +++ b/test/units/plugins/connection/test_ssh.py @@ -26,7 +26,7 @@ import pytest from ansible.errors import AnsibleAuthenticationFailure import unittest from unittest.mock import patch, MagicMock, PropertyMock -from ansible.errors import AnsibleError, AnsibleConnectionFailure, AnsibleFileNotFound +from ansible.errors import AnsibleConnectionFailure import shlex from ansible.module_utils.common.text.converters import to_bytes from ansible.playbook.play_context import PlayContext @@ -241,11 +241,9 @@ class TestConnectionBaseClass(unittest.TestCase): conn.host = "some_host" conn.set_option('reconnection_retries', 9) - conn.set_option('ssh_transfer_method', None) # unless set to None scp_if_ssh is ignored + conn.set_option('ssh_transfer_method', None) # unless set to None - # Test with SCP_IF_SSH set to smart # Test when SFTP works - conn.set_option('scp_if_ssh', 'smart') expected_in_data = b' '.join((b'put', to_bytes(shlex.quote('/path/to/in/file')), to_bytes(shlex.quote('/path/to/dest/file')))) + b'\n' conn.put_file('/path/to/in/file', '/path/to/dest/file') conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) @@ -256,35 +254,6 @@ class TestConnectionBaseClass(unittest.TestCase): conn._bare_run.assert_called_with('some command to run', None, checkrc=False) conn._bare_run.side_effect = None - # test with SCP_IF_SSH enabled - conn.set_option('scp_if_ssh', True) - conn.put_file('/path/to/in/file', '/path/to/dest/file') - conn._bare_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._bare_run.assert_called_with('some command to run', None, checkrc=False) - - # test with SCPP_IF_SSH disabled - conn.set_option('scp_if_ssh', False) - expected_in_data = b' '.join((b'put', to_bytes(shlex.quote('/path/to/in/file')), to_bytes(shlex.quote('/path/to/dest/file')))) + b'\n' - conn.put_file('/path/to/in/file', '/path/to/dest/file') - conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) - - expected_in_data = b' '.join((b'put', - to_bytes(shlex.quote('/path/to/in/file/with/unicode-fö〩')), - to_bytes(shlex.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._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) - - # test that a non-zero rc raises an error - conn._bare_run.return_value = (1, 'stdout', 'some errors') - self.assertRaises(AnsibleError, conn.put_file, '/path/to/bad/file', '/remote/path/to/file') - - # test that a not-found path raises an error - mock_ospe.return_value = False - conn._bare_run.return_value = (0, 'stdout', '') - self.assertRaises(AnsibleFileNotFound, conn.put_file, '/path/to/bad/file', '/remote/path/to/file') - @patch('time.sleep') def test_plugins_connection_ssh_fetch_file(self, mock_sleep): pc = PlayContext() @@ -299,11 +268,9 @@ class TestConnectionBaseClass(unittest.TestCase): conn.host = "some_host" conn.set_option('reconnection_retries', 9) - conn.set_option('ssh_transfer_method', None) # unless set to None scp_if_ssh is ignored + conn.set_option('ssh_transfer_method', None) # unless set to None - # Test with SCP_IF_SSH set to smart # Test when SFTP works - conn.set_option('scp_if_ssh', 'smart') expected_in_data = b' '.join((b'get', to_bytes(shlex.quote('/path/to/in/file')), to_bytes(shlex.quote('/path/to/dest/file')))) + b'\n' conn.set_options({}) conn.fetch_file('/path/to/in/file', '/path/to/dest/file') @@ -314,32 +281,6 @@ class TestConnectionBaseClass(unittest.TestCase): conn.fetch_file('/path/to/in/file', '/path/to/dest/file') conn._bare_run.assert_called_with('some command to run', None, checkrc=False) - # test with SCP_IF_SSH enabled - conn._bare_run.side_effect = None - conn.set_option('ssh_transfer_method', None) # unless set to None scp_if_ssh is ignored - conn.set_option('scp_if_ssh', 'True') - conn.fetch_file('/path/to/in/file', '/path/to/dest/file') - conn._bare_run.assert_called_with('some command to run', None, checkrc=False) - - conn.fetch_file(u'/path/to/in/file/with/unicode-fö〩', u'/path/to/dest/file/with/unicode-fö〩') - conn._bare_run.assert_called_with('some command to run', None, checkrc=False) - - # test with SCP_IF_SSH disabled - conn.set_option('scp_if_ssh', False) - expected_in_data = b' '.join((b'get', to_bytes(shlex.quote('/path/to/in/file')), to_bytes(shlex.quote('/path/to/dest/file')))) + b'\n' - conn.fetch_file('/path/to/in/file', '/path/to/dest/file') - conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) - - expected_in_data = b' '.join((b'get', - to_bytes(shlex.quote('/path/to/in/file/with/unicode-fö〩')), - to_bytes(shlex.quote('/path/to/dest/file/with/unicode-fö〩')))) + b'\n' - conn.fetch_file(u'/path/to/in/file/with/unicode-fö〩', u'/path/to/dest/file/with/unicode-fö〩') - conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) - - # test that a non-zero rc raises an error - conn._bare_run.return_value = (1, 'stdout', 'some errors') - self.assertRaises(AnsibleError, conn.fetch_file, '/path/to/bad/file', '/remote/path/to/file') - class MockSelector(object): def __init__(self):