From dca0397fd7b25c2960e8f42af54d5558575fe40b Mon Sep 17 00:00:00 2001 From: David Shrewsbury Date: Tue, 21 Apr 2020 15:23:25 -0400 Subject: [PATCH 1/4] Make sure collection is a list if a str is given --- .../fragments/69054-collection-as-str.yaml | 2 ++ lib/ansible/playbook/collectionsearch.py | 17 ++++++++++------- test/integration/targets/collections/posix.yml | 7 +++++++ 3 files changed, 19 insertions(+), 7 deletions(-) create mode 100644 changelogs/fragments/69054-collection-as-str.yaml diff --git a/changelogs/fragments/69054-collection-as-str.yaml b/changelogs/fragments/69054-collection-as-str.yaml new file mode 100644 index 00000000000000..5e9845f99b254a --- /dev/null +++ b/changelogs/fragments/69054-collection-as-str.yaml @@ -0,0 +1,2 @@ +bugfixes: +- Make sure if a collection is supplied as a string that we transform it into a list. diff --git a/lib/ansible/playbook/collectionsearch.py b/lib/ansible/playbook/collectionsearch.py index 994e2e13e4c70a..9322f782b795cf 100644 --- a/lib/ansible/playbook/collectionsearch.py +++ b/lib/ansible/playbook/collectionsearch.py @@ -19,12 +19,14 @@ def _ensure_default_collection(collection_list=None): if collection_list is None: collection_list = [] - if default_collection: # FIXME: exclude role tasks? - if isinstance(collection_list, string_types): - collection_list = [collection_list] + # We allow collections to be either a string or a list, so make sure + # we have a list going forward. + if isinstance(collection_list, string_types): + collection_list = [collection_list] - if default_collection not in collection_list: - collection_list.insert(0, default_collection) + # FIXME: exclude role tasks? + if default_collection and default_collection not in collection_list: + collection_list.insert(0, default_collection) # if there's something in the list, ensure that builtin or legacy is always there too if collection_list and 'ansible.builtin' not in collection_list and 'ansible.legacy' not in collection_list: @@ -47,8 +49,9 @@ def _load_collections(self, attr, ds): return None # This duplicates static attr checking logic from post_validate() - # because if the user attempts to template a collection name, it will - # error before it ever gets to the post_validate() warning. + # because if the user attempts to template a collection name, it may + # error before it ever gets to the post_validate() warning (e.g. trying + # to import a role from the collection). env = Environment() for collection_name in ds: if is_template(collection_name, env): diff --git a/test/integration/targets/collections/posix.yml b/test/integration/targets/collections/posix.yml index 0d7c7089c4b591..61f950f50bfa22 100644 --- a/test/integration/targets/collections/posix.yml +++ b/test/integration/targets/collections/posix.yml @@ -406,3 +406,10 @@ hosts: testhost roles: - testns.testcoll.call_standalone + +# Issue https://github.com/ansible/ansible/issues/69054 +- name: Test collection as string + hosts: testhost + collections: foo + tasks: + - debug: msg="Test" From 1ea7adaf215a3efe61ce91125a57e915bd768b58 Mon Sep 17 00:00:00 2001 From: David Shrewsbury Date: Thu, 23 Apr 2020 12:41:53 -0400 Subject: [PATCH 2/4] Call field validation early on collections Because we are doing work on modifying the collections value before it is actually validated, we can validate it ourselves early to make sure the user supplies either a string or list. Dicts are not valid. The new validation allows us to simplify the _ensure_default_collection() function. And since the field is now static, we no longer need to specify a default for it, which also allows us to simplify the function. Since the default is now removed, we can also remove the sanity/ignore.txt entry for collectionsearch.py. New unit tests are added (and the existing one modified) that allow us to make sure that we throw a parser error if a user specifies something other than a string or list for the collections value everywhere it can be specified. --- lib/ansible/playbook/collectionsearch.py | 16 +++---- lib/ansible/playbook/task.py | 14 +++--- test/sanity/ignore.txt | 1 - test/units/playbook/test_collectionsearch.py | 47 +++++++++++++++++--- 4 files changed, 55 insertions(+), 23 deletions(-) diff --git a/lib/ansible/playbook/collectionsearch.py b/lib/ansible/playbook/collectionsearch.py index 9322f782b795cf..695789d0ada7ae 100644 --- a/lib/ansible/playbook/collectionsearch.py +++ b/lib/ansible/playbook/collectionsearch.py @@ -13,17 +13,9 @@ display = Display() -def _ensure_default_collection(collection_list=None): +def _ensure_default_collection(collection_list): default_collection = AnsibleCollectionLoader().default_collection - if collection_list is None: - collection_list = [] - - # We allow collections to be either a string or a list, so make sure - # we have a list going forward. - if isinstance(collection_list, string_types): - collection_list = [collection_list] - # FIXME: exclude role tasks? if default_collection and default_collection not in collection_list: collection_list.insert(0, default_collection) @@ -38,10 +30,14 @@ def _ensure_default_collection(collection_list=None): class CollectionSearch: # this needs to be populated before we can resolve tasks/roles/etc - _collections = FieldAttribute(isa='list', listof=string_types, priority=100, default=_ensure_default_collection, + _collections = FieldAttribute(isa='list', listof=string_types, priority=100, always_post_validate=True, static=True) def _load_collections(self, attr, ds): + # We are always a mixin with Base, so we can validate this untemplated + # field early on to guarantee we are dealing with a list. + ds = self.get_validated_value('collections', self._collections, ds, None) + # this will only be called if someone specified a value; call the shared value _ensure_default_collection(collection_list=ds) diff --git a/lib/ansible/playbook/task.py b/lib/ansible/playbook/task.py index 99e3b7a38870e5..a2fb0d86322856 100644 --- a/lib/ansible/playbook/task.py +++ b/lib/ansible/playbook/task.py @@ -184,14 +184,14 @@ def preprocess_data(self, ds): # since this affects the task action parsing, we have to resolve in preprocess instead of in typical validator default_collection = AnsibleCollectionLoader().default_collection - # use the parent value if our ds doesn't define it - collections_list = ds.get('collections', self.collections) - + collections_list = ds.get('collections') if collections_list is None: - collections_list = [] - - if isinstance(collections_list, string_types): - collections_list = [collections_list] + # use the parent value if our ds doesn't define it + collections_list = self.collections + else: + # Validate this untemplated field early on to guarantee we are dealing with a list. + # This is also done in CollectionSearch._load_collections() but this runs before that call. + collections_list = self.get_validated_value('collections', self._collections, collections_list, None) if default_collection and not self._role: # FIXME: and not a collections role if collections_list: diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index 2a637c93dc3190..4aa64e2ea85146 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -218,7 +218,6 @@ lib/ansible/modules/utilities/logic/async_wrapper.py use-argspec-type-path lib/ansible/modules/utilities/logic/wait_for.py validate-modules:parameter-list-no-elements lib/ansible/parsing/vault/__init__.py pylint:blacklisted-name lib/ansible/playbook/base.py pylint:blacklisted-name -lib/ansible/playbook/collectionsearch.py required-and-default-attributes # https://github.com/ansible/ansible/issues/61460 lib/ansible/playbook/helpers.py pylint:blacklisted-name lib/ansible/playbook/role/__init__.py pylint:blacklisted-name lib/ansible/plugins/action/normal.py action-plugin-docs # default action plugin for modules without a dedicated action plugin diff --git a/test/units/playbook/test_collectionsearch.py b/test/units/playbook/test_collectionsearch.py index fe480d3de1e3ed..b431fe0d60f89c 100644 --- a/test/units/playbook/test_collectionsearch.py +++ b/test/units/playbook/test_collectionsearch.py @@ -18,6 +18,10 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +from ansible.errors import AnsibleParserError +from ansible.playbook.play import Play +from ansible.playbook.task import Task +from ansible.playbook.block import Block from ansible.playbook.collectionsearch import CollectionSearch import pytest @@ -28,11 +32,44 @@ def test_collection_static_warning(capsys): Also, make sure that users see the warning message for the referenced name. """ - - collection_name = 'foo.{{bar}}' - cs = CollectionSearch() - assert collection_name in cs._load_collections(None, [collection_name]) - + collection_name = "foo.{{bar}}" + p = Play.load(dict( + name="test play", + hosts=['foo'], + gather_facts=False, + connection='local', + collections=collection_name, + )) + assert collection_name in p.collections std_out, std_err = capsys.readouterr() assert '[WARNING]: "collections" is not templatable, but we found: %s' % collection_name in std_err assert '' == std_out + +def test_collection_invalid_data_play(): + """Test that collection as a dict at the play level fails with parser error""" + collection_name = {'name': 'foo'} + with pytest.raises(AnsibleParserError): + Play.load(dict( + name="test play", + hosts=['foo'], + gather_facts=False, + connection='local', + collections=collection_name, + )) + +def test_collection_invalid_data_task(): + """Test that collection as a dict at the task level fails with parser error""" + collection_name = {'name': 'foo'} + with pytest.raises(AnsibleParserError): + Task.load(dict( + name="test task", + collections=collection_name, + )) + +def test_collection_invalid_data_block(): + """Test that collection as a dict at the block level fails with parser error""" + collection_name = {'name': 'foo'} + with pytest.raises(AnsibleParserError): + Block.load(dict( + block=[dict(name="test task", collections=collection_name)] + )) From 77f21b4330268aa3779ce185a5c17f0f5ee7cb7e Mon Sep 17 00:00:00 2001 From: David Shrewsbury Date: Thu, 23 Apr 2020 17:12:38 -0400 Subject: [PATCH 3/4] Revert removing the collection default The default is actually used, so restore it. --- lib/ansible/playbook/collectionsearch.py | 8 ++++++-- test/sanity/ignore.txt | 1 + test/units/playbook/test_collectionsearch.py | 3 +++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/ansible/playbook/collectionsearch.py b/lib/ansible/playbook/collectionsearch.py index 695789d0ada7ae..d80b6a1c6a3efc 100644 --- a/lib/ansible/playbook/collectionsearch.py +++ b/lib/ansible/playbook/collectionsearch.py @@ -13,9 +13,13 @@ display = Display() -def _ensure_default_collection(collection_list): +def _ensure_default_collection(collection_list=None): default_collection = AnsibleCollectionLoader().default_collection + # Will be None when used as the default + if collection_list is None: + collection_list = [] + # FIXME: exclude role tasks? if default_collection and default_collection not in collection_list: collection_list.insert(0, default_collection) @@ -30,7 +34,7 @@ def _ensure_default_collection(collection_list): class CollectionSearch: # this needs to be populated before we can resolve tasks/roles/etc - _collections = FieldAttribute(isa='list', listof=string_types, priority=100, + _collections = FieldAttribute(isa='list', listof=string_types, priority=100, default=_ensure_default_collection, always_post_validate=True, static=True) def _load_collections(self, attr, ds): diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index 4aa64e2ea85146..2a637c93dc3190 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -218,6 +218,7 @@ lib/ansible/modules/utilities/logic/async_wrapper.py use-argspec-type-path lib/ansible/modules/utilities/logic/wait_for.py validate-modules:parameter-list-no-elements lib/ansible/parsing/vault/__init__.py pylint:blacklisted-name lib/ansible/playbook/base.py pylint:blacklisted-name +lib/ansible/playbook/collectionsearch.py required-and-default-attributes # https://github.com/ansible/ansible/issues/61460 lib/ansible/playbook/helpers.py pylint:blacklisted-name lib/ansible/playbook/role/__init__.py pylint:blacklisted-name lib/ansible/plugins/action/normal.py action-plugin-docs # default action plugin for modules without a dedicated action plugin diff --git a/test/units/playbook/test_collectionsearch.py b/test/units/playbook/test_collectionsearch.py index b431fe0d60f89c..be40d85e303dcb 100644 --- a/test/units/playbook/test_collectionsearch.py +++ b/test/units/playbook/test_collectionsearch.py @@ -45,6 +45,7 @@ def test_collection_static_warning(capsys): assert '[WARNING]: "collections" is not templatable, but we found: %s' % collection_name in std_err assert '' == std_out + def test_collection_invalid_data_play(): """Test that collection as a dict at the play level fails with parser error""" collection_name = {'name': 'foo'} @@ -57,6 +58,7 @@ def test_collection_invalid_data_play(): collections=collection_name, )) + def test_collection_invalid_data_task(): """Test that collection as a dict at the task level fails with parser error""" collection_name = {'name': 'foo'} @@ -66,6 +68,7 @@ def test_collection_invalid_data_task(): collections=collection_name, )) + def test_collection_invalid_data_block(): """Test that collection as a dict at the block level fails with parser error""" collection_name = {'name': 'foo'} From 4e76a2b6120ad5c400fddbf35fffa8abdffcac8a Mon Sep 17 00:00:00 2001 From: David Shrewsbury Date: Fri, 24 Apr 2020 11:07:00 -0400 Subject: [PATCH 4/4] Fix unit tests in test_helpers.py affected by early collection validation --- test/units/playbook/test_helpers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/units/playbook/test_helpers.py b/test/units/playbook/test_helpers.py index 2dc67eeebdf180..a4ed6178d4ed69 100644 --- a/test/units/playbook/test_helpers.py +++ b/test/units/playbook/test_helpers.py @@ -316,7 +316,7 @@ def test_one_include_not_static(self): # print(res) def test_one_bogus_include_role(self): - ds = [{'include_role': {'name': 'bogus_role'}}] + ds = [{'include_role': {'name': 'bogus_role'}, 'collections': []}] res = helpers.load_list_of_tasks(ds, play=self.mock_play, block=self.mock_block, variable_manager=self.mock_variable_manager, loader=self.fake_role_loader) @@ -324,7 +324,7 @@ def test_one_bogus_include_role(self): self._assert_is_task_list_or_blocks(res) def test_one_bogus_include_role_use_handlers(self): - ds = [{'include_role': {'name': 'bogus_role'}}] + ds = [{'include_role': {'name': 'bogus_role'}, 'collections': []}] res = helpers.load_list_of_tasks(ds, play=self.mock_play, use_handlers=True, block=self.mock_block, variable_manager=self.mock_variable_manager, @@ -395,7 +395,7 @@ def test_empty_block(self): loader=None) def test_block_unknown_action(self): - ds = [{'action': 'foo'}] + ds = [{'action': 'foo', 'collections': []}] mock_play = MagicMock(name='MockPlay') res = helpers.load_list_of_blocks(ds, mock_play, parent_block=None, role=None, task_include=None, use_handlers=False, variable_manager=None, loader=None)