Don't copy the parent block of TaskIncludes when loading statically

When loading an include statically, we previously were simply doing a
copy() of the TaskInclude object, which recurses up the parents creating
a new lineage of objects. This caused problems when used inside load_list_of_blocks
as the new parent Block of the new TaskInclude was not actually in the list
of blocks being operated on. In most circumstances, this did not cause a
problem as the new parent block was a proper copy, however when used in
combination with PlaybookInclude (which copies conditionals to the list of
blocks loaded) this untracked parent was not being properly updated, leading
to tasks being run improperly.

Fixes #18206
This commit is contained in:
James Cammarata 2016-11-11 02:34:01 -06:00
commit 5b87951d6c
7 changed files with 27 additions and 36 deletions

View file

@ -197,8 +197,8 @@ class Base(with_metaclass(BaseMeta, object)):
def dump_me(self, depth=0): def dump_me(self, depth=0):
if depth == 0: if depth == 0:
display.debug("DUMPING OBJECT ------------------------------------------------------") print("DUMPING OBJECT ------------------------------------------------------")
display.debug("%s- %s (%s, id=%s)" % (" " * depth, self.__class__.__name__, self, id(self))) print("%s- %s (%s, id=%s)" % (" " * depth, self.__class__.__name__, self, id(self)))
if hasattr(self, '_parent') and self._parent: if hasattr(self, '_parent') and self._parent:
self._parent.dump_me(depth+2) self._parent.dump_me(depth+2)
dep_chain = self._parent.get_dep_chain() dep_chain = self._parent.get_dep_chain()

View file

@ -56,6 +56,9 @@ class Block(Base, Become, Conditional, Taggable):
super(Block, self).__init__() super(Block, self).__init__()
def __repr__(self):
return "BLOCK(uuid=%s)(id=%s)(parent=%s)" % (self._uuid, id(self), self._parent)
def get_vars(self): def get_vars(self):
''' '''
Blocks do not store variables directly, however they may be a member Blocks do not store variables directly, however they may be a member
@ -255,17 +258,6 @@ class Block(Base, Become, Conditional, Taggable):
self._parent = p self._parent = p
self._dep_chain = self._parent.get_dep_chain() self._dep_chain = self._parent.get_dep_chain()
def evaluate_conditional(self, templar, all_vars):
dep_chain = self.get_dep_chain()
if dep_chain:
for dep in dep_chain:
if not dep.evaluate_conditional(templar, all_vars):
return False
if self._parent is not None:
if not self._parent.evaluate_conditional(templar, all_vars):
return False
return super(Block, self).evaluate_conditional(templar, all_vars)
def set_loader(self, loader): def set_loader(self, loader):
self._loader = loader self._loader = loader
if self._parent: if self._parent:

View file

@ -51,6 +51,17 @@ class Conditional:
if not isinstance(value, list): if not isinstance(value, list):
setattr(self, name, [ value ]) 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)
return when
def evaluate_conditional(self, templar, all_vars): def evaluate_conditional(self, templar, all_vars):
''' '''
Loops through the conditionals set on this object, returning Loops through the conditionals set on this object, returning

View file

