From 2adac2562a02fb20cb5fc783c2e6019a1a76354f Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 12 Nov 2020 12:30:19 +0200 Subject: [PATCH] bgpd: Do not send BGP UPDATE if the route actually not changed Reference: https://www.cmand.org/communityexploration --y2-- / | \ c1 ---- x1 ---- y1 | z1 \ | / --y3-- 1. z1 announces 192.168.255.254/32 to y2, y3. 2. y2 and y3 tags this prefix at ingress with appropriate communities 65004:2 (y2) and 65004:3 (y3). 3. x1 filters all communities at the egress to c1. 4. Shutdown the link between y1 and y2. 5. y1 will generate a BGP UPDATE message regarding the next-hop change. 6. x1 will generate a BGP UPDATE message regarding community change. To avoid sending duplicate BGP UPDATE messages we should make sure we send only actual route updates. In this example, x1 will skip BGP UPDATE to c1 because the actual route is the same (filtered communities - nothing changes). Signed-off-by: Donatas Abraitis --- bgpd/bgp_advertise.h | 3 ++ bgpd/bgp_nb.c | 7 +++++ bgpd/bgp_nb.h | 4 +++ bgpd/bgp_nb_config.c | 21 +++++++++++++ bgpd/bgp_packet.c | 12 +++++++ bgpd/bgp_updgrp.c | 5 +++ bgpd/bgp_updgrp.h | 12 ++----- bgpd/bgp_updgrp_adv.c | 22 +++++++++++++ bgpd/bgp_vty.c | 67 ++++++++++++++++++++++++++++++++++++++++ bgpd/bgpd.h | 1 + yang/frr-bgp-common.yang | 7 +++++ 11 files changed, 152 insertions(+), 9 deletions(-) diff --git a/bgpd/bgp_advertise.h b/bgpd/bgp_advertise.h index f2cf78efde..745a0dffce 100644 --- a/bgpd/bgp_advertise.h +++ b/bgpd/bgp_advertise.h @@ -83,6 +83,9 @@ struct bgp_adj_out { /* Advertisement information. */ struct bgp_advertise *adv; + + /* Attribute hash */ + uint32_t attr_hash; }; RB_HEAD(bgp_adj_out_rb, bgp_adj_out); diff --git a/bgpd/bgp_nb.c b/bgpd/bgp_nb.c index f098332b29..08ec64242d 100644 --- a/bgpd/bgp_nb.c +++ b/bgpd/bgp_nb.c @@ -351,6 +351,13 @@ const struct frr_yang_module_info frr_bgp_info = { .modify = bgp_global_ebgp_requires_policy_modify, } }, + { + .xpath = "/frr-routing:routing/control-plane-protocols/control-plane-protocol/frr-bgp:bgp/global/suppress-duplicates", + .cbs = { + .cli_show = cli_show_router_bgp_suppress_duplicates, + .modify = bgp_global_suppress_duplicates_modify, + } + }, { .xpath = "/frr-routing:routing/control-plane-protocols/control-plane-protocol/frr-bgp:bgp/global/show-hostname", .cbs = { diff --git a/bgpd/bgp_nb.h b/bgpd/bgp_nb.h index f608d4f8c1..9c81c2457e 100644 --- a/bgpd/bgp_nb.h +++ b/bgpd/bgp_nb.h @@ -127,6 +127,7 @@ int bgp_global_fast_external_failover_modify(struct nb_cb_modify_args *args); int bgp_global_local_pref_modify(struct nb_cb_modify_args *args); int bgp_global_default_shutdown_modify(struct nb_cb_modify_args *args); int bgp_global_ebgp_requires_policy_modify(struct nb_cb_modify_args *args); +int bgp_global_suppress_duplicates_modify(struct nb_cb_modify_args *args); int bgp_global_show_hostname_modify(struct nb_cb_modify_args *args); int bgp_global_show_nexthop_hostname_modify(struct nb_cb_modify_args *args); int bgp_global_import_check_modify(struct nb_cb_modify_args *args); @@ -3639,6 +3640,9 @@ void cli_show_router_bgp_route_selection(struct vty *vty, void cli_show_router_bgp_ebgp_requires_policy(struct vty *vty, struct lyd_node *dnode, bool show_defaults); +void cli_show_router_bgp_suppress_duplicates(struct vty *vty, + struct lyd_node *dnode, + bool show_defaults); void cli_show_router_bgp_default_shutdown(struct vty *vty, struct lyd_node *dnode, bool show_defaults); diff --git a/bgpd/bgp_nb_config.c b/bgpd/bgp_nb_config.c index 6e7a1b650c..0991feb8e9 100644 --- a/bgpd/bgp_nb_config.c +++ b/bgpd/bgp_nb_config.c @@ -1597,6 +1597,27 @@ int bgp_global_ebgp_requires_policy_modify(struct nb_cb_modify_args *args) return NB_OK; } +/* + * XPath: + * /frr-routing:routing/control-plane-protocols/control-plane-protocol/frr-bgp:bgp/global/suppress-duplicates + */ +int bgp_global_suppress_duplicates_modify(struct nb_cb_modify_args *args) +{ + if (args->event != NB_EV_APPLY) + return NB_OK; + + struct bgp *bgp; + + bgp = nb_running_get_entry(args->dnode, NULL, true); + + if (yang_dnode_get_bool(args->dnode, NULL)) + SET_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_DUPLICATES); + else + UNSET_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_DUPLICATES); + + return NB_OK; +} + /* * XPath: * /frr-routing:routing/control-plane-protocols/control-plane-protocol/frr-bgp:bgp/global/show-hostname diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index cf7a265b11..2127c501a5 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -444,6 +444,13 @@ int bgp_generate_updgrp_packets(struct thread *thread) * yet. */ if (!next_pkt || !next_pkt->buffer) { + /* Make sure we supress BGP UPDATES + * for normal processing later again. + */ + if (!paf->t_announce_route) + UNSET_FLAG(paf->subgroup->sflags, + SUBGRP_STATUS_FORCE_UPDATES); + if (CHECK_FLAG(peer->cap, PEER_CAP_RESTART_RCV)) { if (!(PAF_SUBGRP(paf))->t_coalesce @@ -2116,6 +2123,11 @@ static int bgp_route_refresh_receive(struct peer *peer, bgp_size_t size) peer->orf_plist[afi][safi]; } + /* Avoid supressing duplicate routes later + * when processing in subgroup_announce_table(). + */ + SET_FLAG(paf->subgroup->sflags, SUBGRP_STATUS_FORCE_UPDATES); + /* If the peer is configured for default-originate clear the * SUBGRP_STATUS_DEFAULT_ORIGINATE flag so that we will * re-advertise the diff --git a/bgpd/bgp_updgrp.c b/bgpd/bgp_updgrp.c index 2788a8ea4f..059e05ef71 100644 --- a/bgpd/bgp_updgrp.c +++ b/bgpd/bgp_updgrp.c @@ -1387,6 +1387,11 @@ static int updgrp_policy_update_walkcb(struct update_group *updgrp, void *arg) } UPDGRP_FOREACH_SUBGRP (updgrp, subgrp) { + /* Avoid supressing duplicate routes later + * when processing in subgroup_announce_table(). + */ + SET_FLAG(subgrp->sflags, SUBGRP_STATUS_FORCE_UPDATES); + if (changed) { if (bgp_debug_update(NULL, NULL, updgrp, 0)) zlog_debug( diff --git a/bgpd/bgp_updgrp.h b/bgpd/bgp_updgrp.h index 9cad78c26d..7261933dc9 100644 --- a/bgpd/bgp_updgrp.h +++ b/bgpd/bgp_updgrp.h @@ -252,19 +252,13 @@ struct update_subgroup { uint64_t id; uint16_t sflags; +#define SUBGRP_STATUS_DEFAULT_ORIGINATE (1 << 0) +#define SUBGRP_STATUS_FORCE_UPDATES (1 << 1) - /* Subgroup flags, see below */ uint16_t flags; +#define SUBGRP_FLAG_NEEDS_REFRESH (1 << 0) }; -/* - * We need to do an outbound refresh to get this subgroup into a - * consistent state. - */ -#define SUBGRP_FLAG_NEEDS_REFRESH (1 << 0) - -#define SUBGRP_STATUS_DEFAULT_ORIGINATE (1 << 0) - /* * Add the given value to the specified counter on a subgroup and its * parent structures. diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index da9e1f28ae..b1ff9ac251 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -464,6 +464,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, struct peer *adv_peer; struct peer_af *paf; struct bgp *bgp; + uint32_t attr_hash = attrhash_key_make(attr); peer = SUBGRP_PEER(subgrp); afi = SUBGRP_AFI(subgrp); @@ -487,6 +488,26 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, return; } + /* Check if we are sending the same route. This is needed to + * avoid duplicate UPDATES. For instance, filtering communities + * at egress, neighbors will see duplicate UPDATES despite + * the route wasn't changed actually. + * Do not suppress BGP UPDATES for route-refresh. + */ + if (CHECK_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_DUPLICATES) + && !CHECK_FLAG(subgrp->sflags, SUBGRP_STATUS_FORCE_UPDATES) + && adj->attr_hash == attr_hash) { + if (BGP_DEBUG(update, UPDATE_OUT)) { + char attr_str[BUFSIZ] = {0}; + + bgp_dump_attr(attr, attr_str, sizeof(attr_str)); + + zlog_debug("%s suppress UPDATE w/ attr: %s", peer->host, + attr_str); + } + return; + } + if (adj->adv) bgp_advertise_clean_subgroup(subgrp, adj); adj->adv = bgp_advertise_new(); @@ -502,6 +523,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, else adv->baa = baa_new(); adv->adj = adj; + adj->attr_hash = attr_hash; /* Add new advertisement to advertisement attribute list. */ bgp_advertise_add(adv->baa, adv); diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 563a1faf96..bbd350b038 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -119,6 +119,10 @@ FRR_CFG_DEFAULT_BOOL(BGP_EBGP_REQUIRES_POLICY, { .val_bool = false, .match_version = "< 7.4", }, { .val_bool = true }, ) +FRR_CFG_DEFAULT_BOOL(BGP_SUPPRESS_DUPLICATES, + { .val_bool = false, .match_version = "< 7.6", }, + { .val_bool = true }, +) DEFINE_HOOK(bgp_inst_config_write, (struct bgp *bgp, struct vty *vty), @@ -475,6 +479,8 @@ int bgp_get_vty(struct bgp **bgp, as_t *as, const char *name, SET_FLAG((*bgp)->flags, BGP_FLAG_DETERMINISTIC_MED); if (DFLT_BGP_EBGP_REQUIRES_POLICY) SET_FLAG((*bgp)->flags, BGP_FLAG_EBGP_REQUIRES_POLICY); + if (DFLT_BGP_SUPPRESS_DUPLICATES) + SET_FLAG((*bgp)->flags, BGP_FLAG_SUPPRESS_DUPLICATES); ret = BGP_SUCCESS; } @@ -864,6 +870,7 @@ static int bgp_peer_clear(struct peer *peer, afi_t afi, safi_t safi, struct listnode **nnode, enum bgp_clear_type stype) { int ret = 0; + struct peer_af *paf; /* if afi/.safi not specified, spin thru all of them */ if ((afi == AFI_UNSPEC) && (safi == SAFI_UNSPEC)) { @@ -871,6 +878,11 @@ static int bgp_peer_clear(struct peer *peer, afi_t afi, safi_t safi, safi_t tmp_safi; FOREACH_AFI_SAFI (tmp_afi, tmp_safi) { + paf = peer_af_find(peer, tmp_afi, tmp_safi); + if (paf && paf->subgroup) + SET_FLAG(paf->subgroup->sflags, + SUBGRP_STATUS_FORCE_UPDATES); + if (!peer->afc[tmp_afi][tmp_safi]) continue; @@ -889,6 +901,11 @@ static int bgp_peer_clear(struct peer *peer, afi_t afi, safi_t safi, if (!peer->afc[afi][tmp_safi]) continue; + paf = peer_af_find(peer, afi, tmp_safi); + if (paf && paf->subgroup) + SET_FLAG(paf->subgroup->sflags, + SUBGRP_STATUS_FORCE_UPDATES); + if (stype == BGP_CLEAR_SOFT_NONE) ret = peer_clear(peer, nnode); else @@ -900,6 +917,11 @@ static int bgp_peer_clear(struct peer *peer, afi_t afi, safi_t safi, if (!peer->afc[afi][safi]) return 1; + paf = peer_af_find(peer, afi, safi); + if (paf && paf->subgroup) + SET_FLAG(paf->subgroup->sflags, + SUBGRP_STATUS_FORCE_UPDATES); + if (stype == BGP_CLEAR_SOFT_NONE) ret = peer_clear(peer, nnode); else @@ -2515,6 +2537,37 @@ DEFUN_YANG(no_bgp_always_compare_med, return nb_cli_apply_changes(vty, NULL); } +DEFUN_YANG(bgp_suppress_duplicates, + bgp_suppress_duplicates_cmd, + "bgp suppress-duplicates", + "BGP specific commands\n" + "Suppress duplicate updates if the route actually not changed\n") +{ + nb_cli_enqueue_change(vty, "./global/suppress-duplicates", + NB_OP_MODIFY, "true"); + return nb_cli_apply_changes(vty, NULL); +} + +DEFUN_YANG(no_bgp_suppress_duplicates, + no_bgp_suppress_duplicates_cmd, + "no bgp suppress-duplicates", + NO_STR + "BGP specific commands\n" + "Suppress duplicate updates if the route actually not changed\n") +{ + nb_cli_enqueue_change(vty, "./global/suppress-duplicates", + NB_OP_MODIFY, "false"); + return nb_cli_apply_changes(vty, NULL); +} + +void cli_show_router_bgp_suppress_duplicates(struct vty *vty, + struct lyd_node *dnode, + bool show_defaults) +{ + if (yang_dnode_get_bool(dnode, NULL) != SAVE_BGP_SUPPRESS_DUPLICATES) + vty_out(vty, " bgp suppress-duplicates\n"); +} + DEFUN_YANG(bgp_ebgp_requires_policy, bgp_ebgp_requires_policy_cmd, "bgp ebgp-requires-policy", @@ -16985,6 +17038,16 @@ int bgp_config_write(struct vty *vty) if (bgp->reject_as_sets) vty_out(vty, " bgp reject-as-sets\n"); + /* Suppress duplicate updates if the route actually not changed + */ + if (!!CHECK_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_DUPLICATES) + != SAVE_BGP_SUPPRESS_DUPLICATES) + vty_out(vty, " %sbgp suppress-duplicates\n", + CHECK_FLAG(bgp->flags, + BGP_FLAG_SUPPRESS_DUPLICATES) + ? "" + : "no "); + /* BGP default ipv4-unicast. */ if (CHECK_FLAG(bgp->flags, BGP_FLAG_NO_DEFAULT_IPV4)) vty_out(vty, " no bgp default ipv4-unicast\n"); @@ -17563,6 +17626,10 @@ void bgp_vty_init(void) install_element(BGP_NODE, &bgp_ebgp_requires_policy_cmd); install_element(BGP_NODE, &no_bgp_ebgp_requires_policy_cmd); + /* bgp suppress-duplicates */ + install_element(BGP_NODE, &bgp_suppress_duplicates_cmd); + install_element(BGP_NODE, &no_bgp_suppress_duplicates_cmd); + /* bgp reject-as-sets */ install_element(BGP_NODE, &bgp_reject_as_sets_cmd); install_element(BGP_NODE, &no_bgp_reject_as_sets_cmd); diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index e867159fa6..fd739d2d65 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -460,6 +460,7 @@ struct bgp { /* This flag is set if the instance is in administrative shutdown */ #define BGP_FLAG_SHUTDOWN (1 << 27) #define BGP_FLAG_SUPPRESS_FIB_PENDING (1 << 28) +#define BGP_FLAG_SUPPRESS_DUPLICATES (1 << 29) enum global_mode GLOBAL_GR_FSM[BGP_GLOBAL_GR_MODE] [BGP_GLOBAL_GR_EVENT_CMD]; diff --git a/yang/frr-bgp-common.yang b/yang/frr-bgp-common.yang index cb1a6a8f56..1840e3728c 100644 --- a/yang/frr-bgp-common.yang +++ b/yang/frr-bgp-common.yang @@ -358,6 +358,13 @@ submodule frr-bgp-common { "Apply administrative shutdown to newly configured peers."; } + leaf suppress-duplicates { + type boolean; + default "true"; + description + "Suppress duplicate updates if the route actually not changed."; + } + leaf ebgp-requires-policy { type boolean; default "true"; -- 2.39.5