fix nxos_hsrp issues (#38410)

* fix nxos_hsrp issues

* shippable fix

* add examples for md5 auth

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

@@ -53,21 +53,25 @@ options:
   version:
     description:
       - HSRP version.
-    default: 2
+    default: 1
     choices: ['1','2']
   priority:
     description:
-      - HSRP priority.
+      - HSRP priority or keyword 'default'.
   preempt:
     description:
       - Enable/Disable preempt.
     choices: ['enabled', 'disabled']
   vip:
     description:
-      - HSRP virtual IP address.
+      - HSRP virtual IP address or keyword 'default'
   auth_string:
     description:
-      - Authentication string.
+      - Authentication string. If this needs to be hidden(for md5 type), the string
+        should be 7 followed by the key string. Otherwise, it can be 0 followed by
+        key string or just key string (for backward compatibility). For text type,
+        this should be just be a key string. if this is 'default', authentication
+        is removed.
   auth_type:
     description:
       - Authentication type.
@@ -90,6 +94,7 @@ EXAMPLES = '''
     host: 68.170.147.165
 
 - name: Ensure HSRP is configured with following params on a SVI
+        with clear text authentication
   nxos_hsrp:
     group: 10
     vip: 10.1.1.1
@@ -100,6 +105,30 @@ EXAMPLES = '''
     auth_type: text
     auth_string: CISCO
 
+- name: Ensure HSRP is configured with md5 authentication and clear
+        authentication string
+  nxos_hsrp:
+    group: 10
+    vip: 10.1.1.1
+    priority: 150
+    interface: vlan10
+    preempt: enabled
+    host: 68.170.147.165
+    auth_type: md5
+    auth_string: "0 1234"
+
+- name: Ensure HSRP is configured with md5 authentication and hidden
+        authentication string
+  nxos_hsrp:
+    group: 10
+    vip: 10.1.1.1
+    priority: 150
+    interface: vlan10
+    preempt: enabled
+    host: 68.170.147.165
+    auth_type: md5
+    auth_string: "7 1234"
+
 - name: Remove HSRP config for given interface, group, and VIP
   nxos_hsrp:
     group: 10
@@ -122,6 +151,14 @@ from ansible.module_utils.network.nxos.nxos import get_capabilities, nxos_argume
 from ansible.module_utils.basic import AnsibleModule
 
 
+PARAM_TO_DEFAULT_KEYMAP = {
+    'vip': None,
+    'priority': '100',
+    'auth_type': 'text',
+    'auth_string': 'cisco',
+}
+
+
 def execute_show_command(command, module):
     device_info = get_capabilities(module)
     network_api = device_info.get('network_api', 'nxapi')
@@ -186,29 +223,8 @@ def get_interface_mode(interface, intf_type, module):
     return mode
 
 
-def get_hsrp_groups_on_interfaces(device, module):
-    command = 'show hsrp all'
-    hsrp = {}
-
-    try:
-        body = execute_show_command(command, module)[0]
-        get_data = body['TABLE_grp_detail']['ROW_grp_detail']
-    except (IndexError, KeyError, AttributeError):
-        return {}
-
-    for entry in get_data:
-        interface = str(entry['sh_if_index'].lower())
-        value = hsrp.get(interface, 'new')
-        if value == 'new':
-            hsrp[interface] = []
-        group = str(entry['sh_group_num'])
-        hsrp[interface].append(group)
-
-    return hsrp
-
-
 def get_hsrp_group(group, interface, module):
-    command = 'show hsrp group {0}'.format(group)
+    command = 'show hsrp group {0} all'.format(group)
     hsrp = {}
 
     hsrp_key = {
@@ -219,6 +235,7 @@ def get_hsrp_group(group, interface, module):
         'sh_preempt': 'preempt',
         'sh_vip': 'vip',
         'sh_authentication_type': 'auth_type',
+        'sh_keystring_attr': 'auth_enc',
         'sh_authentication_data': 'auth_string'
     }
 
@@ -241,6 +258,12 @@ def get_hsrp_group(group, interface, module):
         elif parsed_hsrp['version'] == 'v2':
             parsed_hsrp['version'] = '2'
 
+        if parsed_hsrp['auth_type'] == 'md5':
+            if parsed_hsrp['auth_enc'] == 'hidden':
+                parsed_hsrp['auth_enc'] = '7'
+            else:
+                parsed_hsrp['auth_enc'] = '0'
+
         if parsed_hsrp['interface'] == interface:
             return parsed_hsrp
 
@@ -252,24 +275,45 @@ def get_commands_remove_hsrp(group, interface):
     return commands
 
 
-def get_commands_config_hsrp(delta, interface, args):
+def get_commands_config_hsrp(delta, interface, args, existing):
     commands = []
 
     config_args = {
         'group': 'hsrp {group}',
-        'priority': 'priority {priority}',
+        'priority': '{priority}',
         'preempt': '{preempt}',
-        'vip': 'ip {vip}'
+        'vip': '{vip}'
     }
 
     preempt = delta.get('preempt', None)
     group = delta.get('group', None)
+    vip = delta.get('vip', None)
+    priority = delta.get('priority', None)
+
     if preempt:
         if preempt == 'enabled':
             delta['preempt'] = 'preempt'
         elif preempt == 'disabled':
             delta['preempt'] = 'no preempt'
 
+    if priority:
+        if priority == 'default':
+            if existing and existing.get('priority') != PARAM_TO_DEFAULT_KEYMAP.get('priority'):
+                delta['priority'] = 'no priority'
+            else:
+                del(delta['priority'])
+        else:
+            delta['priority'] = 'priority {0}'.format(delta['priority'])
+
+    if vip:
+        if vip == 'default':
+            if existing and existing.get('vip') != PARAM_TO_DEFAULT_KEYMAP.get('vip'):
+                delta['vip'] = 'no ip'
+            else:
+                del(delta['vip'])
+        else:
+            delta['vip'] = 'ip {0}'.format(delta['vip'])
+
     for key in delta:
         command = config_args.get(key, 'DNE').format(**delta)
         if command and command != 'DNE':
@@ -281,17 +325,22 @@ def get_commands_config_hsrp(delta, interface, args):
 
     auth_type = delta.get('auth_type', None)
     auth_string = delta.get('auth_string', None)
+    auth_enc = delta.get('auth_enc', None)
     if auth_type or auth_string:
         if not auth_type:
             auth_type = args['auth_type']
         elif not auth_string:
             auth_string = args['auth_string']
+        if auth_string != 'default':
             if auth_type == 'md5':
-            command = 'authentication md5 key-string {0}'.format(auth_string)
+                command = 'authentication md5 key-string {0} {1}'.format(auth_enc, auth_string)
                 commands.append(command)
             elif auth_type == 'text':
                 command = 'authentication text {0}'.format(auth_string)
                 commands.append(command)
+        else:
+            if existing and existing.get('auth_string') != PARAM_TO_DEFAULT_KEYMAP.get('auth_string'):
+                commands.append('no authentication')
 
     if commands and not group:
         commands.insert(0, 'hsrp {0}'.format(args['group']))
@@ -336,35 +385,11 @@ def validate_config(body, vip, module):
                          vip=vip)
 
 
-def validate_params(param, module):
-    value = module.params[param]
-    version = module.params['version']
-
-    if param == 'group':
-        try:
-            if (int(value) < 0 or int(value) > 255) and version == '1':
-                raise ValueError
-            elif int(value) < 0 or int(value) > 4095:
-                raise ValueError
-        except ValueError:
-            module.fail_json(msg="Warning! 'group' must be an integer between"
-                                 " 0 and 255 when version 1 and up to 4095 "
-                                 "when version 2.", group=value,
-                                 version=version)
-    elif param == 'priority':
-        try:
-            if (int(value) < 0 or int(value) > 255):
-                raise ValueError
-        except ValueError:
-            module.fail_json(msg="Warning! 'priority' must be an integer "
-                                 "between 0 and 255", priority=value)
-
-
 def main():
     argument_spec = dict(
         group=dict(required=True, type='str'),
         interface=dict(required=True),
-        version=dict(choices=['1', '2'], default='2', required=False),
+        version=dict(choices=['1', '2'], default='1', required=False),
         priority=dict(type='str', required=False),
         preempt=dict(type='str', choices=['disabled', 'enabled'], required=False),
         vip=dict(type='str', required=False),
@@ -388,18 +413,24 @@ def main():
     preempt = module.params['preempt']
     vip = module.params['vip']
     auth_type = module.params['auth_type']
-    auth_string = module.params['auth_string']
+    auth_full_string = module.params['auth_string']
+    auth_enc = '0'
+    auth_string = None
+    if auth_full_string:
+        kstr = auth_full_string.split()
+        if len(kstr) == 2:
+            auth_enc = kstr[0]
+            auth_string = kstr[1]
+        elif len(kstr) == 1:
+            auth_string = kstr[0]
+        else:
+            module.fail_json(msg='Inavlid auth_string')
+        if auth_enc != '0' and auth_enc != '7':
+            module.fail_json(msg='Inavlid auth_string, only 0 or 7 allowed')
 
     device_info = get_capabilities(module)
     network_api = device_info.get('network_api', 'nxapi')
 
-    if state == 'present' and not vip:
-        module.fail_json(msg='the "vip" param is required when state=present')
-
-    for param in ['group', 'priority']:
-        if module.params[param] is not None:
-            validate_params(param, module)
-
     intf_type = get_interface_type(interface)
     if (intf_type != 'ethernet' and network_api == 'cliconf'):
         if is_default(interface, module) == 'DNE':
@@ -421,7 +452,7 @@ def main():
 
     args = dict(group=group, version=version, priority=priority,
                 preempt=preempt, vip=vip, auth_type=auth_type,
-                auth_string=auth_string)
+                auth_string=auth_string, auth_enc=auth_enc)
 
     proposed = dict((k, v) for k, v in args.items() if v is not None)
 
@@ -435,7 +466,7 @@ def main():
 
     elif not proposed.get('auth_type', None) and existing:
         if (proposed['version'] == '1' and
-                existing['auth_type'] == 'md5'):
+                existing['auth_type'] == 'md5') and state == 'present':
             module.fail_json(msg="Existing auth_type is md5. It's recommended "
                                  "to use HSRP v2 when using md5")
 
