From 9a86f8c10e297049d0a2582a5ce905a9c83600b6 Mon Sep 17 00:00:00 2001 From: Abhijeet Kasurde Date: Mon, 8 Mar 2021 16:25:55 +0530 Subject: [PATCH] [2.10][InventoryManager] Fix two unhandled exceptions (#73798) 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 Signed-off-by: Abhijeet Kasurde (cherry picked from commit fa046d302c0e352a8dfaa8d5d364eeca7e815afd) Co-authored-by: Rick Elrod --- ...ymanager-throws-on-empty-inventory-dir.yml | 3 + lib/ansible/inventory/manager.py | 4 +- test/integration/targets/inventory/runme.sh | 64 +++++++++++++++---- 3 files changed, 56 insertions(+), 15 deletions(-) create mode 100644 changelogs/fragments/73658-inventorymanager-throws-on-empty-inventory-dir.yml diff --git a/changelogs/fragments/73658-inventorymanager-throws-on-empty-inventory-dir.yml b/changelogs/fragments/73658-inventorymanager-throws-on-empty-inventory-dir.yml new file mode 100644 index 00000000000..e01ab713383 --- /dev/null +++ b/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. diff --git a/lib/ansible/inventory/manager.py b/lib/ansible/inventory/manager.py index d908b74e734..27b30da40c8 100644 --- a/lib/ansible/inventory/manager.py +++ b/lib/ansible/inventory/manager.py @@ -243,6 +243,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 @@ -271,7 +272,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', ''))) @@ -632,6 +632,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: diff --git a/test/integration/targets/inventory/runme.sh b/test/integration/targets/inventory/runme.sh index 70565a99a3c..4e42ce7012a 100755 --- a/test/integration/targets/inventory/runme.sh +++ b/test/integration/targets/inventory/runme.sh @@ -1,48 +1,84 @@ #!/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 ANSIBLE_TRANSFORM_INVALID_GROUP_CHARS=always ansible-playbook -i ../../inventory "$@" strategy.yml ANSIBLE_TRANSFORM_INVALID_GROUP_CHARS=never ansible-playbook -i ../../inventory "$@" strategy.yml -# 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