From fe9e887f86dca26db5bec333913724a08c93e35b Mon Sep 17 00:00:00 2001 From: Trishna Guha Date: Fri, 16 Feb 2018 10:12:42 +0530 Subject: [PATCH] fix nxos_bgp_af issues (#36147) (#36283) * fix nxos_bgp_af issues * shippable fix * review comments * shippable error fix (cherry picked from commit 75a34f666826be988b25df41cb72bcd5d1428736) --- .../modules/network/nxos/nxos_bgp_af.py | 117 +++++---- .../targets/nxos_bgp_af/defaults/main.yaml | 3 + .../nxos_bgp_af/tests/common/sanity.yaml | 236 ++++++++++++++++-- .../modules/network/nxos/test_nxos_bgp_af.py | 2 +- 4 files changed, 280 insertions(+), 78 deletions(-) diff --git a/lib/ansible/modules/network/nxos/nxos_bgp_af.py b/lib/ansible/modules/network/nxos/nxos_bgp_af.py index 1327fe5f4d5..d055274a443 100644 --- a/lib/ansible/modules/network/nxos/nxos_bgp_af.py +++ b/lib/ansible/modules/network/nxos/nxos_bgp_af.py @@ -337,42 +337,15 @@ def get_value(arg, config, module): command_val_re = re.compile(r'(?:{0}\s)(?P.*)$'.format(command), re.M) has_command_val = command_val_re.search(config) - if arg == 'inject_map': - inject_re = r'.*inject-map\s(?P\S+)\sexist-map\s(?P\S+)-*' - - value = [] - match_inject = re.match(inject_re, config, re.DOTALL) - if match_inject: - inject_group = match_inject.groupdict() - inject_map = inject_group['inject_map'] - exist_map = inject_group['exist_map'] - value.append(inject_map) - value.append(exist_map) - - inject_map_command = ('inject-map {0} exist-map {1} ' - 'copy-attributes'.format( - inject_group['inject_map'], - inject_group['exist_map'])) - - inject_re = re.compile(r'\s+{0}\s*$'.format(inject_map_command), re.M) - if inject_re.search(config): - value.append('copy_attributes') - - elif arg == 'networks': + if arg in ['networks', 'redistribute', 'inject_map']: value = [] - for network in command_val_re.findall(config): - value.append(network.split()) - - elif arg == 'redistribute': - value = [] - if has_command_val: - value = has_command_val.group('value').split() - - if value: - if len(value) == 3: - value.pop(1) - elif len(value) == 4: - value = ['{0} {1}'.format(value[0], value[1]), value[3]] + for ele in command_val_re.findall(config): + tl = ele.split() + if 'exist-map' in tl: + tl.remove('exist-map') + elif 'route-map' in tl: + tl.remove('route-map') + value.append(tl) elif command == 'distance': distance_re = r'.*distance\s(?P\w+)\s(?P\w+)\s(?P\w+)' @@ -408,7 +381,9 @@ def get_value(arg, config, module): value = dampening_group['suppress'] elif arg == 'dampening_max_suppress_time': value = dampening_group['max_suppress'] - + else: + if arg == 'dampening_state': + value = True if 'dampening' in config else False elif arg == 'table_map_filter': tmf_regex = re.compile(r'\s+table-map.*filter$', re.M) value = False @@ -464,7 +439,14 @@ def get_existing(module, args, warnings): if config: for arg in args: if arg not in ['asn', 'afi', 'safi', 'vrf']: - existing[arg] = get_value(arg, config, module) + gv = get_value(arg, config, module) + if gv: + existing[arg] = gv + else: + if arg != 'client_to_client' and arg in PARAM_TO_DEFAULT_KEYMAP.keys(): + existing[arg] = PARAM_TO_DEFAULT_KEYMAP.get(arg) + else: + existing[arg] = gv existing['asn'] = existing_asn existing['afi'] = module.params['afi'] @@ -540,6 +522,11 @@ def default_existing(existing_value, key, value): elif len(maps) == 3: commands.append('no inject-map {0} exist-map {1} ' 'copy-attributes'.format(maps[0], maps[1])) + + elif key == 'redistribute': + for maps in existing_value: + commands.append('no redistribute {0} route-map {1}'.format(maps[0], maps[1])) + else: commands.append('no {0} {1}'.format(key, existing_value)) return commands @@ -557,6 +544,13 @@ def get_network_command(existing, key, value): elif len(inet) == 2: command = '{0} {1} route-map {2}'.format(key, inet[0], inet[1]) commands.append(command) + for enet in existing_networks: + if enet not in value: + if len(enet) == 1: + command = 'no {0} {1}'.format(key, enet[0]) + elif len(enet) == 2: + command = 'no {0} {1} route-map {2}'.format(key, enet[0], enet[1]) + commands.append(command) return commands @@ -575,21 +569,33 @@ def get_inject_map_command(existing, key, value): 'copy-attributes'.format(maps[0], maps[1])) commands.append(command) + for emaps in existing_maps: + if emaps not in value: + if len(emaps) == 2: + command = ('no inject-map {0} exist-map {1}'.format( + emaps[0], emaps[1])) + elif len(emaps) == 3: + command = ('no inject-map {0} exist-map {1} ' + 'copy-attributes'.format(emaps[0], + emaps[1])) + commands.append(command) return commands def get_redistribute_command(existing, key, value): commands = [] + existing_rules = existing.get('redistribute', []) for rule in value: - if rule[1] == 'default': - existing_rule = existing.get('redistribute', []) - for each_rule in existing_rule: - if rule[0] in each_rule: - command = 'no {0} {1} route-map {2}'.format( - key, each_rule[0], each_rule[1]) - commands.append(command) - else: - command = '{0} {1} route-map {2}'.format(key, rule[0], rule[1]) + if not isinstance(rule, list): + rule = [rule] + if rule not in existing_rules: + command = ('redistribute {0} route-map {1}'.format( + rule[0], rule[1])) + commands.append(command) + for erule in existing_rules: + if erule not in value: + command = ('no redistribute {0} route-map {1}'.format( + erule[0], erule[1])) commands.append(command) return commands @@ -740,8 +746,19 @@ def main(): argument_spec.update(nxos_argument_spec) + mutually_exclusive = [('dampening_state', 'dampening_routemap'), + ('dampening_state', 'dampening_half_time'), + ('dampening_state', 'dampening_suppress_time'), + ('dampening_state', 'dampening_reuse_time'), + ('dampening_state', 'dampening_max_suppress_time'), + ('dampening_routemap', 'dampening_half_time'), + ('dampening_routemap', 'dampening_suppress_time'), + ('dampening_routemap', 'dampening_reuse_time'), + ('dampening_routemap', 'dampening_max_suppress_time')] + module = AnsibleModule( argument_spec=argument_spec, + mutually_exclusive=mutually_exclusive, required_together=[DAMPENING_PARAMS, ['distance_ibgp', 'distance_ebgp', 'distance_local']], supports_check_mode=True, ) @@ -752,12 +769,6 @@ def main(): state = module.params['state'] - if module.params['dampening_routemap']: - for param in DAMPENING_PARAMS: - if module.params[param]: - module.fail_json(msg='dampening_routemap cannot be used with' - ' the {0} param'.format(param)) - if module.params['advertise_l2vpn_evpn']: if module.params['vrf'] == 'default': module.fail_json(msg='It is not possible to advertise L2VPN ' @@ -780,7 +791,7 @@ def main(): proposed_args = dict((k, v) for k, v in module.params.items() if v is not None and k in args) - for arg in ['networks', 'inject_map']: + for arg in ['networks', 'inject_map', 'redistribute']: if proposed_args.get(arg): if proposed_args[arg][0] == 'default': proposed_args[arg] = 'default' diff --git a/test/integration/targets/nxos_bgp_af/defaults/main.yaml b/test/integration/targets/nxos_bgp_af/defaults/main.yaml index 5f709c5aac1..525b7aab903 100644 --- a/test/integration/targets/nxos_bgp_af/defaults/main.yaml +++ b/test/integration/targets/nxos_bgp_af/defaults/main.yaml @@ -1,2 +1,5 @@ --- testcase: "*" +vrfs: + - default + - myvrf diff --git a/test/integration/targets/nxos_bgp_af/tests/common/sanity.yaml b/test/integration/targets/nxos_bgp_af/tests/common/sanity.yaml index c0ac6b4d165..c0e9239fcd4 100644 --- a/test/integration/targets/nxos_bgp_af/tests/common/sanity.yaml +++ b/test/integration/targets/nxos_bgp_af/tests/common/sanity.yaml @@ -35,10 +35,10 @@ provider: "{{ connection }}" when: platform is search('N9K') - - name: "Configure BGP_AF defaults" - nxos_bgp_af: &configure_default + - name: "Configure BGP_AF 1" + nxos_bgp_af: &configure1 asn: 65535 - vrf: TESTING + vrf: testing afi: ipv4 safi: unicast advertise_l2vpn_evpn: "{{advertise_l2vpn_evpn|default(omit)}}" @@ -51,7 +51,7 @@ - "result.changed == true" - name: "Check Idempotence" - nxos_bgp_af: *configure_default + nxos_bgp_af: *configure1 register: result - assert: &false @@ -59,35 +59,91 @@ - "result.changed == false" - name: "Remove BGP" - nxos_bgp: *remove + nxos_bgp_af: &remove_af + asn: 65535 + vrf: testing + afi: ipv4 + safi: unicast + state: absent + provider: "{{ connection }}" register: result - assert: *true - - name: "Check Idempotence" - nxos_bgp: *remove - register: result - - - assert: *false - - - name: "Configure BGP_AF non defaults" - nxos_bgp_af: &configure_non_default + - name: "Configure BGP_AF 2" + nxos_bgp_af: &configure2 asn: 65535 - vrf: TESTING + vrf: "{{ item }}" afi: ipv4 safi: unicast + dampening_state: True additional_paths_install: true additional_paths_receive: true additional_paths_selection: RouteMap additional_paths_send: true - advertise_l2vpn_evpn: "{{advertise_l2vpn_evpn|default(omit)}}" - client_to_client: false - dampen_igp_metric: 200 - dampening_half_time: 1 - dampening_max_suppress_time: 4 - dampening_reuse_time: 2 - dampening_suppress_time: 3 + client_to_client: False default_information_originate: true + state: present + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp_af: *configure2 + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + - name: "Configure BGP_AF def2" + nxos_bgp_af: &configuredef2 + asn: 65535 + vrf: "{{ item }}" + afi: ipv4 + safi: unicast + dampening_state: False + additional_paths_install: False + additional_paths_receive: False + additional_paths_selection: default + additional_paths_send: False + client_to_client: True + default_information_originate: False + state: present + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp_af: *configuredef2 + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + - name: "Remove BGP" + nxos_bgp_af: &remove_af_vrf + asn: 65535 + vrf: "{{ item }}" + afi: ipv4 + safi: unicast + state: absent + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Configure BGP_AF 3" + nxos_bgp_af: &configure3 + asn: 65535 + vrf: "{{ item }}" + afi: ipv4 + safi: unicast + dampening_routemap: 'abcd' default_metric: 50 distance_ebgp: 30 distance_ibgp: 60 @@ -100,24 +156,150 @@ table_map_filter: true state: present provider: "{{ connection }}" + with_items: "{{ vrfs }}" register: result - assert: *true - name: "Check Idempotence" - nxos_bgp_af: *configure_non_default + nxos_bgp_af: *configure3 + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + - name: "Configure BGP_AF def3" + nxos_bgp_af: &configuredef3 + asn: 65535 + vrf: "{{ item }}" + afi: ipv4 + safi: unicast + dampening_routemap: default + default_metric: default + distance_ebgp: default + distance_ibgp: default + distance_local: default + maximum_paths: default + maximum_paths_ibgp: default + next_hop_route_map: default + suppress_inactive: False + table_map: default + table_map_filter: False + state: present + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp_af: *configuredef3 + with_items: "{{ vrfs }}" register: result - assert: *false - name: "Remove BGP" - nxos_bgp: *remove + nxos_bgp_af: *remove_af_vrf + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Configure BGP_AF 4" + nxos_bgp_af: &configure4 + asn: 65535 + vrf: "{{ item }}" + afi: ipv4 + safi: unicast + dampen_igp_metric: 200 + dampening_half_time: 1 + dampening_max_suppress_time: 4 + dampening_reuse_time: 2 + dampening_suppress_time: 3 + inject_map: [['lax_inject_map', 'lax_exist_map'], ['nyc_inject_map', 'nyc_exist_map', 'copy-attributes'], ['fsd_inject_map', 'fsd_exist_map']] + networks: [['10.0.0.0/16', 'routemap_LA'], ['192.168.1.1/32', 'Chicago'], ['192.168.2.0/24'], ['192.168.3.0/24', 'routemap_NYC']] + redistribute: [['direct', 'rm_direct'], ['lisp', 'rm_lisp']] + state: present + provider: "{{ connection }}" + with_items: "{{ vrfs }}" register: result - assert: *true - name: "Check Idempotence" - nxos_bgp: *remove + nxos_bgp_af: *configure4 + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + - name: "Configure BGP_AF 5" + nxos_bgp_af: &configure5 + asn: 65535 + vrf: "{{ item }}" + afi: ipv4 + safi: unicast + dampen_igp_metric: 300 + dampening_half_time: 10 + dampening_max_suppress_time: 40 + dampening_reuse_time: 20 + dampening_suppress_time: 30 + inject_map: [['fsd_inject_map', 'fsd_exist_map']] + networks: [['192.168.2.0/24']] + redistribute: [['lisp', 'rm_lisp']] + state: present + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp_af: *configure5 + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + - name: "Configure BGP_AF def5" + nxos_bgp_af: &configuredef5 + asn: 65535 + vrf: "{{ item }}" + afi: ipv4 + safi: unicast + dampen_igp_metric: default + dampening_half_time: default + dampening_max_suppress_time: default + dampening_reuse_time: default + dampening_suppress_time: default + inject_map: default + networks: default + redistribute: default + state: present + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp_af: *configuredef5 + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + - name: "Remove BGP" + nxos_bgp_af: *remove_af_vrf + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp_af: *remove_af_vrf + with_items: "{{ vrfs }}" register: result - assert: *false @@ -141,6 +323,12 @@ provider: "{{ connection }}" ignore_errors: yes + # Some platforms will timeout if the + # 'no nv overlay evpn' command is sent + # too quickly following bgp disablement. + - pause: + seconds: 5 + - name: "Remove nv overlay evpn" nxos_config: lines: diff --git a/test/units/modules/network/nxos/test_nxos_bgp_af.py b/test/units/modules/network/nxos/test_nxos_bgp_af.py index 2708d59660f..b508047e821 100644 --- a/test/units/modules/network/nxos/test_nxos_bgp_af.py +++ b/test/units/modules/network/nxos/test_nxos_bgp_af.py @@ -89,7 +89,7 @@ class TestNxosBgpAfModule(TestNxosModule): dampening_half_time=5, dampening_suppress_time=2000, dampening_reuse_time=1900, dampening_max_suppress_time=10)) result = self.execute_module(failed=True) - self.assertEqual(result['msg'], 'dampening_routemap cannot be used with the dampening_half_time param') + self.assertEqual(result['msg'], 'parameters are mutually exclusive: dampening_routemap, dampening_half_time') def test_nxos_bgp_af_client(self): set_module_args(dict(asn=65535, afi='ipv4', safi='unicast',