From d59eb9edab66a650a88493f5a70cf4948892b0fc Mon Sep 17 00:00:00 2001 From: Kevin Breit Date: Thu, 16 May 2019 11:10:32 -0500 Subject: [PATCH] meraki_switchport - Improve reliability (#54275) * Rewrite much of the execution of meraki_switchport - Previous versions had problems with idempotency and allowed_vlans * Modified payload creation - Parameter map is used - propsed is created using .copy() - Much cleaner this way * Add whitespace for lint * Add bugfix snippet for changelog --- ...ki_switchport_improve_vlan_reliability.yml | 2 + .../network/meraki/meraki_switchport.py | 118 ++++++++---------- .../targets/meraki_switchport/tasks/main.yml | 48 ++++++- 3 files changed, 98 insertions(+), 70 deletions(-) create mode 100644 changelogs/fragments/53601-meraki_switchport_improve_vlan_reliability.yml diff --git a/changelogs/fragments/53601-meraki_switchport_improve_vlan_reliability.yml b/changelogs/fragments/53601-meraki_switchport_improve_vlan_reliability.yml new file mode 100644 index 0000000000..a3bc6321b3 --- /dev/null +++ b/changelogs/fragments/53601-meraki_switchport_improve_vlan_reliability.yml @@ -0,0 +1,2 @@ +bugfixes: +- "meraki_switchport - improve reliability with native VLAN functionality." diff --git a/lib/ansible/modules/network/meraki/meraki_switchport.py b/lib/ansible/modules/network/meraki/meraki_switchport.py index 9a7b9ae047..68f4ccc6b4 100644 --- a/lib/ansible/modules/network/meraki/meraki_switchport.py +++ b/lib/ansible/modules/network/meraki/meraki_switchport.py @@ -245,34 +245,32 @@ from ansible.module_utils.urls import fetch_url from ansible.module_utils._text import to_native from ansible.module_utils.network.meraki.meraki import MerakiModule, meraki_argument_spec -# param_map isn't used but leaving in as it is a more efficient method for implementation later -param_map = {'accessPolicyNumber': 'access_policy_number', - 'allowedVlans': 'allowed_vlans', +param_map = {'access_policy_number': 'accessPolicyNumber', + 'allowed_vlans': 'allowedVlans', 'enabled': 'enabled', - 'isolationEnabled': 'isolation_enabled', - 'linkNegotiation': 'link_negotiation', + 'isolation_enabled': 'isolationEnabled', + 'link_negotiation': 'linkNegotiation', 'name': 'name', 'number': 'number', - 'poeEnabled': 'poe_enabled', - 'rstpEnabled': 'rstp_enabled', - 'stpGuard': 'stp_guard', + 'poe_enabled': 'poeEnabled', + 'rstp_enabled': 'rstpEnabled', + 'stp_guard': 'stpGuard', 'tags': 'tags', 'type': 'type', 'vlan': 'vlan', - 'voiceVlan': 'voice_vlan', + 'voice_vlan': 'voiceVlan', } -def list_to_csv(items): - csv = '' - count = 0 - for item in items: - if count == len(items) - 1: - csv = csv + str(item) - else: - csv = csv + str(item) + ',' - count += 1 - return csv +def sort_vlans(meraki, vlans): + converted = set() + for vlan in vlans: + converted.add(int(vlan)) + vlans_sorted = sorted(converted) + vlans_str = [] + for vlan in vlans_sorted: + vlans_str.append(str(vlan)) + return ','.join(vlans_str) def main(): @@ -309,18 +307,13 @@ def main(): meraki = MerakiModule(module, function='switchport') meraki.params['follow_redirects'] = 'all' - # argument checks - # if meraki.params['type'] == 'access': - # if meraki.params['allowed_vlans']: - # meraki.fail_json(msg='allowed_vlans is not allowed on access ports') if meraki.params['type'] == 'trunk': if not meraki.params['allowed_vlans']: meraki.params['allowed_vlans'] = ['all'] # Backdoor way to set default without conflicting on access - # TODO: Add support for serial in the construct_path() method - query_urls = {'switchport': '/devices/serial/switchPorts'} - query_url = {'switchport': '/devices/serial/switchPorts/'} - update_url = {'switchport': '/devices/serial/switchPorts/'} + query_urls = {'switchport': '/devices/{serial}/switchPorts'} + query_url = {'switchport': '/devices/{serial}/switchPorts/{number}'} + update_url = {'switchport': '/devices/{serial}/switchPorts/{number}'} meraki.url_catalog['get_all'].update(query_urls) meraki.url_catalog['get_one'].update(query_url) @@ -341,64 +334,53 @@ def main(): # part where your module will do what it needs to do) if meraki.params['state'] == 'query': if meraki.params['number']: - path = meraki.construct_path('get_one') + meraki.params['number'] - path = path.replace('serial', meraki.params['serial']) + path = meraki.construct_path('get_one', custom={'serial': meraki.params['serial'], + 'number': meraki.params['number'], + }) response = meraki.request(path, method='GET') meraki.result['data'] = response else: - path = meraki.construct_path('get_all') - path = path.replace('serial', meraki.params['serial']) + path = meraki.construct_path('get_all', custom={'serial': meraki.params['serial']}) response = meraki.request(path, method='GET') meraki.result['data'] = response elif meraki.params['state'] == 'present': payload = dict() - proposed = dict() - # if meraki.params['type'] == 'access': - payload['name'] = meraki.params['name'] - payload['tags'] = meraki.params['tags'] - payload['enabled'] = meraki.params['enabled'] - payload['poeEnabled'] = meraki.params['poe_enabled'] - payload['type'] = meraki.params['type'] - payload['vlan'] = meraki.params['vlan'] - payload['voiceVlan'] = meraki.params['voice_vlan'] - payload['isolationEnabled'] = meraki.params['isolation_enabled'] - payload['rstpEnabled'] = meraki.params['rstp_enabled'] - payload['stpGuard'] = meraki.params['stp_guard'] - payload['accessPolicyNumber'] = meraki.params['access_policy_number'] - payload['linkNegotiation'] = meraki.params['link_negotiation'] - payload['allowedVlans'] = list_to_csv(meraki.params['allowed_vlans']) - proposed['name'] = meraki.params['name'] - proposed['tags'] = meraki.params['tags'] - proposed['enabled'] = meraki.params['enabled'] - proposed['poeEnabled'] = meraki.params['poe_enabled'] - proposed['type'] = meraki.params['type'] - proposed['vlan'] = meraki.params['vlan'] - proposed['voiceVlan'] = meraki.params['voice_vlan'] - proposed['isolationEnabled'] = meraki.params['isolation_enabled'] - proposed['rstpEnabled'] = meraki.params['rstp_enabled'] - proposed['stpGuard'] = meraki.params['stp_guard'] - proposed['accessPolicyNumber'] = meraki.params['access_policy_number'] - proposed['linkNegotiation'] = meraki.params['link_negotiation'] - proposed['allowedVlans'] = list_to_csv(meraki.params['allowed_vlans']) + for k, v in meraki.params.items(): + try: + payload[param_map[k]] = v + except KeyError: + pass + + allowed = set() # Use a set to remove duplicate items + if meraki.params['allowed_vlans'][0] == 'all': + allowed.add('all') + else: + for vlan in meraki.params['allowed_vlans']: + allowed.add(str(vlan)) + if meraki.params['vlan'] is not None: + allowed.add(str(meraki.params['vlan'])) + if len(allowed) > 1: # Convert from list to comma separated + payload['allowedVlans'] = sort_vlans(meraki, allowed) + else: + payload['allowedVlans'] = next(iter(allowed)) # Exceptions need to be made for idempotency check based on how Meraki returns - if meraki.params['type'] == 'trunk': - if meraki.params['vlan'] and meraki.params['allowed_vlans'] != 'all': - proposed['allowedVlans'] = str(meraki.params['vlan']) + ',' + proposed['allowedVlans'] - else: + if meraki.params['type'] == 'access': if not meraki.params['vlan']: # VLAN needs to be specified in access ports, but can't default to it payload['vlan'] = 1 - proposed['vlan'] = 1 - query_path = meraki.construct_path('get_one') + meraki.params['number'] - query_path = query_path.replace('serial', meraki.params['serial']) + proposed = payload.copy() + query_path = meraki.construct_path('get_one', custom={'serial': meraki.params['serial'], + 'number': meraki.params['number'], + }) original = meraki.request(query_path, method='GET') if meraki.params['type'] == 'trunk': proposed['voiceVlan'] = original['voiceVlan'] # API shouldn't include voice VLAN on a trunk port if meraki.is_update_required(original, proposed, optional_ignore=('number')): - path = meraki.construct_path('update') + meraki.params['number'] - path = path.replace('serial', meraki.params['serial']) + path = meraki.construct_path('update', custom={'serial': meraki.params['serial'], + 'number': meraki.params['number'], + }) response = meraki.request(path, method='PUT', payload=json.dumps(payload)) meraki.result['data'] = response meraki.result['changed'] = True diff --git a/test/integration/targets/meraki_switchport/tasks/main.yml b/test/integration/targets/meraki_switchport/tasks/main.yml index 74de61c129..f7fba5043a 100644 --- a/test/integration/targets/meraki_switchport/tasks/main.yml +++ b/test/integration/targets/meraki_switchport/tasks/main.yml @@ -24,7 +24,7 @@ auth_key: '{{ auth_key }}' use_https: false state: query - serial: Q2HP-2C6E- + serial: Q2HP-2C6E-GTLD output_level: debug delegate_to: localhost register: http @@ -132,6 +132,48 @@ that: - update_access_port.data.vlan == 10 +- name: Configure port as trunk + meraki_switchport: + auth_key: '{{auth_key}}' + state: present + serial: Q2HP-2C6E-GTLD + number: 8 + enabled: true + name: Test Port + type: trunk + vlan: 10 + allowed_vlans: 10, 100, 200 + delegate_to: localhost + +- name: Convert trunk port to access + meraki_switchport: + auth_key: '{{auth_key}}' + state: present + serial: Q2HP-2C6E-GTLD + number: 8 + enabled: true + name: Test Port + type: access + vlan: 10 + delegate_to: localhost + +- name: Test converted port for idempotency + meraki_switchport: + auth_key: '{{auth_key}}' + state: present + serial: Q2HP-2C6E-GTLD + number: 8 + enabled: true + name: Test Port + type: access + vlan: 10 + delegate_to: localhost + register: convert_idempotent + +- assert: + that: + - convert_idempotent.changed == False + - name: Configure access port with voice VLAN meraki_switchport: auth_key: '{{auth_key}}' @@ -188,6 +230,7 @@ tags: server type: trunk allowed_vlans: all + vlan: 8 delegate_to: localhost register: update_trunk @@ -210,6 +253,7 @@ name: Server port tags: server type: trunk + vlan: 8 allowed_vlans: - 10 - 15 @@ -224,7 +268,7 @@ that: - update_trunk.data.tags == 'server' - update_trunk.data.type == 'trunk' - - update_trunk.data.allowedVlans == '10,15,20' + - update_trunk.data.allowedVlans == '8,10,15,20' - name: Configure trunk port with specific VLANs and native VLAN meraki_switchport: