Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Local connected #16300

Merged
merged 5 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"192.168.44.0/24":[
{
"prefix":"192.168.44.0/24",
"prefixLen":24,
"protocol":"connected",
"vrfName":"default",
"selected":true,
"destSelected":true,
"distance":0,
"metric":0,
"installed":true,
"table":254,
"nexthops":[
{
"fib":true,
"directlyConnected":true,
"interfaceName":"r1-eth1",
"active":true
}
]
}
]
}
24 changes: 24 additions & 0 deletions tests/topotests/zebra_multiple_connected/r1/ip_route_kernel.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"4.5.6.7/32":[
{
"prefix":"4.5.6.7/32",
"prefixLen":32,
"protocol":"kernel",
"vrfName":"default",
"selected":true,
"destSelected":true,
"distance":0,
"metric":0,
"installed":true,
"table":254,
"nexthops":[
{
"fib":true,
"directlyConnected":true,
"interfaceName":"r1-eth1",
"active":true
}
]
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
import pytest
import json
from functools import partial
from lib.topolog import logger

pytestmark = pytest.mark.random_order(disabled=True)

# Save the Current Working Directory to find configuration files.
CWD = os.path.dirname(os.path.realpath(__file__))
Expand Down Expand Up @@ -159,6 +162,46 @@ def test_zebra_noprefix_connected():
assert result, "Connected Route should not have been added"


def test_zebra_noprefix_connected_add():
"Test that a noprefixroute created with a manual route works as expected, this is for NetworkManager"

tgen = get_topogen()
if tgen.routers_have_failure():
pytest.skip(tgen.errors)

router = tgen.gears["r1"]
router.run("ip route add 192.168.44.0/24 dev r1-eth1")

connected = "{}/{}/ip_route_connected.json".format(CWD, router.name)
expected = json.loads(open(connected).read())

test_func = partial(
topotest.router_json_cmp, router, "show ip route 192.168.44.0/24 json", expected
)
result, _ = topotest.run_and_expect(test_func, None, count=20, wait=1)
assert result, "Connected Route should have been added\n{}".format(_)


def test_zebra_kernel_route_add():
"Test that a random kernel route is properly handled as expected"

tgen = get_topogen()
if tgen.routers_have_failure():
pytest.skip(tgen.errors)

router = tgen.gears["r1"]
router.run("ip route add 4.5.6.7/32 dev r1-eth1")

kernel = "{}/{}/ip_route_kernel.json".format(CWD, router.name)
expected = json.loads(open(kernel).read())

test_func = partial(
topotest.router_json_cmp, router, "show ip route 4.5.6.7/32 json", expected
)
result, _ = topotest.run_and_expect(test_func, None, count=20, wait=1)
assert result, "Connected Route should have been added\n{}".format(_)


if __name__ == "__main__":
args = ["-s"] + sys.argv[1:]
sys.exit(pytest.main(args))
2 changes: 2 additions & 0 deletions zebra/interface.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

void if_refresh(struct interface *ifp)
Expand Down
5 changes: 4 additions & 1 deletion zebra/rib.h
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ typedef struct rib_tables_iter_t_ {

/* Events/reasons triggering a RIB update. */
enum rib_update_event {
RIB_UPDATE_INTERFACE_DOWN,
RIB_UPDATE_KERNEL,
RIB_UPDATE_RMAP_CHANGE,
RIB_UPDATE_OTHER,
Expand Down Expand Up @@ -395,7 +396,7 @@ extern int rib_add_multipath_nhe(afi_t afi, safi_t safi, struct prefix *p,

extern void rib_delete(afi_t afi, safi_t safi, vrf_id_t vrf_id, int type,
unsigned short instance, uint32_t flags,
struct prefix *p, struct prefix_ipv6 *src_p,
const struct prefix *p, const struct prefix_ipv6 *src_p,
const struct nexthop *nh, uint32_t nhe_id,
uint32_t table_id, uint32_t metric, uint8_t distance,
bool fromkernel);
Expand Down Expand Up @@ -477,6 +478,8 @@ extern uint8_t route_distance(int type);
extern void zebra_rib_evaluate_rn_nexthops(struct route_node *rn, uint32_t seq,
bool rt_delete);

extern void rib_update_handle_vrf_all(enum rib_update_event event, int rtype);

/*
* rib_find_rn_from_ctx
*
Expand Down
2 changes: 0 additions & 2 deletions zebra/rt_netlink.c
Original file line number Diff line number Diff line change
Expand Up @@ -799,8 +799,6 @@ int netlink_route_change_read_unicast_internal(struct nlmsghdr *h,
return 0;
if (rtm->rtm_protocol == RTPROT_REDIRECT)
return 0;
if (rtm->rtm_protocol == RTPROT_KERNEL)
return 0;

selfroute = is_selfroute(rtm->rtm_protocol);

Expand Down
115 changes: 99 additions & 16 deletions zebra/zebra_rib.c
Original file line number Diff line number Diff line change
Expand Up @@ -1619,6 +1619,10 @@ 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 &&
(re1->nhe->nhg.nexthop->ifindex == re2->nhe->nhg.nexthop->ifindex))
return true;

if (re1->type != ZEBRA_ROUTE_CONNECT && re1->type != ZEBRA_ROUTE_LOCAL)
return true;

Expand Down Expand Up @@ -2863,10 +2867,11 @@ static void process_subq_early_route_add(struct zebra_early_route *ere)

/* Link new re to node.*/
if (IS_ZEBRA_DEBUG_RIB) {
rnode_debug(
rn, re->vrf_id,
"Inserting route rn %p, re %p (%s) existing %p, same_count %d",
rn, re, zebra_route_string(re->type), same, same_count);
rnode_debug(rn, re->vrf_id,
"Inserting route rn %p, re %p (%s/%s/%s) existing %p, same_count %d",
rn, re, zebra_route_string(re->type),
afi2str(ere->afi), safi2str(ere->safi), same,
same_count);

if (IS_ZEBRA_DEBUG_RIB_DETAILED)
route_entry_dump(
Expand Down Expand Up @@ -4370,8 +4375,10 @@ int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p,
return -1;

/* We either need nexthop(s) or an existing nexthop id */
if (ng == NULL && re->nhe_id == 0)
if (ng == NULL && re->nhe_id == 0) {
zebra_rib_route_entry_free(re);
return -1;
}

/*
* Use a temporary nhe to convey info to the common/main api.
Expand All @@ -4383,6 +4390,34 @@ int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p,
nhe.id = re->nhe_id;

n = zebra_nhe_copy(&nhe, 0);

if (re->type == ZEBRA_ROUTE_KERNEL) {
struct interface *ifp;
struct connected *connected;

if (p->family == AF_INET6 &&
IN6_IS_ADDR_LINKLOCAL(&p->u.prefix6)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_nhg_free(n);
zebra_rib_route_entry_free(re);
return -1;
}

ifp = if_lookup_prefix(p, re->vrf_id);
if (ifp) {
connected = connected_lookup_prefix(ifp, p);

if (connected && !CHECK_FLAG(connected->flags,
ZEBRA_IFA_NOPREFIXROUTE)) {
zebra_nhg_free(n);
zebra_rib_route_entry_free(re);
return -1;
}

if (ifp->ifindex == ng->nexthop->ifindex)
re->type = ZEBRA_ROUTE_CONNECT;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}
}

ret = rib_add_multipath_nhe(afi, safi, p, src_p, re, n, startup);

/* In error cases, free the route also */
Expand All @@ -4393,8 +4428,8 @@ int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p,
}

void rib_delete(afi_t afi, safi_t safi, vrf_id_t vrf_id, int type,
unsigned short instance, uint32_t flags, struct prefix *p,
struct prefix_ipv6 *src_p, const struct nexthop *nh,
unsigned short instance, uint32_t flags, const struct prefix *p,
const struct prefix_ipv6 *src_p, const struct nexthop *nh,
uint32_t nhe_id, uint32_t table_id, uint32_t metric,
uint8_t distance, bool fromkernel)
{
Expand Down Expand Up @@ -4458,6 +4493,9 @@ static const char *rib_update_event2str(enum rib_update_event event)
const char *ret = "UNKNOWN";

switch (event) {
case RIB_UPDATE_INTERFACE_DOWN:
ret = "RIB_UPDATE_INTERFACE_DOWN";
break;
case RIB_UPDATE_KERNEL:
ret = "RIB_UPDATE_KERNEL";
break;
Expand All @@ -4474,15 +4512,56 @@ static const char *rib_update_event2str(enum rib_update_event event)
return ret;
}

/*
* We now keep kernel routes, but we don't have any
* trigger events for them when they are implicitly
* deleted. Since we are already walking the
* entire table on a down event let's look at
* the few kernel routes we may have
*/
static void
rib_update_handle_kernel_route_down_possibility(struct route_node *rn,
struct route_entry *re)
{
struct nexthop *nexthop = NULL;
bool alive = false;

for (ALL_NEXTHOPS(re->nhe->nhg, nexthop)) {
struct interface *ifp = if_lookup_by_index(nexthop->ifindex,
nexthop->vrf_id);

if (ifp && if_is_up(ifp)) {
alive = true;
mjstapp marked this conversation as resolved.
Show resolved Hide resolved
break;
}
}

if (!alive) {
struct rib_table_info *rib_table = srcdest_rnode_table_info(rn);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

const struct prefix *p;
const struct prefix_ipv6 *src_p;

srcdest_rnode_prefixes(rn, &p, (const struct prefix **)&src_p);

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
		}```

}
}


/* Schedule route nodes to be processed if they match the type */
static void rib_update_route_node(struct route_node *rn, int type)
static void rib_update_route_node(struct route_node *rn, int type,
enum rib_update_event event)
{
struct route_entry *re, *next;
bool re_changed = false;

RNODE_FOREACH_RE_SAFE (rn, re, next) {
if (type == ZEBRA_ROUTE_ALL || type == re->type) {
if (event == RIB_UPDATE_INTERFACE_DOWN && type == re->type &&
type == ZEBRA_ROUTE_KERNEL)
rib_update_handle_kernel_route_down_possibility(rn, re);
else if (type == ZEBRA_ROUTE_ALL || type == re->type) {
SET_FLAG(re->status, ROUTE_ENTRY_CHANGED);
re_changed = true;
}
Expand Down Expand Up @@ -4522,28 +4601,32 @@ void rib_update_table(struct route_table *table, enum rib_update_event event,
/*
* If we are looking at a route node and the node
* has already been queued we don't
* need to queue it up again
* need to queue it up again, unless it is
* an interface down event as that we need
* to process this no matter what.
*/
if (rn->info
&& CHECK_FLAG(rib_dest_from_rnode(rn)->flags,
RIB_ROUTE_ANY_QUEUED))
if (rn->info &&
CHECK_FLAG(rib_dest_from_rnode(rn)->flags,
RIB_ROUTE_ANY_QUEUED) &&
event != RIB_UPDATE_INTERFACE_DOWN)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

continue;

switch (event) {
case RIB_UPDATE_INTERFACE_DOWN:
case RIB_UPDATE_KERNEL:
rib_update_route_node(rn, ZEBRA_ROUTE_KERNEL);
rib_update_route_node(rn, ZEBRA_ROUTE_KERNEL, event);
break;
case RIB_UPDATE_RMAP_CHANGE:
case RIB_UPDATE_OTHER:
rib_update_route_node(rn, rtype);
rib_update_route_node(rn, rtype, event);
break;
case RIB_UPDATE_MAX:
break;
}
}
}

static void rib_update_handle_vrf_all(enum rib_update_event event, int rtype)
void rib_update_handle_vrf_all(enum rib_update_event event, int rtype)
{
struct zebra_router_table *zrt;

Expand Down
Loading