zabbix_host: fix various idempotency problems (#33138)

* reorder interfaces handling for force=no, making sure it works when no interfaces are specified in the module parameters
when no interfaces are specified on update, use existing interfaces obtained from API.
check whether visible_name is set in check_all_properties; if not set as module parameter, no comparison is necessary.
Check if description is set as module parameter before comparing as well

* link_templates need the same treatment

* add inventory update checks and simplify update procedure

* make specifying proxy optional on update (keeping it as is when not specified), as well

* pep8 fixes

* add tls_*-checks for updates and make tls_*-options actually optional

lib/ansible/modules/monitoring/zabbix/zabbix_host.py

@@ -274,13 +274,13 @@ class Host(object):
                 parameters['proxy_hostid'] = proxy_id
             if visible_name:
                 parameters['name'] = visible_name
-            if tls_psk_identity:
+            if tls_psk_identity is not None:
                 parameters['tls_psk_identity'] = tls_psk_identity
-            if tls_psk:
+            if tls_psk is not None:
                 parameters['tls_psk'] = tls_psk
-            if tls_issuer:
+            if tls_issuer is not None:
                 parameters['tls_issuer'] = tls_issuer
-            if tls_subject:
+            if tls_subject is not None:
                 parameters['tls_subject'] = tls_subject
             if description:
                 parameters['description'] = description
@@ -353,7 +353,7 @@ class Host(object):
 
     # get host by host name
     def get_host_by_host_name(self, host_name):
-        host_list = self._zapi.host.get({'output': 'extend', 'filter': {'host': [host_name]}})
+        host_list = self._zapi.host.get({'output': 'extend', 'selectInventory': 'extend', 'filter': {'host': [host_name]}})
         if len(host_list) < 1:
             self._module.fail_json(msg="Host not found: %s" % host_name)
         else:
@@ -429,7 +429,9 @@ class Host(object):
 
     # check all the properties before link or clear template
     def check_all_properties(self, host_id, host_groups, status, interfaces, template_ids,
-                             exist_interfaces, host, proxy_id, visible_name, description, host_name):
+                             exist_interfaces, host, proxy_id, visible_name, description, host_name,
+                             inventory_mode, inventory_zabbix, tls_accept, tls_psk_identity, tls_psk,
+                             tls_issuer, tls_subject, tls_connect):
         # get the existing host's groups
         exist_host_groups = self.get_host_groups_by_host_id(host_id)
         if set(host_groups) != set(exist_host_groups):
@@ -453,14 +455,51 @@ class Host(object):
             return True
 
         # Check whether the visible_name has changed; Zabbix defaults to the technical hostname if not set.
-        if host['name'] != visible_name and host['name'] != host_name:
+        if visible_name:
-            return True
+            if host['name'] != visible_name and host['name'] != host_name:
+                return True
 
-        # The Zabbbix API returns an empty description as an empty string
+        # Only compare description if it is given as a module parameter
-        if description is None:
+        if description:
-            description = ''
+            if host['description'] != description:
-        if host['description'] != description:
+                return True
-            return True
+
+        if inventory_mode:
+            if host['inventory']:
+                if int(host['inventory']['inventory_mode']) != self.inventory_mode_numeric(inventory_mode):
+                    return True
+            elif inventory_mode != 'disabled':
+                return True
+
+        if inventory_zabbix:
+            proposed_inventory = copy.deepcopy(host['inventory'])
+            proposed_inventory.update(inventory_zabbix)
+            if proposed_inventory != host['inventory']:
+                return True
+
+        if tls_accept is not None:
+            if int(host['tls_accept']) != tls_accept:
+                return True
+
+        if tls_psk_identity is not None:
+            if host['tls_psk_identity'] != tls_psk_identity:
+                return True
+
+        if tls_psk is not None:
+            if host['tls_psk'] != tls_psk:
+                return True
+
+        if tls_issuer is not None:
+            if host['tls_issuer'] != tls_issuer:
+                return True
+
+        if tls_subject is not None:
+            if host['tls_subject'] != tls_subject:
+                return True
+
+        if tls_connect is not None:
+            if int(host['tls_connect']) != tls_connect:
+                return True
 
         return False
 
