From 881b76fede508d11bb92197f7f15d933c8ac0a36 Mon Sep 17 00:00:00 2001 From: Conner Crosby Date: Sat, 3 Aug 2024 17:34:55 -0400 Subject: [PATCH] Don't ignore invalid collection entries in requirements.yml (#83699) --- ...3715-raise-on-invalid-collection-entry.yml | 2 + .../dependency_resolution/dataclasses.py | 83 ++++++++++++++++--- .../tasks/requirements.yml | 8 +- .../tasks/pinned_pre_releases_in_deptree.yml | 2 +- test/units/cli/test_galaxy.py | 4 +- test/units/galaxy/test_collection_install.py | 21 +++++ 6 files changed, 97 insertions(+), 23 deletions(-) create mode 100644 changelogs/fragments/83715-raise-on-invalid-collection-entry.yml diff --git a/changelogs/fragments/83715-raise-on-invalid-collection-entry.yml b/changelogs/fragments/83715-raise-on-invalid-collection-entry.yml new file mode 100644 index 00000000000..402ae836276 --- /dev/null +++ b/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). diff --git a/lib/ansible/galaxy/dependency_resolution/dataclasses.py b/lib/ansible/galaxy/dependency_resolution/dataclasses.py index ea4c875adb4..bf1498a35e8 100644 --- a/lib/ansible/galaxy/dependency_resolution/dataclasses.py +++ b/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 diff --git a/test/integration/targets/ansible-galaxy-collection-scm/tasks/requirements.yml b/test/integration/targets/ansible-galaxy-collection-scm/tasks/requirements.yml index 10070f1a052..ff815a0fa7a 100644 --- a/test/integration/targets/ansible-galaxy-collection-scm/tasks/requirements.yml +++ b/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 .. 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' diff --git a/test/integration/targets/ansible-galaxy-collection/tasks/pinned_pre_releases_in_deptree.yml b/test/integration/targets/ansible-galaxy-collection/tasks/pinned_pre_releases_in_deptree.yml index 3745fa31e2c..49969016a3c 100644 --- a/test/integration/targets/ansible-galaxy-collection/tasks/pinned_pre_releases_in_deptree.yml +++ b/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: diff --git a/test/units/cli/test_galaxy.py b/test/units/cli/test_galaxy.py index 5444d148d06..8b3da282104 100644 --- a/test/units/cli/test_galaxy.py +++ b/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 \.\. " - 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) diff --git a/test/units/galaxy/test_collection_install.py b/test/units/galaxy/test_collection_install.py index 1803611d9c3..4cc91faecbc 100644 --- a/test/units/galaxy/test_collection_install.py +++ b/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']