Fix include_role error consitency and add rescueable option (#86012)

* include_role now behaves more like task on error

changes _from errors from syntax to task failures, by default
which makes it more consistent with other existing errors
 * also force 'missing role' to behave as syntax error when false
 * also error when subdir does not exist, previouslly we ignored missing
   file
 * add 'rescuable' toggle to allow user to chose error type

Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>
Co-authored-by: Sloane Hertel <19572925+s-hertel@users.noreply.github.com>

changelogs/fragments/include_tasks_fail_rescue.yml

@@ -0,0 +1,5 @@
+minor_changes:
+  - include_role has new option `rescuable` to allow it to toggle between task failure and syntax errors.
+bugfixes:
+  - include_role would emit a syntax error on X_from options errors, but a task failure when missing a role to make it consistent now it also emits a task failure on missing tasks_from, which makes it subject to error handling in the play.
+  - include_role, would ignore missing X_from files if the subdir (tasks/vars/handlers/defaults) did not exist, now it is a proper error.

lib/ansible/modules/include_role.py

@@ -71,6 +71,12 @@ options:
     type: bool
     default: yes
     version_added: '2.11'
+  rescuable:
+    description:
+      - This toggle allows for errors from the include itself to either be a task failure, which is 'rescuable', or fatal syntax errors.
+    type: bool
+    default: yes
+    version_added: '2.21'
 extends_documentation_fragment:
     - action_common_attributes
     - action_common_attributes.conn
@@ -83,6 +89,8 @@ attributes:
     diff_mode:
         support: none
 notes:
+  - Beginning in ansible 2.21 we have normalized how error types from the include itself are emitted.
+    Using the O(rescuable) option, you can control if it is a task failure or a syntax error.
   - Handlers and are made available to the whole play.
   - After Ansible 2.4, you can use M(ansible.builtin.import_role) for B(static) behaviour and this action for B(dynamic) one.
 seealso:

lib/ansible/playbook/role/__init__.py

@@ -17,6 +17,7 @@
 
 from __future__ import annotations
 
+import functools as _functools
 import os
 import typing as _t
 
@@ -24,7 +25,7 @@ from collections.abc import Container, Mapping, Set, Sequence
 from types import MappingProxyType
 
 from ansible import constants as C
-from ansible.errors import AnsibleError, AnsibleParserError, AnsibleAssertionError
+from ansible.errors import AnsibleError, AnsibleParserError, AnsibleAssertionError, AnsibleActionFail
 from ansible.module_utils.common.sentinel import Sentinel
 from ansible.module_utils.common.text.converters import to_text
 from ansible.playbook.base import Base
@@ -118,13 +119,16 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
                  from_include: bool = False,
                  validate: bool = True,
                  public: bool = None,
-                 static: bool = True) -> None:
+                 static: bool = True,
+                 rescuable: bool = True) -> None:
+
         self._role_name: str = None
         self._role_path: str = None
         self._role_collection: str = None
         self._role_params: dict[str, dict[str, str]] = dict()
         self._loader = None
         self.static: bool = static
+        self._rescuable: bool = rescuable
 
         # includes (static=false) default to private, while imports (static=true) default to public
         # but both can be overridden by global config if set
@@ -165,6 +169,10 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
     def __repr__(self):
         return self.get_name()
 
+    @_functools.cached_property
+    def _FAIL(self):
+        return AnsibleActionFail if self._rescuable else AnsibleParserError
+
     def get_name(self, include_role_fqcn=True):
         if include_role_fqcn:
             return '.'.join(x for x in (self._role_collection, self._role_name) if x)
@@ -198,7 +206,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
         return self._get_hash_dict() == other._get_hash_dict()
 
     @staticmethod
-    def load(role_include, play, parent_role=None, from_files=None, from_include=False, validate=True, public=None, static=True):
+    def load(role_include, play, parent_role=None, from_files=None, from_include=False, validate=True, public=None, static=True, rescuable=True):
         if from_files is None:
             from_files = {}
         try:
@@ -206,7 +214,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
             #  for the in-flight in role cache as a sentinel that we're already trying to load
             #  that role?)
             # see https://github.com/ansible/ansible/issues/61527
