zebra: Resolve the nexthop being tracked to a non-BGP route

Currently a nexthop being tracked in NHT can be resolved to a BGP route. As BGP routes are usually recursive, the nexthop resolution may not be complete, resulting in wrong IGP metric being given to BGP for bestpath calculation. The fix is to make sure the nexthop being tracked is always resolved to a non-BGP route. Signed-off-by: Enke Chen <[email protected]>
FRRouting · Sep 14, 2024 · 658b99a · 658b99a
1 parent f3fc33e
commit 658b99a
Show file tree

Hide file tree

Showing 13 changed files with 330 additions and 6 deletions.
diff --git a/tests/topotests/zebra_nht_double_recursion/r1/bgpd.conf b/tests/topotests/zebra_nht_double_recursion/r1/bgpd.conf
@@ -0,0 +1,12 @@
+router bgp 65001
+ no bgp ebgp-requires-policy
+ no bgp network import-check
+ neighbor 10.0.0.2 remote-as internal
+ neighbor 10.0.0.2 update-source lo
+ neighbor 10.0.0.2 timers 1 3
+ neighbor 10.0.0.2 timers connect 1
+ neighbor 10.0.0.3 remote-as internal
+ neighbor 10.0.0.3 update-source lo
+ neighbor 10.0.0.3 timers 1 3
+ neighbor 10.0.0.3 timers connect 1
+!
diff --git a/tests/topotests/zebra_nht_double_recursion/r1/ospfd.conf b/tests/topotests/zebra_nht_double_recursion/r1/ospfd.conf
@@ -0,0 +1,17 @@
+!
+interface lo
+ ip ospf passive
+!
+interface r1-eth0
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+interface r1-eth1
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 30
+!
+router ospf
+ router-id 10.0.0.1
+ network 0.0.0.0/0 area 0
diff --git a/tests/topotests/zebra_nht_double_recursion/r1/zebra.conf b/tests/topotests/zebra_nht_double_recursion/r1/zebra.conf
@@ -0,0 +1,10 @@
+!
+interface lo
+ ip address 10.0.0.1/32
+!
+interface r1-eth0
+ ip address 192.168.12.1/24
+!
+interface r1-eth1
+ ip address 192.168.13.1/24
+!
diff --git a/tests/topotests/zebra_nht_double_recursion/r2/bgpd.conf b/tests/topotests/zebra_nht_double_recursion/r2/bgpd.conf
@@ -0,0 +1,17 @@
+router bgp 65001
+ no bgp ebgp-requires-policy
+ no bgp network import-check
+ neighbor 10.0.0.1 remote-as internal
+ neighbor 10.0.0.1 update-source lo
+ neighbor 10.0.0.1 timers 1 3
+ neighbor 10.0.0.1 timers connect 1
+ neighbor 192.168.24.4 remote-as external
+ neighbor 192.168.24.4 timers 1 3
+ neighbor 192.168.24.4 timers connect 1
+ address-family ipv4
+  redistribute connected route-map connected-to-bgp
+ exit-address-family
+!
+route-map connected-to-bgp permit 10
+ set community no-export
+!
diff --git a/tests/topotests/zebra_nht_double_recursion/r2/ospfd.conf b/tests/topotests/zebra_nht_double_recursion/r2/ospfd.conf
@@ -0,0 +1,13 @@
+!
+interface lo
+ ip ospf passive
+!
+interface r2-eth0
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+router ospf
+ router-id 10.0.0.2
+ network 192.168.12.0/24 area 0
+ network 10.0.0.2/32 area 0
diff --git a/tests/topotests/zebra_nht_double_recursion/r2/zebra.conf b/tests/topotests/zebra_nht_double_recursion/r2/zebra.conf
@@ -0,0 +1,10 @@
+!
+interface lo
+ ip address 10.0.0.2/32
+!
+interface r2-eth0
+ ip address 192.168.12.2/24
+!
+interface r2-eth1
+ ip address 192.168.24.2/24
+!
diff --git a/tests/topotests/zebra_nht_double_recursion/r3/bgpd.conf b/tests/topotests/zebra_nht_double_recursion/r3/bgpd.conf
@@ -0,0 +1,17 @@
+router bgp 65001
+ no bgp ebgp-requires-policy
+ no bgp network import-check
+ neighbor 10.0.0.1 remote-as internal
+ neighbor 10.0.0.1 update-source lo
+ neighbor 10.0.0.1 timers 1 3
+ neighbor 10.0.0.1 timers connect 1
+ neighbor 192.168.34.4 remote-as external
+ neighbor 192.168.34.4 timers 1 3
+ neighbor 192.168.34.4 timers connect 1
+ address-family ipv4
+  redistribute connected route-map connected-to-bgp
+ exit-address-family
+!
+route-map connected-to-bgp permit 10
+ set community no-export
+!
diff --git a/tests/topotests/zebra_nht_double_recursion/r3/ospfd.conf b/tests/topotests/zebra_nht_double_recursion/r3/ospfd.conf
@@ -0,0 +1,13 @@
+!
+interface lo
+ ip ospf passive
+!
+interface r3-eth0
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 30
+!
+router ospf
+ router-id 10.0.0.3
+ network 192.168.13.0/24 area 0
+ network 10.0.0.3/32 area 0
diff --git a/tests/topotests/zebra_nht_double_recursion/r3/zebra.conf b/tests/topotests/zebra_nht_double_recursion/r3/zebra.conf
@@ -0,0 +1,10 @@
+!
+interface lo
+ ip address 10.0.0.3/32
+!
+interface r3-eth0
+ ip address 192.168.13.3/24
+!
+interface r3-eth1
+ ip address 192.168.34.3/24
+!
diff --git a/tests/topotests/zebra_nht_double_recursion/r4/bgpd.conf b/tests/topotests/zebra_nht_double_recursion/r4/bgpd.conf
@@ -0,0 +1,13 @@
+router bgp 65002
+ no bgp ebgp-requires-policy
+ no bgp network import-check
+ neighbor 192.168.24.2 remote-as external
+ neighbor 192.168.24.2 timers 1 3
+ neighbor 192.168.24.2 timers connect 1
+ neighbor 192.168.34.3 remote-as external
+ neighbor 192.168.34.3 timers 1 3
+ neighbor 192.168.34.3 timers connect 1
+ address-family ipv4
+  network 10.0.0.4/32
+ exit-address-family
+!
diff --git a/tests/topotests/zebra_nht_double_recursion/r4/zebra.conf b/tests/topotests/zebra_nht_double_recursion/r4/zebra.conf
@@ -0,0 +1,10 @@
+!
+interface lo
+ ip address 10.0.0.4/32
+!
+interface r4-eth0
+ ip address 192.168.24.4/24
+!
+interface r4-eth1
+ ip address 192.168.34.4/24
+!
diff --git a/tests/topotests/zebra_nht_double_recursion/test_nht_double_recusion.py b/tests/topotests/zebra_nht_double_recursion/test_nht_double_recusion.py
@@ -0,0 +1,153 @@
+#!/usr/bin/env python
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+#
+# Copyright (C) 2024 Palo Alto Networks, Inc. All Rights Reserved,
+# contribution by Enke Chen <[email protected]>
+#
+
+"""
+Test to make sure the nexthop being tracked is resolved recursivley to a
+non-BGP route so that the right IGP metric is given to BGP for bestpath
+calculation.
+
+In this test, on r2/r3, the "next-hop-self" is not configured, and the
+connected routes are redistributed into BGP for the nexthop reachability
+of the EBGP routes.
+
+IP Addresses for the loopback interfaces:
+
+  r1: 10.0.0.1/32
+  r2: 10.0.0.2/32
+  r3: 10.0.0.3/32
+  r4: 10.0.0.4/32
+
+10.0.0.4/32 is redistributed into BGP on r4, and is advertised to both
+r2 and r3.  Then r1 receives 10.0.0.4/32 from both r2 and r3, and each of
+the nexthops is resolved to another BGP route which is then resolved to
+an OSPF route.
+
+
+                    10               30
+              r2  ------- r1 (UUT) ------- r3
+              |                            |
+         ebgp |                            | ebgp
+              |                            |
+              +----------- r4 -------------+
+                     .24.4    .34.4
+
+
+On r1 (UUT):
+
+[BGP] 10.0.0.4/32: nexthop 192.168.24.4
+                   nexthop 192.168.34.4
+
+192.168.24.4 --> [BGP] 192.168.24.0/24
+192.168.34.4 --> [BGP] 192.168.34.0/24
+
+[BGP] 192.168.24.0/24: nexthop 10.0.0.2
+[BGP] 192.168.34.0/24: nexthop 10.0.0.3
+
+10.0.0.2 --> [OSPF] 10.0.0.2/32, nexthop xxx, intf xxx
+10.0.0.3 --> [OSPF] 10.0.0.3/32, nexthop xxx, intf xxx
+"""
+
+import os
+import sys
+import json
+import pytest
+import functools
+
+CWD = os.path.dirname(os.path.realpath(__file__))
+sys.path.append(os.path.join(CWD, "../"))
+
+# pylint: disable=C0413
+from lib import topotest
+from lib.topogen import Topogen, TopoRouter, get_topogen
+from lib.common_config import step
+
+pytestmark = [pytest.mark.bgpd]
+
+
+def build_topo(tgen):
+    for routern in range(1, 5):
+        tgen.add_router("r{}".format(routern))
+
+    switch = tgen.add_switch("s1")
+    switch.add_link(tgen.gears["r1"])
+    switch.add_link(tgen.gears["r2"])
+
+    switch = tgen.add_switch("s2")
+    switch.add_link(tgen.gears["r1"])
+    switch.add_link(tgen.gears["r3"])
+
+    switch = tgen.add_switch("s3")
+    switch.add_link(tgen.gears["r2"])
+    switch.add_link(tgen.gears["r4"])
+
+    switch = tgen.add_switch("s4")
+    switch.add_link(tgen.gears["r3"])
+    switch.add_link(tgen.gears["r4"])
+
+
+def setup_module(mod):
+    tgen = Topogen(build_topo, mod.__name__)
+    tgen.start_topology()
+
+    router_list = tgen.routers()
+
+    for i, (rname, router) in enumerate(router_list.items(), 1):
+        router.load_config(
+            TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname))
+        )
+        router.load_config(
+            TopoRouter.RD_OSPF, os.path.join(CWD, "{}/ospfd.conf".format(rname))
+        )
+        router.load_config(
+            TopoRouter.RD_BGP, os.path.join(CWD, "{}/bgpd.conf".format(rname))
+        )
+
+    tgen.start_router()
+
+
+def teardown_module(mod):
+    tgen = get_topogen()
+    tgen.stop_topology()
+
+
+def test_nht_recursive():
+    tgen = get_topogen()
+
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    r1 = tgen.gears["r1"]
+    r2 = tgen.gears["r2"]
+    r3 = tgen.gears["r3"]
+    r4 = tgen.gears["r4"]
+
+    def _bgp_check_igp_metric_bestpath():
+        output = json.loads(r1.vtysh_cmd("show bgp ipv4 unicast 10.0.0.4/32 json"))
+        expected = {
+            "paths": [
+                {
+                    "bestpath": {"selectionReason": "IGP Metric"},
+                    "nexthops": [{"ip": "192.168.24.4", "hostname": "r2", "metric": 10, "accessible": True}],
+                },
+                {
+                    "nexthops": [{"ip": "192.168.34.4", "hostname": "r3", "metric": 30, "accessible": True}],
+                },
+            ]
+        }
+        return topotest.json_cmp(output, expected)
+
+
+    # Check BGP bestpath selected on IGP metric
+    test_func = functools.partial(_bgp_check_igp_metric_bestpath)
+    _, result = topotest.run_and_expect(test_func, None, count=20, wait=2)
+    assert result is None, "BGP bestpath not selected on igp metric"
+
+
+if __name__ == "__main__":
+    args = ["-s"] + sys.argv[1:]
+    sys.exit(pytest.main(args))
diff --git a/zebra/zebra_rnh.c b/zebra/zebra_rnh.c
@@ -552,14 +552,19 @@ zebra_rnh_resolve_nexthop_entry(struct zebra_vrf *zvrf, afi_t afi,
 	struct route_table *route_table;
 	struct route_node *rn;
 	struct route_entry *re;
+	struct nexthop *nexthop;
+	struct prefix p;
 
 	*prn = NULL;
 
 	route_table = zvrf->table[afi][rnh->safi];
 	if (!route_table)
 		return NULL;
 
-	rn = route_node_match(route_table, &nrn->p);
+	p = nrn->p;
+
+resolve_again:
+	rn = route_node_match(route_table, &p);
 	if (!rn)
 		return NULL;
 
@@ -571,9 +576,9 @@ zebra_rnh_resolve_nexthop_entry(struct zebra_vrf *zvrf, afi_t afi,
 	 */
 	while (rn) {
 		if (IS_ZEBRA_DEBUG_NHT_DETAILED)
-			zlog_debug("%s: %s(%u):%pRN Possible Match to %pRN",
+			zlog_debug("%s: %s(%u):%pFX Possible Match to %pRN",
 				   __func__, VRF_LOGNAME(zvrf->vrf),
-				   rnh->vrf_id, rnh->node, rn);
+				   rnh->vrf_id, &p, rn);
 
 		/* Do not resolve over default route unless allowed &&
 		 * match route to be exact if so specified
@@ -623,10 +628,34 @@ zebra_rnh_resolve_nexthop_entry(struct zebra_vrf *zvrf, afi_t afi,
 				break;
 		}
 
-		/* Route entry found, we're done; else, walk up the tree. */
+		/* Route entry found, we're done; else, walk up the tree.
+		 *
+		 * BGP routes are usually recursive, though. Keep resolving
+		 * on the nexthop until hitting a non-BGP route.
+		 */
 		if (re) {
-			*prn = rn;
-			return re;
+			if (re->type != ZEBRA_ROUTE_BGP) {
+				*prn = rn;
+				return re;
+			}
+
+			/*
+			 * Note an aggregate has the NEXTHOP_TYPE_BLACKHOLE
+			 * as the nexthop type.
+			 */
+			nexthop = re->nhe->nhg.nexthop;
+			if (!nexthop || ((nexthop->type != NEXTHOP_TYPE_IPV4) &&
+					 (nexthop->type != NEXTHOP_TYPE_IPV6))) {
+				*prn = rn;
+				return re;
+			}
+
+			addr2hostprefix(afi2family(afi), &nexthop->gate, &p);
+			if (IS_ZEBRA_DEBUG_NHT_DETAILED) {
+				zlog_debug("  Continue resolving on nexthop %pFX",
+					   &p);
+			}
+			goto resolve_again;
 		} else {
 			/* Resolve the nexthop recursively by finding matching
 			 * route with lower prefix length