From 2adac2562a02fb20cb5fc783c2e6019a1a76354f Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 12 Nov 2020 12:30:19 +0200 Subject: [PATCH 1/3] 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"; From 2beb3d396729ba480801ed9d1edaf7bb2cb38359 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 12 Nov 2020 12:31:30 +0200 Subject: [PATCH 2/3] tests: Check if we are not sending duplicate BGP UPDATEs Signed-off-by: Donatas Abraitis --- .../bgp_community_change_update/__init__.py | 0 .../bgp_community_change_update/c1/bgpd.conf | 11 + .../bgp_community_change_update/c1/zebra.conf | 6 + .../test_bgp_community_change_update.py | 225 ++++++++++++++++++ .../bgp_community_change_update/x1/bgpd.conf | 20 ++ .../bgp_community_change_update/x1/zebra.conf | 9 + .../bgp_community_change_update/y1/bgpd.conf | 12 + .../bgp_community_change_update/y1/zebra.conf | 12 + .../bgp_community_change_update/y2/bgpd.conf | 18 ++ .../bgp_community_change_update/y2/zebra.conf | 12 + .../bgp_community_change_update/y3/bgpd.conf | 18 ++ .../bgp_community_change_update/y3/zebra.conf | 12 + .../bgp_community_change_update/z1/bgpd.conf | 10 + .../bgp_community_change_update/z1/zebra.conf | 12 + 14 files changed, 377 insertions(+) create mode 100644 tests/topotests/bgp_community_change_update/__init__.py create mode 100644 tests/topotests/bgp_community_change_update/c1/bgpd.conf create mode 100644 tests/topotests/bgp_community_change_update/c1/zebra.conf create mode 100644 tests/topotests/bgp_community_change_update/test_bgp_community_change_update.py create mode 100644 tests/topotests/bgp_community_change_update/x1/bgpd.conf create mode 100644 tests/topotests/bgp_community_change_update/x1/zebra.conf create mode 100644 tests/topotests/bgp_community_change_update/y1/bgpd.conf create mode 100644 tests/topotests/bgp_community_change_update/y1/zebra.conf create mode 100644 tests/topotests/bgp_community_change_update/y2/bgpd.conf create mode 100644 tests/topotests/bgp_community_change_update/y2/zebra.conf create mode 100644 tests/topotests/bgp_community_change_update/y3/bgpd.conf create mode 100644 tests/topotests/bgp_community_change_update/y3/zebra.conf create mode 100644 tests/topotests/bgp_community_change_update/z1/bgpd.conf create mode 100644 tests/topotests/bgp_community_change_update/z1/zebra.conf diff --git a/tests/topotests/bgp_community_change_update/__init__.py b/tests/topotests/bgp_community_change_update/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/topotests/bgp_community_change_update/c1/bgpd.conf b/tests/topotests/bgp_community_change_update/c1/bgpd.conf new file mode 100644 index 0000000000..24cf9dffb9 --- /dev/null +++ b/tests/topotests/bgp_community_change_update/c1/bgpd.conf @@ -0,0 +1,11 @@ +! +debug bgp updates +! +router bgp 65001 + no bgp ebgp-requires-policy + neighbor 10.0.1.2 remote-as external + neighbor 10.0.1.2 timers 3 10 + address-family ipv4 unicast + redistribute connected + exit-address-family +! diff --git a/tests/topotests/bgp_community_change_update/c1/zebra.conf b/tests/topotests/bgp_community_change_update/c1/zebra.conf new file mode 100644 index 0000000000..e3dbbc051d --- /dev/null +++ b/tests/topotests/bgp_community_change_update/c1/zebra.conf @@ -0,0 +1,6 @@ +! +interface c1-eth0 + ip address 10.0.1.1/30 +! +ip forwarding +! diff --git a/tests/topotests/bgp_community_change_update/test_bgp_community_change_update.py b/tests/topotests/bgp_community_change_update/test_bgp_community_change_update.py new file mode 100644 index 0000000000..5fc4310266 --- /dev/null +++ b/tests/topotests/bgp_community_change_update/test_bgp_community_change_update.py @@ -0,0 +1,225 @@ +#!/usr/bin/env python + +# Copyright (c) 2020 by +# Donatas Abraitis +# +# Permission to use, copy, modify, and/or distribute this software +# for any purpose with or without fee is hereby granted, provided +# that the above copyright notice and this permission notice appear +# in all copies. +# +# THE SOFTWARE IS PROVIDED "AS IS" AND NETDEF DISCLAIMS ALL WARRANTIES +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL NETDEF BE LIABLE FOR +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY +# DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, +# WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS +# ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE +# OF THIS SOFTWARE. +# + +""" +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). +""" + +import os +import sys +import json +import pytest +import functools + +CWD = os.path.dirname(os.path.realpath(__file__)) +sys.path.append(os.path.join(CWD, "../")) + +# pylint: disable=C0413 +from lib import topotest +from lib.topogen import Topogen, TopoRouter, get_topogen +from lib.topolog import logger +from mininet.topo import Topo + +from lib.common_config import step +from time import sleep + + +class TemplateTopo(Topo): + def build(self, *_args, **_opts): + tgen = get_topogen(self) + + tgen.add_router("z1") + tgen.add_router("y1") + tgen.add_router("y2") + tgen.add_router("y3") + tgen.add_router("x1") + tgen.add_router("c1") + + # 10.0.1.0/30 + switch = tgen.add_switch("s1") + switch.add_link(tgen.gears["c1"]) + switch.add_link(tgen.gears["x1"]) + + # 10.0.2.0/30 + switch = tgen.add_switch("s2") + switch.add_link(tgen.gears["x1"]) + switch.add_link(tgen.gears["y1"]) + + # 10.0.3.0/30 + switch = tgen.add_switch("s3") + switch.add_link(tgen.gears["y1"]) + switch.add_link(tgen.gears["y2"]) + + # 10.0.4.0/30 + switch = tgen.add_switch("s4") + switch.add_link(tgen.gears["y1"]) + switch.add_link(tgen.gears["y3"]) + + # 10.0.5.0/30 + switch = tgen.add_switch("s5") + switch.add_link(tgen.gears["y2"]) + switch.add_link(tgen.gears["y3"]) + + # 10.0.6.0/30 + switch = tgen.add_switch("s6") + switch.add_link(tgen.gears["y2"]) + switch.add_link(tgen.gears["z1"]) + + # 10.0.7.0/30 + switch = tgen.add_switch("s7") + switch.add_link(tgen.gears["y3"]) + switch.add_link(tgen.gears["z1"]) + + +def setup_module(mod): + tgen = Topogen(TemplateTopo, mod.__name__) + tgen.start_topology() + + router_list = tgen.routers() + + for i, (rname, router) in enumerate(router_list.items(), 1): + router.load_config( + TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname)) + ) + router.load_config( + TopoRouter.RD_BGP, os.path.join(CWD, "{}/bgpd.conf".format(rname)) + ) + + tgen.start_router() + + +def teardown_module(mod): + tgen = get_topogen() + tgen.stop_topology() + + +def test_bgp_community_update_path_change(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + def _bgp_converge_initial(): + output = json.loads( + tgen.gears["c1"].vtysh_cmd("show ip bgp neighbor 10.0.1.2 json") + ) + expected = { + "10.0.1.2": { + "bgpState": "Established", + "addressFamilyInfo": {"ipv4Unicast": {"acceptedPrefixCounter": 8}}, + } + } + return topotest.json_cmp(output, expected) + + step("Check if an initial topology is converged") + test_func = functools.partial(_bgp_converge_initial) + success, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + assert result is None, "Failed to see bgp convergence in c1" + + step("Disable link between y1 and y2") + tgen.gears["y1"].run("ip link set dev y1-eth1 down") + + def _bgp_converge_link_disabled(): + output = json.loads(tgen.gears["y1"].vtysh_cmd("show ip bgp nei 10.0.3.2 json")) + expected = {"10.0.3.2": {"bgpState": "Active"}} + return topotest.json_cmp(output, expected) + + step("Check if a topology is converged after a link down between y1 and y2") + test_func = functools.partial(_bgp_converge_link_disabled) + success, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + assert result is None, "Failed to see bgp convergence in y1" + + def _bgp_check_for_duplicate_updates(): + duplicate = False + i = 0 + while i < 5: + if ( + len( + tgen.gears["c1"].run( + 'grep "10.0.1.2 rcvd 192.168.255.254/32 IPv4 unicast...duplicate ignored" bgpd.log' + ) + ) + > 0 + ): + duplicate = True + i += 1 + sleep(0.5) + return duplicate + + step("Check if we see duplicate BGP UPDATE message in c1 (suppress-duplicates)") + assert ( + _bgp_check_for_duplicate_updates() == False + ), "Seen duplicate BGP UPDATE message in c1 from x1" + + step("Disable bgp suppress-duplicates at x1") + tgen.gears["x1"].run( + "vtysh -c 'conf' -c 'router bgp' -c 'no bgp suppress-duplicates'" + ) + + step("Enable link between y1 and y2") + tgen.gears["y1"].run("ip link set dev y1-eth1 up") + + def _bgp_converge_link_enabled(): + output = json.loads(tgen.gears["y1"].vtysh_cmd("show ip bgp nei 10.0.3.2 json")) + expected = { + "10.0.3.2": { + "bgpState": "Established", + "addressFamilyInfo": { + "ipv4Unicast": {"acceptedPrefixCounter": 5, "sentPrefixCounter": 4} + }, + } + } + return topotest.json_cmp(output, expected) + + step("Check if a topology is converged after a link up between y1 and y2") + test_func = functools.partial(_bgp_converge_link_enabled) + success, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + assert result is None, "Failed to see bgp convergence in y1" + + step( + "Check if we see duplicate BGP UPDATE message in c1 (no bgp suppress-duplicates)" + ) + assert ( + _bgp_check_for_duplicate_updates() == True + ), "Didn't see duplicate BGP UPDATE message in c1 from x1" + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args)) diff --git a/tests/topotests/bgp_community_change_update/x1/bgpd.conf b/tests/topotests/bgp_community_change_update/x1/bgpd.conf new file mode 100644 index 0000000000..8d7bcb948b --- /dev/null +++ b/tests/topotests/bgp_community_change_update/x1/bgpd.conf @@ -0,0 +1,20 @@ +! +debug bgp updates +! +router bgp 65002 + no bgp ebgp-requires-policy + neighbor 10.0.1.1 remote-as external + neighbor 10.0.1.1 timers 3 10 + neighbor 10.0.2.2 remote-as external + neighbor 10.0.2.2 timers 3 10 + address-family ipv4 unicast + redistribute connected + neighbor 10.0.1.1 route-map c1 out + exit-address-family +! +bgp community-list standard c1 seq 1 permit 65004:2 +bgp community-list standard c1 seq 2 permit 65004:3 +! +route-map c1 permit 10 + set comm-list c1 delete +! diff --git a/tests/topotests/bgp_community_change_update/x1/zebra.conf b/tests/topotests/bgp_community_change_update/x1/zebra.conf new file mode 100644 index 0000000000..8b7c03ffbf --- /dev/null +++ b/tests/topotests/bgp_community_change_update/x1/zebra.conf @@ -0,0 +1,9 @@ +! +interface x1-eth0 + ip address 10.0.1.2/30 +! +interface x1-eth1 + ip address 10.0.2.1/30 +! +ip forwarding +! diff --git a/tests/topotests/bgp_community_change_update/y1/bgpd.conf b/tests/topotests/bgp_community_change_update/y1/bgpd.conf new file mode 100644 index 0000000000..a010085835 --- /dev/null +++ b/tests/topotests/bgp_community_change_update/y1/bgpd.conf @@ -0,0 +1,12 @@ +router bgp 65003 + no bgp ebgp-requires-policy + neighbor 10.0.2.1 remote-as external + neighbor 10.0.2.1 timers 3 10 + neighbor 10.0.3.2 remote-as internal + neighbor 10.0.3.2 timers 3 10 + neighbor 10.0.4.2 remote-as internal + neighbor 10.0.4.2 timers 3 10 + address-family ipv4 unicast + redistribute connected + exit-address-family +! diff --git a/tests/topotests/bgp_community_change_update/y1/zebra.conf b/tests/topotests/bgp_community_change_update/y1/zebra.conf new file mode 100644 index 0000000000..4096f2a2e3 --- /dev/null +++ b/tests/topotests/bgp_community_change_update/y1/zebra.conf @@ -0,0 +1,12 @@ +! +interface y1-eth0 + ip address 10.0.2.2/30 +! +interface y1-eth1 + ip address 10.0.3.1/30 +! +interface y1-eth2 + ip address 10.0.4.1/30 +! +ip forwarding +! diff --git a/tests/topotests/bgp_community_change_update/y2/bgpd.conf b/tests/topotests/bgp_community_change_update/y2/bgpd.conf new file mode 100644 index 0000000000..27f1e81f7d --- /dev/null +++ b/tests/topotests/bgp_community_change_update/y2/bgpd.conf @@ -0,0 +1,18 @@ +router bgp 65003 + no bgp ebgp-requires-policy + neighbor 10.0.3.1 remote-as internal + neighbor 10.0.3.1 timers 3 10 + neighbor 10.0.5.2 remote-as internal + neighbor 10.0.5.2 timers 3 10 + neighbor 10.0.6.2 remote-as external + neighbor 10.0.6.2 timers 3 10 + address-family ipv4 unicast + redistribute connected + neighbor 10.0.3.1 route-reflector-client + neighbor 10.0.5.2 route-reflector-client + neighbor 10.0.6.2 route-map z1 in + exit-address-family +! +route-map z1 permit 10 + set community 65004:2 +! diff --git a/tests/topotests/bgp_community_change_update/y2/zebra.conf b/tests/topotests/bgp_community_change_update/y2/zebra.conf new file mode 100644 index 0000000000..7e9458a45c --- /dev/null +++ b/tests/topotests/bgp_community_change_update/y2/zebra.conf @@ -0,0 +1,12 @@ +! +interface y2-eth0 + ip address 10.0.3.2/30 +! +interface y2-eth1 + ip address 10.0.5.1/30 +! +interface y2-eth2 + ip address 10.0.6.1/30 +! +ip forwarding +! diff --git a/tests/topotests/bgp_community_change_update/y3/bgpd.conf b/tests/topotests/bgp_community_change_update/y3/bgpd.conf new file mode 100644 index 0000000000..8e57284929 --- /dev/null +++ b/tests/topotests/bgp_community_change_update/y3/bgpd.conf @@ -0,0 +1,18 @@ +router bgp 65003 + no bgp ebgp-requires-policy + neighbor 10.0.4.1 remote-as internal + neighbor 10.0.4.1 timers 3 10 + neighbor 10.0.5.1 remote-as internal + neighbor 10.0.5.1 timers 3 10 + neighbor 10.0.7.2 remote-as external + neighbor 10.0.7.2 timers 3 10 + address-family ipv4 unicast + redistribute connected + neighbor 10.0.4.1 route-reflector-client + neighbor 10.0.5.1 route-reflector-client + neighbor 10.0.7.2 route-map z1 in + exit-address-family +! +route-map z1 permit 10 + set community 65004:3 +! diff --git a/tests/topotests/bgp_community_change_update/y3/zebra.conf b/tests/topotests/bgp_community_change_update/y3/zebra.conf new file mode 100644 index 0000000000..93dd87dbd2 --- /dev/null +++ b/tests/topotests/bgp_community_change_update/y3/zebra.conf @@ -0,0 +1,12 @@ +! +interface y3-eth0 + ip address 10.0.4.2/30 +! +interface y3-eth1 + ip address 10.0.5.2/30 +! +interface y3-eth2 + ip address 10.0.7.1/30 +! +ip forwarding +! diff --git a/tests/topotests/bgp_community_change_update/z1/bgpd.conf b/tests/topotests/bgp_community_change_update/z1/bgpd.conf new file mode 100644 index 0000000000..2f470f18b8 --- /dev/null +++ b/tests/topotests/bgp_community_change_update/z1/bgpd.conf @@ -0,0 +1,10 @@ +router bgp 65004 + no bgp ebgp-requires-policy + neighbor 10.0.6.1 remote-as external + neighbor 10.0.6.1 timers 3 10 + neighbor 10.0.7.1 remote-as external + neighbor 10.0.7.1 timers 3 10 + address-family ipv4 unicast + redistribute connected + exit-address-family +! diff --git a/tests/topotests/bgp_community_change_update/z1/zebra.conf b/tests/topotests/bgp_community_change_update/z1/zebra.conf new file mode 100644 index 0000000000..7f6bbb66b3 --- /dev/null +++ b/tests/topotests/bgp_community_change_update/z1/zebra.conf @@ -0,0 +1,12 @@ +! +interface lo + ip address 192.168.255.254/32 +! +interface z1-eth0 + ip address 10.0.6.2/30 +! +interface z1-eth1 + ip address 10.0.7.2/30 +! +ip forwarding +! From 105227afe78099a2e7ac830343df8a36424361b7 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 30 Nov 2020 21:27:35 +0200 Subject: [PATCH 3/3] doc: Add paragraph about `bgp suppress-duplicates` command Signed-off-by: Donatas Abraitis --- doc/user/bgp.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/doc/user/bgp.rst b/doc/user/bgp.rst index 73c286631a..1ee8f885d5 100644 --- a/doc/user/bgp.rst +++ b/doc/user/bgp.rst @@ -454,6 +454,17 @@ 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. +Suppress duplicate updates +-------------------------- + +.. index:: [no] bgp suppress-duplicates +.. clicmd:: [no] bgp suppress-duplicates + + For example, BGP routers can generate multiple identical announcements with + empty community attributes if stripped at egress. This is an undesired behavior. + Suppress duplicate updates if the route actually not changed. + Default: enabled. + Disable checking if nexthop is connected on EBGP sessions ---------------------------------------------------------