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.

Sign up for GitHub

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 multi IP support for SBR #623

Merged
merged 2 commits into from
May 19, 2021
Merged

Add multi IP support for SBR #623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f34c600
[sbr]: Use different tableID for every ipCfg
anuragensemble Apr 27, 2021
f72aa98
[sbr]: Use different tableID for every ipCfg
anuragensemble May 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 62 additions & 41 deletions plugins/meta/sbr/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand All @@ -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())
Expand Down Expand Up @@ -274,38 +285,48 @@ 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++
Copy link
Contributor

Choose a reason for hiding this comment

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

In the other places where a table ID is generated there are a few lines of code to make sure that the table isn't already in use. I think you strictly ought to do that here - i.e. take the little bit of code around firstTableID and turn it into a simple subroutine so it does the same checks in both places.

This is unlikely to go wrong, but if it does it would be horrible to have to figure out.

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
// 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
Expand Down
116 changes: 116 additions & 0 deletions plugins/meta/sbr/sbr_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down