fix delegation vars usage (debug still shows inventory_hostname (#69520)

* fix delegation vars usage (debug still shows inventory_hostname) (#69244)

* fix delegation vars usage and reporting

 - just pass delegated host vars + task vars to plugins
   and avoid poluting with original host vars
 - updated tests

(cherry picked from commit 2165f9ac40)

* fix delegated interpreter discovery (#69604)

* fix delegated interpeter
* allow returning fact if it is 'the right host'
* added note for future fix/efficiency
 as it stands we rerun discovery for the delegated host
unless its saving facts to itself
 * fixed test lacking delegate_to mock

(cherry picked from commit de3f7c7739)

* Fix `ansible -K` become_pass regression (#69629)

* Fix `ansible -K` become_pass regression

Change:
- This fixes a breaking change introduced in
  2165f9ac40

Test Plan:
- Local VM for now, with plans to add an integration test for -K going
  forward.

Tickets:
Refs #69244

(cherry picked from commit fe9696be52)

* fix discovery on loop with delegation (#70013)

* fix discovery on loop with delegation

fixes #69963

(cherry picked from commit 4c9d9dbb56)

Co-authored-by: Rick Elrod <rick@elrod.me>

changelogs/fragments/discovery_delegation_fix.yml

@@ -0,0 +1,3 @@
+bugfixes:
+    - interpreter discovery will now use correct vars (from delegated host) when in delegate_to task.
+    - discovery will NOT update incorrect host anymore when in delegate_to task.

changelogs/fragments/ensure_discovery_delegate.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - ensure we pass on interpreter discovery values to delegated host.

lib/ansible/executor/task_executor.py

@@ -28,7 +28,7 @@ from ansible.plugins.loader import become_loader, cliconf_loader, connection_loa
 from ansible.template import Templar
 from ansible.utils.collection_loader import AnsibleCollectionLoader
 from ansible.utils.listify import listify_lookup_plugin_terms
-from ansible.utils.unsafe_proxy import AnsibleUnsafe, to_unsafe_text, wrap_var
+from ansible.utils.unsafe_proxy import to_unsafe_text, wrap_var
 from ansible.vars.clean import namespace_facts, clean_facts
 from ansible.utils.display import Display
 from ansible.utils.vars import combine_vars, isidentifier
@@ -604,7 +604,16 @@ class TaskExecutor:
             # to be replaced with the one templated above, in case other data changed
             self._connection._play_context = self._play_context
 
-        self._set_connection_options(variables, templar)
+        if self._task.delegate_to:
+            # use vars from delegated host (which already include task vars) instead of original host
+            delegated_vars = variables.get('ansible_delegated_vars', {}).get(self._task.delegate_to, {})
+            orig_vars = templar.available_variables
+            templar.available_variables = delegated_vars
+            plugin_vars = self._set_connection_options(delegated_vars, templar)
+            templar.available_variables = orig_vars
+        else:
+            # just use normal host vars
+            plugin_vars = self._set_connection_options(variables, templar)
 
         # get handler
         self._handler = self._get_action_handler(connection=self._connection, templar=templar)
@@ -771,15 +780,10 @@ class TaskExecutor:
 
         # add the delegated vars to the result, so we can reference them
         # on the results side without having to do any further templating
-        # FIXME: we only want a limited set of variables here, so this is currently
+        if self._task.delegate_to:
-        #        hardcoded but should be possibly fixed if we want more or if
-        #        there is another source of truth we can use
-        delegated_vars = variables.get('ansible_delegated_vars', dict()).get(self._task.delegate_to, dict()).copy()
-        if len(delegated_vars) > 0:
             result["_ansible_delegated_vars"] = {'ansible_delegated_host': self._task.delegate_to}
-            for k in ('ansible_host', ):
+            for k in plugin_vars:
                 result["_ansible_delegated_vars"][k] = delegated_vars.get(k)
-
         # and return
         display.debug("attempt loop complete, returning result")
         return result
@@ -872,21 +876,20 @@ class TaskExecutor:
         '''
 
         if self._task.delegate_to is not None:
-            # since we're delegating, we don't want to use interpreter values
+            cvars = variables.get('ansible_delegated_vars', {}).get(self._task.delegate_to, {})
-            # which would have been set for the original target host
+        else:
-            for i in list(variables.keys()):
+            cvars = variables
-                if isinstance(i, string_types) and i.startswith('ansible_') and i.endswith('_interpreter'):
+
-                    del variables[i]
+        # use magic var if it exists, if not, let task inheritance do it's thing.
-            # now replace the interpreter values with those that may have come
+        self._play_context.connection = cvars.get('ansible_connection', self._task.connection)
-            # from the delegated-to host
+
-            delegated_vars = variables.get('ansible_delegated_vars', dict()).get(self._task.delegate_to, dict())
+        # TODO: play context has logic to update the conneciton for 'smart'
-            if isinstance(delegated_vars, dict):
+        # (default value, will chose between ssh and paramiko) and 'persistent'
-                for i in delegated_vars:
+        # (really paramiko), evnentually this should move to task object itself.
-                    if isinstance(i, string_types) and i.startswith("ansible_") and i.endswith("_interpreter"):
+        connection_name = self._play_context.connection
-                        variables[i] = delegated_vars[i]
 
         # load connection
-        conn_type = self._play_context.connection
+        conn_type = connection_name
         connection = self._shared_loader_obj.connection_loader.get(
             conn_type,
             self._play_context,
@@ -899,28 +902,27 @@ class TaskExecutor:
             raise AnsibleError("the connection plugin '%s' was not found" % conn_type)
 
         # load become plugin if needed
-        become_plugin = None
+        if cvars.get('ansible_become', self._task.become):
-        if self._play_context.become:
+            become_plugin = self._get_become(cvars.get('ansible_become_method', self._task.become_method))
-            become_plugin = self._get_become(self._play_context.become_method)
-
-        if getattr(become_plugin, 'require_tty', False) and not getattr(connection, 'has_tty', False):
-            raise AnsibleError(
-                "The '%s' connection does not provide a tty which is required for the selected "
-                "become plugin: %s." % (conn_type, become_plugin.name)
-            )
 
             try:
                 connection.set_become_plugin(become_plugin)
             except AttributeError:
-            # Connection plugin does not support set_become_plugin
+                # Older connection plugin that does not support set_become_plugin
                 pass
 
+            if getattr(connection.become, 'require_tty', False) and not getattr(connection, 'has_tty', False):
+                raise AnsibleError(
+                    "The '%s' connection does not provide a TTY which is required for the selected "
+                    "become plugin: %s." % (conn_type, become_plugin.name)
+                )
+
             # Backwards compat for connection plugins that don't support become plugins
             # Just do this unconditionally for now, we could move it inside of the
             # AttributeError above later
-        self._play_context.set_become_plugin(become_plugin)
+            self._play_context.set_become_plugin(become_plugin.name)
 
-        # FIXME: remove once all plugins pull all data from self._options
+        # Also backwards compat call for those still using play_context
         self._play_context.set_attributes_from_plugin(connection)
 
         if any(((connection.supports_persistence and C.USE_PERSISTENT_CONNECTIONS), connection.force_persistence)):
@@ -956,6 +958,7 @@ class TaskExecutor:
         except AttributeError:
             # Some plugins are assigned to private attrs, ``become`` is not
             plugin = getattr(self._connection, plugin_type)
+
         option_vars = C.config.get_plugin_vars(plugin_type, plugin._load_name)
         options = {}
         for k in option_vars:
@@ -964,54 +967,55 @@ class TaskExecutor:
         # TODO move to task method?
         plugin.set_options(task_keys=task_keys, var_options=options)
 
-    def _set_connection_options(self, variables, templar):
+        return option_vars
 
-        # Keep the pre-delegate values for these keys
+    def _set_connection_options(self, variables, templar):
-        PRESERVE_ORIG = ('inventory_hostname',)
 
-        # create copy with delegation built in
+        # keep list of variable names possibly consumed
-        final_vars = combine_vars(
+        varnames = []
-            variables,
-            variables.get('ansible_delegated_vars', {}).get(self._task.delegate_to, {})
-        )
 
         # grab list of usable vars for this plugin
         option_vars = C.config.get_plugin_vars('connection', self._connection._load_name)
+        varnames.extend(option_vars)
 
         # create dict of 'templated vars'
         options = {'_extras': {}}
         for k in option_vars:
-            if k in PRESERVE_ORIG:
+            if k in variables:
                 options[k] = templar.template(variables[k])
-            elif k in final_vars:
-                options[k] = templar.template(final_vars[k])
 
         # add extras if plugin supports them
         if getattr(self._connection, 'allow_extras', False):
-            for k in final_vars:
+            for k in variables:
                 if k.startswith('ansible_%s_' % self._connection._load_name) and k not in options:
-                    options['_extras'][k] = templar.template(final_vars[k])
+                    options['_extras'][k] = templar.template(variables[k])
 
         task_keys = self._task.dump_attrs()
 
-        # set options with 'templated vars' specific to this plugin and dependant ones
+        # set options with 'templated vars' specific to this plugin and dependent ones
         self._connection.set_options(task_keys=task_keys, var_options=options)
-        self._set_plugin_options('shell', final_vars, templar, task_keys)
+        varnames.extend(self._set_plugin_options('shell', variables, templar, task_keys))
 
         if self._connection.become is not None:
-            # FIXME: find alternate route to provide passwords,
+            if self._play_context.become_pass:
-            # keep out of play objects to avoid accidental disclosure
+                # FIXME: eventually remove from task and play_context, here for backwards compat
+                # keep out of play objects to avoid accidental disclosure, only become plugin should have
+                # The become pass is already in the play_context if given on
+                # the CLI (-K). Make the plugin aware of it in this case.
                 task_keys['become_pass'] = self._play_context.become_pass
-            self._set_plugin_options('become', final_vars, templar, task_keys)
+
+            varnames.extend(self._set_plugin_options('become', variables, templar, task_keys))
 
             # FOR BACKWARDS COMPAT:
-            for option in ('become_user', 'become_flags', 'become_exe'):
+            for option in ('become_user', 'become_flags', 'become_exe', 'become_pass'):
                 try:
                     setattr(self._play_context, option, self._connection.become.get_option(option))
                 except KeyError:
                     pass  # some plugins don't support all base flags
             self._play_context.prompt = self._connection.become.prompt
 
+        return varnames
+
     def _get_action_handler(self, connection, templar):
         '''
         Returns the correct action plugin to handle the requestion task action

lib/ansible/playbook/task.py

@@ -23,8 +23,8 @@ import os
 
 from ansible import constants as C
 from ansible.errors import AnsibleError, AnsibleParserError, AnsibleUndefinedVariable, AnsibleAssertionError
-from ansible.module_utils.six import iteritems, string_types
 from ansible.module_utils._text import to_native
+from ansible.module_utils.six import iteritems, string_types
 from ansible.parsing.mod_args import ModuleArgsParser
 from ansible.parsing.yaml.objects import AnsibleBaseYAMLObject, AnsibleMapping
 from ansible.plugins.loader import lookup_loader

lib/ansible/plugins/action/__init__.py

@@ -157,8 +157,14 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         Handles the loading and templating of the module code through the
         modify_module() function.
         '''
+
         if task_vars is None:
-            task_vars = dict()
+            use_vars = dict()
+
+        if self._task.delegate_to:
+            use_vars = task_vars.get('ansible_delegated_vars')[self._task.delegate_to]
+        else:
+            use_vars = task_vars
 
         # Search module path(s) for named module.
         for mod_type in self._connection.module_implementation_preferences:
@@ -202,7 +208,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         for dummy in (1, 2):
             try:
                 (module_data, module_style, module_shebang) = modify_module(module_name, module_path, module_args, self._templar,
-                                                                            task_vars=task_vars,
+                                                                            task_vars=use_vars,
                                                                             module_compression=self._play_context.module_compression,
                                                                             async_timeout=self._task.async_val,
                                                                             environment=final_environment,
@@ -213,17 +219,27 @@ class ActionBase(with_metaclass(ABCMeta, object)):
                     action=self,
                     interpreter_name=idre.interpreter_name,
                     discovery_mode=idre.discovery_mode,
-                    task_vars=task_vars))
+                    task_vars=use_vars))
 
                 # update the local task_vars with the discovered interpreter (which might be None);
                 # we'll propagate back to the controller in the task result
                 discovered_key = 'discovered_interpreter_%s' % idre.interpreter_name
+
+                # TODO: this condition prevents 'wrong host' from being updated
+                # but in future we would want to be able to update 'delegated host facts'
+                # irrespective of task settings
+                if not self._task.delegate_to or self._task.delegate_facts:
                     # store in local task_vars facts collection for the retry and any other usages in this worker
-                if task_vars.get('ansible_facts') is None:
+                    if use_vars.get('ansible_facts') is None:
                         task_vars['ansible_facts'] = {}
                     task_vars['ansible_facts'][discovered_key] = self._discovered_interpreter
                     # preserve this so _execute_module can propagate back to controller as a fact
                     self._discovered_interpreter_key = discovered_key
+                else:
+                    task_vars['ansible_delegated_vars'][self._task.delegate_to]
+                    if task_vars['ansible_delegated_vars'][self._task.delegate_to].get('ansible_facts') is None:
+                        task_vars['ansible_delegated_vars'][self._task.delegate_to]['ansible_facts'] = {}
+                    task_vars['ansible_delegated_vars'][self._task.delegate_to]['ansible_facts'][discovered_key] = self._discovered_interpreter
 
         return (module_style, module_shebang, module_data, module_path)
 

lib/ansible/vars/manager.py

@@ -538,6 +538,7 @@ class VariableManager:
             has_loop = False
             items = [None]
 
+        # since host can change per loop, we keep dict per host name resolved
         delegated_host_vars = dict()
         item_var = getattr(task.loop_control, 'loop_var', 'item')
         cache_items = False
@@ -554,28 +555,13 @@ class VariableManager:
                 raise AnsibleError(message="Undefined delegate_to host for task:", obj=task._ds)
             if not isinstance(delegated_host_name, string_types):
                 raise AnsibleError(message="the field 'delegate_to' has an invalid type (%s), and could not be"
-                                           " converted to a string type." % type(delegated_host_name),
+                                           " converted to a string type." % type(delegated_host_name), obj=task._ds)
-                                   obj=task._ds)
+
             if delegated_host_name in delegated_host_vars:
                 # no need to repeat ourselves, as the delegate_to value
                 # does not appear to be tied to the loop item variable
                 continue
 
-            # a dictionary of variables to use if we have to create a new host below
-            # we set the default port based on the default transport here, to make sure
-            # we use the proper default for windows
-            new_port = C.DEFAULT_REMOTE_PORT
-            if C.DEFAULT_TRANSPORT == 'winrm':
-                new_port = 5986
-
-            new_delegated_host_vars = dict(
-                ansible_delegated_host=delegated_host_name,
-                ansible_host=delegated_host_name,  # not redundant as other sources can change ansible_host
-                ansible_port=new_port,
-                ansible_user=C.DEFAULT_REMOTE_USER,
-                ansible_connection=C.DEFAULT_TRANSPORT,
-            )
-
             # now try to find the delegated-to host in inventory, or failing that,
             # create a new host on the fly so we can fetch variables for it
             delegated_host = None
@@ -584,9 +570,6 @@ class VariableManager:
                 # try looking it up based on the address field, and finally
                 # fall back to creating a host on the fly to use for the var lookup
                 if delegated_host is None:
-                    if delegated_host_name in C.LOCALHOST:
-                        delegated_host = self._inventory.localhost
-                    else:
                     for h in self._inventory.get_hosts(ignore_limits=True, ignore_restrictions=True):
                         # check if the address matches, or if both the delegated_to host
                         # and the current host are in the list of localhost aliases
@@ -595,10 +578,8 @@ class VariableManager:
                             break
                     else:
                         delegated_host = Host(name=delegated_host_name)
-                            delegated_host.vars = combine_vars(delegated_host.vars, new_delegated_host_vars)
             else:
                 delegated_host = Host(name=delegated_host_name)
-                delegated_host.vars = combine_vars(delegated_host.vars, new_delegated_host_vars)
 
             # now we go fetch the vars for the delegated-to host and save them in our
             # master dictionary of variables to be used later in the TaskExecutor/PlayContext

test/integration/targets/cli/aliases

@@ -1,2 +1,5 @@
+destructive
+needs/root
+needs/ssh
 needs/target/setup_pexpect
 shippable/posix/group3

test/integration/targets/cli/setup.yml

@@ -1,4 +1,42 @@
 - hosts: localhost
-  gather_facts: no
+  gather_facts: yes
   roles:
     - setup_pexpect
+
+  tasks:
+    - name: Test ansible-playbook and ansible with -K
+      block:
+        - name: Create user to connect as
+          user:
+            name: cliuser1
+            shell: /bin/bash
+            groups: wheel
+            append: yes
+            password: "{{ 'secretpassword' | password_hash('sha512', 'mysecretsalt') }}"
+        - name: Create user to become
+          user:
+            name: cliuser2
+            shell: /bin/bash
+            password: "{{ 'secretpassword' | password_hash('sha512', 'mysecretsalt') }}"
+        # Sometimes this file doesn't get removed, and we need it gone to ssh
+        - name: Remove /run/nologin
+          file:
+            path: /run/nologin
+            state: absent
+        # Make Ansible run Python to run Ansible
+        - name: Run the test
+          shell: python test_k_and_K.py {{ ansible_python_interpreter }}
+      always:
+        - name: Remove users
+          user:
+            name: "{{ item }}"
+            state: absent
+          with_items:
+            - cliuser1
+            - cliuser2
+      # For now, we don't test this everywhere, because `user` works differently
+      # on some platforms, as does sudo/sudoers. On Fedora, we can just add
+      # the user to 'wheel' and things magically work.
+      # TODO: In theory, we should test this with all the different 'become'
+      # plugins in base.
+      when: ansible_distribution == 'Fedora'

test/integration/targets/cli/test_k_and_K.py

@@ -0,0 +1,27 @@
+#!/usr/bin/env python
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+# Make coding more python3-ish
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import os
+import sys
+
+import pexpect
+
+os.environ['ANSIBLE_NOCOLOR'] = '1'
+
+out = pexpect.run(
+    'ansible -c ssh -i localhost, -u cliuser1 -e ansible_python_interpreter={0} '
+    '-m command -a whoami -Kkb --become-user cliuser2 localhost'.format(sys.argv[1]),
+    events={
+        'SSH password:': 'secretpassword\n',
+        'BECOME password': 'secretpassword\n',
+    },
+    timeout=10
+)
+
+print(out)
+
+assert b'cliuser2' in out

test/integration/targets/delegate_to/delegate_vars_hanldling.yml

@@ -0,0 +1,58 @@
+- name: setup delegated hsot
+  hosts: localhost
+  gather_facts: false
+  tasks:
+    - add_host:
+        name: delegatetome
+        ansible_host: 127.0.0.4
+
+- name: ensure we dont use orig host vars if delegated one does not define them
+  hosts: testhost
+  gather_facts: false
+  connection: local
+  tasks:
+    - name: force current host to use winrm
+      set_fact:
+        ansible_connection: winrm
+
+    - name: this should fail (missing winrm or unreachable)
+      ping:
+      ignore_errors: true
+      ignore_unreachable: true
+      register: orig
+
+    - name: ensure prev failed
+      assert:
+        that:
+            - orig is failed or orig is unreachable
+
+    - name: this will only fail if we take orig host ansible_connection instead of defaults
+      ping:
+      delegate_to: delegatetome
+
+
+- name: ensure plugin specific vars are properly used
+  hosts: testhost
+  gather_facts: false
+  tasks:
+    - name: set unusable ssh args
+      set_fact:
+         ansible_host: 127.0.0.1
+         ansible_connection: ssh
+         ansible_ssh_common_args: 'MEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE'
+         ansible_connection_timeout: 5
+
+    - name: fail to ping with bad args
+      ping:
+      register: bad_args_ping
+      ignore_unreachable: true
+
+    - debug: var=bad_args_ping
+    - name: ensure prev failed
+      assert:
+        that:
+            - bad_args_ping is failed or bad_args_ping is unreachable
+
+    - name: this should work by ignoring the bad ags for orig host
+      ping:
+      delegate_to: delegatetome

test/integration/targets/delegate_to/discovery_applied.yml

@@ -0,0 +1,8 @@
+- hosts: testhost
+  gather_facts: no
+  tasks:
+  - command: ls
+    delegate_to: "{{ item }}"
+    with_items:
+    - localhost
+    - "{{ inventory_hostname }}"

test/integration/targets/delegate_to/inventory_interpreters

@@ -0,0 +1,5 @@
+testhost ansible_python_interpreter=firstpython
+testhost2 ansible_python_interpreter=secondpython
+
+[all:vars]
+ansible_connection=local

test/integration/targets/delegate_to/library/detect_interpreter.py

@@ -0,0 +1,18 @@
+#!/usr/bin/python
+
+from __future__ import absolute_import, division, print_function
+
+__metaclass__ = type
+
+import sys
+
+from ansible.module_utils.basic import AnsibleModule
+
+
+def main():
+    module = AnsibleModule(argument_spec={})
+    module.exit_json(**dict(found=sys.executable))
+
+
+if __name__ == '__main__':
+    main()

test/integration/targets/delegate_to/runme.sh

@@ -55,3 +55,18 @@ ansible-playbook delegate_and_nolog.yml -i inventory -v "$@"
 ansible-playbook delegate_facts_block.yml -i inventory -v "$@"
 
 ansible-playbook test_delegate_to_loop_caching.yml -i inventory -v "$@"
+
+# ensure we are using correct settings when delegating
+ANSIBLE_TIMEOUT=3 ansible-playbook delegate_vars_hanldling.yml -i inventory -v "$@"
+
+
+# test ansible_x_interpreter
+# python
+source virtualenv.sh
+(
+cd "${OUTPUT_DIR}"/venv/bin
+ln -s python firstpython
+ln -s python secondpython
+)
+ansible-playbook verify_interpreter.yml -i inventory_interpreters -v "$@"
+ansible-playbook discovery_applied.yml -i inventory -v "$@"

test/integration/targets/delegate_to/verify_interpreter.yml

@@ -0,0 +1,47 @@
+- name: ensure they are different
+  hosts: localhost
+  tasks:
+    - name: dont game me
+      assert:
+        msg: 'expected different values but got ((hostvars["testhost"]["ansible_python_interpreter"]}} and {{hostvars["testhost2"]["ansible_python_interpreter"]}}'
+        that:
+           - hostvars["testhost"]["ansible_python_interpreter"] != hostvars["testhost2"]["ansible_python_interpreter"]
+
+- name: no delegation
+  hosts: all
+  gather_facts: false
+  tasks:
+    - name: detect interpreter used by each host
+      detect_interpreter:
+      register: baseline
+
+    - name: verify it
+      assert:
+        msg: 'expected {{ansible_python_interpreter}} but got {{baseline.found|basename}}'
+        that:
+           - baseline.found|basename == ansible_python_interpreter
+
+- name: actual test
+  hosts: testhost
+  gather_facts: false
+  tasks:
+    - name: original host
+      detect_interpreter:
+      register: found
+
+    - name: verify it orig host
+      assert:
+        msg: 'expected {{ansible_python_interpreter}} but got {{found.found|basename}}'
+        that:
+           - found.found|basename == ansible_python_interpreter
+
+    - name: delegated host
+      detect_interpreter:
+      register: found2
+      delegate_to: testhost2
+
+    - name: verify it delegated
+      assert:
+        msg: 'expected {{hostvars["testhost2"]["ansible_python_interpreter"]}} but got {{found2.found|basename}}'
+        that:
+           - found2.found|basename == hostvars["testhost2"]["ansible_python_interpreter"]

test/units/plugins/action/test_action.py

@@ -116,6 +116,7 @@ class TestActionBase(unittest.TestCase):
         mock_task = MagicMock()
         mock_task.action = "copy"
         mock_task.async_val = 0
+        mock_task.delegate_to = None
 
         # create a mock connection, so we don't actually try and connect to things
         mock_connection = MagicMock()