Add routes propagation for VRF plugin #874

korroot · 2023-03-31T10:16:38Z

Up until now, if previous plugin assigned routes to interface, movement of this interface to new VRF cause routes to be deleted.

This patch adds funtionality to VRF plugin to save the routes before interface is assgined to VRF, and then re-apply all saved routes to new VRF.

Closes: #866

squeed

Really nice!
Can you add a test case that includes both address families?

korroot · 2023-04-12T16:25:41Z

Added IPv6 routing test. All works fine.

squeed · 2023-04-17T15:52:55Z

I talked to @mlguerrero12 , who observed that this is a behavior change, and might break existing setups. He's going to run some internal tests on it.

If so, we may need to put this behind a configuration variable, something like preserveRoutes.

Separately, can you fix the lint / format issues?

korroot · 2023-04-24T14:20:57Z

@squeed Lint error fixed.

In terms of behavior change - does it mean that deleting the routing in some cases is desirable?
I am not sure how exactly this configuration variable to enable/disable routes propagation should be implemented?

mlguerrero12 · 2023-05-01T14:47:58Z

plugins/meta/vrf/vrf.go

+	// because otherwise the routes will be deleted after interface is moved.
+	routes, err := netlink.RouteList(i, netlink.FAMILY_ALL)
+	if err != nil {
+		return fmt.Errorf("failed getting ipv4 routes for %s", intf)


not only ipv4

mlguerrero12 · 2023-05-01T14:48:28Z

plugins/meta/vrf/vrf.go

+		// modify original table to vrf one
+		// equivalent of 'ip route add <address> table <int>'
+		route.Table = int(vrf.Table)
+		netlink.RouteAdd(&route)


unhandled error

Yes, I noticed it before, but it is not handled by purpose - the error can be ignored, because if adding a route returns error - it can mean that is already there.

in that case, to be consistent with the logic of ipv6 addresses which is above this block, only add the routes that do not exist

What do we gain by this? Replacing one-liner with 20-30 lines of code?

Then check if the error is because the route is already there. You can't assume that the error will always be due to that. A user might complain that the routes are not being added and we won't have logs to troubleshoot.

Ok, I have added a check, if route exists, and if not, return error during adding a route.

mlguerrero12 · 2023-05-01T15:23:34Z

plugins/meta/vrf/vrf.go

+		// modify original table to vrf one
+		// equivalent of 'ip route add <address> table <int>'
+		route.Table = int(vrf.Table)
+		netlink.RouteAdd(&route)


to be on the safe side, create a local copy of route inside the for loop

mlguerrero12 · 2023-05-19T11:12:13Z

plugins/meta/vrf/vrf.go

+		err = netlink.RouteAdd(&r)
+		if err != nil {
+			// If route is already present, returned error is "file exists"
+			if !strings.Contains(fmt.Sprintf("%v", err), "file exists") {


does err.Error() not give you the string?

mlguerrero12 · 2023-05-19T11:13:56Z

plugins/meta/vrf/vrf.go

+		if err != nil {
+			// If route is already present, returned error is "file exists"
+			if !strings.Contains(fmt.Sprintf("%v", err), "file exists") {
+				return fmt.Errorf("error while adding route \"%s\": %v\n", r, err)


no need to add \n and perhaps something like "failed adding ..." or "could not add ..." would be more consistent with other errors in this file

mlguerrero12 · 2023-05-31T14:46:56Z

plugins/meta/vrf/vrf.go

+		// Modify original table to vrf one,
+		// equivalent of 'ip route add <address> table <int>'.
+		r.Table = int(vrf.Table)
+		err = netlink.RouteAdd(&r)


you can use netlink.RouteReplace instead (ip route replace)

it won't fail if the route exists

Hmm what do we get by using RouteReplace? I think if the route is there, we maybe not suppose to replace it...

What I want to avoid is checking the string of an error to make a decision or even to have an error. I believe we have the means to do it.

When an interface is enslaved, only local and connected routes are moved to the table associated with the vrf. So, instead of fetching all routes (with RouteList), you can use netlink.RouteListFiltered to get all routes of the device that are not connected or local. Something like:

filter := &netlink.Route{ LinkIndex: link.Attrs().Index, Scope: netlink.SCOPE_UNIVERSE, // Exclude local and connected routes } filterMask := netlink.RT_FILTER_LINK | netlink.RT_FILTER_SCOPE // Filter based on link index and scope routes, err := netlink.RouteListFiltered(netlink.FAMILY_ALL, filter, filterMask)

and then, add those routes. if we still get an error, then I think we should just return it. The conflict should be investigated and solved by users. Not the task of the plugin to decide what to do (ignore or replace).

Make sense, thanks for the explanation. I will address it shortly.

Up until now, if previous plugin assigned routes to interface, movement of this interface to new VRF cause routes to be deleted. This patch adds funtionality to VRF plugin to save the routes before interface is assgined to VRF, and then re-apply all saved routes to new VRF. Signed-off-by: Artur Korzeniewski <artur.korzeniewski@travelping.com>

korroot · 2023-06-02T12:25:29Z

@mlguerrero12 your latest comment is addressed, thanks for improving this PR :)
Do you see something more to adjust, or are we closer to merge :D ?

mlguerrero12 · 2023-06-02T15:03:08Z

plugins/meta/vrf/vrf.go

+		// Modify original table to vrf one,
+		r.Table = int(vrf.Table)
+		// equivalent of 'ip route replace <address> table <int>'.
+		err = netlink.RouteReplace(&r)


I think you can use add here now since you are making sure to copy only routes that are not supposed to exist. In case there is a conflict (error), users should be aware of it and fix it.

I tried to use the RouteAdd here but it seems that one of IPv6 route is copied to saved routes:

[FAILED] Unexpected error: <*errors.errorString | 0xc0001b56a0>: { s: "cmdAdd failed: could not add route '{Ifindex: 2 Dst: abcd:1234:ffff::/64 Src: <nil> Gw: <nil> Flags: [] Table: 1 Realm: 0}': file exists", } cmdAdd failed: could not add route '{Ifindex: 2 Dst: abcd:1234:ffff::/64 Src: <nil> Gw: <nil> Flags: [] Table: 1 Realm: 0}': file exists occurred

From the vrf_test.go: https://github.com/containernetworking/plugins/pull/874/files#diff-d8b21f1b7cbf938511615ea01f7b56215325cb65d64b45b5cd089e01c210bf6eR197

So instead I have used the RouteReplace

Thank you @korroot.

Yes, it seems that the concept of SCOPE_UNIVERSE is different in IPv6 (read about it and ran some tests). There will be conflicts after the ipv6 addresses are added with the block above (or with the sysctl option).

Anyway, I'm fine with using Replace to overcome this. But I think it might be cleaner to check that the Gw is different than nil, and add only those routes (with regular Add). Either option is fine.

@squeed, could you please approve the workflow of this PR?

korroot · 2023-06-12T09:41:47Z

@squeed can you re-review this PR? Also, can somebody re-run the CI? it broke on timeout, not code issues.

korroot · 2023-06-16T11:37:50Z

@squeed thanks for running the CI, can you approve this PR? Or comment if there are any more improvements needed?

korroot force-pushed the vrf-add-routes branch from e8793c4 to 0da23be Compare March 31, 2023 12:57

squeed requested changes Apr 5, 2023

View reviewed changes

korroot force-pushed the vrf-add-routes branch from 0da23be to 4b9b173 Compare April 12, 2023 14:35

korroot force-pushed the vrf-add-routes branch from 4b9b173 to 32e1a44 Compare April 24, 2023 14:04

korroot force-pushed the vrf-add-routes branch 2 times, most recently from 9f04aab to b654c52 Compare April 24, 2023 19:54

mlguerrero12 reviewed May 1, 2023

View reviewed changes

korroot force-pushed the vrf-add-routes branch 2 times, most recently from 82ebd6d to a3d905a Compare May 15, 2023 13:53

mlguerrero12 reviewed May 19, 2023

View reviewed changes

korroot force-pushed the vrf-add-routes branch from a3d905a to 5bdca18 Compare May 19, 2023 15:59

mlguerrero12 reviewed May 31, 2023

View reviewed changes

korroot force-pushed the vrf-add-routes branch from 5bdca18 to ca12d49 Compare June 2, 2023 12:22

mlguerrero12 reviewed Jun 2, 2023

View reviewed changes

mlguerrero12 approved these changes Jun 5, 2023

View reviewed changes

squeed approved these changes Jun 27, 2023

View reviewed changes

squeed merged commit d03b84d into containernetworking:main Jun 27, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add routes propagation for VRF plugin #874

Add routes propagation for VRF plugin #874

korroot commented Mar 31, 2023

squeed left a comment

korroot commented Apr 12, 2023

squeed commented Apr 17, 2023

korroot commented Apr 24, 2023

mlguerrero12 May 1, 2023

mlguerrero12 May 1, 2023

korroot May 9, 2023

mlguerrero12 May 9, 2023

korroot May 11, 2023

mlguerrero12 May 15, 2023

korroot May 15, 2023

mlguerrero12 May 1, 2023

mlguerrero12 May 19, 2023

mlguerrero12 May 19, 2023

mlguerrero12 May 31, 2023 •

edited

korroot Jun 1, 2023

mlguerrero12 Jun 1, 2023

korroot Jun 2, 2023

korroot commented Jun 2, 2023

mlguerrero12 Jun 2, 2023

korroot Jun 2, 2023

mlguerrero12 Jun 5, 2023

korroot commented Jun 12, 2023

korroot commented Jun 16, 2023

Add routes propagation for VRF plugin #874

Add routes propagation for VRF plugin #874

Conversation

korroot commented Mar 31, 2023

squeed left a comment

Choose a reason for hiding this comment

korroot commented Apr 12, 2023

squeed commented Apr 17, 2023

korroot commented Apr 24, 2023

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

mlguerrero12 May 31, 2023 • edited

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

korroot commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korroot commented Jun 12, 2023

korroot commented Jun 16, 2023

mlguerrero12 May 31, 2023 •

edited