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>

diff --git a/bgpd/bgp_mpath.c b/bgpd/bgp_mpath.c
index 42077d4e8e7f544d741335386f8f8b1f40fcfe55..ba66bf3b6e2a0f91c8fd0b4f59cdfb0234f4d2c0 100644 (file)
--- 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 659029b04ca9d7f28b3f7a6857f2cc3d040066d6..e24c1ab764a967684081ce393d4fbfe4cb1affd8 100644 (file)
--- 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 4838e6c7ddc1811a224414c64c92498cba246fd7..5b124068f95683c7889a3b46c849c5e4557076d7 100644 (file)
--- 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 2fd80495d934f5e3e7f2b72d290c48be4d52fdcc..4cc56c86495e018bd545d1f3bcd98079d1be1aa8 100644 (file)
--- 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);