From ebf971f931290a113f738f658d7a6e095994e120 Mon Sep 17 00:00:00 2001 From: James Cammarata Date: Fri, 5 Jan 2018 20:51:44 -0600 Subject: [PATCH] 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 --- lib/ansible/playbook/attribute.py | 20 +++++++++-- lib/ansible/playbook/base.py | 9 ++--- lib/ansible/playbook/block.py | 35 +++++++++++-------- lib/ansible/playbook/conditional.py | 13 +------ lib/ansible/playbook/taggable.py | 11 +----- lib/ansible/playbook/task.py | 17 ++++----- .../targets/iso_extract/tasks/main.yml | 2 +- 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