@@ -479,13 +518,13 @@ class Host(object):
         templates_clear_list = list(templates_clear)
         request_str = {'hostid': host_id, 'templates': template_id_list, 'templates_clear': templates_clear_list,
                        'tls_connect': tls_connect, 'tls_accept': tls_accept}
-        if tls_psk_identity:
+        if tls_psk_identity is not None:
             request_str['tls_psk_identity'] = tls_psk_identity
-        if tls_psk:
+        if tls_psk is not None:
             request_str['tls_psk'] = tls_psk
-        if tls_issuer:
+        if tls_issuer is not None:
             request_str['tls_issuer'] = tls_issuer
-        if tls_subject:
+        if tls_subject is not None:
             request_str['tls_subject'] = tls_subject
         try:
             if self._module.check_mode:
@@ -494,6 +533,15 @@ class Host(object):
         except Exception as e:
             self._module.fail_json(msg="Failed to link template to host: %s" % e)
 
+    def inventory_mode_numeric(self, inventory_mode):
+        if inventory_mode == "automatic":
+            return int(1)
+        elif inventory_mode == "manual":
+            return int(0)
+        elif inventory_mode == "disabled":
+            return int(-1)
+        return inventory_mode
+
     # Update the host inventory_mode
     def update_inventory_mode(self, host_id, inventory_mode):
 
@@ -501,12 +549,7 @@ class Host(object):
         if not inventory_mode:
             return
 
-        if inventory_mode == "automatic":
+        inventory_mode = self.inventory_mode_numeric(inventory_mode)
-            inventory_mode = int(1)
-        elif inventory_mode == "manual":
-            inventory_mode = int(0)
-        elif inventory_mode == "disabled":
-            inventory_mode = int(-1)
 
         # watch for - https://support.zabbix.com/browse/ZBX-6033
         request_str = {'hostid': host_id, 'inventory_mode': inventory_mode}
@@ -621,20 +664,24 @@ def main():
             if interface['type'] == 1:
                 ip = interface['ip']
 
+    # Use proxy specified, or set to 0
+    if proxy:
+        proxy_id = host.get_proxyid_by_proxy_name(proxy)
+    else:
+        proxy_id = 0
+
     # check if host exist
     is_host_exist = host.is_host_exist(host_name)
 
     if is_host_exist:
-        # Use proxy specified, or set to None when updating host
-        if proxy:
-            proxy_id = host.get_proxyid_by_proxy_name(proxy)
-        else:
-            proxy_id = 0
-
         # get host id by host name
         zabbix_host_obj = host.get_host_by_host_name(host_name)
         host_id = zabbix_host_obj['hostid']
 
+        # If proxy is not specified as a module parameter, use the existing setting
+        if proxy is None:
+            proxy_id = zabbix_host_obj['proxy_hostid']
+
         if state == "absent":
             # remove host
             host.delete_host(host_id, host_name)
@@ -646,14 +693,18 @@ def main():
                 host_groups = host.get_host_groups_by_host_id(host_id)
                 group_ids = host.get_group_ids_by_group_names(host_groups)
 
-            if not force:
+            # get existing host's interfaces
-                # get existing groups, interfaces and templates and merge them with ones provided as an argument
+            exist_interfaces = host._zapi.hostinterface.get({'output': 'extend', 'hostids': host_id})
-                # we do not want to overwrite anything if force: no is explicitly used, we just want to add new ones
-                for group_id in host.get_group_ids_by_group_names(host.get_host_groups_by_host_id(host_id)):
-                    if group_id not in group_ids:
-                        group_ids.append(group_id)
 
-                for interface in host._zapi.hostinterface.get({'output': 'extend', 'hostids': host_id}):
+            # if no interfaces were specified with the module, start with an empty list
+            if not interfaces:
+                interfaces = []
+
+            # When force=no is specified, append existing interfaces to interfaces to update. When
+            # no interfaces have been specified, copy existing interfaces as specified from the API.
+            # Do the same with templates and host groups.
+            if not force or not interfaces:
+                for interface in copy.deepcopy(exist_interfaces):
                     # remove values not used during hostinterface.add/update calls
                     for key in interface.keys():
                         if key in ['interfaceid', 'hostid', 'bulk']:
