From 6f8d3ad70d86682253f6a2c145249e2735cab06f Mon Sep 17 00:00:00 2001 From: saichint Date: Tue, 9 Jan 2018 21:22:26 -0800 Subject: [PATCH] Fixes for nxos_bgp (#34590) * fix bgp issues * add new tests cases * review comments --- lib/ansible/modules/network/nxos/nxos_bgp.py | 85 ++++-- .../targets/nxos_bgp/defaults/main.yaml | 3 + .../nxos_bgp/tests/common/dis_policy.yaml | 72 +++++ .../targets/nxos_bgp/tests/common/hels.yaml | 85 ++++++ .../nxos_bgp/tests/common/isolate.yaml | 68 +++++ .../targets/nxos_bgp/tests/common/param.yaml | 276 ++++++++++++++++++ .../nxos_bgp/tests/common/supp_fib.yaml | 106 +++++++ 7 files changed, 670 insertions(+), 25 deletions(-) create mode 100644 test/integration/targets/nxos_bgp/tests/common/dis_policy.yaml create mode 100644 test/integration/targets/nxos_bgp/tests/common/hels.yaml create mode 100644 test/integration/targets/nxos_bgp/tests/common/isolate.yaml create mode 100644 test/integration/targets/nxos_bgp/tests/common/param.yaml create mode 100644 test/integration/targets/nxos_bgp/tests/common/supp_fib.yaml diff --git a/lib/ansible/modules/network/nxos/nxos_bgp.py b/lib/ansible/modules/network/nxos/nxos_bgp.py index 77289faefa6..a13526847a3 100644 --- a/lib/ansible/modules/network/nxos/nxos_bgp.py +++ b/lib/ansible/modules/network/nxos/nxos_bgp.py @@ -197,13 +197,11 @@ options: description: - Set maximum time for a restart sent to the BGP peer. required: false - choices: ['true','false'] default: null graceful_restart_timers_stalepath_time: description: - Set maximum time that BGP keeps the stale routes from the restarting BGP peer. - choices: ['true','false'] default: null isolate: description: @@ -265,12 +263,6 @@ options: in seconds. required: false default: null - timer_bestpath_limit_always: - description: - - Enable/Disable update-delay-always option. - required: false - choices: ['true','false'] - default: null timer_bgp_hold: description: - Set BGP hold timer. @@ -349,11 +341,13 @@ GLOBAL_PARAMS = [ 'fast_external_fallover', 'flush_routes', 'isolate', + 'suppress_fib_pending', 'shutdown' ] PARAM_TO_DEFAULT_KEYMAP = { 'timer_bgp_keepalive': '60', 'timer_bgp_hold': '180', + 'timer_bestpath_limit': '300', 'graceful_restart': True, 'graceful_restart_timers_restart': '120', 'graceful_restart_timers_stalepath_time': '300', @@ -362,8 +356,16 @@ PARAM_TO_DEFAULT_KEYMAP = { 'fast_external_fallover': True, 'enforce_first_as': True, 'event_history_cli': True, + 'event_history_detail': False, 'event_history_events': True, 'event_history_periodic': True, + 'maxas_limit': '', + 'router_id': '', + 'cluster_id': '', + 'disable_policy_batching_ipv4_prefix_list': '', + 'disable_policy_batching_ipv6_prefix_list': '', + 'local_as': '', + 'confederation_id': '', } PARAM_TO_COMMAND_KEYMAP = { 'asn': 'router bgp', @@ -521,11 +523,12 @@ def state_present(module, existing, proposed, candidate): if key == 'confederation peers': existing_value = ' '.join(existing_value) commands.append('no {0} {1}'.format(key, existing_value)) + elif not value: + existing_value = existing_commands.get(key) + if existing_value: + commands.append('no {0} {1}'.format(key, existing_value)) elif key == 'confederation peers': - existing_confederation_peers = set(existing.get('confederation_peers', [])) - new_values = set(value.split()) - peer_string = ' '.join(existing_confederation_peers | new_values) - commands.append('{0} {1}'.format(key, peer_string)) + commands.append('{0} {1}'.format(key, value)) elif key.startswith('timers bgp'): command = 'timers bgp {0} {1}'.format( proposed['timer_bgp_keepalive'], @@ -580,16 +583,38 @@ def fix_commands(commands): confederation_peers_command = command if local_as_command and confederation_id_command: - commands.pop(commands.index(local_as_command)) - commands.pop(commands.index(confederation_id_command)) - commands.append(local_as_command) - commands.append(confederation_id_command) - - elif confederation_peers_command and confederation_id_command: - commands.pop(commands.index(confederation_peers_command)) - commands.pop(commands.index(confederation_id_command)) - commands.append(confederation_id_command) - commands.append(confederation_peers_command) + if 'no' in confederation_id_command: + commands.pop(commands.index(local_as_command)) + commands.pop(commands.index(confederation_id_command)) + commands.append(confederation_id_command) + commands.append(local_as_command) + else: + commands.pop(commands.index(local_as_command)) + commands.pop(commands.index(confederation_id_command)) + commands.append(local_as_command) + commands.append(confederation_id_command) + + if confederation_peers_command and confederation_id_command: + if local_as_command: + if 'no' in local_as_command: + commands.pop(commands.index(local_as_command)) + commands.pop(commands.index(confederation_id_command)) + commands.pop(commands.index(confederation_peers_command)) + commands.append(confederation_id_command) + commands.append(confederation_peers_command) + commands.append(local_as_command) + else: + commands.pop(commands.index(local_as_command)) + commands.pop(commands.index(confederation_id_command)) + commands.pop(commands.index(confederation_peers_command)) + commands.append(local_as_command) + commands.append(confederation_id_command) + commands.append(confederation_peers_command) + else: + commands.pop(commands.index(confederation_peers_command)) + commands.pop(commands.index(confederation_id_command)) + commands.append(confederation_id_command) + commands.append(confederation_peers_command) return commands @@ -608,7 +633,7 @@ def main(): bestpath_med_non_deterministic=dict(required=False, type='bool'), cluster_id=dict(required=False, type='str'), confederation_id=dict(required=False, type='str'), - confederation_peers=dict(required=False, type='str'), + confederation_peers=dict(required=False, type='list'), disable_policy_batching=dict(required=False, type='bool'), disable_policy_batching_ipv4_prefix_list=dict(required=False, type='str'), disable_policy_batching_ipv6_prefix_list=dict(required=False, type='str'), @@ -672,8 +697,18 @@ def main(): if key not in ['asn', 'vrf']: if str(value).lower() == 'default': value = PARAM_TO_DEFAULT_KEYMAP.get(key, 'default') - if existing.get(key) != value: - proposed[key] = value + if key == 'confederation_peers': + if value[0] == 'default': + if existing.get(key): + proposed[key] = 'default' + else: + v = set([int(i) for i in value]) + ex = set([int(i) for i in existing.get(key)]) + if v != ex: + proposed[key] = ' '.join(str(s) for s in v) + else: + if existing.get(key) != value: + proposed[key] = value candidate = CustomNetworkConfig(indent=3) if state == 'present': diff --git a/test/integration/targets/nxos_bgp/defaults/main.yaml b/test/integration/targets/nxos_bgp/defaults/main.yaml index 5f709c5aac1..525b7aab903 100644 --- a/test/integration/targets/nxos_bgp/defaults/main.yaml +++ b/test/integration/targets/nxos_bgp/defaults/main.yaml @@ -1,2 +1,5 @@ --- testcase: "*" +vrfs: + - default + - myvrf diff --git a/test/integration/targets/nxos_bgp/tests/common/dis_policy.yaml b/test/integration/targets/nxos_bgp/tests/common/dis_policy.yaml new file mode 100644 index 00000000000..367289d1b4d --- /dev/null +++ b/test/integration/targets/nxos_bgp/tests/common/dis_policy.yaml @@ -0,0 +1,72 @@ +--- +- debug: msg="START connection={{ ansible_connection }} nxos_bgp parameter test" +- debug: msg="Using provider={{ connection.transport }}" + when: ansible_connection == "local" + +- name: "Disable feature BGP" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + +- name: "Enable feature BGP" + nxos_feature: + feature: bgp + state: enabled + provider: "{{ connection }}" + ignore_errors: yes + +- block: + # these tasks will fail on N3k A8 code and n7k running helsinki + # due to no support + - name: "set disable policy" + nxos_bgp: &set1 + asn: 65535 + disable_policy_batching: true + disable_policy_batching_ipv4_prefix_list: v4_p + disable_policy_batching_ipv6_prefix_list: v6_p + provider: "{{ connection }}" + register: result + + - assert: &true + that: + - "result.changed == true" + + - name: "Check Idempotence" + nxos_bgp: *set1 + register: result + + - assert: &false + that: + - "result.changed == false" + + - name: "reset disable policy" + nxos_bgp: &reset1 + asn: 65535 + disable_policy_batching: false + disable_policy_batching_ipv4_prefix_list: default + disable_policy_batching_ipv6_prefix_list: default + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset1 + register: result + + - assert: *false + + rescue: + - debug: msg="Tests can fail on A8 or helsinki images" + + always: + - name: "Disable feature bgp" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + + - debug: msg="END connection={{ ansible_connection }} nxos_bgp parameter test" diff --git a/test/integration/targets/nxos_bgp/tests/common/hels.yaml b/test/integration/targets/nxos_bgp/tests/common/hels.yaml new file mode 100644 index 00000000000..7417a5132ec --- /dev/null +++ b/test/integration/targets/nxos_bgp/tests/common/hels.yaml @@ -0,0 +1,85 @@ +--- +- debug: msg="START connection={{ ansible_connection }} nxos_bgp parameter test" +- debug: msg="Using provider={{ connection.transport }}" + when: ansible_connection == "local" + +- name: "Disable feature BGP" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + +- name: "Enable feature BGP" + nxos_feature: + feature: bgp + state: enabled + provider: "{{ connection }}" + ignore_errors: yes + +- block: + # these tasks will fail on n7k running helsinki + # due to no support + - name: "set helsinki" + nxos_bgp: &set1 + asn: 65535 + vrf: "{{ item }}" + graceful_restart_timers_restart: 130 + graceful_restart_timers_stalepath_time: 310 + neighbor_down_fib_accelerate: true + reconnect_interval: 55 + timer_bgp_hold: 110 + timer_bgp_keepalive: 45 + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: &true + that: + - "result.changed == true" + + - name: "Check Idempotence" + nxos_bgp: *set1 + with_items: "{{ vrfs }}" + register: result + + - assert: &false + that: + - "result.changed == false" + + - name: "reset helsinki" + nxos_bgp: &reset1 + asn: 65535 + vrf: "{{ item }}" + graceful_restart: true + graceful_restart_timers_restart: default + graceful_restart_timers_stalepath_time: default + neighbor_down_fib_accelerate: false + reconnect_interval: default + timer_bgp_hold: default + timer_bgp_keepalive: default + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset1 + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + rescue: + - debug: msg="Tests can fail on helsinki images" + + always: + - name: "Disable feature bgp" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + + - debug: msg="END connection={{ ansible_connection }} nxos_bgp parameter test" diff --git a/test/integration/targets/nxos_bgp/tests/common/isolate.yaml b/test/integration/targets/nxos_bgp/tests/common/isolate.yaml new file mode 100644 index 00000000000..8afd8a1d1ca --- /dev/null +++ b/test/integration/targets/nxos_bgp/tests/common/isolate.yaml @@ -0,0 +1,68 @@ +--- +- debug: msg="START connection={{ ansible_connection }} nxos_bgp parameter test" +- debug: msg="Using provider={{ connection.transport }}" + when: ansible_connection == "local" + +- name: "Disable feature BGP" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + +- name: "Enable feature BGP" + nxos_feature: + feature: bgp + state: enabled + provider: "{{ connection }}" + ignore_errors: yes + +- block: + # these tasks will fail on n3k running A8 + # due to no support + - name: "set isolate" + nxos_bgp: &set1 + asn: 65535 + isolate: false + provider: "{{ connection }}" + register: result + + - assert: &true + that: + - "result.changed == true" + + - name: "Check Idempotence" + nxos_bgp: *set1 + register: result + + - assert: &false + that: + - "result.changed == false" + + - name: "reset isolate" + nxos_bgp: &reset1 + asn: 65535 + isolate: true + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset1 + register: result + + - assert: *false + + rescue: + - debug: msg="Tests can fail on A8 images" + + always: + - name: "Disable feature bgp" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + + - debug: msg="END connection={{ ansible_connection }} nxos_bgp parameter test" diff --git a/test/integration/targets/nxos_bgp/tests/common/param.yaml b/test/integration/targets/nxos_bgp/tests/common/param.yaml new file mode 100644 index 00000000000..5ea70bf6a6b --- /dev/null +++ b/test/integration/targets/nxos_bgp/tests/common/param.yaml @@ -0,0 +1,276 @@ +--- +- debug: msg="START connection={{ ansible_connection }} nxos_bgp parameter test" +- debug: msg="Using provider={{ connection.transport }}" + when: ansible_connection == "local" + +- name: "Disable feature BGP" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + +- name: "Enable feature BGP" + nxos_feature: + feature: bgp + state: enabled + provider: "{{ connection }}" + ignore_errors: yes + +- block: + - name: "set multi vrf params" + nxos_bgp: &set_multi_vrf + asn: 65535 + vrf: "{{ item }}" + router_id: 1.1.1.1 + bestpath_always_compare_med: true + bestpath_aspath_multipath_relax: true + bestpath_compare_routerid: true + bestpath_cost_community_ignore: true + bestpath_med_confed: true + bestpath_med_missing_as_worst: true + bestpath_med_non_deterministic: true +# grace_restart is failing with error code -32603 only on CLI transport, nxapi ok +# graceful_restart: false + graceful_restart_helper: true + log_neighbor_changes: true + maxas_limit: 50 + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: &true + that: + - "result.changed == true" + + - name: "Check Idempotence" + nxos_bgp: *set_multi_vrf + with_items: "{{ vrfs }}" + register: result + + - assert: &false + that: + - "result.changed == false" + + - name: "reset multi vrf params" + nxos_bgp: &reset_multi_vrf + asn: 65535 + vrf: "{{ item }}" + bestpath_always_compare_med: false + bestpath_aspath_multipath_relax: false + bestpath_compare_routerid: false + bestpath_cost_community_ignore: false + bestpath_med_confed: false + bestpath_med_missing_as_worst: false + bestpath_med_non_deterministic: false + graceful_restart_helper: false + log_neighbor_changes: false + maxas_limit: default + router_id: default + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset_multi_vrf + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + - name: "set clusterid" + nxos_bgp: &set_cluster_id + asn: 65535 + vrf: "{{ item }}" + cluster_id: 10.0.0.1 + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *set_cluster_id + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + - name: "reset cluster_id" + nxos_bgp: &reset_cluster_id + asn: 65535 + vrf: "{{ item }}" + cluster_id: default + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset_cluster_id + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + - name: "set confederation" + nxos_bgp: &set_confederation + asn: 65535 + confederation_id: 99 + confederation_peers: + - 16 + - 22 + - 18 + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *set_confederation + register: result + + - assert: *false + + - name: "reset confederation" + nxos_bgp: &reset_confederation + asn: 65535 + confederation_id: default + confederation_peers: default + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset_confederation + register: result + + - assert: *false + + - name: "set confederation_local_as" + nxos_bgp: &set_confederation_la + asn: 65535 + vrf: myvrf + local_as: 33 + confederation_id: 99 + confederation_peers: + - 16 + - 22 + - 18 + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *set_confederation_la + register: result + + - assert: *false + + - name: "reset confederation local_as" + nxos_bgp: &reset_confederation_la + asn: 65535 + vrf: myvrf + local_as: default + confederation_id: default + confederation_peers: default + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset_confederation_la + register: result + + - assert: *false + + - name: "set local_as" + nxos_bgp: &set_local_as + asn: 65535 + vrf: myvrf + local_as: 33 + confederation_id: 99 + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *set_local_as + register: result + + - assert: *false + + - name: "reset local_as" + nxos_bgp: &reset_local_as + asn: 65535 + vrf: myvrf + confederation_id: default + local_as: default + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset_local_as + register: result + + - assert: *false + + - name: "set default vrf params" + nxos_bgp: &set_def_vrf + asn: 65535 + event_history_cli: size_medium + event_history_detail: size_large + event_history_events: size_medium + event_history_periodic: size_small + enforce_first_as: false + fast_external_fallover: false + flush_routes: true + shutdown: true + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *set_def_vrf + register: result + + - assert: *false + + - name: "reset default vrf params" + nxos_bgp: &reset_def_vrf + asn: 65535 + event_history_detail: default + enforce_first_as: true + fast_external_fallover: true + flush_routes: false + shutdown: false + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset_def_vrf + register: result + + - assert: *false + + always: + - name: "Disable feature bgp" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + + - debug: msg="END connection={{ ansible_connection }} nxos_bgp parameter test" diff --git a/test/integration/targets/nxos_bgp/tests/common/supp_fib.yaml b/test/integration/targets/nxos_bgp/tests/common/supp_fib.yaml new file mode 100644 index 00000000000..46ab3e3bde0 --- /dev/null +++ b/test/integration/targets/nxos_bgp/tests/common/supp_fib.yaml @@ -0,0 +1,106 @@ +--- +- debug: msg="START connection={{ ansible_connection }} nxos_bgp parameter test" +- debug: msg="Using provider={{ connection.transport }}" + when: ansible_connection == "local" + +- name: "Disable feature BGP" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + +- name: "Enable feature BGP" + nxos_feature: + feature: bgp + state: enabled + provider: "{{ connection }}" + ignore_errors: yes + +- block: + # this task will fail on n9k running I2 + # due to no support + - name: "set bestpath limit" + nxos_bgp: &set1 + asn: 65535 + vrf: "{{ item }}" + timer_bestpath_limit: 255 + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: &true + that: + - "result.changed == true" + + - name: "Check Idempotence" + nxos_bgp: *set1 + with_items: "{{ vrfs }}" + register: result + + - assert: &false + that: + - "result.changed == false" + + - name: "reset bestpath limit" + nxos_bgp: &reset1 + asn: 65535 + vrf: "{{ item }}" + timer_bestpath_limit: default + provider: "{{ connection }}" + with_items: "{{ vrfs }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset1 + with_items: "{{ vrfs }}" + register: result + + - assert: *false + + # this task will fail on n9k running I2 or I4 + # due to no support + - name: "set suppress fib" + nxos_bgp: &set2 + asn: 65535 + suppress_fib_pending: false + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *set2 + register: result + + - assert: *false + + - name: "reset suppress fib" + nxos_bgp: &reset2 + asn: 65535 + suppress_fib_pending: true + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence" + nxos_bgp: *reset2 + register: result + + - assert: *false + + rescue: + - debug: msg="Tests can fail on I2/I4/A8/Fretta or helsinki images" + + always: + - name: "Disable feature bgp" + nxos_feature: + feature: bgp + state: disabled + provider: "{{ connection }}" + ignore_errors: yes + + - debug: msg="END connection={{ ansible_connection }} nxos_bgp parameter test"