Static stuff #1819

donaldsharp · 2018-03-02T15:10:10Z

It is possible to cause zebra to crash when you have static routes configured under a non-fully initialized vrf. These commits fix this issue.

NetDEF-CI · 2018-03-02T15:59:33Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2725/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

CLANG Static Analyzer Summary

Github Pull Request 1819, comparing to Git base SHA a8e31fc

No Changes in Static Analysis warnings compared to base

19 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2725/artifact/shared/static_analysis/index.html

LabN-CI · 2018-03-04T00:22:16Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/1819 `908e1b2`
Date	03/03/2018
Start	18:59:09
Finish	19:22:14
Run-Time	23:05
Total	1816
Pass	1816
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-03-03-18:59:09.txt
Log	autoscript-2018-03-03-18:59:50.log.bz2

For details, please contact louberger

qlyoung · 2018-03-05T20:21:01Z

zebra/zebra_vty.c

+			return CMD_SUCCESS;
+		}
+
+		assert(0);


I've seen a neat assert variant used elsewhere in the codebase that provides some debugging info, it looks like this:
assert(!"my error message")
It's helped me in the past with debugging. Alternatively a comment around this area might be good.

pguibert6WIND

2 remarks:

what about cases where routes are installed but fail due to other error ( no mpls, bad mpls value, bad format)
some errors should be displayed upon vrf on

pguibert6WIND · 2018-03-07T14:31:13Z

zebra/zebra_vty.c

+			shr->dest_str, shr->mask_str, shr->src_str,
+			shr->gate_str, shr->ifname, shr->flag_str, shr->tag_str,
+			shr->distance_str, shr->label_str);
+


getting the returned value of zebra_statis_route_leak could permit to know exactly if an entry has been installed or not ( case mpls is not present in kernel)

Not much to do with it other than to notice the rejection.

can't we remove the static entry from the list ?

pguibert6WIND · 2018-03-07T14:32:11Z

zebra/zebra_vty.c

-			vty_out(vty, "%% Nexthop interface cannot be Null0, reject or blackhole\n");
+			if (vty)
+				vty_out(vty,
+					"%% Nexthop interface cannot be Null0, reject or blackhole\n");
 			return CMD_WARNING_CONFIG_FAILED;


that event should be logged into a zlog_xxx

pguibert6WIND · 2018-03-07T14:32:22Z

zebra/zebra_vty.c

+					vty_out(vty,
+						"%% Cannot use reserved label(s) (%d-%d)\n",
+						MPLS_LABEL_RESERVED_MIN,
+						MPLS_LABEL_RESERVED_MAX);
 				break;


that event should be logged into a zlog_xxx

pguibert6WIND · 2018-03-07T14:32:27Z

zebra/zebra_vty.c

-				"%% MPLS not turned on in kernel, ignoring command\n");
+			if (vty)
+				vty_out(vty,
+					"%% MPLS not turned on in kernel, ignoring command\n");
 			return CMD_WARNING_CONFIG_FAILED;


that event should be logged into a zlog_xxx

When you have individual 'ip route..' commands under a VRF allow them to be displayed properly Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

When zebra is being configed we allow for static routes to be entered. This presents a problem for when a vrf is cli configed but not kernel configed yet. Modify zebra to notice that when a static route is entered and either the nexthop vrf or the vrf is not fully configed, to save that config to the side. When vrf's become active( kernel configed ) parse through the list of saved to the side static routes and determine if any of them can be installed. Additionally modify the cli to output the saved to the side cli, so that we can properly handle a wr mem. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

LabN-CI · 2018-03-07T20:10:15Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/1819 `4060008`
Date	03/07/2018
Start	14:35:21
Finish	14:58:07
Run-Time	22:46
Total	1816
Pass	1816
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-03-07-14:35:21.txt
Log	autoscript-2018-03-07-14:35:57.log.bz2

For details, please contact louberger

NetDEF-CI · 2018-03-07T20:26:36Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2793/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings:

Report for zebra_vty.c
===============================================
< WARNING: line over 80 characters
< #252: FILE: /tmp/f1/zebra_vty.c:252:
< +		assert(!"We should not have found a duplicate and not remove it");
<

CLANG Static Analyzer Summary

Github Pull Request 1819, comparing to Git base SHA 54085ea

No Changes in Static Analysis warnings compared to base

19 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2793/artifact/shared/static_analysis/index.html

rwestphal · 2018-03-12T16:01:47Z

I wonder if we couldn't solve this problem in a simpler way.

We used to have a similar problem with static routes pointing to non-existing interfaces. This was fixed by this commit: c3c04063

In short, the fix was to store nexthop interfaces using their names and not their ifindexes. Then, pior to calling static_install_route(), we'd try to find the corresponding ifindex using if_lookup_by_name(ifname, ...). If the interface wasn't found, then the route installation would be postponed until the interface was added to the system (if_add_update()).

