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 <sapk@vmware.com>
Co-authored-by: Kantesh Mundaragi <kmundaragi@vmware.com>
Signed-off-by: Iqra Siddiqui <imujeebsiddi@vmware.com>

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;
 }
 

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,

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);

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);