Merge pull request #17199 from enkechen-panw/aigp-fix5

bgpd: compare aigp after local route check in bgp_path_info_cmp()

5 files changed, 83 insertions, 33 deletions

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 6fa1505998..d1b3919d72 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -1064,7 +1064,32 @@ int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new,
 		}
 	}
 
-	/* Tie-breaker - AIGP (Metric TLV) attribute */
+	/* 3. Local route check. We prefer:
+	 *  - BGP_ROUTE_STATIC
+	 *  - BGP_ROUTE_AGGREGATE
+	 *  - BGP_ROUTE_REDISTRIBUTE
+	 */
+	new_origin = !(new->sub_type == BGP_ROUTE_NORMAL || new->sub_type == BGP_ROUTE_IMPORTED);
+	exist_origin = !(exist->sub_type == BGP_ROUTE_NORMAL ||
+			 exist->sub_type == BGP_ROUTE_IMPORTED);
+
+	if (new_origin && !exist_origin) {
+		*reason = bgp_path_selection_local_route;
+		if (debug)
+			zlog_debug("%s: %s wins over %s due to preferred BGP_ROUTE type", pfx_buf,
+				   new_buf, exist_buf);
+		return 1;
+	}
+
+	if (!new_origin && exist_origin) {
+		*reason = bgp_path_selection_local_route;
+		if (debug)
+			zlog_debug("%s: %s loses to %s due to preferred BGP_ROUTE type", pfx_buf,
+				   new_buf, exist_buf);
+		return 0;
+	}
+
+	/* 3.5. Tie-breaker - AIGP (Metric TLV) attribute */
 	if (CHECK_FLAG(newattr->flag, ATTR_FLAG_BIT(BGP_ATTR_AIGP)) &&
 	    CHECK_FLAG(existattr->flag, ATTR_FLAG_BIT(BGP_ATTR_AIGP)) &&
 	    CHECK_FLAG(bgp->flags, BGP_FLAG_COMPARE_AIGP)) {
@@ -1094,34 +1119,6 @@ int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new,
 		}
 	}
 
-	/* 3. Local route check. We prefer:
-	 *  - BGP_ROUTE_STATIC
-	 *  - BGP_ROUTE_AGGREGATE
-	 *  - BGP_ROUTE_REDISTRIBUTE
-	 */
-	new_origin = !(new->sub_type == BGP_ROUTE_NORMAL ||
-		       new->sub_type == BGP_ROUTE_IMPORTED);
-	exist_origin = !(exist->sub_type == BGP_ROUTE_NORMAL ||
-			 exist->sub_type == BGP_ROUTE_IMPORTED);
-
-	if (new_origin && !exist_origin) {
-		*reason = bgp_path_selection_local_route;
-		if (debug)
-			zlog_debug(
-				"%s: %s wins over %s due to preferred BGP_ROUTE type",
-				pfx_buf, new_buf, exist_buf);
-		return 1;
-	}
-
-	if (!new_origin && exist_origin) {
-		*reason = bgp_path_selection_local_route;
-		if (debug)
-			zlog_debug(
-				"%s: %s loses to %s due to preferred BGP_ROUTE type",
-				pfx_buf, new_buf, exist_buf);
-		return 0;
-	}
-
 	/* Here if these are imported routes then get ultimate pi for
 	 * path compare.
 	 */
diff --git a/doc/user/bgp.rst b/doc/user/bgp.rst
index 6a46128d72..f2005c18dd 100644
--- a/doc/user/bgp.rst
+++ b/doc/user/bgp.rst
@@ -160,16 +160,16 @@ bottom until one of the factors can be used.
 
    Prefer higher local preference routes to lower.
 
+3. **Local route check**
+
+   Prefer local routes (statics, aggregates, redistributed) to received routes.
+
    If ``bgp bestpath aigp`` is enabled, and both paths that are compared have
    AIGP attribute, BGP uses AIGP tie-breaking unless both of the paths have the
    AIGP metric attribute. This means that the AIGP attribute is not evaluated
    during the best path selection process between two paths when one path does
    not have the AIGP attribute.
 
-3. **Local route check**
-
-   Prefer local routes (statics, aggregates, redistributed) to received routes.
-
 4. **AS path length check**
 
    Prefer shortest hop-count AS_PATHs.
