From facf3dfde2e432ba2fb0826afc50d2f03104eb3a Mon Sep 17 00:00:00 2001
From: s-hertel <19572925+s-hertel@users.noreply.github.com>
Date: Tue, 7 Nov 2023 15:38:19 -0500
Subject: [PATCH] Fix validating role params of dependencies that are used by
 ansible-galaxy

Validate normally if these are documented by the role, but don't require
documentation since the role may be unaware of these fields used by
ansible-galaxy.
---
 lib/ansible/cli/galaxy.py                     |  8 +++----
 lib/ansible/galaxy/role.py                    |  5 ++++-
 lib/ansible/playbook/helpers.py               |  4 ++--
 lib/ansible/playbook/role/__init__.py         | 13 +++++++++--
 lib/ansible/playbook/role/definition.py       |  4 +++-
 lib/ansible/playbook/role/include.py          | 10 +++++----
 lib/ansible/playbook/role/metadata.py         | 10 +++++++--
 .../plugins/action/validate_argument_spec.py  | 16 ++++++++++++++
 .../roles/role_with_deps/meta/main.yml        | 11 ++++++++++
 .../roles/validate_role_req/meta/main.yml     | 17 ++++++++++++++
 .../targets/roles_arg_spec/test.yml           | 22 +++++++++++++++++++
 11 files changed, 103 insertions(+), 17 deletions(-)
 create mode 100644 test/integration/targets/roles_arg_spec/roles/role_with_deps/meta/main.yml
 create mode 100644 test/integration/targets/roles_arg_spec/roles/validate_role_req/meta/main.yml

diff --git a/lib/ansible/cli/galaxy.py b/lib/ansible/cli/galaxy.py
index 07e652a2e95..a6f07eceadc 100755
--- a/lib/ansible/cli/galaxy.py
+++ b/lib/ansible/cli/galaxy.py
@@ -808,8 +808,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")
@@ -1279,9 +1277,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)
 
diff --git a/lib/ansible/galaxy/role.py b/lib/ansible/galaxy/role.py
index 50873c4ca82..bdc8f7d29e4 100644
--- a/lib/ansible/galaxy/role.py
+++ b/lib/ansible/galaxy/role.py
@@ -79,7 +79,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:
+            AnsibleParserError("Must specify name or src for role")
 
         self._metadata = None
         self._metadata_dependencies = None
diff --git a/lib/ansible/playbook/helpers.py b/lib/ansible/playbook/helpers.py
index b908a0f9722..2ec48e4462f 100644
--- a/lib/ansible/playbook/helpers.py
+++ b/lib/ansible/playbook/helpers.py
@@ -303,7 +303,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
@@ -323,7 +323,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
diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py
index 04256898cee..0a0d7f8c07b 100644
--- a/lib/ansible/playbook/role/__init__.py
+++ b/lib/ansible/playbook/role/__init__.py
@@ -198,6 +198,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()
 
@@ -258,11 +259,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)
@@ -353,6 +361,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,
diff --git a/lib/ansible/playbook/role/definition.py b/lib/ansible/playbook/role/definition.py
index 12d6ce182c8..d919248ceff 100644
--- a/lib/ansible/playbook/role/definition.py
+++ b/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>')
 
diff --git a/lib/ansible/playbook/role/include.py b/lib/ansible/playbook/role/include.py
index 934b53ce9b4..8efc9c8b70e 100644
--- a/lib/ansible/playbook/role/include.py
+++ b/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)
diff --git a/lib/ansible/playbook/role/metadata.py b/lib/ansible/playbook/role/metadata.py
index 482539a75d6..47f30b53d67 100644
--- a/lib/ansible/playbook/role/metadata.py
+++ b/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)
 
diff --git a/lib/ansible/plugins/action/validate_argument_spec.py b/lib/ansible/plugins/action/validate_argument_spec.py
index 4d68067a343..530596adfcc 100644
--- a/lib/ansible/plugins/action/validate_argument_spec.py
+++ b/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)
diff --git a/test/integration/targets/roles_arg_spec/roles/role_with_deps/meta/main.yml b/test/integration/targets/roles_arg_spec/roles/role_with_deps/meta/main.yml
new file mode 100644
index 00000000000..e567919144c
--- /dev/null
+++ b/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"
diff --git a/test/integration/targets/roles_arg_spec/roles/validate_role_req/meta/main.yml b/test/integration/targets/roles_arg_spec/roles/validate_role_req/meta/main.yml
new file mode 100644
index 00000000000..f216e3fc317
--- /dev/null
+++ b/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'
diff --git a/test/integration/targets/roles_arg_spec/test.yml b/test/integration/targets/roles_arg_spec/test.yml
index b88d2e183d8..5b0d9f8c51f 100644
--- a/test/integration/targets/roles_arg_spec/test.yml
+++ b/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"