Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add routes propagation for VRF plugin #874

Merged
merged 1 commit into from
Jun 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions plugins/meta/vrf/vrf.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,19 @@ func addInterface(vrf *netlink.Vrf, intf string) error {
if err != nil {
return fmt.Errorf("failed getting ipv6 addresses for %s", intf)
}

// Save all routes that are not local and connected, before setting master,
// because otherwise those routes will be deleted after interface is moved.
filter := &netlink.Route{
LinkIndex: i.Attrs().Index,
Scope: netlink.SCOPE_UNIVERSE, // Exclude local and connected routes
}
filterMask := netlink.RT_FILTER_IIF | netlink.RT_FILTER_SCOPE // Filter based on link index and scope
routes, err := netlink.RouteListFiltered(netlink.FAMILY_ALL, filter, filterMask)
if err != nil {
return fmt.Errorf("failed getting all routes for %s", intf)
}

err = netlink.LinkSetMaster(i, vrf)
if err != nil {
return fmt.Errorf("could not set vrf %s as master of %s: %v", vrf.Name, intf, err)
Expand All @@ -130,6 +143,18 @@ CONTINUE:
}
}

// Apply all saved routes for the interface that was moved to the VRF
for _, route := range routes {
r := route
// Modify original table to vrf one,
r.Table = int(vrf.Table)
// equivalent of 'ip route replace <address> table <int>'.
err = netlink.RouteReplace(&r)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if err != nil {
return fmt.Errorf("could not add route '%s': %v", r, err)
}
}

return nil
}

Expand Down
168 changes: 167 additions & 1 deletion plugins/meta/vrf/vrf_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@ package main
import (
"encoding/json"
"fmt"
"net"
"strings"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/vishvananda/netlink"
"golang.org/x/sys/unix"

"github.com/containernetworking/cni/pkg/skel"
"github.com/containernetworking/cni/pkg/types"
Expand Down Expand Up @@ -107,7 +110,7 @@ var _ = Describe("vrf plugin", func() {
},
})
Expect(err).NotTo(HaveOccurred())
_, err = netlink.LinkByName(IF0Name)
_, err = netlink.LinkByName(IF1Name)
Expect(err).NotTo(HaveOccurred())
return nil
})
Expand Down Expand Up @@ -177,6 +180,102 @@ var _ = Describe("vrf plugin", func() {
Expect(err).NotTo(HaveOccurred())
})

It("adds the interface and custom routing to new VRF", func() {
conf := configWithRouteFor("test", IF0Name, VRF0Name, "10.0.0.2/24", "10.10.10.0/24")

By("Setting custom routing first", func() {
err := targetNS.Do(func(ns.NetNS) error {
defer GinkgoRecover()

ipv4, err := types.ParseCIDR("10.0.0.2/24")
Expect(err).NotTo(HaveOccurred())
Expect(ipv4).NotTo(BeNil())

_, routev4, err := net.ParseCIDR("10.10.10.0/24")
Expect(err).NotTo(HaveOccurred())

ipv6, err := types.ParseCIDR("abcd:1234:ffff::cdde/64")
Expect(err).NotTo(HaveOccurred())
Expect(ipv6).NotTo(BeNil())

_, routev6, err := net.ParseCIDR("1111:dddd::/80")
Expect(err).NotTo(HaveOccurred())
Expect(routev6).NotTo(BeNil())

link, err := netlink.LinkByName(IF0Name)
Expect(err).NotTo(HaveOccurred())

// Add IP addresses for network reachability
netlink.AddrAdd(link, &netlink.Addr{IPNet: ipv4})
netlink.AddrAdd(link, &netlink.Addr{IPNet: ipv6})

ipAddrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
Expect(err).NotTo(HaveOccurred())
// Check if address was assigned properly
Expect(ipAddrs[0].IP.String()).To(Equal("10.0.0.2"))

// Set interface UP, otherwise local route to 10.0.0.0/24 is not present
err = netlink.LinkSetUp(link)
Expect(err).NotTo(HaveOccurred())

// Add additional route to 10.10.10.0/24 via 10.0.0.1 gateway
r := netlink.Route{
LinkIndex: link.Attrs().Index,
Src: ipv4.IP,
Dst: routev4,
Gw: net.ParseIP("10.0.0.1"),
}
err = netlink.RouteAdd(&r)
Expect(err).NotTo(HaveOccurred())

r6 := netlink.Route{
LinkIndex: link.Attrs().Index,
Src: ipv6.IP,
Dst: routev6,
Gw: net.ParseIP("abcd:1234:ffff::1"),
}
err = netlink.RouteAdd(&r6)
Expect(err).NotTo(HaveOccurred())

return nil
})
Expect(err).NotTo(HaveOccurred())
})

args := &skel.CmdArgs{
ContainerID: "dummy",
Netns: targetNS.Path(),
IfName: IF0Name,
StdinData: conf,
}

err := originalNS.Do(func(ns.NetNS) error {
defer GinkgoRecover()
r, _, err := testutils.CmdAddWithArgs(args, func() error {
return cmdAdd(args)
})
Expect(err).NotTo(HaveOccurred())

result, err := current.GetResult(r)
Expect(err).NotTo(HaveOccurred())

Expect(result.Interfaces).To(HaveLen(1))
Expect(result.Interfaces[0].Name).To(Equal(IF0Name))
Expect(result.Routes).To(HaveLen(1))
Expect(result.Routes[0].Dst.IP.String()).To(Equal("10.10.10.0"))
return nil
})
Expect(err).NotTo(HaveOccurred())

err = targetNS.Do(func(ns.NetNS) error {
defer GinkgoRecover()
checkInterfaceOnVRF(VRF0Name, IF0Name)
checkRoutesOnVRF(VRF0Name, IF0Name, "10.10.10.0/24", "1111:dddd::/80")
return nil
})
Expect(err).NotTo(HaveOccurred())
})