-            r = Role(play=play, from_files=from_files, from_include=from_include, validate=validate, public=public, static=static)
+            r = Role(play=play, from_files=from_files, from_include=from_include, validate=validate, public=public, static=static, rescuable=rescuable)
             r._load_role_data(role_include, parent_role=parent_role)
 
             role_path = r.get_role_path()
@@ -428,8 +436,8 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
                 for found in found_files:
 
                     if not is_subpath(found, file_path):
-                        raise AnsibleParserError("Failed loading '%s' for role (%s) as it is not inside the expected role path: '%s'" %
-                                                 (to_text(found), self._role_name, to_text(file_path)))
+                        raise self._FAIL(f"Failed loading '{found!r}' for role ({self._role_name}) "
+                                         f"as it is not inside the expected role path: {file_path!r}")
 
                     new_data = self._loader.load_from_file(found, trusted_as_template=True)
                     if new_data:
@@ -444,7 +452,10 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
 
             elif main is not None:
                 # this won't trigger with default only when <subdir>_from is specified
-                raise AnsibleParserError("Could not find specified file in role: %s/%s" % (subdir, main))
+                raise self._FAIL(f"Could not find specified file in role: {subdir}/{main}")
+        elif main is not None:
+            # this won't trigger with default only when <subdir>_from is specified
+            raise self._FAIL(f"Could not find specified file in role, its '{subdir}/' is not usable.")
 
         return data
 

lib/ansible/playbook/role_include.py

@@ -17,7 +17,7 @@
 from __future__ import annotations
 
 import ansible.constants as C
-from ansible.errors import AnsibleParserError
+from ansible.errors import AnsibleError, AnsibleParserError
 from ansible.playbook.attribute import NonInheritableFieldAttribute
 from ansible.playbook.task_include import TaskInclude
 from ansible.playbook.role import Role
@@ -39,7 +39,7 @@ class IncludeRole(TaskInclude):
 
     BASE = frozenset(('name', 'role'))  # directly assigned
     FROM_ARGS = frozenset(('tasks_from', 'vars_from', 'defaults_from', 'handlers_from'))  # used to populate from dict in role
-    OTHER_ARGS = frozenset(('apply', 'public', 'allow_duplicates', 'rolespec_validate'))  # assigned to matching property
+    OTHER_ARGS = frozenset(('apply', 'public', 'allow_duplicates', 'rolespec_validate', 'rescuable'))  # assigned to matching property
     VALID_ARGS = BASE | FROM_ARGS | OTHER_ARGS  # all valid args
 
     # =================================================================================
@@ -49,6 +49,7 @@ class IncludeRole(TaskInclude):
     # private as this is a 'module options' vs a task property
     allow_duplicates = NonInheritableFieldAttribute(isa='bool', default=True, private=True, always_post_validate=True)
     rolespec_validate = NonInheritableFieldAttribute(isa='bool', default=True, private=True, always_post_validate=True)
+    rescuable = NonInheritableFieldAttribute(isa='bool', default=True, private=True, always_post_validate=True)
 
     def __init__(self, block=None, role=None, task_include=None):
 
@@ -71,7 +72,13 @@ class IncludeRole(TaskInclude):
         else:
             myplay = play
 
-        ri = RoleInclude.load(self._role_name, play=myplay, variable_manager=variable_manager, loader=loader, collection_list=self.collections)
+        try:
+            ri = RoleInclude.load(self._role_name, play=myplay, variable_manager=variable_manager, loader=loader, collection_list=self.collections)
+        except AnsibleError as e:
+            if not self.rescuable:
+                raise AnsibleParserError("Could not include role.") from e
+            raise
+
         ri.vars |= self.vars
 
         if variable_manager is not None:
@@ -82,8 +89,8 @@ class IncludeRole(TaskInclude):
         from_files = templar.template(self._from_files)
 
         # build role
-        actual_role = Role.load(ri, myplay, parent_role=self._parent_role, from_files=from_files,
-                                from_include=True, validate=self.rolespec_validate, public=self.public, static=self.statically_loaded)
+        actual_role = Role.load(ri, myplay, parent_role=self._parent_role, from_files=from_files, from_include=True,
+                                validate=self.rolespec_validate, public=self.public, static=self.statically_loaded, rescuable=self.rescuable)
         actual_role._metadata.allow_duplicates = self.allow_duplicates
 
         # add role to play
