Don't ignore invalid collection entries in requirements.yml (#83699)

changelogs/fragments/83715-raise-on-invalid-collection-entry.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - ansible-galaxy collection install - raise errors on invalid collection entry in requirements.yml (https://github.com/ansible/ansible/issues/83699).

lib/ansible/galaxy/dependency_resolution/dataclasses.py

@@ -31,11 +31,16 @@ from ansible.galaxy.api import GalaxyAPI
 from ansible.galaxy.collection import HAS_PACKAGING, PkgReq
 from ansible.module_utils.common.text.converters import to_bytes, to_native, to_text
 from ansible.module_utils.common.arg_spec import ArgumentSpecValidator
+from ansible.module_utils.errors import UnsupportedError
 from ansible.utils.collection_loader import AnsibleCollectionRef
 from ansible.utils.display import Display
+from ansible.utils.version import SemanticVersion
 
+try:
+    from packaging.specifiers import SpecifierSet, InvalidSpecifier
+except ImportError:
+    SpecifierSet = None  # type: ignore[misc]
 
-_ALLOW_CONCRETE_POINTER_IN_SOURCE = False  # NOTE: This is a feature flag
 _GALAXY_YAML = b'galaxy.yml'
 _MANIFEST_JSON = b'MANIFEST.json'
 _SOURCE_METADATA_FILE = b'GALAXY.yml'
@@ -307,12 +312,36 @@ class _ComputedReqKindsMixin:
 
     @classmethod
     def from_requirement_dict(cls, collection_req, art_mgr, validate_signature_options=True):
-        req_name = collection_req.get('name', None)
-        req_version = collection_req.get('version', '*')
-        req_type = collection_req.get('type')
+        result = ArgumentSpecValidator(
+            argument_spec=dict(
+                name=dict(type='str', required=True),
+                signatures=dict(type='list', elements='str'),
+                source=dict(type='raw'),
+                type=dict(type='str'),
+                version=dict(default='*', type='str')
+            )
+        ).validate(collection_req)
+
+        req_name = result.validated_parameters.get('name')
+        req_version = result.validated_parameters.get('version')
+        req_type = result.validated_parameters.get('type')
         # TODO: decide how to deprecate the old src API behavior
-        req_source = collection_req.get('source', None)
-        req_signature_sources = collection_req.get('signatures', None)
+        req_source = result.validated_parameters.get('source')
+        req_signature_sources = result.validated_parameters.get('signatures')
+        if result.error_messages:
+            error = result.errors[0]
+            if isinstance(error, UnsupportedError):
+                raise AnsibleError(
+                    "The following {collection_name!s} collection requirement entry keys are not "
+                    "valid: {unsupported_params!s}".format(
+                        collection_name=req_name,
+                        unsupported_params=', '.join(sorted(result.unsupported_parameters))
+                    )
+                )
+            raise AnsibleError(
+                f"Failed to install collection requirement entry: {error.msg}"
+            )
+
         if req_signature_sources is not None:
             if validate_signature_options and art_mgr.keyring is None:
                 raise AnsibleError(
@@ -323,14 +352,24 @@ class _ComputedReqKindsMixin:
                 req_signature_sources = [req_signature_sources]
             req_signature_sources = frozenset(req_signature_sources)
 
+        if (
+            req_version and req_type == 'subdirs'
+        ):
+            raise AnsibleError(
+                f"The {req_name} 'version' key is not applicable when 'type' is set to 'subdirs'."
+            )
+
+        if (
+            req_name is not None
+            and not AnsibleCollectionRef.is_valid_collection_name(req_name)
+            and req_source is not None
+        ):
+            raise AnsibleError(
+                f"The {req_name} 'source' key is not applicable when 'name' is not a FQCN."
+            )
+
         if req_type is None:
-            if (  # FIXME: decide on the future behavior:
-                    _ALLOW_CONCRETE_POINTER_IN_SOURCE
-                    and req_source is not None
-                    and _is_concrete_artifact_pointer(req_source)
-            ):
-                src_path = req_source
-            elif (
+            if (
                     req_name is not None
                     and AnsibleCollectionRef.is_valid_collection_name(req_name)
             ):
@@ -418,6 +457,24 @@ class _ComputedReqKindsMixin:
                 'index server.',
             )
 
+        if req_type != 'git' and req_version != '*':
+            if not HAS_PACKAGING:
+                raise AnsibleError("Failed to import packaging, check that a supported version is installed")
+
+            try:
+                specifier = next(iter(SpecifierSet(req_version)))
+            except InvalidSpecifier:
+                specifier = None
+
+            try:
+                version = req_version if not specifier else specifier.version
+                SemanticVersion(version)
+            except ValueError:
+                raise AnsibleError(
+                    f"The {req_name} 'version' key must be a valid collection version "
+                    "(see specification at https://semver.org/)."
+                )
+
         if req_type != 'galaxy' and req_source is None:
             req_source, req_name = req_name, None
 

test/integration/targets/ansible-galaxy-collection-scm/tasks/requirements.yml

@@ -26,12 +26,8 @@
     that:
       - result.failed
       - >-
-        "ERROR! Neither the collection requirement entry key 'name',
-        nor 'source' point to a concrete resolvable collection artifact.
-        Also 'name' is not an FQCN. A valid collection name must be in
-        the format <namespace>.<collection>. Please make sure that the
-        namespace and the collection name contain characters from
-        [a-zA-Z0-9_] only." in result.stderr
+        "ERROR! Failed to install collection requirement entry: missing
+        required arguments: name" in result.stderr
 
 - name: test source is not a git repo even if name is provided
   command: 'ansible-galaxy collection install -r source_and_name.yml'

test/integration/targets/ansible-galaxy-collection/tasks/pinned_pre_releases_in_deptree.yml

@@ -19,7 +19,7 @@
     command: >-
       ansible-galaxy collection install
       meta_ns_with_transitive_wildcard_dep.meta_name_with_transitive_wildcard_dep
-      rc_meta_ns_with_transitive_dev_dep.rc_meta_name_with_transitive_dev_dep:=2.4.5-rc5
+      rc_meta_ns_with_transitive_dev_dep.rc_meta_name_with_transitive_dev_dep:==2.4.5-rc5
       {{ galaxy_verbosity }}
     register: prioritize_direct_req
   - assert:

test/units/cli/test_galaxy.py

@@ -1106,9 +1106,7 @@ def test_parse_requirements_without_mandatory_name_key(requirements_cli, require
     # Used to be "Collections requirement entry should contain the key name."
     # Should we check that either source or name is provided before using the dep resolver?
 
-    expected = "Neither the collection requirement entry key 'name', nor 'source' point to a concrete resolvable collection artifact. "
-    expected += r"Also 'name' is not an FQCN\. A valid collection name must be in the format <namespace>\.<collection>\. "
-    expected += r"Please make sure that the namespace and the collection name contain characters from \[a\-zA\-Z0\-9_\] only\."
+    expected = "Failed to install collection requirement entry: missing required arguments: name"
 
     with pytest.raises(AnsibleError, match=expected):
         requirements_cli._parse_requirements_file(requirements_file)

test/units/galaxy/test_collection_install.py

@@ -398,6 +398,27 @@ def test_build_requirement_from_tar_invalid_manifest(tmp_path_factory):
         Requirement.from_requirement_dict({'name': to_text(tar_path)}, concrete_artifact_cm)
 
 
+def test_build_requirement_from_tar_unsupported_collection_keys(collection_artifact):
+    tmp_path = os.path.join(os.path.split(collection_artifact[1])[0], b'temp')
+    concrete_artifact_cm = collection.concrete_artifact_manager.ConcreteArtifactsManager(tmp_path, validate_certs=False)
+    expected = (
+        'The following {collection_name!s} collection requirement entry keys are not '
+        'valid: invalid_key, key'.format(collection_name=to_text(collection_artifact[1]))
+    )
+    with pytest.raises(AnsibleError, match=expected):
+        Requirement.from_requirement_dict({'name': to_text(collection_artifact[1]), 'invalid_key': 'foo', 'key': 'bar'}, concrete_artifact_cm)
+
+
+def test_build_requirement_from_tar_invalid_name_with_source(collection_artifact):
+    tmp_path = os.path.join(os.path.split(collection_artifact[1])[0], b'temp')
+    concrete_artifact_cm = collection.concrete_artifact_manager.ConcreteArtifactsManager(tmp_path, validate_certs=False)
+    expected = (
+        "The foo 'source' key is not applicable when 'name' is not a FQCN."
+    )
+    with pytest.raises(AnsibleError, match=expected):
+        Requirement.from_requirement_dict({'name': 'foo', 'source': 'foobar'}, concrete_artifact_cm)
+
+
 def test_build_requirement_from_name(galaxy_server, monkeypatch, tmp_path_factory):
     mock_get_versions = MagicMock()
     mock_get_versions.return_value = ['2.1.9', '2.1.10']