Merge 01586ab132 into 6aa8192131

changelogs/fragments/82154-fix-galaxy-requirement-spec-validation.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - Fix validating role params that may be unused by the role, but are supported by ansible-galaxy (https://github.com/ansible/ansible/issues/82154).

lib/ansible/cli/galaxy.py

@@ -810,8 +810,6 @@ class GalaxyCLI(CLI):
             if "include" not in requirement:
                 role = RoleRequirement.role_yaml_parse(requirement)
                 display.vvv("found role %s in yaml file" % to_text(role))
-                if "name" not in role and "src" not in role:
-                    raise AnsibleError("Must specify name or src for role")
                 return [GalaxyRole(self.galaxy, self.lazy_role_api, **role)]
             else:
                 b_include_path = to_bytes(requirement["include"], errors="surrogate_or_strict")
@@ -1286,9 +1284,9 @@ class GalaxyCLI(CLI):
                 role_info.update(gr.metadata)
 
             req = RoleRequirement()
-            role_spec = req.role_yaml_parse({'role': role})
-            if role_spec:
-                role_info.update(role_spec)
+            if ',' in role:
+                raise AnsibleError("Invalid old style role requirement: %s" % role)
+            role_info.update({'name': role, 'role': role})
 
             data += self._display_role_info(role_info)
 

lib/ansible/galaxy/role.py

@@ -80,7 +80,10 @@ class GalaxyRole(object):
     META_REQUIREMENTS = (os.path.join('meta', 'requirements.yml'), os.path.join('meta', 'requirements.yaml'))
     ROLE_DIRS = ('defaults', 'files', 'handlers', 'meta', 'tasks', 'templates', 'vars', 'tests')
 
-    def __init__(self, galaxy, api, name, src=None, version=None, scm=None, path=None):
+    def __init__(self, galaxy, api, name=None, src=None, version=None, scm=None, path=None):
+
+        if name is None:
+            raise AnsibleParserError("Must specify name or src for role")
 
         self._metadata = None
         self._metadata_dependencies = None

lib/ansible/playbook/helpers.py

@@ -301,7 +301,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
     return task_list
 
 
-def load_list_of_roles(ds, play, current_role_path=None, variable_manager=None, loader=None, collection_search_list=None):
+def load_list_of_roles(ds, play, current_role_path=None, variable_manager=None, loader=None, collection_search_list=None, extra_spec=None):
     """
     Loads and returns a list of RoleInclude objects from the ds list of role definitions
     :param ds: list of roles to load
@@ -321,7 +321,7 @@ def load_list_of_roles(ds, play, current_role_path=None, variable_manager=None,
     roles = []
     for role_def in ds:
         i = RoleInclude.load(role_def, play=play, current_role_path=current_role_path, variable_manager=variable_manager,
-                             loader=loader, collection_list=collection_search_list)
+                             loader=loader, collection_list=collection_search_list, extra_spec=extra_spec)
         roles.append(i)
 
     return roles

lib/ansible/playbook/role/__init__.py

@@ -209,6 +209,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
         self._role_path = role_include.get_role_path()
         self._role_collection = role_include._role_collection
         self._role_params = role_include.get_role_params()
+        self._internal_galaxy_spec = role_include._internal_galaxy_spec
         self._variable_manager = role_include.get_variable_manager()
         self._loader = role_include.get_loader()
 
@@ -269,11 +270,18 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
 
         if self._should_validate:
             role_argspecs = self._get_role_argspecs()
-            task_data = self._prepend_validation_task(task_data, role_argspecs)
+            validation_task_data = self._prepend_validation_task(None, role_argspecs)
+            validation_blocks = load_list_of_blocks(
+                validation_task_data, play=self._play, role=self, loader=self._loader, variable_manager=self._variable_manager
+            )
+            for block in validation_blocks:
+                for task in block.block:
+                    task.implicit = True
+            self._task_blocks = validation_blocks
 
         if task_data:
             try:
-                self._task_blocks = load_list_of_blocks(task_data, play=self._play, role=self, loader=self._loader, variable_manager=self._variable_manager)
+                self._task_blocks += load_list_of_blocks(task_data, play=self._play, role=self, loader=self._loader, variable_manager=self._variable_manager)
             except AssertionError as e:
                 raise AnsibleParserError("The tasks/main.yml file for role '%s' must contain a list of tasks" % self._role_name,
                                          obj=task_data, orig_exc=e)
@@ -364,6 +372,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
                 # Pass only the 'options' portion of the arg spec to the module.
                 'argument_spec': argument_spec.get('options', {}),
                 'provided_arguments': self._role_params,
+                'optional_galaxy_arguments': self._internal_galaxy_spec,
                 'validate_args_context': {
                     'type': 'role',
                     'name': self._role_name,

lib/ansible/playbook/role/definition.py

@@ -43,7 +43,7 @@ class RoleDefinition(Base, Conditional, Taggable, CollectionSearch):
 
     role = NonInheritableFieldAttribute(isa='string')
 
-    def __init__(self, play=None, role_basedir=None, variable_manager=None, loader=None, collection_list=None):
+    def __init__(self, play=None, role_basedir=None, variable_manager=None, loader=None, collection_list=None, extra_spec=None):
 
         super(RoleDefinition, self).__init__()
 
@@ -57,6 +57,8 @@ class RoleDefinition(Base, Conditional, Taggable, CollectionSearch):
         self._role_params = dict()
         self._collection_list = collection_list
 
+        self._internal_galaxy_spec = extra_spec
+
     # def __repr__(self):
     #     return 'ROLEDEF: ' + self._attributes.get('role', '<no name set>')
 

lib/ansible/playbook/role/include.py

@@ -35,12 +35,12 @@ class RoleInclude(RoleDefinition, Delegatable):
     is included for execution in a play.
     """
 
-    def __init__(self, play=None, role_basedir=None, variable_manager=None, loader=None, collection_list=None):
+    def __init__(self, play=None, role_basedir=None, variable_manager=None, loader=None, collection_list=None, extra_spec=None):
         super(RoleInclude, self).__init__(play=play, role_basedir=role_basedir, variable_manager=variable_manager,
-                                          loader=loader, collection_list=collection_list)
+                                          loader=loader, collection_list=collection_list, extra_spec=extra_spec)
 
     @staticmethod
