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
This commit is contained in:
James Cammarata 2018-01-05 20:51:44 -06:00 committed by Brian Coca
parent ab5dbca47e
commit ebf971f931
7 changed files with 53 additions and 54 deletions

View file

@ -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)

View file

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

View file

@ -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:

View file

@ -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 = []

View file

@ -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 '''

View file

@ -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()

View file

@ -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