fix nxos_igmp_interface issues (#38752)

* fix nxos_igmp_interface issues

* shippable fix

* fix oif_prefix and oif_source

* shippable fix

* shippable fix

* shippable fix

* add an example for oif_ps

* review comments

* review comments

* more review comments

* fix typo

CHANGELOG.md

@@ -10,6 +10,8 @@ Ansible Changes By Release
 ### Major Changes
 
 ### Deprecations (to be removed in 2.10)
+* In the `nxos_igmp_interface` module, `oif_prefix` and `oif_source` properties are deprecated.
+  Use the `oif_ps` parameter with a dictionary of prefix and source to values instead.
 
 See [Porting Guide](http://docs.ansible.com/ansible/devel/porting_guides/porting_guides.html) for more information
 

docs/docsite/rst/porting_guides/porting_guide_2.6.rst

@@ -22,7 +22,8 @@ No notable changes.
 Deprecated
 ==========
 
-No notable changes.
+* In the :ref:`nxos_igmp_interface <nxos_igmp_interface>` module, `oif_prefix` and `oif_source` properties are deprecated. Use
+  `ois_ps` parameter with a dictionary of prefix and source to values instead.
 
 Modules
 =======

lib/ansible/modules/network/nxos/nxos_igmp_interface.py

@@ -39,14 +39,15 @@ notes:
       C(startup_query_count), C(robustness), C(querier_timeout), C(query_mrt),
       C(query_interval), C(last_member_qrt), C(last_member_query_count),
       C(group_timeout), C(report_llg), and C(immediate_leave).
-    - When C(state=absent), all configs for C(oif_prefix), C(oif_source), and
+    - When C(state=absent), all configs for C(oif_ps), and
       C(oif_routemap) will be removed.
     - PIM must be enabled to use this module.
     - This module is for Layer 3 interfaces.
     - Route-map check not performed (same as CLI) check when configuring
       route-map with 'static-oif'
     - If restart is set to true with other params set, the restart will happen
-      last, i.e. after the configuration takes place.
+      last, i.e. after the configuration takes place. However, 'restart' itself
+      is not idempotent as it is an action and not configuration.
 options:
     interface:
         description:
@@ -55,48 +56,51 @@ options:
         required: true
     version:
         description:
-            - IGMP version. It can be 2 or 3.
+            - IGMP version. It can be 2 or 3 or keyword 'default'.
-        choices: ['2', '3']
+        choices: ['2', '3', 'default']
     startup_query_interval:
         description:
             - Query interval used when the IGMP process starts up.
-              The range is from 1 to 18000. The default is 31.
+              The range is from 1 to 18000 or keyword 'default'.
+              The default is 31.
     startup_query_count:
         description:
             - Query count used when the IGMP process starts up.
-              The range is from 1 to 10. The default is 2.
+              The range is from 1 to 10 or keyword 'default'.
+              The default is 2.
     robustness:
         description:
-            - Sets the robustness variable. Values can range from 1 to 7.
+            - Sets the robustness variable. Values can range from 1 to 7 or
-              The default is 2.
+              keyword 'default'. The default is 2.
     querier_timeout:
         description:
             - Sets the querier timeout that the software uses when deciding
               to take over as the querier. Values can range from 1 to 65535
-              seconds. The default is 255 seconds.
+              seconds or keyword 'default'. The default is 255 seconds.
     query_mrt:
         description:
             - Sets the response time advertised in IGMP queries.
-              Values can range from 1 to 25 seconds. The default is 10 seconds.
+              Values can range from 1 to 25 seconds or keyword 'default'.
+              The default is 10 seconds.
     query_interval:
         description:
             - Sets the frequency at which the software sends IGMP host query
-              messages. Values can range from 1 to 18000 seconds.
+              messages. Values can range from 1 to 18000 seconds or keyword
-              The default is 125 seconds.
+              'default'. The default is 125 seconds.
     last_member_qrt:
         description:
             - Sets the query interval waited after sending membership reports
               before the software deletes the group state. Values can range
-              from 1 to 25 seconds. The default is 1 second.
+              from 1 to 25 seconds or keyword 'default'. The default is 1 second.
     last_member_query_count:
         description:
             - Sets the number of times that the software sends an IGMP query
               in response to a host leave message.
-              Values can range from 1 to 5. The default is 2.
+              Values can range from 1 to 5 or keyword 'default'. The default is 2.
     group_timeout:
         description:
             - Sets the group membership timeout for IGMPv2.
-              Values can range from 3 to 65,535 seconds.
+              Values can range from 3 to 65,535 seconds or keyword 'default'.
               The default is 260 seconds.
     report_llg:
         description:
@@ -105,7 +109,6 @@ options:
               Reports are always sent for nonlink local groups.
               By default, reports are not sent for link local groups.
         type: bool
-        default: 'no'
     immediate_leave:
         description:
             - Enables the device to remove the group entry from the multicast
@@ -115,30 +118,44 @@ options:
               device does not send group-specific queries.
               The default is disabled.
         type: bool
-        default: 'no'
     oif_routemap:
         description:
-            - Configure a routemap for static outgoing interface (OIF).
+            - Configure a routemap for static outgoing interface (OIF) or
+              keyword 'default'.
     oif_prefix:
         description:
-            - Configure a prefix for static outgoing interface (OIF).
+            - This argument is deprecated, please use oif_ps instead.
+              Configure a prefix for static outgoing interface (OIF).
     oif_source:
         description:
-            - Configure a source for static outgoing interface (OIF).
+            - This argument is deprecated, please use oif_ps instead.
+              Configure a source for static outgoing interface (OIF).
+    oif_ps:
+        description:
+            - Configure prefixes and sources for static outgoing interface (OIF). This
+              is a list of dict where each dict has source and prefix defined or just
+              prefix if source is not needed. The specified values will be configured
+              on the device and if any previous prefix/sources exist, they will be removed.
+              Keyword 'default' is also accpted which removes all existing prefix/sources.
+        version_added: 2.6
     restart:
         description:
-            - Restart IGMP.
+            - Restart IGMP. This is NOT idempotent as this is action only.
         type: bool
+        default: False
     state:
         description:
             - Manages desired state of the resource.
         default: present
-        choices: ['present', 'default']
+        choices: ['present', 'absent', 'default']
 '''
 EXAMPLES = '''
 - nxos_igmp_interface:
     interface: ethernet1/32
     startup_query_interval: 30
+    oif_ps:
+      - { 'prefix': '238.2.2.6' }
+      - { 'source': '192.168.0.1', 'prefix': '238.2.2.5'}
     state: present
 '''
 RETURN = '''
@@ -146,54 +163,25 @@ proposed:
     description: k/v pairs of parameters passed into module
     returned: always
     type: dict
-    sample: {"asn": "65535", "router_id": "1.1.1.1", "vrf": "test"}
+    sample: {"startup_query_count": "30",
+             "oif_ps": [{'prefix': '238.2.2.6'}, {'source': '192.168.0.1', 'prefix': '238.2.2.5'}]}
 existing:
-    description: k/v pairs of existing BGP configuration
+    description: k/v pairs of existing igmp_interface configuration
     returned: always
     type: dict
-    sample: {"asn": "65535", "bestpath_always_compare_med": false,
+    sample: {"startup_query_count": "2", "oif_ps": []}
-            "bestpath_aspath_multipath_relax": false,
-            "bestpath_compare_neighborid": false,
-            "bestpath_compare_routerid": false,
-            "bestpath_cost_community_ignore": false,
-            "bestpath_med_confed": false,
-            "bestpath_med_missing_as_worst": false,
-            "bestpath_med_non_deterministic": false, "cluster_id": "",
-            "confederation_id": "", "confederation_peers": "",
-            "graceful_restart": true, "graceful_restart_helper": false,
-            "graceful_restart_timers_restart": "120",
-            "graceful_restart_timers_stalepath_time": "300", "local_as": "",
-            "log_neighbor_changes": false, "maxas_limit": "",
-            "neighbor_down_fib_accelerate": false, "reconnect_interval": "60",
-            "router_id": "11.11.11.11", "suppress_fib_pending": false,
-            "timer_bestpath_limit": "", "timer_bgp_hold": "180",
-            "timer_bgp_keepalive": "60", "vrf": "test"}
 end_state:
-    description: k/v pairs of BGP configuration after module execution
+    description: k/v pairs of igmp interface configuration after module execution
     returned: always
     type: dict
-    sample: {"asn": "65535", "bestpath_always_compare_med": false,
+    sample: {"startup_query_count": "30",
-            "bestpath_aspath_multipath_relax": false,
+             "oif_ps": [{'prefix': '238.2.2.6'}, {'source': '192.168.0.1', 'prefix': '238.2.2.5'}]}
-            "bestpath_compare_neighborid": false,
-            "bestpath_compare_routerid": false,
-            "bestpath_cost_community_ignore": false,
-            "bestpath_med_confed": false,
-            "bestpath_med_missing_as_worst": false,
-            "bestpath_med_non_deterministic": false, "cluster_id": "",
-            "confederation_id": "", "confederation_peers": "",
-            "graceful_restart": true, "graceful_restart_helper": false,
-            "graceful_restart_timers_restart": "120",
-            "graceful_restart_timers_stalepath_time": "300", "local_as": "",
-            "log_neighbor_changes": false, "maxas_limit": "",
-            "neighbor_down_fib_accelerate": false, "reconnect_interval": "60",
-            "router_id": "1.1.1.1",  "suppress_fib_pending": false,
-            "timer_bestpath_limit": "", "timer_bgp_hold": "180",
-            "timer_bgp_keepalive": "60", "vrf": "test"}
 updates:
     description: commands sent to the device
     returned: always
     type: list
-    sample: ["router bgp 65535", "vrf test", "router-id 1.1.1.1"]
+    sample: ["interface Ethernet1/32", "ip igmp startup-query-count 30",
+             "ip igmp static-oif 238.2.2.6", "ip igmp static-oif 238.2.2.5 source 192.168.0.1"]
 changed:
     description: check to see if a change was made on the device
     returned: always
@@ -299,6 +287,8 @@ def get_igmp_interface(module, interface):
     body = execute_show_command(command, module)[0]
 
     if body:
+        if 'not running' in body:
+            return igmp
         resource = body['TABLE_vrf']['ROW_vrf']['TABLE_if']['ROW_if']
         igmp = apply_key_map(key_map, resource)
         report_llg = str(resource['ReportingForLinkLocal']).lower()
@@ -369,7 +359,7 @@ def get_igmp_interface(module, interface):
     return igmp
 
 
-def config_igmp_interface(delta, found_both, found_prefix):
+def config_igmp_interface(delta, existing, existing_oif_prefix_source):
     CMDS = {
         'version': 'ip igmp version {0}',
         'startup_query_interval': 'ip igmp startup-query-interval {0}',
@@ -390,18 +380,45 @@ def config_igmp_interface(delta, found_both, found_prefix):
 
     commands = []
     command = None
+    def_vals = get_igmp_interface_defaults()
 
     for key, value in delta.items():
-        if key == 'oif_source' or found_both or found_prefix:
+        if key == 'oif_ps':
-            pass
+            for each in value:
-        elif key == 'oif_prefix':
+                if each in existing_oif_prefix_source:
-            if delta.get('oif_source'):
+                    existing_oif_prefix_source.remove(each)
-                command = CMDS.get('oif_prefix_source').format(
+                else:
-                    delta.get('oif_prefix'), delta.get('oif_source'))
+                    # add new prefix/sources
+                    pf = each['prefix']
+                    src = ''
+                    if 'source' in each.keys():
+                        src = each['source']
+                    if src:
+                        commands.append(CMDS.get('oif_prefix_source').format(pf, src))
                     else:
-                command = CMDS.get('oif_prefix').format(
+                        commands.append(CMDS.get('oif_prefix').format(pf))
-                    delta.get('oif_prefix'))
+            if existing_oif_prefix_source:
+                for each in existing_oif_prefix_source:
+                        # remove stale prefix/sources
+                        pf = each['prefix']
+                        src = ''
+                        if 'source' in each.keys():
+                            src = each['source']
+                        if src:
+                            commands.append('no ' + CMDS.get('oif_prefix_source').format(pf, src))
+                        else:
+                            commands.append('no ' + CMDS.get('oif_prefix').format(pf))
+        elif key == 'oif_routemap':
+            if value == 'default':
+                if existing.get(key):
+                    command = 'no ' + CMDS.get(key).format(existing.get(key))
+            else:
+                command = CMDS.get(key).format(value)
         elif value:
+            if value == 'default':
+                if def_vals.get(key) != existing.get(key):
+                    command = CMDS.get(key).format(def_vals.get(key))
+            else:
                 command = CMDS.get(key).format(value)
         elif not value:
             command = 'no {0}'.format(CMDS.get(key).format(value))
@@ -442,12 +459,12 @@ def get_igmp_interface_defaults():
     return default
 
 
-def config_default_igmp_interface(existing, delta, found_both, found_prefix):
+def config_default_igmp_interface(existing, delta):
     commands = []
     proposed = get_igmp_interface_defaults()
     delta = dict(set(proposed.items()).difference(existing.items()))
     if delta:
-        command = config_igmp_interface(delta, found_both, found_prefix)
+        command = config_igmp_interface(delta, existing, existing_oif_prefix_source=None)
 
         if command:
             for each in command:
@@ -459,10 +476,9 @@ def config_default_igmp_interface(existing, delta, found_both, found_prefix):
 def config_remove_oif(existing, existing_oif_prefix_source):
     commands = []
     command = None
-    if existing.get('routemap'):
+    if existing.get('oif_routemap'):
-        command = 'no ip igmp static-oif route-map {0}'.format(
+        commands.append('no ip igmp static-oif route-map {0}'.format(existing.get('oif_routemap')))
-            existing.get('routemap'))
+    elif existing_oif_prefix_source:
-    if existing_oif_prefix_source:
         for each in existing_oif_prefix_source:
             if each.get('prefix') and each.get('source'):
                 command = 'no ip igmp static-oif {0} source {1} '.format(
@@ -495,16 +511,22 @@ def main():
         report_llg=dict(type='bool'),
         immediate_leave=dict(type='bool'),
         oif_routemap=dict(required=False, type='str'),
-        oif_prefix=dict(required=False, type='str'),
+        oif_prefix=dict(required=False, type='str', removed_in_version='2.10'),
-        oif_source=dict(required=False, type='str'),
+        oif_source=dict(required=False, type='str', removed_in_version='2.10'),
+        oif_ps=dict(required=False, type='list', elements='dict'),
         restart=dict(type='bool', default=False),
         state=dict(choices=['present', 'absent', 'default'],
                    default='present')
     )
 
     argument_spec.update(nxos_argument_spec)
+    mutually_exclusive = [('oif_ps', 'oif_prefix'),
+                          ('oif_ps', 'oif_source'),
+                          ('oif_ps', 'oif_routemap'),
+                          ('oif_prefix', 'oif_routemap')]
 
     module = AnsibleModule(argument_spec=argument_spec,
+                           mutually_exclusive=mutually_exclusive,
                            supports_check_mode=True)
 
     warnings = list()
@@ -515,19 +537,19 @@ def main():
     oif_prefix = module.params['oif_prefix']
     oif_source = module.params['oif_source']
     oif_routemap = module.params['oif_routemap']
+    oif_ps = module.params['oif_ps']
 
-    if oif_source:
+    if oif_source and not oif_prefix:
-        if not oif_prefix:
         module.fail_json(msg='oif_prefix required when setting oif_source')
+    elif oif_source and oif_prefix:
+        oif_ps = [{'source': oif_source, 'prefix': oif_prefix}]
+    elif not oif_source and oif_prefix:
+        oif_ps = [{'prefix': oif_prefix}]
 
     intf_type = get_interface_type(interface)
     if get_interface_mode(interface, intf_type, module) == 'layer2':
         module.fail_json(msg='this module only works on Layer 3 interfaces')
 
-    if oif_prefix and oif_routemap:
-        module.fail_json(msg='cannot use oif_prefix AND oif_routemap.'
-                             '  select one.')
-
     existing = get_igmp_interface(module, interface)
     existing_copy = existing.copy()
     end_state = existing_copy
@@ -543,7 +565,7 @@ def main():
         module.fail_json(msg='Delete static-oif configurations on this '
                              'interface if you want to use a routemap')
 
-    if oif_prefix and existing.get('oif_routemap'):
+    if oif_ps and existing.get('oif_routemap'):
         module.fail_json(msg='Delete static-oif route-map configuration '
                              'on this interface if you want to config '
                              'static entries')
@@ -562,8 +584,6 @@ def main():
         'report_llg',
         'immediate_leave',
         'oif_routemap',
-        'oif_prefix',
-        'oif_source'
     ]
 
     changed = False
@@ -581,41 +601,26 @@ def main():
         for each in CANNOT_ABSENT:
             if each in proposed:
                 module.fail_json(msg='only params: oif_prefix, oif_source, '
-                                     'oif_routemap can be used when '
+                                     'oif_ps, oif_routemap can be used when '
                                      'state=absent')
 
-    # delta check for all params except oif_prefix and oif_source
+    # delta check for all params except oif_ps
     delta = dict(set(proposed.items()).difference(existing.items()))
 
-    # now check to see there is a delta for prefix and source command option
+    if oif_ps:
-    found_both = False
+        if oif_ps == ['default']:
-    found_prefix = False
+            delta['oif_ps'] = []
-
+        else:
-    if existing_oif_prefix_source:
+            delta['oif_ps'] = oif_ps
-        if oif_prefix and oif_source:
-            for each in existing_oif_prefix_source:
-                if (oif_prefix == each.get('prefix') and
-                        oif_source == each.get('source')):
-                    found_both = True
-            if not found_both:
-                delta['prefix'] = oif_prefix
-                delta['source'] = oif_source
-        elif oif_prefix:
-            for each in existing_oif_prefix_source:
-                if oif_prefix == each.get('prefix') and not each.get('source'):
-                    found_prefix = True
-            if not found_prefix:
-                delta['prefix'] = oif_prefix
 
     if state == 'present':
         if delta:
-            command = config_igmp_interface(delta, found_both, found_prefix)
+            command = config_igmp_interface(delta, existing, existing_oif_prefix_source)
             if command:
                 commands.append(command)
 
     elif state == 'default':
-        command = config_default_igmp_interface(existing, delta,
+        command = config_default_igmp_interface(existing, delta)
-                                                found_both, found_prefix)
         if command:
             commands.append(command)
     elif state == 'absent':
@@ -626,14 +631,10 @@ def main():
         if command:
             commands.append(command)
 
-        command = config_default_igmp_interface(existing, delta,
+        command = config_default_igmp_interface(existing, delta)
-                                                found_both, found_prefix)
         if command:
             commands.append(command)
 
-    if module.params['restart']:
-        commands.append('restart igmp')
-
     cmds = []
     results = {}
     if commands:
@@ -649,6 +650,10 @@ def main():
             if 'configure' in cmds:
                 cmds.pop(0)
 
+    if module.params['restart']:
+        cmd = {'command': 'restart igmp', 'output': 'text'}
+        run_commands(module, cmd)
+
     results['proposed'] = proposed
     results['existing'] = existing_copy
     results['updates'] = cmds

test/integration/targets/nxos_igmp_interface/tests/common/sanity.yaml

@@ -6,6 +6,9 @@
 # Select interface for test
 - set_fact: intname="{{ nxos_int1 }}"
 
+- set_fact: restart="true"
+  when: platform is not match("N35")
+
 - name: "Enable feature PIM"
   nxos_feature:
     feature: pim
@@ -13,6 +16,14 @@
     provider: "{{ connection }}"
   ignore_errors: yes
 
+- name: Put interface in default mode
+  nxos_config:
+    commands:
+    - "default interface {{ intname }}"
+    provider: "{{ connection }}"
+    match: none
+  ignore_errors: yes
+
 - block:
   
   - name: put interface in L3 and enable PIM
@@ -39,7 +50,10 @@
       last_member_query_count: 4
       report_llg: true
       immediate_leave: true
-      restart: false
+      group_timeout: 300
+      # deprecated
+      oif_prefix: 239.255.255.2
+      oif_source: 1.1.1.1
       state: present
       provider: "{{ connection }}"
     register: result
@@ -56,7 +70,78 @@
       that:
         - "result.changed == false"
 
-  - name: Configure igmp interface with default value
+  - name: Configure igmp interface with some default values
+    nxos_igmp_interface: &sdef
+      interface: "{{ intname }}"
+      version: default
+      startup_query_interval: default
+      startup_query_count: default
+      robustness: default
+      querier_timeout: default
+      query_mrt: default
+      query_interval: default
+      last_member_qrt: default
+      last_member_query_count: default
+      group_timeout: default
+      oif_ps:
+        - {'prefix': '238.2.2.6'}
+        - {'prefix': '238.2.2.5'}
+        - {'source': '1.1.1.1', 'prefix': '238.2.2.5'}
+      state: present
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Configure igmp interface with some default values"
+    nxos_igmp_interface: *sdef
+    register: result
+
+  - assert: *false
+
+  - name: restart igmp
+    nxos_igmp_interface: &restart
+      interface: "{{ intname }}"
+      restart: "{{restart|default(omit)}}"
+      provider: "{{ connection }}"
+
+  - name: Configure igmp interface with default oif_ps
+    nxos_igmp_interface: &defoif
+      interface: "{{ intname }}"
+      oif_ps: default
+      state: present
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Configure igmp interface with default oif_ps"
+    nxos_igmp_interface: *defoif
+    register: result
+
+  - assert: *false
+
+  - name: Configure igmp interface with oif_routemap
+    nxos_igmp_interface: &orm
+      interface: "{{ intname }}"
+      version: 3
+      startup_query_interval: 60
+      startup_query_count: 5
+      robustness: 6
+      oif_routemap: abcd
+      state: present
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Configure igmp interface with oif_routemap"
+    nxos_igmp_interface: *orm
+    register: result
+
+  - assert: *false
+
+  - name: Configure igmp interface with default state
     nxos_igmp_interface: &default
       interface: "{{ intname }}"
       state: default
@@ -65,15 +150,30 @@
 
   - assert: *true
 
-  - name: "Check Idempotence - Configure igmp interface with default value"
+  - name: "Check Idempotence - Configure igmp interface with default state"
     nxos_igmp_interface: *default
     register: result
 
   - assert: *false
 
+  - name: Configure igmp interface with absent state
+    nxos_igmp_interface: &absent
+      interface: "{{ intname }}"
+      state: absent
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Configure igmp interface with absent state"
+    nxos_igmp_interface: *absent
+    register: result
+
+  - assert: *false
+
   always:
-  - name: Configure igmp interface with default value
+  - name: Configure igmp interface with absent state
-    nxos_igmp_interface: *default
+    nxos_igmp_interface: *absent
     register: result
 
   - name: Put interface in default mode