nxos_acl fixes (#23915)

* Update nxos_acl

* unit tests for nxos_acl

* Remove nxos_acl from pep8-legacy

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

@@ -16,9 +16,11 @@
 # along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 #
 
-ANSIBLE_METADATA = {'metadata_version': '1.0',
-                    'status': ['preview'],
-                    'supported_by': 'community'}
+ANSIBLE_METADATA = {
+    'metadata_version': '1.0',
+    'status': ['preview'],
+    'supported_by': 'community'
+}
 
 
 DOCUMENTATION = '''
@@ -28,179 +30,178 @@ extends_documentation_fragment: nxos
 version_added: "2.2"
 short_description: Manages access list entries for ACLs.
 description:
-    - Manages access list entries for ACLs.
+  - Manages access list entries for ACLs.
 author:
-    - Jason Edelman (@jedelman8)
-    - Gabriele Gerbino (@GGabriele)
+  - Jason Edelman (@jedelman8)
+  - Gabriele Gerbino (@GGabriele)
 notes:
-    - C(state=absent) removes the ACE if it exists.
-    - C(state=delete_acl) deleted the ACL if it exists.
-    - For idempotency, use port numbers for the src/dest port
-      params like I(src_port1) and names for the well defined protocols
-      for the I(proto) param.
-    - Although this module is idempotent in that if the ace as presented in
-      the task is identical to the one on the switch, no changes will be made.
-      If there is any difference, what is in Ansible will be pushed (configured
-      options will be overridden).  This is to improve security, but at the
-      same time remember an ACE is removed, then re-added, so if there is a
-      change, the new ACE will be exactly what parameters you are sending to
-      the module.
+  - C(state=absent) removes the ACE if it exists.
+  - C(state=delete_acl) deletes the ACL if it exists.
+  - For idempotency, use port numbers for the src/dest port
+    params like I(src_port1) and names for the well defined protocols
+    for the I(proto) param.
+  - Although this module is idempotent in that if the ace as presented in
+    the task is identical to the one on the switch, no changes will be made.
+    If there is any difference, what is in Ansible will be pushed (configured
+    options will be overridden).  This is to improve security, but at the
+    same time remember an ACE is removed, then re-added, so if there is a
+    change, the new ACE will be exactly what parameters you are sending to
+    the module.
 options:
-    seq:
-        description:
-            - Sequence number of the entry (ACE).
-        required: false
-        default: null
-    name:
-        description:
-            - Case sensitive name of the access list (ACL).
-        required: true
-    action:
-        description:
-            - Action of the ACE.
-        required: false
-        default: null
-        choices: ['permit', 'deny', 'remark']
-    remark:
-        description:
-            - If action is set to remark, this is the description.
-        required: false
-        default: null
-    proto:
-        description:
-            - Port number or protocol (as supported by the switch).
-        required: false
-        default: null
-    src:
-        description:
-            - Source ip and mask using IP/MASK notation and
-              supports keyword 'any'.
-        required: false
-        default: null
-    src_port_op:
-        description:
-            - Source port operands such as eq, neq, gt, lt, range.
-        required: false
-        default: null
-        choices: ['any', 'eq', 'gt', 'lt', 'neq', 'range']
-    src_port1:
-        description:
-            - Port/protocol and also first (lower) port when using range
-              operand.
-        required: false
-        default: null
-    src_port2:
-        description:
-            - Second (end) port when using range operand.
-        required: false
-        default: null
-    dest:
-        description:
-            - Destination ip and mask using IP/MASK notation and supports the
-              keyword 'any'.
-        required: false
-        default: null
-    dest_port_op:
-        description:
-            - Destination port operands such as eq, neq, gt, lt, range.
-        required: false
-        default: null
-        choices: ['any', 'eq', 'gt', 'lt', 'neq', 'range']
-    dest_port1:
-        description:
-            - Port/protocol and also first (lower) port when using range
-              operand.
-        required: false
-        default: null
-    dest_port2:
-        description:
-            - Second (end) port when using range operand.
-        required: false
-        default: null
-    log:
-        description:
-            - Log matches against this entry.
-        required: false
-        default: null
-        choices: ['enable']
-    urg:
-        description:
-            - Match on the URG bit.
-        required: false
-        default: null
-        choices: ['enable']
-    ack:
-        description:
-            - Match on the ACK bit.
-        required: false
-        default: null
-        choices: ['enable']
-    psh:
-        description:
-            - Match on the PSH bit.
-        required: false
-        default: null
-        choices: ['enable']
-    rst:
-        description:
-            - Match on the RST bit.
-        required: false
-        default: null
-        choices: ['enable']
-    syn:
-        description:
-            - Match on the SYN bit.
-        required: false
-        default: null
-        choices: ['enable']
-    fin:
-        description:
-            - Match on the FIN bit.
-        required: false
-        default: null
-        choices: ['enable']
-    established:
-        description:
-            - Match established connections.
-        required: false
-        default: null
-        choices: ['enable']
-    fragments:
-        description:
-            - Check non-initial fragments.
-        required: false
-        default: null
-        choices: ['enable']
-    time-range:
-        description:
-            - Name of time-range to apply.
-        required: false
-        default: null
-    precedence:
-        description:
-            - Match packets with given precedence.
-        required: false
-        default: null
-        choices: ['critical', 'flash', 'flash-override', 'immediate',
-                  'internet', 'network', 'priority', 'routine']
-    dscp:
-        description:
-            - Match packets with given dscp value.
-        required: false
-        default: null
-        choices: ['af11', 'af12', 'af13', 'af21', 'af22', 'af23','af31','af32',
-                  'af33', 'af41', 'af42', 'af43', 'cs1', 'cs2', 'cs3', 'cs4',
-                  'cs5', 'cs6', 'cs7', 'default', 'ef']
-    state:
-        description:
-            - Specify desired state of the resource.
-        required: false
-        default: present
-        choices: ['present','absent','delete_acl']
+  seq:
+    description:
+      - Sequence number of the entry (ACE).
+    required: false
+    default: null
+  name:
+    description:
+      - Case sensitive name of the access list (ACL).
+    required: true
+  action:
+    description:
+      - Action of the ACE.
+    required: false
+    default: null
+    choices: ['permit', 'deny', 'remark']
+  remark:
+    description:
+      - If action is set to remark, this is the description.
+    required: false
+    default: null
+  proto:
+    description:
+      - Port number or protocol (as supported by the switch).
+    required: false
+    default: null
+  src:
+    description:
+      - Source ip and mask using IP/MASK notation and
+        supports keyword 'any'.
+    required: false
+    default: null
+  src_port_op:
+    description:
+      - Source port operands such as eq, neq, gt, lt, range.
+    required: false
+    default: null
+    choices: ['any', 'eq', 'gt', 'lt', 'neq', 'range']
+  src_port1:
+    description:
+      - Port/protocol and also first (lower) port when using range
+        operand.
+    required: false
+    default: null
+  src_port2:
+    description:
+      - Second (end) port when using range operand.
+    required: false
+    default: null
+  dest:
+    description:
+      - Destination ip and mask using IP/MASK notation and supports the
+        keyword 'any'.
+    required: false
+    default: null
+  dest_port_op:
+    description:
+      - Destination port operands such as eq, neq, gt, lt, range.
+    required: false
+    default: null
+    choices: ['any', 'eq', 'gt', 'lt', 'neq', 'range']
+  dest_port1:
+    description:
+      - Port/protocol and also first (lower) port when using range
+        operand.
+    required: false
+    default: null
+  dest_port2:
+    description:
+      - Second (end) port when using range operand.
+    required: false
+    default: null
+  log:
+    description:
+      - Log matches against this entry.
+    required: false
+    default: null
+    choices: ['enable']
+  urg:
+    description:
+      - Match on the URG bit.
+    required: false
+    default: null
+    choices: ['enable']
+  ack:
+    description:
+      - Match on the ACK bit.
+    required: false
+    default: null
+    choices: ['enable']
+  psh:
+    description:
+      - Match on the PSH bit.
+    required: false
+    default: null
+    choices: ['enable']
+  rst:
+    description:
+      - Match on the RST bit.
+    required: false
+    default: null
+    choices: ['enable']
+  syn:
+    description:
+      - Match on the SYN bit.
+    required: false
+    default: null
+    choices: ['enable']
+  fin:
+    description:
+      - Match on the FIN bit.
+    required: false
+    default: null
+    choices: ['enable']
+  established:
+    description:
+      - Match established connections.
+    required: false
+    default: null
+    choices: ['enable']
+  fragments:
+    description:
+      - Check non-initial fragments.
+    required: false
+    default: null
+    choices: ['enable']
+  time-range:
+    description:
+      - Name of time-range to apply.
+    required: false
+    default: null
+  precedence:
+    description:
+      - Match packets with given precedence.
+    required: false
+    default: null
+    choices: ['critical', 'flash', 'flash-override', 'immediate',
+              'internet', 'network', 'priority', 'routine']
+  dscp:
+    description:
+      - Match packets with given dscp value.
+    required: false
+    default: null
+    choices: ['af11', 'af12', 'af13', 'af21', 'af22', 'af23','af31','af32',
+              'af33', 'af41', 'af42', 'af43', 'cs1', 'cs2', 'cs3', 'cs4',
+              'cs5', 'cs6', 'cs7', 'default', 'ef']
+  state:
+    description:
+      - Specify desired state of the resource.
+    required: false
+    default: present
+    choices: ['present','absent','delete_acl']
 '''
 
 EXAMPLES = '''
-
 # configure ACL ANSIBLE
 - nxos_acl:
     name: ANSIBLE
@@ -214,49 +215,22 @@ EXAMPLES = '''
 '''
 
 RETURN = '''
-proposed:
-    description: k/v pairs of parameters passed into module.
-    returned: always
-    type: dict
-    sample: {"action": "permit", "dest": "any", "name": "ANSIBLE",
-            "proto": "tcp", "seq": "10", "src": "1.1.1.1/24"}
-existing:
-    description: k/v pairs of existing ACL entries.
-    returned: always
-    type: dict
-    sample: {}
-end_state:
-    description: k/v pairs of ACL entries after module execution.
-    returned: always
-    type: dict
-    sample: {"action": "permit", "dest": "any", "name": "ANSIBLE",
-            "proto": "tcp", "seq": "10", "src": "1.1.1.1/24"}
-updates:
+commands:
     description: commands sent to the device
     returned: always
     type: list
     sample: ["ip access-list ANSIBLE", "10 permit tcp 1.1.1.1/24 any"]
-changed:
-    description: check to see if a change was made on the device
-    returned: always
-    type: boolean
-    sample: true
 '''
-import re
-
 from ansible.module_utils.nxos import load_config, run_commands
 from ansible.module_utils.nxos import nxos_argument_spec, check_args
 from ansible.module_utils.basic import AnsibleModule
 
 
-def execute_show_command(command, module, command_type='cli_show'):
+def execute_show_command(command, module):
     if module.params['transport'] == 'cli':
         command += ' | json'
-        cmds = [command]
-        body = run_commands(module, cmds)
-    elif module.params['transport'] == 'nxapi':
-        cmds = [command]
-        body = run_commands(module, cmds)
+    cmds = [command]
+    body = run_commands(module, cmds)
     return body
 
 
@@ -264,7 +238,6 @@ def get_acl(module, acl_name, seq_number):
     command = 'show ip access-list'
     new_acl = []
     saveme = {}
-    seqs = []
     acl_body = {}
 
     body = execute_show_command(command, module)[0]
@@ -278,18 +251,19 @@ def get_acl(module, acl_name, seq_number):
         acl_entries = acl_body['TABLE_seqno']['ROW_seqno']
         acl_name = acl_body.get('acl_name')
     except KeyError:  # could be raised if no ACEs are configured for an ACL
-        return saveme, [{'acl': 'no_entries'}], seqs
+        return {}, [{'acl': 'no_entries'}]
 
     if isinstance(acl_entries, dict):
         acl_entries = [acl_entries]
 
     for each in acl_entries:
-        temp = collections.OrderedDict()
-        keep = {}
+        temp = {}
+        options = {}
+        remark = each.get('remark')
+
         temp['name'] = acl_name
         temp['seq'] = str(each.get('seqno'))
-        temp['options'] = {}
-        remark = each.get('remark')
+
         if remark:
             temp['remark'] = remark
             temp['action'] = 'remark'
@@ -305,7 +279,6 @@ def get_acl(module, acl_name, seq_number):
             temp['dest_port1'] = each.get('dest_port1_num')
             temp['dest_port2'] = each.get('dest_port2_num')
 
-            options = collections.OrderedDict()
             options['log'] = each.get('log')
             options['urg'] = each.get('urg')
             options['ack'] = each.get('ack')
@@ -319,27 +292,23 @@ def get_acl(module, acl_name, seq_number):
             options['fragments'] = each.get('fragments')
             options['time_range'] = each.get('timerange')
 
-            options_no_null = {}
-            for key, value in options.items():
-                if value is not None:
-                    options_no_null[key] = value
-
-            keep['options'] = options_no_null
-
+        keep = {}
         for key, value in temp.items():
             if value:
                 keep[key] = value
-        # ensure options is always in the dict
-        if keep.get('options', 'DNE') == 'DNE':
-            keep['options'] = {}
+
+        options_no_null = {}
+        for key, value in options.items():
+            if value is not None:
+                options_no_null[key] = value
+        keep['options'] = options_no_null
 
         if keep.get('seq') == seq_number:
             saveme = dict(keep)
 
-        seqs.append(str(keep.get('seq')))
         new_acl.append(keep)
 
-    return saveme, new_acl, seqs
+    return saveme, new_acl
 
 
 def _acl_operand(operand, srcp1, sprcp2):
@@ -442,18 +411,17 @@ def main():
         established=dict(required=False, choices=['enable']),
         time_range=dict(required=False),
         precedence=dict(required=False, choices=['critical', 'flash',
-                                                     'flash-override',
-                                                     'immediate', 'internet',
-                                                     'network', 'priority',
-                                                     'routine']),
+                                                 'flash-override',
+                                                 'immediate', 'internet',
+                                                 'network', 'priority',
+                                                 'routine']),
         dscp=dict(required=False, choices=['af11', 'af12', 'af13', 'af21',
-                                               'af22', 'af23', 'af31', 'af32',
-                                               'af33', 'af41', 'af42', 'af43',
-                                               'cs1', 'cs2', 'cs3', 'cs4',
-                                               'cs5', 'cs6', 'cs7', 'default',
-                                               'ef']),
-        state=dict(choices=['absent', 'present', 'delete_acl'],
-                       default='present'),
+                                           'af22', 'af23', 'af31', 'af32',
+                                           'af33', 'af41', 'af42', 'af43',
+                                           'cs1', 'cs2', 'cs3', 'cs4',
+                                           'cs5', 'cs6', 'cs7', 'default',
+                                           'ef']),
+        state=dict(choices=['absent', 'present', 'delete_acl'], default='present'),
         protocol=dict(choices=['http', 'https'], default='http'),
         host=dict(required=True),
         username=dict(type='str'),
@@ -466,11 +434,12 @@ def main():
     argument_spec.update(nxos_argument_spec)
 
     module = AnsibleModule(argument_spec=argument_spec,
-                                supports_check_mode=True)
+                           supports_check_mode=True)
 
     warnings = list()
     check_args(module, warnings)
 
+    results = dict(changed=False, warnings=warnings)
 
     state = module.params['state']
     action = module.params['action']
@@ -482,8 +451,7 @@ def main():
     seq = module.params['seq']
 
     if action == 'remark' and not remark:
-        module.fail_json(msg='when state is action, remark param is also '
-                             'required')
+        module.fail_json(msg='when state is action, remark param is also required')
 
     REQUIRED = ['seq', 'name', 'action', 'proto', 'src', 'dest']
     ABSENT = ['name', 'seq']
@@ -530,14 +498,12 @@ def main():
     existing_options = {}
 
     # getting existing existing_core=dict, acl=list, seq=list
-    existing_core, acl, seqs = get_acl(module, name, seq)
+    existing_core, acl = get_acl(module, name, seq)
     if existing_core:
         existing_options = existing_core.get('options')
         existing_core.pop('options')
 
-    end_state = acl
     commands = []
-    changed = False
     delta_core = {}
     delta_options = {}
 
@@ -545,7 +511,7 @@ def main():
         delta_core = dict(
             set(proposed_core.items()).difference(
                 existing_core.items())
-            )
+        )
         delta_options = dict(
             set(proposed_options.items()).difference(
                 existing_options.items())
@@ -569,7 +535,6 @@ def main():
         if acl[0].get('acl') != 'no_entries':
             commands.append(['no ip access-list {0}'.format(name)])
 
-    results = {}
     cmds = []
     if commands:
         preface = []
@@ -582,21 +547,14 @@ def main():
             module.exit_json(changed=True, commands=cmds)
         else:
             load_config(module, cmds)
-            changed = True
-            new_existing_core, end_state, seqs = get_acl(module, name, seq)
+            results['changed'] = True
             if 'configure' in cmds:
                 cmds.pop(0)
 
-    results['proposed'] = proposed
-    results['existing'] = existing_core
-    results['changed'] = changed
-    results['warnings'] = warnings
-    results['updates'] = cmds
-    results['end_state'] = end_state
+    results['commands'] = cmds
 
     module.exit_json(**results)
 
 
 if __name__ == '__main__':
     main()
-