From 4bc1bdd93db368f06f738fe0b478c16b3fba327b Mon Sep 17 00:00:00 2001 From: Matt Davis <6775756+nitzmahone@users.noreply.github.com> Date: Thu, 22 Jan 2026 14:25:45 -0800 Subject: [PATCH] Backport 2.19 broken conditional detection (#86442) --- .../fragments/broken_conditional_backport.yml | 5 ++ lib/ansible/config/base.yml | 26 ++++++++ lib/ansible/executor/task_executor.py | 7 +++ lib/ansible/playbook/conditional.py | 62 +++++++++++++++++-- ...roken_conditional_with_approx_location.yml | 6 ++ .../broken_conditional_with_location.yml | 6 ++ .../broken_conditional_without_location.yml | 6 ++ .../integration/targets/conditionals/play.yml | 27 ++++---- .../integration/targets/conditionals/runme.sh | 2 + .../validate_broken_conditionals.yml | 54 ++++++++++++++++ 10 files changed, 182 insertions(+), 19 deletions(-) create mode 100644 changelogs/fragments/broken_conditional_backport.yml create mode 100644 test/integration/targets/conditionals/broken_conditional_with_approx_location.yml create mode 100644 test/integration/targets/conditionals/broken_conditional_with_location.yml create mode 100644 test/integration/targets/conditionals/broken_conditional_without_location.yml create mode 100644 test/integration/targets/conditionals/validate_broken_conditionals.yml diff --git a/changelogs/fragments/broken_conditional_backport.yml b/changelogs/fragments/broken_conditional_backport.yml new file mode 100644 index 00000000000..33c6672755e --- /dev/null +++ b/changelogs/fragments/broken_conditional_backport.yml @@ -0,0 +1,5 @@ +minor_changes: + - conditionals - Conditional expressions (such as ``when`` in playbook tasks or ``that`` in the ``assert`` action) with results other than True or False will issue a deprecation warning. + Such expressions mask subtle errors and will fail by default in Ansible Core >= 2.19. + They should be corrected to always ensure a strictly boolean (True or False) result. + The stricter Ansible Core >= 2.19 failure behavior can be enabled by configuring ``ALLOW_BROKEN_CONDITIONALS`` to False. diff --git a/lib/ansible/config/base.yml b/lib/ansible/config/base.yml index a5790f6d45e..29e3e42bcd7 100644 --- a/lib/ansible/config/base.yml +++ b/lib/ansible/config/base.yml @@ -1,6 +1,32 @@ # Copyright (c) 2017 Ansible Project # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) --- +_DISABLE_BACKPORTED_INSPECTIONS: + name: Disable inspections backported from future Ansible Core releases + default: false + description: + - When true, bypasses backported content inspections from future Ansible Core releases. + - Unsupported, for internal use only. + env: [ { name: _ANSIBLE_DISABLE_BACKPORTED_INSPECTIONS } ] + ini: + - { key: _disable_backported_inspections, section: defaults } + type: boolean + version_added: '2.14' +ALLOW_BROKEN_CONDITIONALS: + name: Allow broken conditionals + default: true + description: + - When true (default), this option allows conditionals with non-boolean results to be used. + - A deprecation warning will be emitted in these cases. + - When false, non-boolean conditionals result in an error. + - Such results often indicate unintentional use of templates where they are not supported, resulting in a conditional that is always true. + - When this option is enabled, conditional expressions which are a literal ``None`` or empty string will evaluate as true for backwards compatibility. + - This configuration was backported from Ansible Core 2.19 to enable detection and correction of problematic conditional expressions that default to an error in Ansible Core 2.19+. + env: [{name: ANSIBLE_ALLOW_BROKEN_CONDITIONALS}] + ini: + - {key: allow_broken_conditionals, section: defaults} + type: boolean + version_added: '2.14' ANSIBLE_HOME: name: The Ansible home path description: diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index 6869fb6dad0..3e50175c5f4 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -4,6 +4,7 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +import contextlib import os import pty import time @@ -693,12 +694,16 @@ class TaskExecutor: if self._task.changed_when is not None and self._task.changed_when: cond = Conditional(loader=self._loader) cond.when = self._task.changed_when + with contextlib.suppress(AttributeError): + cond._ds = self._task._ds # propagate task context for errors result['changed'] = cond.evaluate_conditional(templar, vars_copy) def _evaluate_failed_when_result(result): if self._task.failed_when: cond = Conditional(loader=self._loader) cond.when = self._task.failed_when + with contextlib.suppress(AttributeError): + cond._ds = self._task._ds # propagate task context for errors failed_when_result = cond.evaluate_conditional(templar, vars_copy) result['failed_when_result'] = result['failed'] = failed_when_result else: @@ -763,6 +768,8 @@ class TaskExecutor: if retries > 1: cond = Conditional(loader=self._loader) cond.when = self._task.until or [not result['failed']] + with contextlib.suppress(AttributeError): + cond._ds = self._task._ds if cond.evaluate_conditional(templar, vars_copy): break else: diff --git a/lib/ansible/playbook/conditional.py b/lib/ansible/playbook/conditional.py index 449b4a9c476..5088ea623b3 100644 --- a/lib/ansible/playbook/conditional.py +++ b/lib/ansible/playbook/conditional.py @@ -19,8 +19,10 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +import contextlib import typing as t +from ansible import constants as C from ansible.errors import AnsibleError, AnsibleUndefinedVariable, AnsibleTemplateError from ansible.module_utils.common.text.converters import to_native from ansible.playbook.attribute import FieldAttribute @@ -74,9 +76,12 @@ class Conditional: try: res = self._check_conditional(conditional, templar, all_vars) except AnsibleError as e: + # propagate error `obj` if present, use conditional if position-tagged, fall back to task DS + obj = e.obj or conditional if hasattr(conditional, 'ansible_pos') else getattr(self, '_ds', None) + raise AnsibleError( "The conditional check '%s' failed. The error was: %s" % (to_native(conditional), to_native(e)), - obj=getattr(self, '_ds', None) + obj=obj, ) display.debug("Evaluated conditional (%s): %s" % (conditional, res)) @@ -88,6 +93,7 @@ class Conditional: def _check_conditional(self, conditional: str, templar: Templar, all_vars: dict[str, t.Any]) -> bool: original = conditional templar.available_variables = all_vars + try: if templar.is_template(conditional): display.warning( @@ -101,15 +107,59 @@ class Conditional: elif conditional == "": return False + # these should be module-global, but can't be for esoteric config chicken/egg scenarios + _allow_broken_conditionals = C.config.get_config_value('ALLOW_BROKEN_CONDITIONALS') + _disable_backported_inspections = C.config.get_config_value('_DISABLE_BACKPORTED_INSPECTIONS') + # If the result of the first-pass template render (to resolve inline templates) is marked unsafe, # explicitly fail since the next templating operation would never evaluate if hasattr(conditional, '__UNSAFE__'): raise AnsibleTemplateError('Conditional is marked as unsafe, and cannot be evaluated.') - # NOTE The spaces around True and False are intentional to short-circuit literal_eval for - # jinja2_native=False and avoid its expensive calls. - return templar.template( - "{%% if %s %%} True {%% else %%} False {%% endif %%}" % conditional, - ).strip() == "True" + if _disable_backported_inspections: + # internal escape hatch to restore original conditional wrapper behavior + # NOTE The spaces around True and False are intentional to short-circuit literal_eval for + # jinja2_native=False and avoid its expensive calls. + return templar.template( + "{%% if %s %%} True {%% else %%} False {%% endif %%}" % conditional, + ).strip() == "True" + + result, result_type_name = templar.template(f'{{% set __cres = {conditional} %}}{{{{ [true if __cres else false, __cres.__class__.__name__] }}}}') + + if result_type_name != 'bool': + if _allow_broken_conditionals: + display.deprecated( + msg=f"Conditional result at {self._get_conditional_source_context(conditional)} was of type {result_type_name!r}. " + f"Conditional results should only be True or False. The result was interpreted as {result}.", + version="2.19", + ) + else: + raise AnsibleTemplateError( + message=f"Conditional result was of type {result_type_name!r}. " + "Conditional results must be True or False when `ALLOW_BROKEN_CONDITIONALS` is disabled, " + "which is the default in Ansible Core >= 2.19.", + obj=original, + ) + + return result except AnsibleUndefinedVariable as e: raise AnsibleUndefinedVariable("error while evaluating conditional (%s): %s" % (original, e)) + + def _get_conditional_source_context(self, conditional: str) -> str: + """Utility method to approximate 2.19+ source context for warnings.""" + src: str | None = None + location_label: str | None = None + + # most string expressions should have been tagged by the YAML parser + with contextlib.suppress(AttributeError, ValueError): + src, line, col = conditional.ansible_pos + location_label = "location" + + if not location_label: + # report approximate location from the conditional's task DS, if available + with contextlib.suppress(AttributeError, ValueError): + src, line, col = self._ds.ansible_pos + location_label = "approximate location" + + # display the conditional expression inline only if we have no other source context + return f'{location_label} {src} {line}:{col}' if location_label else f'unknown location (conditional: {conditional!r})' diff --git a/test/integration/targets/conditionals/broken_conditional_with_approx_location.yml b/test/integration/targets/conditionals/broken_conditional_with_approx_location.yml new file mode 100644 index 00000000000..989f076e9d7 --- /dev/null +++ b/test/integration/targets/conditionals/broken_conditional_with_approx_location.yml @@ -0,0 +1,6 @@ +- hosts: localhost + gather_facts: no + tasks: + - name: conditional expression with non-boolean input (inherits task location) + debug: + when: 1 diff --git a/test/integration/targets/conditionals/broken_conditional_with_location.yml b/test/integration/targets/conditionals/broken_conditional_with_location.yml new file mode 100644 index 00000000000..15521bd4cc2 --- /dev/null +++ b/test/integration/targets/conditionals/broken_conditional_with_location.yml @@ -0,0 +1,6 @@ +- hosts: localhost + gather_facts: no + tasks: + - name: conditional expression with non-boolean result (has location) + debug: + when: 1 + 1 diff --git a/test/integration/targets/conditionals/broken_conditional_without_location.yml b/test/integration/targets/conditionals/broken_conditional_without_location.yml new file mode 100644 index 00000000000..081f745302d --- /dev/null +++ b/test/integration/targets/conditionals/broken_conditional_without_location.yml @@ -0,0 +1,6 @@ +- hosts: localhost + gather_facts: no + tasks: + - name: non-task conditional expression with non-boolean input (no location) + assert: + that: 1 diff --git a/test/integration/targets/conditionals/play.yml b/test/integration/targets/conditionals/play.yml index 56ec843854f..58d497b39be 100644 --- a/test/integration/targets/conditionals/play.yml +++ b/test/integration/targets/conditionals/play.yml @@ -652,19 +652,20 @@ - top is skipped - sub is skipped - - name: test that 'comparison expression' item works with_items - assert: - that: - - item - with_items: - - 1 == 1 - - - name: test that 'comparison expression' item works in loop - assert: - that: - - item - loop: - - 1 == 1 +# these tests have never actually worked; they were only asserting a truthy string, not executing an indirected expression +# - name: test that 'comparison expression' item works with_items +# assert: +# that: +# - item +# with_items: +# - 1 == 1 +# +# - name: test that 'comparison expression' item works in loop +# assert: +# that: +# - item +# loop: +# - 1 == 1 - set_fact: sentinel_file: '{{ lookup("env", "OUTPUT_DIR")}}/LOOKUP_SIDE_EFFECT.txt' diff --git a/test/integration/targets/conditionals/runme.sh b/test/integration/targets/conditionals/runme.sh index 4858fbf2a6d..a675d1d806c 100755 --- a/test/integration/targets/conditionals/runme.sh +++ b/test/integration/targets/conditionals/runme.sh @@ -3,3 +3,5 @@ set -eux ansible-playbook -i ../../inventory play.yml "$@" + +ansible-playbook validate_broken_conditionals.yml "$@" diff --git a/test/integration/targets/conditionals/validate_broken_conditionals.yml b/test/integration/targets/conditionals/validate_broken_conditionals.yml new file mode 100644 index 00000000000..1480351744f --- /dev/null +++ b/test/integration/targets/conditionals/validate_broken_conditionals.yml @@ -0,0 +1,54 @@ +- hosts: localhost + gather_facts: no + environment: + ANSIBLE_NOCOLOR: 1 + ANSIBLE_FORCE_COLOR: 0 + ANSIBLE_DEPRECATION_WARNINGS: 1 + tasks: + - name: broken conditional warning with exact location and bypass + command: ansible-playbook broken_conditional_with_location.yml + environment: + - '{{ item.extra_env }}' + loop: + - scenario: default + extra_env: {} + - scenario: explicit + extra_env: + ANSIBLE_ALLOW_BROKEN_CONDITIONALS: 1 + - scenario: bypass + extra_env: + _ANSIBLE_DISABLE_BACKPORTED_INSPECTIONS: 1 + register: result + + - assert: + that: + - result.results[0].stderr | replace('\n', '') is search("result at location.+was of type 'int'") + - result.results[1].stderr | replace('\n', '') is search("result at location.+was of type 'int'") + - result.results[2].stderr | replace('\n', '') is not search("result at location.+was of type 'int'") + + - name: validate broken conditional default warning with approximate location + command: ansible-playbook broken_conditional_with_approx_location.yml + register: result + + - assert: + that: + - result.stderr | replace('\n', '') is search("result at approximate location.+was of type 'int'") + + - name: validate broken conditional without location + command: ansible-playbook broken_conditional_without_location.yml + register: result + + - assert: + that: + - result.stderr | replace('\n', '') is search("result at unknown location.+was of type 'int'") + + - name: validate broken conditional error + command: ansible-playbook broken_conditional_with_location.yml + environment: + ANSIBLE_ALLOW_BROKEN_CONDITIONALS: 0 + register: result + failed_when: result.rc == 0 + + - assert: + that: + - result.stdout | replace('\n', '') is search("conditional check.+failed.+was of type 'int'")