Repository: ansible/ansible
Pull request: Fix for nxos_snmp_host issues
##### SUMMARY
<!--- Describe the change, including rationale and design decisions -->
fixes #39641 
<!--- If you are fixing an existing issue, please include "Fixes #nnn" in your
commit message and your description; but you should still explain what
the change does.-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Bugfix Pull Request

##### COMPONENT NAME
<!--- Name of the module, plugin, module or task -->
nxos_snmp_host
##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
```
ansible 2.6.0 (devel fed20b825f) last updated 2018/02/15 12:51:12 (GMT -400)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /root/agents-ci/ansible/lib/ansible
  executable location = /root/agents-ci/ansible/bin/ansible
  python version = 2.7.6 (default, Oct 26 2016, 20:30:19) [GCC 4.8.4]
```


##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here.
For bugs that don't have a linked bug report, a step-by-step reproduction
of the problem is helpful. -->
* This PR fixes #39641 
* Integration tests are enhanced for various platforms and version 1,2 and 3.
* All tests pass on all platforms
<!--- Paste verbatim command output below, e.g. before and after your change -->
```

```


Problem statement:
nxos_snmp_host issues
<!---
Verify first that your issue/request is not already reported on GitHub.
THIS FORM WILL BE READ BY A MACHINE, COMPLETE ALL SECTIONS AS DESCRIBED.
Also test if the latest release, and devel branch are affected too.
ALWAYS add information AFTER (OUTSIDE) these html comments.
Otherwise it may end up being automatically closed by our bot. -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
 - Bug Report

##### COMPONENT NAME
<!--- Insert, BELOW THIS COMMENT, the name of the module, plugin, task or feature.
Do not include extra details here, e.g. "vyos_command" not "the network module vyos_command" or the full path-->
nxos_snmp_host

##### ANSIBLE VERSION
<!--- Paste, BELOW THIS COMMENT, verbatim output from "ansible --version" between quotes below -->
```
ansible 2.6.0 (devel fed20b825f) last updated 2018/02/15 12:51:12 (GMT -400)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /root/agents-ci/ansible/lib/ansible
  executable location = /root/agents-ci/ansible/bin/ansible
  python version = 2.7.6 (default, Oct 26 2016, 20:30:19) [GCC 4.8.4]
```
##### OS / ENVIRONMENT
<!--- Mention, BELOW THIS COMMENT, the OS you are running Ansible from, and the OS you are
managing, or say "N/A" for anything that is not platform-specific.
Also mention the specific version of what you are trying to control,
e.g. if this is a network bug the version of firmware on the network device.-->
Ansible Server : Ubuntu 14.04
Device: N9K running 7.0(3)I2(5)
##### SUMMARY
<!--- Explain the problem briefly -->

##### STEPS TO REPRODUCE
<!--- For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used. -->
There are some MAJOR issues with nxos_snmp_host module.

1. udp option does not work at all.
2. We can have multiple hosts with the same ip address but with different udp ports. However, the code only considers host ip as the key and so if we have multiple hosts present with different udp ports, the entire module breaks down as it picks up one of the existing hosts which is wrong.
3. The default for snmp version is 1 but it is assumed to be v2c and there is no mention of version 1 anywhere.
4. when the type is 'inform', the code fails if the version is v2c. This is wrong as v2c and v3 both accept inform type.
5. There is no way to remove vrf, vrf_filter and src_intf on their own without removing the entire config for that host. 

Note: I will open a PR shortly to address the above issues

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/network/nxos/nxos_snmp_host.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/nxos/nxos_snmp_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 @GGabriele @jedelman8 @mikewiebe @privateip @rahushen @rcarrillocruz @trishnaguha
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: fix shippable

Patch to review:
diff --git a/lib/ansible/modules/network/nxos/nxos_snmp_host.py b/lib/ansible/modules/network/nxos/nxos_snmp_host.py
index a87b52bb4210b1..652a498a4825da 100644
--- a/lib/ansible/modules/network/nxos/nxos_snmp_host.py
+++ b/lib/ansible/modules/network/nxos/nxos_snmp_host.py
@@ -43,8 +43,8 @@
     version:
         description:
             - SNMP version.
