Fix notifying handlers by using an exact match (#55624)

* Fix notifying handlers by using an exact match rather than a string subset if listen is text rather than a list

* Enforce better type checking for listeners

* Share code for validating handler listeners

* Add test for handlers without names

* Add test for templating in handlers

* Add test for include_role

* Add a couple notes about 'listen' for handlers

* changelog

(cherry picked from commit ec1287ca7e)

changelogs/fragments/55575_notify_handlers_by_exact_match_only.yaml

@@ -0,0 +1,3 @@
+bugfixes:
+  - handlers - Only notify a handler if the handler is an exact match by ensuring `listen` is a list of strings.
+    (https://github.com/ansible/ansible/issues/55575)

docs/docsite/keyword_desc.yml

@@ -34,7 +34,7 @@ force_handlers: Will force notified handler execution for hosts even if they fai
 gather_facts: "A boolean that controls if the play will automatically run the 'setup' task to gather facts for the hosts."
 gather_subset: Allows you to pass subset options to the  fact gathering plugin controlled by :term:`gather_facts`.
 gather_timeout: Allows you to set the timeout for the fact gathering plugin controlled by :term:`gather_facts`.
-handlers: "A section with tasks that are treated as handlers, these won't get executed normally, only when notified after each section of tasks is complete."
+handlers: "A section with tasks that are treated as handlers, these won't get executed normally, only when notified after each section of tasks is complete. A handler's `listen` field is not templatable."
 hosts: "A list of groups, hosts or host pattern that translates into a list of hosts that are the play's target."
 ignore_errors: Boolean that allows you to ignore task failures and continue with play. It does not affect connection errors.
 ignore_unreachable: Boolean that allows you to ignore unreachable hosts and continue with play. This does not affect other task errors (see :term:`ignore_errors`) but is useful for groups of volatile/ephemeral hosts.

docs/docsite/rst/user_guide/playbooks_intro.rst

@@ -454,6 +454,7 @@ a shared source like Galaxy).
 .. note::
    * Notify handlers are always run in the same order they are defined, `not` in the order listed in the notify-statement. This is also the case for handlers using `listen`.
    * Handler names and `listen` topics live in a global namespace.
+   * Handler names are templatable and `listen` topics are not.
    * Use unique handler names. If you trigger more than one handler with the same name, the first one(s) get overwritten. Only the last one defined will run.
    * You cannot notify a handler that is defined inside of an include. As of Ansible 2.1, this does work, however the include must be `static`.
 

lib/ansible/playbook/base.py

@@ -339,6 +339,57 @@ class FieldAttributeBase(with_metaclass(BaseMeta, object)):
 
         return new_me
 
+    def get_validated_value(self, name, attribute, value, templar):
+        if attribute.isa == 'string':
+            value = to_text(value)
+        elif attribute.isa == 'int':
+            value = int(value)
+        elif attribute.isa == 'float':
+            value = float(value)
+        elif attribute.isa == 'bool':
+            value = boolean(value, strict=False)
+        elif attribute.isa == 'percent':
+            # special value, which may be an integer or float
+            # with an optional '%' at the end
+            if isinstance(value, string_types) and '%' in value:
+                value = value.replace('%', '')
+            value = float(value)
+        elif attribute.isa == 'list':
+            if value is None:
+                value = []
+            elif not isinstance(value, list):
+                value = [value]
+            if attribute.listof is not None:
+                for item in value:
+                    if not isinstance(item, attribute.listof):
+                        raise AnsibleParserError("the field '%s' should be a list of %s, "
+                                                 "but the item '%s' is a %s" % (name, attribute.listof, item, type(item)), obj=self.get_ds())
+                    elif attribute.required and attribute.listof == string_types:
+                        if item is None or item.strip() == "":
+                            raise AnsibleParserError("the field '%s' is required, and cannot have empty values" % (name,), obj=self.get_ds())
+        elif attribute.isa == 'set':
+            if value is None:
+                value = set()
+            elif not isinstance(value, (list, set)):
+                if isinstance(value, string_types):
+                    value = value.split(',')
+                else:
+                    # Making a list like this handles strings of
+                    # text and bytes properly
+                    value = [value]
+            if not isinstance(value, set):
+                value = set(value)
+        elif attribute.isa == 'dict':
+            if value is None:
+                value = dict()
+            elif not isinstance(value, dict):
+                raise TypeError("%s is not a dictionary" % value)
+        elif attribute.isa == 'class':
+            if not isinstance(value, attribute.class_type):
+                raise TypeError("%s is not a valid %s (got a %s instead)" % (name, attribute.class_type, type(value)))
+            value.post_validate(templar=templar)
+        return value
+
     def post_validate(self, templar):
         '''
         we can't tell that everything is of the right type until we have
@@ -394,54 +445,7 @@ class FieldAttributeBase(with_metaclass(BaseMeta, object)):
 
                 # and make sure the attribute is of the type it should be
                 if value is not None:
-                    if attribute.isa == 'string':
+                    value = self.get_validated_value(name, attribute, value, templar)
-                        value = to_text(value)
-                    elif attribute.isa == 'int':
-                        value = int(value)
-                    elif attribute.isa == 'float':
-                        value = float(value)
-                    elif attribute.isa == 'bool':
-                        value = boolean(value, strict=False)
-                    elif attribute.isa == 'percent':
-                        # special value, which may be an integer or float
-                        # with an optional '%' at the end
-                        if isinstance(value, string_types) and '%' in value:
-                            value = value.replace('%', '')
-                        value = float(value)
-                    elif attribute.isa == 'list':
-                        if value is None:
-                            value = []
-                        elif not isinstance(value, list):
-                            value = [value]
-                        if attribute.listof is not None:
-                            for item in value:
-                                if not isinstance(item, attribute.listof):
-                                    raise AnsibleParserError("the field '%s' should be a list of %s, "
-                                                             "but the item '%s' is a %s" % (name, attribute.listof, item, type(item)), obj=self.get_ds())
-                                elif attribute.required and attribute.listof == string_types:
-                                    if item is None or item.strip() == "":
-                                        raise AnsibleParserError("the field '%s' is required, and cannot have empty values" % (name,), obj=self.get_ds())
-                    elif attribute.isa == 'set':
-                        if value is None:
-                            value = set()
-                        elif not isinstance(value, (list, set)):
-                            if isinstance(value, string_types):
-                                value = value.split(',')
-                            else:
-                                # Making a list like this handles strings of
-                                # text and bytes properly
-                                value = [value]
-                        if not isinstance(value, set):
-                            value = set(value)
-                    elif attribute.isa == 'dict':
-                        if value is None:
-                            value = dict()
-                        elif not isinstance(value, dict):
-                            raise TypeError("%s is not a dictionary" % value)
-                    elif attribute.isa == 'class':
-                        if not isinstance(value, attribute.class_type):
-                            raise TypeError("%s is not a valid %s (got a %s instead)" % (name, attribute.class_type, type(value)))
-                        value.post_validate(templar=templar)
 
                 # and assign the massaged value back to the attribute field
                 setattr(self, name, value)

lib/ansible/playbook/handler.py

@@ -21,11 +21,12 @@ __metaclass__ = type
 
 from ansible.playbook.attribute import FieldAttribute
 from ansible.playbook.task import Task
+from ansible.module_utils.six import string_types
 
 
 class Handler(Task):
 
-    _listen = FieldAttribute(isa='list', default=list)
+    _listen = FieldAttribute(isa='list', default=list, listof=string_types, static=True)
 
     def __init__(self, block=None, role=None, task_include=None):
         self.notified_hosts = []

lib/ansible/plugins/strategy/__init__.py

@@ -350,6 +350,10 @@ class StrategyBase:
 
         return [actual_host]
 
+    def get_handler_templar(self, handler_task, iterator):
+        handler_vars = self._variable_manager.get_vars(play=iterator._play, task=handler_task)
+        return Templar(loader=self._loader, variables=handler_vars)
+
     @debug_closure
     def _process_pending_results(self, iterator, one_pass=False, max_passes=None):
         '''
@@ -373,8 +377,7 @@ class StrategyBase:
                 for handler_task in handler_block.block:
                     if handler_task.name:
                         if not handler_task.cached_name:
-                            handler_vars = self._variable_manager.get_vars(play=iterator._play, task=handler_task)
+                            templar = self.get_handler_templar(handler_task, iterator)
-                            templar = Templar(loader=self._loader, variables=handler_vars)
                             handler_task.name = templar.template(handler_task.name)
                             handler_task.cached_name = True
 
@@ -530,6 +533,10 @@ class StrategyBase:
                                 for listening_handler_block in iterator._play.handlers:
                                     for listening_handler in listening_handler_block.block:
                                         listeners = getattr(listening_handler, 'listen', []) or []
+                                        templar = self.get_handler_templar(listening_handler, iterator)
+                                        listeners = listening_handler.get_validated_value(
+                                            'listen', listening_handler._valid_attrs['listen'], listeners, templar
+                                        )
                                         if handler_name not in listeners:
                                             continue
                                         else:

test/integration/targets/handlers/roles/test_templating_in_handlers/handlers/main.yml

@@ -0,0 +1,15 @@
+---
+- name: name1
+  set_fact:
+    role_non_templated_name: True
+- name: "{{ handler2 }}"
+  set_fact:
+    role_templated_name: True
+- name: testlistener1
+  set_fact:
+    role_non_templated_listener: True
+  listen: name3
+- name: testlistener2
+  set_fact:
+    role_templated_listener: True
+  listen: "{{ handler4 }}"

test/integration/targets/handlers/roles/test_templating_in_handlers/tasks/main.yml

@@ -0,0 +1,16 @@
+---
+- command: echo Hello World
+  notify:
+    - "{{ handler1 }}"
+    - "{{ handler2 }}"
+    - "{{ handler3 }}"
+    - "{{ handler4 }}"
+
+- meta: flush_handlers
+
+- assert:
+    that:
+      - role_non_templated_name is defined
+      - role_templated_name is defined
+      - role_non_templated_listener is defined
+      - role_templated_listener is undefined

test/integration/targets/handlers/runme.sh

@@ -66,6 +66,8 @@ grep -q "ERROR! The requested handler 'notify_inexistent_handler' was not found
 # Notify inexistent handlers without errors when ANSIBLE_ERROR_ON_MISSING_HANDLER=false
 ANSIBLE_ERROR_ON_MISSING_HANDLER=false ansible-playbook test_handlers_inexistent_notify.yml -i inventory.handlers -v "$@"
 
+ANSIBLE_ERROR_ON_MISSING_HANDLER=false ansible-playbook test_templating_in_handlers.yml -v "$@"
+
 # https://github.com/ansible/ansible/issues/36649
 output_dir=/tmp
 set +e

test/integration/targets/handlers/test_handlers_listen.yml

@@ -13,6 +13,7 @@
         that:
           - "notify_listen_ran_1_1 is defined"
           - "notify_listen_ran_1_2 is defined"
+          - "notify_listen_ran_1_3 is undefined"
   handlers:
     - name: notify_handler_ran_1_1
       set_fact:
@@ -22,6 +23,10 @@
       set_fact:
         notify_listen_ran_1_2: True
       listen: notify_listen
+    - name: notify_handler_ran_1_3
+      set_fact:
+        notify_handler_ran_1_3: True
+      listen: notify_listen2
 
 - name: test listen unnamed handlers
   hosts: localhost
@@ -38,6 +43,7 @@
         that:
           - "notify_listen_ran_1 is defined"
           - "notify_listen_ran_2 is defined"
+          - "notify_listen_ran_3 is undefined"
   handlers:
     - set_fact:
         notify_listen_ran_1: True
@@ -45,6 +51,9 @@
     - set_fact:
         notify_listen_ran_2: True
       listen: notify_listen
+    - set_fact:
+        notify_handler_ran_3: True
+      listen: notify_listen2
 
 - name: test with mixed notify by name and listen
   hosts: localhost

test/integration/targets/handlers/test_templating_in_handlers.yml

@@ -0,0 +1,43 @@
+- name: test templated values in handlers
+  hosts: localhost
+  gather_facts: no
+  vars:
+    handler1: name1
+    handler2: name2
+    handler3: name3
+    handler4: name4
+
+  handlers:
+    - name: name1
+      set_fact:
+        non_templated_name: True
+    - name: "{{ handler2 }}"
+      set_fact:
+        templated_name: True
+    - name: testlistener1
+      set_fact:
+        non_templated_listener: True
+      listen: name3
+    - name: testlistener2
+      set_fact:
+        templated_listener: True
+      listen: "{{ handler4 }}"
+
+  tasks:
+    - command: echo Hello World
+      notify:
+        - "{{ handler1 }}"
+        - "{{ handler2 }}"
+        - "{{ handler3 }}"
+        - "{{ handler4 }}"
+
+    - meta: flush_handlers
+
+    - assert:
+        that:
+          - non_templated_name is defined
+          - templated_name is defined
+          - non_templated_listener is defined
+          - templated_listener is undefined
+
+    - include_role: name=test_templating_in_handlers