Validate inventory from CLI early (#1307)

* Handle logging early in init_runner()

* Fix pylint error for cmdline()

* Validate inventory from command line

* Add documentation for --inventory

docs/standalone.rst

@@ -96,6 +96,33 @@ Ansible/Runner doesn't pollute or overwrite the playbook content you can give a
 **Runner** will copy the project directory to a temporary directory created under that path, set it as the working directory, and execute from that location.
 After running that temp directory will be cleaned up and removed.
 
+
+Specifying an Alternate Inventory
+---------------------------------
+
+The default inventory, if not specified, will be ``<private_data_dir>/inventory/``.
+All files within this subdirectory of the private data directory will be processed as
+potential inventory host files. You may specify a different inventory using the ``--inventory``
+option. This value may be one of:
+
+  - A file name located within ``<private_data_dir>/inventory/``.
+  - An absolute or relative path to an alternate inventory file or directory.
+    This path is not required to be inside of the private data directory.
+
+Examples::
+
+  # Use inventory <private_data_dir>/inventory/hosts.backup
+  $ ansible-runner run demo -p test.yml --inventory hosts.backup
+
+  # Use inventory in the /path/to/alternate-inventory directory (outside of <private_data_dir>)
+  $ ansible-runner run demo -p test.yml --inventory /path/to/alternate-inventory
+
+  # Use inventory in the inventory2 subdirectory, relative to current directory
+  $ ansible-runner run demo -p test.yml --inventory inventory2
+
+.. note:: This option has no effect when using process isolation.
+
+
 Outputting json (raw event data) to the console instead of normal output
 ------------------------------------------------------------------------
 

src/ansible_runner/__main__.py

@@ -16,6 +16,8 @@
 # specific language governing permissions and limitations
 # under the License.
 #
+from __future__ import annotations
+
 import ast
 import threading
 import traceback
@@ -32,6 +34,7 @@
 import tempfile
 
 from contextlib import contextmanager
+from pathlib import Path
 from uuid import uuid4
 
 import daemon
@@ -45,7 +48,6 @@
 from ansible_runner.utils.capacity import get_cpu_count, get_mem_in_bytes, ensure_uuid
 from ansible_runner.utils.importlib_compat import importlib_metadata
 from ansible_runner.runner import Runner
-from ansible_runner.exceptions import AnsibleRunnerException
 
 VERSION = importlib_metadata.version("ansible_runner")
 
@@ -437,11 +439,8 @@ def role_manager(vargs):
         output.debug(f"using playbook file {playbook}")
 
         if vargs.get('inventory'):
-            inventory_file = os.path.join(vargs.get('private_data_dir'), 'inventory', vargs.get('inventory'))
-            if not os.path.exists(inventory_file):
-                raise AnsibleRunnerException('location specified by --inventory does not exist')
-            kwargs.inventory = inventory_file
-            output.debug(f"using inventory file {inventory_file}")
+            kwargs.inventory = vargs.get('inventory')
+            output.debug(f"using inventory file {kwargs.inventory}")
 
         roles_path = vargs.get('roles_path') or os.path.join(vargs.get('private_data_dir'), 'roles')
         roles_path = os.path.abspath(roles_path)
@@ -519,6 +518,34 @@ def add_args_to_parser(parser, args):
         parser.add_argument(*arg[0], **arg[1])
 
 
+def valid_inventory(private_data_dir: str, inventory: str) -> str | None:
+    """
+    Validate the --inventory value is an actual file or directory.
+
+    The inventory value from the CLI may only be an existing file. Validate it
+    exists. Supplied value may either be relative to <private_data_dir>/inventory/
+    or an absolute path to a file or directory (even outside of private_data_dir).
+    Since ansible itself accepts a file or directory for the inventory, we check
+    for either.
+
+    :return: Absolute path to the valid inventory, or None otherwise.
+    """
+
+    # check if absolute or relative path exists
+    inv = Path(inventory)
+    if inv.exists() and (inv.is_file() or inv.is_dir()):
+        return str(inv.absolute())
+
+    # check for a file in the pvt_data_dir inventory subdir
+    inv_subdir_path = Path(private_data_dir, 'inventory', inv)
+    if (not inv.is_absolute()
+            and inv_subdir_path.exists()
+            and (inv_subdir_path.is_file() or inv_subdir_path.is_dir())):
+        return str(inv_subdir_path.absolute())
+
+    return None
+
+
 def main(sys_args=None):
     """Main entry point for ansible-runner executable
 
@@ -789,6 +816,11 @@ def main(sys_args=None):
             parser.exit(status=1, message="The --hosts option can only be used with -m or -r\n")
         if not (vargs.get('module') or vargs.get('role')) and not vargs.get('playbook'):
             parser.exit(status=1, message="The -p option must be specified when not using -m or -r\n")
+        if vargs.get('inventory'):
+            if not (abs_inv := valid_inventory(vargs['private_data_dir'], vargs.get('inventory'))):
+                parser.exit(status=1, message="Value for --inventory does not appear to be a valid path.\n")
+            else:
+                vargs['inventory'] = abs_inv
 
     output.configure()
 

src/ansible_runner/interface.py

@@ -50,6 +50,19 @@ def init_runner(**kwargs):
 
     See parameters given to :py:func:`ansible_runner.interface.run`
     '''
+
+    # Handle logging first thing
+    debug = kwargs.pop('debug', None)
+    logfile = kwargs.pop('logfile', None)
+
+    if not kwargs.pop("ignore_logging", True):
+        output.configure()
+        if debug in (True, False):
+            output.set_debug('enable' if debug is True else 'disable')
+
+        if logfile:
+            output.set_logfile(logfile)
+
     # If running via the transmit-worker-process method, we must only extract things as read-only
     # inside of one of these commands. That could be either transmit or worker.
     if kwargs.get('streamer') not in ('worker', 'process'):
@@ -72,17 +85,6 @@ def init_runner(**kwargs):
         if os.path.isabs(roles_path) and roles_path.startswith(private_data_dir):
             kwargs['envvars']['ANSIBLE_ROLES_PATH'] = os.path.relpath(roles_path, private_data_dir)
 
-    debug = kwargs.pop('debug', None)
-    logfile = kwargs.pop('logfile', None)
-
-    if not kwargs.pop("ignore_logging", True):
-        output.configure()
-        if debug in (True, False):
-            output.set_debug('enable' if debug is True else 'disable')
-
-        if logfile:
-            output.set_logfile(logfile)
-
     event_callback_handler = kwargs.pop('event_handler', None)
     status_callback_handler = kwargs.pop('status_handler', None)
     artifacts_handler = kwargs.pop('artifacts_handler', None)

test/integration/test___main__.py

@@ -27,9 +27,9 @@ def random_json(keys=None):
 
 
 def cmdline(command, *args):
-    cmdline = ['ansible-runner', command]
-    cmdline.extend(args)
-    sys.argv = cmdline
+    cmd = ['ansible-runner', command]
+    cmd.extend(args)
+    sys.argv = cmd
 
 
 def test_main_bad_private_data_dir():
@@ -131,7 +131,8 @@ def test_cmdline_playbook_hosts():
     cmdline('run', 'private_data_dir', '-p', 'fake', '--hosts', 'all')
     with pytest.raises(SystemExit) as exc:
         ansible_runner.__main__.main()
-        assert exc == 1
+
+    assert exc.value.code == 1
 
 
 def test_cmdline_includes_one_option():
@@ -140,7 +141,8 @@ def test_cmdline_includes_one_option():
     cmdline('run', 'private_data_dir')
     with pytest.raises(SystemExit) as exc:
         ansible_runner.__main__.main()
-        assert exc == 1
+
+    assert exc.value.code == 1
 
 
 def test_cmdline_cmdline_override(tmp_path):
@@ -162,3 +164,19 @@ def test_cmdline_cmdline_override(tmp_path):
 
     cmdline('run', str(private_data_dir), '-p', str(playbook), '--cmdline', '-e foo=bar')
     assert ansible_runner.__main__.main() == 0
+
+
+def test_cmdline_invalid_inventory(tmp_path):
+    """
+    Test that an invalid inventory path causes an error.
+    """
+    private_data_dir = tmp_path
+    inv_path = private_data_dir / 'inventory'
+    inv_path.mkdir(parents=True)
+
+    cmdline('run', str(private_data_dir), '-p', 'test.yml', '--inventory', 'badInventoryPath')
+
+    with pytest.raises(SystemExit) as exc:
+        ansible_runner.__main__.main()
+
+    assert exc.value.code == 1

test/integration/test_main.py

@@ -1,14 +1,12 @@
 # -*- coding: utf-8 -*-
 import multiprocessing
 
-from ansible_runner.__main__ import main
+from test.utils.common import iterate_timeout
 
 import pytest
 import yaml
 
-
-from ansible_runner.exceptions import AnsibleRunnerException
-from test.utils.common import iterate_timeout
+from ansible_runner.__main__ import main
 
 
 @pytest.mark.parametrize(
@@ -102,8 +100,8 @@ def test_role_bad_project_dir(tmp_path, project_fixtures):
 @pytest.mark.parametrize('envvars', [
     {'msg': 'hi'},
     {
-        'msg': u'utf-8-䉪ቒ칸ⱷ?噂폄蔆㪗輥',
-        u'蔆㪗輥': u'䉪ቒ칸'
+        'msg': 'utf-8-䉪ቒ칸ⱷ?噂폄蔆㪗輥',
+        '蔆㪗輥': '䉪ቒ칸'
     }],
     ids=['regular-text', 'utf-8-text']
 )
@@ -143,12 +141,13 @@ def test_role_run_inventory(project_fixtures):
 
 
 def test_role_run_inventory_missing(project_fixtures):
-    with pytest.raises(AnsibleRunnerException):
+    with pytest.raises(SystemExit) as exc:
         main(['run', '-r', 'benthomasson.hello_role',
               '--hosts', 'testhost',
               '--roles-path', str(project_fixtures / 'use_role' / 'roles'),
               '--inventory', 'does_not_exist',
               str(project_fixtures / 'use_role')])
+    assert exc.value.code == 1
 
 
 def test_role_start(project_fixtures):

test/unit/test__main__.py

@@ -0,0 +1,52 @@
+from ansible_runner.__main__ import valid_inventory
+
+
+def test_valid_inventory_file_in_inventory(tmp_path):
+    """
+    Test a relative file name within inventory subdir.
+    """
+    data_dir = tmp_path / "datadir"
+    inv_dir = data_dir / "inventory"
+    inv_dir.mkdir(parents=True)
+
+    hosts = inv_dir / "hosts.xyz"
+    hosts.touch()
+
+    assert valid_inventory(str(data_dir), "hosts.xyz") == str(hosts.absolute())
+
+
+def test_valid_inventory_absolute_path_to_file(tmp_path):
+    """
+    Test an absolute path to a file outside of data dir.
+    """
+    data_dir = tmp_path / "datadir"
+    inv_dir = data_dir / "inventory"
+    inv_dir.mkdir(parents=True)
+
+    (tmp_path / "otherdir").mkdir()
+    hosts = tmp_path / "otherdir" / "hosts.xyz"
+    hosts.touch()
+
+    assert valid_inventory(str(data_dir), str(hosts.absolute())) == str(hosts.absolute())
+
+
+def test_valid_inventory_absolute_path_to_directory(tmp_path):
+    """
+    Test an absolute path to a directory outside of data dir.
+    """
+    data_dir = tmp_path / "datadir"
+    inv_dir = data_dir / "inventory"
+    inv_dir.mkdir(parents=True)
+
+    (tmp_path / "otherdir").mkdir()
+    hosts = tmp_path / "otherdir"
+    hosts.touch()
+
+    assert valid_inventory(str(data_dir), str(hosts.absolute())) == str(hosts.absolute())
+
+
+def test_valid_inventory_doesnotexist(tmp_path):
+    """
+    Test that a bad inventory path returns False.
+    """
+    assert valid_inventory(str(tmp_path), "doesNotExist") is None