From ee1986f1b5ae6b94b446b12e1b77cc30d8f5f46d Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 30 Jan 2024 15:44:38 +0200 Subject: [PATCH 1/2] bgpd: Reinstall aggregated routes if using route-maps and it was changed Without this change when we change the route-map, we never reinstall the route if the route-map has changed. We checked only some attributes like aspath, communities, large-communities, extended-communities, but ignoring the rest of attributes. With this change, let's check if the route-map has changed. bgp_route_map_process_update() is triggered on route-map change, and we set `changed` to true, which treats aggregated route as not the same as it was before. Signed-off-by: Donatas Abraitis --- bgpd/bgp_route.c | 6 ++++-- bgpd/bgp_route.h | 1 + bgpd/bgp_routemap.c | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 8bcbd3dd8cea..518e848258ff 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -7369,8 +7369,9 @@ static void bgp_aggregate_install( * If the aggregate information has not changed * no need to re-install it again. */ - if (pi && bgp_aggregate_info_same(pi, origin, aspath, community, - ecommunity, lcommunity)) { + if (pi && (!aggregate->rmap.changed && + bgp_aggregate_info_same(pi, origin, aspath, community, + ecommunity, lcommunity))) { bgp_dest_unlock_node(dest); if (aspath) @@ -8377,6 +8378,7 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi, aggregate->rmap.name = XSTRDUP(MTYPE_ROUTE_MAP_NAME, rmap); aggregate->rmap.map = route_map_lookup_by_name(rmap); + aggregate->rmap.changed = true; route_map_counter_increment(aggregate->rmap.map); } diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 0599e8dce1f0..935dafccf29f 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -419,6 +419,7 @@ struct bgp_aggregate { struct { char *name; struct route_map *map; + bool changed; } rmap; /* Suppress-count. */ diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index 382e8ae409f5..36e04c5e681e 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -4596,6 +4596,7 @@ static void bgp_route_map_process_update(struct bgp *bgp, const char *rmap_name, route_map_counter_increment(map); aggregate->rmap.map = map; + aggregate->rmap.changed = true; matched = true; } From 9aed5777b73f95df044f97add63506dd6fb45b43 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 30 Jan 2024 15:54:04 +0200 Subject: [PATCH 2/2] tests: Check if attributes are reapplied for aggregate routes with route-maps Signed-off-by: Donatas Abraitis --- .../test_bgp_aggregate-address_route-map.py | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/tests/topotests/bgp_aggregate_address_route_map/test_bgp_aggregate-address_route-map.py b/tests/topotests/bgp_aggregate_address_route_map/test_bgp_aggregate-address_route-map.py index cec06920cb8b..eeac7146b1fa 100644 --- a/tests/topotests/bgp_aggregate_address_route_map/test_bgp_aggregate-address_route-map.py +++ b/tests/topotests/bgp_aggregate_address_route_map/test_bgp_aggregate-address_route-map.py @@ -74,7 +74,8 @@ def test_bgp_maximum_prefix_invalid(): if tgen.routers_have_failure(): pytest.skip(tgen.errors) - router = tgen.gears["r2"] + r1 = tgen.gears["r1"] + r2 = tgen.gears["r2"] def _bgp_converge(router): output = json.loads(router.vtysh_cmd("show ip bgp neighbor 192.168.255.1 json")) @@ -86,22 +87,30 @@ def _bgp_converge(router): } return topotest.json_cmp(output, expected) - def _bgp_aggregate_address_has_metric(router): + def _bgp_aggregate_address_has_metric(router, metric): output = json.loads(router.vtysh_cmd("show ip bgp 172.16.255.0/24 json")) - expected = {"paths": [{"metric": 123}]} + expected = {"paths": [{"metric": metric}]} return topotest.json_cmp(output, expected) - test_func = functools.partial(_bgp_converge, router) - success, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) - - assert result is None, 'Failed to see bgp convergence in "{}"'.format(router) - - test_func = functools.partial(_bgp_aggregate_address_has_metric, router) - success, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) - - assert ( - result is None - ), 'Failed to see applied metric for aggregated prefix in "{}"'.format(router) + test_func = functools.partial(_bgp_converge, r2) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + assert result is None, "Failed to see bgp convergence in r2" + + test_func = functools.partial(_bgp_aggregate_address_has_metric, r2, 123) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + assert result is None, "Failed to see applied metric for aggregated prefix in r2" + + r1.vtysh_cmd( + """ + configure terminal + route-map aggr-rmap permit 10 + set metric 666 + """ + ) + + test_func = functools.partial(_bgp_aggregate_address_has_metric, r2, 666) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + assert result is None, "Failed to see applied metric for aggregated prefix in r2" if __name__ == "__main__":