From ef7c53e244d5d0cd7c0ba2536fc931abf28df656 Mon Sep 17 00:00:00 2001 From: Abhinay Ramesh Date: Wed, 31 Mar 2021 11:05:56 +0000 Subject: [PATCH 1/5] bgpd: vrf route leaking, fix the bgp instance delete and re-add Description: FRR doesn't re-install the routes, imported from a tenant VRF, when bgp instance for source vrf is deleted and re-added again. When bgp instance is removed and re-added, when import statement is already there, then route leaking stops between two VRFs. Every 'router bgp' command should trigger re-export of all the routes to the importing bgp vrf instances. When a router bgp is configured, there could be bgp vrf instance(s) importing routes from this newly configured bgp vrf instance. We need to export routes from configured bgp vrf to VPN. This can impact performance, whenever we are testing scale from vrf route-leaking perspective. We should not trigger re-export for already existing bgp vrf instances. Co-authored-by: Santosh P K Co-authored-by: Kantesh Mundaragi Signed-off-by: Abhinay Ramesh --- bgpd/bgp_nb_config.c | 7 ++++++- bgpd/bgpd.c | 2 +- bgpd/bgpd.h | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_nb_config.c b/bgpd/bgp_nb_config.c index 5a88bd08d9..94ff362d1a 100644 --- a/bgpd/bgp_nb_config.c +++ b/bgpd/bgp_nb_config.c @@ -123,7 +123,12 @@ int bgp_router_create(struct nb_cb_create_args *args) if (is_new_bgp && inst_type == BGP_INSTANCE_TYPE_DEFAULT) vpn_leak_postchange_all(); - if (inst_type == BGP_INSTANCE_TYPE_VRF) + /* + * Check if we need to export to other VRF(s). + * Leak the routes to importing bgp vrf instances, + * only when new bgp vrf instance is configured. + */ + if (ret != BGP_INSTANCE_EXISTS) bgp_vpn_leak_export(bgp); UNSET_FLAG(bgp->vrf_flags, BGP_VRF_AUTO); diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index d37b9fa48c..bad62f9946 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -3402,7 +3402,7 @@ int bgp_get(struct bgp **bgp_val, as_t *as, const char *name, return ret; case BGP_SUCCESS: if (*bgp_val) - return ret; + return BGP_INSTANCE_EXISTS; } bgp = bgp_create(as, name, inst_type); diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 51134dc8c5..f9aa62c682 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -1844,6 +1844,7 @@ enum bgp_clear_type { /* BGP error codes. */ #define BGP_SUCCESS 0 #define BGP_CREATED 1 +#define BGP_INSTANCE_EXISTS 2 #define BGP_ERR_INVALID_VALUE -1 #define BGP_ERR_INVALID_FLAG -2 #define BGP_ERR_INVALID_AS -3 From b19c4f0829d9841858b146688207975c8f0d2d05 Mon Sep 17 00:00:00 2001 From: Abhinay Ramesh Date: Wed, 31 Mar 2021 11:32:57 +0000 Subject: [PATCH 2/5] bgpd: vrf route leaking, fix vrf delete Description: Imported/leak-from routes do not get withdrawn/removed even if the source VRF is deleted. Deleting and re-adding a tenant vrf, does not refresh the RIB. Whenever VRF is deleted (bgp_vrf_disable), currently we are withdrawing leak-from-vrf and leak-to-vrf routes from vpn table for the vrf, which is deleted. But we are currently not withdrawing routes from leak-to vrfs. We should also withdraw leak-to routes from leak-to vrfs (calling vpn_leak_to_vrf_withdraw). Co-authored-by: Santosh P K Co-authored-by: Kantesh Mundaragi Signed-off-by: Abhinay Ramesh --- bgpd/bgp_mplsvpn.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index a902c63dea..b5828723a4 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -1027,6 +1027,8 @@ void vpn_leak_from_vrf_withdraw_all(struct bgp *bgp_vpn, /* to */ if (debug) zlog_debug("%s: deleting it", __func__); + /* withdraw from leak-to vrfs as well */ + vpn_leak_to_vrf_withdraw(bgp_vpn, bpi); bgp_aggregate_decrement( bgp_vpn, bgp_dest_get_prefix(bn), bpi, From 6b2433c63f7fd3027cea8e06ca45f85bd3eea6f2 Mon Sep 17 00:00:00 2001 From: Abhinay Ramesh Date: Thu, 1 Apr 2021 05:17:54 +0000 Subject: [PATCH 3/5] bgpd: vrf route leaking, fix vrf redistribute Description: After FRR restart, routes are not getting redistributed; when routes added first and then 'redistribute static' cmd is issued. During the frr restart, vrf_id will be unknown, so irrespective of redistribution, we set the redistribute vrf bitmap. Later, when we add a route and then issue 'redistribute' cmd, we check the redistribute vrf bitmap and return CMD_WARNING; zebra_redistribute_add also checks the redistribute vrf bitmap and returns. Instead of checking the redistribute vrf bitmap, always set it anyways. Co-authored-by: Santosh P K Co-authored-by: Kantesh Mundaragi Signed-off-by: Abhinay Ramesh --- bgpd/bgp_zebra.c | 3 --- zebra/redistribute.c | 17 ++++++----------- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index d1912db01f..ae0bf7fe92 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -1700,9 +1700,6 @@ int bgp_redistribute_set(struct bgp *bgp, afi_t afi, int type, redist_add_instance(&zclient->mi_redist[afi][type], instance); } else { - if (vrf_bitmap_check(zclient->redist[afi][type], bgp->vrf_id)) - return CMD_WARNING; - #ifdef ENABLE_BGP_VNC if (EVPN_ENABLED(bgp) && type == ZEBRA_ROUTE_VNC_DIRECT) { vnc_export_bgp_enable( diff --git a/zebra/redistribute.c b/zebra/redistribute.c index 9e675011ee..104f952b3b 100644 --- a/zebra/redistribute.c +++ b/zebra/redistribute.c @@ -347,17 +347,12 @@ void zebra_redistribute_add(ZAPI_HANDLER_ARGS) zvrf_id(zvrf), afi); } } else { - if (!vrf_bitmap_check(client->redist[afi][type], - zvrf_id(zvrf))) { - if (IS_ZEBRA_DEBUG_EVENT) - zlog_debug( - "%s: setting vrf %s(%u) redist bitmap", - __func__, VRF_LOGNAME(zvrf->vrf), - zvrf_id(zvrf)); - vrf_bitmap_set(client->redist[afi][type], - zvrf_id(zvrf)); - zebra_redistribute(client, type, 0, zvrf_id(zvrf), afi); - } + if (IS_ZEBRA_DEBUG_EVENT) + zlog_debug("%s: setting vrf %s(%u) redist bitmap", + __func__, VRF_LOGNAME(zvrf->vrf), + zvrf_id(zvrf)); + vrf_bitmap_set(client->redist[afi][type], zvrf_id(zvrf)); + zebra_redistribute(client, type, 0, zvrf_id(zvrf), afi); } stream_failure: From 8fac400c798c760bafbb4f4460a138b85d565ce7 Mon Sep 17 00:00:00 2001 From: Abhinay Ramesh Date: Thu, 1 Apr 2021 05:22:23 +0000 Subject: [PATCH 4/5] VRF-Lite: Fix export of withdraw-in-progress routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: Stale routes are seen in the bgp table(ipv4 and ipv6) RCA: Scenario1: Interface down and withdraw is in-progress. Router bgp config leading to re-leaking. Now, withdraw-in-progress routes, are again leaked to bgp vrf instance(s) importing routes. Whenever we see an interface down and corresponding address delete, while withdrawal of exported routes is in-progress, routes are marked as being removed and put into work queue. ‘router bgp’ config is updated, which triggers bgp_vpn_leak_export; which exports routes from configured bgp vrf to VPN. So withdraw-in-progress routes, are again leaked to bgp vrf instance(s) importing routes; leading to stale routes. Scenario2: - 'no import vrf non-default-vrf’ [in the default vrf] - bgp update from the peer withdrawing prefix [non-default vrf] - 'import vrf non-default-vrf’ [configured in the default vrf] While withdrawal of exported routes is in-progress, routes are marked as being removed and put into work queue, In the meantime, if import vrf is configured, which exports routes from configured bgp vrf to VPN. So withdraw-in-progress new routes, are again leaked to bgp vrf instance(s) importing routes; leading to stale routes. Fix: Whenever leaking routes (leak_update), for already existing routes, skip the routes with bgp_path_info marked as being removed. Also added the log message for the return. Co-authored-by: Santosh P K Co-authored-by: Kantesh Mundaragi Signed-off-by: Abhinay Ramesh --- bgpd/bgp_mplsvpn.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index b5828723a4..dc48a99471 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -540,6 +540,17 @@ leak_update(struct bgp *bgp, /* destination bgp instance */ if (bpi) { bool labelssame = labels_same(bpi, label, num_labels); + if (CHECK_FLAG(source_bpi->flags, BGP_PATH_REMOVED) + && CHECK_FLAG(bpi->flags, BGP_PATH_REMOVED)) { + if (debug) { + zlog_debug( + "%s: ->%s(s_flags: 0x%x b_flags: 0x%x): %pFX: Found route, being removed, not leaking", + __func__, bgp->name_pretty, + source_bpi->flags, bpi->flags, p); + } + return NULL; + } + if (attrhash_cmp(bpi->attr, new_attr) && labelssame && !CHECK_FLAG(bpi->flags, BGP_PATH_REMOVED)) { @@ -613,6 +624,16 @@ leak_update(struct bgp *bgp, /* destination bgp instance */ return bpi; } + if (CHECK_FLAG(source_bpi->flags, BGP_PATH_REMOVED)) { + if (debug) { + zlog_debug( + "%s: ->%s(s_flags: 0x%x): %pFX: New route, being removed, not leaking", + __func__, bgp->name_pretty, + source_bpi->flags, p); + } + return NULL; + } + new = info_make(ZEBRA_ROUTE_BGP, BGP_ROUTE_IMPORTED, 0, bgp->peer_self, new_attr, bn); From de7cee09ebec4ac4b0c8c13a1df323969b7fbb26 Mon Sep 17 00:00:00 2001 From: Abhinay Ramesh Date: Thu, 1 Apr 2021 05:39:57 +0000 Subject: [PATCH 5/5] bgpd: vrf route leaking, fix vpn router id update Description: Route leaking from default vrf to non-default vrf stops after frr restart. If the interface comes up after route leaking is configured, in the case of vpn router id update, we delete the ecommunity value and never reconfigure the rtlist. This results in skipping route leak to non-default vrfs (vpn to vrf). Router-id change that is not explicitly configured (a change from zebra, frr restart) should not replace a configured vpn RD/RT. Added few helpful debugs as well. Co-authored-by: Santosh P K Co-authored-by: Kantesh Mundaragi Signed-off-by: Abhinay Ramesh --- bgpd/bgp_ecommunity.c | 22 ++++++++++++--------- bgpd/bgp_mplsvpn.c | 45 ++++++++++++++++++++----------------------- 2 files changed, 34 insertions(+), 33 deletions(-) diff --git a/bgpd/bgp_ecommunity.c b/bgpd/bgp_ecommunity.c index 7f6f61e141..923c9b0d7e 100644 --- a/bgpd/bgp_ecommunity.c +++ b/bgpd/bgp_ecommunity.c @@ -1294,15 +1294,19 @@ bool ecommunity_del_val(struct ecommunity *ecom, struct ecommunity_val *eval) /* Delete the selected value */ ecom->size--; - p = XMALLOC(MTYPE_ECOMMUNITY_VAL, ecom->size * ecom->unit_size); - if (c != 0) - memcpy(p, ecom->val, c * ecom->unit_size); - if ((ecom->size - c) != 0) - memcpy(p + (c)*ecom->unit_size, - ecom->val + (c + 1) * ecom->unit_size, - (ecom->size - c) * ecom->unit_size); - XFREE(MTYPE_ECOMMUNITY_VAL, ecom->val); - ecom->val = p; + if (ecom->size) { + p = XMALLOC(MTYPE_ECOMMUNITY_VAL, ecom->size * ecom->unit_size); + if (c != 0) + memcpy(p, ecom->val, c * ecom->unit_size); + if ((ecom->size - c) != 0) + memcpy(p + (c)*ecom->unit_size, + ecom->val + (c + 1) * ecom->unit_size, + (ecom->size - c) * ecom->unit_size); + XFREE(MTYPE_ECOMMUNITY_VAL, ecom->val); + ecom->val = p; + } else + ecom->val = NULL; + return true; } diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index dc48a99471..6b028ef32f 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -1124,7 +1124,10 @@ vpn_leak_to_vrf_update_onevrf(struct bgp *bgp_vrf, /* to */ if (!ecom_intersect( bgp_vrf->vpn_policy[afi].rtlist[BGP_VPN_POLICY_DIR_FROMVPN], path_vpn->attr->ecommunity)) { - + if (debug) + zlog_debug( + "from vpn to vrf %s, skipping after no intersection of route targets", + bgp_vrf->name_pretty); return; } @@ -1562,7 +1565,8 @@ void vpn_handle_router_id_update(struct bgp *bgp, bool withdraw, bool is_config) { afi_t afi; - int debug; + int debug = (BGP_DEBUG(vpn, VPN_LEAK_TO_VRF) + | BGP_DEBUG(vpn, VPN_LEAK_FROM_VRF)); char *vname; const char *export_name; char buf[RD_ADDRSTRLEN]; @@ -1571,14 +1575,23 @@ void vpn_handle_router_id_update(struct bgp *bgp, bool withdraw, struct ecommunity *ecom; vpn_policy_direction_t idir, edir; + /* + * Router-id change that is not explicitly configured + * (a change from zebra, frr restart for example) + * should not replace a configured vpn RD/RT. + */ + if (!is_config) { + if (debug) + zlog_debug("%s: skipping non explicit router-id change", + __func__); + return; + } + if (bgp->inst_type != BGP_INSTANCE_TYPE_DEFAULT && bgp->inst_type != BGP_INSTANCE_TYPE_VRF) return; export_name = bgp->name ? bgp->name : VRF_DEFAULT_NAME; - debug = (BGP_DEBUG(vpn, VPN_LEAK_TO_VRF) | - BGP_DEBUG(vpn, VPN_LEAK_FROM_VRF)); - idir = BGP_VPN_POLICY_DIR_FROMVPN; edir = BGP_VPN_POLICY_DIR_TOVPN; @@ -1604,26 +1617,12 @@ void vpn_handle_router_id_update(struct bgp *bgp, bool withdraw, if (!bgp_import) continue; - ecommunity_del_val(bgp_import->vpn_policy[afi]. - rtlist[idir], + ecommunity_del_val( + bgp_import->vpn_policy[afi] + .rtlist[idir], (struct ecommunity_val *)ecom->val); - } } else { - /* - * Router-id changes that are not explicit config - * changes should not replace configured RD/RT. - */ - if (!is_config) { - if (CHECK_FLAG(bgp->vpn_policy[afi].flags, - BGP_VPN_POLICY_TOVPN_RD_SET)) { - if (debug) - zlog_debug("%s: auto router-id change skipped", - __func__); - goto postchange; - } - } - /* New router-id derive auto RD and RT and export * to VPN */ @@ -1654,10 +1653,8 @@ void vpn_handle_router_id_update(struct bgp *bgp, bool withdraw, else bgp_import->vpn_policy[afi].rtlist[idir] = ecommunity_dup(ecom); - } -postchange: /* Update routes to VPN */ vpn_leak_postchange(BGP_VPN_POLICY_DIR_TOVPN, afi, bgp_get_default(),