Repository: ansible/ansible
Pull request: zabbix_host, interface.port parameter is a string
##### SUMMARY
interface.port should be an string to be able to use macros in that
value.
This fixes the case when interface.port is a macro (eg.: "{$MACRO}" and
force=false.

Because, until now, setting the interface.port to an integer was the
correct way to work with force=false, a type validation has been added
to that parameter.
Previously, if an string was used for interface.port, it was converted
to an integer, comparasion didn't work (if interface not in interfaces)
and the module tried to register the same interface twice, returning an
error.

Zabbix API manual specifies that only 'main, 'type', 'useip' and 'bulk'
are integers.
https://www.zabbix.com/documentation/current/manual/api/reference/hostinterface/object

Fixes #63449 and PR 63637
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
zabbix_host

##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here -->
<!--- A step-by-step reproduction of the problem is helpful if there is no related issue -->

Executing this playbook twice will show the error fixed in this PR:
```
- hosts: localhost
  gather_facts: false
  tasks:
    - name: "Register cluster virtual host n Zabbix"
      zabbix_host:
        force: "false"
        server_url: "http://zabbix"
        login_user: "Admin"
        login_password: "zabbix"
        host_name: "somehost"
        description: "Added by Ansible"
        host_groups: "somegroup"
        inventory_mode: "manual"
        interfaces:
          - type: 1
            main: 1
            useip: 0
            ip: 127.0.0.1
            dns: "somehost"
            port: "{$MACRO}"
```

Problem statement:
zabbix_host: cannot update host with port set to usermacro
##### SUMMARY
If a host has port set to a usermacro, the host is created successfully. On the next run, the update fails with `ValueError: invalid literal for int() with base 10: {$MACRO}`.

Tested with the latest `zabbix_host` from git.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
zabbix_host

##### ANSIBLE VERSION
latest

##### CONFIGURATION
Not relevant.

##### OS / ENVIRONMENT
Not relevant.

##### STEPS TO REPRODUCE
Have a host definition with usermacro used for port in an interface. Create the host via Ansible, then run the playbook again.

##### EXPECTED RESULTS
Success (reported as no change).

##### ACTUAL RESULTS

```paste below
Traceback (most recent call last):
  File \"/home/ansible/.ansible/tmp/ansible-tmp-1571042967.61-236562069605480/AnsiballZ_zabbix_host.py\", line 114, in <module>
    _ansiballz_main()
  File \"/home/ansible/.ansible/tmp/ansible-tmp-1571042967.61-236562069605480/AnsiballZ_zabbix_host.py\", line 106, in _ansiballz_main
    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
  File \"/home/ansible/.ansible/tmp/ansible-tmp-1571042967.61-236562069605480/AnsiballZ_zabbix_host.py\", line 49, in invoke_module
    imp.load_module(__main__, mod, module, MOD_DESC)
  File \"/tmp/ansible_zabbix_host_payload_7ml4r2/__main__.py\", line 857, in <module>
  File \"/tmp/ansible_zabbix_host_payload_7ml4r2/__main__.py\", line 817, in main
  File \"/tmp/ansible_zabbix_host_payload_7ml4r2/__main__.py\", line 479, in check_all_properties
  File \"/tmp/ansible_zabbix_host_payload_7ml4r2/__main__.py\", line 437, in check_interface_properties
ValueError: invalid literal for int() with base 10: {$MACRO}
```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/monitoring/zabbix/zabbix_host.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/monitoring/zabbix/zabbix_host.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @D3DeFi @cove @dj-wasabi @eikef @harrisongu @rubentsirunyan
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Hey @richlv ! Thanks for reporting this and other issues. I haven't had much time to do any work on zabbix modules these past weeks, but I will try to address them in the near future.

If there is someone that would like to open a PR I wouldn't object a bit :) 

This bug seems to be, as traceback says, in [check_interface_properties method](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/monitoring/zabbix/zabbix_host.py#L475). 
Hi @richlv, @D3DeFi

Please can you check this PR(https://github.com/ansible/ansible/pull/63637) works for you?
I'd like to wait for your review and feedback.
> 
> 
> Hi @richlv, @D3DeFi
> 
> Please can you check this PR(#63637) works for you?
> I'd like to wait for your review and feedback.

Thanks for the fix @sky-joker :)
Yay, thank you for the fix :)
This PR is uncomplete. With ``force=false`` it does not work.

Fixed in https://github.com/ansible/ansible/pull/64986

Commit message: zabbix_host, interface.port parameter is a string

interface.port should be an string to be able to use macros in that
value.
This fixes the case when interface.port is a macro (eg.: "{$MACRO}" and
force=false.

Because, until now, setting the interface.port to an integer was the
correct way to work with force=false, a type validation has been added
to that parameter.
Previously, if an string was used for interface.port, it was converted
to an integer, comparasion didn't work (if interface not in interfaces)
and the module tried to register the same interface twice, returning an
error.

Zabbix API manual specifies that only 'main, 'type', 'useip' and 'bulk'
are integers.
https://www.zabbix.com/documentation/current/manual/api/reference/hostinterface/object

Tests are changed to use always str in the interface.port.

Two new tests are added.
First one is to show that now registering a host with force=false and
a macro in interface.port works.
The other one tests that interfaces defined using string for port are
compared correctly when force=false is used. Previously it was
a comparasion between an int and a str, interfaces were seen as diferent
and an error was thrown because we were trying to create twice a main
interface.

Patch to review:
diff --git a/lib/ansible/modules/monitoring/zabbix/zabbix_host.py b/lib/ansible/modules/monitoring/zabbix/zabbix_host.py
index 0454591bb09abe..0e10122b07152c 100644
--- a/lib/ansible/modules/monitoring/zabbix/zabbix_host.py
+++ b/lib/ansible/modules/monitoring/zabbix/zabbix_host.py
@@ -252,13 +252,13 @@
         useip: 1
         ip: 10.xx.xx.xx
         dns: ""
-        port: 10050
+        port: "10050"
       - type: 4
         main: 1
         useip: 1
         ip: 10.xx.xx.xx
         dns: ""
-        port: 12345
+        port: "12345"
     proxy: a.zabbix.proxy
 - name: Update an existing host's TLS settings
   local_action:
@@ -780,6 +780,8 @@ def main():
                 interface['ip'] = ''
             if 'main' not in interface:
                 interface['main'] = 0
+            if 'port' in interface and not isinstance(interface['port'], str):
+                module.fail_json(msg="port should be an string on interface '%s'." % interface)
             if 'port' not in interface:
                 if interface['type'] == 1:
                     interface['port'] = "10050"
@@ -840,7 +842,7 @@ def main():
                             interface.pop(key, None)
 
                     for index in interface.keys():
-                        if index in ['useip', 'main', 'type', 'port']:
+                        if index in ['useip', 'main', 'type']:
                             interface[index] = int(interface[index])
 
                     if interface not in interfaces:
diff --git a/test/integration/targets/zabbix_host/tasks/zabbix_host_doc.yml b/test/integration/targets/zabbix_host/tasks/zabbix_host_doc.yml
index 41690947ca4644..dfdd2001c6126b 100644
--- a/test/integration/targets/zabbix_host/tasks/zabbix_host_doc.yml
+++ b/test/integration/targets/zabbix_host/tasks/zabbix_host_doc.yml
@@ -36,13 +36,13 @@
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 10050
+        port: "10050"
       - type: 4
         main: 1
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 12345
+        port: "12345"
   register: zabbix_host1
 
 - name: Update an existing host's tls settings
@@ -58,7 +58,7 @@
         useip: 1
         ip: 10.1.1.2
         dns: ""
-        port: 10050
+        port: "10050"
     host_groups:
       - Linux servers
     tls_psk_identity: test
diff --git a/test/integration/targets/zabbix_host/tasks/zabbix_host_tests.yml b/test/integration/targets/zabbix_host/tasks/zabbix_host_tests.yml
index af95251908227c..1f51677bea38a6 100644
--- a/test/integration/targets/zabbix_host/tasks/zabbix_host_tests.yml
+++ b/test/integration/targets/zabbix_host/tasks/zabbix_host_tests.yml
@@ -31,13 +31,19 @@
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 10050
+        port: "10050"
+      - type: 1
+        main: 0
+        useip: 1
+        ip: 10.1.1.1
+        dns: ""
+        port: "{$MACRO}"
       - type: 4
         main: 1
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 12345
+        port: "12345"
     proxy: ExampleProxy
     tls_psk_identity: test
     tls_connect: 2
@@ -80,13 +86,19 @@
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 10050
+        port: "10050"
+      - type: 1
+        main: 0
+        useip: 1
+        ip: 10.1.1.1
+        dns: ""
+        port: "{$MACRO}"
       - type: 4
         main: 1
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 12345
+        port: "12345"
     proxy: ExampleProxy
     tls_psk_identity: test
     tls_connect: 2
@@ -98,6 +110,118 @@
       that:
           - "not zabbix_host1 is changed"
 
+- name: "test: try to create the same host with the same settings and force false"
+  zabbix_host:
+    force: false
+    server_url: "{{ zabbix_server_url }}"
+    login_user: "{{ zabbix_login_user }}"
+    login_password: "{{ zabbix_login_password }}"
+    host_name: ExampleHost
+    visible_name: ExampleName
+    description: My ExampleHost Description
+    host_groups:
+      - Linux servers
+      - Zabbix servers
+    link_templates:
+      - Template App IMAP Service
+      - Template App NTP Service
+    status: enabled
+    state: present
+    inventory_mode: manual
+    inventory_zabbix:
+      tag: test-tag
+      alias: test-alias
+      notes: "Special Informations: test-info"
+      location: test-location
+      site_rack: test-rack
+      os: test-os
+      hardware: test-hw
+    interfaces:
+      - type: 1
+        main: 1
+        useip: 1
+        ip: 10.1.1.1
+        dns: ""
+        port: "10050"
+      - type: 1
+        main: 0
+        useip: 1
+        ip: 10.1.1.1
+        dns: ""
+        port: "{$MACRO}"
+      - type: 4
+        main: 1
+        useip: 1
+        ip: 10.1.1.1
+        dns: ""
+        port: "12345"
+    proxy: ExampleProxy
+    tls_psk_identity: test
+    tls_connect: 2
+    tls_psk: 123456789abcdef123456789abcdef12
+  register: zabbix_host1
+
+- name: updating with same values and force false should be idempotent
+  assert:
+      that:
+          - "not zabbix_host1 is changed"
+
+- name: "test: try to create the same host changing one parameter in the inventory with force false"
+  zabbix_host:
+    force: false
+    server_url: "{{ zabbix_server_url }}"
+    login_user: "{{ zabbix_login_user }}"
+    login_password: "{{ zabbix_login_password }}"
+    host_name: ExampleHost
+    visible_name: ExampleName
+    description: My ExampleHost Description
+    host_groups:
+      - Linux servers
+      - Zabbix servers
+    link_templates:
+      - Template App IMAP Service
+      - Template App NTP Service
+    status: enabled
+    state: present
+    inventory_mode: manual
+    inventory_zabbix:
+      tag: test-tag
+      alias: test-alias
+      notes: "Special Informations: test-info"
+      location: test-location
+      site_rack: test-rack
+      os: test-os
+      hardware: test-hw-modified
+    interfaces:
+      - type: 1
+        main: 1
+        useip: 1
+        ip: 10.1.1.1
+        dns: ""
+        port: "10050"
+      - type: 1
+        main: 0
+        useip: 1
+        ip: 10.1.1.1
+        dns: ""
+        port: "{$MACRO}"
+      - type: 4
+        main: 1
+        useip: 1
+        ip: 10.1.1.1
+        dns: ""
+        port: "12345"
+    proxy: ExampleProxy
+    tls_psk_identity: test
+    tls_connect: 2
+    tls_psk: 123456789abcdef123456789abcdef12
+  register: zabbix_host1
+
+- name: changing the value of an already defined inventory should work and mark task as changed
+  assert:
+      that:
+          - "zabbix_host1 is changed"
+
 - name: "test: change visible_name"
   zabbix_host:
     server_url: "{{ zabbix_server_url }}"
@@ -524,7 +648,7 @@
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 10050
+        port: "10050"
   register: zabbix_host1
 
 - name: expect to succeed and that things have changed
@@ -544,7 +668,7 @@
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 10050
+        port: "10050"
   register: zabbix_host1
 
 - name: expect to succeed and that things have not changed
@@ -564,7 +688,7 @@
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 12345
+        port: "12345"
     force: no
   register: zabbix_host1
 
@@ -585,13 +709,13 @@
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 10050
+        port: "10050"
       - type: 4
         main: 1
         useip: 1
         ip: 10.1.1.1
         dns: ""
-        port: 12345
+        port: "12345"
   register: zabbix_host1
 
 - name: expect to succeed and that things have not changed


Code review comments:
diff_hunk: @@ -780,6 +780,8 @@ def main():
                 interface['ip'] = ''
             if 'main' not in interface:
                 interface['main'] = 0
+            if 'port' in interface and not isinstance(interface['port'], str):
+                module.fail_json(msg="port should be an string on interface '%s'." % interface)
path: lib/ansible/modules/monitoring/zabbix/zabbix_host.py
text: @user1:
I wouldn't go as far as failing module because someone has not provided integer in quotes. This would require us to deprecate the feature first and then remove it in usually 4th release from now (that would be ansible 2.14)

What about this instead?

```suggestion
                try:
                    interface['port'] = str(interface['port'])
                except ValueError:
                    module.fail_json(msg="port should be convertable to string on interface '%s'." % interface)
```

@author:
Way better!