From caf896d6ef18d917a9b9cee9cb48719ea9757e37 Mon Sep 17 00:00:00 2001 From: anlan_cs Date: Mon, 19 Jun 2023 09:10:57 +0800 Subject: [PATCH 1/3] zebra: Remove unnecessary condition check for kernel routes There are relaxed nexthop requirements for kernel routes because we trust kernel routes. Two minor changes for kernel routes: 1. `if_is_up()` is one of the necessary conditions for `if_is_operative()`. Here, we can remove this unnecessary check for clarity. 2. Since `nexthop_active()` doesn't distinguish whether it is kernel route, modified the corresponding comment in it. Signed-off-by: anlan_cs --- zebra/zebra_nhg.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 96e021292b..fb474b1add 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2181,11 +2181,7 @@ static int nexthop_active(struct nexthop *nexthop, struct nhg_hash_entry *nhe, case NEXTHOP_TYPE_IFINDEX: ifp = if_lookup_by_index(nexthop->ifindex, nexthop->vrf_id); - /* - * If the interface exists and its operative or its a kernel - * route and interface is up, its active. We trust kernel routes - * to be good. - */ + /* If the interface exists and its operative, it's active */ if (ifp && (if_is_operative(ifp))) return 1; else @@ -2595,7 +2591,7 @@ static unsigned nexthop_active_check(struct route_node *rn, ifp = if_lookup_by_index(nexthop->ifindex, nexthop->vrf_id); - if (ifp && (if_is_operative(ifp) || if_is_up(ifp))) { + if (ifp && if_is_up(ifp)) { SET_FLAG(nexthop->flags, NEXTHOP_FLAG_ACTIVE); goto skip_check; } From 098519caf8836f1bb0df9568ecd5daa5b4d5140b Mon Sep 17 00:00:00 2001 From: anlan_cs Date: Mon, 19 Jun 2023 10:21:28 +0800 Subject: [PATCH 2/3] zebra: fix wrong nexthop check for kernel routes When changing one interface's vrf, the kernel routes are wrongly kept in old vrf. Finally, the forwarding table in that old vrf can't forward traffic correctly for those residual entries. Follow these steps to make this problem happen: ( Firstly, "x1" interface of default vrf is with address of "6.6.6.6/24". ) ``` anlan# ip route add 4.4.4.0/24 via 6.6.6.8 dev x1 anlan# ip link add vrf1 type vrf table 1 anlan# ip link set vrf1 up anlan# ip link set x1 master vrf1 ``` Then check `show ip route`, the route of "4.4.4.0/24" is still selected in default vrf. If the interface goes down, the kernel routes will be reevaluated. Those kernel routes with active interface of nexthop can be kept no change, it is a fast path. Otherwise, it enters into slow path to do careful examination on this nexthop. After the interface's vrf had been changed into new vrf, the down message of this interface came. It means the interface is not in old vrf although it still exists during that checking, so the kernel routes should be dropped after this nexthop matching against a default route in slow path. But, in current code they are wrongly kept in fast path for not checking vrf. So, modified the checking active nexthop with vrf comparision for the interface during reevaluation. Signed-off-by: anlan_cs --- zebra/zebra_nhg.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index fb474b1add..68fce701c4 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2582,6 +2582,8 @@ static unsigned nexthop_active_check(struct route_node *rn, if (IS_ZEBRA_DEBUG_NHG_DETAIL) zlog_debug("%s: re %p, nexthop %pNHv", __func__, re, nexthop); + vrf_id = zvrf_id(rib_dest_vrf(rib_dest_from_rnode(rn))); + /* * If this is a kernel route, then if the interface is *up* then * by golly gee whiz it's a good route. @@ -2591,13 +2593,12 @@ static unsigned nexthop_active_check(struct route_node *rn, ifp = if_lookup_by_index(nexthop->ifindex, nexthop->vrf_id); - if (ifp && if_is_up(ifp)) { + if (ifp && ifp->vrf->vrf_id == vrf_id && if_is_up(ifp)) { SET_FLAG(nexthop->flags, NEXTHOP_FLAG_ACTIVE); goto skip_check; } } - vrf_id = zvrf_id(rib_dest_vrf(rib_dest_from_rnode(rn))); switch (nexthop->type) { case NEXTHOP_TYPE_IFINDEX: if (nexthop_active(nexthop, nhe, &rn->p, re->type, re->flags, From 019ac03e5b9fd46c51bd14f9f3a16c746e28fdac Mon Sep 17 00:00:00 2001 From: anlan_cs Date: Mon, 26 Jun 2023 18:20:58 +0800 Subject: [PATCH 3/3] tests: Check if kernel routes work with changed vrf Check `show ip route` for specific kernel routes after the interface as their nexthop changes vrf. After moving interface's vrf, there should be no kernel route in old vrf. Signed-off-by: anlan_cs --- .../zebra_rib/r1/v4_route_1_vrf_before.json | 27 ++++++++++++++ tests/topotests/zebra_rib/test_zebra_rib.py | 35 +++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 tests/topotests/zebra_rib/r1/v4_route_1_vrf_before.json diff --git a/tests/topotests/zebra_rib/r1/v4_route_1_vrf_before.json b/tests/topotests/zebra_rib/r1/v4_route_1_vrf_before.json new file mode 100644 index 0000000000..997d12f68d --- /dev/null +++ b/tests/topotests/zebra_rib/r1/v4_route_1_vrf_before.json @@ -0,0 +1,27 @@ +{ + "3.5.1.0/24":[ + { + "prefix":"3.5.1.0/24", + "prefixLen":24, + "protocol":"kernel", + "vrfId":0, + "vrfName":"default", + "selected":true, + "destSelected":true, + "distance":0, + "metric":0, + "installed":true, + "table":254, + "nexthops":[ + { + "flags":3, + "fib":true, + "ip":"192.168.210.1", + "afi":"ipv4", + "interfaceName":"r1-eth0", + "active":true + } + ] + } + ] +} diff --git a/tests/topotests/zebra_rib/test_zebra_rib.py b/tests/topotests/zebra_rib/test_zebra_rib.py index e2863218b0..c45ac82d30 100644 --- a/tests/topotests/zebra_rib/test_zebra_rib.py +++ b/tests/topotests/zebra_rib/test_zebra_rib.py @@ -77,6 +77,41 @@ def teardown_module(mod): tgen.stop_topology() +def test_zebra_kernel_route_vrf(): + "Test kernel routes should be removed after interface changes vrf" + logger.info("Test kernel routes should be removed after interface changes vrf") + vrf = "RED" + tgen = get_topogen() + r1 = tgen.gears["r1"] + + # Add kernel routes, the interface is initially in default vrf + r1.run("ip route add 3.5.1.0/24 via 192.168.210.1 dev r1-eth0") + json_file = "{}/r1/v4_route_1_vrf_before.json".format(CWD) + expected = json.loads(open(json_file).read()) + test_func = partial( + topotest.router_json_cmp, r1, "show ip route 3.5.1.0/24 json", expected + ) + _, result = topotest.run_and_expect(test_func, None, count=5, wait=1) + assert result is None, '"r1" JSON output mismatches' + + # Change the interface's vrf + r1.run("ip link add {} type vrf table 1".format(vrf)) + r1.run("ip link set {} up".format(vrf)) + r1.run("ip link set dev r1-eth0 master {}".format(vrf)) + + expected = "{}" + test_func = partial( + topotest.router_output_cmp, r1, "show ip route 3.5.1.0/24 json", expected + ) + result, diff = topotest.run_and_expect(test_func, "", count=5, wait=1) + assertmsg = "{} should not have the kernel route.\n{}".format('"r1"', diff) + assert result, assertmsg + + # Clean up + r1.run("ip link set dev r1-eth0 nomaster") + r1.run("ip link del dev {}".format(vrf)) + + def test_zebra_kernel_admin_distance(): "Test some basic kernel routes added that should be accepted" logger.info("Test some basic kernel routes that should be accepted")