From ec1287ca7e2bda3c4f609c03c3824df5d6c089e2 Mon Sep 17 00:00:00 2001 From: Sloane Hertel Date: Thu, 27 Jun 2019 17:29:20 -0400 Subject: [PATCH] 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 * Add a test for handlers without names * Test templating in handlers * changelog * Add some tests for include_role * Add a couple notes about 'listen' for handlers * make more sense * move local function into a class method --- ...5_notify_handlers_by_exact_match_only.yaml | 3 + docs/docsite/keyword_desc.yml | 2 +- .../rst/user_guide/playbooks_intro.rst | 1 + lib/ansible/playbook/base.py | 100 +++++++++--------- lib/ansible/playbook/handler.py | 3 +- lib/ansible/plugins/strategy/__init__.py | 11 +- .../handlers/main.yml | 15 +++ .../tasks/main.yml | 16 +++ test/integration/targets/handlers/runme.sh | 2 + .../targets/handlers/test_handlers_listen.yml | 9 ++ .../handlers/test_templating_in_handlers.yml | 43 ++++++++ 11 files changed, 153 insertions(+), 52 deletions(-) create mode 100644 changelogs/fragments/55575_notify_handlers_by_exact_match_only.yaml create mode 100644 test/integration/targets/handlers/roles/test_templating_in_handlers/handlers/main.yml create mode 100644 test/integration/targets/handlers/roles/test_templating_in_handlers/tasks/main.yml create mode 100644 test/integration/targets/handlers/test_templating_in_handlers.yml diff --git a/changelogs/fragments/55575_notify_handlers_by_exact_match_only.yaml b/changelogs/fragments/55575_notify_handlers_by_exact_match_only.yaml new file mode 100644 index 00000000000..ed0015cbac2 --- /dev/null +++ b/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) diff --git a/docs/docsite/keyword_desc.yml b/docs/docsite/keyword_desc.yml index 4eefee55c6e..cc1076c99c3 100644 --- a/docs/docsite/keyword_desc.yml +++ b/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. diff --git a/docs/docsite/rst/user_guide/playbooks_intro.rst b/docs/docsite/rst/user_guide/playbooks_intro.rst index d5d33918dbc..38aad862758 100644 --- a/docs/docsite/rst/user_guide/playbooks_intro.rst +++ b/docs/docsite/rst/user_guide/playbooks_intro.rst @@ -455,6 +455,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`. diff --git a/lib/ansible/playbook/base.py b/lib/ansible/playbook/base.py index 74f079e41d1..3b0e2ac23dc 100644 --- a/lib/ansible/playbook/base.py +++ b/lib/ansible/playbook/base.py @@ -334,6 +334,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 @@ -389,54 +440,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 = 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) + value = self.get_validated_value(name, attribute, value, templar) # and assign the massaged value back to the attribute field setattr(self, name, value) diff --git a/lib/ansible/playbook/handler.py b/lib/ansible/playbook/handler.py index cabbd556ab1..79eaf3fe314 100644 --- a/lib/ansible/playbook/handler.py +++ b/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 = [] diff --git a/lib/ansible/plugins/strategy/__init__.py b/lib/ansible/plugins/strategy/__init__.py index 7141218b61d..e46e6b1e3bf 100644 --- a/lib/ansible/plugins/strategy/__init__.py +++ b/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 = Templar(loader=self._loader, variables=handler_vars) + templar = self.get_handler_templar(handler_task, iterator) 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: diff --git a/test/integration/targets/handlers/roles/test_templating_in_handlers/handlers/main.yml b/test/integration/targets/handlers/roles/test_templating_in_handlers/handlers/main.yml new file mode 100644 index 00000000000..cc5b3a3d945 --- /dev/null +++ b/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 }}" diff --git a/test/integration/targets/handlers/roles/test_templating_in_handlers/tasks/main.yml b/test/integration/targets/handlers/roles/test_templating_in_handlers/tasks/main.yml new file mode 100644 index 00000000000..47dbee3c73e --- /dev/null +++ b/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 diff --git a/test/integration/targets/handlers/runme.sh b/test/integration/targets/handlers/runme.sh index 1bddc079c9e..03de5136756 100755 --- a/test/integration/targets/handlers/runme.sh +++ b/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 diff --git a/test/integration/targets/handlers/test_handlers_listen.yml b/test/integration/targets/handlers/test_handlers_listen.yml index 5477968bedf..dd2cd87dd9a 100644 --- a/test/integration/targets/handlers/test_handlers_listen.yml +++ b/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 diff --git a/test/integration/targets/handlers/test_templating_in_handlers.yml b/test/integration/targets/handlers/test_templating_in_handlers.yml new file mode 100644 index 00000000000..b6aa71dfe6e --- /dev/null +++ b/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