Revert "bgpd: Handle Addpath capability using dynamic capabilities"

This reverts commit 05cf9d03b345393b8d63ffe9345c42debd8362b6.

TL;DR; Handling BGP AddPath capability is not trivial (possible) dynamically.

When the sender is AddPath-capable and sends NLRIs encoded with AddPath ID,
and at the same time the receiver sends AddPath capability "disable-addpath-rx"
(flag update) via dynamic capabilities, both peers are out of sync about the
AddPath state. The receiver thinks already he's not AddPath-capable anymore,
hence it tries to parse NLRIs as non-AddPath, while they are actually encoded
as AddPath.

AddPath capability itself does not provide (in RFC) any mechanism on backward
compatible way to handle NLRIs if they come mixed (AddPath + non-AddPath).

This explains why we have failures in our CI periodically.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>

2 files changed, 13 insertions, 112 deletions

diff --git a/tests/topotests/bgp_dynamic_capability/r2/frr.conf b/tests/topotests/bgp_dynamic_capability/r2/frr.conf
index 621e9381e3..cca07078ea 100644
--- a/tests/topotests/bgp_dynamic_capability/r2/frr.conf
+++ b/tests/topotests/bgp_dynamic_capability/r2/frr.conf
@@ -18,7 +18,6 @@ router bgp 65002
  neighbor 192.168.1.1 timers connect 1
  neighbor 192.168.1.1 capability dynamic
  neighbor 192.168.1.1 capability extended-nexthop
- neighbor 192.168.1.1 addpath-rx-paths-limit 20
  neighbor 2001:db8::1 remote-as external
  neighbor 2001:db8::1 timers 1 3
  neighbor 2001:db8::1 timers connect 1
@@ -27,6 +26,9 @@ router bgp 65002
  !
  address-family ipv4 unicast
   redistribute connected
+  neighbor 192.168.1.1 addpath-tx-all-paths
+  neighbor 192.168.1.1 disable-addpath-rx
+  neighbor 192.168.1.1 addpath-rx-paths-limit 20
  exit-address-family
  !
  address-family ipv6 unicast
diff --git a/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_addpath.py b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_path_limit.py
index 91df89b1b5..22e4fe687b 100644
--- a/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_addpath.py
+++ b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_path_limit.py
@@ -9,8 +9,6 @@
 Test if Addpath/Paths-Limit capabilities are adjusted dynamically.
 T1: Enable Addpath/Paths-Limit capabilities and check if they are exchanged dynamically
 T2: Disable paths limit and check if it's exchanged dynamically
-T3: Disable Addpath capability RX and check if it's exchanged dynamically
-T4: Disable Addpath capability and check if it's exchanged dynamically
 """
 
 import os
@@ -65,12 +63,12 @@ def test_bgp_addpath_paths_limit():
                     "dynamic": "advertisedAndReceived",
                     "addPath": {
                         "ipv4Unicast": {
-                            "txAdvertisedAndReceived": False,
+                            "txAdvertisedAndReceived": True,
                             "txAdvertised": True,
-                            "txReceived": False,
-                            "rxAdvertisedAndReceived": True,
+                            "txReceived": True,
+                            "rxAdvertisedAndReceived": False,
                             "rxAdvertised": True,
-                            "rxReceived": True,
+                            "rxReceived": False,
                         }
                     },
                     "pathsLimit": {
@@ -105,7 +103,6 @@ def test_bgp_addpath_paths_limit():
     configure terminal
      router bgp
       address-family ipv4 unicast
-       neighbor 192.168.1.1 addpath-tx-all-paths
        neighbor 192.168.1.1 addpath-rx-paths-limit 21
     """
     )
@@ -122,9 +119,9 @@ def test_bgp_addpath_paths_limit():
                             "txAdvertisedAndReceived": True,
                             "txAdvertised": True,
                             "txReceived": True,
-                            "rxAdvertisedAndReceived": True,
+                            "rxAdvertisedAndReceived": False,
                             "rxAdvertised": True,