-    def load(data, play, current_role_path=None, parent_role=None, variable_manager=None, loader=None, collection_list=None):
+    def load(data, play, current_role_path=None, parent_role=None, variable_manager=None, loader=None, collection_list=None, extra_spec=None):
 
         if not (isinstance(data, string_types) or isinstance(data, dict) or isinstance(data, AnsibleBaseYAMLObject)):
             raise AnsibleParserError("Invalid role definition: %s" % to_native(data))
@@ -48,5 +48,7 @@ class RoleInclude(RoleDefinition, Delegatable):
         if isinstance(data, string_types) and ',' in data:
             raise AnsibleError("Invalid old style role requirement: %s" % data)
 
-        ri = RoleInclude(play=play, role_basedir=current_role_path, variable_manager=variable_manager, loader=loader, collection_list=collection_list)
+        ri = RoleInclude(
+            play=play, role_basedir=current_role_path, variable_manager=variable_manager, loader=loader, collection_list=collection_list, extra_spec=extra_spec
+        )
         return ri.load_data(data, variable_manager=variable_manager, loader=loader)

lib/ansible/playbook/role/metadata.py

@@ -26,7 +26,7 @@ from ansible.playbook.attribute import NonInheritableFieldAttribute
 from ansible.playbook.base import Base
 from ansible.playbook.collectionsearch import CollectionSearch
 from ansible.playbook.helpers import load_list_of_roles
-from ansible.playbook.role.requirement import RoleRequirement
+from ansible.playbook.role.requirement import RoleRequirement, VALID_SPEC_KEYS
 
 __all__ = ['RoleMetadata']
 
@@ -101,10 +101,16 @@ class RoleMetadata(Base, CollectionSearch):
             if 'ansible.legacy' not in collection_search_list:
                 collection_search_list.append('ansible.legacy')
 