I think we could try to do something similar here. We could change the static_route structure to store VRFs by their name and not by their index. Then, whenever a new VRF is detected in the system (zebra_vrf_enable()), loop through all static routes from all VRFs and detect which ones can now be installed with static_install_route() (both the VRF and nexthop-VRF must be available). What do you think about this?

rwestphal · 2018-03-12T16:10:23Z

Also, this diff doesn't solve the problem completely:

debian(config)# vrf RED
debian(config-vrf)# ip route 50.0.0.0/8 eth80 nexthop-vrf BLUE
% nexthop vrf BLUE is not defined
debian(config)# exit
debian(config)# ip route 60.0.0.0/8 eth50 vrf BLACK
% vrf BLACK is not defined

If we store VRF using their names then the commands above will work even if the VRFs don't exist.

pguibert6WIND

-I don't know if Renatos idea could be applied to this.
I would tend to think this change could be done.
-like that, my comment on "removing the static entry from the list " could be ignored, since all entries may be valid but will really be taken into account if underlying ( lets start with vrf) are available.

pguibert6WIND · 2018-03-08T09:06:57Z

zebra/zebra_vty.c

+			shr->dest_str, shr->mask_str, shr->src_str,
+			shr->gate_str, shr->ifname, shr->flag_str, shr->tag_str,
+			shr->distance_str, shr->label_str);
+


can't we remove the static entry from the list ?

pguibert6WIND · 2018-03-08T09:07:09Z

zebra/zebra_vty.c

-			vty_out(vty, "%% Nexthop interface cannot be Null0, reject or blackhole\n");
+			if (vty)
+				vty_out(vty,
+					"%% Nexthop interface cannot be Null0, reject or blackhole\n");
 			return CMD_WARNING_CONFIG_FAILED;


pguibert6WIND · 2018-03-14T14:21:30Z

I agree to keep the changes as they are
and track the continuation on #1819

rwestphal · 2018-03-14T14:25:44Z

In the light of #1887, I think these changes can get in and we should take the issue of non-existing VRFs into consideration when refactoring the handling of static routes in zebra. Will merge now.

qlyoung approved these changes Mar 5, 2018

View reviewed changes

donaldsharp requested review from rwestphal and pguibert6WIND March 6, 2018 17:03

pguibert6WIND reviewed Mar 7, 2018

View reviewed changes

donaldsharp added 2 commits March 7, 2018 13:50

zebra: Fix vrf output

2bfe7ba

When you have individual 'ip route..' commands under a VRF allow them to be displayed properly Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

donaldsharp force-pushed the static_stuff branch from 908e1b2 to 4060008 Compare March 7, 2018 19:34

rwestphal mentioned this pull request Mar 13, 2018

More nh groups #1863

Merged

pguibert6WIND requested changes Mar 13, 2018

View reviewed changes

pguibert6WIND approved these changes Mar 14, 2018

View reviewed changes

rwestphal merged commit f22ab4c into FRRouting:master Mar 14, 2018

donaldsharp deleted the static_stuff branch March 24, 2018 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static stuff #1819

Static stuff #1819

donaldsharp commented Mar 2, 2018

NetDEF-CI commented Mar 2, 2018

LabN-CI commented Mar 4, 2018

qlyoung Mar 5, 2018

donaldsharp Mar 7, 2018

pguibert6WIND left a comment

pguibert6WIND Mar 7, 2018

donaldsharp Mar 7, 2018

pguibert6WIND Mar 8, 2018

pguibert6WIND Mar 7, 2018

donaldsharp Mar 7, 2018

pguibert6WIND Mar 8, 2018

pguibert6WIND Mar 7, 2018

donaldsharp Mar 7, 2018

pguibert6WIND Mar 7, 2018

donaldsharp Mar 7, 2018

LabN-CI commented Mar 7, 2018

NetDEF-CI commented Mar 7, 2018

rwestphal commented Mar 12, 2018

rwestphal commented Mar 12, 2018

pguibert6WIND left a comment

pguibert6WIND Mar 8, 2018

pguibert6WIND Mar 8, 2018

pguibert6WIND commented Mar 14, 2018

rwestphal commented Mar 14, 2018 •

edited

Loading

Static stuff #1819

Static stuff #1819

Conversation

donaldsharp commented Mar 2, 2018

NetDEF-CI commented Mar 2, 2018

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

19 Static Analyzer issues remaining.

LabN-CI commented Mar 4, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND left a 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

LabN-CI commented Mar 7, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Mar 7, 2018

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

Checkout code: Successful with additional warnings:

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

19 Static Analyzer issues remaining.

rwestphal commented Mar 12, 2018

rwestphal commented Mar 12, 2018

pguibert6WIND left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND commented Mar 14, 2018

rwestphal commented Mar 14, 2018 • edited Loading

rwestphal commented Mar 14, 2018 •

edited

Loading