From 4c0af6c8089f1102655ee8eef782b233d9913ee5 Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Thu, 23 Jul 2020 09:02:57 -0700 Subject: [PATCH] fix internal cases of actions calling unqualified module names (#70818) * fix internal cases of actions calling unqualified module names * add porting_guide entry * misc other fixes around action/module resolution broken by redirection ci_complete * Update docs/docsite/rst/porting_guides/porting_guide_2.10.rst Co-authored-by: Rick Elrod * Update docs/docsite/rst/porting_guides/porting_guide_2.10.rst Co-authored-by: Rick Elrod * address review feedback * pep8 * unit test fixes * win fixes * gather_facts fix module args ignores * docs sanity * pep8 * fix timeout test * fix win name rewrites Co-authored-by: Rick Elrod --- .../fragments/fq_action_module_resolution.yml | 2 ++ .../rst/porting_guides/porting_guide_2.10.rst | 5 +++ lib/ansible/config/base.yml | 25 +++++++------- lib/ansible/executor/task_executor.py | 4 +-- lib/ansible/plugins/action/__init__.py | 34 +++++++++++++------ lib/ansible/plugins/action/assemble.py | 7 ++-- lib/ansible/plugins/action/async_status.py | 2 +- lib/ansible/plugins/action/command.py | 4 ++- lib/ansible/plugins/action/copy.py | 10 +++--- lib/ansible/plugins/action/fetch.py | 2 +- lib/ansible/plugins/action/gather_facts.py | 7 ++-- lib/ansible/plugins/action/normal.py | 1 + lib/ansible/plugins/action/package.py | 15 ++++++-- lib/ansible/plugins/action/reboot.py | 6 ++-- lib/ansible/plugins/action/service.py | 16 +++++++-- lib/ansible/plugins/action/shell.py | 4 +-- lib/ansible/plugins/action/template.py | 3 +- lib/ansible/plugins/action/unarchive.py | 5 +-- lib/ansible/plugins/action/uri.py | 7 ++-- .../plugins/action/wait_for_connection.py | 2 +- lib/ansible/plugins/action/yum.py | 12 +++++-- test/integration/targets/playbook/timeout.yml | 2 +- test/units/executor/test_task_executor.py | 2 +- .../units/plugins/action/test_gather_facts.py | 2 +- 24 files changed, 116 insertions(+), 63 deletions(-) create mode 100644 changelogs/fragments/fq_action_module_resolution.yml diff --git a/changelogs/fragments/fq_action_module_resolution.yml b/changelogs/fragments/fq_action_module_resolution.yml new file mode 100644 index 00000000000..e6d58ce9623 --- /dev/null +++ b/changelogs/fragments/fq_action_module_resolution.yml @@ -0,0 +1,2 @@ +bugfixes: + - action plugins - change all action/module delegations to use FQ names while allowing overrides (https://github.com/ansible/ansible/issues/69788) diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.10.rst b/docs/docsite/rst/porting_guides/porting_guide_2.10.rst index dcdf5e94779..e8cd37e13ed 100644 --- a/docs/docsite/rst/porting_guides/porting_guide_2.10.rst +++ b/docs/docsite/rst/porting_guides/porting_guide_2.10.rst @@ -132,6 +132,11 @@ Noteworthy plugin changes * Cache plugins in collections can be used to cache data from inventory plugins. Previously, cache plugins in collections could only be used for fact caching. * Some undocumented arguments from ``FILE_COMMON_ARGUMENTS`` have been removed; plugins using these, in particular action plugins, need to be adjusted. The undocumented arguments which were removed are ``src``, ``follow``, ``force``, ``content``, ``backup``, ``remote_src``, ``regexp``, ``delimiter``, and ``directory_mode``. +Action plugins which execute modules should use fully-qualified module names +---------------------------------------------------------------------------- + +* Action plugins that call modules should pass explicit, fully-qualified module names to ``_execute_module()`` whenever possible (eg, ``ansible.builtin.file`` rather than ``file``). This ensures that the task's collection search order is not consulted to resolve the module. Otherwise, a module from a collection earlier in the search path could be used when not intended. + Porting custom scripts ====================== diff --git a/lib/ansible/config/base.yml b/lib/ansible/config/base.yml index 630c50aa7cf..86653d7b126 100644 --- a/lib/ansible/config/base.yml +++ b/lib/ansible/config/base.yml @@ -1345,29 +1345,30 @@ ERROR_ON_MISSING_HANDLER: CONNECTION_FACTS_MODULES: name: Map of connections to fact modules default: - asa: asa_facts + # use ansible.legacy names on unqualified facts modules to allow library/ overrides + asa: ansible.legacy.asa_facts cisco.asa.asa: cisco.asa.asa_facts - eos: eos_facts + eos: ansible.legacy.eos_facts arista.eos.eos: arista.eos.eos_facts - frr: frr_facts + frr: ansible.legacy.frr_facts frr.frr.frr: frr.frr.frr_facts - ios: ios_facts + ios: ansible.legacy.ios_facts cisco.ios.ios: cisco.ios.ios_facts - iosxr: iosxr_facts + iosxr: ansible.legacy.iosxr_facts cisco.iosxr.iosxr: cisco.iosxr.iosxr_facts - junos: junos_facts + junos: ansible.legacy.junos_facts junipernetworks.junos.junos: junipernetworks.junos.junos_facts - nxos: nxos_facts + nxos: ansible.legacy.nxos_facts cisco.nxos.nxos: cisco.nxos.nxos_facts - vyos: vyos_facts + vyos: ansible.legacy.vyos_facts vyos.vyos.vyos: vyos.vyos.vyos_facts - exos: exos_facts + exos: ansible.legacy.exos_facts extreme.exos.exos: extreme.exos.exos_facts - slxos: slxos_facts + slxos: ansible.legacy.slxos_facts extreme.slxos.slxos: extreme.slxos.slxos_facts - voss: voss_facts + voss: ansible.legacy.voss_facts extreme.voss.voss: extreme.voss.voss_facts - ironware: ironware_facts + ironware: ansible.legacy.ironware_facts community.network.ironware: community.network.ironware_facts description: "Which modules to run during a play's fact gathering stage based on connection" env: [{name: ANSIBLE_CONNECTION_FACTS_MODULES}] diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index 40a09326076..1c528dfea68 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -982,8 +982,8 @@ class TaskExecutor: elif all((module_prefix in C.NETWORK_GROUP_MODULES, self._shared_loader_obj.action_loader.has_plugin(network_action, collection_list=collections))): handler_name = network_action else: - # FUTURE: once we're comfortable with collections impl, preface this action with ansible.builtin so it can't be hijacked - handler_name = 'normal' + # use ansible.legacy.normal to allow (historic) local action_plugins/ override without collections search + handler_name = 'ansible.legacy.normal' collections = None # until then, we don't want the task's collection list to be consulted; use the builtin handler = self._shared_loader_obj.action_loader.get( diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index 62bdd851158..11d77817427 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -163,6 +163,10 @@ class ActionBase(with_metaclass(ABCMeta, object)): else: use_vars = task_vars + split_module_name = module_name.split('.') + collection_name = '.'.join(split_module_name[0:2]) if len(split_module_name) > 2 else '' + leaf_module_name = resource_from_fqcr(module_name) + # Search module path(s) for named module. for mod_type in self._connection.module_implementation_preferences: # Check to determine if PowerShell modules are supported, and apply @@ -171,17 +175,21 @@ class ActionBase(with_metaclass(ABCMeta, object)): # FIXME: This should be temporary and moved to an exec subsystem plugin where we can define the mapping # for each subsystem. win_collection = 'ansible.windows' - + rewrite_collection_names = ['ansible.builtin', 'ansible.legacy', ''] # async_status, win_stat, win_file, win_copy, and win_ping are not just like their # python counterparts but they are compatible enough for our # internal usage - if module_name in ('stat', 'file', 'copy', 'ping') and self._task.action != module_name: - module_name = '%s.win_%s' % (win_collection, module_name) - elif module_name in ['async_status']: - module_name = '%s.%s' % (win_collection, module_name) + # NB: we only rewrite the module if it's not being called by the user (eg, an action calling something else) + # and if it's unqualified or FQ to a builtin + if leaf_module_name in ('stat', 'file', 'copy', 'ping') and \ + collection_name in rewrite_collection_names and self._task.action != module_name: + module_name = '%s.win_%s' % (win_collection, leaf_module_name) + elif leaf_module_name == 'async_status' and collection_name in rewrite_collection_names: + module_name = '%s.%s' % (win_collection, leaf_module_name) + # TODO: move this tweak down to the modules, not extensible here # Remove extra quotes surrounding path parameters before sending to module. - if resource_from_fqcr(module_name) in ['win_stat', 'win_file', 'win_copy', 'slurp'] and module_args and \ + if leaf_module_name in ['win_stat', 'win_file', 'win_copy', 'slurp'] and module_args and \ hasattr(self._connection._shell, '_unquote'): for key in ('src', 'dest', 'path'): if key in module_args: @@ -724,7 +732,7 @@ class ActionBase(with_metaclass(ABCMeta, object)): get_checksum=checksum, checksum_algorithm='sha1', ) - mystat = self._execute_module(module_name='stat', module_args=module_args, task_vars=all_vars, + mystat = self._execute_module(module_name='ansible.legacy.stat', module_args=module_args, task_vars=all_vars, wrap_async=False) if mystat.get('failed'): @@ -1015,8 +1023,8 @@ class ActionBase(with_metaclass(ABCMeta, object)): if wrap_async and not self._connection.always_pipeline_modules: # configure, upload, and chmod the async_wrapper module - (async_module_style, shebang, async_module_data, async_module_path) = self._configure_module(module_name='async_wrapper', module_args=dict(), - task_vars=task_vars) + (async_module_style, shebang, async_module_data, async_module_path) = self._configure_module( + module_name='ansible.legacy.async_wrapper', module_args=dict(), task_vars=task_vars) async_module_remote_filename = self._connection._shell.get_remote_filename(async_module_path) remote_async_module_path = self._connection._shell.join_path(tmpdir, async_module_remote_filename) self._transfer_data(remote_async_module_path, async_module_data) @@ -1255,7 +1263,9 @@ class ActionBase(with_metaclass(ABCMeta, object)): diff = {} display.debug("Going to peek to see if file has changed permissions") - peek_result = self._execute_module(module_name='file', module_args=dict(path=destination, _diff_peek=True), task_vars=task_vars, persist_files=True) + peek_result = self._execute_module( + module_name='ansible.legacy.file', module_args=dict(path=destination, _diff_peek=True), + task_vars=task_vars, persist_files=True) if peek_result.get('failed', False): display.warning(u"Failed to get diff between '%s' and '%s': %s" % (os.path.basename(source), destination, to_text(peek_result.get(u'msg', u'')))) @@ -1271,7 +1281,9 @@ class ActionBase(with_metaclass(ABCMeta, object)): diff['dst_larger'] = C.MAX_FILE_SIZE_FOR_DIFF else: display.debug(u"Slurping the file %s" % source) - dest_result = self._execute_module(module_name='slurp', module_args=dict(path=destination), task_vars=task_vars, persist_files=True) + dest_result = self._execute_module( + module_name='ansible.legacy.slurp', module_args=dict(path=destination), + task_vars=task_vars, persist_files=True) if 'content' in dest_result: dest_contents = dest_result['content'] if dest_result['encoding'] == u'base64': diff --git a/lib/ansible/plugins/action/assemble.py b/lib/ansible/plugins/action/assemble.py index 6ec456b586e..06fa2df32f6 100644 --- a/lib/ansible/plugins/action/assemble.py +++ b/lib/ansible/plugins/action/assemble.py @@ -104,7 +104,8 @@ class ActionModule(ActionBase): raise AnsibleActionFail("src and dest are required") if boolean(remote_src, strict=False): - result.update(self._execute_module(module_name='assemble', task_vars=task_vars)) + # call assemble via ansible.legacy to allow library/ overrides of the module without collection search + result.update(self._execute_module(module_name='ansible.legacy.assemble', task_vars=task_vars)) raise _AnsibleActionDone() else: try: @@ -150,12 +151,12 @@ class ActionModule(ActionBase): new_module_args.update(dict(src=xfered,)) - res = self._execute_module(module_name='copy', module_args=new_module_args, task_vars=task_vars) + res = self._execute_module(module_name='ansible.legacy.copy', module_args=new_module_args, task_vars=task_vars) if diff: res['diff'] = diff result.update(res) else: - result.update(self._execute_module(module_name='file', module_args=new_module_args, task_vars=task_vars)) + result.update(self._execute_module(module_name='ansible.legacy.file', module_args=new_module_args, task_vars=task_vars)) except AnsibleAction as e: result.update(e.result) diff --git a/lib/ansible/plugins/action/async_status.py b/lib/ansible/plugins/action/async_status.py index e7273c305d4..7b69f6246c9 100644 --- a/lib/ansible/plugins/action/async_status.py +++ b/lib/ansible/plugins/action/async_status.py @@ -40,7 +40,7 @@ class ActionModule(ActionBase): async_dir = self.get_shell_option('async_dir', default="~/.ansible_async") module_args = dict(jid=jid, mode=mode, _async_dir=async_dir) - status = self._execute_module(task_vars=task_vars, + status = self._execute_module(module_name='ansible.legacy.async_status', task_vars=task_vars, module_args=module_args) results = merge_hash(results, status) return results diff --git a/lib/ansible/plugins/action/command.py b/lib/ansible/plugins/action/command.py index 46fcc9b3120..f267eb737f9 100644 --- a/lib/ansible/plugins/action/command.py +++ b/lib/ansible/plugins/action/command.py @@ -21,7 +21,9 @@ class ActionModule(ActionBase): self._task.args['warn'] = C.COMMAND_WARNINGS wrap_async = self._task.async_val and not self._connection.has_native_async - results = merge_hash(results, self._execute_module(task_vars=task_vars, wrap_async=wrap_async)) + # explicitly call `ansible.legacy.command` for backcompat to allow library/ override of `command` while not allowing + # collections search for an unqualified `command` module + results = merge_hash(results, self._execute_module(module_name='ansible.legacy.command', task_vars=task_vars, wrap_async=wrap_async)) if not wrap_async: # remove a temporary path we created diff --git a/lib/ansible/plugins/action/copy.py b/lib/ansible/plugins/action/copy.py index a20c55f9b95..e56ef99ea11 100644 --- a/lib/ansible/plugins/action/copy.py +++ b/lib/ansible/plugins/action/copy.py @@ -333,7 +333,7 @@ class ActionModule(ActionBase): if lmode: new_module_args['mode'] = lmode - module_return = self._execute_module(module_name='copy', module_args=new_module_args, task_vars=task_vars) + module_return = self._execute_module(module_name='ansible.legacy.copy', module_args=new_module_args, task_vars=task_vars) else: # no need to transfer the file, already correct hash, but still need to call @@ -372,7 +372,7 @@ class ActionModule(ActionBase): new_module_args['mode'] = lmode # Execute the file module. - module_return = self._execute_module(module_name='file', module_args=new_module_args, task_vars=task_vars) + module_return = self._execute_module(module_name='ansible.legacy.file', module_args=new_module_args, task_vars=task_vars) if not module_return.get('checksum'): module_return['checksum'] = local_checksum @@ -448,7 +448,7 @@ class ActionModule(ActionBase): # if we have first_available_file in our vars # look up the files and use the first one we find as src elif remote_src: - result.update(self._execute_module(module_name='copy', task_vars=task_vars)) + result.update(self._execute_module(module_name='ansible.legacy.copy', task_vars=task_vars)) return self._ensure_invocation(result) else: # find_needle returns a path that may not have a trailing slash on @@ -543,7 +543,7 @@ class ActionModule(ActionBase): new_module_args['recurse'] = False del new_module_args['src'] - module_return = self._execute_module(module_name='file', module_args=new_module_args, task_vars=task_vars) + module_return = self._execute_module(module_name='ansible.legacy.file', module_args=new_module_args, task_vars=task_vars) if module_return.get('failed'): result.update(module_return) @@ -569,7 +569,7 @@ class ActionModule(ActionBase): if new_module_args.get('mode', None) == 'preserve': new_module_args.pop('mode') - module_return = self._execute_module(module_name='file', module_args=new_module_args, task_vars=task_vars) + module_return = self._execute_module(module_name='ansible.legacy.file', module_args=new_module_args, task_vars=task_vars) module_executed = True if module_return.get('failed'): diff --git a/lib/ansible/plugins/action/fetch.py b/lib/ansible/plugins/action/fetch.py index 47e23474b06..4f05d2bfad2 100644 --- a/lib/ansible/plugins/action/fetch.py +++ b/lib/ansible/plugins/action/fetch.py @@ -78,7 +78,7 @@ class ActionModule(ActionBase): # use slurp if permissions are lacking or privilege escalation is needed remote_data = None if remote_checksum in ('1', '2', None): - slurpres = self._execute_module(module_name='slurp', module_args=dict(src=source), task_vars=task_vars) + slurpres = self._execute_module(module_name='ansible.legacy.slurp', module_args=dict(src=source), task_vars=task_vars) if slurpres.get('failed'): if not fail_on_missing and (slurpres.get('msg').startswith('file not found') or remote_checksum == '1'): result['msg'] = "the remote file does not exist, not transferring, ignored" diff --git a/lib/ansible/plugins/action/gather_facts.py b/lib/ansible/plugins/action/gather_facts.py index cd181203b54..4276c090b9d 100644 --- a/lib/ansible/plugins/action/gather_facts.py +++ b/lib/ansible/plugins/action/gather_facts.py @@ -21,8 +21,7 @@ class ActionModule(ActionBase): mod_args = self._task.args.copy() # deal with 'setup specific arguments' - if fact_module != 'setup': - + if fact_module not in ['ansible.legacy.setup', 'ansible.builtin.setup', 'setup']: # network facts modules must support gather_subset if self._connection._load_name not in ('network_cli', 'httpapi', 'netconf'): subset = mod_args.pop('gather_subset', None) @@ -68,7 +67,7 @@ class ActionModule(ActionBase): if 'smart' in modules: connection_map = C.config.get_config_value('CONNECTION_FACTS_MODULES', variables=task_vars) network_os = self._task.args.get('network_os', task_vars.get('ansible_network_os', task_vars.get('ansible_facts', {}).get('network_os'))) - modules.extend([connection_map.get(network_os or self._connection._load_name, 'setup')]) + modules.extend([connection_map.get(network_os or self._connection._load_name, 'ansible.legacy.setup')]) modules.pop(modules.index('smart')) failed = {} @@ -104,7 +103,7 @@ class ActionModule(ActionBase): while jobs: for module in jobs: poll_args = {'jid': jobs[module]['ansible_job_id'], '_async_dir': os.path.dirname(jobs[module]['results_file'])} - res = self._execute_module(module_name='async_status', module_args=poll_args, task_vars=task_vars, wrap_async=False) + res = self._execute_module(module_name='ansible.legacy.async_status', module_args=poll_args, task_vars=task_vars, wrap_async=False) if res.get('finished', 0) == 1: if res.get('failed', False): failed[module] = res diff --git a/lib/ansible/plugins/action/normal.py b/lib/ansible/plugins/action/normal.py index 766cebc5a36..1582c8619e1 100644 --- a/lib/ansible/plugins/action/normal.py +++ b/lib/ansible/plugins/action/normal.py @@ -47,6 +47,7 @@ class ActionModule(ActionBase): # hack to keep --verbose from showing all the setup module result # moved from setup module as now we filter out all _ansible_ from result + # FIXME: is this still accurate with gather_facts etc, or does it need support for FQ and other names? if self._task.action == 'setup': result['_ansible_verbose_override'] = True diff --git a/lib/ansible/plugins/action/package.py b/lib/ansible/plugins/action/package.py index c4349ca4d70..c16386588df 100644 --- a/lib/ansible/plugins/action/package.py +++ b/lib/ansible/plugins/action/package.py @@ -29,6 +29,9 @@ class ActionModule(ActionBase): TRANSFERS_FILES = False + BUILTIN_PKG_MGR_MODULES = set(['apk', 'apt', 'dnf', 'homebrew', 'installp', 'macports', 'opkg', 'portage', 'pacman', + 'pkg5', 'pkgin', 'pkgng', 'sorcery', 'svr4pkg', 'swdepot', 'swupd', 'urpmi', 'xbps', 'yum', 'zypper']) + def run(self, tmp=None, task_vars=None): ''' handler for package operations ''' @@ -51,13 +54,15 @@ class ActionModule(ActionBase): try: if module == 'auto': - facts = self._execute_module(module_name='setup', module_args=dict(filter='ansible_pkg_mgr', gather_subset='!all'), task_vars=task_vars) + facts = self._execute_module( + module_name='ansible.legacy.setup', + module_args=dict(filter='ansible_pkg_mgr', gather_subset='!all'), + task_vars=task_vars) display.debug("Facts %s" % facts) module = facts.get('ansible_facts', {}).get('ansible_pkg_mgr', 'auto') if module != 'auto': - - if module not in self._shared_loader_obj.module_loader: + if not self._shared_loader_obj.module_loader.has_plugin(module): raise AnsibleActionFail('Could not find a module for %s.' % module) else: # run the 'package' module @@ -70,6 +75,10 @@ class ActionModule(ActionBase): module, new_module_args, self._task.module_defaults, self._templar, self._task._ansible_internal_redirect_list ) + if module in self.BUILTIN_PKG_MGR_MODULES: + # prefix with ansible.legacy to eliminate external collisions while still allowing library/ override + module = 'ansible.legacy.' + module + display.vvvv("Running %s" % module) result.update(self._execute_module(module_name=module, module_args=new_module_args, task_vars=task_vars, wrap_async=self._task.async_val)) else: diff --git a/lib/ansible/plugins/action/reboot.py b/lib/ansible/plugins/action/reboot.py index 1a7ecfcb134..7d4d6871bc5 100644 --- a/lib/ansible/plugins/action/reboot.py +++ b/lib/ansible/plugins/action/reboot.py @@ -120,11 +120,12 @@ class ActionModule(ActionBase): return args.format(delay_sec=self.pre_reboot_delay, delay_min=delay_min, message=reboot_message) def get_distribution(self, task_vars): + # FIXME: only execute the module if we don't already have the facts we need distribution = {} display.debug('{action}: running setup module to get distribution'.format(action=self._task.action)) module_output = self._execute_module( task_vars=task_vars, - module_name='setup', + module_name='ansible.legacy.setup', module_args={'gather_subset': 'min'}) try: if module_output.get('failed', False): @@ -164,7 +165,8 @@ class ActionModule(ActionBase): paths=search_paths)) find_result = self._execute_module( task_vars=task_vars, - module_name='find', + # prevent collection search by calling with ansible.legacy (still allows library/ override of find) + module_name='ansible.legacy.find', module_args={ 'paths': search_paths, 'patterns': [shutdown_bin], diff --git a/lib/ansible/plugins/action/service.py b/lib/ansible/plugins/action/service.py index c6af6248e61..42d44361ed7 100644 --- a/lib/ansible/plugins/action/service.py +++ b/lib/ansible/plugins/action/service.py @@ -31,6 +31,10 @@ class ActionModule(ActionBase): 'systemd': ['pattern', 'runlevel', 'sleep', 'arguments', 'args'], } + # HACK: list of unqualified service manager names that are/were built-in, we'll prefix these with `ansible.legacy` to + # avoid collisions with collections search + BUILTIN_SVC_MGR_MODULES = set(['openwrt_init', 'service', 'systemd', 'sysvinit']) + def run(self, tmp=None, task_vars=None): ''' handler for package operations ''' @@ -53,12 +57,14 @@ class ActionModule(ActionBase): try: if module == 'auto': - facts = self._execute_module(module_name='setup', module_args=dict(gather_subset='!all', filter='ansible_service_mgr'), task_vars=task_vars) + facts = self._execute_module( + module_name='ansible.legacy.setup', + module_args=dict(gather_subset='!all', filter='ansible_service_mgr'), task_vars=task_vars) self._display.debug("Facts %s" % facts) module = facts.get('ansible_facts', {}).get('ansible_service_mgr', 'auto') - if not module or module == 'auto' or module not in self._shared_loader_obj.module_loader: - module = 'service' + if not module or module == 'auto' or not self._shared_loader_obj.module_loader.has_plugin(module): + module = 'ansible.legacy.service' if module != 'auto': # run the 'service' module @@ -77,6 +83,10 @@ class ActionModule(ActionBase): module, new_module_args, self._task.module_defaults, self._templar, self._task._ansible_internal_redirect_list ) + # collection prefix known internal modules to avoid collisions from collections search, while still allowing library/ overrides + if module in self.BUILTIN_SVC_MGR_MODULES: + module = 'ansible.legacy.' + module + self._display.vvvv("Running %s" % module) result.update(self._execute_module(module_name=module, module_args=new_module_args, task_vars=task_vars, wrap_async=self._task.async_val)) else: diff --git a/lib/ansible/plugins/action/shell.py b/lib/ansible/plugins/action/shell.py index d383535904c..617a373d41a 100644 --- a/lib/ansible/plugins/action/shell.py +++ b/lib/ansible/plugins/action/shell.py @@ -5,7 +5,6 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type from ansible.plugins.action import ActionBase -from ansible.utils.vars import merge_hash class ActionModule(ActionBase): @@ -13,8 +12,7 @@ class ActionModule(ActionBase): def run(self, tmp=None, task_vars=None): del tmp # tmp no longer has any effect - # Shell module is implemented via command - self._task.action = 'command' + # Shell module is implemented via command with a special arg self._task.args['_uses_shell'] = True command_action = self._shared_loader_obj.action_loader.get('ansible.legacy.command', diff --git a/lib/ansible/plugins/action/template.py b/lib/ansible/plugins/action/template.py index f400d7b96ea..645afff27fc 100644 --- a/lib/ansible/plugins/action/template.py +++ b/lib/ansible/plugins/action/template.py @@ -168,7 +168,8 @@ class ActionModule(ActionBase): follow=follow, ), ) - copy_action = self._shared_loader_obj.action_loader.get('copy', + # call with ansible.legacy prefix to eliminate collisions with collections while still allowing local override + copy_action = self._shared_loader_obj.action_loader.get('ansible.legacy.copy', task=new_task, connection=self._connection, play_context=self._play_context, diff --git a/lib/ansible/plugins/action/unarchive.py b/lib/ansible/plugins/action/unarchive.py index c221af8fa9a..4d188e3d34a 100644 --- a/lib/ansible/plugins/action/unarchive.py +++ b/lib/ansible/plugins/action/unarchive.py @@ -101,8 +101,9 @@ class ActionModule(ActionBase): self._fixup_perms2((self._connection._shell.tmpdir, tmp_src)) new_module_args['src'] = tmp_src - # execute the unarchive module now, with the updated args - result.update(self._execute_module(module_args=new_module_args, task_vars=task_vars)) + # execute the unarchive module now, with the updated args (using ansible.legacy prefix to eliminate collections + # collisions with local override + result.update(self._execute_module(module_name='ansible.legacy.unarchive', module_args=new_module_args, task_vars=task_vars)) except AnsibleAction as e: result.update(e.result) finally: diff --git a/lib/ansible/plugins/action/uri.py b/lib/ansible/plugins/action/uri.py index 4f13ef64dd6..5ad04dee95c 100644 --- a/lib/ansible/plugins/action/uri.py +++ b/lib/ansible/plugins/action/uri.py @@ -39,7 +39,9 @@ class ActionModule(ActionBase): if remote_src: # everything is remote, so we just execute the module # without changing any of the module arguments - raise _AnsibleActionDone(result=self._execute_module(task_vars=task_vars, wrap_async=self._task.async_val)) + # call with ansible.legacy prefix to prevent collections collisions while allowing local override + raise _AnsibleActionDone(result=self._execute_module(module_name='ansible.legacy.uri', + task_vars=task_vars, wrap_async=self._task.async_val)) kwargs = {} @@ -83,7 +85,8 @@ class ActionModule(ActionBase): new_module_args = self._task.args.copy() new_module_args.update(kwargs) - result.update(self._execute_module('uri', module_args=new_module_args, task_vars=task_vars, wrap_async=self._task.async_val)) + # call with ansible.legacy prefix to prevent collections collisions while allowing local override + result.update(self._execute_module('ansible.legacy.uri', module_args=new_module_args, task_vars=task_vars, wrap_async=self._task.async_val)) except AnsibleAction as e: result.update(e.result) finally: diff --git a/lib/ansible/plugins/action/wait_for_connection.py b/lib/ansible/plugins/action/wait_for_connection.py index a9fbd45fc91..8489c76721e 100644 --- a/lib/ansible/plugins/action/wait_for_connection.py +++ b/lib/ansible/plugins/action/wait_for_connection.py @@ -88,7 +88,7 @@ class ActionModule(ActionBase): except AttributeError: pass - ping_result = self._execute_module(module_name='ping', module_args=dict(), task_vars=task_vars) + ping_result = self._execute_module(module_name='ansible.legacy.ping', module_args=dict(), task_vars=task_vars) # Test module output if ping_result['ping'] != 'pong': diff --git a/lib/ansible/plugins/action/yum.py b/lib/ansible/plugins/action/yum.py index aee8e987cbd..9d3e1454446 100644 --- a/lib/ansible/plugins/action/yum.py +++ b/lib/ansible/plugins/action/yum.py @@ -59,7 +59,9 @@ class ActionModule(ActionBase): pass # could not get it from template! if module not in VALID_BACKENDS: - facts = self._execute_module(module_name="setup", module_args=dict(filter="ansible_pkg_mgr", gather_subset="!all"), task_vars=task_vars) + facts = self._execute_module( + module_name="ansible.legacy.setup", module_args=dict(filter="ansible_pkg_mgr", gather_subset="!all"), + task_vars=task_vars) display.debug("Facts %s" % facts) module = facts.get("ansible_facts", {}).get("ansible_pkg_mgr", "auto") if (not self._task.delegate_to or self._task.delegate_facts) and module != 'auto': @@ -78,7 +80,10 @@ class ActionModule(ActionBase): if module == "yum4": module = "dnf" - if module not in self._shared_loader_obj.module_loader: + # eliminate collisions with collections search while still allowing local override + module = 'ansible.legacy.' + module + + if not self._shared_loader_obj.module_loader.has_plugin(module): result.update({'failed': True, 'msg': "Could not find a yum module backend for %s." % module}) else: # run either the yum (yum3) or dnf (yum4) backend module @@ -87,7 +92,8 @@ class ActionModule(ActionBase): del new_module_args['use_backend'] display.vvvv("Running %s as the backend for the yum action plugin" % module) - result.update(self._execute_module(module_name=module, module_args=new_module_args, task_vars=task_vars, wrap_async=self._task.async_val)) + result.update(self._execute_module( + module_name=module, module_args=new_module_args, task_vars=task_vars, wrap_async=self._task.async_val)) # Cleanup if not self._task.async_val: diff --git a/test/integration/targets/playbook/timeout.yml b/test/integration/targets/playbook/timeout.yml index d576fa80a42..442e13aed5b 100644 --- a/test/integration/targets/playbook/timeout.yml +++ b/test/integration/targets/playbook/timeout.yml @@ -9,4 +9,4 @@ - assert: that: - time is failed - - '"The command action failed to execute in the expected time frame" in time["msg"]' + - '"The shell action failed to execute in the expected time frame" in time["msg"]' diff --git a/test/units/executor/test_task_executor.py b/test/units/executor/test_task_executor.py index 300fd339d90..b27faf9b616 100644 --- a/test/units/executor/test_task_executor.py +++ b/test/units/executor/test_task_executor.py @@ -291,7 +291,7 @@ class TestTaskExecutor(unittest.TestCase): mock.call(module_prefix, collection_list=te._task.collections)]) action_loader.get.assert_called_once_with( - 'normal', task=te._task, connection=mock_connection, + 'ansible.legacy.normal', task=te._task, connection=mock_connection, play_context=te._play_context, loader=te._loader, templar=mock_templar, shared_loader_obj=te._shared_loader_obj, collection_list=None) diff --git a/test/units/plugins/action/test_gather_facts.py b/test/units/plugins/action/test_gather_facts.py index 6d81200014c..e15edd396f8 100644 --- a/test/units/plugins/action/test_gather_facts.py +++ b/test/units/plugins/action/test_gather_facts.py @@ -63,7 +63,7 @@ class TestNetworkFacts(unittest.TestCase): self.assertEqual(mod_args['gather_subset'], 'min') facts_modules = C.config.get_config_value('FACTS_MODULES', variables=self.task_vars) - self.assertEqual(facts_modules, ['ios_facts']) + self.assertEqual(facts_modules, ['ansible.legacy.ios_facts']) @patch.object(module_common, '_get_collection_metadata', return_value={}) def test_network_gather_facts_fqcn(self, mock_collection_metadata):