+        # The dependency format is used by ansible-galaxy and when running roles.
+        # Since role runtime (with argument spec) requires all role params be documented,
+        # add a supplemental spec so this isn't a failure by default for ansible-galaxy options.
+        # If the role actually documents these options, they'll be validated normally.
+        internal_galaxy_spec = {k: {} for k in VALID_SPEC_KEYS}
+
         try:
             return load_list_of_roles(roles, play=self._owner._play, current_role_path=current_role_path,
                                       variable_manager=self._variable_manager, loader=self._loader,
-                                      collection_search_list=collection_search_list)
+                                      collection_search_list=collection_search_list, extra_spec=internal_galaxy_spec)
         except AssertionError as e:
             raise AnsibleParserError("A malformed list of role dependencies was encountered.", obj=self._ds, orig_exc=e)
 

lib/ansible/plugins/action/validate_argument_spec.py

@@ -6,6 +6,7 @@ from __future__ import annotations
 from ansible.errors import AnsibleError
 from ansible.plugins.action import ActionBase
 from ansible.module_utils.common.arg_spec import ArgumentSpecValidator
+from ansible.playbook.role.requirement import RoleRequirement
 from ansible.utils.vars import combine_vars
 
 
@@ -75,6 +76,21 @@ class ActionModule(ActionBase):
         if not isinstance(provided_arguments, dict):
             raise AnsibleError('Incorrect type for provided_arguments, expected dict and got %s' % type(provided_arguments))
 
+        if self._task.implicit and (role_spec := self._task.args.get('optional_galaxy_arguments', ())):
+            galaxy_opts = {field: provided_arguments[field] for field in role_spec if field in provided_arguments and field not in argument_spec_data}
+            if galaxy_opts:
+                try:
+                    # TODO: port this to use an argument spec? May need to deprecate syntax or add new argspec behavior:
+                    # Suboptions don't support 'str' elements, defaults are determined from other options, values are mutated with custom rules, etc...
+                    RoleRequirement.role_yaml_parse(galaxy_opts)
+                except AnsibleError:
+                    # invalid for ansible-galaxy, validate normally
+                    pass
+                else:
+                    self._display.vvv(f"{self._task._role} - validated internal ansible-galaxy dependency format")
+                    for opt in galaxy_opts:
+                        provided_arguments.pop(opt)
+
         args_from_vars = self.get_args_from_task_vars(argument_spec_data, task_vars)
         validator = ArgumentSpecValidator(argument_spec_data)
         validation_result = validator.validate(combine_vars(args_from_vars, provided_arguments), validate_role_argument_spec=True)

test/integration/targets/roles_arg_spec/roles/role_with_deps/meta/main.yml

@@ -0,0 +1,11 @@
+dependencies:
+  # src/scm/version/role are used by ansible-galaxy when installing dependencies
+  - src: a
+    version: unknown
+    a_str: required
+
+  # only validate these fields if the argument spec includes them
+  - role: validate_role_req
+    scm: svn
+    src: othersource
+    version: "1.0.0"

test/integration/targets/roles_arg_spec/roles/validate_role_req/meta/main.yml

@@ -0,0 +1,17 @@
+argument_specs:
+  main:
+    short_description: Main entrypoint to test validating the overloaded role params scm, src, and version.
+    options:
+      scm:
+        type: str
+        description: role variable 'scm'
+        choices:
+          - "git"
+      src:
+        type: str
+        description: role variable 'src'
+        choices:
+          - "src"
+      version:
+        type: int
+        description: role variable 'version'

test/integration/targets/roles_arg_spec/test.yml

@@ -478,3 +478,25 @@
     - name: Import role with an empty argument_specs key
       import_role:
         name: empty_argspec
+
+- name: "New play to reset vars: Test not validating ambiguous role params by default"
+  hosts: localhost
+  gather_facts: false
+  tasks:
+    - block:
+      - name: Include role with dependencies using ansible-galaxy requirements syntax
+        include_role:
+          name: role_with_deps
+
+      - fail:
+          msg: "should not get here"
+
+      rescue:
+        - debug: var=ansible_failed_result
+
+        - name: Validate failure for role params
+          assert:
+            that:
+              - ansible_failed_result.argument_errors | length == 3
+              - ansible_failed_result.validate_args_context.name == 'validate_role_req'
+              - ansible_failed_result.validate_args_context.argument_spec_name == "main"