From 7016b3b9ca64b61e7cbac62a68d9fac97c5feab9 Mon Sep 17 00:00:00 2001 From: Nathaniel Case Date: Mon, 5 Mar 2018 09:28:37 -0500 Subject: [PATCH] ios_static_route idempotence fix (#35912) * Remove default admin_distance and fix the idempotence thereof Fixes #33290 * Fix tests and use yaml anchors to shorten tests * Add test for undefined admin_distance * Read config from `show run` if `show ip static route` fails * Restore flags to ios.get_config & use get_config where appropriate --- lib/ansible/module_utils/network/ios/ios.py | 12 +- .../modules/network/ios/ios_static_route.py | 114 ++++++++++++------ lib/ansible/plugins/cliconf/ios.py | 3 +- .../ios_static_route/tests/cli/basic.yaml | 65 +++++----- .../tests/cli/net_static_route.yaml | 14 +-- 5 files changed, 121 insertions(+), 87 deletions(-) diff --git a/lib/ansible/module_utils/network/ios/ios.py b/lib/ansible/module_utils/network/ios/ios.py index 86f1d371f4..e609528f60 100644 --- a/lib/ansible/module_utils/network/ios/ios.py +++ b/lib/ansible/module_utils/network/ios/ios.py @@ -108,15 +108,15 @@ def get_defaults_flag(module): def get_config(module, flags=None): - global _DEVICE_CONFIGS + flag_str = ' '.join(to_list(flags)) - if _DEVICE_CONFIGS != {}: - return _DEVICE_CONFIGS - else: + try: + return _DEVICE_CONFIGS[flag_str] + except KeyError: connection = get_connection(module) - out = connection.get_config() + out = connection.get_config(flags=flags) cfg = to_text(out, errors='surrogate_then_replace').strip() - _DEVICE_CONFIGS = cfg + _DEVICE_CONFIGS[flag_str] = cfg return cfg diff --git a/lib/ansible/modules/network/ios/ios_static_route.py b/lib/ansible/modules/network/ios/ios_static_route.py index d26bcd843f..3bd44cc428 100644 --- a/lib/ansible/modules/network/ios/ios_static_route.py +++ b/lib/ansible/modules/network/ios/ios_static_route.py @@ -50,7 +50,6 @@ options: admin_distance: description: - Admin distance of the static route. - default: 1 aggregate: description: List of static route definitions. state: @@ -98,14 +97,14 @@ commands: - ip route 192.168.2.0 255.255.255.0 10.0.0.1 """ from copy import deepcopy +import re from ansible.module_utils._text import to_text from ansible.module_utils.basic import AnsibleModule -from ansible.module_utils.connection import exec_command +from ansible.module_utils.connection import ConnectionError from ansible.module_utils.network.common.utils import remove_default_spec -from ansible.module_utils.network.ios.ios import load_config, run_commands +from ansible.module_utils.network.ios.ios import get_config, load_config, run_commands from ansible.module_utils.network.ios.ios import ios_argument_spec, check_args -import re try: from ipaddress import ip_network @@ -114,23 +113,39 @@ except ImportError: HAS_IPADDRESS = False -def map_obj_to_commands(updates, module): +def map_obj_to_commands(want, have, module): commands = list() - want, have = updates for w in want: + # Try to match an existing config with the desired config + for h in have: + for key in ['prefix', 'mask', 'next_hop']: + # If any key doesn't match, skip to the next set + if w[key] != h[key]: + break + # If all keys match, don't execute final else + else: + break + # If no matches found, clear `h` + else: + h = None + prefix = w['prefix'] mask = w['mask'] next_hop = w['next_hop'] - admin_distance = w['admin_distance'] + admin_distance = w.get('admin_distance') + if not admin_distance and h: + w['admin_distance'] = admin_distance = h['admin_distance'] state = w['state'] del w['state'] if state == 'absent' and w in have: commands.append('no ip route %s %s %s' % (prefix, mask, next_hop)) elif state == 'present' and w not in have: - commands.append('ip route %s %s %s %s' % (prefix, mask, next_hop, - admin_distance)) + if admin_distance: + commands.append('ip route %s %s %s %s' % (prefix, mask, next_hop, admin_distance)) + else: + commands.append('ip route %s %s %s' % (prefix, mask, next_hop)) return commands @@ -138,55 +153,82 @@ def map_obj_to_commands(updates, module): def map_config_to_obj(module): obj = [] - rc, out, err = exec_command(module, 'show ip static route') - match = re.search(r'.*Static local RIB for default\s*(.*)$', out, re.DOTALL) + try: + out = run_commands(module, 'show ip static route')[0] + match = re.search(r'.*Static local RIB for default\s*(.*)$', out, re.DOTALL) - if match and match.group(1): - for r in match.group(1).splitlines(): - splitted_line = r.split() + if match and match.group(1): + for r in match.group(1).splitlines(): + splitted_line = r.split() - code = splitted_line[0] + code = splitted_line[0] - if code != 'M': + if code != 'M': + continue + + cidr = ip_network(to_text(splitted_line[1])) + prefix = str(cidr.network_address) + mask = str(cidr.netmask) + next_hop = splitted_line[4] + admin_distance = splitted_line[2][1] + + obj.append({ + 'prefix': prefix, 'mask': mask, 'next_hop': next_hop, + 'admin_distance': admin_distance + }) + + except ConnectionError: + out = get_config(module, flags='| include ip route') + + for line in out.splitlines(): + splitted_line = line.split() + if len(splitted_line) not in (5, 6): continue - cidr = ip_network(to_text(splitted_line[1])) - prefix = str(cidr.network_address) - mask = str(cidr.netmask) + prefix = splitted_line[2] + mask = splitted_line[3] next_hop = splitted_line[4] - admin_distance = splitted_line[2][1] + if len(splitted_line) == 6: + admin_distance = splitted_line[5] + else: + admin_distance = '1' - obj.append({'prefix': prefix, 'mask': mask, - 'next_hop': next_hop, - 'admin_distance': admin_distance}) + obj.append({ + 'prefix': prefix, 'mask': mask, 'next_hop': next_hop, + 'admin_distance': admin_distance + }) return obj def map_params_to_obj(module, required_together=None): + keys = ['prefix', 'mask', 'next_hop', 'admin_distance', 'state'] obj = [] aggregate = module.params.get('aggregate') if aggregate: for item in aggregate: - for key in item: - if item.get(key) is None: - item[key] = module.params[key] + route = item.copy() + for key in keys: + if route.get(key) is None: + route[key] = module.params.get(key) - module._check_required_together(required_together, item) - d = item.copy() - d['admin_distance'] = str(module.params['admin_distance']) - - obj.append(d) + module._check_required_together(required_together, route) + obj.append(route) else: + module._check_required_together(required_together, module.params) obj.append({ 'prefix': module.params['prefix'].strip(), 'mask': module.params['mask'].strip(), 'next_hop': module.params['next_hop'].strip(), - 'admin_distance': str(module.params['admin_distance']), - 'state': module.params['state'] + 'admin_distance': module.params.get('admin_distance'), + 'state': module.params['state'], }) + for route in obj: + if route['admin_distance']: + route['admin_distance'] = str(route['admin_distance']) + return obj @@ -197,7 +239,7 @@ def main(): prefix=dict(type='str'), mask=dict(type='str'), next_hop=dict(type='str'), - admin_distance=dict(default=1, type='int'), + admin_distance=dict(type='int'), state=dict(default='present', choices=['present', 'absent']) ) @@ -220,7 +262,6 @@ def main(): module = AnsibleModule(argument_spec=argument_spec, required_one_of=required_one_of, - required_together=required_together, mutually_exclusive=mutually_exclusive, supports_check_mode=True) @@ -236,7 +277,7 @@ def main(): want = map_params_to_obj(module, required_together=required_together) have = map_config_to_obj(module) - commands = map_obj_to_commands((want, have), module) + commands = map_obj_to_commands(want, have, module) result['commands'] = commands if commands: @@ -247,5 +288,6 @@ def main(): module.exit_json(**result) + if __name__ == '__main__': main() diff --git a/lib/ansible/plugins/cliconf/ios.py b/lib/ansible/plugins/cliconf/ios.py index f8b3deb9b9..d0009924c6 100644 --- a/lib/ansible/plugins/cliconf/ios.py +++ b/lib/ansible/plugins/cliconf/ios.py @@ -61,8 +61,7 @@ class Cliconf(CliconfBase): else: cmd = 'show startup-config' - flags = [] if flags is None else flags - cmd += ' '.join(flags) + cmd += ' '.join(to_list(flags)) cmd = cmd.strip() return self.send_command(cmd) diff --git a/test/integration/targets/ios_static_route/tests/cli/basic.yaml b/test/integration/targets/ios_static_route/tests/cli/basic.yaml index 0378bc6765..c58a0e3408 100644 --- a/test/integration/targets/ios_static_route/tests/cli/basic.yaml +++ b/test/integration/targets/ios_static_route/tests/cli/basic.yaml @@ -1,15 +1,17 @@ --- - debug: msg="START ios cli/ios_static_route.yaml on connection={{ ansible_connection }}" -- name: delete static route - setup - net_static_route: - prefix: 172.16.31.0 +- name: Clear all static routes + ios_static_route: &delete_all + aggregate: + - { prefix: 172.16.31.0 } + - { prefix: 172.16.32.0 } + - { prefix: 172.16.33.0 } + - { prefix: 172.16.34.0 } mask: 255.255.255.0 next_hop: 10.0.0.8 - admin_distance: 1 state: absent provider: "{{ cli }}" - register: result - name: create static route ios_static_route: @@ -23,13 +25,14 @@ - assert: that: - 'result.changed == true' - - 'result.commands == ["ip route 172.16.31.0 255.255.255.0 10.0.0.8 1"]' + - 'result.commands == ["ip route 172.16.31.0 255.255.255.0 10.0.0.8"]' -- name: create static route again (idempotent) +- name: Verify idempotence with default admin_distance ios_static_route: prefix: 172.16.31.0 mask: 255.255.255.0 next_hop: 10.0.0.8 + admin_distance: 1 state: present provider: "{{ cli }}" register: result @@ -39,7 +42,7 @@ - 'result.changed == false' - name: modify admin distance of static route - ios_static_route: + ios_static_route: &admin2 prefix: 172.16.31.0 mask: 255.255.255.0 next_hop: 10.0.0.8 @@ -54,11 +57,18 @@ - 'result.commands == ["ip route 172.16.31.0 255.255.255.0 10.0.0.8 2"]' - name: modify admin distance of static route again (idempotent) + ios_static_route: *admin2 + register: result + +- assert: + that: + - 'result.changed == false' + +- name: Verify idempotence with unspecified admin_distance ios_static_route: prefix: 172.16.31.0 mask: 255.255.255.0 next_hop: 10.0.0.8 - admin_distance: 2 state: present provider: "{{ cli }}" register: result @@ -68,11 +78,10 @@ - 'result.changed == false' - name: delete static route - ios_static_route: + ios_static_route: &delete prefix: 172.16.31.0 mask: 255.255.255.0 next_hop: 10.0.0.8 - admin_distance: 2 state: absent provider: "{{ cli }}" register: result @@ -83,13 +92,7 @@ - 'result.commands == ["no ip route 172.16.31.0 255.255.255.0 10.0.0.8"]' - name: delete static route again (idempotent) - ios_static_route: - prefix: 172.16.31.0 - mask: 255.255.255.0 - next_hop: 10.0.0.8 - admin_distance: 2 - state: absent - provider: "{{ cli }}" + ios_static_route: *delete register: result - assert: @@ -99,8 +102,10 @@ - name: Add static route aggregates ios_static_route: aggregate: - - { prefix: 172.16.32.0, mask: 255.255.255.0, next_hop: 10.0.0.8 } - - { prefix: 172.16.33.0, mask: 255.255.255.0, next_hop: 10.0.0.8 } + - { prefix: 172.16.32.0 } + - { prefix: 172.16.33.0 } + mask: 255.255.255.0 + next_hop: 10.0.0.8 state: present provider: "{{ cli }}" register: result @@ -108,14 +113,16 @@ - assert: that: - 'result.changed == true' - - 'result.commands == ["ip route 172.16.32.0 255.255.255.0 10.0.0.8 1", "ip route 172.16.33.0 255.255.255.0 10.0.0.8 1"]' + - 'result.commands == ["ip route 172.16.32.0 255.255.255.0 10.0.0.8", "ip route 172.16.33.0 255.255.255.0 10.0.0.8"]' - name: Add and remove static route aggregates with overrides ios_static_route: aggregate: - - { prefix: 172.16.32.0, mask: 255.255.255.0, next_hop: 10.0.0.8 } - - { prefix: 172.16.33.0, mask: 255.255.255.0, next_hop: 10.0.0.8, state: absent } - - { prefix: 172.16.34.0, mask: 255.255.255.0, next_hop: 10.0.0.8 } + - { prefix: 172.16.32.0 } + - { prefix: 172.16.33.0, state: absent } + - { prefix: 172.16.34.0 } + mask: 255.255.255.0 + next_hop: 10.0.0.8 state: present provider: "{{ cli }}" register: result @@ -123,16 +130,10 @@ - assert: that: - 'result.changed == true' - - 'result.commands == ["no ip route 172.16.33.0 255.255.255.0 10.0.0.8", "ip route 172.16.34.0 255.255.255.0 10.0.0.8 1"]' + - 'result.commands == ["no ip route 172.16.33.0 255.255.255.0 10.0.0.8", "ip route 172.16.34.0 255.255.255.0 10.0.0.8"]' - name: Remove static route aggregates - ios_static_route: - aggregate: - - { prefix: 172.16.32.0, mask: 255.255.255.0, next_hop: 10.0.0.8 } - - { prefix: 172.16.33.0, mask: 255.255.255.0, next_hop: 10.0.0.8 } - - { prefix: 172.16.34.0, mask: 255.255.255.0, next_hop: 10.0.0.8 } - state: absent - provider: "{{ cli }}" + ios_static_route: *delete_all register: result - assert: diff --git a/test/integration/targets/ios_static_route/tests/cli/net_static_route.yaml b/test/integration/targets/ios_static_route/tests/cli/net_static_route.yaml index 9a8d351172..3e7c99d4c6 100644 --- a/test/integration/targets/ios_static_route/tests/cli/net_static_route.yaml +++ b/test/integration/targets/ios_static_route/tests/cli/net_static_route.yaml @@ -5,20 +5,19 @@ # implementation module and module run is successful. - name: delete static route - setup - net_static_route: + net_static_route: &delete prefix: 172.16.31.0 mask: 255.255.255.0 next_hop: 10.0.0.8 - admin_distance: 1 state: absent provider: "{{ cli }}" - register: result - name: create static route using platform agnostic module net_static_route: prefix: 172.16.31.0 mask: 255.255.255.0 next_hop: 10.0.0.8 + admin_distance: 1 state: present provider: "{{ cli }}" register: result @@ -29,13 +28,6 @@ - 'result.commands == ["ip route 172.16.31.0 255.255.255.0 10.0.0.8 1"]' - name: delete static route - teardown - net_static_route: - prefix: 172.16.31.0 - mask: 255.255.255.0 - next_hop: 10.0.0.8 - admin_distance: 2 - state: absent - provider: "{{ cli }}" - register: result + net_static_route: *delete - debug: msg="END ios cli/net_static_route.yaml on connection={{ ansible_connection }}"