@@ -444,7 +475,7 @@ def main():
         delta = dict(
             set(proposed.items()).difference(existing.items()))
         if delta:
-            command = get_commands_config_hsrp(delta, interface, args)
+            command = get_commands_config_hsrp(delta, interface, args, existing)
             commands.extend(command)
 
     elif state == 'absent':

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

@@ -4,7 +4,8 @@
   when: ansible_connection == "local"
 
 # Select interface for test
-- set_fact: intname="{{ nxos_int1 }}"
+- set_fact: intname1="{{ nxos_int1 }}"
+- set_fact: intname2="{{ nxos_int2 }}"
 
 - block:
   - name: "Enable feature hsrp"
@@ -13,25 +14,34 @@
       state: enabled
       provider: "{{ connection }}"
 
-  - name: "change interface mode"
+  - name: "change int1 mode"
     nxos_config:
       commands:
         - no switchport
       parents:
-        - "interface {{ intname }}"
+        - "interface {{ intname1 }}"
+      match: none
+      provider: "{{ connection }}"
+
+  - name: "change int2 mode"
+    nxos_config:
+      commands:
+        - no switchport
+      parents:
+        - "interface {{ intname2 }}"
       match: none
       provider: "{{ connection }}"
 
   - name: "configure nxos_hsrp"
-    nxos_hsrp: &configure
+    nxos_hsrp: &conf1000
-      group: 10
+      group: 1000
       version: 2
       vip: 10.1.1.1
       priority: 150
-      interface: "{{ intname }}"
+      interface: "{{ intname1 }}"
       preempt: enabled
-      auth_type: text
+      auth_type: md5
-      auth_string: CISCO
+      auth_string: "7 1234"
       provider: "{{ connection }}"
     register: result
 
@@ -40,24 +50,97 @@
         - "result.changed == true"
 
   - name: "Conf Idempotence"
-    nxos_hsrp: *configure
+    nxos_hsrp: *conf1000
     register: result
 
   - assert: &false
       that:
         - "result.changed == false"
 
+  - name: "configure group 100"
+    nxos_hsrp: &conf100
+      group: 100
+      version: 2
+      vip: 2.2.2.2
+      priority: 25
+      interface: "{{ intname1 }}"
+      preempt: enabled
+      auth_type: md5
+      auth_string: "0 1234"
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Conf Idempotence"
+    nxos_hsrp: *conf100
+    register: result
+
+  - assert: *false
+
+  - name: "change group 100"
+    nxos_hsrp: &chg100
+      group: 100
+      version: 2
+      vip: default
+      priority: default
+      interface: "{{ intname1 }}"
+      preempt: disabled
+      auth_type: md5
+      auth_string: "0 1234"
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Conf Idempotence"
+    nxos_hsrp: *chg100
+    register: result
+
+  - assert: *false
+
+  - name: "configure group 200"
+    nxos_hsrp: &conf200
+      group: 200
+      vip: 3.3.3.3
+      version: 1
+      interface: "{{ intname2 }}"
+      auth_type: text
+      auth_string: "1234"
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Conf Idempotence"
+    nxos_hsrp: *conf200
+    register: result
+
+  - assert: *false
+
+  - name: "change group 200"
+    nxos_hsrp: &chg200
+      group: 200
+      vip: 3.3.3.3
+      version: 2
+      interface: "{{ intname2 }}"
+      auth_type: text
+      auth_string: default
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Conf Idempotence"
+    nxos_hsrp: *chg200
+    register: result
+
+  - assert: *false
+
   - name: "remove nxos_hsrp"
     nxos_hsrp: &remove
-      group: 10
+      group: 1000
-      version: 2
+      interface: "{{ intname1 }}"
-      vip: 10.1.1.1
-      priority: 150
-      interface: "{{ intname }}"
-      preempt: enabled
-      auth_type: text
-      auth_string: CISCO
-      provider: "{{ connection }}"
       state: absent
     register: result
 
@@ -70,20 +153,6 @@
   - assert: *false
 
   always:
-  - name: "remove nxos_hsrp"
-    nxos_hsrp:
-      group: 10
-      version: 2
-      vip: 10.1.1.1
-      priority: 150
-      interface: "{{ intname }}"
-      preempt: enabled
-      auth_type: text
-      auth_string: CISCO
-      provider: "{{ connection }}"
-      state: absent
-    ignore_errors: yes
-
   - name: "Disable feature hsrp"
     nxos_feature: 
       feature: hsrp

test/units/modules/network/nxos/test_nxos_hsrp.py

@@ -59,7 +59,7 @@ class TestNxosHsrpModule(TestNxosModule):
         result = self.execute_module(changed=True)
         self.assertEqual(sorted(result['commands']), sorted(['config t',
                                                              'interface ethernet1/2',
-                                                             'hsrp version 2',
+                                                             'hsrp version 1',
                                                              'hsrp 10',
                                                              'priority 150',
                                                              'ip 192.0.2.2/8',