From 925b365a87f21f29c9b1378cae468b2f989c346f Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 30 Jan 2025 10:32:22 +0200 Subject: [PATCH 1/3] bgpd: Do not advertise aggregate routes to contributing ASes draft-ietf-idr-deprecate-as-set-confed-set-16 defines that we MUST NOT advertise an aggregate prefix to the contributing ASes. Signed-off-by: Donatas Abraitis --- bgpd/bgp_aspath.c | 42 +++++++++++++++++++++++++++++++++++++++++- bgpd/bgp_aspath.h | 1 + bgpd/bgp_route.c | 37 ++++++++++++++++++++++++++----------- 3 files changed, 68 insertions(+), 12 deletions(-) diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index a86b42e25015..d626e3badf11 100644 --- a/bgpd/bgp_aspath.c +++ b/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; +} diff --git a/bgpd/bgp_aspath.h b/bgpd/bgp_aspath.h index 46202fd34afc..295837b1371d 100644 --- a/bgpd/bgp_aspath.h +++ b/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 */ diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index f2e61e1e7faf..6b17c7f0a3c7 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -2614,15 +2614,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 - * Filter routes having AS_SET or AS_CONFED_SET in the path. - * Eventually, This document (if approved) updates RFC 4271 - * and RFC 5065 by eliminating AS_SET and AS_CONFED_SET types, - * and obsoletes RFC 6472. - */ - if (peer->bgp->reject_as_sets) - if (aspath_check_as_sets(attr->aspath)) + /* draft-ietf-idr-deprecate-as-set-confed-set-16 */ + if (peer->bgp->reject_as_sets && aspath_check_as_sets(attr->aspath)) { + struct aspath *aspath_new; + + /* An aggregate prefix MUST NOT be announced to the contributing ASes */ + if (pi->sub_type == BGP_ROUTE_AGGREGATE && + aspath_loop_check(attr->aspath, peer->as)) { + 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 @@ -8922,7 +8939,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, @@ -8980,7 +8996,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__); @@ -8989,7 +9004,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 From 25a37e936763056a725accedc8fb182f1090f3ad Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 30 Jan 2025 10:34:32 +0200 Subject: [PATCH 2/3] tests: Check if aggregated prefix is not advertised to contributing ASes Signed-off-by: Donatas Abraitis --- .../topotests/bgp_reject_as_sets/r2/bgpd.conf | 3 + .../bgp_reject_as_sets/r2/zebra.conf | 3 + .../topotests/bgp_reject_as_sets/r4/bgpd.conf | 6 + .../bgp_reject_as_sets/r4/zebra.conf | 6 + .../test_bgp_reject_as_sets.py | 109 +++++++++++++----- 5 files changed, 96 insertions(+), 31 deletions(-) create mode 100644 tests/topotests/bgp_reject_as_sets/r4/bgpd.conf create mode 100644 tests/topotests/bgp_reject_as_sets/r4/zebra.conf diff --git a/tests/topotests/bgp_reject_as_sets/r2/bgpd.conf b/tests/topotests/bgp_reject_as_sets/r2/bgpd.conf index 453961762af8..f51634d7f258 100644 --- a/tests/topotests/bgp_reject_as_sets/r2/bgpd.conf +++ b/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 diff --git a/tests/topotests/bgp_reject_as_sets/r2/zebra.conf b/tests/topotests/bgp_reject_as_sets/r2/zebra.conf index f0d357c5ff93..6112ca545e33 100644 --- a/tests/topotests/bgp_reject_as_sets/r2/zebra.conf +++ b/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 ! diff --git a/tests/topotests/bgp_reject_as_sets/r4/bgpd.conf b/tests/topotests/bgp_reject_as_sets/r4/bgpd.conf new file mode 100644 index 000000000000..957b5ba56803 --- /dev/null +++ b/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 +! diff --git a/tests/topotests/bgp_reject_as_sets/r4/zebra.conf b/tests/topotests/bgp_reject_as_sets/r4/zebra.conf new file mode 100644 index 000000000000..a8c386f39a13 --- /dev/null +++ b/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 +! diff --git a/tests/topotests/bgp_reject_as_sets/test_bgp_reject_as_sets.py b/tests/topotests/bgp_reject_as_sets/test_bgp_reject_as_sets.py index b9d8ce681914..141f353bd7fa 100644 --- a/tests/topotests/bgp_reject_as_sets/test_bgp_reject_as_sets.py +++ b/tests/topotests/bgp_reject_as_sets/test_bgp_reject_as_sets.py @@ -38,7 +38,7 @@ 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): - output = json.loads(router.vtysh_cmd("show ip bgp neighbor 192.168.255.2 json")) + def _bgp_converge(): + 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 _bgp_converge(router): } return topotest.json_cmp(output, expected) - def _bgp_has_aggregated_route_with_stripped_as_set(router): - output = json.loads(router.vtysh_cmd("show ip bgp 172.16.0.0/16 json")) - expected = { - "paths": [{"aspath": {"string": "Local", "segments": [], "length": 0}}] - } - return topotest.json_cmp(output, expected) + test_func = functools.partial(_bgp_converge) + _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + assert result is None, "Failed bgp convergence at r2" - def _bgp_announce_route_without_as_sets(router): - output = json.loads( - router.vtysh_cmd( - "show ip bgp neighbor 192.168.254.2 advertised-routes json" - ) - ) + def _bgp_has_aggregated_route(): + output = json.loads(r2.vtysh_cmd("show ip bgp 172.16.0.0/16 json")) expected = { - "advertisedRoutes": { - "172.16.0.0/16": {"path": ""}, - "192.168.254.0/30": {"path": "65003"}, - "192.168.255.0/30": {"path": "65001"}, - }, - "totalPrefixCounter": 3, + "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) - test_func = functools.partial(_bgp_converge, router) + test_func = functools.partial(_bgp_has_aggregated_route) _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + assert result is None, "Failed to see an aggregated route at r2" - assert result is None, 'Failed bgp convergence in "{}"'.format(router) + def _bgp_announce_route_without_as_sets(): + output = json.loads(r4.vtysh_cmd("show ip bgp 172.16.0.0/16 json")) + expected = { + "paths": [ + { + "aspath": { + "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_has_aggregated_route_with_stripped_as_set, router - ) + 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" - assert result is None, 'Failed to see an aggregated route in "{}"'.format(router) + 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, + } + ], + }, + "totalRoutes": 3, + "totalPaths": 4, + } + return topotest.json_cmp(output, expected) - test_func = functools.partial(_bgp_announce_route_without_as_sets, 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 - ), '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__": From 28178dde4cd2d0f59c5c8e167382418974f558e6 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 30 Jan 2025 11:37:43 +0200 Subject: [PATCH 3/3] doc: Add more details for `bgp reject-as-sets` command Signed-off-by: Donatas Abraitis --- doc/user/bgp.rst | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/user/bgp.rst b/doc/user/bgp.rst index 1493c2fb9830..c6c3a6de711e 100644 --- a/doc/user/bgp.rst +++ b/doc/user/bgp.rst @@ -535,6 +535,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 ---------------- @@ -2938,7 +2945,7 @@ BGP Extended Communities in Route Map ``CO:COLOR`` This is a format to define colors value. ``CO`` part is always 00 (default), - it can be used to support the requirements of Color-Only steering when using + it can be used to support the requirements of Color-Only steering when using a Null Endpoint in the SR-TE Policy as specified in Section 8.8 of [RFC9256]. The below shows in detail what the different combinations of ``CO`` bits can match on to for the purpose of determining what type of SR-TE Policy Tunnel