-                            "rxReceived": True,
+                            "rxReceived": False,
                         }
                     },
                     "pathsLimit": {
@@ -143,7 +140,7 @@ def test_bgp_addpath_paths_limit():
                 "messageStats": {
                     "notificationsRecv": 0,
                     "notificationsSent": 0,
-                    "capabilityRecv": 2,
+                    "capabilityRecv": 1,
                 },
             }
         }
@@ -181,58 +178,6 @@ def test_bgp_addpath_paths_limit():
                             "txAdvertisedAndReceived": True,
                             "txAdvertised": True,
                             "txReceived": True,
-                            "rxAdvertisedAndReceived": True,
-                            "rxAdvertised": True,
-                            "rxReceived": True,
-                        }
-                    },
-                    "pathsLimit": {
-                        "ipv4Unicast": {
-                            "advertisedAndReceived": True,
-                            "advertisedPathsLimit": 10,
-                            "receivedPathsLimit": 0,
-                        }
-                    },
-                },
-                "messageStats": {
-                    "notificationsRecv": 0,
-                    "notificationsSent": 0,
-                    "capabilityRecv": 3,
-                },
-            }
-        }
-        return topotest.json_cmp(output, expected)
-
-    test_func = functools.partial(
-        _disable_paths_limit,
-    )
-    _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
-    assert result is None, "Something went wrong after disabling paths limit"
-
-    ###
-    # T3: Disable Addpath capability RX and check if it's exchanged dynamically
-    ###
-    r2.vtysh_cmd(
-        """
-    configure terminal
-    router bgp
-     address-family ipv4 unicast
-      neighbor 192.168.1.1 disable-addpath-rx
-    """
-    )
-
-    def _disable_addpath_rx():
-        output = json.loads(r1.vtysh_cmd("show bgp neighbor json"))
-        expected = {
-            "192.168.1.2": {
-                "bgpState": "Established",
-                "neighborCapabilities": {
-                    "dynamic": "advertisedAndReceived",
-                    "addPath": {
-                        "ipv4Unicast": {
-                            "txAdvertisedAndReceived": True,
-                            "txAdvertised": True,
-                            "txReceived": True,
                             "rxAdvertisedAndReceived": False,
                             "rxAdvertised": True,
                             "rxReceived": False,
@@ -249,63 +194,17 @@ def test_bgp_addpath_paths_limit():
                 "messageStats": {
                     "notificationsRecv": 0,
                     "notificationsSent": 0,
-                    "capabilityRecv": 4,
-                },
-            }
-        }
-        return topotest.json_cmp(output, expected)
-
-    test_func = functools.partial(
-        _disable_addpath_rx,
-    )
-    _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
-    assert result is None, "Something went wrong after disabling Addpath RX flags"
-
-    ###
-    # T4: Disable Addpath capability and check if it's exchanged dynamically
-    ###
-    r1.vtysh_cmd(
-        """
-    configure terminal
-    router bgp
-     address-family ipv4 unicast
-      no neighbor 192.168.1.2 addpath-tx-all-paths
-    """
-    )
-
-    def _disable_addpath():
-        output = json.loads(r1.vtysh_cmd("show bgp neighbor json"))
-        expected = {
-            "192.168.1.2": {
-                "bgpState": "Established",
-                "neighborCapabilities": {
-                    "dynamic": "advertisedAndReceived",
-                    "addPath": {
-                        "ipv4Unicast": {
-                            "txAdvertisedAndReceived": False,
-                            "txAdvertised": False,
-                            "txReceived": True,
-                            "rxAdvertisedAndReceived": False,
-                            "rxAdvertised": True,
-                            "rxReceived": False,
-                        }
-                    },
-                },
-                "messageStats": {
-                    "notificationsRecv": 0,
-                    "notificationsSent": 0,
-                    "capabilitySent": 1,
-                    "capabilityRecv": 4,
+                    "capabilityRecv": 2,
                 },
             }
         }
         return topotest.json_cmp(output, expected)
 
     test_func = functools.partial(
-        _disable_addpath,
+        _disable_paths_limit,
     )
     _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
-    assert result is None, "Something went wrong when disabling Addpath capability"
+    assert result is None, "Something went wrong after disabling paths limit"
 
 
 if __name__ == "__main__":