ansible-test - Fix subprocess management. (#77641)

* Run code-smell sanity tests in UTF-8 Mode.
* Update subprocess use in sanity test programs.
* Use raw_command instead of run_command with always=True set.
* Add more capture=True usage.
* Don't expose stdin to subprocesses.
* Capture more output. Warn on retry.
* Add more captures.
* Capture coverage cli output.
* Capture windows and network host checks.
* Be explicit about interactive usage.
* Use a shell for non-captured, non-interactive subprocesses.
* Add integration test to assert no TTY.
* Add unit test to assert no TTY.
* Require blocking stdin/stdout/stderr.
* Use subprocess.run in ansible-core sanity tests.
* Remove unused arg.
* Be explicit with subprocess.run check=False.
* Add changelog.
* Use a Python subprocess instead of a shell.
* Use InternalError instead of Exception.
* Require capture argument.
* Check for invalid raw_command arguments.
* Removed pointless communicate=True usage.
* Relocate stdout w/o capture check.
* Use threads instead of a subprocess for IO.

changelogs/fragments/ansible-test-subprocess-isolation.yml

@@ -0,0 +1,10 @@
+bugfixes:
+  - ansible-test - Subprocesses are now isolated from the stdin, stdout and stderr of ansible-test.
+                   This avoids issues with subprocesses tampering with the file descriptors, such as SSH making them non-blocking.
+                   As a result of this change, subprocess output from unit and integration tests on stderr now go to stdout.
+  - ansible-test - Subprocesses no longer have access to the TTY ansible-test is connected to, if any.
+                   This maintains consistent behavior between local testing and CI systems, which typically do not provide a TTY.
+                   Tests which require a TTY should use pexpect or another mechanism to create a PTY.
+minor_changes:
+  - ansible-test - Blocking mode is now enforced for stdin, stdout and stderr.
+                   If any of these are non-blocking then ansible-test will exit during startup with an error.

test/integration/targets/ansible-test-no-tty/aliases

@@ -0,0 +1,2 @@
+context/controller
+shippable/posix/group1

test/integration/targets/ansible-test-no-tty/runme.py

@@ -0,0 +1,7 @@
+#!/usr/bin/env python
+
+import sys
+
+assert not sys.stdin.isatty()
+assert not sys.stdout.isatty()
+assert not sys.stderr.isatty()

test/integration/targets/ansible-test-no-tty/runme.sh

@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+set -eux
+
+./runme.py

test/lib/ansible_test/_internal/ansible_util.py

@@ -22,11 +22,11 @@
     ANSIBLE_SOURCE_ROOT,
     ANSIBLE_TEST_TOOLS_ROOT,
     get_ansible_version,
+    raw_command,
 )
 
 from .util_common import (
     create_temp_dir,
-    run_command,
     ResultType,
     intercept_python,
     get_injector_path,
@@ -258,12 +258,12 @@ def __init__(self, reason):  # type: (str) -> None
         self.reason = reason
 
 
-def get_collection_detail(args, python):  # type: (EnvironmentConfig, PythonConfig) -> CollectionDetail
+def get_collection_detail(python):  # type: (PythonConfig) -> CollectionDetail
     """Return collection detail."""
     collection = data_context().content.collection
     directory = os.path.join(collection.root, collection.directory)
 
-    stdout = run_command(args, [python.path, os.path.join(ANSIBLE_TEST_TOOLS_ROOT, 'collection_detail.py'), directory], capture=True, always=True)[0]
+    stdout = raw_command([python.path, os.path.join(ANSIBLE_TEST_TOOLS_ROOT, 'collection_detail.py'), directory], capture=True)[0]
     result = json.loads(stdout)
     error = result.get('error')
 
@@ -282,15 +282,15 @@ def run_playbook(
         args,  # type: EnvironmentConfig
         inventory_path,  # type: str
         playbook,   # type: str
-        run_playbook_vars=None,  # type: t.Optional[t.Dict[str, t.Any]]
-        capture=False,  # type: bool
+        capture,  # type: bool
+        variables=None,  # type: t.Optional[t.Dict[str, t.Any]]
 ):  # type: (...) -> None
     """Run the specified playbook using the given inventory file and playbook variables."""
     playbook_path = os.path.join(ANSIBLE_TEST_DATA_ROOT, 'playbooks', playbook)
     cmd = ['ansible-playbook', '-i', inventory_path, playbook_path]
 
-    if run_playbook_vars:
-        cmd.extend(['-e', json.dumps(run_playbook_vars)])
+    if variables:
+        cmd.extend(['-e', json.dumps(variables)])
 
     if args.verbosity:
         cmd.append('-%s' % ('v' * args.verbosity))

test/lib/ansible_test/_internal/commands/coverage/__init__.py

@@ -100,7 +100,16 @@ def run_coverage(args, host_state, output_file, command, cmd):  # type: (Coverag
 
     cmd = ['python', '-m', 'coverage.__main__', command, '--rcfile', COVERAGE_CONFIG_PATH] + cmd
 
-    intercept_python(args, host_state.controller_profile.python, cmd, env)
+    stdout, stderr = intercept_python(args, host_state.controller_profile.python, cmd, env, capture=True)
+
+    stdout = (stdout or '').strip()
+    stderr = (stderr or '').strip()
+
+    if stdout:
+        display.info(stdout)
+
+    if stderr:
+        display.warning(stderr)
 
 
 def get_all_coverage_files():  # type: () -> t.List[str]

test/lib/ansible_test/_internal/commands/coverage/combine.py

@@ -18,11 +18,11 @@
     ANSIBLE_TEST_TOOLS_ROOT,
     display,
     ApplicationError,
+    raw_command,
 )
 
 from ...util_common import (
     ResultType,
-    run_command,
     write_json_file,
     write_json_test_results,
 )