diff --git a/tests/topotests/bgp_aigp_rr/r1/bgpd.conf b/tests/topotests/bgp_aigp_rr/r1/bgpd.conf
index 90d34bdf83..4099a248f1 100644
--- a/tests/topotests/bgp_aigp_rr/r1/bgpd.conf
+++ b/tests/topotests/bgp_aigp_rr/r1/bgpd.conf
@@ -18,6 +18,7 @@ router bgp 65001
  neighbor 10.0.0.4 timers connect 1
  neighbor 10.0.0.4 route-reflector-client
  address-family ipv4
+  network 10.0.1.2/32 route-map set-aigp
   neighbor 10.0.0.4 route-map set-nexthop out
  exit-address-family
 !
@@ -25,3 +26,7 @@ route-map set-nexthop permit 10
  set ip next-hop peer-address
 exit
 !
+route-map set-aigp permit 10
+ set aigp 50
+ set weight 0
+!
diff --git a/tests/topotests/bgp_aigp_rr/r2/bgpd.conf b/tests/topotests/bgp_aigp_rr/r2/bgpd.conf
index 97059fdd33..0159dc8e7c 100644
--- a/tests/topotests/bgp_aigp_rr/r2/bgpd.conf
+++ b/tests/topotests/bgp_aigp_rr/r2/bgpd.conf
@@ -7,6 +7,7 @@ router bgp 65001
  neighbor 10.0.0.1 timers connect 1
  address-family ipv4
   redistribute connected route-map connected-to-bgp
+  network 10.0.1.2/32 route-map set-aigp
   neighbor 10.0.0.1 next-hop-self
  exit-address-family
 !
@@ -16,3 +17,6 @@ route-map connected-to-bgp permit 10
  match ip address prefix-list p22
  set aigp 2
 !
+route-map set-aigp permit 10
+ set aigp 10
+!
diff --git a/tests/topotests/bgp_aigp_rr/test_bgp_aigp_rr.py b/tests/topotests/bgp_aigp_rr/test_bgp_aigp_rr.py
index 0079535da7..206e229b2e 100644
--- a/tests/topotests/bgp_aigp_rr/test_bgp_aigp_rr.py
+++ b/tests/topotests/bgp_aigp_rr/test_bgp_aigp_rr.py
@@ -101,6 +101,45 @@ def test_bgp_aigp_rr():
         expected = {"paths": [{"aigpMetric": aigp, "valid": True}]}
         return topotest.json_cmp(output, expected)
 
+    def _bgp_check_aigp_bestpath():
+        output = json.loads(r1.vtysh_cmd("show bgp ipv4 unicast 10.0.1.2/32 json"))
+        expected = {
+            "prefix": "10.0.1.2/32",
+            "paths": [
+                {
+                    "aigpMetric": 50,
+                    "valid": True,
+                    "sourced": True,
+                    "local": True,
+                    "bestpath": {"overall": True, "selectionReason": "Local Route"},
+                    "nexthops": [
+                        {
+                            "ip": "0.0.0.0",
+                            "hostname": "r1",
+                            "afi": "ipv4",
+                            "metric": 0,
+                            "accessible": True,
+                            "used": True,
+                        }
+                    ],
+                },
+                {
+                    "aigpMetric": 10,
+                    "valid": True,
+                    "nexthops": [
+                        {
+                            "ip": "10.0.0.2",
+                            "hostname": "r2",
+                            "afi": "ipv4",
+                            "metric": 10,
+                            "accessible": True,
+                            "used": True,
+                        }
+                    ],
+                },
+            ],
+        }
+        return topotest.json_cmp(output, expected)
 
     # r2, 10.0.2.2/32 with aigp-metric 2
     test_func = functools.partial(_bgp_check_aigp_metric, r2, "10.0.2.2/32", 2)
@@ -122,6 +161,11 @@ def test_bgp_aigp_rr():
     _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
     assert result is None, "aigp-metric for 10.0.2.2/32 is not 12"
 
+    # r1, check if the local route is favored over AIGP comparison
+    test_func = functools.partial(_bgp_check_aigp_bestpath)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
+    assert result is None, "Local route is not favored over AIGP in best-path selection"
+
 
 if __name__ == "__main__":
     args = ["-s"] + sys.argv[1:]