Local connected #16300

donaldsharp · 2024-06-26T17:23:21Z

Allow kernel routes to be read in when they are created in linux. Additionally treat the connected routes as connected. This fixes one of the issues seen with NOPREFIXROUTE

mjstapp · 2024-06-26T20:50:24Z

zebra/zebra_rib.c

@@ -1599,7 +1599,8 @@ static void zebra_rib_fixup_system(struct route_node *rn)
 }

 /* Route comparison logic, with various special cases. */
-static bool rib_compare_routes(const struct route_entry *re1,
+static bool rib_compare_routes(const struct route_node *rn,


don't see "rn" used?

yeah that is left over from some debugging I added.

mjstapp · 2024-06-26T20:50:54Z

zebra/zebra_rib.c

@@ -1619,6 +1620,11 @@ static bool rib_compare_routes(const struct route_entry *re1,
 	 * v6 link-locals, and we also support multiple addresses in the same
 	 * subnet on a single interface.
 	 */
+	if ((re1->type == ZEBRA_ROUTE_CONNECT &&
+	     re2->type == ZEBRA_ROUTE_CONNECT) &&


we already tested that the two types are equal - can just test either re1 or re2?

good point, fixed

mjstapp · 2024-06-26T20:52:16Z

zebra/zebra_rib.c

+		struct interface *ifp;
+
+		if (p->family == AF_INET6 &&
+		    IN6_IS_ADDR_LINKLOCAL(&p->u.prefix6)) {


interesting - don't protocols sometimes want to know about the link-locals though?

not as a kernel route. We already have code that handles the link locals. This is just a special case for how we are receiving the v6 LL routes from the kernel as far as I can tell.

zebra/zebra_rib.c

mjstapp · 2024-06-26T20:56:06Z

zebra/zebra_rib.c

+	}
+
+	if (!alive) {
+		struct rib_table_info *rib_table = srcdest_rnode_table_info(rn);


it seems a little too bad to have to do these lookups for each prefix - could we pass something useful down from rib_update_table or rib_update_route_node, like the afi/safi?

I looked at that and it was adding several layers of parameters. Just seemed very very easy to look it up in this special case instead of changing a whole bunch of functions to just pass afi/safi in for the !alive case( which should not be many routes)

riw777

looks good

riw777 · 2024-07-02T14:12:25Z

looks like this might be related --

E   AssertionError: Testcase test_verify_default_originate_after_BGP_and_FRR_restart_p2 : Failed : Redistributed routes from R0 is not learned in Router R1 RIB 
Error: Missing route in RIB of router r1, routes: ['2001:db8:f::1:17/128']
assert "Missing route in RIB of router r1, routes: ['2001:db8:f::1:17/128']\n" is True

but I'm trying to rerun just that test again to make certain

tohojo · 2024-07-24T12:52:37Z

zebra/zebra_rib.c

+		ifp = if_lookup_prefix(p, re->vrf_id);
+		if (ifp) {
+			if (ifp->ifindex == ng->nexthop->ifindex)
+				re->type = ZEBRA_ROUTE_CONNECT;


So this only considers a route to be connected if it exactly matches a prefix installed on the interface. However, the kernel is perfectly happy to accept "connected" prefixes where this is not the case (and routing works just fine with them):

# ip -br a lo UNKNOWN 127.0.0.1/8 ::1/128 veth0@if5 UP 10.10.10.10/32 fe80::2ceb:48ff:fe4a:954f/64 # ip r # ip r add 10.1.1.0/24 dev veth0 # ip r add 10.10.10.11/32 dev veth0 # ip r add default dev veth0 # ip r default dev veth0 scope link 10.1.1.0/24 dev veth0 scope link 10.10.10.11 dev veth0 scope link # ping -c 1 10.1.1.1 PING 10.1.1.1 (10.1.1.1) 56(84) bytes of data. 64 bytes from 10.1.1.1: icmp_seq=1 ttl=64 time=0.072 ms --- 10.1.1.1 ping statistics --- 1 packets transmitted, 1 received, 0% packet loss, time 0ms rtt min/avg/max/mdev = 0.072/0.072/0.072/0.000 ms

So with that in mind, wouldn't it be better to use the kernel's notion of a connected route (scope link) to set the ZEBRA_ROUTE_CONNECT type, instead of using this heuristic? That's the approach I took in #16418 for the same reason, but cf the discussion there, it's probably easier to incorporate that bit into this PR? :)

I would prefer that we keep things the way we have them as that this is how we think about the problem in FRR. I'm not sure that it matters though.

to clarify, my point a bit more. I do not want any route that has link scope to become a connected route. A connected route is a route that has an address associated with it on the interface in question. That is the definition that I want to stick with.

Right, okay. My concern was mostly that by not mirroring the exact semantics of the kernel FRR can end up with a different view of how packets will be routed than what the forwarding plane actually does. However, I don't have a concrete use case where this will lead to problems, and I guess for the original "set noprefixroute but install another replacement route for the prefix in the kernel" use case, this is fine :)

be aware that this patch will just turn those local scope routes into kernel routes so nothing to worry about

Ah, right, so FRR will still end up with a consistent view of the forwarding state (because it no longer skips all RTPROT_KERNEL routes), but only those with a matching interface prefix will be considered connected? Makes sense!

mjstapp · 2024-08-22T14:50:59Z

zebra/zebra_rib.c

+
+		rib_delete(rib_table->afi, rib_table->safi, re->vrf_id,
+			   re->type, re->instance, re->flags, p, src_p, NULL, 0,
+			   re->table, re->metric, re->distance, true);


that last "true" is the "fromkernel" boolean, right? Will that cause problems, since that usually indicates an update arriving from the kernel, where this is an update that zebra is generating?

in this code path I intentionally wanted to hit the code in zebra_rib.c:

if (same) { struct nexthop *tmp_nh; if (ere->fromkernel && CHECK_FLAG(ere->re->flags, ZEBRA_FLAG_SELFROUTE) && !zrouter.allow_delete) { rib_install_kernel(rn, same, NULL); route_unlock_node(rn); early_route_memory_free(ere); return; }```

mjstapp · 2024-08-22T14:55:44Z

zebra/zebra_rib.c

+		if (rn->info &&
+		    CHECK_FLAG(rib_dest_from_rnode(rn)->flags,
+			       RIB_ROUTE_ANY_QUEUED) &&
+		    event != RIB_UPDATE_INTERFACE_DOWN)


could the comment be updated a bit to explain this added test?
are there any possible ordering problems, if the dest is already queued for processing, and then we enqueue a delete event in rib_update_handle_kernel_route_down_possibility() ?

mjstapp · 2024-08-22T14:59:08Z

zebra/interface.c

@@ -1058,6 +1058,8 @@ void if_down(struct interface *ifp)

 	/* Delete all neighbor addresses learnt through IPv6 RA */
 	if_down_del_nbr_connected(ifp);
+
+	rib_update_handle_vrf_all(RIB_UPDATE_INTERFACE_DOWN, ZEBRA_ROUTE_KERNEL);


so this is going to do this processing synchronously. that "handle_vrf_all()" is currently an async event callback, via "rib_update()": why not use that path to do the work? was there some issue with doing it async?

that whole system area is a mess of code. I was just hooking into it quickly. I need to rethink that whole mess if we want to do it async. Although I am not sure it matters at this point in time. Can I add it to the list of things that needs to be reworked and rethought?

The prefix'es p and src_p are not const. Let's make them so. Useful to signal that we will not change this data. Signed-off-by: Donald Sharp <[email protected]>

This function will be used on interface down events to allow for kernel routes to be cleaned up. Signed-off-by: Donald Sharp <[email protected]>

Current code intentionally ignores kernel routes. Modify zebra to allow these routes to be read in on linux. Also modify zebra to look to see if a route should be treated as a connected and mark it as such. Additionally this should properly handle some of the issues being seen with NOPREFIXROUTE. Signed-off-by: Donald Sharp <[email protected]>

There exists a path in rib_add_multipath where if a decision is made to not use the passed in re, we just drop the memory instead of freeing it. Let's free it. Signed-off-by: Donald Sharp <[email protected]>

a) A noprefix address by itself should not create a connected route. This was pre-existing. b) A noprefix address with a corresponding route should result in a connected route. This is how NetworkManager appears to work. This is new behavior, so a new test. c) A route is added to the system from someone else. This is new behavior, so a new test. Signed-off-by: Donald Sharp <[email protected]>

mjstapp

ok, approving

New Features Highlight: - PIM candidate BSR/RP [#16438] - Static IGMP join without an IGMP report [1#6450] - PIM AutoRP discovery/announcements [#16634] - IGMP proxy [#16861] - SRv6 SID Manager [#15604] - Add `bgp ipv6-auto-ra` command [#16354] - Implement `neighbor x remote-as auto` for BGP [#16345] - Implement `bgp dual-as` for BGP [#16816] - Implement BGP-wide configuration for graceful restart [#16099] - Handle kernel routes appropriately (should fix recent NOPREFIXROUTE issue) [#16300] - Add `cisco-authentication` password support for NHRP [#16172] Signed-off-by: Jafar Al-Gharaibeh <[email protected]>

frrbot bot added the zebra label Jun 26, 2024

github-actions bot added size/L master labels Jun 26, 2024

mjstapp reviewed Jun 26, 2024

View reviewed changes

donaldsharp force-pushed the local_connected branch from 60806f3 to e506c59 Compare June 27, 2024 14:07

riw777 approved these changes Jul 2, 2024

View reviewed changes

donaldsharp mentioned this pull request Jul 2, 2024

zebra: Add an explicit warning/notice when noprefixroute flag is used #16288

Closed

donaldsharp force-pushed the local_connected branch from e506c59 to cb12b33 Compare July 2, 2024 19:02

tohojo mentioned this pull request Jul 17, 2024

zebra: Import the kernel's notion of connected routes #16418

Closed

tohojo reviewed Jul 24, 2024

View reviewed changes

donaldsharp force-pushed the local_connected branch from cb12b33 to b8cb706 Compare August 15, 2024 20:06

frrbot bot added the tests Topotests, make check, etc label Aug 15, 2024

donaldsharp force-pushed the local_connected branch from b8cb706 to 8b6f1df Compare August 22, 2024 12:12

mjstapp reviewed Aug 22, 2024

View reviewed changes

donaldsharp force-pushed the local_connected branch from 8b6f1df to 9b9f23d Compare August 23, 2024 18:25

donaldsharp added 5 commits August 27, 2024 06:25

zebra: Make p and src_p const for rib_delete

f450e1c

The prefix'es p and src_p are not const. Let's make them so. Useful to signal that we will not change this data. Signed-off-by: Donald Sharp <[email protected]>

zebra: Expose rib_update_handle_vrf_all

bdfccf6

This function will be used on interface down events to allow for kernel routes to be cleaned up. Signed-off-by: Donald Sharp <[email protected]>

zebra: Prevent accidental re memory leak in odd case

9bc0cd8

There exists a path in rib_add_multipath where if a decision is made to not use the passed in re, we just drop the memory instead of freeing it. Let's free it. Signed-off-by: Donald Sharp <[email protected]>

donaldsharp force-pushed the local_connected branch from 9b9f23d to 37dd518 Compare August 27, 2024 10:26

mjstapp approved these changes Aug 28, 2024

View reviewed changes

mjstapp merged commit 8b23abf into FRRouting:master Aug 28, 2024
11 checks passed

k0ste mentioned this pull request Sep 20, 2024

Problems with BGP routes on FRR 10.0-01 #16101

Closed

2 tasks

log1cb0mb mentioned this pull request Oct 19, 2024

RTPROT_KERNEL Causing IPv4PodCIDR Route Discovery Failure in Quagga and FRR cilium/cilium#31243

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local connected #16300

Local connected #16300

donaldsharp commented Jun 26, 2024

mjstapp Jun 26, 2024

donaldsharp Jun 27, 2024

donaldsharp Jun 27, 2024

mjstapp Jun 26, 2024

donaldsharp Jun 27, 2024

mjstapp Jun 26, 2024

donaldsharp Jun 27, 2024

mjstapp Jun 26, 2024

donaldsharp Jun 27, 2024

riw777 left a comment

riw777 commented Jul 2, 2024 •

edited

Loading

tohojo Jul 24, 2024

donaldsharp Aug 15, 2024

donaldsharp Aug 16, 2024

tohojo Aug 16, 2024

donaldsharp Aug 16, 2024

tohojo Aug 16, 2024

mjstapp Aug 22, 2024

donaldsharp Aug 23, 2024

mjstapp Aug 22, 2024

donaldsharp Aug 23, 2024

mjstapp Aug 22, 2024

donaldsharp Aug 23, 2024

mjstapp left a comment

Local connected #16300

Local connected #16300

Conversation

donaldsharp commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riw777 left a comment

Choose a reason for hiding this comment

riw777 commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjstapp left a comment

Choose a reason for hiding this comment

riw777 commented Jul 2, 2024 •

edited

Loading