From 611d0e4dcf6113e676117da95cd113176c1c57be Mon Sep 17 00:00:00 2001
From: Brian Coca <bcoca@users.noreply.github.com>
Date: Mon, 4 Dec 2023 10:19:12 -0500
Subject: [PATCH] Better errors for delegate_to (#82319)

Handle empty result of templating
  Also skip work when we omit

(cherry picked from commit 6ebefaceb6cd0d4961776a94d63a71fc1fc28bc0)
---
 changelogs/fragments/delegate_to_invalid.yml  |  2 +
 lib/ansible/executor/task_executor.py         |  6 +--
 lib/ansible/vars/manager.py                   | 50 +++++++++++--------
 .../targets/delegate_to/test_delegate_to.yml  | 19 +++++++
 4 files changed, 51 insertions(+), 26 deletions(-)
 create mode 100644 changelogs/fragments/delegate_to_invalid.yml

diff --git a/changelogs/fragments/delegate_to_invalid.yml b/changelogs/fragments/delegate_to_invalid.yml
new file mode 100644
index 00000000000..5eca5f189ba
--- /dev/null
+++ b/changelogs/fragments/delegate_to_invalid.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - delegate_to when set to an empty or undefined variable will now give a proper error.
diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py
index 59c9eae40e7..0e7394f693c 100644
--- a/lib/ansible/executor/task_executor.py
+++ b/lib/ansible/executor/task_executor.py
@@ -408,11 +408,7 @@ class TaskExecutor:
         """This method is responsible for effectively pre-validating Task.delegate_to and will
         happen before Task.post_validate is executed
         """
-        delegated_vars, delegated_host_name = self._variable_manager.get_delegated_vars_and_hostname(
-            templar,
-            self._task,
-            variables
-        )
+        delegated_vars, delegated_host_name = self._variable_manager.get_delegated_vars_and_hostname(templar, self._task, variables)
         # At the point this is executed it is safe to mutate self._task,
         # since `self._task` is either a copy referred to by `tmp_task` in `_run_loop`
         # or just a singular non-looped task
diff --git a/lib/ansible/vars/manager.py b/lib/ansible/vars/manager.py
index d210eacd8e2..8282190e4e0 100644
--- a/lib/ansible/vars/manager.py
+++ b/lib/ansible/vars/manager.py
@@ -524,27 +524,35 @@ class VariableManager:
         delegated_host_name = None
         if task.delegate_to:
             delegated_host_name = templar.template(task.delegate_to, fail_on_undefined=False)
-            delegated_host = self._inventory.get_host(delegated_host_name)
-            if delegated_host is None:
-                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
-                    if h.address == delegated_host_name:
-                        delegated_host = h
-                        break
-                else:
-                    delegated_host = Host(name=delegated_host_name)
-
-            delegated_vars['ansible_delegated_vars'] = {
-                delegated_host_name: self.get_vars(
-                    play=task.get_play(),
-                    host=delegated_host,
-                    task=task,
-                    include_delegate_to=False,
-                    include_hostvars=True,
-                )
-            }
-            delegated_vars['ansible_delegated_vars'][delegated_host_name]['inventory_hostname'] = variables.get('inventory_hostname')
+
+            # no need to do work if omitted
+            if delegated_host_name != self._omit_token:
+
+                if not delegated_host_name:
+                    raise AnsibleError('Empty hostname produced from delegate_to: "%s"' % task.delegate_to)
+
+                delegated_host = self._inventory.get_host(delegated_host_name)
+                if delegated_host is None:
+                    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
+                        if h.address == delegated_host_name:
+                            delegated_host = h
+                            break
+                    else:
+                        delegated_host = Host(name=delegated_host_name)
+
+                delegated_vars['ansible_delegated_vars'] = {
+                    delegated_host_name: self.get_vars(
+                        play=task.get_play(),
+                        host=delegated_host,
+                        task=task,
+                        include_delegate_to=False,
+                        include_hostvars=True,
+                    )
+                }
+                delegated_vars['ansible_delegated_vars'][delegated_host_name]['inventory_hostname'] = variables.get('inventory_hostname')
+
         return delegated_vars, delegated_host_name
 
     def _get_delegated_vars(self, play, task, existing_variables):
diff --git a/test/integration/targets/delegate_to/test_delegate_to.yml b/test/integration/targets/delegate_to/test_delegate_to.yml
index 34cfd209423..eb601e02ba6 100644
--- a/test/integration/targets/delegate_to/test_delegate_to.yml
+++ b/test/integration/targets/delegate_to/test_delegate_to.yml
@@ -57,6 +57,25 @@
     - name: remove test file
       file: path={{ output_dir }}/tmp.txt state=absent
 
+    - name: Use omit to thwart delegation
+      ping:
+      delegate_to: "{{ jenkins_install_key_on|default(omit) }}"
+      register: d_omitted
+
+    - name: Use empty to thwart delegation should fail
+      ping:
+      delegate_to: "{{ jenkins_install_key_on }}"
+      when: jenkins_install_key_on != ""
+      vars:
+        jenkins_install_key_on: ''
+      ignore_errors: true
+      register: d_empty
+
+    - name: Ensure previous 2 tests actually did what was expected
+      assert:
+        that:
+          - d_omitted is success
+          - d_empty is failed
 
 - name: verify delegation with per host vars
   hosts: testhost6