nxos bugfixes cherry-pick (#38950)

* fix nxos_ntp_options (#38695) (cherry picked from commit 1d975bdc93) * fix ntp_auth issues (#38824) (cherry picked from commit a372142434) * update nxos changelog Signed-off-by: Trishna Guha <trishnaguha17@gmail.com>
2018-04-18 09:59:39 -04:00 · 2018-04-18 09:59:39 -04:00 · 456fe2add1
commit 456fe2add1
parent 9a7da8d3da
5 changed files with 182 additions and 105 deletions
--- a/changelogs/fragments/nxos_bugfixes.yaml
+++ b/changelogs/fragments/nxos_bugfixes.yaml
@ -18,3 +18,5 @@ bugfixes:
 - nxos_igmp - Fix nxos_igmp (https://github.com/ansible/ansible/pull/38496)
 - nxos_hsrp - Fix nxos_hsrp (https://github.com/ansible/ansible/pull/38410)
 - nxos_igmp_snooping - Fix nxos_igmp_snooping (https://github.com/ansible/ansible/pull/38566)
+- nxos_ntp_auth - Fix nxos_ntp_auth issues (https://github.com/ansible/ansible/pull/38824)
+- nxos_ntp_options - Fix nxos_ntp_options issues (https://github.com/ansible/ansible/pull/38695)
--- a/lib/ansible/modules/network/nxos/nxos_ntp_auth.py
+++ b/lib/ansible/modules/network/nxos/nxos_ntp_auth.py
@ -34,20 +34,15 @@ author:
    - Jason Edelman (@jedelman8)
 notes:
    - Tested against NXOSv 7.3.(0)D1(1) on VIRL
-    - If C(state=absent), the module will attempt to remove the given key configuration.
-      If a matching key configuration isn't found on the device, the module will fail.
+    - If C(state=absent), the module will remove the given key configuration if it exists.
    - If C(state=absent) and C(authentication=on), authentication will be turned off.
-    - If C(state=absent) and C(authentication=off), authentication will be turned on.
 options:
    key_id:
        description:
            - Authentication key identifier (numeric).
-        required: true
    md5string:
        description:
            - MD5 String.
-        required: true
-        default: null
    auth_type:
        description:
            - Whether the given md5string is in cleartext or
@ -162,17 +157,19 @@ def get_ntp_auth_key(key_id, module):
    authentication_key = {}
    command = 'show run | inc ntp.authentication-key.{0}'.format(key_id)
    auth_regex = (r".*ntp\sauthentication-key\s(?P<key_id>\d+)\s"
-                  r"md5\s(?P<md5string>\S+).*")
+                  r"md5\s(?P<md5string>\S+)\s(?P<atype>\S+).*")

    body = execute_show_command(command, module)[0]

    try:
        match_authentication = re.match(auth_regex, body, re.DOTALL)
        group_authentication = match_authentication.groupdict()
-        key_id = group_authentication["key_id"]
-        md5string = group_authentication['md5string']
-        authentication_key['key_id'] = key_id
-        authentication_key['md5string'] = md5string
+        authentication_key['key_id'] = group_authentication['key_id']
+        authentication_key['md5string'] = group_authentication['md5string']
+        if group_authentication['atype'] == '7':
+            authentication_key['auth_type'] = 'encrypt'
+        else:
+            authentication_key['auth_type'] = 'text'
    except (AttributeError, TypeError):
        authentication_key = {}

@ -206,10 +203,11 @@ def auth_type_to_num(auth_type):

 def set_ntp_auth_key(key_id, md5string, auth_type, trusted_key, authentication):
    ntp_auth_cmds = []
-    auth_type_num = auth_type_to_num(auth_type)
-    ntp_auth_cmds.append(
-        'ntp authentication-key {0} md5 {1} {2}'.format(
-            key_id, md5string, auth_type_num))
+    if key_id and md5string:
+        auth_type_num = auth_type_to_num(auth_type)
+        ntp_auth_cmds.append(
+            'ntp authentication-key {0} md5 {1} {2}'.format(
+                key_id, md5string, auth_type_num))

    if trusted_key == 'true':
        ntp_auth_cmds.append(
@ -230,25 +228,22 @@ def set_ntp_auth_key(key_id, md5string, auth_type, trusted_key, authentication):

 def remove_ntp_auth_key(key_id, md5string, auth_type, trusted_key, authentication):
    auth_remove_cmds = []
-    auth_type_num = auth_type_to_num(auth_type)
-    auth_remove_cmds.append(
-        'no ntp authentication-key {0} md5 {1} {2}'.format(
-            key_id, md5string, auth_type_num))
+    if key_id:
+        auth_type_num = auth_type_to_num(auth_type)
+        auth_remove_cmds.append(
+            'no ntp authentication-key {0} md5 {1} {2}'.format(
+                key_id, md5string, auth_type_num))

-    if authentication == 'on':
+    if authentication:
        auth_remove_cmds.append(
            'no ntp authenticate')
-    elif authentication == 'off':
-        auth_remove_cmds.append(
-            'ntp authenticate')
-
    return auth_remove_cmds


 def main():
    argument_spec = dict(
-        key_id=dict(required=True, type='str'),
-        md5string=dict(required=True, type='str'),
+        key_id=dict(type='str'),
+        md5string=dict(type='str'),
        auth_type=dict(choices=['text', 'encrypt'], default='text'),
        trusted_key=dict(choices=['true', 'false'], default='false'),
        authentication=dict(choices=['on', 'off']),
@ -270,6 +265,10 @@ def main():
    authentication = module.params['authentication']
    state = module.params['state']

+    if key_id:
+        if not trusted_key and not md5string:
+            module.fail_json(msg='trusted_key or md5string MUST be specified')
+
    args = dict(key_id=key_id, md5string=md5string,
                auth_type=auth_type, trusted_key=trusted_key,
                authentication=authentication)
@ -286,18 +285,20 @@ def main():
    if state == 'present':
        if delta:
            command = set_ntp_auth_key(
-                key_id, md5string, auth_type, trusted_key, delta.get('authentication'))
+                key_id, md5string, delta.get('auth_type'),
+                delta.get('trusted_key'), delta.get('authentication'))
            if command:
                commands.append(command)
    elif state == 'absent':
-        if existing:
-            auth_toggle = None
-            if authentication == existing.get('authentication'):
-                auth_toggle = authentication
-            command = remove_ntp_auth_key(
-                key_id, md5string, auth_type, trusted_key, auth_toggle)
-            if command:
-                commands.append(command)
+        auth_toggle = None
+        if existing.get('authentication') == 'on':
+            auth_toggle = True
+        if not existing.get('key_id'):
+            key_id = None
+        command = remove_ntp_auth_key(
+            key_id, md5string, auth_type, trusted_key, auth_toggle)
+        if command:
+            commands.append(command)

    cmds = flatten_list(commands)
    if cmds:
--- a/lib/ansible/modules/network/nxos/nxos_ntp_options.py
+++ b/lib/ansible/modules/network/nxos/nxos_ntp_options.py
@ -34,12 +34,8 @@ author:
    - Jason Edelman (@jedelman8)
 notes:
    - Tested against NXOSv 7.3.(0)D1(1) on VIRL
-    - At least one of C(master) or C(logging) params must be supplied.
-    - When C(state=absent), boolean parameters are flipped,
-      e.g. C(master=true) will disable the authoritative server.
-    - When C(state=absent) and C(master=true), the stratum will be removed as well.
-    - When C(state=absent) and C(master=false), the stratum will be configured
-      to its default value, 8.
+    - When C(state=absent), master and logging will be set to False and
+      stratum will be removed as well
 options:
    master:
        description:
@ -82,7 +78,7 @@ updates:
    description: command sent to the device
    returned: always
    type: list
-    sample: ["no ntp logging", "ntp master 11"]
+    sample: ["no ntp logging", "ntp master 12"]
 '''
 import re

@ -92,20 +88,20 @@ from ansible.module_utils.basic import AnsibleModule


 def get_current(module):
-    cmd = ('show running-config', 'show ntp logging')
+    cmd = ('show running-config | inc ntp')

-    output = run_commands(module, ({'command': cmd[0], 'output': 'text'},
-                                   {'command': cmd[1], 'output': 'text'}))
+    master = False
+    logging = False
+    stratum = None

-    match = re.search(r"^ntp master(?: (\d+))", output[0], re.M)
-    if match:
-        master = True
-        stratum = match.group(1)
-    else:
-        master = False
-        stratum = None
+    output = run_commands(module, ({'command': cmd, 'output': 'text'}))[0]

-    logging = 'enabled' in output[1].lower()
+    if output:
+        match = re.search(r"^ntp master(?: (\d+))", output, re.M)
+        if match:
+            master = True
+            stratum = match.group(1)
+        logging = 'ntp logging' in output.lower()

    return {'master': master, 'stratum': stratum, 'logging': logging}

@ -113,7 +109,7 @@ def get_current(module):
 def main():
    argument_spec = dict(
        master=dict(required=False, type='bool'),
-        stratum=dict(required=False, type='str', default='8'),
+        stratum=dict(required=False, type='str'),
        logging=dict(required=False, type='bool'),
        state=dict(choices=['absent', 'present'], default='present'),
    )
@ -131,15 +127,10 @@ def main():
    logging = module.params['logging']
    state = module.params['state']

-    if stratum:
-        try:
-            stratum_int = int(stratum)
-            if stratum_int < 1 or stratum_int > 15:
-                raise ValueError
-        except ValueError:
-            module.fail_json(msg='stratum must be an integer between 1 and 15')
+    if stratum and master is False:
+        if stratum != 8:
+            module.fail_json(msg='master MUST be True when stratum is changed')

-    desired = {'master': master, 'stratum': stratum, 'logging': logging}
    current = get_current(module)

    result = {'changed': False}
@ -153,19 +144,17 @@ def main():
            commands.append('no ntp logging')

    elif state == 'present':
-        if desired['master'] and desired['master'] != current['master']:
-            if desired['stratum']:
-                commands.append('ntp master %s' % stratum)
-            else:
-                commands.append('ntp master')
-        elif desired['stratum'] and desired['stratum'] != current['stratum']:
+        if master and not current['master']:
+            commands.append('ntp master')
+        elif master is False and current['master']:
+            commands.append('no ntp master')
+        if stratum and stratum != current['stratum']:
            commands.append('ntp master %s' % stratum)

-        if desired['logging'] and desired['logging'] != current['logging']:
-            if desired['logging']:
-                commands.append('ntp logging')
-            else:
-                commands.append('no ntp logging')
+        if logging and not current['logging']:
+            commands.append('ntp logging')
+        elif logging is False and current['logging']:
+            commands.append('no ntp logging')

    result['commands'] = commands
    result['updates'] = commands
--- a/test/integration/targets/nxos_ntp_auth/tests/common/sanity.yaml
+++ b/test/integration/targets/nxos_ntp_auth/tests/common/sanity.yaml
@ -17,9 +17,7 @@
    nxos_ntp_auth: &configure_text
      key_id: 32
      md5string: hello
-      auth_type: text
-      trusted_key: true
-      authentication: on
+      authentication: off
      state: present
      provider: "{{ connection }}"      
    register: result
@ -28,21 +26,11 @@
      that:
        - "result.changed == true"

-  - name: "Check Idempotence - Configure text ntp authentication"
-    nxos_ntp_auth: *configure_text
-    register: result
-
-  - assert: &false
-      that:
-        - "result.changed == false"
-
  - name: Remove text ntp authentication
    nxos_ntp_auth: &remove_text
      key_id: 32
      md5string: hello
-      auth_type: text
-      trusted_key: true
-      authentication: on
+      authentication: off
      state: absent
      provider: "{{ connection }}"      
    register: result
@ -54,8 +42,6 @@
      key_id: 32
      md5string: hello
      auth_type: encrypt
-      trusted_key: true
-      authentication: on
      state: present
      provider: "{{ connection }}"      
    register: result
@ -66,6 +52,70 @@
    nxos_ntp_auth: *configure_encrypt
    register: result

+  - assert: &false
+      that:
+        - "result.changed == false"
+
+  - name: Turn on authentication
+    nxos_ntp_auth: &authon
+      authentication: on
+      state: present
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Turn on authentication"
+    nxos_ntp_auth: *authon
+    register: result
+
+  - assert: *false
+
+  - name: Turn off authentication
+    nxos_ntp_auth: &authoff
+      authentication: off
+      state: present
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Turn off authentication"
+    nxos_ntp_auth: *authoff
+    register: result
+
+  - assert: *false
+
+  - name: Add trusted key
+    nxos_ntp_auth: &tkey
+      key_id: 32
+      trusted_key: true
+      state: present
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Add trusted key"
+    nxos_ntp_auth: *tkey
+    register: result
+
+  - assert: *false
+
+  - name: Remove trusted key
+    nxos_ntp_auth: &rtkey
+      key_id: 32
+      trusted_key: false
+      state: present
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Remove trusted key"
+    nxos_ntp_auth: *rtkey
+    register: result
+
  - assert: *false

  - name: Remove encrypt ntp authentication
@ -73,7 +123,6 @@
      key_id: 32
      md5string: hello
      auth_type: encrypt
-      trusted_key: true
      authentication: on
      state: absent
      provider: "{{ connection }}"
@ -81,6 +130,12 @@

  - assert: *true

+  - name: "Check Idempotence - Remove encrypt ntp authentication"
+    nxos_ntp_auth: *remove_encrypt
+    register: result
+
+  - assert: *false
+
  always:
  - name: Cleanup ntp auth config
    nxos_ntp_auth: *setup
--- a/test/integration/targets/nxos_ntp_options/tests/common/sanity.yaml
+++ b/test/integration/targets/nxos_ntp_options/tests/common/sanity.yaml
@ -31,20 +31,9 @@
      that:
        - "result.changed == false"

-  - name: Remove ntp with master and default stratum
-    nxos_ntp_options: &remove_master_default_stratum
-      logging: true
-      master: true
-      state: absent
-      provider: "{{ connection }}"      
-    register: result
-
-  - assert: *true
-
  - name: Configure ntp with master and non-default stratum
    nxos_ntp_options: &configure_master_non_default_stratum
      master: true
-      logging: true
      stratum: 10
      state: present
      provider: "{{ connection }}"      
@ -58,16 +47,57 @@

  - assert: *false

-  - name: Remove ntp with master and non-default stratum
-    nxos_ntp_options: &remove_master_non_default_stratum
-      logging: true
+  - name: Configure ntp with master and no logging
+    nxos_ntp_options: &configure_no_log
      master: true
-      state: absent
+      stratum: 10
+      logging: false
+      state: present
      provider: "{{ connection }}"
    register: result

  - assert: *true

+  - name: "Check Idempotence - Configure ntp with master and no logging"
+    nxos_ntp_options: *configure_no_log
+    register: result
+
+  - assert: *false
+
+  - name: Configure ntp with logging and no master
+    nxos_ntp_options: &configure_no_master
+      master: false
+      logging: true
+      state: present
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Configure ntp with logging and no master"
+    nxos_ntp_options: *configure_no_master
+    register: result
+
+  - assert: *false
+
+  - name: "Configure ntp with master and non-default stratum again"
+    nxos_ntp_options: *configure_master_non_default_stratum
+    register: result
+
+  - assert: *true
+
+  - name: Remove ntp options
+    nxos_ntp_options: *default
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Remove"
+    nxos_ntp_options: *default
+    register: result
+
+  - assert: *false
+
  always:
  - name: Cleanup ntp config
    nxos_ntp_options: *default