Merge pull request #17961 from opensourcerouting/fix/bgp_reject_as_aggregate

bgpd: Do not advertise aggregate routes to contributing ASes

bgpd/bgp_aspath.c

@@ -424,8 +424,12 @@ static unsigned int aspath_count_hops_internal(const struct aspath *aspath)
 /* Check if aspath has AS_SET or AS_CONFED_SET */
 bool aspath_check_as_sets(struct aspath *aspath)
 {
-	struct assegment *seg = aspath->segments;
+	struct assegment *seg;
 
+	if (!aspath || !aspath->segments)
+		return false;
+
+	seg = aspath->segments;
 	while (seg) {
 		if (seg->type == AS_SET || seg->type == AS_CONFED_SET)
 			return true;
@@ -2512,3 +2516,39 @@ void bgp_remove_aspath_from_aggregate_hash(struct bgp_aggregate *aggregate,
 	}
 }
 
+struct aspath *aspath_delete_as_set_seq(struct aspath *aspath)
+{
+	struct assegment *seg, *prev, *next;
+	bool removed = false;
+
+	if (!(aspath && aspath->segments))
+		return aspath;
+
+	seg = aspath->segments;
+	next = NULL;
+	prev = NULL;
+
+	while (seg) {
+		next = seg->next;
+
+		if (seg->type == AS_SET || seg->type == AS_CONFED_SET) {
+			if (aspath->segments == seg)
+				aspath->segments = seg->next;
+			else
+				prev->next = seg->next;
+
+			assegment_free(seg);
+			removed = true;
+		} else
+			prev = seg;
+
+		seg = next;
+	}
+
+	if (removed) {
+		aspath_str_update(aspath, false);
+		aspath->count = aspath_count_hops_internal(aspath);
+	}
+
+	return aspath;
+}

bgpd/bgp_aspath.h

@@ -168,5 +168,6 @@ extern void bgp_remove_aspath_from_aggregate_hash(
 						struct aspath *aspath);
 
 extern void bgp_aggr_aspath_remove(void *arg);
+extern struct aspath *aspath_delete_as_set_seq(struct aspath *aspath);
 
 #endif /* _QUAGGA_BGP_ASPATH_H */

bgpd/bgp_route.c

@@ -2621,15 +2621,32 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi,
 	bgp_peer_remove_private_as(bgp, afi, safi, peer, attr);
 	bgp_peer_as_override(bgp, afi, safi, peer, attr);
 
-	/* draft-ietf-idr-deprecate-as-set-confed-set
+	/* draft-ietf-idr-deprecate-as-set-confed-set-16 */
-	 * Filter routes having AS_SET or AS_CONFED_SET in the path.
+	if (peer->bgp->reject_as_sets && aspath_check_as_sets(attr->aspath)) {
-	 * Eventually, This document (if approved) updates RFC 4271
+		struct aspath *aspath_new;
-	 * and RFC 5065 by eliminating AS_SET and AS_CONFED_SET types,
+
-	 * and obsoletes RFC 6472.
+		/* An aggregate prefix MUST NOT be announced to the contributing ASes */
-	 */
+		if (pi->sub_type == BGP_ROUTE_AGGREGATE &&
-	if (peer->bgp->reject_as_sets)
+		    aspath_loop_check(attr->aspath, peer->as)) {
-		if (aspath_check_as_sets(attr->aspath))
+			zlog_warn("%pBP [Update:SEND] %pFX is filtered by `bgp reject-as-sets`",
+				  peer, p);
 			return false;
+		}
+
+		/* When aggregating prefixes, network operators MUST use consistent brief
+		 * aggregation as described in Section 5.2. In consistent brief aggregation,
+		 * the AGGREGATOR and ATOMIC_AGGREGATE Path Attributes are included, but the
+		 * AS_PATH does not have AS_SET or AS_CONFED_SET path segment types.
+		 * The ATOMIC_AGGREGATE Path Attribute is subsequently attached to the BGP
+		 * route, if AS_SETs are dropped.
+		 */
+		if (attr->aspath->refcnt)
+			aspath_new = aspath_dup(attr->aspath);
+		else
+			aspath_new = attr->aspath;
+
+		attr->aspath = aspath_delete_as_set_seq(aspath_new);
+	}
 
 	/* If neighbor soo is configured, then check if the route has
 	 * SoO extended community and validate against the configured
@@ -8902,7 +8919,6 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi,
 	struct prefix p;
 	struct bgp_dest *dest;
 	struct bgp_aggregate *aggregate;
-	uint8_t as_set_new = as_set;
 
 	if (suppress_map && summary_only) {
 		vty_out(vty,
@@ -8960,7 +8976,6 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi,
 	 */
 	if (bgp->reject_as_sets) {
 		if (as_set == AGGREGATE_AS_SET) {
-			as_set_new = AGGREGATE_AS_UNSET;
 			zlog_warn(
 				"%s: Ignoring as-set because `bgp reject-as-sets` is enabled.",
 				__func__);
@@ -8969,7 +8984,7 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi,
 		}
 	}
 
-	aggregate->as_set = as_set_new;
+	aggregate->as_set = as_set;
 
 	/* Override ORIGIN attribute if defined.
 	 * E.g.: Cisco and Juniper set ORIGIN for aggregated address

doc/user/bgp.rst

@@ -537,6 +537,13 @@ Reject routes with AS_SET or AS_CONFED_SET types
 
    This command enables rejection of incoming and outgoing routes having AS_SET or AS_CONFED_SET type.
 
+   The aggregated routes are not sent to the contributing neighbors.
+
+.. seealso::
+   https://datatracker.ietf.org/doc/html/draft-ietf-idr-deprecate-as-set-confed-set
+
+   Default: disabled.
+
 Enforce first AS
 ----------------
 

tests/topotests/bgp_reject_as_sets/r2/bgpd.conf

@@ -6,6 +6,9 @@ router bgp 65002
   neighbor 192.168.255.2 timers 3 10
   neighbor 192.168.254.2 remote-as 65003
   neighbor 192.168.254.2 timers 3 10
+  neighbor 192.168.253.2 remote-as 65004
+  neighbor 192.168.253.2 timers 3 10
+  neighbor 192.168.253.2 solo
   address-family ipv4 unicast
     aggregate-address 172.16.0.0/16 as-set summary-only
   exit-address-family

tests/topotests/bgp_reject_as_sets/r2/zebra.conf

@@ -5,5 +5,8 @@ interface r2-eth0
 interface r2-eth1
  ip address 192.168.254.1/30
 !
+interface r2-eth2
+ ip address 192.168.253.1/30
+!
 ip forwarding
 !

tests/topotests/bgp_reject_as_sets/r4/bgpd.conf

@@ -0,0 +1,6 @@
+!
+router bgp 65004
+ no bgp ebgp-requires-policy
+ neighbor 192.168.253.1 remote-as 65002
+ neighbor 192.168.253.1 timers 3 10
+!

tests/topotests/bgp_reject_as_sets/r4/zebra.conf

@@ -0,0 +1,6 @@
+!
+interface r4-eth0
+ ip address 192.168.253.2/30
+!
+ip forwarding
+!

tests/topotests/bgp_reject_as_sets/test_bgp_reject_as_sets.py

@@ -38,7 +38,7 @@ pytestmark = [pytest.mark.bgpd]
 
 
 def build_topo(tgen):
-    for routern in range(1, 4):
+    for routern in range(1, 5):
         tgen.add_router("r{}".format(routern))
 
     switch = tgen.add_switch("s1")
@@ -49,6 +49,10 @@ def build_topo(tgen):
     switch.add_link(tgen.gears["r2"])
     switch.add_link(tgen.gears["r3"])
 
+    switch = tgen.add_switch("s3")
+    switch.add_link(tgen.gears["r2"])
+    switch.add_link(tgen.gears["r4"])
+
 
 def setup_module(mod):
     tgen = Topogen(build_topo, mod.__name__)
@@ -78,10 +82,12 @@ def test_bgp_reject_as_sets():
     if tgen.routers_have_failure():
         pytest.skip(tgen.errors)
 
-    router = tgen.gears["r2"]
+    r2 = tgen.gears["r2"]
+    r3 = tgen.gears["r3"]
+    r4 = tgen.gears["r4"]
 
-    def _bgp_converge(router):
+    def _bgp_converge():
-        output = json.loads(router.vtysh_cmd("show ip bgp neighbor 192.168.255.2 json"))
+        output = json.loads(r2.vtysh_cmd("show ip bgp neighbor 192.168.255.2 json"))
         expected = {
             "192.168.255.2": {
                 "bgpState": "Established",
@@ -90,47 +96,88 @@ def test_bgp_reject_as_sets():
         }
         return topotest.json_cmp(output, expected)
 
-    def _bgp_has_aggregated_route_with_stripped_as_set(router):
+    test_func = functools.partial(_bgp_converge)
-        output = json.loads(router.vtysh_cmd("show ip bgp 172.16.0.0/16 json"))
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    assert result is None, "Failed bgp convergence at r2"
+
+    def _bgp_has_aggregated_route():
+        output = json.loads(r2.vtysh_cmd("show ip bgp 172.16.0.0/16 json"))
         expected = {
-            "paths": [{"aspath": {"string": "Local", "segments": [], "length": 0}}]
+            "paths": [
+                {
+                    "aspath": {
+                        "string": "{65001,65003}",
+                        "segments": [{"type": "as-set", "list": [65001, 65003]}],
+                        "length": 1,
+                    },
+                    "aggregatorAs": 65002,
+                    "aggregatorId": "192.168.255.1",
+                }
+            ]
         }
         return topotest.json_cmp(output, expected)
 
-    def _bgp_announce_route_without_as_sets(router):
+    test_func = functools.partial(_bgp_has_aggregated_route)
-        output = json.loads(
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
-            router.vtysh_cmd(
+    assert result is None, "Failed to see an aggregated route at r2"
-                "show ip bgp neighbor 192.168.254.2 advertised-routes json"
+
-            )
+    def _bgp_announce_route_without_as_sets():
-        )
+        output = json.loads(r4.vtysh_cmd("show ip bgp 172.16.0.0/16 json"))
         expected = {
-            "advertisedRoutes": {
+            "paths": [
-                "172.16.0.0/16": {"path": ""},
+                {
-                "192.168.254.0/30": {"path": "65003"},
+                    "aspath": {
-                "192.168.255.0/30": {"path": "65001"},
+                        "string": "65002",
+                        "segments": [{"type": "as-sequence", "list": [65002]}],
+                        "length": 1,
+                    },
+                    "aggregatorAs": 65002,
+                    "aggregatorId": "192.168.255.1",
+                }
+            ]
+        }
+        return topotest.json_cmp(output, expected)
+
+    test_func = functools.partial(_bgp_announce_route_without_as_sets)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    assert result is None, "Route 172.16.0.0/16 should be sent without AS_SET to r4"
+
+    def _bgp_filter_aggregated_route_to_contributing_as():
+        output = json.loads(r3.vtysh_cmd("show ip bgp json"))
+        expected = {
+            "routes": {
+                "172.16.254.254/32": [
+                    {
+                        "valid": True,
+                        "bestpath": True,
+                    }
+                ],
+                "192.168.254.0/30": [
+                    {
+                        "valid": True,
+                        "bestpath": True,
+                    },
+                    {
+                        "valid": True,
+                    },
+                ],
+                "192.168.255.0/30": [
+                    {
+                        "valid": True,
+                        "bestpath": True,
+                    }
+                ],
             },
-            "totalPrefixCounter": 3,
+            "totalRoutes": 3,
+            "totalPaths": 4,
         }
         return topotest.json_cmp(output, expected)
 
-    test_func = functools.partial(_bgp_converge, router)
+    test_func = functools.partial(_bgp_filter_aggregated_route_to_contributing_as)
     _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
-
-    assert result is None, 'Failed bgp convergence in "{}"'.format(router)
-
-    test_func = functools.partial(
-        _bgp_has_aggregated_route_with_stripped_as_set, router
-    )
-    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
-
-    assert result is None, 'Failed to see an aggregated route in "{}"'.format(router)
-
-    test_func = functools.partial(_bgp_announce_route_without_as_sets, router)
-    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
-
     assert (
         result is None
-    ), 'Route 172.16.0.0/16 should be sent without AS_SET to r3 "{}"'.format(router)
+    ), "Route 172.16.0.0/16 should NOT be sent to contributing AS (r3)"
 
 
 if __name__ == "__main__":