Merge pull request #623 from anuragensemble/multi-ip-intf

Add multi IP support for SBR

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.
+// getNextTableID picks the first free table id from a giveen candidate id
-func doRoutes(ipCfgs []*current.IPConfig, origRoutes []*types.Route, iface string) error {
+func getNextTableID(rules []netlink.Rule, routes []netlink.Route, candidateID int) int {
-	// Get a list of rules and routes ready.
+	table := candidateID
-	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
 	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)
@@ -225,6 +230,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 +285,48 @@ func doRoutes(ipCfgs []*current.IPConfig, origRoutes []*types.Route, iface strin
 					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++
+		table = getNextTableID(rules, routes, table)
 	}
 
-	// Move all routes into the correct table. We are taking a shortcut; all
+	// Delete all the interface routes in the default routing table, which were
-	// the routes have been added to the interface anyway but in the wrong
+	// copied to source based routing tables.
-	// table, so instead of removing them we just move them to the table we
+	// Not deleting them while copying to accommodate for multiple ipCfgs from
-	// want them in.
+	// the same subnet. Else, (error for network is unreachable while adding gateway)
-	routes, err = netlink.RouteList(link, netlink.FAMILY_ALL)
-	if err != nil {
-		return fmt.Errorf("Unable to list routes: %v", err)
-	}
-
 	for _, route := range routes {
-		log.Printf("Moving route %s from table %d to %d",
+		log.Printf("Deleting route %s from table %d", route.String(), route.Table)
-			route.String(), route.Table, 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

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 table 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 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 := `{
 	"cniVersion": "0.2.0",