zebra route-leaking for static routes #1618

donaldsharp · 2018-01-10T16:29:24Z

This code allows zebra to read routes from the linux kernel that have nexthops in a different vrf and then properly display them in various 'show .. route' commands and to disseminate the nexthops vrf up to the different routing protocols.

Additionally routing protocols now have the ability to send to zebra routes that are leaked, currently no routing protocol takes advantage of this yet.

nexthops are currently limited to 1 vrf.

static routes can now be created that have nexthops in different vrfs.

This supersedes #1607

LabN-CI · 2018-01-10T17:13:02Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/1618 `f1a027a`
Date	01/10/2018
Start	11:50:10
Finish	12:13:01
Run-Time	22:51
Total	1805
Pass	1805
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-01-10-11:50:10.txt
Log	autoscript-2018-01-10-11:50:47.log.bz2

For details, please contact louberger

NetDEF-CI · 2018-01-10T17:18:56Z

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-2286/

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 1618, comparing to Git base SHA d215f9a

No Changes in Static Analysis warnings compared to base

21 Static Analyzer issues remaining.

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

riw777

Other than the one observation, which I don't think is an issue, these changes look fine to me.

riw777 · 2018-01-11T01:06:59Z

lib/if.c

+	if (vrf_id == VRF_UNKNOWN) {
+		struct interface *ifp;
+
+		RB_FOREACH(vrf, vrf_id_head, &vrfs_by_id) {


Observation -- this will find the first instance of a nexthop across all the vrf's -- if there are multiple instances (say 10.1.1.1 exists in two differen vrfs) it will only find the first. I think this is okay, and I don't know what else you would do here, just wanted to bring it up to make certain this case is considered.

This is a lookup by ifindex not an ip address, and ifindex's are unique in the system so I think we are good

Ah, yes -- this makes sense. This is good then.

ifindexes won't be unique when we add support for using netns's as VRFs :/

that sucks :(

rwestphal · 2018-01-11T01:30:47Z

Overall looks good to me.

A few comments:
1- Route leaking doesn't seem to work when we don't specify the nexthop interface. Example:

debian# sh run
[snip]
vrf-red
 ip route 100.0.0.0/8 1.1.1.1 nexthop-vrf vrf-blue
!

debian# show ip route vrf vrf-red
[snip]
VRF vrf-red:
S   101.0.0.0/8 [1/0] via 1.1.1.1(vrf vrf-blue) inactive, 00:32:12

debian# show ip route vrf vrf-blue
VRF vrf-blue:
C>* 1.1.1.1/32 is directly connected, rt1-eth1, 00:34:46

2 - You can change the vrf_var_handlers variable in lib/vrf.c to allow the CLI auto-complete to work for nexthop-vrf.

3 - I think that the vrf option in the "ip route" commands is now redundant and can be removed, since static routes can be configured under the VRF node (much better!). If we do this, we can reuse the same DEFPYs in both the CONFIG_NODE and VRF_NODE, which would remove a considerable amount of code duplication. BTW, any reason for not providing the nexthop-vrf option for static routes in the global VRF?

donaldsharp · 2018-01-11T15:08:12Z

I'm working on this now. Hopefully today.
I was aware of that. Was going to do that once I got further into the project.
I left the vrf keyword in for backwards compatibility, I didn't see a way to not break someone. I will add the ability for nexthop-vrf under global in a few minutes here.

NetDEF-CI · 2018-01-11T17:45:11Z

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-2293/

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 1618, comparing to Git base SHA d215f9a

No Changes in Static Analysis warnings compared to base

21 Static Analyzer issues remaining.

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

donaldsharp · 2018-01-11T17:48:21Z

I've fixed #1 and #3. I'm going to do #2 later as that it wasn't obvious what I was missing. I'd like to get back to working on the bgp side of this.

LabN-CI · 2018-01-11T18:10:56Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/1618 `e481a68`
Date	01/11/2018
Start	12:48:06
Finish	13:10:55
Run-Time	22:49
Total	1808
Pass	1808
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-01-11-12:48:06.txt
Log	autoscript-2018-01-11-12:48:41.log.bz2

For details, please contact louberger

NetDEF-CI · 2018-01-11T18:16:27Z

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-2295/

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 1618, comparing to Git base SHA c124004

No Changes in Static Analysis warnings compared to base

21 Static Analyzer issues remaining.

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

rwestphal

Looks good to me.

I left the vrf keyword in for backwards compatibility

Fair enough. Just one concern: isn't frr-reload.py going to have issues if we allow the same command to be configured in two different ways? Maybe @dwalton76 can give us a hand here.

donaldsharp · 2018-01-11T23:54:06Z

frr-reload is going to have issues and it's on my list of things to fix already. I'll publish my work swag in the morning.

The zapi_ipv4_route, zapi_ipv6_route and zapi_ipv4_route_ipv6_nexthop functions are deprecated. Add notice of when we can remove the deprecated code from the system. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Modify if_lookup_by_index to accept a VRF_UNKNOWN as a vrf_id. This will cause it to look in all vrf's for the interface pointer. Subsequently all if_XXXX functions that call this function will also get this behavior. VRF_UNKNOWN *should* not be used for interface creation as that this will break some core assumptions. This work is part of allowing vrf route leaking. Currently it is possible to create a route in the linux kernel that has a nexthop across vrf boundaries. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

With VRF route-leaking we need to know what vrf the nexthops are in compared to this vrf. This code adds the nh_vrf_id to the route entry and sets it up correctly for the non-route-leaking case. The assumption here is that future commits will make the nh_vrf_id *different* than the vrf_id. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Use the nexthop vrf_id to properly lookup the ifp pointer for display purposes. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Add to the rib_add function the ability to pass in the nexthops vrf. Additionally when we decode the netlink message from the linux kernel, properly figure out the nexthops vrf_id. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

When we are handling nexthops in zebra, use the appropriate vrf to figure out if the nexthops are active or not. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

If the vrf for the nexthop is different than the vrf the route is in, display the nexthops vrf. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Modify the code to send and receive to/from zebra the nexthops vrf_id. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Move the NS/VRF initialization code for zebra to an earlier point in startup. In the future we will have code that will want to install_element into a VRF_NODE from zebra_vty.c Signed-off-by: Donald Sharp <sahrpd@cumulusnetworks.com>

Add a function to handle the route leaking of a static route. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Optimize vrf_config_write a tiny bit to be a bit more efficient. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Add the ability to accept 'ip route ...' commands from within a vrf context. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Move the code that generates the 'show run' output for 'ip route' to be controlled by the vrf config generation code. Since it really belongs there. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

In order for routes to be leaked the ifindex must be sent down into the kernel over the netlink protocol. So send it( we always figure it out ) when we add the route. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Allow this to work: vrf DONNA ip route 4.3.2.1/32 192.168.1.5 nexthop-vrf EVA The static route code was not properly telling the nexthop resolution code what vrf to use. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Allow the end user to specify static routes that leak across vrf's in the default vrf. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

donaldsharp · 2018-01-12T14:27:51Z

I've rebased code to fix merge issues from evpn getting in.

LabN-CI · 2018-01-12T15:02:57Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/1618 `6140853`
Date	01/12/2018
Start	09:40:09
Finish	10:02:55
Run-Time	22:46
Total	1808
Pass	1808
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-01-12-09:40:09.txt
Log	autoscript-2018-01-12-09:40:45.log.bz2

For details, please contact louberger

NetDEF-CI · 2018-01-12T15:18:23Z

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-2306/

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 1618, comparing to Git base SHA b782607

No Changes in Static Analysis warnings compared to base

21 Static Analyzer issues remaining.

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

louberger

So it's unfortunate that VRF_DEFAULT is zero and unknown is 0xffffffff. If we reversed this, then it would be possible to use vrf_id when vrf_id =0=UNKNOWN and it wouldn't be necessary to change all users of zapi. @donaldsharp what do you think it it worth making this change, or does its downsides outweigh the benefit...

donaldsharp · 2018-01-16T18:48:34Z

I'm nervous about making such a change. Especially in places where we assume the vrf_id =0 == tableid. I also know I have run across places where the code assumes VRF_DEFAULT is 0 and things just work because of this.

donaldsharp · 2018-01-19T13:39:16Z

Created #1657 to track the VRF_DEFAULT/VRF_UNKNOWN idea

pguibert6WIND · 2018-01-19T14:25:43Z

zebra/rt_netlink.c

+					nh_vrf_id = ifp->vrf_id;
+			}
+
+			rib_add(afi, SAFI_UNICAST, vrf_id, nh_vrf_id, proto,
 				0, flags, &p, NULL, &nh, table, metric,


my understanding is that vrf route leak mechanism is not yet available on netns.
so I think that lookup mechanism should be put in place only for vrf with backend vrf lite.

at least what I will do is : if (!(vrf_id has backend netns) && index)

I would suggest that you stop route leaking before we get to this point and just leave this code alone. It should not care about the underlying mechanism

I have some other code in #1633 where I will have to know the kind of backend.
mainly because you can have the same interface name and interface index name for different vrfs.
so when you say "stop route leaking before", I need to ack with you that I will have that case very quickly.

pguibert6WIND · 2018-01-19T14:38:52Z

zebra/rt_netlink.c

+	 * This is especially useful if we are doing route
+	 * leaking.
+	 */
+	if (nexthop->type != NEXTHOP_TYPE_BLACKHOLE)
 		addattr32(nlmsg, req_size, RTA_OIF, nexthop->ifindex);



if the vrf has a backend netns, one could imagine to send the NETNSID value, in addition to the ifindex.

I can't see how that could help in this part of the code. For route leaking with the netns backend, we should use the same netlink install functions. But the routes should point to some sort of virtual device (e.g. veth or macvlans) to make the leaking possible. Since netns isolation happens deep down in the network stack (at layer 2), we can't install a route pointing to an interface from a different netns (each netns has its own list of interfaces). What needs to be done is to install a route pointing to a different netns using some sort of internal network (e.g. 169.254.0.0/16), and rely on a second route-lookup in this netns to route the packet to its actual next hop.

Hi @rwestphal ,
I see two questions:

the kernel implementation perspective
I don"t know what exactly the kernel needs to do route leaking through netns.
there will be attributes that will be retrieved from the route table, and perhaps a new attribute will be stored in the nexthop information: the NSID.
what is sure is that currently, this feature does not exist.

the config trick that consists in using veth interfaces for instance.

That remark, I don't plan to do modifications in relationship to 1 ( since none kernel do it).
I also think that with the proposal of using veth, one can do the same route leak mechanism with current vty level..

I got your point now. Yes, it would be nice to have direct support for netns route leaking in the Linux kernel. But I'm afraid that this feature might not be implemented due to security concerns.

I also think that with the proposal of using veth, one can do the same route leak mechanism with current vty level..

Indeed. But ideally we should provide some level of abstraction so the operator can create inter-VRF routes in the same way regardless of the VRF backend.

louberger

looks ready

louberger · 2018-01-19T16:44:42Z

@pguibert6WIND think this is ready to merge once you're ready...

pguibert6WIND · 2018-01-22T17:34:09Z

it is ok for my side.
I wait for a last comment from Donald about the discussion we had zebra/rt_netlink.c .
things are clarified from my side.

donaldsharp · 2018-01-22T17:56:38Z

IMO, we should not allow route leaking to occur in a namespaced based VRF by preventing the creation of a VRF route leak at the cli level of the code, instead of having to make the underpinnings aware of this distinction.

donaldsharp mentioned this pull request Jan 10, 2018

Vrf route leaking Start #1607

Closed

donaldsharp requested a review from louberger January 10, 2018 16:29

rwestphal self-requested a review January 10, 2018 20:17

riw777 approved these changes Jan 11, 2018

View reviewed changes

donaldsharp force-pushed the zebra_startup_ordering branch from 9596a59 to e481a68 Compare January 11, 2018 17:24

rwestphal approved these changes Jan 11, 2018

View reviewed changes

donaldsharp added 15 commits January 12, 2018 09:19

lib: Add notice of when we can remove some deprecated code.

0cb76b9

The zapi_ipv4_route, zapi_ipv6_route and zapi_ipv4_route_ipv6_nexthop functions are deprecated. Add notice of when we can remove the deprecated code from the system. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: Use the correct vrf id to lookup the ifp pointer

99b9d96

Use the nexthop vrf_id to properly lookup the ifp pointer for display purposes. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: Add nh_vrf_id to rib_add

8795f90

Add to the rib_add function the ability to pass in the nexthops vrf. Additionally when we decode the netlink message from the linux kernel, properly figure out the nexthops vrf_id. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: When handling nexthops use the correct vrf

007dbee

When we are handling nexthops in zebra, use the appropriate vrf to figure out if the nexthops are active or not. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: When displaying nexthop information show correct vrf

2793a09

If the vrf for the nexthop is different than the vrf the route is in, display the nexthops vrf. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

*: Send/receive the nexthop vrf_id

d5b2119

Modify the code to send and receive to/from zebra the nexthops vrf_id. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

lib: Increment zapi version number

9ceac4c

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: Move NS/VRF initialization earlier

f84fc2c

Move the NS/VRF initialization code for zebra to an earlier point in startup. In the future we will have code that will want to install_element into a VRF_NODE from zebra_vty.c Signed-off-by: Donald Sharp <sahrpd@cumulusnetworks.com>

zebra: Add zebra_static_route_leak function

2f03bc8

Add a function to handle the route leaking of a static route. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: Add the zvrf pointer to the 'struct static_route'

cbb0dbf

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: Cleanup vrf_config_write

ab32921

Optimize vrf_config_write a tiny bit to be a bit more efficient. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: Add vrf level 'ip route ...' commands

b2ffa06

Add the ability to accept 'ip route ...' commands from within a vrf context. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: Move ip route ... generation to vrf control

3772804

Move the code that generates the 'show run' output for 'ip route' to be controlled by the vrf config generation code. Since it really belongs there. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

donaldsharp added 3 commits January 12, 2018 09:26

zebra: Encode the ifindex over netlink

5e21052

In order for routes to be leaked the ifindex must be sent down into the kernel over the netlink protocol. So send it( we always figure it out ) when we add the route. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: Allow static non interface based routes to leak

5bdd34d

Allow this to work: vrf DONNA ip route 4.3.2.1/32 192.168.1.5 nexthop-vrf EVA The static route code was not properly telling the nexthop resolution code what vrf to use. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

zebra: Add ability for default vrf to route-leak

6140853

Allow the end user to specify static routes that leak across vrf's in the default vrf. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

donaldsharp force-pushed the zebra_startup_ordering branch from e481a68 to 6140853 Compare January 12, 2018 14:27

louberger reviewed Jan 12, 2018

View reviewed changes

pguibert6WIND reviewed Jan 19, 2018

View reviewed changes

louberger approved these changes Jan 19, 2018

View reviewed changes

pguibert6WIND merged commit d6fed38 into FRRouting:master Jan 23, 2018

donaldsharp deleted the zebra_startup_ordering branch March 24, 2018 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zebra route-leaking for static routes #1618

zebra route-leaking for static routes #1618

donaldsharp commented Jan 10, 2018

LabN-CI commented Jan 10, 2018

NetDEF-CI commented Jan 10, 2018

riw777 left a comment

riw777 Jan 11, 2018

donaldsharp Jan 11, 2018

riw777 Jan 11, 2018

rwestphal Jan 11, 2018

donaldsharp Jan 11, 2018

rwestphal commented Jan 11, 2018

donaldsharp commented Jan 11, 2018

NetDEF-CI commented Jan 11, 2018

donaldsharp commented Jan 11, 2018

LabN-CI commented Jan 11, 2018

NetDEF-CI commented Jan 11, 2018

rwestphal left a comment

donaldsharp commented Jan 11, 2018

donaldsharp commented Jan 12, 2018

LabN-CI commented Jan 12, 2018

NetDEF-CI commented Jan 12, 2018

louberger left a comment

donaldsharp commented Jan 16, 2018

donaldsharp commented Jan 19, 2018

pguibert6WIND Jan 19, 2018

donaldsharp Jan 19, 2018

pguibert6WIND Jan 22, 2018 •

edited

Loading

pguibert6WIND Jan 19, 2018

rwestphal Jan 22, 2018

pguibert6WIND Jan 22, 2018

rwestphal Jan 22, 2018

louberger left a comment

louberger commented Jan 19, 2018

pguibert6WIND commented Jan 22, 2018

donaldsharp commented Jan 22, 2018

zebra route-leaking for static routes #1618

zebra route-leaking for static routes #1618

Conversation

donaldsharp commented Jan 10, 2018

LabN-CI commented Jan 10, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Jan 10, 2018

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

21 Static Analyzer issues remaining.

riw777 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

rwestphal commented Jan 11, 2018

donaldsharp commented Jan 11, 2018

NetDEF-CI commented Jan 11, 2018

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

21 Static Analyzer issues remaining.

donaldsharp commented Jan 11, 2018

LabN-CI commented Jan 11, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Jan 11, 2018

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

21 Static Analyzer issues remaining.

rwestphal left a comment

Choose a reason for hiding this comment

donaldsharp commented Jan 11, 2018

donaldsharp commented Jan 12, 2018

LabN-CI commented Jan 12, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Jan 12, 2018

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

21 Static Analyzer issues remaining.

louberger left a comment

Choose a reason for hiding this comment

donaldsharp commented Jan 16, 2018

donaldsharp commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND Jan 22, 2018 • 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

louberger left a comment

Choose a reason for hiding this comment

louberger commented Jan 19, 2018

pguibert6WIND commented Jan 22, 2018

donaldsharp commented Jan 22, 2018

pguibert6WIND Jan 22, 2018 •

edited

Loading