Fixes for nxos_bgp (#34590)

* fix bgp issues

* add new tests cases

* review comments

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)
+        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)
 
-    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 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':

test/integration/targets/nxos_bgp/defaults/main.yaml

@@ -1,2 +1,5 @@
 ---
 testcase: "*"
+vrfs:
+  - default
+  - myvrf

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"

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"

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"

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"

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"