Created attachment 504106 [details, diff] proposed patch Hello. Currently netifrc iproute2 module does not support adding routes with types like "throw" or "blackhole". One of the reasons is because the "dev" option is added to the route unconditionally. I faced with the problem of adding such routes several times. And I was forced to do it by postup function which is not so convenient. I propose this patch to change this behaviour. It checks if specific types are set for the route and drops adding device for it. Also it drops using dev option for "route show" in couple of places. This is not needed to find the route and anyway two similar routes with different devices are not allowed, so it probably will behave better when detecting already defined routes. The only open problem with such routes is that they will not be flushed in interface stop. But I think it will not cause big problems, but the convenience is obvious.
There's 2 problems with your patch as-is: 1. "route show" It is possible (but uncommon) to have the same route on two different interfaces, with different metrics (if they have the same metric, the kernel will reject them as you noted for similar routes) -ip ${family} route show ${cmd_nometric} dev "${IFACE}" 2>/dev/null | \ +ip ${family} route show ${cmd_nometric} 2>/dev/null | \ ... -eval $msgfunc \"$(ip $family route show ${cmd_nometric} dev "${IFACE}" 2>&1)\" +eval $msgfunc \"$(ip $family route show ${cmd_nometric} 2>&1)\" I think that dev should be conditionally added here if it's not one of the no-interface route types. 2. loop for cmd_nometric: + throw|unreachable|prohibit|blackhole) cmd="${cmd} ${1}" ;; This line needs to ALSO append cmd_nometric.
> 1. "route show" Yes. I forgot that it calls "show" without metric when dropped the "dev" here. Thank you for noticing this. But there is a problem with "nodev" routes because if we add "dev", they they would not match. On the other side, metric can not be used for matching (I wonder why??). So it looks like there should be more ifs. :) > 2. loop for cmd_nometric: > + throw|unreachable|prohibit|blackhole) cmd="${cmd} ${1}" ;; > This line needs to ALSO append cmd_nometric. I disagree here. For example "ip route show throw 192.168.0.0/24" will not work. We can probably add "type throw" here, but on the other side it will match only throw routes, but there can be anoter matching routes, which will stop this route from adding, so they should be matched too. Probably the only way to overcome this is to run "ip route show" without metric, and then grep metric of interest. Should I try to do this?
(In reply to Alexander Zubkov from comment #2) > Probably the only way to overcome this is to run "ip route show" without > metric, and then grep metric of interest. Should I try to do this? It already does the grep: 250 # Check for route already existing: 251 ip ${family} route show ${cmd_nometric} dev "${IFACE}" 2>/dev/null | \ 252 fgrep -sq "${cmd%% *}" 253 route_already_exists=$? Build different variables for add vs show. - Rename cmd_nometric to cmd_show. - Rename cmd to cmd_add - For nodev types: -- Set nodev variable. -- cmd_add="${cmd} ${1}" ; cmd_show="${cmd_show} type ${1}" ; nodev=1 - After the loop that builds cmd_show/cmd_add, and the metric/nexthop check, add a new check for nodev variable, and append to cmd_show/cmd_add as needed.
Good. Thank you for the clues. That is was my question about - if I can do something in approved way. But why do you want to do this: > cmd_show="${cmd_show} type ${1}" For example if we have in routing table: 192.168.1.0/24 via 192.168.0.1 And we will try to add: ip route add throw 192.168.1.0/24 - it will fail, because the same prefix with the same metric already there. But this show: ip route show 192.168.1.0/24 type throw will not find that blocker.
(In reply to Alexander Zubkov from comment #4) > But this show: > > ip route show 192.168.1.0/24 type throw > > will not find that blocker. Ah, I see your concern here yes. So for cmd_show, I'm wondering if we can isolate the needed parts (prefix, vrf?, table?) and just pass that to show, and improve how we grep those results.
> # We cannot use a metric if we're using a nexthop But I can add route with metric and several nexthops. May be this is some legacy?
Created attachment 504454 [details, diff] netifrc-1.patch And here first patch for starting, I just renamed cmds and I think there was a bug also: > netmask) x="/$(_netmask2cidr "$2")" ; cmd="${cmd}${x}" ; cmd_nometric="${cmd}${x}" ; shift;; I think cmd_nometric should use cmd_nometric, not cmd? Or it was made intentionally?
(In reply to Alexander Zubkov from comment #7) > Created attachment 504454 [details, diff] [details, diff] > netifrc-1.patch > > And here first patch for starting, I just renamed cmds and I think there was > a bug also: > > > netmask) x="/$(_netmask2cidr "$2")" ; cmd="${cmd}${x}" ; cmd_nometric="${cmd}${x}" ; shift;; > > I think cmd_nometric should use cmd_nometric, not cmd? Or it was made > intentionally? Good catch, yes, that's a bug :-). So on top of this one, now have a second patch that adds the new types. Also that cmd_show ends up with just the prefix. (In reply to Alexander Zubkov from comment #6) > > # We cannot use a metric if we're using a nexthop > > But I can add route with metric and several nexthops. May be this is some > legacy? Good question, will need to dig into the history of that to see.
FYI efec0b5012efd7af5a882348c62e28f3e62c58fc in openrc.git is the original commit for nexthop vs metric.
It is interesting. But I still do not understand the reason for this. :) I'm not sure it needs to be kept. I only can suppose such options: - metric was placed after nexthop and broke things of course - it was confused with weight If this check will be removed - the only problem I can see if someone have 2 routes towards the same prefix. One with nexthops and metric 0 (default) and other of any kind with greater metric. After that change nexthops route can get greater metric than other route or the same. And this will change behaviour.
Created attachment 504470 [details, diff] netifrc-2.patch Second patch. It have several problems. I'm uploading it just to show the idea. There is a function which shows required routes and additionaly fiters metric we need. cmd_show contains only fields which identify route uniquely, except the metric: table, tos. The list of problems to fix: - cmd_show is missing prefix now, we need to figure out it for options, probably the best guess is the first non-special word - remove dev $IFACE when show, because it does not included in "unique key" of the routes - deal with the default route, because "ip route show default" shows all routes for IPv4 (bug?), not only default and "ip route show 0.0.0.0/0" shows only default, but default route is showed as "default" always
Created attachment 504532 [details, diff] netifrc-2.2.patch The thing getting little huge. I am attaching updated second patch for review. But I have not tested it yet, will do it later. It turned out there is no problem with "ip show default" because if we add family to the command - it works as expected. I also split cmd_add into two parts: cmd_prefix and cmd_add. The first should be safe to append metric to. So we can use our default metric with nexthop and similar routes.
Created attachment 505050 [details, diff] netifrc-iproute2-total.patch So I made some basic testing, fixed errors, add couple of additional improvements and here is combined patch. I have tested that routes are adding correctly including "nodev" routes, auto metric is working with multiple nexthop routes, when errors routes are showed in the warning with the same key fields. There is a little thing there. When appending route returns an error, it will dump routes with the same key fields. But as I noticed - routes are _appended_ and so it will fail only if such route really exists. And it will dump not only conflicted route, but all similar routes, which will stop _adding_ routes. Maybe such extended information is good or maybe cmd_add and cmd_show need to be redone to show only conflicting route for appending.
> Maybe such extended information is good or maybe cmd_add and cmd_show need to be redone to show only conflicting route for appending. But I think that it will be hard to cut only such route anyway, because not all options can be passed to show.
By the way, my patch to iproute2 to support filtering by metric was included into upstream: https://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git/commit/?id=ed7fdc950d641376352061f2799dd2d0dd72accd So when it will get all the way to the stable, that ugly part of scripting for metric filtering could be removed.
Anything stops this from going further? If there any questions to the patch - I can work on them more.
I have the same requirement here. We need to add some blackhole route, and I think here is the best place rather than rc.local, are there any plan? thanks.
I believe my patches is greatly outdated already. But I'm still here and can work on new patches if there is interest from the netifrc team.
(In reply to Alexander Zubkov from comment #18) > I believe my patches is greatly outdated already. But I'm still here and can > work on new patches if there is interest from the netifrc team. I can help get this into netifrc. Can you rebase on current and send as a PR to https://github.com/gentoo/netifrc ?
(In reply to matoro from comment #19) > I can help get this into netifrc. Can you rebase on current and send as a > PR to https://github.com/gentoo/netifrc ? Sure, I'll look into it. But it might take some time. Thanks!
(In reply to matoro from comment #19) > (In reply to Alexander Zubkov from comment #18) > > I believe my patches is greatly outdated already. But I'm still here and can > > work on new patches if there is interest from the netifrc team. > > I can help get this into netifrc. Can you rebase on current and send as a > PR to https://github.com/gentoo/netifrc ? Hi, I've created the pull request: https://github.com/gentoo/netifrc/pull/53 It deals only with nodev routes. But I still might be interested in improving other things. But they seems no so obvious now, I have some questions about some things, where can I put them? For example why some things were designed in that way or another - if there is something to improve, or it is done intentionally. For example I see that _add_route() parses the "family" options in the beginning, but they might arrive from net.lo. Also if it must support busybox "ip" command to, or we can expect iproute2 is used? Whether script is interpreted by bash or more restricted shell? Regarding metric, iproute2 list command can filter routes by metric for 6+ years already and this part may be simplified. But if busybox needs to be supported too, it will not be possible then.
(In reply to Alexander Zubkov from comment #21) > (In reply to matoro from comment #19) > > (In reply to Alexander Zubkov from comment #18) > > > I believe my patches is greatly outdated already. But I'm still here and can > > > work on new patches if there is interest from the netifrc team. > > > > I can help get this into netifrc. Can you rebase on current and send as a > > PR to https://github.com/gentoo/netifrc ? > > Hi, > > I've created the pull request: > > https://github.com/gentoo/netifrc/pull/53 Thanks, I added a comment there so you can tag the commits appropriately. I'll try to get rid of STYLE since it's long out of date. > It deals only with nodev routes. But I still might be interested in > improving other things. But they seems no so obvious now, I have some > questions about some things, where can I put them? For example why some > things were designed in that way or another - if there is something to > improve, or it is done intentionally. The original authors of most of the code are gone now, except Robin is around only sporadically. I signed up to the project to keep netifrc from being removed from Gentoo entirely, and as I am also not a dev I cannot directly merge code or make releases, only make a fuss until a developer does. But please do reach out on IRC (same nick as my name here) with any questions. We can discuss them together and try to identify what was intended, what can be removed, etc. > For example I see that _add_route() parses the "family" options in the > beginning, but they might arrive from net.lo. Also if it must support > busybox "ip" command to, or we can expect iproute2 is used? Whether script > is interpreted by bash or more restricted shell? We can assume iproute2 (see _netns in sh/functions.sh) but I believe shell compat needs to stick with POSIX sh. > Regarding metric, iproute2 list command can filter routes by metric for 6+ > years already and this part may be simplified. But if busybox needs to be > supported too, it will not be possible then. No, ignoring busybox ip is fine.
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/proj/netifrc.git/commit/?id=7c6a8de0c521ea474bccb0dbda4338ff293cdfc6 commit 7c6a8de0c521ea474bccb0dbda4338ff293cdfc6 Author: Alexander Zubkov <green@qrator.net> AuthorDate: 2024-05-10 21:38:02 +0000 Commit: Patrick McLean <chutzpah@gentoo.org> CommitDate: 2024-05-23 16:12:29 +0000 Allow setting blackhole-like routes There were several problems preventing usage of routes of types blackhole, prohibit, throw, unreachable in IFACE_routes variables: - Those route types do not allow to use dev in the route definition, but it was added unconditionally - As there is no dev, such routes are not flushed automatically by dev, they need to be remembered and deleted while stopping the interface - Route type must go before the prefix in the command, but first parameters have special meaning Signed-off-by: Alexander Zubkov <green@qrator.net> Closes: https://bugs.gentoo.org/637394 Closes: https://github.com/gentoo/netifrc/pull/53 X-Gentoo-Bug: 637394 X-Gentoo-Bug-URL: https://bugs.gentoo.org/637394 Signed-off-by: Patrick McLean <chutzpah@gentoo.org> init.d/net.lo.in | 13 ++++++++++--- net/iproute2.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 6 deletions(-)