@@ -140,13 +147,17 @@ class IncludeRole(TaskInclude):
                 raise AnsibleParserError('Expected a string for %s but got %s instead' % (key, type(args_value)))
             ir._from_files[from_key] = args_value
 
-        # apply is only valid for includes, not imports as they inherit directly
+        # apply and rescuable are only valid for includes, not imports as they inherit directly
         apply_attrs = ir.args.get('apply', {})
         if apply_attrs and ir.action not in C._ACTION_INCLUDE_ROLE:
             raise AnsibleParserError('Invalid options for %s: apply' % ir.action, obj=data)
         elif not isinstance(apply_attrs, dict):
             raise AnsibleParserError('Expected a dict for apply but got %s instead' % type(apply_attrs), obj=data)
 
+        resc_attr = ir.args.get('rescuable', None)
+        if resc_attr and ir.action not in C._ACTION_INCLUDE_ROLE:
+            raise AnsibleParserError(f'Invalid options for {ir.action}: rescuable', obj=data)
+
         # manual list as otherwise the options would set other task parameters we don't want.
         for option in my_arg_names.intersection(IncludeRole.OTHER_ARGS):
             setattr(ir, option, ir.args.get(option))

test/integration/targets/includes/import_no_rescue.yml

@@ -0,0 +1,6 @@
+- hosts: localhost
+  gather_facts: no
+  tasks:
+    - import_role: 
+        name: test_includes
+        rescuable: true

test/integration/targets/includes/include_role_error_handling.yml

@@ -0,0 +1,37 @@
+- hosts: localhost
+  gather_facts: false
+  tasks:
+    - name: test missing role rescue
+      vars:
+        rescue_1: false
+      block:
+        - name: Include a role that doesn't exist
+          include_role:
+            name: missing_role
+            rescuable: '{{ rescueme | default(omit) }}'
+
+      rescue:
+        - set_fact: 
+              rescue_1: true
+      always:
+        - assert:
+            that:
+              - rescue_1 == rescueme|default(True)
+
+    - name: Test _from rescue
+      vars:
+        rescue_2: false
+      block:
+        - name: Include a task file that doesn't exist, but role exists
+          include_role:
+            name: include_roles
+            tasks_from: missing_task_list
+            rescuable: '{{ rescueme | default(omit) }}'
+
+      rescue:
+        - set_fact: 
+              rescue_2: true
+      always:
+        - assert:
+            that:
+              - rescue_2 == rescueme|default(True)

test/integration/targets/includes/include_role_missing.yml

@@ -0,0 +1,24 @@
+- hosts: localhost
+  gather_facts: no
+  tasks:
+  - file:
+      path: roles/rolename
+      state: "{{ item }}"
+    loop:
+      - absent
+      - directory
+
+  - name: ensure we fail when missing both file and tasks/ subdir
+    block:
+    - include_role:
+        name: rolename
+        tasks_from: missing.yml
+    rescue:
+      - set_fact:
+          wefailed: True
+    always:
+      - name: check we actually failed
+        assert:
+          that: 
+            - wefailed is defined
+            - wefailed

test/integration/targets/includes/runme.sh

@@ -18,3 +18,15 @@ ansible-playbook includes_loop_rescue.yml --extra-vars strategy=linear "$@"
 ansible-playbook includes_loop_rescue.yml --extra-vars strategy=free "$@"
 
 ansible-playbook includes_from_dedup.yml -i ../../inventory "$@"
+
+# test 'rescueable' default (true)
+ansible-playbook include_role_error_handling.yml "$@"
+# test 'rescueable' explicit true 
+ansible-playbook include_role_error_handling.yml "$@" -e '{"rescueme": true}'
+# test 'rescueable' explicit false 
+[[ $(ansible-playbook include_role_error_handling.yml "$@" -e '{"rescueme": false}') != 0 ]]
+# ensure imports are not rescuable
+[[ $(ansible-playbook import_no_rescue.yml "$@") != 0 ]]
+
+# test for missing task_from when missing tasks/
+ansible-playbook include_role_missing.yml "$@"