Merge pull request #15957 from FRRouting/mergify/bp/stable/9.1/pr-15895

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

bgpd/bgp_attr.c

@@ -1517,13 +1517,22 @@ 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)
 		return false;
 	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."

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;
     }
   }
 }

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
+!

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
+!

tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py

@@ -31,21 +31,23 @@
 
 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)
 
 
 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 _bgp_converge():
         """
     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 _bgp_check_if_attributes_discarded():
         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."