From 0633f73faf962620e09774a45a65aab968f6fb6d Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Thu, 22 Mar 2018 14:23:10 -0700 Subject: [PATCH] Fix loader for filters (#37748) * Fix loading of filter and test plugins Filter and test plugins are different than other plugins in that they can have many plugins in a single file. Therefore they need to operate a little differently. They need to have all of the potential files returned. Then the caller takes care of passing those onto jinja2 in order for jinja2 to make use of them. This problem was (most recently) introduced with f921369445900dcc756821e40c9257d5359087af This commit also restructures how we deduplicate plugins to take paths into account. If we want to start scoping which set of modules are loaded (due to roles, for instance) we'll need to hang on to the path information. * add integration test for override * Fix style checks for bcoca code * Implement jinja2 plugin loader as a subclass Having a subclass allows us to customize the overriding of jinja plugins. We can then move common parts of common code into the Loader. --- lib/ansible/plugins/loader.py | 114 +++++++++++++++--- lib/ansible/template/__init__.py | 11 +- lib/ansible/template/safe_eval.py | 4 +- .../integration/targets/plugin_loader/aliases | 1 + .../targets/plugin_loader/normal/filters.yml | 13 ++ .../override/filter_plugins/core.py | 18 +++ .../plugin_loader/override/filters.yml | 15 +++ .../targets/plugin_loader/runme.sh | 24 ++++ 8 files changed, 174 insertions(+), 26 deletions(-) create mode 100644 test/integration/targets/plugin_loader/aliases create mode 100644 test/integration/targets/plugin_loader/normal/filters.yml create mode 100644 test/integration/targets/plugin_loader/override/filter_plugins/core.py create mode 100644 test/integration/targets/plugin_loader/override/filters.yml create mode 100755 test/integration/targets/plugin_loader/runme.sh diff --git a/lib/ansible/plugins/loader.py b/lib/ansible/plugins/loader.py index 7ec35d3062e..8a7c4aec330 100644 --- a/lib/ansible/plugins/loader.py +++ b/lib/ansible/plugins/loader.py @@ -35,13 +35,11 @@ def get_all_plugin_loaders(): class PluginLoader: - ''' PluginLoader loads plugins from the configured plugin directories. - It searches for plugins by iterating through the combined list of - play basedirs, configured paths, and the python path. - The first match is used. + It searches for plugins by iterating through the combined list of play basedirs, configured + paths, and the python path. The first match is used. ''' def __init__(self, class_name, package, config, subdir, aliases=None, required_base_class=None): @@ -410,36 +408,72 @@ class PluginLoader: display.debug(msg) def all(self, *args, **kwargs): - ''' instantiates all plugins with the same arguments ''' + ''' + Iterate through all plugins of this type + + A plugin loader is initialized with a specific type. This function is an iterator returning + all of the plugins of that type to the caller. + + :kwarg path_only: If this is set to True, then we return the paths to where the plugins reside + instead of an instance of the plugin. This conflicts with class_only and both should + not be set. + :kwarg class_only: If this is set to True then we return the python class which implements + a plugin rather than an instance of the plugin. This conflicts with path_only and both + should not be set. + :kwarg _dedupe: By default, we only return one plugin per plugin name. Deduplication happens + in the same way as the :meth:`get` and :meth:`find_plugin` methods resolve which plugin + should take precedence. If this is set to False, then we return all of the plugins + found, including those with duplicate names. In the case of duplicates, the order in + which they are returned is the one that would take precedence first, followed by the + others in decreasing precedence order. This should only be used by subclasses which + want to manage their own deduplication of the plugins. + :*args: Any extra arguments are passed to each plugin when it is instantiated. + :**kwargs: Any extra keyword arguments are passed to each plugin when it is instantiated. + ''' + # TODO: Change the signature of this method to: + # def all(return_type='instance', args=None, kwargs=None): + # if args is None: args = [] + # if kwargs is None: kwargs = {} + # return_type can be instance, class, or path. + # These changes will mean that plugin parameters won't conflict with our params and + # will also make it impossible to request both a path and a class at the same time. + # + # Move _dedupe to be a class attribute, CUSTOM_DEDUPE, with subclasses for filters and + # tests setting it to True global _PLUGIN_FILTERS + dedupe = kwargs.pop('_dedupe', True) path_only = kwargs.pop('path_only', False) class_only = kwargs.pop('class_only', False) + # Having both path_only and class_only is a coding bug + if path_only and class_only: + raise AnsibleError('Do not set both path_only and class_only when calling PluginLoader.all()') + all_matches = [] found_in_cache = True for i in self._get_paths(): all_matches.extend(glob.glob(os.path.join(i, "*.py"))) + loaded_modules = set() for path in sorted(all_matches, key=os.path.basename): - name = os.path.basename(os.path.splitext(path)[0]) + name = os.path.splitext(path)[0] + basename = os.path.basename(name) - if '__init__' in name or name in _PLUGIN_FILTERS[self.package]: + if basename == '__init__' or basename in _PLUGIN_FILTERS[self.package]: continue + if dedupe and basename in loaded_modules: + continue + loaded_modules.add(basename) + if path_only: yield path continue if path not in self._module_cache: module = self._load_module_source(name, path) - if module in self._module_cache.values(): - # In ``_load_module_source`` if a plugin has a duplicate name, we just return the - # previously matched plugin from sys.modules, which means you are never getting both, - # just one, but cached for both paths, this isn't normally a problem, except with callbacks - # where it will run that single callback twice. This rejects duplicates. - continue self._module_cache[path] = module found_in_cache = False @@ -461,7 +495,7 @@ class PluginLoader: if not issubclass(obj, plugin_class): continue - self._display_plugin_load(self.class_name, name, self._searched_paths, path, found_in_cache=found_in_cache, class_only=class_only) + self._display_plugin_load(self.class_name, basename, self._searched_paths, path, found_in_cache=found_in_cache, class_only=class_only) if not class_only: try: obj = obj(*args, **kwargs) @@ -470,12 +504,58 @@ class PluginLoader: # load plugin config data if not found_in_cache: - self._load_config_defs(name, path) + self._load_config_defs(basename, path) self._update_object(obj, name, path) yield obj +class Jinja2Loader(PluginLoader): + """ + PluginLoader optimized for Jinja2 plugins + + The filter and test plugins are Jinja2 plugins encapsulated inside of our plugin format. + The way the calling code is setup, we need to do a few things differently in the all() method + """ + def find_plugin(self, name): + # Nothing using Jinja2Loader use this method. We can't use the base class version because + # we deduplicate differently than the base class + raise AnsibleError('No code should call find_plugin for Jinja2Loaders (Not implemented)') + + def get(self, name, *args, **kwargs): + # Nothing using Jinja2Loader use this method. We can't use the base class version because + # we deduplicate differently than the base class + raise AnsibleError('No code should call find_plugin for Jinja2Loaders (Not implemented)') + + def all(self, *args, **kwargs): + """ + Differences with :meth:`PluginLoader.all`: + + * We do not deduplicate ansible plugin names. This is because we don't care about our + plugin names, here. We care about the names of the actual jinja2 plugins which are inside + of our plugins. + * We reverse the order of the list of plugins compared to other PluginLoaders. This is + because of how calling code chooses to sync the plugins from the list. It adds all the + Jinja2 plugins from one of our Ansible plugins into a dict. Then it adds the Jinja2 + plugins from the next Ansible plugin, overwriting any Jinja2 plugins that had the same + name. This is an encapsulation violation (the PluginLoader should not know about what + calling code does with the data) but we're pushing the common code here. We'll fix + this in the future by moving more of the common code into this PluginLoader. + * We return a list. We could iterate the list instead but that's extra work for no gain because + the API receiving this doesn't care. It just needs an iterable + """ + # We don't deduplicate ansible plugin names. Instead, calling code deduplicates jinja2 + # plugin names. + kwargs['_dedupe'] = False + + # We have to instantiate a list of all plugins so that we can reverse it. We reverse it so + # that calling code will deduplicate this correctly. + plugins = [p for p in super(Jinja2Loader, self).all(*args, **kwargs)] + plugins.reverse() + + return plugins + + def _load_plugin_filter(): filters = defaultdict(frozenset) @@ -610,14 +690,14 @@ lookup_loader = PluginLoader( required_base_class='LookupBase', ) -filter_loader = PluginLoader( +filter_loader = Jinja2Loader( 'FilterModule', 'ansible.plugins.filter', C.DEFAULT_FILTER_PLUGIN_PATH, 'filter_plugins', ) -test_loader = PluginLoader( +test_loader = Jinja2Loader( 'TestModule', 'ansible.plugins.test', C.DEFAULT_TEST_PLUGIN_PATH, diff --git a/lib/ansible/template/__init__.py b/lib/ansible/template/__init__.py index 1241ed78432..3a2ff89b9c8 100644 --- a/lib/ansible/template/__init__.py +++ b/lib/ansible/template/__init__.py @@ -300,16 +300,15 @@ class Templar: if self._filters is not None: return self._filters.copy() - plugins = [x for x in self._filter_loader.all()] - self._filters = dict() - for fp in plugins: - self._filters.update(fp.filters()) # TODO: Remove registering tests as filters in 2.9 for name, func in self._get_tests().items(): self._filters[name] = tests_as_filters_warning(name, func) + for fp in self._filter_loader.all(): + self._filters.update(fp.filters()) + return self._filters.copy() def _get_tests(self): @@ -320,10 +319,8 @@ class Templar: if self._tests is not None: return self._tests.copy() - plugins = [x for x in self._test_loader.all()] - self._tests = dict() - for fp in plugins: + for fp in self._test_loader.all(): self._tests.update(fp.tests()) return self._tests.copy() diff --git a/lib/ansible/template/safe_eval.py b/lib/ansible/template/safe_eval.py index a142d1d2740..3bbd3acbd4f 100644 --- a/lib/ansible/template/safe_eval.py +++ b/lib/ansible/template/safe_eval.py @@ -91,8 +91,8 @@ def safe_eval(expr, locals=None, include_exceptions=False): ) filter_list = [] - for filter in filter_loader.all(): - filter_list.extend(filter.filters().keys()) + for filter_ in filter_loader.all(): + filter_list.extend(filter_.filters().keys()) test_list = [] for test in test_loader.all(): diff --git a/test/integration/targets/plugin_loader/aliases b/test/integration/targets/plugin_loader/aliases new file mode 100644 index 00000000000..79d8b9285eb --- /dev/null +++ b/test/integration/targets/plugin_loader/aliases @@ -0,0 +1 @@ +posix/ci/group3 diff --git a/test/integration/targets/plugin_loader/normal/filters.yml b/test/integration/targets/plugin_loader/normal/filters.yml new file mode 100644 index 00000000000..f9069be1a61 --- /dev/null +++ b/test/integration/targets/plugin_loader/normal/filters.yml @@ -0,0 +1,13 @@ +- hosts: testhost + gather_facts: false + tasks: + - name: ensure filters work as shipped from core + assert: + that: + - a|flatten == [1, 2, 3, 4, 5] + - a|ternary('yes', 'no') == 'yes' + vars: + a: + - 1 + - 2 + - [3, 4, 5] diff --git a/test/integration/targets/plugin_loader/override/filter_plugins/core.py b/test/integration/targets/plugin_loader/override/filter_plugins/core.py new file mode 100644 index 00000000000..f283dc398b0 --- /dev/null +++ b/test/integration/targets/plugin_loader/override/filter_plugins/core.py @@ -0,0 +1,18 @@ +# Make coding more python3-ish +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + + +def do_flag(myval): + return 'flagged' + + +class FilterModule(object): + ''' Ansible core jinja2 filters ''' + + def filters(self): + return { + # jinja2 overrides + 'flag': do_flag, + 'flatten': do_flag, + } diff --git a/test/integration/targets/plugin_loader/override/filters.yml b/test/integration/targets/plugin_loader/override/filters.yml new file mode 100644 index 00000000000..e51ab4e95b1 --- /dev/null +++ b/test/integration/targets/plugin_loader/override/filters.yml @@ -0,0 +1,15 @@ +- hosts: testhost + gather_facts: false + tasks: + - name: ensure local 'flag' filter works, 'flatten' is overriden and 'ternary' is still from core + assert: + that: + - a|flag == 'flagged' + - a|flatten != [1, 2, 3, 4, 5] + - a|flatten == "flagged" + - a|ternary('yes', 'no') == 'yes' + vars: + a: + - 1 + - 2 + - [3, 4, 5] diff --git a/test/integration/targets/plugin_loader/runme.sh b/test/integration/targets/plugin_loader/runme.sh new file mode 100755 index 00000000000..2a1bdedaca3 --- /dev/null +++ b/test/integration/targets/plugin_loader/runme.sh @@ -0,0 +1,24 @@ +#!/usr/bin/env bash + +set -ux + + +# check normal execution +for myplay in normal/*.yml +do + ansible-playbook "${myplay}" -i ../../inventory -vvv "$@" + if test $? != 0 ; then + echo "### Failed to run ${myplay} normally" + exit 1 + fi +done + +# check overrides +for myplay in override/*.yml +do + ansible-playbook "${myplay}" -i ../../inventory -vvv "$@" + if test $? != 0 ; then + echo "### Failed to run ${myplay} override" + exit 1 + fi +done