@ -56,7 +56,7 @@ def load_list_of_blocks(ds, play, parent_block=None, role=None, task_include=Non
task_include=task_include, task_include=task_include,
use_handlers=use_handlers, use_handlers=use_handlers,
variable_manager=variable_manager, variable_manager=variable_manager,
loader=loader loader=loader,
) )
# Implicit blocks are created by bare tasks listed in a play without # Implicit blocks are created by bare tasks listed in a play without
# an explicit block statement. If we have two implicit blocks in a row, # an explicit block statement. If we have two implicit blocks in a row,
@ -219,11 +219,13 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
task_list.append(t) task_list.append(t)
continue continue
ti_copy = t.copy(exclude_parent=True)
ti_copy._parent = block
included_blocks = load_list_of_blocks( included_blocks = load_list_of_blocks(
data, data,
play=play, play=play,
parent_block=None, parent_block=None,
task_include=t.copy(), task_include=ti_copy,
role=role, role=role,
use_handlers=use_handlers, use_handlers=use_handlers,
loader=loader, loader=loader,
@ -233,12 +235,12 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
# pop tags out of the include args, if they were specified there, and assign # pop tags out of the include args, if they were specified there, and assign
# them to the include. If the include already had tags specified, we raise an # them to the include. If the include already had tags specified, we raise an
# error so that users know not to specify them both ways # error so that users know not to specify them both ways
tags = t.vars.pop('tags', []) tags = ti_copy.vars.pop('tags', [])
if isinstance(tags, string_types): if isinstance(tags, string_types):
tags = tags.split(',') tags = tags.split(',')
if len(tags) > 0: if len(tags) > 0:
if len(t.tags) > 0: if len(ti_copy.tags) > 0:
raise AnsibleParserError( raise AnsibleParserError(
"Include tasks should not specify tags in more than one way (both via args and directly on the task). " \ "Include tasks should not specify tags in more than one way (both via args and directly on the task). " \
"Mixing styles in which tags are specified is prohibited for whole import hierarchy, not only for single import statement", "Mixing styles in which tags are specified is prohibited for whole import hierarchy, not only for single import statement",
@ -247,7 +249,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
) )
display.deprecated("You should not specify tags in the include parameters. All tags should be specified using the task-level option") display.deprecated("You should not specify tags in the include parameters. All tags should be specified using the task-level option")
else: else:
tags = t.tags[:] tags = ti_copy.tags[:]
# now we extend the tags on each of the included blocks # now we extend the tags on each of the included blocks
for b in included_blocks: for b in included_blocks:

View file

@ -61,15 +61,6 @@ class PlaybookInclude(Base, Conditional, Taggable):
templar = Templar(loader=loader, variables=all_vars) templar = Templar(loader=loader, variables=all_vars)
try:
forward_conditional = False
if not new_obj.evaluate_conditional(templar=templar, all_vars=all_vars):
return None
except AnsibleError:
# conditional evaluation raised an error, so we set a flag to indicate
# we need to forward the conditionals on to the included play(s)
forward_conditional = True
# then we use the object to load a Playbook # then we use the object to load a Playbook
pb = Playbook(loader=loader) pb = Playbook(loader=loader)
@ -95,9 +86,9 @@ class PlaybookInclude(Base, Conditional, Taggable):
# Check to see if we need to forward the conditionals on to the included # Check to see if we need to forward the conditionals on to the included
# plays. If so, we can take a shortcut here and simply prepend them to # plays. If so, we can take a shortcut here and simply prepend them to
# those attached to each block (if any) # those attached to each block (if any)
if forward_conditional: if new_obj.when:
for task_block in entry.pre_tasks + entry.roles + entry.tasks + entry.post_tasks: for task_block in (entry.pre_tasks + entry.roles + entry.tasks + entry.post_tasks):
task_block.when = self.when[:] + task_block.when task_block._attributes['when'] = new_obj.when[:] + task_block.when[:]
return pb return pb

View file

@ -376,12 +376,6 @@ class Task(Base, Conditional, Taggable, Become):
super(Task, self).deserialize(data) super(Task, self).deserialize(data)
def evaluate_conditional(self, templar, all_vars):
if self._parent is not None:
if not self._parent.evaluate_conditional(templar, all_vars):
return False
return super(Task, self).evaluate_conditional(templar, all_vars)
def set_loader(self, loader): def set_loader(self, loader):
''' '''
Sets the loader on this object and recursively on parent, child objects. Sets the loader on this object and recursively on parent, child objects.

View file

@ -654,6 +654,7 @@ class StrategyBase:
obj=included_file._task._ds) obj=included_file._task._ds)
display.deprecated("You should not specify tags in the include parameters. All tags should be specified using the task-level option") display.deprecated("You should not specify tags in the include parameters. All tags should be specified using the task-level option")
included_file._task.tags = tags included_file._task.tags = tags
ti_copy.vars = temp_vars ti_copy.vars = temp_vars
block_list = load_list_of_blocks( block_list = load_list_of_blocks(