From 365ab2e74b0ebfd98311f7edb76016b946f0018e Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Sun, 18 Oct 2020 19:17:02 -0300 Subject: [PATCH 1/5] bgpd: aggregate address suppress more specific Add new aggregate-address option to selectively suppress routes based on route map results. Signed-off-by: Rafael Zalamena --- bgpd/bgp_route.c | 146 ++++++++++++++++++++++++++++++++++++++++++-- bgpd/bgp_route.h | 5 ++ bgpd/bgp_routemap.c | 10 +++ 3 files changed, 155 insertions(+), 6 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 73f5526fe2..9f7c04423e 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6177,11 +6177,46 @@ static struct bgp_aggregate *bgp_aggregate_new(void) static void bgp_aggregate_free(struct bgp_aggregate *aggregate) { + XFREE(MTYPE_ROUTE_MAP_NAME, aggregate->suppress_map_name); + route_map_counter_decrement(aggregate->suppress_map); XFREE(MTYPE_ROUTE_MAP_NAME, aggregate->rmap.name); route_map_counter_decrement(aggregate->rmap.map); XFREE(MTYPE_BGP_AGGREGATE, aggregate); } +/** + * Helper function to avoid repeated code: prepare variables for a + * `route_map_apply` call. + * + * \returns `true` on route map match, otherwise `false`. + */ +static bool aggr_suppress_map_test(struct bgp *bgp, + struct bgp_aggregate *aggregate, + struct bgp_path_info *pi) +{ + const struct prefix *p = bgp_dest_get_prefix(pi->net); + route_map_result_t rmr = RMAP_DENYMATCH; + struct bgp_path_info rmap_path = {}; + struct attr attr = {}; + + /* No route map entries created, just don't match. */ + if (aggregate->suppress_map == NULL) + return false; + + /* Call route map matching and return result. */ + attr.aspath = aspath_empty(); + rmap_path.peer = bgp->peer_self; + rmap_path.attr = &attr; + + SET_FLAG(bgp->peer_self->rmap_type, PEER_RMAP_TYPE_AGGREGATE); + rmr = route_map_apply(aggregate->suppress_map, p, RMAP_BGP, &rmap_path); + bgp->peer_self->rmap_type = 0; + + bgp_attr_flush(&attr); + + return rmr == RMAP_PERMITMATCH; +} + static bool bgp_aggregate_info_same(struct bgp_path_info *pi, uint8_t origin, struct aspath *aspath, struct community *comm, @@ -6565,6 +6600,24 @@ void bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, match++; } + /* + * Suppress more specific routes that match the route + * map results. + * + * MED matching: + * Don't suppress routes if MED matching is enabled and + * it mismatched otherwise we might end up with no + * routes for this path. + */ + if (aggregate->suppress_map_name + && AGGREGATE_MED_VALID(aggregate) + && aggr_suppress_map_test(bgp, aggregate, pi)) { + (bgp_path_info_extra_get(pi))->suppress++; + bgp_path_info_set_flag(dest, pi, + BGP_PATH_ATTR_CHANGED); + match++; + } + aggregate->count++; /* @@ -6711,6 +6764,32 @@ void bgp_aggregate_delete(struct bgp *bgp, const struct prefix *p, afi_t afi, match++; } } + + if (aggregate->suppress_map_name + && AGGREGATE_MED_VALID(aggregate) + && aggr_suppress_map_test(bgp, aggregate, pi)) { + /* + * We can only get here if we were suppressed + * before: it is a failure to not have + * `pi->extra`. + */ + assert(pi->extra != NULL); + /* + * If we suppressed before then the value must + * be greater than zero. + */ + assert(pi->extra->suppress > 0); + + pi->extra->suppress--; + /* Unsuppress route if we reached `0`. */ + if (pi->extra->suppress == 0) { + bgp_path_info_set_flag( + dest, pi, + BGP_PATH_ATTR_CHANGED); + match++; + } + } + aggregate->count--; if (pi->attr->origin == BGP_ORIGIN_INCOMPLETE) @@ -6803,6 +6882,10 @@ static void bgp_add_route_to_aggregate(struct bgp *bgp, if (aggregate->summary_only && AGGREGATE_MED_VALID(aggregate)) (bgp_path_info_extra_get(pinew))->suppress++; + if (aggregate->suppress_map_name && AGGREGATE_MED_VALID(aggregate) + && aggr_suppress_map_test(bgp, aggregate, pinew)) + (bgp_path_info_extra_get(pinew))->suppress++; + switch (pinew->attr->origin) { case BGP_ORIGIN_INCOMPLETE: aggregate->incomplete_origin_count++; @@ -6908,8 +6991,30 @@ static void bgp_remove_route_from_aggregate(struct bgp *bgp, afi_t afi, } } + if (aggregate->suppress_map_name && AGGREGATE_MED_VALID(aggregate) + && aggr_suppress_map_test(bgp, aggregate, pi)) { + /* + * We can only get here if we were suppressed before: + * it is a failure to not have `pi->extra`. + */ + assert(pi->extra != NULL); + /* + * If we suppressed before then the value must be + * greater than zero. + */ + assert(pi->extra->suppress > 0); + + pi->extra->suppress--; + /* Unsuppress when we reached `0`. */ + if (pi->extra->suppress == 0) { + bgp_path_info_set_flag(pi->net, pi, + BGP_PATH_ATTR_CHANGED); + match++; + } + } + /* - * This must be called after `summary` check to avoid + * This must be called after `summary`, `suppress-map` check to avoid * "unsuppressing" twice. */ if (aggregate->match_med) @@ -7172,7 +7277,8 @@ static int bgp_aggregate_unset(struct vty *vty, const char *prefix_str, static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi, safi_t safi, const char *rmap, uint8_t summary_only, uint8_t as_set, - uint8_t origin, bool match_med) + uint8_t origin, bool match_med, + const char *suppress_map) { VTY_DECLVAR_CONTEXT(bgp, bgp); int ret; @@ -7181,6 +7287,12 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi, struct bgp_aggregate *aggregate; uint8_t as_set_new = as_set; + if (suppress_map && summary_only) { + vty_out(vty, + "'summary-only' and 'suppress-map' can't be used at the same time\n"); + return CMD_WARNING_CONFIG_FAILED; + } + /* Convert string to prefix structure. */ ret = str2prefix(prefix_str, &p); if (!ret) { @@ -7252,6 +7364,18 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi, aggregate->rmap.map = route_map_lookup_by_name(rmap); route_map_counter_increment(aggregate->rmap.map); } + + if (suppress_map) { + XFREE(MTYPE_ROUTE_MAP_NAME, aggregate->suppress_map_name); + route_map_counter_decrement(aggregate->suppress_map); + + aggregate->suppress_map_name = + XSTRDUP(MTYPE_ROUTE_MAP_NAME, suppress_map); + aggregate->suppress_map = + route_map_lookup_by_name(aggregate->suppress_map_name); + route_map_counter_increment(aggregate->suppress_map); + } + bgp_dest_set_bgp_aggregate_info(dest, aggregate); /* Aggregate address insert into BGP routing table. */ @@ -7267,6 +7391,7 @@ DEFPY(aggregate_addressv4, aggregate_addressv4_cmd, "|route-map WORD$rmap_name" "|origin $origin_s" "|matching-MED-only$match_med" + "|suppress-map WORD$suppress_map" "}", NO_STR "Configure BGP aggregate entries\n" @@ -7279,7 +7404,9 @@ DEFPY(aggregate_addressv4, aggregate_addressv4_cmd, "Remote EGP\n" "Local IGP\n" "Unknown heritage\n" - "Only aggregate routes with matching MED\n") + "Only aggregate routes with matching MED\n" + "Suppress the selected more specific routes\n" + "Route map with the route selectors\n") { const char *prefix_s = NULL; safi_t safi = bgp_node_safi(vty); @@ -7315,7 +7442,7 @@ DEFPY(aggregate_addressv4, aggregate_addressv4_cmd, return bgp_aggregate_set(vty, prefix_s, AFI_IP, safi, rmap_name, summary_only != NULL, as_set, origin, - match_med != NULL); + match_med != NULL, suppress_map); } DEFPY(aggregate_addressv6, aggregate_addressv6_cmd, @@ -7325,6 +7452,7 @@ DEFPY(aggregate_addressv6, aggregate_addressv6_cmd, "|route-map WORD$rmap_name" "|origin $origin_s" "|matching-MED-only$match_med" + "|suppress-map WORD$suppress_map" "}", NO_STR "Configure BGP aggregate entries\n" @@ -7337,7 +7465,9 @@ DEFPY(aggregate_addressv6, aggregate_addressv6_cmd, "Remote EGP\n" "Local IGP\n" "Unknown heritage\n" - "Only aggregate routes with matching MED\n") + "Only aggregate routes with matching MED\n" + "Suppress the selected more specific routes\n" + "Route map with the route selectors\n") { uint8_t origin = BGP_ORIGIN_UNSPECIFIED; int as_set = AGGREGATE_AS_UNSET; @@ -7361,7 +7491,7 @@ DEFPY(aggregate_addressv6, aggregate_addressv6_cmd, return bgp_aggregate_set(vty, prefix_str, AFI_IP6, SAFI_UNICAST, rmap_name, summary_only != NULL, as_set, - origin, match_med != NULL); + origin, match_med != NULL, suppress_map); } /* Redistribute route treatment. */ @@ -13949,6 +14079,10 @@ void bgp_config_write_network(struct vty *vty, struct bgp *bgp, afi_t afi, if (bgp_aggregate->match_med) vty_out(vty, " matching-MED-only"); + if (bgp_aggregate->suppress_map_name) + vty_out(vty, " suppress-map %s", + bgp_aggregate->suppress_map_name); + vty_out(vty, "\n"); } } diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 962a086081..40710ecf02 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -398,6 +398,11 @@ struct bgp_aggregate { #define AGGREGATE_MED_VALID(aggregate) \ (((aggregate)->match_med && !(aggregate)->med_mismatched) \ || !(aggregate)->match_med) + + /** Suppress map route map name (`NULL` when disabled). */ + char *suppress_map_name; + /** Suppress map route map pointer. */ + struct route_map *suppress_map; }; #define BGP_NEXTHOP_AFI_FROM_NHLEN(nhlen) \ diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index 968cda3f10..dd22a47c89 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -3845,6 +3845,16 @@ static void bgp_route_map_process_update(struct bgp *bgp, const char *rmap_name, if (!aggregate) continue; + /* Update suppress map pointer. */ + if (aggregate->suppress_map_name + && strmatch(aggregate->suppress_map_name, + rmap_name)) { + if (!aggregate->rmap.map) + route_map_counter_increment(map); + + aggregate->suppress_map = map; + } + if (!aggregate->rmap.name || (strcmp(rmap_name, aggregate->rmap.name) != 0)) continue; From 8fbb9c958183297f0ec8b868d58b5425f1e1505c Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Sun, 18 Oct 2020 19:18:37 -0300 Subject: [PATCH 2/5] doc: document new aggregate address option Add documentation for the newly implement aggregate address command. Signed-off-by: Rafael Zalamena --- doc/user/bgp.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/doc/user/bgp.rst b/doc/user/bgp.rst index fb0ca10da4..79e5afde7c 100644 --- a/doc/user/bgp.rst +++ b/doc/user/bgp.rst @@ -1005,6 +1005,12 @@ Route Aggregation-IPv4 Address Family Configure the aggregated address to only be created when the routes MED match, otherwise no aggregated route will be created. +.. index:: aggregate-address A.B.C.D/M suppress-map NAME +.. clicmd:: aggregate-address A.B.C.D/M suppress-map NAME + + Similar to `summary-only`, but will only suppress more specific routes that + are matched by the selected route-map. + .. index:: no aggregate-address A.B.C.D/M .. clicmd:: no aggregate-address A.B.C.D/M @@ -1063,6 +1069,11 @@ Route Aggregation-IPv6 Address Family Configure the aggregated address to only be created when the routes MED match, otherwise no aggregated route will be created. +.. index:: aggregate-address X:X::X:X/M suppress-map NAME +.. clicmd:: aggregate-address X:X::X:X/M suppress-map NAME + + Similar to `summary-only`, but will only suppress more specific routes that + are matched by the selected route-map. .. index:: no aggregate-address X:X::X:X/M .. clicmd:: no aggregate-address X:X::X:X/M From e84dfa4c2e5710b0f280ec7e077237c3dc4ea33a Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Sun, 18 Oct 2020 19:19:21 -0300 Subject: [PATCH 3/5] topotests: test aggregate address suppress map Add test for new aggregate address option: test aggregate address option without converged routes, then test again with a different route map with converged routes. Signed-off-by: Rafael Zalamena --- .../peer1/exabgp.cfg | 4 + .../bgp_aggregate_address_topo1/r1/bgpd.conf | 16 ++++ .../test_bgp_aggregate_address_topo1.py | 87 +++++++++++++++++++ 3 files changed, 107 insertions(+) diff --git a/tests/topotests/bgp_aggregate_address_topo1/peer1/exabgp.cfg b/tests/topotests/bgp_aggregate_address_topo1/peer1/exabgp.cfg index 277c6603ad..e0f6ab601f 100644 --- a/tests/topotests/bgp_aggregate_address_topo1/peer1/exabgp.cfg +++ b/tests/topotests/bgp_aggregate_address_topo1/peer1/exabgp.cfg @@ -13,5 +13,9 @@ neighbor 10.0.0.1 { route 192.168.1.1/32 next-hop 10.0.0.2 med 10; route 192.168.1.2/32 next-hop 10.0.0.2 med 10; route 192.168.1.3/32 next-hop 10.0.0.2 med 20; + + route 192.168.2.1/32 next-hop 10.0.0.2; + route 192.168.2.2/32 next-hop 10.0.0.2; + route 192.168.2.3/32 next-hop 10.0.0.2; } } diff --git a/tests/topotests/bgp_aggregate_address_topo1/r1/bgpd.conf b/tests/topotests/bgp_aggregate_address_topo1/r1/bgpd.conf index 4e1406177d..5fd6e8e83d 100644 --- a/tests/topotests/bgp_aggregate_address_topo1/r1/bgpd.conf +++ b/tests/topotests/bgp_aggregate_address_topo1/r1/bgpd.conf @@ -1,3 +1,18 @@ +access-list acl-sup-one seq 5 permit 192.168.2.1/32 +access-list acl-sup-one seq 10 deny any +! +access-list acl-sup-two seq 5 permit 192.168.2.2/32 +access-list acl-sup-two seq 10 deny any +! +access-list acl-sup-three seq 5 permit 192.168.2.3/32 +access-list acl-sup-three seq 10 deny any +! +route-map rm-sup-one permit 10 + match ip address acl-sup-one +! +route-map rm-sup-two permit 10 + match ip address acl-sup-two +! router bgp 65000 no bgp ebgp-requires-policy neighbor 10.0.0.2 remote-as 65001 @@ -8,5 +23,6 @@ router bgp 65000 redistribute connected aggregate-address 192.168.0.0/24 matching-MED-only aggregate-address 192.168.1.0/24 matching-MED-only + aggregate-address 192.168.2.0/24 suppress-map rm-sup-one exit-address-family ! diff --git a/tests/topotests/bgp_aggregate_address_topo1/test_bgp_aggregate_address_topo1.py b/tests/topotests/bgp_aggregate_address_topo1/test_bgp_aggregate_address_topo1.py index d3656b8701..3f3b71dea3 100644 --- a/tests/topotests/bgp_aggregate_address_topo1/test_bgp_aggregate_address_topo1.py +++ b/tests/topotests/bgp_aggregate_address_topo1/test_bgp_aggregate_address_topo1.py @@ -85,6 +85,20 @@ def teardown_module(mod): tgen.stop_topology() +def expect_route(router_name, routes_expected): + "Helper function to avoid repeated code." + tgen = get_topogen() + test_func = functools.partial( + topotest.router_json_cmp, + tgen.gears[router_name], + "show ip route json", + routes_expected, + ) + _, result = topotest.run_and_expect(test_func, None, count=120, wait=1) + assertmsg = '"{}" BGP convergence failure'.format(router_name) + assert result is None, assertmsg + + def test_expect_convergence(): "Test that BGP protocol converged." @@ -185,6 +199,79 @@ aggregate-address 192.168.1.0/24 matching-MED-only summary-only assert result is None, assertmsg +def test_bgp_aggregate_address_suppress_map(): + "Test that the command suppress-map works." + + tgen = get_topogen() + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + expect_route('r2', { + "192.168.2.0/24": [{"protocol": "bgp"}], + "192.168.2.1/32": None, + "192.168.2.2/32": [{"protocol": "bgp"}], + "192.168.2.3/32": [{"protocol": "bgp"}], + }) + + # Change route map and test again. + tgen.gears["r1"].vtysh_multicmd( + """ +configure terminal +router bgp 65000 +address-family ipv4 unicast +no aggregate-address 192.168.2.0/24 suppress-map rm-sup-one +aggregate-address 192.168.2.0/24 suppress-map rm-sup-two +""" + ) + + expect_route('r2', { + "192.168.2.0/24": [{"protocol": "bgp"}], + "192.168.2.1/32": [{"protocol": "bgp"}], + "192.168.2.2/32": None, + "192.168.2.3/32": [{"protocol": "bgp"}], + }) + + +def test_bgp_aggregate_address_suppress_map_update_route_map(): + "Test that the suppress-map late route map creation works." + tgen = get_topogen() + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + tgen.gears["r1"].vtysh_multicmd( + """ +configure terminal +router bgp 65000 +address-family ipv4 unicast +no aggregate-address 192.168.2.0/24 suppress-map rm-sup-two +aggregate-address 192.168.2.0/24 suppress-map rm-sup-three +""" + ) + + expect_route('r2', { + "192.168.2.0/24": [{"protocol": "bgp"}], + "192.168.2.1/32": [{"protocol": "bgp"}], + "192.168.2.2/32": [{"protocol": "bgp"}], + "192.168.2.3/32": [{"protocol": "bgp"}], + }) + + # Create missing route map and test again. + tgen.gears["r1"].vtysh_multicmd( + """ +configure terminal +route-map rm-sup-three permit 10 +match ip address acl-sup-three +""" + ) + + expect_route('r2', { + "192.168.2.0/24": [{"protocol": "bgp"}], + "192.168.2.1/32": [{"protocol": "bgp"}], + "192.168.2.2/32": [{"protocol": "bgp"}], + "192.168.2.3/32": None, + }) + + def test_memory_leak(): "Run the memory leak test and report results." tgen = get_topogen() From 4056a5f6a582ec5b3c44b3e2a6275845f5a859d1 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Wed, 21 Oct 2020 21:22:04 -0300 Subject: [PATCH 4/5] bgpd: route suppression refactory Instead of just counting the route suppressions, keep a reference for all aggregations that are doing it. It should help the with the following problems: - Which aggregation suppressed the route. - Double suppression - Double unsuppression - Avoids calling `bgp_process` if already suppressed/unsuppressed. - Easier code maintenance and understanding This also fixes a crash when modifying a route map that is associated with a working aggregate-address. Signed-off-by: Rafael Zalamena --- bgpd/bgp_route.c | 205 +++++++++++++++++++++++--------------------- bgpd/bgp_route.h | 8 +- bgpd/bgp_routemap.c | 26 ++++-- 3 files changed, 133 insertions(+), 106 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 9f7c04423e..c47b610584 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -115,6 +115,14 @@ DEFINE_HOOK(bgp_process, struct peer *peer, bool withdraw), (bgp, afi, safi, bn, peer, withdraw)) +/** Test if path is suppressed. */ +static bool bgp_path_suppressed(struct bgp_path_info *pi) +{ + if (pi->extra == NULL || pi->extra->aggr_suppressors == NULL) + return false; + + return listcount(pi->extra->aggr_suppressors) > 0; +} struct bgp_dest *bgp_afi_node_get(struct bgp_table *table, afi_t afi, safi_t safi, const struct prefix *p, @@ -1704,10 +1712,8 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi, } /* Aggregate-address suppress check. */ - if (pi->extra && pi->extra->suppress) - if (!UNSUPPRESS_MAP_NAME(filter)) { - return false; - } + if (bgp_path_suppressed(pi) && !UNSUPPRESS_MAP_NAME(filter)) + return false; /* * If we are doing VRF 2 VRF leaking via the import @@ -1944,7 +1950,7 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi, bgp_peer_as_override(bgp, afi, safi, peer, attr); /* Route map & unsuppress-map apply. */ - if (ROUTE_MAP_OUT_NAME(filter) || (pi->extra && pi->extra->suppress)) { + if (ROUTE_MAP_OUT_NAME(filter) || bgp_path_suppressed(pi)) { struct bgp_path_info rmap_path = {0}; struct bgp_path_info_extra dummy_rmap_path_extra = {0}; struct attr dummy_attr = {0}; @@ -1969,7 +1975,7 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi, SET_FLAG(peer->rmap_type, PEER_RMAP_TYPE_OUT); - if (pi->extra && pi->extra->suppress) + if (bgp_path_suppressed(pi)) ret = route_map_apply(UNSUPPRESS_MAP(filter), p, RMAP_BGP, &rmap_path); else @@ -6217,6 +6223,71 @@ static bool aggr_suppress_map_test(struct bgp *bgp, return rmr == RMAP_PERMITMATCH; } +/** Test whether the aggregation has suppressed this path or not. */ +static bool aggr_suppress_exists(struct bgp_aggregate *aggregate, + struct bgp_path_info *pi) +{ + if (pi->extra == NULL || pi->extra->aggr_suppressors == NULL) + return false; + + return listnode_lookup(pi->extra->aggr_suppressors, aggregate) != NULL; +} + +/** + * Suppress this path and keep the reference. + * + * \returns `true` if needs processing otherwise `false`. + */ +static bool aggr_suppress_path(struct bgp_aggregate *aggregate, + struct bgp_path_info *pi) +{ + struct bgp_path_info_extra *pie; + + /* Path is already suppressed by this aggregation. */ + if (aggr_suppress_exists(aggregate, pi)) + return false; + + pie = bgp_path_info_extra_get(pi); + + /* This is the first suppression, allocate memory and list it. */ + if (pie->aggr_suppressors == NULL) + pie->aggr_suppressors = list_new(); + + listnode_add(pie->aggr_suppressors, aggregate); + + /* Only mark for processing if suppressed. */ + if (listcount(pie->aggr_suppressors) == 1) { + bgp_path_info_set_flag(pi->net, pi, BGP_PATH_ATTR_CHANGED); + return true; + } + + return false; +} + +/** + * Unsuppress this path and remove the reference. + * + * \returns `true` if needs processing otherwise `false`. + */ +static bool aggr_unsuppress_path(struct bgp_aggregate *aggregate, + struct bgp_path_info *pi) +{ + /* Path wasn't suppressed. */ + if (!aggr_suppress_exists(aggregate, pi)) + return false; + + listnode_delete(pi->extra->aggr_suppressors, aggregate); + + /* Unsuppress and free extra memory if last item. */ + if (listcount(pi->extra->aggr_suppressors) == 0) { + list_delete(&pi->extra->aggr_suppressors); + bgp_path_info_set_flag(pi->net, pi, BGP_PATH_ATTR_CHANGED); + return true; + } + + return false; +} + static bool bgp_aggregate_info_same(struct bgp_path_info *pi, uint8_t origin, struct aspath *aspath, struct community *comm, @@ -6411,13 +6482,11 @@ static bool bgp_aggregate_test_all_med(struct bgp_aggregate *aggregate, * Toggles the route suppression status for this aggregate address * configuration. */ -static void bgp_aggregate_toggle_suppressed(struct bgp_aggregate *aggregate, - struct bgp *bgp, - const struct prefix *p, afi_t afi, - safi_t safi, bool suppress) +void bgp_aggregate_toggle_suppressed(struct bgp_aggregate *aggregate, + struct bgp *bgp, const struct prefix *p, + afi_t afi, safi_t safi, bool suppress) { struct bgp_table *table = bgp->rib[afi][safi]; - struct bgp_path_info_extra *pie; const struct prefix *dest_p; struct bgp_dest *dest, *top; struct bgp_path_info *pi; @@ -6438,32 +6507,17 @@ static void bgp_aggregate_toggle_suppressed(struct bgp_aggregate *aggregate, if (pi->sub_type == BGP_ROUTE_AGGREGATE) continue; - /* - * On installation it is possible that pi->extra is - * set to NULL, otherwise it must exists. - */ - assert(!suppress && pi->extra != NULL); - /* We are toggling suppression back. */ if (suppress) { - pie = bgp_path_info_extra_get(pi); /* Suppress route if not suppressed already. */ - pie->suppress++; - bgp_path_info_set_flag(dest, pi, - BGP_PATH_ATTR_CHANGED); - toggle_suppression = true; + if (aggr_suppress_path(aggregate, pi)) + toggle_suppression = true; continue; } - pie = pi->extra; - assert(pie->suppress > 0); - pie->suppress--; /* Install route if there is no more suppression. */ - if (pie->suppress == 0) { - bgp_path_info_set_flag(dest, pi, - BGP_PATH_ATTR_CHANGED); + if (aggr_unsuppress_path(aggregate, pi)) toggle_suppression = true; - } } if (toggle_suppression) @@ -6550,6 +6604,17 @@ void bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, if (aggregate->match_med) bgp_aggregate_test_all_med(aggregate, bgp, p, afi, safi); + /* + * Reset aggregate count: we might've been called from route map + * update so in that case we must retest all more specific routes. + * + * \see `bgp_route_map_process_update`. + */ + aggregate->count = 0; + aggregate->incomplete_origin_count = 0; + aggregate->incomplete_origin_count = 0; + aggregate->egp_origin_count = 0; + /* ORIGIN attribute: If at least one route among routes that are aggregated has ORIGIN with the value INCOMPLETE, then the aggregated route must have the ORIGIN attribute with the value @@ -6594,10 +6659,8 @@ void bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, */ if (aggregate->summary_only && AGGREGATE_MED_VALID(aggregate)) { - (bgp_path_info_extra_get(pi))->suppress++; - bgp_path_info_set_flag(dest, pi, - BGP_PATH_ATTR_CHANGED); - match++; + if (aggr_suppress_path(aggregate, pi)) + match++; } /* @@ -6612,10 +6675,8 @@ void bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, if (aggregate->suppress_map_name && AGGREGATE_MED_VALID(aggregate) && aggr_suppress_map_test(bgp, aggregate, pi)) { - (bgp_path_info_extra_get(pi))->suppress++; - bgp_path_info_set_flag(dest, pi, - BGP_PATH_ATTR_CHANGED); - match++; + if (aggr_suppress_path(aggregate, pi)) + match++; } aggregate->count++; @@ -6755,39 +6816,15 @@ void bgp_aggregate_delete(struct bgp *bgp, const struct prefix *p, afi_t afi, if (aggregate->summary_only && pi->extra && AGGREGATE_MED_VALID(aggregate)) { - pi->extra->suppress--; - - if (pi->extra->suppress == 0) { - bgp_path_info_set_flag( - dest, pi, - BGP_PATH_ATTR_CHANGED); + if (aggr_unsuppress_path(aggregate, pi)) match++; - } } if (aggregate->suppress_map_name && AGGREGATE_MED_VALID(aggregate) && aggr_suppress_map_test(bgp, aggregate, pi)) { - /* - * We can only get here if we were suppressed - * before: it is a failure to not have - * `pi->extra`. - */ - assert(pi->extra != NULL); - /* - * If we suppressed before then the value must - * be greater than zero. - */ - assert(pi->extra->suppress > 0); - - pi->extra->suppress--; - /* Unsuppress route if we reached `0`. */ - if (pi->extra->suppress == 0) { - bgp_path_info_set_flag( - dest, pi, - BGP_PATH_ATTR_CHANGED); + if (aggr_unsuppress_path(aggregate, pi)) match++; - } } aggregate->count--; @@ -6880,11 +6917,11 @@ static void bgp_add_route_to_aggregate(struct bgp *bgp, pinew, true); if (aggregate->summary_only && AGGREGATE_MED_VALID(aggregate)) - (bgp_path_info_extra_get(pinew))->suppress++; + aggr_suppress_path(aggregate, pinew); if (aggregate->suppress_map_name && AGGREGATE_MED_VALID(aggregate) && aggr_suppress_map_test(bgp, aggregate, pinew)) - (bgp_path_info_extra_get(pinew))->suppress++; + aggr_suppress_path(aggregate, pinew); switch (pinew->attr->origin) { case BGP_ORIGIN_INCOMPLETE: @@ -6980,38 +7017,14 @@ static void bgp_remove_route_from_aggregate(struct bgp *bgp, afi_t afi, if (pi->sub_type == BGP_ROUTE_AGGREGATE) return; - if (aggregate->summary_only && pi->extra && pi->extra->suppress > 0 - && AGGREGATE_MED_VALID(aggregate)) { - pi->extra->suppress--; - - if (pi->extra->suppress == 0) { - bgp_path_info_set_flag(pi->net, pi, - BGP_PATH_ATTR_CHANGED); + if (aggregate->summary_only && AGGREGATE_MED_VALID(aggregate)) + if (aggr_unsuppress_path(aggregate, pi)) match++; - } - } if (aggregate->suppress_map_name && AGGREGATE_MED_VALID(aggregate) - && aggr_suppress_map_test(bgp, aggregate, pi)) { - /* - * We can only get here if we were suppressed before: - * it is a failure to not have `pi->extra`. - */ - assert(pi->extra != NULL); - /* - * If we suppressed before then the value must be - * greater than zero. - */ - assert(pi->extra->suppress > 0); - - pi->extra->suppress--; - /* Unsuppress when we reached `0`. */ - if (pi->extra->suppress == 0) { - bgp_path_info_set_flag(pi->net, pi, - BGP_PATH_ATTR_CHANGED); + && aggr_suppress_map_test(bgp, aggregate, pi)) + if (aggr_unsuppress_path(aggregate, pi)) match++; - } - } /* * This must be called after `summary`, `suppress-map` check to avoid @@ -7802,7 +7815,7 @@ static void route_vty_short_status_out(struct vty *vty, if (CHECK_FLAG(path->flags, BGP_PATH_STALE)) json_object_boolean_true_add(json_path, "stale"); - if (path->extra && path->extra->suppress) + if (path->extra && bgp_path_suppressed(path)) json_object_boolean_true_add(json_path, "suppressed"); if (CHECK_FLAG(path->flags, BGP_PATH_VALID) @@ -7839,7 +7852,7 @@ static void route_vty_short_status_out(struct vty *vty, vty_out(vty, "R"); else if (CHECK_FLAG(path->flags, BGP_PATH_STALE)) vty_out(vty, "S"); - else if (path->extra && path->extra->suppress) + else if (bgp_path_suppressed(path)) vty_out(vty, "s"); else if (CHECK_FLAG(path->flags, BGP_PATH_VALID) && !CHECK_FLAG(path->flags, BGP_PATH_HISTORY)) @@ -10524,7 +10537,7 @@ void route_vty_out_detail_header(struct vty *vty, struct bgp *bgp, count++; if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) { best = count; - if (pi->extra && pi->extra->suppress) + if (bgp_path_suppressed(pi)) suppress = 1; if (pi->attr->community == NULL) diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 40710ecf02..b3efbbafd8 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -110,8 +110,8 @@ struct bgp_path_info_extra { /* Pointer to dampening structure. */ struct bgp_damp_info *damp_info; - /* This route is suppressed with aggregation. */ - int suppress; + /** List of aggregations that suppress this path. */ + struct list *aggr_suppressors; /* Nexthop reachability check. */ uint32_t igpmetric; @@ -715,4 +715,8 @@ extern bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, struct attr *attr, struct bgp_dest *dest); extern int bgp_evpn_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new, struct bgp_path_info *exist, int *paths_eq); +extern void bgp_aggregate_toggle_suppressed(struct bgp_aggregate *aggregate, + struct bgp *bgp, + const struct prefix *p, afi_t afi, + safi_t safi, bool suppress); #endif /* _QUAGGA_BGP_ROUTE_H */ diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index dd22a47c89..5065940967 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -3746,6 +3746,7 @@ static void bgp_route_map_process_update(struct bgp *bgp, const char *rmap_name, int route_update) { int i; + bool matched; afi_t afi; safi_t safi; struct peer *peer; @@ -3845,26 +3846,35 @@ static void bgp_route_map_process_update(struct bgp *bgp, const char *rmap_name, if (!aggregate) continue; + matched = false; + /* Update suppress map pointer. */ if (aggregate->suppress_map_name && strmatch(aggregate->suppress_map_name, rmap_name)) { - if (!aggregate->rmap.map) + if (aggregate->rmap.map == NULL) route_map_counter_increment(map); aggregate->suppress_map = map; + + bgp_aggregate_toggle_suppressed( + aggregate, bgp, bgp_dest_get_prefix(bn), + afi, safi, false); + + matched = true; } - if (!aggregate->rmap.name - || (strcmp(rmap_name, aggregate->rmap.name) != 0)) - continue; + if (aggregate->rmap.name + && strmatch(rmap_name, aggregate->rmap.name)) { + if (aggregate->rmap.map == NULL) + route_map_counter_increment(map); - if (!aggregate->rmap.map) - route_map_counter_increment(map); + aggregate->rmap.map = map; - aggregate->rmap.map = map; + matched = true; + } - if (route_update) { + if (matched && route_update) { const struct prefix *bn_p = bgp_dest_get_prefix(bn); From 6ba6de7e172afdbbd82cb2a3eb1f6b091900d1ea Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Wed, 21 Oct 2020 21:48:43 -0300 Subject: [PATCH 5/5] bgpd,topotests: log route suppression messages To see the messages activate the BGP debug: `debug bgp updates`. Signed-off-by: Rafael Zalamena --- bgpd/bgp_route.c | 8 ++++++++ tests/topotests/bgp_aggregate_address_topo1/r1/bgpd.conf | 2 ++ 2 files changed, 10 insertions(+) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index c47b610584..1edcbc39b6 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6257,6 +6257,10 @@ static bool aggr_suppress_path(struct bgp_aggregate *aggregate, /* Only mark for processing if suppressed. */ if (listcount(pie->aggr_suppressors) == 1) { + if (BGP_DEBUG(update, UPDATE_OUT)) + zlog_debug("aggregate-address suppressing: %pFX", + bgp_dest_get_prefix(pi->net)); + bgp_path_info_set_flag(pi->net, pi, BGP_PATH_ATTR_CHANGED); return true; } @@ -6280,6 +6284,10 @@ static bool aggr_unsuppress_path(struct bgp_aggregate *aggregate, /* Unsuppress and free extra memory if last item. */ if (listcount(pi->extra->aggr_suppressors) == 0) { + if (BGP_DEBUG(update, UPDATE_OUT)) + zlog_debug("aggregate-address unsuppressing: %pFX", + bgp_dest_get_prefix(pi->net)); + list_delete(&pi->extra->aggr_suppressors); bgp_path_info_set_flag(pi->net, pi, BGP_PATH_ATTR_CHANGED); return true; diff --git a/tests/topotests/bgp_aggregate_address_topo1/r1/bgpd.conf b/tests/topotests/bgp_aggregate_address_topo1/r1/bgpd.conf index 5fd6e8e83d..fa52150085 100644 --- a/tests/topotests/bgp_aggregate_address_topo1/r1/bgpd.conf +++ b/tests/topotests/bgp_aggregate_address_topo1/r1/bgpd.conf @@ -1,3 +1,5 @@ +debug bgp updates +! access-list acl-sup-one seq 5 permit 192.168.2.1/32 access-list acl-sup-one seq 10 deny any !