Merge pull request #17110 from FRRouting/mergify/bp/stable/10.0/pr-17093

bgpd: fix route selection with AIGP (backport #17093)

15 files changed, 123 insertions, 47 deletions

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 5966b90ad0..944c4583e5 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -479,16 +479,6 @@ static bool bgp_attr_aigp_get_tlv_metric(uint8_t *pnt, int length,
 	return false;
 }
 
-static uint64_t bgp_aigp_metric_total(struct bgp_path_info *bpi)
-{
-	uint64_t aigp = bgp_attr_get_aigp_metric(bpi->attr);
-
-	if (bpi->nexthop)
-		return aigp + bpi->nexthop->metric;
-	else
-		return aigp;
-}
-
 static void stream_put_bgp_aigp_tlv_metric(struct stream *s,
 					   struct bgp_path_info *bpi)
 {
diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
index d78f04c6dd..7e735f0f0c 100644
--- a/bgpd/bgp_attr.h
+++ b/bgpd/bgp_attr.h
@@ -601,6 +601,16 @@ static inline void bgp_attr_set_aigp_metric(struct attr *attr, uint64_t aigp)
 		SET_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_AIGP));
 }
 
+static inline uint64_t bgp_aigp_metric_total(struct bgp_path_info *bpi)
+{
+	uint64_t aigp = bgp_attr_get_aigp_metric(bpi->attr);
+
+	if (bpi->nexthop)
+		return aigp + bpi->nexthop->metric;
+	else
+		return aigp;
+}
+
 static inline struct cluster_list *bgp_attr_get_cluster(const struct attr *attr)
 {
 	return attr->cluster1;
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 5fb1b05ef9..c7580ae0a7 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -970,8 +970,8 @@ int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new,
 	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)) {
-		uint64_t new_aigp = bgp_attr_get_aigp_metric(newattr);
-		uint64_t exist_aigp = bgp_attr_get_aigp_metric(existattr);
+		uint64_t new_aigp = bgp_aigp_metric_total(new);
+		uint64_t exist_aigp = bgp_aigp_metric_total(exist);
 
 		if (new_aigp < exist_aigp) {
 			*reason = bgp_path_selection_aigp;
diff --git a/tests/topotests/bgp_aigp/r1/ospfd.conf b/tests/topotests/bgp_aigp/r1/ospfd.conf
index 38aa11d036..098bf57b03 100644
--- a/tests/topotests/bgp_aigp/r1/ospfd.conf
+++ b/tests/topotests/bgp_aigp/r1/ospfd.conf
@@ -1,6 +1,6 @@
 !
 interface lo
- ip ospf cost 10
+ ip ospf passive
 !
 interface r1-eth0
  ip ospf dead-interval 4
diff --git a/tests/topotests/bgp_aigp/r2/bgpd.conf b/tests/topotests/bgp_aigp/r2/bgpd.conf
index 4db4687536..4539016f91 100644
--- a/tests/topotests/bgp_aigp/r2/bgpd.conf
+++ b/tests/topotests/bgp_aigp/r2/bgpd.conf
@@ -1,6 +1,7 @@
 router bgp 65001
  no bgp ebgp-requires-policy
  no bgp network import-check
+ bgp route-reflector allow-outbound-policy
  neighbor 10.0.0.1 remote-as internal
  neighbor 10.0.0.1 timers 1 3
  neighbor 10.0.0.1 timers connect 1
@@ -11,5 +12,6 @@ router bgp 65001
  neighbor 192.168.24.4 aigp
  address-family ipv4
   redistribute connected
+  neighbor 10.0.0.1 next-hop-self force
  exit-address-family
 !
diff --git a/tests/topotests/bgp_aigp/r2/ospfd.conf b/tests/topotests/bgp_aigp/r2/ospfd.conf
index ed31941f65..106a46251d 100644
--- a/tests/topotests/bgp_aigp/r2/ospfd.conf
+++ b/tests/topotests/bgp_aigp/r2/ospfd.conf
@@ -1,6 +1,6 @@
 !
 interface lo
- ip ospf cost 10
+ ip ospf passive
 !
 interface r2-eth0
  ip ospf dead-interval 4
diff --git a/tests/topotests/bgp_aigp/r3/bgpd.conf b/tests/topotests/bgp_aigp/r3/bgpd.conf
index 5ab712eaba..bdaa5cf55d 100644
--- a/tests/topotests/bgp_aigp/r3/bgpd.conf
+++ b/tests/topotests/bgp_aigp/r3/bgpd.conf
@@ -1,6 +1,7 @@
 router bgp 65001
  no bgp ebgp-requires-policy
  no bgp network import-check
+ bgp route-reflector allow-outbound-policy
  neighbor 10.0.0.1 remote-as internal
  neighbor 10.0.0.1 timers 1 3
  neighbor 10.0.0.1 timers connect 1
@@ -11,5 +12,6 @@ router bgp 65001
  neighbor 192.168.35.5 aigp
  address-family ipv4
   redistribute connected
+  neighbor 10.0.0.1 next-hop-self force
  exit-address-family
 !
diff --git a/tests/topotests/bgp_aigp/r3/ospfd.conf b/tests/topotests/bgp_aigp/r3/ospfd.conf
index f971ad6f89..9ede3a1fab 100644
--- a/tests/topotests/bgp_aigp/r3/ospfd.conf
+++ b/tests/topotests/bgp_aigp/r3/ospfd.conf
@@ -1,6 +1,6 @@
 !
 interface lo
- ip ospf cost 10
+ ip ospf passive
 !
 interface r3-eth0
  ip ospf dead-interval 4
diff --git a/tests/topotests/bgp_aigp/r4/bgpd.conf b/tests/topotests/bgp_aigp/r4/bgpd.conf
index aa88bac913..e12c45e0bb 100644
--- a/tests/topotests/bgp_aigp/r4/bgpd.conf
+++ b/tests/topotests/bgp_aigp/r4/bgpd.conf
@@ -1,6 +1,7 @@
 router bgp 65001
  no bgp ebgp-requires-policy
  no bgp network import-check
+ bgp route-reflector allow-outbound-policy
  neighbor 192.168.24.2 remote-as internal
  neighbor 192.168.24.2 timers 1 3
  neighbor 192.168.24.2 timers connect 1
@@ -11,7 +12,18 @@ router bgp 65001
  neighbor 10.0.0.6 timers connect 1
  neighbor 10.0.0.6 update-source lo
  address-family ipv4
-  redistribute connected
-  redistribute ospf
+  redistribute connected route-map connected-to-bgp
+  neighbor 192.168.24.2 route-map set-nexthop out
  exit-address-family
 !
+! Two OSPF domains should be isolated - otherwise the connected routes
+! on r4 would be advertised to r3 (via r4 -> r6 -> r5 -> r3), and can
+! mess up bgp bestpath calculation (igp metrics for the BGP nexthops).
+!
+route-map connected-to-bgp permit 10
+ set community no-advertise
+!
+route-map set-nexthop permit 10
+ set ip next-hop peer-address
+exit
+!
diff --git a/tests/topotests/bgp_aigp/r4/ospfd.conf b/tests/topotests/bgp_aigp/r4/ospfd.conf
index c9e6796f6e..237b5e18ab 100644
--- a/tests/topotests/bgp_aigp/r4/ospfd.conf
+++ b/tests/topotests/bgp_aigp/r4/ospfd.conf
@@ -1,6 +1,6 @@
 !
 interface lo
- ip ospf cost 10
+ ip ospf passive
 !
 interface r4-eth1
  ip ospf dead-interval 4
diff --git a/tests/topotests/bgp_aigp/r5/bgpd.conf b/tests/topotests/bgp_aigp/r5/bgpd.conf
index 4fde262053..3d1f5e8572 100644
--- a/tests/topotests/bgp_aigp/r5/bgpd.conf
+++ b/tests/topotests/bgp_aigp/r5/bgpd.conf
@@ -1,6 +1,7 @@
 router bgp 65001
  no bgp ebgp-requires-policy
  no bgp network import-check
+ bgp route-reflector allow-outbound-policy
  neighbor 192.168.35.3 remote-as internal
  neighbor 192.168.35.3 timers 1 3
  neighbor 192.168.35.3 timers connect 1
@@ -11,7 +12,18 @@ router bgp 65001
  neighbor 10.0.0.6 timers connect 1
  neighbor 10.0.0.6 update-source lo
  address-family ipv4
-  redistribute connected
-  redistribute ospf
+  redistribute connected route-map connected-to-bgp
+  neighbor 192.168.35.3 route-map set-nexthop out
  exit-address-family
 !
+! Two OSPF domains should be isolated - otherwise the connected routes
+! on r5 would be advertised to r2 (via r5 -> r6 -> r4 -> r2), and can
+! mess up bgp bestpath calculation (igp metrics for the BGP nexthops).
+!
+route-map connected-to-bgp permit 10
+ set community no-advertise
+!
+route-map set-nexthop permit 10
+ set ip next-hop peer-address
+exit
+!
diff --git a/tests/topotests/bgp_aigp/r5/ospfd.conf b/tests/topotests/bgp_aigp/r5/ospfd.conf
index b853c74102..65a213df17 100644
--- a/tests/topotests/bgp_aigp/r5/ospfd.conf
+++ b/tests/topotests/bgp_aigp/r5/ospfd.conf
@@ -1,6 +1,6 @@
 !
 interface lo
- ip ospf cost 10
+ ip ospf passive
 !
 interface r5-eth1
  ip ospf dead-interval 4
diff --git a/tests/topotests/bgp_aigp/r6/bgpd.conf b/tests/topotests/bgp_aigp/r6/bgpd.conf
index 2faae7720c..2d5f7a89ba 100644
--- a/tests/topotests/bgp_aigp/r6/bgpd.conf
+++ b/tests/topotests/bgp_aigp/r6/bgpd.conf
@@ -1,6 +1,7 @@
 router bgp 65001
  no bgp ebgp-requires-policy
  no bgp network import-check
+ bgp route-reflector allow-outbound-policy
  neighbor 10.0.0.4 remote-as internal
  neighbor 10.0.0.4 timers 1 3
  neighbor 10.0.0.4 timers connect 1
@@ -15,6 +16,11 @@ router bgp 65001
  neighbor 192.168.67.7 timers 1 3
  neighbor 192.168.67.7 timers connect 1
  address-family ipv4
-  redistribute ospf
+  neighbor 10.0.0.4 route-map set-nexthop out
+  neighbor 10.0.0.5 route-map set-nexthop out
  exit-address-family
 !
+route-map set-nexthop permit 10
+ set ip next-hop peer-address
+exit
+!
diff --git a/tests/topotests/bgp_aigp/r6/ospfd.conf b/tests/topotests/bgp_aigp/r6/ospfd.conf
index 46b2933178..89cbefa895 100644
--- a/tests/topotests/bgp_aigp/r6/ospfd.conf
+++ b/tests/topotests/bgp_aigp/r6/ospfd.conf
@@ -1,6 +1,6 @@
 !
 interface lo
- ip ospf cost 10
+ ip ospf passive
 !
 interface r6-eth0
  ip ospf dead-interval 4
diff --git a/tests/topotests/bgp_aigp/test_bgp_aigp.py b/tests/topotests/bgp_aigp/test_bgp_aigp.py
index 655e9ad184..108b19c4a5 100644
--- a/tests/topotests/bgp_aigp/test_bgp_aigp.py
+++ b/tests/topotests/bgp_aigp/test_bgp_aigp.py
@@ -12,9 +12,9 @@ r7 sets aigp-metric for 10.0.0.71/32 to 71, and 72 for 10.0.0.72/32.
 r6 receives those routes with aigp-metric TLV.
 
 r2 and r3 receives those routes with aigp-metric TLV increased by 20,
-and 30 appropriately.
+and 10 appropriately.
 
-r1 receives routes with aigp-metric TLV 111,131 and 112,132 appropriately.
+r1 receives routes with aigp-metric TLV 81, 91 and 82, 92 respectively.
 """
 
 import os
@@ -109,15 +109,29 @@ def test_bgp_aigp():
         expected = {
             "paths": [
                 {
-                    "aigpMetric": 111,
+                    "aigpMetric": 81,
                     "valid": True,
-                    "nexthops": [{"hostname": "r3", "accessible": True}],
+                    "nexthops": [
+                        {
+                            "ip": "10.0.0.3",
+                            "hostname": "r3",
+                            "metric": 30,
+                            "accessible": True,
+                        }
+                    ],
                 },
                 {
-                    "aigpMetric": 131,
+                    "aigpMetric": 91,
                     "valid": True,
-                    "bestpath": {"selectionReason": "Neighbor IP"},
-                    "nexthops": [{"hostname": "r2", "accessible": True}],
+                    "bestpath": {"selectionReason": "IGP Metric"},
+                    "nexthops": [
+                        {
+                            "ip": "10.0.0.2",
+                            "hostname": "r2",
+                            "metric": 10,
+                            "accessible": True,
+                        }
+                    ],
                 },
             ]
         }
@@ -141,30 +155,58 @@ def test_bgp_aigp():
                 "10.0.0.71/32": {
                     "paths": [
                         {
-                            "aigpMetric": 111,
-                            "bestpath": {"selectionReason": "AIGP"},
+                            "aigpMetric": 81,
                             "valid": True,
-                            "nexthops": [{"hostname": "r3", "accessible": True}],
+                            "nexthops": [
+                                {
+                                    "ip": "10.0.0.3",
+                                    "hostname": "r3",
+                                    "metric": 30,
+                                    "accessible": True,
+                                }
+                            ],
                         },
                         {
-                            "aigpMetric": 131,
+                            "aigpMetric": 91,
                             "valid": True,
-                            "nexthops": [{"hostname": "r2", "accessible": True}],
+                            "bestpath": {"selectionReason": "AIGP"},
+                            "nexthops": [
+                                {
+                                    "ip": "10.0.0.2",
+                                    "hostname": "r2",
+                                    "metric": 10,
+                                    "accessible": True,
+                                }
+                            ],
                         },
                     ],
                 },
                 "10.0.0.72/32": {
                     "paths": [
                         {
-                            "aigpMetric": 112,
-                            "bestpath": {"selectionReason": "AIGP"},
+                            "aigpMetric": 82,
                             "valid": True,
-                            "nexthops": [{"hostname": "r3", "accessible": True}],
+                            "nexthops": [
+                                {
+                                    "ip": "10.0.0.3",
+                                    "hostname": "r3",
+                                    "metric": 30,
+                                    "accessible": True,
+                                }
+                            ],
                         },
                         {
-                            "aigpMetric": 132,
+                            "aigpMetric": 92,
                             "valid": True,
-                            "nexthops": [{"hostname": "r2", "accessible": True}],
+                            "bestpath": {"selectionReason": "AIGP"},
+                            "nexthops": [
+                                {
+                                    "ip": "10.0.0.2",
+                                    "hostname": "r2",
+                                    "metric": 10,
+                                    "accessible": True,
+                                }
+                            ],
                         },
                     ],
                 },
@@ -196,17 +238,17 @@ def test_bgp_aigp():
     _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
     assert result is None, "aigp-metric for 10.0.0.72/32 is not 72"
 
-    # r2, 10.0.0.71/32 with aigp-metric 101 (71 + 30)
-    test_func = functools.partial(_bgp_check_aigp_metric, r2, "10.0.0.71/32", 101)
+    # r2, 10.0.0.71/32 with aigp-metric 101 (71 + 20)
+    test_func = functools.partial(_bgp_check_aigp_metric, r2, "10.0.0.71/32", 91)
     _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
-    assert result is None, "aigp-metric for 10.0.0.71/32 is not 101"
+    assert result is None, "aigp-metric for 10.0.0.71/32 is not 91"
 
-    # r3, 10.0.0.72/32 with aigp-metric 92 (72 + 20)
-    test_func = functools.partial(_bgp_check_aigp_metric, r3, "10.0.0.72/32", 92)
+    # r3, 10.0.0.72/32 with aigp-metric 92 (72 + 10)
+    test_func = functools.partial(_bgp_check_aigp_metric, r3, "10.0.0.72/32", 82)
     _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
-    assert result is None, "aigp-metric for 10.0.0.72/32 is not 92"
+    assert result is None, "aigp-metric for 10.0.0.72/32 is not 82"
 
-    # r1, check if AIGP is considered in best-path selection (lowest wins)
+    # r1, check if AIGP is considered in best-path selection (lowest wins: aigp + nexthop-metric)
     test_func = functools.partial(_bgp_check_aigp_metric_bestpath)
     _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
     assert result is None, "AIGP attribute is not considered in best-path selection"