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

Jump to bottom

Fix indexing of routes #30762

Merged
merged 3 commits into from
Feb 14, 2024
Merged

Fix indexing of routes #30762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe35e61
devices: Expand test to check route table
joamaki Feb 9, 2024
07a8eca
datapath/tables: Fix the primary indexer for routes
joamaki Feb 9, 2024
1cc99e8
conformance-e2e: Explicitly set devices to fix e2e tests
dylandreimerink Feb 14, 2024
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
1 change: 1 addition & 0 deletions .github/workflows/conformance-e2e.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ jobs:
kernel: '6.1-20240201.165956'
kube-proxy: 'none'
kpr: 'true'
devices: '{eth0,eth1}'
tunnel: 'vxlan'
lb-mode: 'snat'
egress-gateway: 'true'
Expand Down
93 changes: 75 additions & 18 deletions pkg/datapath/linux/devices_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package linux

import (
"context"
"encoding/json"
"errors"
"net"
"net/netip"
Expand Down Expand Up @@ -84,15 +85,37 @@ func TestDevicesController(t *testing.T) {

logging.SetLogLevelToDebug()

routeExists := func(routes []*tables.Route, linkIndex int, dst, src string) bool {
addrToString := func(addr netip.Addr) string {
if !addr.IsValid() {
return ""
}
return addr.String()
}

routeExists := func(routes []*tables.Route, linkIndex int, dst, src, gw string) bool {
for _, r := range routes {
if r.LinkIndex == linkIndex && r.Dst.String() == dst && r.Src.String() == src {
// undefined IP will stringify as "invalid IP", turn them into "".
actualDst, actualSrc, actualGw := r.Dst.String(), addrToString(r.Src), addrToString(r.Gw)
/*fmt.Printf("%d %s %s %s -- %d %s %s %s\n",
dylandreimerink marked this conversation as resolved.
Show resolved Hide resolved
r.LinkIndex, actualDst, actualSrc, actualGw,
linkIndex, dst, src, gw)*/
if r.LinkIndex == linkIndex && actualDst == dst && actualSrc == src &&
actualGw == gw {
return true
}
}
return false
}

v4Routes := func(routes []*tables.Route) (out []*tables.Route) {
for _, r := range routes {
if r.Dst.Addr().Is4() {
out = append(out, r)
}
}
return
}

orphanRoutes := func(devs []*tables.Device, routes []*tables.Route) bool {
indexes := map[int]bool{}
for _, dev := range devs {
Expand Down Expand Up @@ -126,25 +149,29 @@ func TestDevicesController(t *testing.T) {
devs[0].Name == "dummy0" &&
devs[0].Index > 0 &&
devs[0].Selected &&
routeExists(routes, devs[0].Index, "192.168.0.0/24", "192.168.0.1")
routeExists(routes, devs[0].Index, "192.168.0.0/24", "192.168.0.1", "")
},
},
{
"add dummy1",
func(t *testing.T) {
// Create another dummy to check that the table updates.
require.NoError(t, createDummy("dummy1", "192.168.1.1/24", false))

// Add a default route
assert.NoError(t,
addRoute(addRouteParams{iface: "dummy1", gw: "192.168.1.254", table: unix.RT_TABLE_MAIN}))
},
func(t *testing.T, devs []*tables.Device, routes []*tables.Route) bool {
// Since we're indexing by ifindex, we expect these to be in the order
// they were added.
return len(devs) == 2 &&
"dummy0" == devs[0].Name &&
routeExists(routes, devs[0].Index, "192.168.0.0/24", "192.168.0.1") &&
routeExists(routes, devs[0].Index, "192.168.0.0/24", "192.168.0.1", "") &&
devs[0].Selected &&
"dummy1" == devs[1].Name &&
devs[1].Selected &&
routeExists(routes, devs[1].Index, "192.168.1.0/24", "192.168.1.1")
routeExists(routes, devs[1].Index, "192.168.1.0/24", "192.168.1.1", "")
},
},

Expand Down Expand Up @@ -178,32 +205,62 @@ func TestDevicesController(t *testing.T) {
},

{
"delete-dummy0",
"veth-with-default-gw",
func(t *testing.T) {
require.NoError(t, deleteLink("dummy0"))
// Remove default route from dummy1
assert.NoError(t,
delRoute(addRouteParams{iface: "dummy1", gw: "192.168.1.254", table: unix.RT_TABLE_MAIN}))

// And add one for veth0.
assert.NoError(t,
addRoute(addRouteParams{iface: "veth0", gw: "192.168.4.254", table: unix.RT_TABLE_MAIN}))
},
func(t *testing.T, devs []*tables.Device, routes []*tables.Route) bool {
return len(devs) == 1 &&
"dummy1" == devs[0].Name &&
containsAddress(devs[0], "192.168.1.1", primaryAddress)
return len(devs) == 3 &&
devs[0].Name == "dummy0" &&
devs[1].Name == "dummy1" &&
devs[2].Name == "veth0" &&
containsAddress(devs[2], "192.168.4.1", primaryAddress) &&
routeExists(routes, devs[2].Index, "0.0.0.0/0", "", "192.168.4.254")
},
},

{
"veth-with-default-gw",
"check-all-v4-routes",
func(t *testing.T) {},
func(t *testing.T, devs []*tables.Device, routes []*tables.Route) bool {
routes = v4Routes(routes)
json, _ := json.Marshal(routes)
os.WriteFile("/tmp/routes.json", json, 0644)
return routeExists(routes, devs[0].Index, "192.168.0.0/24", "192.168.0.1", "") &&
routeExists(routes, devs[0].Index, "192.168.0.1/32", "192.168.0.1", "") &&
routeExists(routes, devs[0].Index, "192.168.0.255/32", "192.168.0.1", "") &&

routeExists(routes, devs[1].Index, "192.168.1.0/24", "192.168.1.1", "") &&
routeExists(routes, devs[1].Index, "192.168.1.1/32", "192.168.1.1", "") &&
routeExists(routes, devs[1].Index, "192.168.1.2/32", "192.168.1.1", "") &&
routeExists(routes, devs[1].Index, "192.168.1.255/32", "192.168.1.1", "") &&

routeExists(routes, devs[2].Index, "192.168.4.0/24", "192.168.4.1", "") &&
routeExists(routes, devs[2].Index, "192.168.4.1/32", "192.168.4.1", "") &&
routeExists(routes, devs[2].Index, "192.168.4.255/32", "192.168.4.1", "") &&
routeExists(routes, devs[2].Index, "0.0.0.0/0", "", "192.168.4.254") &&
len(routes) == 11
},
},

{
"delete-dummy0",
func(t *testing.T) {
assert.NoError(t,
addRoute(addRouteParams{iface: "veth0", gw: "192.168.4.254", table: unix.RT_TABLE_MAIN}))
require.NoError(t, deleteLink("dummy0"))
},
func(t *testing.T, devs []*tables.Device, routes []*tables.Route) bool {
return len(devs) == 2 &&
devs[0].Name == "dummy1" &&
devs[0].Selected &&
devs[1].Name == "veth0" &&
containsAddress(devs[1], "192.168.4.1", primaryAddress) &&
devs[1].Selected
"dummy1" == devs[0].Name &&
"veth0" == devs[1].Name
},
},

{
"bond-is-selected",
func(t *testing.T) {
Expand Down
38 changes: 38 additions & 0 deletions pkg/datapath/linux/devices_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,44 @@ func addRoute(p addRouteParams) error {
return nil
}

func delRoute(p addRouteParams) error {
link, err := netlink.LinkByName(p.iface)
if err != nil {
return err
}

var dst *net.IPNet
if p.dst != "" {
_, dst, err = net.ParseCIDR(p.dst)
if err != nil {
return err
}
}

var src net.IP
if p.src != "" {
src = net.ParseIP(p.src)
}

if p.table == 0 {
p.table = unix.RT_TABLE_MAIN
}

route := &netlink.Route{
LinkIndex: link.Attrs().Index,
Dst: dst,
Src: src,
Gw: net.ParseIP(p.gw),
Table: p.table,
Scope: p.scope,
}
if err := netlink.RouteDel(route); err != nil {
return err
}

return nil
}

func delRoutes(iface string) error {
link, err := netlink.LinkByName(iface)
if err != nil {
Expand Down
13 changes: 7 additions & 6 deletions pkg/datapath/tables/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package tables

import (
"encoding/binary"
"fmt"
"net/netip"

Expand Down Expand Up @@ -53,12 +54,12 @@ type RouteID struct {
}

func (id RouteID) Key() index.Key {
key := append(index.Uint64(uint64(id.Table)), '+')
key = append(key, index.Uint64(uint64(id.Table))...)
key = append(key, '+')
key = append(key, index.NetIPPrefix(id.Dst)...)
key = append(key, 0 /* termination */)
return key
key := make([]byte, 0, 4 /* table */ +4 /* link */ +17 /* prefix & bits */)
dylandreimerink marked this conversation as resolved.
Show resolved Hide resolved
key = binary.BigEndian.AppendUint32(key, uint32(id.Table))
key = binary.BigEndian.AppendUint32(key, uint32(id.LinkIndex))
addrBytes := id.Dst.Addr().As16()
dylandreimerink marked this conversation as resolved.
Show resolved Hide resolved
key = append(key, addrBytes[:]...)
return append(key, uint8(id.Dst.Bits()))
}

type Route struct {
Expand Down