From da0c0ef70cfb413450539581de2508fb6a385fc3 Mon Sep 17 00:00:00 2001 From: Kantesh Mundaragi Date: Mon, 8 Jun 2020 14:40:17 -0700 Subject: [PATCH] bgpd: VRF-Lite fix best path selection Description: Incorrect behavior during best path selection for the imported routes. Imported routes are always treated as eBGP routes. Change is intended for fixing the issues related to bgp best path selection for leaked routes: - FRR does ecmp for the imported routes, even without any ecmp related config. If the same prefix is imported from two different VRFs, then we configure the route with ecmp even without any ecmp related config. - Locally imported routes are preferred over imported eBGP routes. If there is a local route and eBGP learned route for the same prefix, if we import both the routes, imported local route is selected as best path. - Same route is imported from multiple tenant VRFs, both imported routes point to the same VRF in nexthop. - When the same route with same nexthop in two different VRFs is imported from those two VRFs, route is not installed as ecmp, even though we had ecmp config. - During best path selection, while comparing the paths for imported routes, we should correctly refer to the original route i.e. the ultimate path. - When the same route is imported from multiple VRF, use the correct VRF while installing in the FIB. - When same route is imported from two different tenant VRFs, while comparing bgp path info as part of bgp best path selection, we should ideally also compare corresponding VRFs. See-also: https://github.com/FRRouting/frr/files/7169555/FRR.and.Cisco.VRF-Lite.Behaviour.pdf Co-authored-by: Santosh P K Co-authored-by: Kantesh Mundaragi Signed-off-by: Iqra Siddiqui --- bgpd/bgp_mpath.c | 14 ++++++++++++++ bgpd/bgp_mplsvpn.c | 10 ++-------- bgpd/bgp_route.c | 37 ++++++++++++++++++++++++++++++++++--- bgpd/bgp_route.h | 2 ++ 4 files changed, 52 insertions(+), 11 deletions(-) diff --git a/bgpd/bgp_mpath.c b/bgpd/bgp_mpath.c index 42077d4e8e..ba66bf3b6e 100644 --- a/bgpd/bgp_mpath.c +++ b/bgpd/bgp_mpath.c @@ -181,6 +181,20 @@ int bgp_path_info_nexthop_cmp(struct bgp_path_info *bpi1, } } + /* + * If both nexthops are same then check + * if they belong to same VRF + */ + if (!compare && bpi1->attr->nh_type != NEXTHOP_TYPE_BLACKHOLE) { + if (bpi1->extra && bpi1->extra->bgp_orig && bpi2->extra + && bpi2->extra->bgp_orig) { + if (bpi1->extra->bgp_orig->vrf_id + != bpi2->extra->bgp_orig->vrf_id) { + compare = 1; + } + } + } + return compare; } diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 659029b04c..e24c1ab764 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -779,10 +779,7 @@ leak_update(struct bgp *bgp, /* destination bgp instance */ * schemes that could be implemented in the future. * */ - for (bpi_ultimate = source_bpi; - bpi_ultimate->extra && bpi_ultimate->extra->parent; - bpi_ultimate = bpi_ultimate->extra->parent) - ; + bpi_ultimate = bgp_get_imported_bpi_ultimate(source_bpi); /* * match parent @@ -1619,10 +1616,7 @@ vpn_leak_to_vrf_update_onevrf(struct bgp *bgp_vrf, /* to */ if (!CHECK_FLAG(bgp_vrf->af_flags[afi][safi], BGP_CONFIG_VRF_TO_VRF_IMPORT)) { /* work back to original route */ - for (bpi_ultimate = path_vpn; - bpi_ultimate->extra && bpi_ultimate->extra->parent; - bpi_ultimate = bpi_ultimate->extra->parent) - ; + bpi_ultimate = bgp_get_imported_bpi_ultimate(path_vpn); /* * if original route was unicast, diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 4838e6c7dd..5b124068f9 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -548,6 +548,25 @@ void bgp_path_info_path_with_addpath_rx_str(struct bgp_path_info *pi, char *buf, snprintf(buf, buf_len, "path %s", pi->peer->host); } + +/* + * Get the ultimate path info. + */ +struct bgp_path_info *bgp_get_imported_bpi_ultimate(struct bgp_path_info *info) +{ + struct bgp_path_info *bpi_ultimate; + + if (info->sub_type != BGP_ROUTE_IMPORTED) + return info; + + for (bpi_ultimate = info; + bpi_ultimate->extra && bpi_ultimate->extra->parent; + bpi_ultimate = bpi_ultimate->extra->parent) + ; + + return bpi_ultimate; +} + /* Compare two bgp route entity. If 'new' is preferable over 'exist' return 1. */ static int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new, @@ -587,6 +606,7 @@ static int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new, bool old_proxy; bool new_proxy; bool new_origin, exist_origin; + struct bgp_path_info *bpi_ultimate; *paths_eq = 0; @@ -598,9 +618,11 @@ static int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new, return 0; } - if (debug) - bgp_path_info_path_with_addpath_rx_str(new, new_buf, + if (debug) { + bpi_ultimate = bgp_get_imported_bpi_ultimate(new); + bgp_path_info_path_with_addpath_rx_str(bpi_ultimate, new_buf, sizeof(new_buf)); + } if (exist == NULL) { *reason = bgp_path_selection_first; @@ -611,7 +633,8 @@ static int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new, } if (debug) { - bgp_path_info_path_with_addpath_rx_str(exist, exist_buf, + bpi_ultimate = bgp_get_imported_bpi_ultimate(exist); + bgp_path_info_path_with_addpath_rx_str(bpi_ultimate, exist_buf, sizeof(exist_buf)); zlog_debug("%s(%s): Comparing %s flags 0x%x with %s flags 0x%x", pfx_buf, bgp->name_pretty, new_buf, new->flags, @@ -859,6 +882,14 @@ static int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new, return 0; } + /* Here if these are imported routes then get ultimate pi for + * path compare. + */ + new = bgp_get_imported_bpi_ultimate(new); + exist = bgp_get_imported_bpi_ultimate(exist); + newattr = new->attr; + existattr = exist->attr; + /* 4. AS path length check. */ if (!CHECK_FLAG(bgp->flags, BGP_FLAG_ASPATH_IGNORE)) { int exist_hops = aspath_count_hops(existattr->aspath); diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 2fd80495d9..4cc56c8649 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -633,6 +633,8 @@ extern struct bgp_dest *bgp_afi_node_get(struct bgp_table *table, afi_t afi, struct prefix_rd *prd); extern struct bgp_path_info *bgp_path_info_lock(struct bgp_path_info *path); extern struct bgp_path_info *bgp_path_info_unlock(struct bgp_path_info *path); +extern struct bgp_path_info * +bgp_get_imported_bpi_ultimate(struct bgp_path_info *info); extern void bgp_path_info_add(struct bgp_dest *dest, struct bgp_path_info *pi); extern void bgp_path_info_extra_free(struct bgp_path_info_extra **extra); extern void bgp_path_info_reap(struct bgp_dest *dest, struct bgp_path_info *pi); -- 2.39.5