Skip to content

Commit

Permalink
Parser errors from within includes should not be rescueable (#73722)
Browse files Browse the repository at this point in the history
* Parser errors from within includes should not be rescueable
* Also fixes unit tests
Fixes #73657
  • Loading branch information
mkrizek committed Nov 1, 2021
1 parent d23226a commit 47ee282
Show file tree
Hide file tree
Showing 8 changed files with 39 additions and 6 deletions.
2 changes: 2 additions & 0 deletions changelogs/fragments/73657-include-parser-error-fail.yml
@@ -0,0 +1,2 @@
bugfixes:
- Parser errors from within includes should not be rescueable (https://github.com/ansible/ansible/issues/73657)
7 changes: 5 additions & 2 deletions lib/ansible/plugins/strategy/__init__.py
Expand Up @@ -35,7 +35,7 @@

from ansible import constants as C
from ansible import context
from ansible.errors import AnsibleError, AnsibleFileNotFound, AnsibleUndefinedVariable
from ansible.errors import AnsibleError, AnsibleFileNotFound, AnsibleUndefinedVariable, AnsibleParserError
from ansible.executor import action_write_locks
from ansible.executor.play_iterator import IteratingStates, FailedStates
from ansible.executor.process.worker import WorkerProcess
Expand Down Expand Up @@ -945,7 +945,8 @@ def _load_included_file(self, included_file, iterator, is_handler=False):
# first processed, we do so now for each host in the list
for host in included_file._hosts:
self._tqm._stats.increment('ok', host.name)

except AnsibleParserError:
raise
except AnsibleError as e:
if isinstance(e, AnsibleFileNotFound):
reason = "Could not find or access '%s' on the Ansible Controller." % to_text(e.file_name)
Expand Down Expand Up @@ -1061,6 +1062,8 @@ def _do_handler_run(self, handler, handler_name, iterator, play_context, notifie
)
if not result:
break
except AnsibleParserError:
raise
except AnsibleError as e:
for host in included_file._hosts:
iterator.mark_host_failed(host)
Expand Down
4 changes: 3 additions & 1 deletion lib/ansible/plugins/strategy/free.py
Expand Up @@ -34,7 +34,7 @@
import time

from ansible import constants as C
from ansible.errors import AnsibleError
from ansible.errors import AnsibleError, AnsibleParserError
from ansible.playbook.included_file import IncludedFile
from ansible.plugins.loader import action_loader
from ansible.plugins.strategy import StrategyBase
Expand Down Expand Up @@ -257,6 +257,8 @@ def run(self, iterator, play_context):
)
else:
new_blocks = self._load_included_file(included_file, iterator=iterator)
except AnsibleParserError:
raise
except AnsibleError as e:
for host in included_file._hosts:
iterator.mark_host_failed(host)
Expand Down
5 changes: 3 additions & 2 deletions lib/ansible/plugins/strategy/linear.py
Expand Up @@ -32,7 +32,7 @@
'''

from ansible import constants as C
from ansible.errors import AnsibleError, AnsibleAssertionError
from ansible.errors import AnsibleError, AnsibleAssertionError, AnsibleParserError
from ansible.executor.play_iterator import IteratingStates, FailedStates
from ansible.module_utils._text import to_text
from ansible.playbook.block import Block
Expand Down Expand Up @@ -382,7 +382,8 @@ def run(self, iterator, play_context):
else:
all_blocks[host].append(noop_block)
display.debug("done iterating over new_blocks loaded from include file")

except AnsibleParserError:
raise
except AnsibleError as e:
for host in included_file._hosts:
self._tqm._failed_hosts[host.name] = True
Expand Down
8 changes: 8 additions & 0 deletions test/integration/targets/include_import/issue73657.yml
@@ -0,0 +1,8 @@
- hosts: localhost
gather_facts: no
tasks:
- block:
- include_tasks: issue73657_tasks.yml
rescue:
- debug:
msg: SHOULD_NOT_EXECUTE
2 changes: 2 additions & 0 deletions test/integration/targets/include_import/issue73657_tasks.yml
@@ -0,0 +1,2 @@
- wrong.wrong.wrong:
parser: error
4 changes: 4 additions & 0 deletions test/integration/targets/include_import/runme.sh
Expand Up @@ -135,3 +135,7 @@ cat out.txt
test "$(grep out.txt -ce 'In imported playbook')" = 2
test "$(grep out.txt -ce 'In imported tasks')" = 3
test "$(grep out.txt -ce 'In imported role')" = 3

# https://github.com/ansible/ansible/issues/73657
ansible-playbook issue73657.yml 2>&1 | tee issue73657.out
test "$(grep -c 'SHOULD_NOT_EXECUTE' issue73657.out)" = 0
13 changes: 12 additions & 1 deletion test/units/plugins/strategy/test_strategy.py
Expand Up @@ -29,6 +29,7 @@
from ansible.executor.task_result import TaskResult
from ansible.inventory.host import Host
from ansible.module_utils.six.moves import queue as Queue
from ansible.playbook.block import Block
from ansible.playbook.handler import Handler
from ansible.plugins.strategy import StrategyBase

Expand Down Expand Up @@ -464,7 +465,15 @@ def _queue_put(item, *args, **kwargs):
mock_task = MagicMock()
mock_task._block = mock_block
mock_task._role = None
mock_task._parent = None

# NOTE Mocking calls below to account for passing parent_block=ti_copy.build_parent_block()
# into load_list_of_blocks() in _load_included_file. Not doing so meant that retrieving
# `collection` attr from parent would result in getting MagicMock instance
# instead of an empty list.
mock_task._parent = MagicMock()
mock_task.copy.return_value = mock_task
mock_task.build_parent_block.return_value = mock_block
mock_block._get_parent_attribute.return_value = None

mock_iterator = MagicMock()
mock_iterator.mark_host_failed.return_value = None
Expand All @@ -474,6 +483,8 @@ def _queue_put(item, *args, **kwargs):

mock_inc_file._filename = "test.yml"
res = strategy_base._load_included_file(included_file=mock_inc_file, iterator=mock_iterator)
self.assertEqual(len(res), 1)
self.assertTrue(isinstance(res[0], Block))

mock_inc_file._filename = "bad.yml"
res = strategy_base._load_included_file(included_file=mock_inc_file, iterator=mock_iterator)
Expand Down

0 comments on commit 47ee282

Please sign in to comment.