Don't use getattr in _get_parent_attribute to avoid recursion issues (#33595)

* Don't use getattr in _get_parent_attribute to avoid recursion issues

Fixes #23609

* Move extend/prepend to field attribute

Also removes _get_attr* methods that were basically just calling
_get_parent_attribute because it needed to set those params.

Also modifies _get_parent_attribute() to pull those values from the
FieldAttributes instead of using the ones passed into the function.

* Better fixes for _get_parent_attribute

7 files changed, 53 insertions, 54 deletions

diff --git a/lib/ansible/playbook/attribute.py b/lib/ansible/playbook/attribute.py
index 455f7ded19..644b368be6 100644
--- a/lib/ansible/playbook/attribute.py
+++ b/lib/ansible/playbook/attribute.py
@@ -24,8 +24,22 @@ from copy import deepcopy
 
 class Attribute:
 
-    def __init__(self, isa=None, private=False, default=None, required=False, listof=None, priority=0, class_type=None, always_post_validate=False,
-                 inherit=True, alias=None):
+    def __init__(
+        self,
+        isa=None,
+        private=False,
+        default=None,
+        required=False,
+        listof=None,
+        priority=0,
+        class_type=None,
+        always_post_validate=False,
+        inherit=True,
+        alias=None,
+        extend=False,
+        prepend=False,
+    ):
+
         """
         :class:`Attribute` specifies constraints for attributes of objects which
         derive from playbook data.  The attributes of the object are basically
@@ -67,6 +81,8 @@ class Attribute:
         self.always_post_validate = always_post_validate
         self.inherit = inherit
         self.alias = alias
+        self.extend = extend
+        self.prepend = prepend
 
         if default is not None and self.isa in ('list', 'dict', 'set'):
             self.default = deepcopy(default)
diff --git a/lib/ansible/playbook/base.py b/lib/ansible/playbook/base.py
index 878db4820b..d949a53331 100644
--- a/lib/ansible/playbook/base.py
+++ b/lib/ansible/playbook/base.py
@@ -48,12 +48,13 @@ def _generic_g_method(prop_name, self):
 
 def _generic_g_parent(prop_name, self):
     try:
-        value = self._attributes[prop_name]
-        if value is None and not self._squashed and not self._finalized:
+        if self._squashed or self._finalized:
+            value = self._attributes[prop_name]
+        else:
             try:
                 value = self._get_parent_attribute(prop_name)
             except AttributeError:
-                pass
+                value = self._attributes[prop_name]
     except KeyError:
         raise AttributeError("'%s' object has no attribute '%s'" % (self.__class__.__name__, prop_name))
 
@@ -152,7 +153,7 @@ class Base(with_metaclass(BaseMeta, object)):
     _vars = FieldAttribute(isa='dict', priority=100, inherit=False)
 
     # flags and misc. settings
-    _environment = FieldAttribute(isa='list')
+    _environment = FieldAttribute(isa='list', extend=True, prepend=True)
     _no_log = FieldAttribute(isa='bool')
     _always_run = FieldAttribute(isa='bool')
     _run_once = FieldAttribute(isa='bool')
diff --git a/lib/ansible/playbook/block.py b/lib/ansible/playbook/block.py
index b34df9f21c..2675b5a1e4 100644
--- a/lib/ansible/playbook/block.py
+++ b/lib/ansible/playbook/block.py
@@ -278,22 +278,22 @@ class Block(Base, Become, Conditional, Taggable):
             for dep in dep_chain:
                 dep.set_loader(loader)
 
-    def _get_attr_environment(self):
-        return self._get_parent_attribute('environment', extend=True, prepend=True)
-
     def _get_parent_attribute(self, attr, extend=False, prepend=False):
         '''
         Generic logic to get the attribute or parent attribute for a block value.
         '''
 
-        value = None
+        extend = self._valid_attrs[attr].extend
+        prepend = self._valid_attrs[attr].prepend
         try:
             value = self._attributes[attr]
-
             if self._parent and (value is None or extend):
                 try:
-                    if attr != 'when' or getattr(self._parent, 'statically_loaded', True):
-                        parent_value = getattr(self._parent, attr, None)
+                    if getattr(self._parent, 'statically_loaded', True):
+                        if hasattr(self._parent, '_get_parent_attribute'):
+                            parent_value = self._parent._get_parent_attribute(attr)
+                        else:
+                            parent_value = self._parent._attributes.get(attr, None)
                         if extend:
                             value = self._extend_value(value, parent_value, prepend)
                         else:
@@ -302,7 +302,10 @@ class Block(Base, Become, Conditional, Taggable):
                     pass
             if self._role and (value is None or extend):
                 try:
-                    parent_value = getattr(self._role, attr, None)
+                    if hasattr(self._role, '_get_parent_attribute'):
+                        parent_value = self._role.get_parent_attribute(attr)
+                    else:
+                        parent_value = self._role._attributes.get(attr, None)
                     if extend:
                         value = self._extend_value(value, parent_value, prepend)
                     else:
@@ -312,7 +315,10 @@ class Block(Base, Become, Conditional, Taggable):
                     if dep_chain and (value is None or extend):
                         dep_chain.reverse()
                         for dep in dep_chain:
-                            dep_value = getattr(dep, attr, None)
+                            if hasattr(dep, '_get_parent_attribute'):
+                                dep_value = dep._get_parent_attribute(attr)
+                            else:
+                                dep_value = dep._attributes.get(attr, None)
                             if extend:
                                 value = self._extend_value(value, dep_value, prepend)
                             else:
@@ -324,11 +330,12 @@ class Block(Base, Become, Conditional, Taggable):
                     pass
             if self._play and (value is None or extend):
                 try:
-                    parent_value = getattr(self._play, attr, None)
-                    if extend:
-                        value = self._extend_value(value, parent_value, prepend)
-                    else:
-                        value = parent_value
+                    play_value = self._play._attributes.get(attr, None)
+                    if play_value is not None:
+                        if extend:
+                            value = self._extend_value(value, play_value, prepend)
+                        else:
+                            value = play_value
                 except AttributeError:
                     pass
         except KeyError:
diff --git a/lib/ansible/playbook/conditional.py b/lib/ansible/playbook/conditional.py
index 310703ca3d..33ee54c69e 100644
--- a/lib/ansible/playbook/conditional.py
+++ b/lib/ansible/playbook/conditional.py
@@ -49,7 +49,7 @@ class Conditional:
     to be run conditionally when a condition is met or skipped.
     '''
 
-    _when = FieldAttribute(isa='list', default=[])
+    _when = FieldAttribute(isa='list', default=[], extend=True, prepend=True)
 
     def __init__(self, loader=None):
         # when used directly, this class needs a loader, but we want to
@@ -66,17 +66,6 @@ class Conditional:
         if not isinstance(value, list):
             setattr(self, name, [value])
 
-    def _get_attr_when(self):
-        '''
-        Override for the 'tags' getattr fetcher, used from Base.
-        '''
-        when = self._attributes['when']
-        if when is None:
-            when = []
-        if hasattr(self, '_get_parent_attribute'):
-            when = self._get_parent_attribute('when', extend=True, prepend=True)
-        return when
-
     def extract_defined_undefined(self, conditional):
         results = []
 
diff --git a/lib/ansible/playbook/taggable.py b/lib/ansible/playbook/taggable.py
index 86f4cab177..b21638263f 100644
--- a/lib/ansible/playbook/taggable.py
+++ b/lib/ansible/playbook/taggable.py
@@ -30,7 +30,7 @@ from ansible.template import Templar
 class Taggable:
 
     untagged = frozenset(['untagged'])
-    _tags = FieldAttribute(isa='list', default=[], listof=(string_types, int))
+    _tags = FieldAttribute(isa='list', default=[], listof=(string_types, int), extend=True)
 
     def __init__(self):
         super(Taggable, self).__init__()
@@ -47,15 +47,6 @@ class Taggable:
         else:
             raise AnsibleError('tags must be specified as a list', obj=ds)
 
-    def _get_attr_tags(self):
-        '''
-        Override for the 'tags' getattr fetcher, used from Base, allow some classes to not give their tags to their 'children'
-        '''
-        tags = self._attributes.get('tags', [])
-        if hasattr(self, '_get_parent_attribute'):
-            tags = self._get_parent_attribute('tags', extend=True)
-        return tags
-
     def evaluate_tags(self, only_tags, skip_tags, all_vars):
         ''' this checks if the current item should be executed depending on tag options '''
 
diff --git a/lib/ansible/playbook/task.py b/lib/ansible/playbook/task.py
index 25c746049f..47bfd49099 100644
--- a/lib/ansible/playbook/task.py
+++ b/lib/ansible/playbook/task.py
@@ -414,16 +414,17 @@ class Task(Base, Conditional, Taggable, Become):
         Generic logic to get the attribute or parent attribute for a task value.
         '''
 
-        value = None
+        extend = self._valid_attrs[attr].extend
+        prepend = self._valid_attrs[attr].prepend
         try:
             value = self._attributes[attr]
             if self._parent and (value is None or extend):
-                if attr != 'when' or getattr(self._parent, 'statically_loaded', True):
+                if getattr(self._parent, 'statically_loaded', True):
                     # vars are always inheritable, other attributes might not be for the partent but still should be for other ancestors
-                    if attr != 'vars' and not getattr(self._parent, '_inheritable', True) and hasattr(self._parent, '_get_parent_attribute'):
-                        parent_value = self._parent._get_parent_attribute(attr, extend=extend, prepend=prepend)
+                    if attr != 'vars' and getattr(self._parent, '_inheritable', True) and hasattr(self._parent, '_get_parent_attribute'):
+                        parent_value = self._parent._get_parent_attribute(attr)
                     else:
-                        parent_value = getattr(self._parent, attr, None)
+                        parent_value = self._parent._attributes.get(attr, None)
 
                     if extend:
                         value = self._extend_value(value, parent_value, prepend)
@@ -442,12 +443,6 @@ class Task(Base, Conditional, Taggable, Become):
             value = C.ANY_ERRORS_FATAL
         return value
 
-    def _get_attr_environment(self):
-        '''
-        Override for the 'tags' getattr fetcher, used from Base.
-        '''
-        return self._get_parent_attribute('environment', extend=True, prepend=True)
-
     def get_dep_chain(self):
         if self._parent:
             return self._parent.get_dep_chain()
diff --git a/test/integration/targets/iso_extract/tasks/main.yml b/test/integration/targets/iso_extract/tasks/main.yml
index 2f812b8289..32b857fa73 100644
--- a/test/integration/targets/iso_extract/tasks/main.yml
+++ b/test/integration/targets/iso_extract/tasks/main.yml
@@ -35,7 +35,7 @@
   include_tasks: prepare.yml
 
 - name: Test in check-mode
-  include_tasks: tests.yml
+  import_tasks: tests.yml
   vars:
     in_check_mode: yes
   check_mode: yes