-        default: v2c
-        choices: ['v2c', 'v3']
+        default: v1
+        choices: ['v1', 'v2c', 'v3']
     v3:
         description:
             - Use this when verion is v3. SNMPv3 Security level.
@@ -55,6 +55,7 @@
     udp:
         description:
             - UDP port number (0-65535).
+        default: 162
     snmp_type:
         description:
             - type of message to send to host.
@@ -63,15 +64,24 @@
     vrf:
         description:
             - VRF to use to source traffic to source.
+              If state = absent, the vrf is removed.
     vrf_filter:
         description:
             - Name of VRF to filter.
+              If state = absent, the vrf is removed from the filter.
     src_intf:
         description:
-            - Source interface.
+            - Source interface. Must be fully qualified interface name.
+              If state = absent, the interface is removed.
     state:
         description:
-            - Manage the state of the resource.
+            - Manage the state of the resource. If state = present, the
+              host is added to the configuration. If only vrf and/or
+              vrf_filter and/or src_intf are given, they will be added to
+              the existing host configuration. If state = absent, the
+              host is removed if community parameter is given. It is possible
+              to remove only vrf and/or src_int and/or vrf_filter
+              by providing only those parameters and no community parameter.
         default: present
         choices: ['present','absent']
 '''
@@ -131,7 +141,7 @@ def flatten_list(command_lists):
     return flat_command_list
 
 
-def get_snmp_host(host, module):
+def get_snmp_host(host, udp, module):
     body = execute_show_command('show snmp host', module)
 
     host_map = {
@@ -160,7 +170,7 @@ def get_snmp_host(host, module):
                 resource_table = [resource_table]
 
             for each in resource_table:
-                key = str(each['host'])
+                key = str(each['host']) + '_' + str(each['port']).strip()
                 src = each.get('src_intf')
                 host_resource = apply_key_map(host_map, each)
 
@@ -188,7 +198,7 @@ def get_snmp_host(host, module):
                     resource_table = [resource_table]
 
                 for each in resource_table:
-                    key = str(each['address'])
+                    key = str(each['address']) + '_' + str(each['port']).strip()
                     src = each.get('src_intf')
                     host_resource = apply_key_map(host_map_5k, each)
 
@@ -197,22 +207,23 @@ def get_snmp_host(host, module):
                         if re.search(r'interface:', src):
                             host_resource['src_intf'] = src.split(':')[1].strip()
 
+                    vrf = each.get('use_vrf_name')
+                    if vrf:
+                        host_resource['vrf'] = vrf.strip()
+
                     vrf_filt = each.get('TABLE_filter_vrf')
                     if vrf_filt:
                         vrf_filter = vrf_filt['ROW_filter_vrf']['filter_vrf_name'].split(',')
                         filters = [vrf.strip() for vrf in vrf_filter]
                         host_resource['vrf_filter'] = filters
 
-                    vrf = each.get('use_vrf_name')
-                    if vrf:
-                        host_resource['vrf'] = vrf.strip()
                     resource[key] = host_resource
             except (KeyError, AttributeError, TypeError):
                 return resource
         except (AttributeError, TypeError):
             return resource
 
-        find = resource.get(host)
+        find = resource.get(host + '_' + udp)
 
         if find:
             fix_find = {}
@@ -226,24 +237,54 @@ def get_snmp_host(host, module):
     return {}
 
 
-def remove_snmp_host(host, existing):
+def remove_snmp_host(host, udp, existing):
     commands = []
     if existing['version'] == 'v3':
         existing['version'] = '3'
         command = 'no snmp-server host {0} {snmp_type} version \
-                    {version} {v3} {community}'.format(host, **existing)
+                    {version} {v3} {community} udp-port {1}'.format(host, udp, **existing)
 
     elif existing['version'] == 'v2c':
         existing['version'] = '2c'
         command = 'no snmp-server host {0} {snmp_type} version \
-                    {version} {community}'.format(host, **existing)
+                    {version} {community} udp-port {1}'.format(host, udp, **existing)
+
+    elif existing['version'] == 'v1':
+        existing['version'] = '1'
+        command = 'no snmp-server host {0} {snmp_type} version \
+                    {version} {community} udp-port {1}'.format(host, udp, **existing)
 
     if command:
         commands.append(command)
     return commands
 
 
-def config_snmp_host(delta, proposed, existing, module):
+def remove_vrf(host, udp, proposed, existing):
+    commands = []
+    if existing.get('vrf'):
+        commands.append('no snmp-server host {0} use-vrf \
+                    {1} udp-port {2}'.format(host, proposed.get('vrf'), udp))
+    return commands
+
+
+def remove_filter(host, udp, proposed, existing):
+    commands = []
+    if existing.get('vrf_filter'):
+        if proposed.get('vrf_filter') in existing.get('vrf_filter'):
+            commands.append('no snmp-server host {0} filter-vrf \
+                    {1} udp-port {2}'.format(host, proposed.get('vrf_filter'), udp))
+    return commands
+
+
+def remove_src(host, udp, proposed, existing):
+    commands = []
+    if existing.get('src_intf'):
+        commands.append('no snmp-server host {0} source-interface \
+                    {1} udp-port {2}'.format(host, proposed.get('src_intf'), udp))
+    return commands
+
+
+def config_snmp_host(delta, udp, proposed, existing, module):
     commands = []
     command_builder = []
     host = proposed['snmp_host']
@@ -262,7 +303,9 @@ def config_snmp_host(delta, proposed, existing, module):
 
         version = version or existing.get('version')
         if version:
-            if version == 'v2c':
+            if version == 'v1':
+                vn = '1'
+            elif version == 'v2c':
                 vn = '2c'
             elif version == 'v3':
                 vn = '3'
@@ -278,21 +321,23 @@ def config_snmp_host(delta, proposed, existing, module):
             community_string = community or existing.get('community')
             command_builder.append(community_string)
 
+        udp_string = ' udp-port {0}'.format(udp)
+        command_builder.append(udp_string)
+
         cmd = ' '.join(command_builder)
 
         commands.append(cmd)
 
     CMDS = {
-        'vrf_filter': 'snmp-server host {0} filter-vrf {vrf_filter}',
-        'vrf': 'snmp-server host {0} use-vrf {vrf}',
-        'udp': 'snmp-server host {0} udp-port {udp}',
-        'src_intf': 'snmp-server host {0} source-interface {src_intf}'
+        'vrf_filter': 'snmp-server host {0} filter-vrf {vrf_filter} udp-port {1}',
+        'vrf': 'snmp-server host {0} use-vrf {vrf} udp-port {1}',
+        'src_intf': 'snmp-server host {0} source-interface {src_intf} udp-port {1}'
     }
 
     for key in delta:
         command = CMDS.get(key)
         if command:
-            cmd = command.format(host, **delta)
+            cmd = command.format(host, udp, **delta)
             commands.append(cmd)
     return commands
 
@@ -301,13 +346,13 @@ def main():
     argument_spec = dict(
         snmp_host=dict(required=True, type='str'),
         community=dict(type='str'),
-        udp=dict(type='str'),
-        version=dict(choices=['v2c', 'v3'], default='v2c'),
+        udp=dict(type='str', default='162'),
+        version=dict(choices=['v1', 'v2c', 'v3']),
         src_intf=dict(type='str'),
         v3=dict(choices=['noauth', 'auth', 'priv']),
         vrf_filter=dict(type='str'),
         vrf=dict(type='str'),
-        snmp_type=dict(choices=['trap', 'inform'], default='trap'),
+        snmp_type=dict(choices=['trap', 'inform']),
         state=dict(choices=['absent', 'present'], default='present'),
     )
 
@@ -330,17 +375,35 @@ def main():
     snmp_type = module.params['snmp_type']
     state = module.params['state']
 
-    if snmp_type == 'inform' and version != 'v3':
-        module.fail_json(msg='inform requires snmp v3')
+    existing = get_snmp_host(snmp_host, udp, module)
+
+    if version is None:
+        if existing:
+            version = existing.get('version')
+        else:
+            version = 'v1'
+
+    if snmp_type is None:
+        if existing:
+            snmp_type = existing.get('snmp_type')
+        else:
+            snmp_type = 'trap'
 
-    if version == 'v2c' and v3:
+    if v3 is None:
+        if version == 'v3' and existing:
+            v3 = existing.get('v3')
+
+    if snmp_type == 'inform' and version == 'v1':
+        module.fail_json(msg='inform requires snmp v2c or v3')
+
+    if (version == 'v1' or version == 'v2c') and v3:
         module.fail_json(msg='param: "v3" should not be used when '
-                             'using version v2c')
+                             'using version v1 or v2c')
 
-    if not any([vrf_filter, vrf, udp, src_intf]):
-        if not all([snmp_type, version, community]):
+    if not any([vrf_filter, vrf, src_intf]):
+        if not all([snmp_type, version, community, udp]):
             module.fail_json(msg='when not configuring options like '
-                                 'vrf_filter, vrf, udp, and src_intf,'
+                                 'vrf_filter, vrf, and src_intf,'
                                  'the following params are required: '
                                  'type, version, community')
 
@@ -348,8 +411,6 @@ def main():
         module.fail_json(msg='when using version=v3, the param v3 '
                              '(options: auth, noauth, priv) is also required')
 
-    existing = get_snmp_host(snmp_host, module)
-
     # existing returns the list of vrfs configured for a given host
     # checking to see if the proposed is in the list
     store = existing.get('vrf_filter')
@@ -360,27 +421,34 @@ def main():
             existing['vrf_filter'] = vrf_filter
     commands = []
 
+    args = dict(
+        community=community,
+        snmp_host=snmp_host,
+        udp=udp,
+        version=version,
+        src_intf=src_intf,
+        vrf_filter=vrf_filter,
+        v3=v3,
+        vrf=vrf,
+        snmp_type=snmp_type
+    )
+    proposed = dict((k, v) for k, v in args.items() if v is not None)
+
     if state == 'absent' and existing:
-        command = remove_snmp_host(snmp_host, existing)
-        commands.append(command)
+        if proposed.get('community'):
+            commands.append(remove_snmp_host(snmp_host, udp, existing))
+        else:
+            if proposed.get('src_intf'):
+                commands.append(remove_src(snmp_host, udp, proposed, existing))
+            if proposed.get('vrf'):
+                commands.append(remove_vrf(snmp_host, udp, proposed, existing))
+            if proposed.get('vrf_filter'):
+                commands.append(remove_filter(snmp_host, udp, proposed, existing))
 
     elif state == 'present':
-        args = dict(
-            community=community,
-            snmp_host=snmp_host,
-            udp=udp,
-            version=version,
-            src_intf=src_intf,
-            vrf_filter=vrf_filter,
-            v3=v3,
-            vrf=vrf,
-            snmp_type=snmp_type
-        )
-        proposed = dict((k, v) for k, v in args.items() if v is not None)
-
         delta = dict(set(proposed.items()).difference(existing.items()))
         if delta:
-            command = config_snmp_host(delta, proposed, existing, module)
+            command = config_snmp_host(delta, udp, proposed, existing, module)
             commands.append(command)
 
     cmds = flatten_list(commands)
diff --git a/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v1_trap.yaml b/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v1_trap.yaml
new file mode 100644
index 00000000000000..e7de1ef55fcc70
--- /dev/null
+++ b/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v1_trap.yaml
@@ -0,0 +1,129 @@
+---
+- set_fact: snmp_type="trap"
+- set_fact: snmp_version="v1"
+
+- debug: msg="START connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }} sanity test"
+- debug: msg="Using provider={{ connection.transport }}"
+  when: ansible_connection == "local"
+
+# Select interface for test
+- set_fact: intname="{{ nxos_int1 }}"
+  when: not (platform is match("N5K"))
+
+- name: Setup - Remove snmp_host if configured
+  nxos_snmp_host: &remove
+    snmp_host: 3.3.3.3
+    community: TESTING
+    version: "{{ snmp_version }}" 
+    snmp_type: "{{ snmp_type }}" 
+    vrf: management
+    vrf_filter: management
+    src_intf: "{{ intname|default(omit) }}"
+    udp: 222
+    state: absent 
+    provider: "{{ connection }}"
+  ignore_errors: yes
+
+- block:
+
+  - name: Configure snmp host 
+    nxos_snmp_host: &config
+      snmp_host: 3.3.3.3
+      community: TESTING
+      version: "{{ snmp_version }}" 
+      snmp_type: "{{ snmp_type }}" 
+      vrf: management
+      vrf_filter: management
+      src_intf: "{{ intname|default(omit) }}"
+      udp: 222
+      state: present 
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: &true
+      that:
+        - "result.changed == true"
+
+  - name: Idempotence Check
+    nxos_snmp_host: *config
+    register: result
+
+  - assert: &false
+      that:
+        - "result.changed == false"
+
+  - block:
+    - name: Add another vrf to filter
+      nxos_snmp_host: &config1
+        snmp_host: 3.3.3.3
+        vrf_filter: default
+        udp: 222
+        state: present 
+        provider: "{{ connection }}"
+      register: result
+
+    - assert: *true
+
+    - name: Idempotence Check
+      nxos_snmp_host: *config1
+      register: result
+
+    - assert: *false
+    when: not (platform is match('N35|N5K'))
+
+  - name: remove some configuration
+    nxos_snmp_host: &rem1
+      snmp_host: 3.3.3.3
+      udp: 222
+      src_intf: "{{ intname|default(omit) }}"
+      vrf: management
+      vrf_filter: management
+      state: absent
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: Idempotence Check
+    nxos_snmp_host: *rem1
+    register: result
+
+  - assert: *false
+
+  - block:
+    - name: remove some more configuration
+      nxos_snmp_host: &rem2
+        snmp_host: 3.3.3.3
+        udp: 222
+        vrf_filter: default
+        state: absent
+        provider: "{{ connection }}"
+      register: result
+
+    - assert: *true
+
+    - name: Idempotence Check
+      nxos_snmp_host: *rem2
+      register: result
+
+    - assert: *false
+    when: not (platform is match('N35|N5K'))
+
+
+  - name: Cleanup
+    nxos_snmp_host: *remove
+    register: result
+
+  - assert: *true
+
+  - name: Cleanup Idempotence
+    nxos_snmp_host: *remove
+    register: result
+
+  - assert: *false
+
+  always:
+  - name: Cleanup
+    nxos_snmp_host: *remove
+
+  - debug: msg="END connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }} sanity test"
diff --git a/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v2_trap.yaml b/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v2_inform.yaml
similarity index 51%
rename from test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v2_trap.yaml
rename to test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v2_inform.yaml
index 16c1843574dc3a..7c7e100077b4e6 100644
--- a/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v2_trap.yaml
+++ b/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v2_inform.yaml
@@ -1,8 +1,8 @@
 ---
-- set_fact: snmp_type="trap"
+- set_fact: snmp_type="inform"
 - set_fact: snmp_version="v2c"
 
-- debug: msg="START connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }}sanity test"
+- debug: msg="START connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }} sanity test"
 - debug: msg="Using provider={{ connection.transport }}"
   when: ansible_connection == "local"
 
@@ -19,6 +19,7 @@
     vrf: management
     vrf_filter: management
     src_intf: "{{ intname|default(omit) }}"
+    udp: 222
     state: absent 
     provider: "{{ connection }}"
   ignore_errors: yes
@@ -34,6 +35,7 @@
       vrf: management
       vrf_filter: management
       src_intf: "{{ intname|default(omit) }}"
+      udp: 222
       state: present 
       provider: "{{ connection }}"
     register: result
@@ -50,7 +52,63 @@
       that:
         - "result.changed == false"
 
-  always:
+  - block:
+    - name: Add another vrf to filter
+      nxos_snmp_host: &config1
+        snmp_host: 3.3.3.3
+        vrf_filter: default
+        udp: 222
+        state: present 
+        provider: "{{ connection }}"
+      register: result
+
+    - assert: *true
+
+    - name: Idempotence Check
+      nxos_snmp_host: *config1
+      register: result
+
+    - assert: *false
+    when: not (platform is match('N35|N5K'))
+
+  - name: remove some configuration
+    nxos_snmp_host: &rem1
+      snmp_host: 3.3.3.3
+      udp: 222
+      src_intf: "{{ intname|default(omit) }}"
+      vrf: management
+      vrf_filter: management
+      state: absent
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: Idempotence Check
+    nxos_snmp_host: *rem1
+    register: result
+
+  - assert: *false
+
+  - block:
+    - name: remove some more configuration
+      nxos_snmp_host: &rem2
+        snmp_host: 3.3.3.3
+        udp: 222
+        vrf_filter: default
+        state: absent
+        provider: "{{ connection }}"
+      register: result
+
+    - assert: *true
+
+    - name: Idempotence Check
+      nxos_snmp_host: *rem2
+      register: result
+
+    - assert: *false
+    when: not (platform is match('N35|N5K'))
+
   - name: Cleanup
     nxos_snmp_host: *remove
     register: result
@@ -63,4 +121,8 @@
 
   - assert: *false
 
-  - debug: msg="END connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }}sanity test"
+  always:
+  - name: Cleanup
+    nxos_snmp_host: *remove
+
+  - debug: msg="END connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }} sanity test"
diff --git a/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v3_inform.yaml b/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v3_inform.yaml
index 84b41a2222c989..9a481e577019a2 100644
--- a/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v3_inform.yaml
+++ b/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v3_inform.yaml
@@ -3,7 +3,7 @@
 - set_fact: snmp_version="v3"
 - set_fact: snmp_auth="noauth"
 
-- debug: msg="START connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }}sanity test"
+- debug: msg="START connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }} sanity test"
 - debug: msg="Using provider={{ connection.transport }}"
   when: ansible_connection == "local"
 
@@ -11,13 +11,17 @@
 - set_fact: intname="{{ nxos_int1 }}"
   when: not (platform is match("N5K"))
 
+- set_fact: run="true"
+- set_fact: run="false"
+  when: platform is match('N35')
+
 - name: Setup - Remove snmp_host if configured
   nxos_snmp_host: &remove
     snmp_host: 3.3.3.3
     community: TESTING
-    v3: "{{ snmp_auth|default(omit) }}"
     version: "{{ snmp_version }}" 
     snmp_type: "{{ snmp_type }}" 
+    v3: "{{ snmp_auth }}" 
     vrf: management
     vrf_filter: management
     src_intf: "{{ intname|default(omit) }}"
@@ -53,9 +57,60 @@
       that:
         - "result.changed == false"
 
-  when: not (platform is match('N35'))
+  - block:
+    - name: Add another vrf to filter
+      nxos_snmp_host: &config1
+        snmp_host: 3.3.3.3
+        vrf_filter: default
+        state: present 
+        provider: "{{ connection }}"
+      register: result
+
+    - assert: *true
+
+    - name: Idempotence Check
+      nxos_snmp_host: *config1
+      register: result
+
+    - assert: *false
+    when: not (platform is match('N35|N5K'))
+
+  - name: remove some configuration
+    nxos_snmp_host: &rem1
+      snmp_host: 3.3.3.3
+      src_intf: "{{ intname|default(omit) }}"
+      vrf: management
+      vrf_filter: management
+      state: absent
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: Idempotence Check
+    nxos_snmp_host: *rem1
+    register: result
+
+  - assert: *false
+
+  - block:
+    - name: remove some more configuration
+      nxos_snmp_host: &rem2
+        snmp_host: 3.3.3.3
+        vrf_filter: default
+        state: absent
+        provider: "{{ connection }}"
+      register: result
+
+    - assert: *true
+
+    - name: Idempotence Check
+      nxos_snmp_host: *rem2
+      register: result
+
+    - assert: *false
+    when: not (platform is match('N35|N5K'))
 
-  always:
   - name: Cleanup
     nxos_snmp_host: *remove
     register: result
@@ -68,4 +123,11 @@
 
   - assert: *false
 
-  - debug: msg="END connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }}sanity test"
+  when: run
+
+  always:
+  - name: Cleanup
+    nxos_snmp_host: *remove
+    register: result
+
+  - debug: msg="END connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }} sanity test"
diff --git a/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v3_trap.yaml b/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v3_trap.yaml
index 0d92d3b94d6d1b..2262a6637f0ea2 100644
--- a/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v3_trap.yaml
+++ b/test/integration/targets/nxos_snmp_host/tests/common/sanity_snmp_v3_trap.yaml
@@ -1,21 +1,27 @@
 ---
 - set_fact: snmp_type="trap"
 - set_fact: snmp_version="v3"
-- set_fact: snmp_auth="noauth"
+- set_fact: snmp_auth="priv"
 
-- debug: msg="START connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }}sanity test"
+- debug: msg="START connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }} sanity test"
 - debug: msg="Using provider={{ connection.transport }}"
   when: ansible_connection == "local"
 
+# Select interface for test
+- set_fact: intname="{{ nxos_int1 }}"
+  when: not (platform is match("N5K"))
+
 - name: Setup - Remove snmp_host if configured
   nxos_snmp_host: &remove
     snmp_host: 3.3.3.3
     community: TESTING
+    udp: 222
     v3: "{{ snmp_auth|default(omit) }}"
     version: "{{ snmp_version }}" 
     snmp_type: "{{ snmp_type }}" 
     vrf: management
     vrf_filter: management
+    src_intf: "{{ intname|default(omit) }}"
     state: absent 
     provider: "{{ connection }}"
   ignore_errors: yes
@@ -26,11 +32,13 @@
     nxos_snmp_host: &config
       snmp_host: 3.3.3.3
       community: TESTING
+      udp: 222
       v3: "{{ snmp_auth|default(omit) }}"
       version: "{{ snmp_version }}" 
       snmp_type: "{{ snmp_type }}" 
       vrf: management
       vrf_filter: management
+      src_intf: "{{ intname|default(omit) }}"
       state: present 
       provider: "{{ connection }}"
     register: result
@@ -47,7 +55,63 @@
       that:
         - "result.changed == false"
 
-  always:
+  - block:
+    - name: Add another vrf to filter
+      nxos_snmp_host: &config1
+        snmp_host: 3.3.3.3
+        udp: 222
+        vrf_filter: default
+        state: present 
+        provider: "{{ connection }}"
+      register: result
+
+    - assert: *true
+
+    - name: Idempotence Check
+      nxos_snmp_host: *config1
+      register: result
+
+    - assert: *false
+    when: not (platform is match('N35|N5K'))
+
+  - name: remove some configuration
+    nxos_snmp_host: &rem1
+      snmp_host: 3.3.3.3
+      udp: 222
+      src_intf: "{{ intname|default(omit) }}"
+      vrf: management
+      vrf_filter: management
+      state: absent
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: Idempotence Check
+    nxos_snmp_host: *rem1
+    register: result
+
+  - assert: *false
+
+  - block:
+    - name: remove some more configuration
+      nxos_snmp_host: &rem2
+        snmp_host: 3.3.3.3
+        udp: 222
+        vrf_filter: default
+        state: absent
+        provider: "{{ connection }}"
+      register: result
+
+    - assert: *true
+
+    - name: Idempotence Check
+      nxos_snmp_host: *rem2
+      register: result
+
+    - assert: *false
+    when: not (platform is match('N35|N5K'))
+
   - name: Cleanup
     nxos_snmp_host: *remove
     register: result
@@ -60,4 +124,9 @@
 
   - assert: *false
 
-  - debug: msg="END connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }}sanity test"
+  always:
+  - name: Cleanup
+    nxos_snmp_host: *remove
+    register: result
+
+  - debug: msg="END connection={{ ansible_connection }} nxos_snmp_host {{ snmp_type }} {{ snmp_version }} sanity test"
diff --git a/test/sanity/validate-modules/ignore.txt b/test/sanity/validate-modules/ignore.txt
index cb7f5c5b9ebd27..a6122a23bce67e 100644
--- a/test/sanity/validate-modules/ignore.txt
+++ b/test/sanity/validate-modules/ignore.txt
@@ -1197,6 +1197,7 @@ lib/ansible/modules/network/nxos/nxos_pim_interface.py E326
 lib/ansible/modules/network/nxos/nxos_pim_rp_address.py E326
 lib/ansible/modules/network/nxos/nxos_reboot.py E325
 lib/ansible/modules/network/nxos/nxos_smu.py E324
+lib/ansible/modules/network/nxos/nxos_snmp_host.py E324
 lib/ansible/modules/network/nxos/nxos_system.py E325
 lib/ansible/modules/network/nxos/nxos_vpc.py E324
 lib/ansible/modules/network/nxos/nxos_vpc_interface.py E325


Code review comments:
diff_hunk: @@ -1197,6 +1197,7 @@ lib/ansible/modules/network/nxos/nxos_pim_interface.py E326
 lib/ansible/modules/network/nxos/nxos_pim_rp_address.py E326
 lib/ansible/modules/network/nxos/nxos_reboot.py E325
 lib/ansible/modules/network/nxos/nxos_smu.py E324
+lib/ansible/modules/network/nxos/nxos_snmp_host.py E324
path: test/sanity/validate-modules/ignore.txt
text: @user1:
New ignores should not be added. The underlying issue should be resolved. If the issue is in module_utils or docs fragments, the fix should be done in a separate PR.

@author:
This was done deliberately to maintain backward compatibility due to the previous code bugs.

Here is the long explanation:

The defaults for snmp_type and snmp_version are 'trap' and 'v1'.
There are 4 different CLIs for nxos_snmp_host.
```
snmp-server host 1.1.1.1 informs version 2c public udp-port 222
snmp-server host 1.1.1.1 use-vrf default udp-port 222
snmp-server host 1.1.1.1 filter-vrf management udp-port 222
snmp-server host 1.1.1.1 source-interface Ethernet1/1
```
So basically the user can add use-vrf, filter-vrf and source interface in different tasks without specifying the version and type if the host ip(and udp port) already exists (the first CLI). If we define the defaults in the argument_spec (which is what is needed if we have to get rid of the ignores), the defaults of v1 and trap type are used which is incorrect, as the type is 'inform' and version is '2c' for the existing configuration. So the configuration fails.

The only other way is to make the version and trap type as 'required' parameters but then older playbooks from previous versions cannot work any longer because they were not 'required' before.
Hence this ignore is added. If the code was written properly in the beginning this could have been avoided but I don't see any other way. And please remember that this PR resolves some very MAJOR issues.
We can make a decision to ignore the backward compatibility all together since this module is broken completely before. In that case, I can take make the version as type as 'required' and remove the new ignores. 
Which option do you suggest?


@user1:
Removing the default from the argument spec is itself a non-backwards compatible change. Any playbook which previously relied on the default of `'trap'` will now use `None` instead. However, if that is indeed the only way to fix the issue then that change is unavoidable.

However, in that case, the fix for the validation error is simple. Remove the default from the documentation so it matches the actual default.

The same is true for the `version` parameter as well. The default in the docs must match the actual default in the arg spec.

@author:
Not really. If you look at the code, the way it works is that, if there is no existing snmp host, internally the code will make the version as 1 and type as trap (even though they are not defined in the arg spec). If there is an existing host, it will use the values from that host. So 'None' will not be an issue.
However, if we remove the defaults from documentation to match the arg spec, then the defaults are 'truly' removed and user MUST specify them always which breaks the backward compatibility. Since you are saying that the doc MUST match the arg spec no matter what, I can do by removing the defaults all together for type and version. But as I mentioned, old playbooks won't work if they have no do not specify the version and type.
Just wanted to clarify this before making changes.

@user1:
Simply remove the `default:` entries from the `version` and `snmp_type` docs. The logic in the code requires no additional changes.

With that change the docs will match the defaults in the arg spec. Any other default-like behavior that occurs outside of the argument spec isn't a true default and should be documented in the description instead.

@author:
@user1 Thanks for the comments. Made the changes and removed the E324 ignore.