It("fails if the interface already has a master set", func() {
conf := configFor("test", IF0Name, VRF0Name, "10.0.0.2/24")

Expand Down Expand Up @@ -690,6 +789,35 @@ func configWithTableFor(name, intf, vrf, ip string, tableID int) []byte {
return []byte(conf)
}

func configWithRouteFor(name, intf, vrf, ip, route string) []byte {
conf := fmt.Sprintf(`{
"name": "%s",
"type": "vrf",
"cniVersion": "0.3.1",
"vrfName": "%s",
"prevResult": {
"interfaces": [
{"name": "%s", "sandbox":"netns"}
],
"ips": [
{
"version": "4",
"address": "%s",
"gateway": "10.0.0.1",
"interface": 0
}
],
"routes": [
{
"dst": "%s",
"gw": "10.0.0.1"
}
]
}
}`, name, vrf, intf, ip, route)
return []byte(conf)
}

func checkInterfaceOnVRF(vrfName, intfName string) {
vrf, err := netlink.LinkByName(vrfName)
Expect(err).NotTo(HaveOccurred())
Expand All @@ -702,3 +830,41 @@ func checkInterfaceOnVRF(vrfName, intfName string) {
Expect(err).NotTo(HaveOccurred())
Expect(master.Attrs().Name).To(Equal(vrfName))
}

func checkRoutesOnVRF(vrfName, intfName string, routesToCheck ...string) {
vrf, err := netlink.LinkByName(vrfName)
Expect(err).NotTo(HaveOccurred())
Expect(vrf).To(BeAssignableToTypeOf(&netlink.Vrf{}))

link, err := netlink.LinkByName(intfName)
Expect(err).NotTo(HaveOccurred())

err = netlink.LinkSetUp(link)
Expect(err).NotTo(HaveOccurred())

ipAddrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
Expect(err).NotTo(HaveOccurred())
Expect(ipAddrs).To(HaveLen(1))
Expect(ipAddrs[0].IP.String()).To(Equal("10.0.0.2"))

// Need to read all tables, so cannot use RouteList
routeFilter := &netlink.Route{
LinkIndex: link.Attrs().Index,
Table: unix.RT_TABLE_UNSPEC,
}

routes, err := netlink.RouteListFiltered(netlink.FAMILY_ALL,
routeFilter,
netlink.RT_FILTER_OIF|netlink.RT_FILTER_TABLE)
Expect(err).NotTo(HaveOccurred())

routesRead := []string{}
for _, route := range routes {
routesRead = append(routesRead, route.String())
Expect(uint32(route.Table)).To(Equal(vrf.(*netlink.Vrf).Table))
}
routesStr := strings.Join(routesRead, "\n")
for _, route := range routesToCheck {
Expect(routesStr).To(ContainSubstring(route))
}
}