Merge pull request #15956 from FRRouting/mergify/bp/stable/10.0/pr-15895

bgpd: Ignore validating the attribute flags if path-attribute is configured (backport #15895)

7 files changed, 81 insertions, 23 deletions

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 53420b492e..40e074d058 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1518,6 +1518,7 @@ static bool bgp_attr_flag_invalid(struct bgp_attr_parser_args *args)
 	uint8_t mask = BGP_ATTR_FLAG_EXTLEN;
 	const uint8_t flags = args->flags;
 	const uint8_t attr_code = args->type;
+	struct peer *peer = args->peer;
 
 	/* there may be attributes we don't know about */
 	if (attr_code > attr_flags_values_max)
@@ -1525,6 +1526,14 @@ static bool bgp_attr_flag_invalid(struct bgp_attr_parser_args *args)
 	if (attr_flags_values[attr_code] == 0)
 		return false;
 
+	/* If `neighbor X path-attribute <discard|treat-as-withdraw>` is
+	 * configured, then ignore checking optional, trasitive flags.
+	 * The attribute/route will be discarded/withdrawned later instead
+	 * of dropping the session.
+	 */
+	if (peer->discard_attrs[attr_code] || peer->withdraw_attrs[attr_code])
+		return false;
+
 	/* RFC4271, "For well-known attributes, the Transitive bit MUST be set
 	 * to
 	 * 1."
diff --git a/tests/topotests/bgp_path_attribute_discard/peer1/exabgp.cfg b/tests/topotests/bgp_path_attribute_discard/peer1/exabgp.cfg
index 7fb9210ecf..dccec7d154 100644
--- a/tests/topotests/bgp_path_attribute_discard/peer1/exabgp.cfg
+++ b/tests/topotests/bgp_path_attribute_discard/peer1/exabgp.cfg
@@ -1,8 +1,8 @@
 neighbor 10.0.0.1 {
-  router-id 10.0.0.2;
-  local-address 10.0.0.2;
-  local-as 65001;
-  peer-as 65002;
+  router-id 10.0.0.254;
+  local-address 10.0.0.254;
+  local-as 65254;
+  peer-as 65001;
 
   capability {
     route-refresh;
@@ -12,13 +12,28 @@ neighbor 10.0.0.1 {
     route 192.168.100.101/32 {
       atomic-aggregate;
       community 65001:101;
-      next-hop 10.0.0.2;
+      next-hop 10.0.0.254;
     }
 
     route 192.168.100.102/32 {
-      originator-id 10.0.0.2;
+      originator-id 10.0.0.254;
       community 65001:102;
-      next-hop 10.0.0.2;
+      next-hop 10.0.0.254;
+    }
+  }
+}
+
+neighbor 10.0.0.2 {
+  router-id 10.0.0.254;
+  local-address 10.0.0.254;
+  local-as 65254;
+  peer-as 65254;
+
+  static {
+    route 192.168.100.101/32 {
+      # AIGP invalid attribute: flagged as transitive + optional.
+      attribute [0x1a 0xc0 0x00000064];
+      next-hop 10.0.0.254;
     }
   }
 }
diff --git a/tests/topotests/bgp_path_attribute_discard/r1/bgpd.conf b/tests/topotests/bgp_path_attribute_discard/r1/bgpd.conf
deleted file mode 100644
index c96f354cc5..0000000000
--- a/tests/topotests/bgp_path_attribute_discard/r1/bgpd.conf
+++ /dev/null
@@ -1,6 +0,0 @@
-!
-router bgp 65002
- no bgp ebgp-requires-policy
- neighbor 10.0.0.2 remote-as external
- neighbor 10.0.0.2 timers 3 10
-!
diff --git a/tests/topotests/bgp_path_attribute_discard/r1/frr.conf b/tests/topotests/bgp_path_attribute_discard/r1/frr.conf
new file mode 100644
index 0000000000..ae7fbdd9a9
--- /dev/null
+++ b/tests/topotests/bgp_path_attribute_discard/r1/frr.conf
@@ -0,0 +1,9 @@
+!
+interface r1-eth0
+ ip address 10.0.0.1/24
+!
+router bgp 65001
+ no bgp ebgp-requires-policy
+ neighbor 10.0.0.254 remote-as external
+ neighbor 10.0.0.254 timers 3 10
+!
diff --git a/tests/topotests/bgp_path_attribute_discard/r1/zebra.conf b/tests/topotests/bgp_path_attribute_discard/r1/zebra.conf
deleted file mode 100644
index 51a1b2657c..0000000000
--- a/tests/topotests/bgp_path_attribute_discard/r1/zebra.conf
+++ /dev/null
@@ -1,4 +0,0 @@
-!
-interface r1-eth0
- ip address 10.0.0.1/24
-!
diff --git a/tests/topotests/bgp_path_attribute_discard/r2/frr.conf b/tests/topotests/bgp_path_attribute_discard/r2/frr.conf
new file mode 100644
index 0000000000..1dafbdd8e1
--- /dev/null
+++ b/tests/topotests/bgp_path_attribute_discard/r2/frr.conf
@@ -0,0 +1,10 @@
+!
+interface r2-eth0
+ ip address 10.0.0.2/24
+!
+router bgp 65254
+ no bgp ebgp-requires-policy
+ neighbor 10.0.0.254 remote-as internal
+ neighbor 10.0.0.254 timers 3 10
+ neighbor 10.0.0.254 path-attribute discard 26
+!
diff --git a/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py b/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py
index c97cd0bdda..bd8cd8e18a 100644
--- a/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py
+++ b/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py
@@ -31,10 +31,12 @@ pytestmark = [pytest.mark.bgpd]
 
 def build_topo(tgen):
     r1 = tgen.add_router("r1")
-    peer1 = tgen.add_exabgp_peer("peer1", ip="10.0.0.2", defaultRoute="via 10.0.0.1")
+    r2 = tgen.add_router("r2")
+    peer1 = tgen.add_exabgp_peer("peer1", ip="10.0.0.254", defaultRoute="via 10.0.0.1")
 
     switch = tgen.add_switch("s1")
     switch.add_link(r1)
+    switch.add_link(r2)
     switch.add_link(peer1)
 
 
@@ -42,10 +44,10 @@ def setup_module(mod):
     tgen = Topogen(build_topo, mod.__name__)
     tgen.start_topology()
 
-    router = tgen.gears["r1"]
-    router.load_config(TopoRouter.RD_ZEBRA, os.path.join(CWD, "r1/zebra.conf"))
-    router.load_config(TopoRouter.RD_BGP, os.path.join(CWD, "r1/bgpd.conf"))
-    router.start()
+    for _, (rname, router) in enumerate(tgen.routers().items(), 1):
+        router.load_frr_config(os.path.join(CWD, "{}/frr.conf".format(rname)))
+
+    tgen.start_router()
 
     peer = tgen.gears["peer1"]
     peer.start(os.path.join(CWD, "peer1"), os.path.join(CWD, "exabgp.env"))
@@ -63,6 +65,7 @@ def test_bgp_path_attribute_discard():
         pytest.skip(tgen.errors)
 
     r1 = tgen.gears["r1"]
+    r2 = tgen.gears["r2"]
 
     def _bgp_converge():
         output = json.loads(r1.vtysh_cmd("show bgp ipv4 unicast json detail"))
@@ -103,7 +106,7 @@ def test_bgp_path_attribute_discard():
         """
     configure terminal
         router bgp
-            neighbor 10.0.0.2 path-attribute discard 6 8
+            neighbor 10.0.0.254 path-attribute discard 6 8
     """
     )
 
@@ -139,6 +142,28 @@ def test_bgp_path_attribute_discard():
         result is None
     ), "Failed to discard path attributes (atomic-aggregate, community)"
 
+    def _bgp_check_if_aigp_invalid_attribute_discarded():
+        output = json.loads(r2.vtysh_cmd("show bgp ipv4 unicast json detail"))
+        expected = {
+            "routes": {
+                "192.168.100.101/32": {
+                    "paths": [
+                        {
+                            "valid": True,
+                            "aigpMetric": None,
+                        }
+                    ],
+                },
+            }
+        }
+        return topotest.json_cmp(output, expected)
+
+    test_func = functools.partial(_bgp_check_if_aigp_invalid_attribute_discarded)
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5)
+    assert (
+        result is None
+    ), "Failed to discard AIGP invalid path attribute (iBGP session)"
+
 
 def test_memory_leak():
     "Run the memory leak test and report results."