@@ -196,7 +196,7 @@ def _default_stub_value(source_paths: list[str]) -> dict[str, dict[int, int]]:
         cmd = ['pwsh', os.path.join(ANSIBLE_TEST_TOOLS_ROOT, 'coverage_stub.ps1')]
         cmd.extend(source_paths)
 
-        stubs = json.loads(run_command(args, cmd, capture=True, always=True)[0])
+        stubs = json.loads(raw_command(cmd, capture=True)[0])
 
         return dict((d['Path'], dict((line, 0) for line in d['Lines'])) for d in stubs)
 

test/lib/ansible_test/_internal/commands/integration/__init__.py

@@ -619,7 +619,7 @@ def command_integration_script(
                 cmd += ['-e', '@%s' % config_path]
 
             env.update(coverage_manager.get_environment(target.name, target.aliases))
-            cover_python(args, host_state.controller_profile.python, cmd, target.name, env, cwd=cwd)
+            cover_python(args, host_state.controller_profile.python, cmd, target.name, env, cwd=cwd, capture=False)
 
 
 def command_integration_role(
@@ -738,7 +738,7 @@ def command_integration_role(
             env['ANSIBLE_ROLES_PATH'] = test_env.targets_dir
 
             env.update(coverage_manager.get_environment(target.name, target.aliases))
-            cover_python(args, host_state.controller_profile.python, cmd, target.name, env, cwd=cwd)
+            cover_python(args, host_state.controller_profile.python, cmd, target.name, env, cwd=cwd, capture=False)
 
 
 def run_setup_targets(

test/lib/ansible_test/_internal/commands/integration/cloud/cs.py

@@ -106,7 +106,7 @@ def _setup_dynamic(self):  # type: () -> None
 
         # apply work-around for OverlayFS issue
         # https://github.com/docker/for-linux/issues/72#issuecomment-319904698
-        docker_exec(self.args, self.DOCKER_SIMULATOR_NAME, ['find', '/var/lib/mysql', '-type', 'f', '-exec', 'touch', '{}', ';'])
+        docker_exec(self.args, self.DOCKER_SIMULATOR_NAME, ['find', '/var/lib/mysql', '-type', 'f', '-exec', 'touch', '{}', ';'], capture=True)
 
         if self.args.explain:
             values = dict(

test/lib/ansible_test/_internal/commands/integration/coverage.py

@@ -118,7 +118,7 @@ def get_environment(self, target_name, aliases):  # type: (str, t.Tuple[str, ...
     def run_playbook(self, playbook, variables):  # type: (str, t.Dict[str, str]) -> None
         """Run the specified playbook using the current inventory."""
         self.create_inventory()
-        run_playbook(self.args, self.inventory_path, playbook, variables)
+        run_playbook(self.args, self.inventory_path, playbook, capture=False, variables=variables)
 
 
 class PosixCoverageHandler(CoverageHandler[PosixConfig]):

test/lib/ansible_test/_internal/commands/sanity/__init__.py

@@ -952,6 +952,7 @@ def test(self, args, targets, python):  # type: (SanityConfig, SanityTargets, Py
         cmd = [python.path, self.path]
 
         env = ansible_environment(args, color=False)
+        env.update(PYTHONUTF8='1')  # force all code-smell sanity tests to run with Python UTF-8 Mode enabled
 
         pattern = None
         data = None

test/lib/ansible_test/_internal/commands/sanity/pylint.py

@@ -141,7 +141,7 @@ def context_filter(path_to_filter):  # type: (str) -> bool
 
         if data_context().content.collection:
             try:
-                collection_detail = get_collection_detail(args, python)
+                collection_detail = get_collection_detail(python)
 
                 if not collection_detail.version:
                     display.warning('Skipping pylint collection version checks since no collection version was found.')

test/lib/ansible_test/_internal/commands/sanity/validate_modules.py

@@ -121,7 +121,7 @@ def test(self, args, targets, python):  # type: (SanityConfig, SanityTargets, Py
             cmd.extend(['--collection', data_context().content.collection.directory])
 
             try:
-                collection_detail = get_collection_detail(args, python)
+                collection_detail = get_collection_detail(python)
 
                 if collection_detail.version:
                     cmd.extend(['--collection-version', collection_detail.version])

test/lib/ansible_test/_internal/commands/shell/__init__.py

@@ -2,6 +2,7 @@
 from __future__ import annotations
 
 import os
+import sys
 import typing as t
 
 from ...util import (
@@ -44,6 +45,9 @@ def command_shell(args):  # type: (ShellConfig) -> None
     if args.raw and isinstance(args.targets[0], ControllerConfig):
         raise ApplicationError('The --raw option has no effect on the controller.')
 
+    if not sys.stdin.isatty():
+        raise ApplicationError('Standard input must be a TTY to launch a shell.')
+
     host_state = prepare_profiles(args, skip_setup=args.raw)  # shell
 
     if args.delegate:
@@ -87,4 +91,4 @@ def command_shell(args):  # type: (ShellConfig) -> None
     else:
         cmd = []
 
-    con.run(cmd)
+    con.run(cmd, capture=False, interactive=True)

test/lib/ansible_test/_internal/commands/units/__init__.py

@@ -275,7 +275,7 @@ def command_units(args):  # type: (UnitsConfig) -> None
         display.info('Unit test %s with Python %s' % (test_context, python.version))
 
         try:
-            cover_python(args, python, cmd, test_context, env)
+            cover_python(args, python, cmd, test_context, env, capture=False)
         except SubprocessError as ex:
             # pytest exits with status code 5 when all tests are skipped, which isn't an error for our use case
             if ex.status != 5:

test/lib/ansible_test/_internal/config.py

@@ -238,6 +238,7 @@ def __init__(self, args):  # type: (t.Any) -> None
         super().__init__(args, 'shell')
 
         self.raw = args.raw  # type: bool
+        self.interactive = True
 
 
 class SanityConfig(TestConfig):

test/lib/ansible_test/_internal/connections.py

@@ -3,7 +3,6 @@
 
 import abc
 import shlex
-import sys
 import tempfile
 import typing as t
 
@@ -46,7 +45,8 @@ class Connection(metaclass=abc.ABCMeta):
     @abc.abstractmethod
     def run(self,
             command,  # type: t.List[str]
-            capture=False,  # type: bool
+            capture,  # type: bool
+            interactive=False,  # type: bool
             data=None,  # type: t.Optional[str]
             stdin=None,  # type: t.Optional[t.IO[bytes]]
             stdout=None,  # type: t.Optional[t.IO[bytes]]
@@ -60,7 +60,7 @@ def extract_archive(self,
         """Extract the given archive file stream in the specified directory."""
         tar_cmd = ['tar', 'oxzf', '-', '-C', chdir]
 
-        retry(lambda: self.run(tar_cmd, stdin=src))
+        retry(lambda: self.run(tar_cmd, stdin=src, capture=True))
 
     def create_archive(self,
                        chdir,  # type: str
@@ -82,7 +82,7 @@ def create_archive(self,
 
         sh_cmd = ['sh', '-c', ' | '.join(' '.join(shlex.quote(cmd) for cmd in command) for command in commands)]
 
-        retry(lambda: self.run(sh_cmd, stdout=dst))
+        retry(lambda: self.run(sh_cmd, stdout=dst, capture=True))
 
 
 class LocalConnection(Connection):
@@ -92,7 +92,8 @@ def __init__(self, args):  # type: (EnvironmentConfig) -> None
 
     def run(self,
             command,  # type: t.List[str]
-            capture=False,  # type: bool
+            capture,  # type: bool
+            interactive=False,  # type: bool
             data=None,  # type: t.Optional[str]
             stdin=None,  # type: t.Optional[t.IO[bytes]]
             stdout=None,  # type: t.Optional[t.IO[bytes]]
@@ -105,6 +106,7 @@ def run(self,
             data=data,
             stdin=stdin,
             stdout=stdout,
+            interactive=interactive,
         )
 
 
@@ -130,7 +132,8 @@ def __init__(self, args, settings, become=None):  # type: (EnvironmentConfig, Ss
 
     def run(self,
             command,  # type: t.List[str]
-            capture=False,  # type: bool
+            capture,  # type: bool
+            interactive=False,  # type: bool
             data=None,  # type: t.Optional[str]
             stdin=None,  # type: t.Optional[t.IO[bytes]]
             stdout=None,  # type: t.Optional[t.IO[bytes]]
@@ -143,7 +146,7 @@ def run(self,
 
         options.append('-q')
 
-        if not data and not stdin and not stdout and sys.stdin.isatty():
+        if interactive:
             options.append('-tt')
 
         with tempfile.NamedTemporaryFile(prefix='ansible-test-ssh-debug-', suffix='.log') as ssh_logfile:
@@ -166,6 +169,7 @@ def error_callback(ex):  # type: (SubprocessError) -> None
                 data=data,
                 stdin=stdin,
                 stdout=stdout,
+                interactive=interactive,
                 error_callback=error_callback,
             )
 
@@ -208,7 +212,8 @@ def __init__(self, args, container_id, user=None):  # type: (EnvironmentConfig,
 
     def run(self,
             command,  # type: t.List[str]
-            capture=False,  # type: bool
+            capture,  # type: bool
+            interactive=False,  # type: bool
             data=None,  # type: t.Optional[str]
             stdin=None,  # type: t.Optional[t.IO[bytes]]
             stdout=None,  # type: t.Optional[t.IO[bytes]]
@@ -219,7 +224,7 @@ def run(self,
         if self.user:
             options.extend(['--user', self.user])
 
-        if not data and not stdin and not stdout and sys.stdin.isatty():
+        if interactive:
             options.append('-it')
 
         return docker_exec(
@@ -231,6 +236,7 @@ def run(self,
             data=data,
             stdin=stdin,
             stdout=stdout,
+            interactive=interactive,
         )
 
     def inspect(self):  # type: () -> DockerInspect

test/lib/ansible_test/_internal/containers.py

@@ -794,7 +794,7 @@ def forward_ssh_ports(
     inventory = generate_ssh_inventory(ssh_connections)
 
     with named_temporary_file(args, 'ssh-inventory-', '.json', None, inventory) as inventory_path:  # type: str
-        run_playbook(args, inventory_path, playbook, dict(hosts_entries=hosts_entries))
+        run_playbook(args, inventory_path, playbook, capture=False, variables=dict(hosts_entries=hosts_entries))
 
     ssh_processes = []  # type: t.List[SshProcess]
 
@@ -827,7 +827,7 @@ def cleanup_ssh_ports(
     inventory = generate_ssh_inventory(ssh_connections)
 
     with named_temporary_file(args, 'ssh-inventory-', '.json', None, inventory) as inventory_path:  # type: str
-        run_playbook(args, inventory_path, playbook, dict(hosts_entries=hosts_entries))
+        run_playbook(args, inventory_path, playbook, capture=False, variables=dict(hosts_entries=hosts_entries))
 
     if ssh_processes:
         for process in ssh_processes:

test/lib/ansible_test/_internal/core_ci.py

@@ -469,7 +469,7 @@ def generate_key_pair(self, args):  # type: (EnvironmentConfig) -> t.Tuple[str,
             make_dirs(os.path.dirname(key))
 
         if not os.path.isfile(key) or not os.path.isfile(pub):
-            run_command(args, ['ssh-keygen', '-m', 'PEM', '-q', '-t', self.KEY_TYPE, '-N', '', '-f', key])
+            run_command(args, ['ssh-keygen', '-m', 'PEM', '-q', '-t', self.KEY_TYPE, '-N', '', '-f', key], capture=True)
 
             if args.explain:
                 return key, pub

test/lib/ansible_test/_internal/coverage_util.py

@@ -141,7 +141,7 @@ def cover_python(
         cmd,  # type: t.List[str]
         target_name,  # type: str
         env,  # type: t.Dict[str, str]
-        capture=False,  # type: bool
+        capture,  # type: bool
         data=None,  # type: t.Optional[str]
         cwd=None,  # type: t.Optional[str]
 ):  # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]]

test/lib/ansible_test/_internal/delegation.py

@@ -160,12 +160,13 @@ def delegate_command(args, host_state, exclude, require):  # type: (EnvironmentC
                 os.path.join(content_root, ResultType.COVERAGE.relative_path),
             ]
 
-            con.run(['mkdir', '-p'] + writable_dirs)
-            con.run(['chmod', '777'] + writable_dirs)
-            con.run(['chmod', '755', working_directory])
-            con.run(['chmod', '644', os.path.join(content_root, args.metadata_path)])
-            con.run(['useradd', pytest_user, '--create-home'])
-            con.run(insert_options(command, options + ['--requirements-mode', 'only']))
+            con.run(['mkdir', '-p'] + writable_dirs, capture=True)
+            con.run(['chmod', '777'] + writable_dirs, capture=True)
+            con.run(['chmod', '755', working_directory], capture=True)
+            con.run(['chmod', '644', os.path.join(content_root, args.metadata_path)], capture=True)
+            con.run(['useradd', pytest_user, '--create-home'], capture=True)
+
+            con.run(insert_options(command, options + ['--requirements-mode', 'only']), capture=False)
 
             container = con.inspect()
             networks = container.get_network_names()
@@ -191,7 +192,7 @@ def delegate_command(args, host_state, exclude, require):  # type: (EnvironmentC
         success = False
 
         try:
-            con.run(insert_options(command, options))
+            con.run(insert_options(command, options), capture=False, interactive=args.interactive)
             success = True
         finally:
             if host_delegation: