clean up ansible-connection (#82992)

* clean up ansible-connection stuff

* eliminate unnecessary usage of pty/termios
* always use default pickle protocol
* remove unnecessary wire hashing

Co-authored-by: Kate Case <this.is@katherineca.se>

bin/ansible-connection

@@ -1 +0,0 @@
-../lib/ansible/cli/scripts/ansible_connection_cli_stub.py

changelogs/fragments/ansible_connection_path.yml

@@ -0,0 +1,8 @@
+bugfixes:
+  - persistent connection plugins - The correct Ansible persistent connection helper is now always used.
+    Previously, the wrong script could be used, depending on the value of the ``PATH`` environment variable.
+    As a result, users were sometimes required to set ``ANSIBLE_CONNECTION_PATH`` to use the correct script.
+deprecated_features:
+  - persistent connection plugins - The ``ANSIBLE_CONNECTION_PATH`` config option no longer has any effect, and will be removed in a future release.
+breaking_changes:
+  - persistent connection plugins - The ``ANSIBLE_CONNECTION_PATH`` config option no longer has any effect.

lib/ansible/cli/scripts/ansible_connection_cli_stub.py

@@ -1,10 +1,8 @@
-#!/usr/bin/env python
 # Copyright: (c) 2017, Ansible Project
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 from __future__ import annotations
 
 import fcntl
-import hashlib
 import io
 import os
 import pickle
@@ -40,13 +38,6 @@ def read_stream(byte_stream):
     if len(data) < size:
         raise Exception("EOF found before data was complete")
 
-    data_hash = to_text(byte_stream.readline().strip())
-    if data_hash != hashlib.sha1(data).hexdigest():
-        raise Exception("Read {0} bytes, but data did not match checksum".format(size))
-
-    # restore escaped loose \r characters
-    data = data.replace(br'\r', b'\r')
-
     return data
 
 
@@ -221,7 +212,7 @@ def main(args=None):
     """ Called to initiate the connect to the remote device
     """
 
-    parser = opt_help.create_base_parser(prog='ansible-connection')
+    parser = opt_help.create_base_parser(prog=None)
     opt_help.add_verbosity_options(parser)
     parser.add_argument('playbook_pid')
     parser.add_argument('task_uuid')

lib/ansible/config/base.yml

@@ -1,6 +1,14 @@
 # Copyright (c) 2017 Ansible Project
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 ---
+_ANSIBLE_CONNECTION_PATH:
+  env:
+  - name: _ANSIBLE_CONNECTION_PATH
+  name: Overrides the location of the Ansible persistent connection helper script.
+  description:
+    - For internal use only.
+  type: path
+  version_added: "2.18"
 ANSIBLE_HOME:
   name: The Ansible home path
   description:
@@ -25,6 +33,9 @@ ANSIBLE_CONNECTION_PATH:
   - {key: ansible_connection_path, section: persistent_connection}
   yaml: {key: persistent_connection.ansible_connection_path}
   version_added: "2.8"
+  deprecated:
+    why: This setting has no effect.
+    version: "2.22"
 ANSIBLE_COW_SELECTION:
   name: Cowsay filter selection
   default: default

lib/ansible/executor/task_executor.py

@@ -4,23 +4,23 @@
 from __future__ import annotations
 
 import os
-import pty
 import time
 import json
+import pathlib
 import signal
 import subprocess
 import sys
-import termios
 import traceback
 
 from ansible import constants as C
+from ansible.cli import scripts
 from ansible.errors import AnsibleError, AnsibleParserError, AnsibleUndefinedVariable, AnsibleConnectionFailure, AnsibleActionFail, AnsibleActionSkip
 from ansible.executor.task_result import TaskResult
 from ansible.executor.module_common import get_action_args_with_defaults
 from ansible.module_utils.parsing.convert_bool import boolean
 from ansible.module_utils.six import binary_type
 from ansible.module_utils.common.text.converters import to_text, to_native
-from ansible.module_utils.connection import write_to_file_descriptor
+from ansible.module_utils.connection import write_to_stream
 from ansible.playbook.conditional import Conditional
 from ansible.playbook.task import Task
 from ansible.plugins import get_plugin_class
@@ -1179,26 +1179,19 @@ class TaskExecutor:
         return handler, module
 
 
+CLI_STUB_NAME = 'ansible_connection_cli_stub.py'
+
+
 def start_connection(play_context, options, task_uuid):
     '''
     Starts the persistent connection
     '''
-    candidate_paths = [C.ANSIBLE_CONNECTION_PATH or os.path.dirname(sys.argv[0])]
-    candidate_paths.extend(os.environ.get('PATH', '').split(os.pathsep))
-    for dirname in candidate_paths:
-        ansible_connection = os.path.join(dirname, 'ansible-connection')
-        if os.path.isfile(ansible_connection):
-            display.vvvv("Found ansible-connection at path {0}".format(ansible_connection))
-            break
-    else:
-        raise AnsibleError("Unable to find location of 'ansible-connection'. "
-                           "Please set or check the value of ANSIBLE_CONNECTION_PATH")
 
     env = os.environ.copy()
     env.update({
         # HACK; most of these paths may change during the controller's lifetime
         # (eg, due to late dynamic role includes, multi-playbook execution), without a way
-        # to invalidate/update, ansible-connection won't always see the same plugins the controller
+        # to invalidate/update, the persistent connection helper won't always see the same plugins the controller
         # can.
         'ANSIBLE_BECOME_PLUGINS': become_loader.print_paths(),
         'ANSIBLE_CLICONF_PLUGINS': cliconf_loader.print_paths(),
@@ -1211,30 +1204,19 @@ def start_connection(play_context, options, task_uuid):
     verbosity = []
     if display.verbosity:
         verbosity.append('-%s' % ('v' * display.verbosity))
-    python = sys.executable
+
-    master, slave = pty.openpty()
+    if not (cli_stub_path := C.config.get_config_value('_ANSIBLE_CONNECTION_PATH')):
+        cli_stub_path = str(pathlib.Path(scripts.__file__).parent / CLI_STUB_NAME)
+
     p = subprocess.Popen(
-        [python, ansible_connection, *verbosity, to_text(os.getppid()), to_text(task_uuid)],
+        [sys.executable, cli_stub_path, *verbosity, to_text(os.getppid()), to_text(task_uuid)],
-        stdin=slave, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env
+        stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env,
     )
-    os.close(slave)
 
-    # We need to set the pty into noncanonical mode. This ensures that we
+    write_to_stream(p.stdin, options)
-    # can receive lines longer than 4095 characters (plus newline) without
+    write_to_stream(p.stdin, play_context.serialize())
-    # truncating.
-    old = termios.tcgetattr(master)
-    new = termios.tcgetattr(master)
-    new[3] = new[3] & ~termios.ICANON
 
-    try:
+    (stdout, stderr) = p.communicate()
-        termios.tcsetattr(master, termios.TCSANOW, new)
-        write_to_file_descriptor(master, options)
-        write_to_file_descriptor(master, play_context.serialize())
-
-        (stdout, stderr) = p.communicate()
-    finally:
-        termios.tcsetattr(master, termios.TCSANOW, old)
-    os.close(master)
 
     if p.returncode == 0:
         result = json.loads(to_text(stdout, errors='surrogate_then_replace'))

lib/ansible/module_utils/connection.py

@@ -29,8 +29,8 @@
 from __future__ import annotations
 
 import os
-import hashlib
 import json
+import pickle
 import socket
 import struct
 import traceback
@@ -40,30 +40,14 @@ from functools import partial
 from ansible.module_utils.common.text.converters import to_bytes, to_text
 from ansible.module_utils.common.json import AnsibleJSONEncoder
 from ansible.module_utils.six import iteritems
-from ansible.module_utils.six.moves import cPickle
 
 
-def write_to_file_descriptor(fd, obj):
+def write_to_stream(stream, obj):
-    """Handles making sure all data is properly written to file descriptor fd.
+    """Write a length+newline-prefixed pickled object to a stream."""
+    src = pickle.dumps(obj)
 
-    In particular, that data is encoded in a character stream-friendly way and
+    stream.write(b'%d\n' % len(src))
-    that all data gets written before returning.
+    stream.write(src)
-    """
-    # Need to force a protocol that is compatible with both py2 and py3.
-    # That would be protocol=2 or less.
-    # Also need to force a protocol that excludes certain control chars as
-    # stdin in this case is a pty and control chars will cause problems.
-    # that means only protocol=0 will work.
-    src = cPickle.dumps(obj, protocol=0)
-
-    # raw \r characters will not survive pty round-trip
-    # They should be rehydrated on the receiving end
-    src = src.replace(b'\r', br'\r')
-    data_hash = to_bytes(hashlib.sha1(src).hexdigest())
-
-    os.write(fd, b'%d\n' % len(src))
-    os.write(fd, src)
-    os.write(fd, b'%s\n' % data_hash)
 
 
 def send_data(s, data):
@@ -146,7 +130,7 @@ class Connection(object):
             data = json.dumps(req, cls=AnsibleJSONEncoder, vault_to_text=True)
         except TypeError as exc:
             raise ConnectionError(
-                "Failed to encode some variables as JSON for communication with ansible-connection. "
+                "Failed to encode some variables as JSON for communication with the persistent connection helper. "
                 "The original exception was: %s" % to_text(exc)
             )
 
@@ -176,7 +160,7 @@ class Connection(object):
         if response['id'] != reqid:
             raise ConnectionError('invalid json-rpc id received')
         if "result_type" in response:
-            response["result"] = cPickle.loads(to_bytes(response["result"]))
+            response["result"] = pickle.loads(to_bytes(response["result"], errors="surrogateescape"))
 
         return response
 

lib/ansible/utils/display.py

@@ -424,7 +424,7 @@ class Display(metaclass=Singleton):
                 msg2 = msg2 + u'\n'
 
             # Note: After Display() class is refactored need to update the log capture
-            # code in 'bin/ansible-connection' (and other relevant places).
+            # code in 'cli/scripts/ansible_connection_cli_stub.py' (and other relevant places).
             if not stderr:
                 fileobj = sys.stdout
             else:

lib/ansible/utils/jsonrpc.py

@@ -83,7 +83,7 @@ class JsonRpcServer(object):
             result = to_text(result)
         if not isinstance(result, text_type):
             response["result_type"] = "pickle"
-            result = to_text(pickle.dumps(result, protocol=0))
+            result = to_text(pickle.dumps(result), errors='surrogateescape')
         response['result'] = result
         return response
 

setup.cfg

@@ -100,7 +100,6 @@ ansible_test =
 #     ansible-playbook = ansible.cli.playbook:main
 #     ansible-pull = ansible.cli.pull:main
 #     ansible-vault = ansible.cli.vault:main
-#     ansible-connection = ansible.cli.scripts.ansible_connection_cli_stub:main
 #     ansible-test = ansible_test._util.target.cli.ansible_test_cli_stub:main
 
 [flake8]

setup.py

@@ -24,7 +24,6 @@ setup(
             'ansible-playbook=ansible.cli.playbook:main',
             'ansible-pull=ansible.cli.pull:main',
             'ansible-vault=ansible.cli.vault:main',
-            'ansible-connection=ansible.cli.scripts.ansible_connection_cli_stub:main',
         ],
     },
 )

test/integration/targets/packaging_cli-doc/verify.py

@@ -6,7 +6,6 @@ import pathlib
 import sys
 
 exclude_programs = {
-    'ansible-connection',
     'ansible-test',
 }
 

test/lib/ansible_test/_internal/ansible_util.py

@@ -115,14 +115,16 @@ def ansible_environment(args: CommonConfig, color: bool = True, ansible_config:
         # enabled even when not using code coverage to surface warnings when worker processes do not exit cleanly
         ANSIBLE_WORKER_SHUTDOWN_POLL_COUNT='100',
         ANSIBLE_WORKER_SHUTDOWN_POLL_DELAY='0.1',
+        # ansible-test specific environment variables require an 'ANSIBLE_TEST_' prefix to distinguish them from ansible-core env vars defined by config
+        ANSIBLE_TEST_ANSIBLE_LIB_ROOT=ANSIBLE_LIB_ROOT,  # used by the coverage injector
     )
 
     if isinstance(args, IntegrationConfig) and args.coverage:
-        # standard path injection is not effective for ansible-connection, instead the location must be configured
+        # standard path injection is not effective for the persistent connection helper, instead the location must be configured
-        # ansible-connection only requires the injector for code coverage
+        # it only requires the injector for code coverage
         # the correct python interpreter is already selected using the sys.executable used to invoke ansible
         ansible.update(
-            ANSIBLE_CONNECTION_PATH=os.path.join(get_injector_path(), 'ansible-connection'),
+            _ANSIBLE_CONNECTION_PATH=os.path.join(get_injector_path(), 'ansible_connection_cli_stub.py'),
         )
 
     if isinstance(args, PosixIntegrationConfig):

test/lib/ansible_test/_internal/constants.py

@@ -37,7 +37,6 @@ SECCOMP_CHOICES = [
 ANSIBLE_BIN_SYMLINK_MAP = {
     'ansible': '../lib/ansible/cli/adhoc.py',
     'ansible-config': '../lib/ansible/cli/config.py',
-    'ansible-connection': '../lib/ansible/cli/scripts/ansible_connection_cli_stub.py',
     'ansible-console': '../lib/ansible/cli/console.py',
     'ansible-doc': '../lib/ansible/cli/doc.py',
     'ansible-galaxy': '../lib/ansible/cli/galaxy.py',

test/lib/ansible_test/_internal/util_common.py

@@ -300,6 +300,7 @@ def get_injector_path() -> str:
     injector_names = sorted(list(ANSIBLE_BIN_SYMLINK_MAP) + [
         'importer.py',
         'pytest',
+        'ansible_connection_cli_stub.py',
     ])
 
     scripts = (

test/lib/ansible_test/_util/target/injector/python.py

@@ -6,12 +6,15 @@ import importlib.util
 import os
 import sys
 
+NETWORKING_CLI_STUB_SCRIPT = 'ansible_connection_cli_stub.py'
+
 
 def main():
     """Main entry point."""
     name = os.path.basename(__file__)
     args = [sys.executable]
 
+    ansible_lib_root = os.environ.get('ANSIBLE_TEST_ANSIBLE_LIB_ROOT')
     coverage_config = os.environ.get('COVERAGE_CONF')
     coverage_output = os.environ.get('COVERAGE_FILE')
 
@@ -33,6 +36,8 @@ def main():
         args += ['-m', 'pytest']
     elif name == 'importer.py':
         args += [find_program(name, False)]
+    elif name == NETWORKING_CLI_STUB_SCRIPT:
+        args += [os.path.join(ansible_lib_root, 'cli/scripts', NETWORKING_CLI_STUB_SCRIPT)]
     else:
         args += [find_program(name, True)]
 

test/lib/ansible_test/config/config.yml

@@ -17,7 +17,7 @@ modules:
   #                   This is the default value if no configuration is provided.
   #  - 'controller' - All Python versions supported by the Ansible controller.
   #                   This indicates the modules/module_utils can only run on the controller.
-  #                   Intended for use only with modules/module_utils that depend on ansible-connection, which only runs on the controller.
+  #                   Intended for use only with modules/module_utils that depend on the Ansible persistent connection helper, which only runs on the controller.
   #                   Unit tests for modules/module_utils will be permitted to import any Ansible code, instead of only module_utils.
   #  - SpecifierSet - A PEP 440 specifier set indicating the supported Python versions.
   #                   This is only needed when modules/module_utils do not support all Python versions supported by Ansible.

test/sanity/ignore.txt

@@ -1,4 +1,3 @@
-lib/ansible/cli/scripts/ansible_connection_cli_stub.py shebang
 lib/ansible/config/base.yml no-unwanted-files
 lib/ansible/executor/powershell/async_watchdog.ps1 pslint:PSCustomUseLiteralPath
 lib/ansible/executor/powershell/async_wrapper.ps1 pslint:PSCustomUseLiteralPath