From f34c600ea48b03914fcd4e489493f358cbf3b35b Mon Sep 17 00:00:00 2001 From: Anurag Dwivedi Date: Tue, 27 Apr 2021 12:32:41 +0530 Subject: [PATCH 1/2] [sbr]: Use different tableID for every ipCfg Move default table routes which match the ipCfg config This allows SBR plugin to accommodate for multi-ip interfaces Fixes #581 Signed-off-by: Anurag Dwivedi --- plugins/meta/sbr/main.go | 65 +++++++++------- plugins/meta/sbr/sbr_linux_test.go | 116 +++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 25 deletions(-) diff --git a/plugins/meta/sbr/main.go b/plugins/meta/sbr/main.go index 0dd1c6ad3..affaf835b 100644 --- a/plugins/meta/sbr/main.go +++ b/plugins/meta/sbr/main.go @@ -225,6 +225,12 @@ func doRoutes(ipCfgs []*current.IPConfig, origRoutes []*types.Route, iface strin linkIndex := link.Attrs().Index + // Get all routes for the interface in the default routing table + routes, err = netlink.RouteList(link, netlink.FAMILY_ALL) + if err != nil { + return fmt.Errorf("Unable to list routes: %v", err) + } + // Loop through setting up source based rules and default routes. for _, ipCfg := range ipCfgs { log.Printf("Set rule for source %s", ipCfg.String()) @@ -274,38 +280,47 @@ func doRoutes(ipCfgs []*current.IPConfig, origRoutes []*types.Route, iface strin err) } } - } - // Move all routes into the correct table. We are taking a shortcut; all - // the routes have been added to the interface anyway but in the wrong - // table, so instead of removing them we just move them to the table we - // want them in. - routes, err = netlink.RouteList(link, netlink.FAMILY_ALL) - if err != nil { - return fmt.Errorf("Unable to list routes: %v", err) - } + // Copy the previously added routes for the interface to the correct + // table; all the routes have been added to the interface anyway but + // in the wrong table, so instead of removing them we just move them + // to the table we want them in. + for _, r := range routes { + if ipCfg.Address.Contains(r.Src) || ipCfg.Address.Contains(r.Gw) || + (r.Src == nil && r.Gw == nil) { + // (r.Src == nil && r.Gw == nil) is inferred as a generic route + log.Printf("Copying route %s from table %d to %d", + r.String(), r.Table, table) + + r.Table = table + + // Reset the route flags since if it is dynamically created, + // adding it to the new table will fail with "invalid argument" + r.Flags = 0 + + // We use route replace in case the route already exists, which + // is possible for the default gateway we added above. + err = netlink.RouteReplace(&r) + if err != nil { + return fmt.Errorf("Failed to readd route: %v", err) + } + } + } + // Use a different table for each ipCfg + table++ + } + + // Delete all the interface routes in the default routing table, which were + // copied to source based routing tables. + // Not deleting them while copying to accommodate for multiple ipCfgs from + // the same subnet. Else, (error for network is unreachable while adding gateway) for _, route := range routes { - log.Printf("Moving route %s from table %d to %d", - route.String(), route.Table, table) - + log.Printf("Deleting route %s from table %d", route.String(), route.Table) err := netlink.RouteDel(&route) if err != nil { return fmt.Errorf("Failed to delete route: %v", err) } - - route.Table = table - - // Reset the route flags since if it is dynamically created, - // adding it to the new table will fail with "invalid argument" - route.Flags = 0 - - // We use route replace in case the route already exists, which - // is possible for the default gateway we added above. - err = netlink.RouteReplace(&route) - if err != nil { - return fmt.Errorf("Failed to readd route: %v", err) - } } return nil diff --git a/plugins/meta/sbr/sbr_linux_test.go b/plugins/meta/sbr/sbr_linux_test.go index d0efecd4b..967bcbb12 100644 --- a/plugins/meta/sbr/sbr_linux_test.go +++ b/plugins/meta/sbr/sbr_linux_test.go @@ -401,6 +401,122 @@ var _ = Describe("sbr test", func() { Expect(equalRoutes(expNet1.Routes, devNet1.Routes)).To(BeTrue()) }) + It("Works with multiple IPs", func() { + ifname := "net1" + conf := `{ + "cniVersion": "0.3.0", + "name": "cni-plugin-sbr-test", + "type": "sbr", + "prevResult": { + "cniVersion": "0.3.0", + "interfaces": [ + { + "name": "%s", + "sandbox": "%s" + } + ], + "ips": [ + { + "version": "4", + "address": "192.168.1.209/24", + "gateway": "192.168.1.1", + "interface": 0 + }, + { + "version": "4", + "address": "192.168.101.209/24", + "gateway": "192.168.101.1", + "interface": 1 + } + ], + "routes": [] + } +}` + conf = fmt.Sprintf(conf, ifname, targetNs.Path()) + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: targetNs.Path(), + IfName: ifname, + StdinData: []byte(conf), + } + + preStatus := createDefaultStatus() + + // Add the second IP on net1 interface + preStatus.Devices[1].Addrs = append(preStatus.Devices[1].Addrs, + net.IPNet{ + IP: net.IPv4(192, 168, 101, 209), + Mask: net.IPv4Mask(255, 255, 255, 0), + }) + + err := setup(targetNs, preStatus) + Expect(err).NotTo(HaveOccurred()) + + oldStatus, err := readback(targetNs, []string{"net1", "eth0"}) + Expect(err).NotTo(HaveOccurred()) + + _, _, err = testutils.CmdAddWithArgs(args, func() error { return cmdAdd(args) }) + Expect(err).NotTo(HaveOccurred()) + + newStatus, err := readback(targetNs, []string{"net1", "eth0"}) + Expect(err).NotTo(HaveOccurred()) + + // Check results. We expect all the routes on net1 to have moved to + // table 100 except for local routes (table 255); a new default gateway + // route to have been created; and 2 rules to exist. + expNet1 := oldStatus.Devices[0] + expEth0 := oldStatus.Devices[1] + + for i := range expNet1.Routes { + if expNet1.Routes[i].Table != 255 { + if expNet1.Routes[i].Src.String() == "192.168.101.209" { + // All 192.168.101.x routes expected in table 101 + expNet1.Routes[i].Table = 101 + } else if (expNet1.Routes[i].Src == nil && expNet1.Routes[i].Gw == nil) { + // Generic Link Local Addresses assigned. They should exist in all + // route tables + expNet1.Routes[i].Table = 100 + expNet1.Routes = append(expNet1.Routes, + netlink.Route{ + Dst: expNet1.Routes[i].Dst, + Table: 101, + LinkIndex: expNet1.Routes[i].LinkIndex}) + } else { + // All 192.168.1.x routes expected in tabele 100 + expNet1.Routes[i].Table = 100 + } + } + } + expNet1.Routes = append(expNet1.Routes, + netlink.Route{ + Gw: net.IPv4(192, 168, 1, 1), + Table: 100, + LinkIndex: expNet1.Routes[0].LinkIndex}) + + expNet1.Routes = append(expNet1.Routes, + netlink.Route{ + Gw: net.IPv4(192, 168, 101, 1), + Table: 101, + LinkIndex: expNet1.Routes[0].LinkIndex}) + + // 2 Rules will ve created for each IP address. (100, 101) + Expect(len(newStatus.Rules)).To(Equal(2)) + + // First entry corresponds to last table + Expect(newStatus.Rules[0].Table).To(Equal(101)) + Expect(newStatus.Rules[0].Src.String()).To(Equal("192.168.101.209/32")) + + // Second entry corresponds to first table (100) + Expect(newStatus.Rules[1].Table).To(Equal(100)) + Expect(newStatus.Rules[1].Src.String()).To(Equal("192.168.1.209/32")) + + devNet1 := newStatus.Devices[0] + devEth0 := newStatus.Devices[1] + Expect(equalRoutes(expNet1.Routes, devNet1.Routes)).To(BeTrue()) + Expect(equalRoutes(expEth0.Routes, devEth0.Routes)).To(BeTrue()) + + }) + It("fails with CNI spec versions that don't support plugin chaining", func() { conf := `{ "cniVersion": "0.2.0", From f72aa98629f260a8d0335473004edacabf2fbfe0 Mon Sep 17 00:00:00 2001 From: Anurag Dwivedi Date: Sat, 15 May 2021 19:32:31 +0530 Subject: [PATCH 2/2] [sbr]: Use different tableID for every ipCfg Check tableID not in use for every ipCfg This allows SBR plugin to accommodate for multi-ip interfaces Fixes #581 Signed-off-by: Anurag Dwivedi --- plugins/meta/sbr/main.go | 48 +++++++++++++++++------------- plugins/meta/sbr/sbr_linux_test.go | 40 ++++++++++++------------- 2 files changed, 47 insertions(+), 41 deletions(-) diff --git a/plugins/meta/sbr/main.go b/plugins/meta/sbr/main.go index affaf835b..7929952a3 100644 --- a/plugins/meta/sbr/main.go +++ b/plugins/meta/sbr/main.go @@ -176,23 +176,9 @@ func cmdAdd(args *skel.CmdArgs) error { return types.PrintResult(conf.PrevResult, conf.CNIVersion) } -// doRoutes does all the work to set up routes and rules during an add. -func doRoutes(ipCfgs []*current.IPConfig, origRoutes []*types.Route, iface string) error { - // Get a list of rules and routes ready. - rules, err := netlink.RuleList(netlink.FAMILY_ALL) - if err != nil { - return fmt.Errorf("Failed to list all rules: %v", err) - } - - routes, err := netlink.RouteList(nil, netlink.FAMILY_ALL) - if err != nil { - return fmt.Errorf("Failed to list all routes: %v", err) - } - - // Pick a table ID to use. We pick the first table ID from firstTableID - // on that has no existing rules mapping to it and no existing routes in - // it. - table := firstTableID +// getNextTableID picks the first free table id from a giveen candidate id +func getNextTableID(rules []netlink.Rule, routes []netlink.Route, candidateID int) int { + table := candidateID for { foundExisting := false for _, rule := range rules { @@ -215,7 +201,26 @@ func doRoutes(ipCfgs []*current.IPConfig, origRoutes []*types.Route, iface strin break } } + return table +} + +// doRoutes does all the work to set up routes and rules during an add. +func doRoutes(ipCfgs []*current.IPConfig, origRoutes []*types.Route, iface string) error { + // Get a list of rules and routes ready. + rules, err := netlink.RuleList(netlink.FAMILY_ALL) + if err != nil { + return fmt.Errorf("Failed to list all rules: %v", err) + } + + routes, err := netlink.RouteList(nil, netlink.FAMILY_ALL) + if err != nil { + return fmt.Errorf("Failed to list all routes: %v", err) + } + // Pick a table ID to use. We pick the first table ID from firstTableID + // on that has no existing rules mapping to it and no existing routes in + // it. + table := getNextTableID(rules, routes, firstTableID) log.Printf("First unreferenced table: %d", table) link, err := netlink.LinkByName(iface) @@ -287,17 +292,17 @@ func doRoutes(ipCfgs []*current.IPConfig, origRoutes []*types.Route, iface strin // to the table we want them in. for _, r := range routes { if ipCfg.Address.Contains(r.Src) || ipCfg.Address.Contains(r.Gw) || - (r.Src == nil && r.Gw == nil) { + (r.Src == nil && r.Gw == nil) { // (r.Src == nil && r.Gw == nil) is inferred as a generic route log.Printf("Copying route %s from table %d to %d", r.String(), r.Table, table) r.Table = table - + // Reset the route flags since if it is dynamically created, // adding it to the new table will fail with "invalid argument" r.Flags = 0 - + // We use route replace in case the route already exists, which // is possible for the default gateway we added above. err = netlink.RouteReplace(&r) @@ -309,8 +314,9 @@ func doRoutes(ipCfgs []*current.IPConfig, origRoutes []*types.Route, iface strin // Use a different table for each ipCfg table++ + table = getNextTableID(rules, routes, table) } - + // Delete all the interface routes in the default routing table, which were // copied to source based routing tables. // Not deleting them while copying to accommodate for multiple ipCfgs from diff --git a/plugins/meta/sbr/sbr_linux_test.go b/plugins/meta/sbr/sbr_linux_test.go index 967bcbb12..3327609f1 100644 --- a/plugins/meta/sbr/sbr_linux_test.go +++ b/plugins/meta/sbr/sbr_linux_test.go @@ -439,50 +439,50 @@ var _ = Describe("sbr test", func() { IfName: ifname, StdinData: []byte(conf), } - + preStatus := createDefaultStatus() - + // Add the second IP on net1 interface preStatus.Devices[1].Addrs = append(preStatus.Devices[1].Addrs, net.IPNet{ - IP: net.IPv4(192, 168, 101, 209), + IP: net.IPv4(192, 168, 101, 209), Mask: net.IPv4Mask(255, 255, 255, 0), }) - + err := setup(targetNs, preStatus) Expect(err).NotTo(HaveOccurred()) - + oldStatus, err := readback(targetNs, []string{"net1", "eth0"}) Expect(err).NotTo(HaveOccurred()) - + _, _, err = testutils.CmdAddWithArgs(args, func() error { return cmdAdd(args) }) Expect(err).NotTo(HaveOccurred()) - + newStatus, err := readback(targetNs, []string{"net1", "eth0"}) Expect(err).NotTo(HaveOccurred()) - + // Check results. We expect all the routes on net1 to have moved to // table 100 except for local routes (table 255); a new default gateway // route to have been created; and 2 rules to exist. expNet1 := oldStatus.Devices[0] expEth0 := oldStatus.Devices[1] - + for i := range expNet1.Routes { if expNet1.Routes[i].Table != 255 { if expNet1.Routes[i].Src.String() == "192.168.101.209" { // All 192.168.101.x routes expected in table 101 expNet1.Routes[i].Table = 101 - } else if (expNet1.Routes[i].Src == nil && expNet1.Routes[i].Gw == nil) { + } else if expNet1.Routes[i].Src == nil && expNet1.Routes[i].Gw == nil { // Generic Link Local Addresses assigned. They should exist in all // route tables expNet1.Routes[i].Table = 100 expNet1.Routes = append(expNet1.Routes, netlink.Route{ - Dst: expNet1.Routes[i].Dst, + Dst: expNet1.Routes[i].Dst, Table: 101, LinkIndex: expNet1.Routes[i].LinkIndex}) } else { - // All 192.168.1.x routes expected in tabele 100 + // All 192.168.1.x routes expected in table 100 expNet1.Routes[i].Table = 100 } } @@ -492,30 +492,30 @@ var _ = Describe("sbr test", func() { Gw: net.IPv4(192, 168, 1, 1), Table: 100, LinkIndex: expNet1.Routes[0].LinkIndex}) - + expNet1.Routes = append(expNet1.Routes, netlink.Route{ Gw: net.IPv4(192, 168, 101, 1), Table: 101, LinkIndex: expNet1.Routes[0].LinkIndex}) - - // 2 Rules will ve created for each IP address. (100, 101) + + // 2 Rules will be created for each IP address. (100, 101) Expect(len(newStatus.Rules)).To(Equal(2)) - + // First entry corresponds to last table Expect(newStatus.Rules[0].Table).To(Equal(101)) Expect(newStatus.Rules[0].Src.String()).To(Equal("192.168.101.209/32")) - + // Second entry corresponds to first table (100) Expect(newStatus.Rules[1].Table).To(Equal(100)) Expect(newStatus.Rules[1].Src.String()).To(Equal("192.168.1.209/32")) - + devNet1 := newStatus.Devices[0] devEth0 := newStatus.Devices[1] Expect(equalRoutes(expNet1.Routes, devNet1.Routes)).To(BeTrue()) Expect(equalRoutes(expEth0.Routes, devEth0.Routes)).To(BeTrue()) - - }) + + }) It("fails with CNI spec versions that don't support plugin chaining", func() { conf := `{