[InventoryManager] Fix two unhandled exceptions (#73667)

Change:
- Fix regression: unhandled exception when given inventory directory
  is empty or contains empty subdirectories.
- Fix unhandled exception when limit file is actually a directory
  instead of a file.
- Fix inventory tests which previously could never fail due to missing
  `set -e`. Fixed up tests that failed after `set -e` was added. Added
  several tests.

Test Plan:
- New tests
- Fixed existing tests which previously could never fail

Tickets:
- Fixes #73658

Signed-off-by: Rick Elrod <rick@elrod.me>

changelogs/fragments/73658-inventorymanager-throws-on-empty-inventory-dir.yml

@@ -0,0 +1,3 @@
+bugfixes:
+  - InventoryManager - Fix unhandled exception when inventory directory was empty or contained empty subdirectories (https://github.com/ansible/ansible/issues/73658).
+  - InventoryManager - Fix unhandled exception when given limit file was actually a directory.

lib/ansible/inventory/manager.py

@@ -244,6 +244,7 @@ class InventoryManager(object):
         ''' Generate or update inventory for the source provided '''
 
         parsed = False
+        failures = []
         display.debug(u'Examining possible inventory source: %s' % source)
 
         # use binary for path functions
@@ -272,7 +273,6 @@ class InventoryManager(object):
             self._inventory.current_source = source
 
             # try source with each plugin
-            failures = []
             for plugin in self._fetch_inventory_plugins():
 
                 plugin_name = to_text(getattr(plugin, '_load_name', getattr(plugin, '_original_path', '')))
@@ -635,6 +635,8 @@ class InventoryManager(object):
                     b_limit_file = to_bytes(x[1:])
                     if not os.path.exists(b_limit_file):
                         raise AnsibleError(u'Unable to find limit file %s' % b_limit_file)
+                    if not os.path.isfile(b_limit_file):
+                        raise AnsibleError(u'Limit starting with "@" must be a file, not a directory: %s' % b_limit_file)
                     with open(b_limit_file) as fd:
                         results.extend([to_text(l.strip()) for l in fd.read().split("\n")])
                 else:

test/integration/targets/inventory/runme.sh

@@ -1,34 +1,40 @@
 #!/usr/bin/env bash
 
-set -x
+set -eux
 
-empty_limit_file="/tmp/limit_file"
+empty_limit_file="$(mktemp)"
 touch "${empty_limit_file}"
 
+tmpdir="$(mktemp -d)"
+
 cleanup() {
     if [[ -f "${empty_limit_file}" ]]; then
             rm -rf "${empty_limit_file}"
     fi
+    rm -rf "$tmpdir"
 }
 
 trap 'cleanup' EXIT
 
 # https://github.com/ansible/ansible/issues/52152
 # Ensure that non-matching limit causes failure with rc 1
-ansible-playbook -i ../../inventory --limit foo playbook.yml
-if [ "$?" != "1" ]; then
+if ansible-playbook -i ../../inventory --limit foo playbook.yml; then
     echo "Non-matching limit should cause failure"
     exit 1
 fi
 
 # Ensure that non-existing limit file causes failure with rc 1
-ansible-playbook -i ../../inventory --limit @foo playbook.yml
-if [ "$?" != "1" ]; then
+if ansible-playbook -i ../../inventory --limit @foo playbook.yml; then
     echo "Non-existing limit file should cause failure"
     exit 1
 fi
 
-# Ensure that non-matching limit causes failure with rc 1
+if ! ansible-playbook -i ../../inventory --limit @"$tmpdir" playbook.yml 2>&1 | grep 'must be a file'; then
+    echo "Using a directory as a limit file should throw proper AnsibleError"
+    exit 1
+fi
+
+# Ensure that empty limit file does not cause IndexError #59695
 ansible-playbook -i ../../inventory --limit @"${empty_limit_file}" playbook.yml
 
 ansible-playbook -i ../../inventory "$@" strategy.yml
@@ -38,14 +44,44 @@ ANSIBLE_TRANSFORM_INVALID_GROUP_CHARS=never ansible-playbook -i ../../inventory
 # test extra vars
 ansible-inventory -i testhost, -i ./extra_vars_constructed.yml --list -e 'from_extras=hey ' "$@"|grep '"example": "hellohey"'
 
-# test parse inventory fail is not an error per config
+# Do not fail when all inventories fail to parse.
+# Do not fail when any inventory fails to parse.
 ANSIBLE_INVENTORY_UNPARSED_FAILED=False ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=False ansible -m ping localhost -i /idontexist "$@"
 
-# test no inventory parse is an error with var
-[ "$(ANSIBLE_INVENTORY_UNPARSED_FAILED=True ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=False ansible -m ping localhost -i /idontexist)" != "0" ]
+# Fail when all inventories fail to parse.
+# Do not fail when just one inventory fails to parse.
+if ANSIBLE_INVENTORY_UNPARSED_FAILED=True ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=False ansible -m ping localhost -i /idontexist; then
+    echo "All inventories failed/did not exist, should cause failure"
+    echo "ran with: ANSIBLE_INVENTORY_UNPARSED_FAILED=True ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=False"
+    exit 1
+fi
 
-# test single inventory no parse is not an error with var
-ANSIBLE_INVENTORY_UNPARSED_FAILED=True ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=False ansible -m ping localhost -i /idontexist -i ../../invenotory "$@"
+# Same as above but ensuring no failure we *only* fail when all inventories fail to parse.
+# Fail when all inventories fail to parse.
+# Do not fail when just one inventory fails to parse.
+ANSIBLE_INVENTORY_UNPARSED_FAILED=True ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=False ansible -m ping localhost -i /idontexist -i ../../inventory "$@"
+# Fail when all inventories fail to parse.
+# Do not fail when just one inventory fails to parse.
 
-# test single inventory no parse is an error with any var
-[ "$(ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=True ansible -m ping localhost -i /idontexist -i ../../invenotory)" != "0" ]
+# Fail when any inventories fail to parse.
+if ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=True ansible -m ping localhost -i /idontexist -i ../../inventory; then
+    echo "One inventory failed/did not exist, should NOT cause failure"
+    echo "ran with: ANSIBLE_INVENTORY_UNPARSED_FAILED=True ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=False"
+    exit 1
+fi
+
+# Ensure we don't throw when an empty directory is used as inventory
+ansible-playbook -i "$tmpdir" playbook.yml
+
+# Ensure we can use a directory of inventories
+cp ../../inventory "$tmpdir"
+ansible-playbook -i "$tmpdir" playbook.yml
+
+# ... even if it contains another empty directory
+mkdir "$tmpdir/empty"
+ansible-playbook -i "$tmpdir" playbook.yml
+
+if ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=True ansible -m ping localhost -i "$tmpdir"; then
+    echo "Empty directory should cause failure when ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=True"
+    exit 1
+fi