From dde10a9afb815e0b223af2b00f39fb6efc95a9fb Mon Sep 17 00:00:00 2001 From: Matt Davis <6775756+nitzmahone@users.noreply.github.com> Date: Wed, 18 Jun 2025 17:38:23 -0700 Subject: [PATCH] import_playbook validation cleanup (#85358) * use declarative FA validation * deleted redundant/broken imperative validation * added test case to ensure templating Co-authored-by: Matt Clay --- lib/ansible/playbook/base.py | 2 - lib/ansible/playbook/playbook_include.py | 65 +++---------------- .../playbook/test_import_playbook.yml | 2 +- 3 files changed, 10 insertions(+), 59 deletions(-) diff --git a/lib/ansible/playbook/base.py b/lib/ansible/playbook/base.py index 9fc5519904d..955962ea324 100644 --- a/lib/ansible/playbook/base.py +++ b/lib/ansible/playbook/base.py @@ -221,8 +221,6 @@ class FieldAttributeBase: def validate(self, all_vars=None): """ validation that is done at parse time, not load time """ - all_vars = {} if all_vars is None else all_vars - if not self._validated: # walk all fields in the object for (name, attribute) in self.fattributes.items(): diff --git a/lib/ansible/playbook/playbook_include.py b/lib/ansible/playbook/playbook_include.py index 9cdd5ee58d3..d278ea6cbed 100644 --- a/lib/ansible/playbook/playbook_include.py +++ b/lib/ansible/playbook/playbook_include.py @@ -19,12 +19,7 @@ from __future__ import annotations import os -import ansible.constants as C -from ansible.errors import AnsibleParserError, AnsibleAssertionError from ansible.module_utils.common.text.converters import to_bytes -from ansible.module_utils._internal._datatag import AnsibleTagHelper -from ansible.module_utils.six import string_types -from ansible.parsing.splitter import split_args from ansible.playbook.attribute import NonInheritableFieldAttribute from ansible.playbook.base import Base from ansible.playbook.conditional import Conditional @@ -32,16 +27,15 @@ from ansible.playbook.taggable import Taggable from ansible.utils.collection_loader import AnsibleCollectionConfig from ansible.utils.collection_loader._collection_finder import _get_collection_name_from_path, _get_collection_playbook_path from ansible._internal._templating._engine import TemplateEngine -from ansible.utils.display import Display - -display = Display() class PlaybookInclude(Base, Conditional, Taggable): - import_playbook = NonInheritableFieldAttribute(isa='string') + import_playbook = NonInheritableFieldAttribute(isa='string', required=True) vars_val = NonInheritableFieldAttribute(isa='dict', default=dict, alias='vars') + _post_validate_object = True # manually post_validate to get free arg validation/coercion + @staticmethod def load(data, basedir, variable_manager=None, loader=None): return PlaybookInclude().load_data(ds=data, basedir=basedir, variable_manager=variable_manager, loader=loader) @@ -62,18 +56,22 @@ class PlaybookInclude(Base, Conditional, Taggable): new_obj = super(PlaybookInclude, self).load_data(ds, variable_manager, loader) all_vars = self.vars.copy() + if variable_manager: all_vars |= variable_manager.get_vars() templar = TemplateEngine(loader=loader, variables=all_vars) + new_obj.post_validate(templar) + # then we use the object to load a Playbook pb = Playbook(loader=loader) - file_name = templar.template(new_obj.import_playbook) + file_name = new_obj.import_playbook # check for FQCN resource = _get_collection_playbook_path(file_name) + if resource is not None: playbook = resource[1] playbook_collection = resource[2] @@ -92,6 +90,7 @@ class PlaybookInclude(Base, Conditional, Taggable): else: # it is NOT a collection playbook, setup adjacent paths AnsibleCollectionConfig.playbook_paths.append(os.path.dirname(os.path.abspath(to_bytes(playbook, errors='surrogate_or_strict')))) + # broken, see: https://github.com/ansible/ansible/issues/85357 pb._load_playbook_data(file_name=playbook, variable_manager=variable_manager, vars=self.vars.copy()) @@ -120,49 +119,3 @@ class PlaybookInclude(Base, Conditional, Taggable): task_block._when = new_obj.when[:] + task_block.when[:] return pb - - def preprocess_data(self, ds): - """ - Reorganizes the data for a PlaybookInclude datastructure to line - up with what we expect the proper attributes to be - """ - - if not isinstance(ds, dict): - raise AnsibleAssertionError('ds (%s) should be a dict but was a %s' % (ds, type(ds))) - - # the new, cleaned datastructure, which will have legacy items reduced to a standard structure suitable for the - # attributes of the task class; copy any tagged data to preserve things like origin - new_ds = AnsibleTagHelper.tag_copy(ds, {}) - - for (k, v) in ds.items(): - if k in C._ACTION_IMPORT_PLAYBOOK: - self._preprocess_import(ds, new_ds, k, v) - else: - # some basic error checking, to make sure vars are properly - # formatted and do not conflict with k=v parameters - if k == 'vars': - if 'vars' in new_ds: - raise AnsibleParserError("import_playbook parameters cannot be mixed with 'vars' entries for import statements", obj=ds) - elif not isinstance(v, dict): - raise AnsibleParserError("vars for import_playbook statements must be specified as a dictionary", obj=ds) - new_ds[k] = v - - return super(PlaybookInclude, self).preprocess_data(new_ds) - - def _preprocess_import(self, ds, new_ds, k, v): - """ - Splits the playbook import line up into filename and parameters - """ - if v is None: - raise AnsibleParserError("playbook import parameter is missing", obj=ds) - elif not isinstance(v, string_types): - raise AnsibleParserError("playbook import parameter must be a string indicating a file path, got %s instead" % type(v), obj=ds) - - # The import_playbook line must include at least one item, which is the filename - # to import. Anything after that should be regarded as a parameter to the import - items = split_args(v) - if len(items) == 0: - raise AnsibleParserError("import_playbook statements must specify the file name to import", obj=ds) - - # DTFIX3: investigate this as a possible "problematic strip" - new_ds['import_playbook'] = AnsibleTagHelper.tag_copy(v, items[0].strip()) diff --git a/test/integration/targets/include_import/playbook/test_import_playbook.yml b/test/integration/targets/include_import/playbook/test_import_playbook.yml index 4fcdb10bf13..e59126c29e6 100644 --- a/test/integration/targets/include_import/playbook/test_import_playbook.yml +++ b/test/integration/targets/include_import/playbook/test_import_playbook.yml @@ -1,5 +1,5 @@ # Test and validate playbook import -- import_playbook: playbook1.yml +- import_playbook: '{{ "playbook1.yml" }}' # ensure templating occurs correctly - import_playbook: validate1.yml # Test and validate conditional import