Backport 2.19 broken conditional detection (#86442)

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.

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:

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:

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})'

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

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

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

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'

test/integration/targets/conditionals/runme.sh

@@ -3,3 +3,5 @@
 set -eux
 
 ansible-playbook -i ../../inventory play.yml "$@"
+
+ansible-playbook validate_broken_conditionals.yml "$@"

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'")