@@ -666,54 +717,38 @@ def main():
                     if interface not in interfaces:
                         interfaces.append(interface)
 
+            if not force or link_templates is None:
                 template_ids = list(set(template_ids + host.get_host_templates_by_host_id(host_id)))
 
-            # get exist host's interfaces
+            if not force:
-            exist_interfaces = host._zapi.hostinterface.get({'output': 'extend', 'hostids': host_id})
+                for group_id in host.get_group_ids_by_group_names(host.get_host_groups_by_host_id(host_id)):
-            exist_interfaces_copy = copy.deepcopy(exist_interfaces)
+                    if group_id not in group_ids:
+                        group_ids.append(group_id)
 
             # update host
-            interfaces_len = len(interfaces) if interfaces else 0
+            if host.check_all_properties(host_id, host_groups, status, interfaces, template_ids,
+                                         exist_interfaces, zabbix_host_obj, proxy_id, visible_name,
+                                         description, host_name, inventory_mode, inventory_zabbix,
+                                         tls_accept, tls_psk_identity, tls_psk, tls_issuer, tls_subject, tls_connect):
+                host.link_or_clear_template(host_id, template_ids, tls_connect, tls_accept, tls_psk_identity,
+                                            tls_psk, tls_issuer, tls_subject)
+                host.update_host(host_name, group_ids, status, host_id,
+                                 interfaces, exist_interfaces, proxy_id, visible_name, description, tls_connect, tls_accept,
+                                 tls_psk_identity, tls_psk, tls_issuer, tls_subject)
+                host.update_inventory_mode(host_id, inventory_mode)
+                host.update_inventory_zabbix(host_id, inventory_zabbix)
 
-            if len(exist_interfaces) > interfaces_len:
+                module.exit_json(changed=True,
-                if host.check_all_properties(host_id, host_groups, status, interfaces, template_ids,
+                                 result="Successfully update host %s (%s) and linked with template '%s'"
-                                             exist_interfaces, zabbix_host_obj, proxy_id, visible_name, description, host_name):
+                                        % (host_name, ip, link_templates))
-                    host.link_or_clear_template(host_id, template_ids, tls_connect, tls_accept, tls_psk_identity,
-                                                tls_psk, tls_issuer, tls_subject)
-                    host.update_host(host_name, group_ids, status, host_id,
-                                     interfaces, exist_interfaces, proxy_id, visible_name, description, tls_connect, tls_accept,
-                                     tls_psk_identity, tls_psk, tls_issuer, tls_subject)
-                    module.exit_json(changed=True,
-                                     result="Successfully update host %s (%s) and linked with template '%s'"
-                                            % (host_name, ip, link_templates))
-                else:
-                    module.exit_json(changed=False)
             else:
-                if host.check_all_properties(host_id, host_groups, status, interfaces, template_ids,
+                module.exit_json(changed=False)
-                                             exist_interfaces_copy, zabbix_host_obj, proxy_id, visible_name, description, host_name):
+
-                    host.update_host(host_name, group_ids, status, host_id, interfaces, exist_interfaces, proxy_id,
-                                     visible_name, description, tls_connect, tls_accept, tls_psk_identity, tls_psk, tls_issuer,
-                                     tls_subject)
-                    host.link_or_clear_template(host_id, template_ids, tls_connect, tls_accept, tls_psk_identity,
-                                                tls_psk, tls_issuer, tls_subject)
-                    host.update_inventory_mode(host_id, inventory_mode)
-                    host.update_inventory_zabbix(host_id, inventory_zabbix)
-                    module.exit_json(changed=True,
-                                     result="Successfully update host %s (%s) and linked with template '%s'"
-                                            % (host_name, ip, link_templates))
-                else:
-                    module.exit_json(changed=False)
     else:
         if state == "absent":
             # the host is already deleted.
             module.exit_json(changed=False)
 
-        # Use proxy specified, or set to 0 when adding new host
-        if proxy:
-            proxy_id = host.get_proxyid_by_proxy_name(proxy)
-        else:
-            proxy_id = 0
-
         if not group_ids:
             module.fail_json(msg="Specify at least one group for creating host '%s'." % host_name)