zebra: fix kernel-route's deletion on vrf#5553
Conversation
|
Before this patch, frr can't work fine about kernel-route deletion on vrf. with that log ( |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10147/ This is a comment from an automated CI system. clang_check |
zebra/rt_netlink.c
Outdated
| nh.bh_type = bh_type; | ||
| } | ||
| nh.ifindex = index; | ||
| nh.vrf_id = vrf_id; |
There was a problem hiding this comment.
The index( nh.ifindex ) should dictate the nh's vrf. This will break in a route leaking situation.
There was a problem hiding this comment.
ahh I see what you mean.
yes this code just needs to be changed to call parse_nexthop_unicast()
There was a problem hiding this comment.
That handles the vrf_id by looking up the ifindex and using the interface's if it finds it, but falls back to the one set here if it doesn't.
sworleys
left a comment
There was a problem hiding this comment.
The delete branch needs to be changed to call parse_nexthop_unicast() exactly as the create branch does. It does some handling for the vrf_id there that we also need to do in this path and might as well combine them with a common API.
|
I'm sorry to late the reaction for your great suggestion... :( (my work was really busy..) |
7f76b26 to
7350bf5
Compare
polychaeta
left a comment
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/1282d1b54ecfc2ee82772f7d77f8447d/raw/5395961336f9c7e7e688551ddb86fff60bf23243/cr_5553_1577082100.diff | git apply
diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c
index 1a041492c..29a341abb 100644
--- a/zebra/rt_netlink.c
+++ b/zebra/rt_netlink.c
@@ -789,8 +789,8 @@ static int netlink_route_change_read_unicast(struct nlmsghdr *h, ns_id_t ns_id,
struct nexthop nh;
nh = parse_nexthop_unicast(
- ns_id, rtm, tb, bh_type, index, prefsrc,
- gate, afi, vrf_id);
+ ns_id, rtm, tb, bh_type, index, prefsrc,
+ gate, afi, vrf_id);
rib_delete(afi, SAFI_UNICAST, vrf_id, proto, 0,
flags, &p, &src_p, &nh, 0, table,
metric, distance, true);
If you are a new contributor to FRR, please see our contributing guidelines.
zebra can catch the kernel's route deletion by netlink. but current FRR can't delete kernel-route on vrf(l3mdev) when kernel operator delete the route on out-side of FRR. It looks problem about kernel-route deletion. This problem is caused around _nexthop_cmp_no_labels(nh1,nh2) that checks the each nexthop's member 'vrf_id'. And _nexthop_cmp_no_labels's caller doesn't set the vrf_id of nexthop structure. This commit fix that case. Signed-off-by: Hiroki Shirokura <slank.dev@gmail.com>
7350bf5 to
760f39d
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10224/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: clang_check |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10223/ This is a comment from an automated CI system. clang_check |
|
@slankdev thank you for your contribution! This PR fixes a zebra cash when using FPM. I'd just note that it introduces a few log messages: To reproduce it just follow the steps I've mentioned in the issue. I don't think it is necessary to fix it here, but if someone is willing why not. |
Those are due to the route being deleted now and us try to delete the nexthop groups in the kernel we are using for them. I think this should probably be a separate issue (assign it to me). I was under the impression nexthop groups weren't deleted with vrf deletion but it looks like that might not be the case. I will need to investigate this further. |
zebra can catch the kernel's route deletion by netlink.
but current FRR can't delete kernel-route on vrf(l3mdev)
when kernel operator delete the route on out-side of FRR.
It looks problem about kernel-route deletion.
This problem is caused around _nexthop_cmp_no_labels(nh1,nh2)
that checks the each nexthop's member 'vrf_id'.
And _nexthop_cmp_no_labels's caller doesn't set the vrf_id
of nexthop structure. This commit fix that case.
Signed-off-by: Hiroki Shirokura slank.dev@gmail.com