Skip to content

Commit

Permalink
Merge pull request #1738 from kyrtapz/invalix_next_hops_413
Browse files Browse the repository at this point in the history 
[release-4.13] OCPBUGS-15496: Fix default GW IPs retrieval
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed Aug 3, 2023
2 parents 2be24c3 + b332d91 commit b1774d7
Show file tree
Hide file tree
Showing 3 changed files with 293 additions and 13 deletions.
2 changes: 1 addition & 1 deletion go-controller/pkg/node/gateway_init.go
View file
Expand Up @@ -139,7 +139,7 @@ func getGatewayNextHops() ([]net.IP, string, error) {
}
gatewayIntf := config.Gateway.Interface
if needIPv4NextHop || needIPv6NextHop || gatewayIntf == "" {
defaultGatewayIntf, defaultGatewayNextHops, err := getDefaultGatewayInterfaceDetails(gatewayIntf)
defaultGatewayIntf, defaultGatewayNextHops, err := getDefaultGatewayInterfaceDetails(gatewayIntf, config.IPv4Mode, config.IPv6Mode)
if err != nil {
return nil, "", err
}
Expand Down
33 changes: 21 additions & 12 deletions go-controller/pkg/node/helper_linux.go
View file
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"net"

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
"github.com/pkg/errors"
"github.com/vishvananda/netlink"
Expand All @@ -18,31 +17,41 @@ import (
// which the default gateway (for route to 0.0.0.0) is configured.
// optionally pass the pre-determined gateway interface
// It also returns the default gateways themselves.
func getDefaultGatewayInterfaceDetails(gwIface string) (string, []net.IP, error) {
func getDefaultGatewayInterfaceDetails(gwIface string, ipV4Mode, ipV6Mode bool) (string, []net.IP, error) {
var intfName string
var gatewayIPs []net.IP

if config.IPv4Mode {
if ipV4Mode {
intfIPv4Name, gw, err := getDefaultGatewayInterfaceByFamily(netlink.FAMILY_V4, gwIface)
if err != nil {
return "", gatewayIPs, err
}
intfName = intfIPv4Name
gatewayIPs = append(gatewayIPs, gw)

// only add the GW IP if it is specified
if len(gw) != 0 {
gatewayIPs = append(gatewayIPs, gw)
}
}

if config.IPv6Mode {
if ipV6Mode {
intfIPv6Name, gw, err := getDefaultGatewayInterfaceByFamily(netlink.FAMILY_V6, gwIface)
if err != nil {
return "", gatewayIPs, err
}
// validate that both IP Families use the same interface for the gateway

// if there is an interface specified for both IP families
// validate they use the same one
if intfName == "" {
intfName = intfIPv6Name
} else if intfName != intfIPv6Name {
} else if (len(intfName) > 0 && len(intfIPv6Name) > 0) && intfName != intfIPv6Name {
return "", nil, fmt.Errorf("multiple gateway interfaces detected: %s %s", intfName, intfIPv6Name)
}
gatewayIPs = append(gatewayIPs, gw)

// only add the GW IP if it is specified
if len(gw) != 0 {
gatewayIPs = append(gatewayIPs, gw)
}
}

return intfName, gatewayIPs, nil
Expand All @@ -58,7 +67,7 @@ func getDefaultGatewayInterfaceByFamily(family int, gwIface string) (string, net
gwIfIdx := 0
// gw interface provided
if len(gwIface) > 0 {
link, err := netlink.LinkByName(gwIface)
link, err := util.GetNetLinkOps().LinkByName(gwIface)
if err != nil {
return "", nil, fmt.Errorf("error looking up gw interface: %q, error: %w", gwIface, err)
}
Expand All @@ -69,7 +78,7 @@ func getDefaultGatewayInterfaceByFamily(family int, gwIface string) (string, net
klog.Infof("Provided gateway interface %q, found as index: %d", gwIface, gwIfIdx)
}

routeList, err := netlink.RouteListFiltered(family, filter, mask)
routeList, err := util.GetNetLinkOps().RouteListFiltered(family, filter, mask)
if err != nil {
return "", nil, errors.Wrapf(err, "failed to get routing table in node")
}
Expand All @@ -78,7 +87,7 @@ func getDefaultGatewayInterfaceByFamily(family int, gwIface string) (string, net
for _, r := range routes {
// no multipath
if len(r.MultiPath) == 0 {
intfLink, err := netlink.LinkByIndex(r.LinkIndex)
intfLink, err := util.GetNetLinkOps().LinkByIndex(r.LinkIndex)
if err != nil {
klog.Warningf("Failed to get interface link for route %v : %v", r, err)
continue
Expand All @@ -104,7 +113,7 @@ func getDefaultGatewayInterfaceByFamily(family int, gwIface string) (string, net
// TODO: revisit for full multipath support
// xref: https://github.com/vishvananda/netlink/blob/6ffafa9fc19b848776f4fd608c4ad09509aaacb4/route.go#L137-L145
for _, nh := range r.MultiPath {
intfLink, err := netlink.LinkByIndex(nh.LinkIndex)
intfLink, err := util.GetNetLinkOps().LinkByIndex(nh.LinkIndex)
if err != nil {
klog.Warningf("Failed to get interface link for route %v : %v", nh, err)
continue
Expand Down
271 changes: 271 additions & 0 deletions go-controller/pkg/node/helper_linux_test.go
View file
@@ -1,9 +1,16 @@
package node

import (
"fmt"
"net"
"reflect"
"testing"

ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing"
netlink_mocks "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/mocks/github.com/vishvananda/netlink"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
util_mocks "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util/mocks"
"github.com/stretchr/testify/assert"
"github.com/vishvananda/netlink"
)

Expand Down Expand Up @@ -174,3 +181,267 @@ func TestFilterRoutesByIfIndex(t *testing.T) {
}
}
}

func TestGetDefaultGatewayInterfaceByFamily(t *testing.T) {
mockNetLinkOps := new(util_mocks.NetLinkOps)
mockLink := new(netlink_mocks.Link)
// below sets the `netLinkOps` in util/net_linux.go to a mock instance for purpose of unit tests execution
util.SetNetLinkOpMockInst(mockNetLinkOps)
defer util.ResetNetLinkOpMockInst()

defaultIf := "testInterface"
customIf := "customTestInterface"
defaultGWIP := ovntest.MustParseIP("1.1.1.1")
customGWIP := ovntest.MustParseIP("fd99::1")

tests := []struct {
desc string
ipFamily int
gwIface string
expIntfName string
expGatewayIP net.IP
expErr bool
netLinkOpsMockHelper []ovntest.TestifyMockHelper
linkMockHelper []ovntest.TestifyMockHelper
}{
{
desc: "no default routes returns empty values",
ipFamily: netlink.FAMILY_V4,
expGatewayIP: net.IP{},
netLinkOpsMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{}, nil}},
},
},
{
desc: "first default route is used when no gw is specified",
gwIface: "",
ipFamily: netlink.FAMILY_V4,
expIntfName: defaultIf,
expGatewayIP: defaultGWIP,
netLinkOpsMockHelper: []ovntest.TestifyMockHelper{

{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{
{
LinkIndex: 2,
Gw: defaultGWIP,
},
{
LinkIndex: 9,
Gw: ovntest.MustParseIP("3.3.3.3"),
},
}, nil}},
{OnCallMethodName: "LinkByIndex", OnCallMethodArgType: []string{"int"}, RetArgList: []interface{}{mockLink, nil}},
},
linkMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
},
},
{
desc: "only routes from the provided GW are considered",
gwIface: customIf,
ipFamily: netlink.FAMILY_V6,
expIntfName: customIf,
expGatewayIP: customGWIP,
netLinkOpsMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "LinkByName", OnCallMethodArgType: []string{"string", "string"}, RetArgList: []interface{}{mockLink, nil}},
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{
{
LinkIndex: 1,
Gw: defaultGWIP,
},
{
LinkIndex: 2,
Gw: customGWIP,
},
}, nil}},
{OnCallMethodName: "LinkByIndex", OnCallMethodArgType: []string{"int"}, RetArgList: []interface{}{mockLink, nil}},
},
linkMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Index: 2}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Index: 2}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: customIf}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: customIf}}},
},
},
}
for i, tc := range tests {
t.Run(fmt.Sprintf("%d:%s", i, tc.desc), func(t *testing.T) {
ovntest.ProcessMockFnList(&mockNetLinkOps.Mock, tc.netLinkOpsMockHelper)
ovntest.ProcessMockFnList(&mockLink.Mock, tc.linkMockHelper)
intfName, gwIP, err := getDefaultGatewayInterfaceByFamily(tc.ipFamily, tc.gwIface)
if intfName != tc.expIntfName {
t.Fatalf("TestGetDefaultGatewayInterfaceByFamily(%d): Default gateway interface should be '%v' but got '%v'",
i, tc.expIntfName, intfName)
}
if !reflect.DeepEqual(tc.expGatewayIP, gwIP) {
t.Fatalf("TestGetDefaultGatewayInterfaceByFamily(%d): Default gateway IP should be '%v' but got '%v'",
i, tc.expGatewayIP, gwIP)
}

t.Log(err)
if tc.expErr {
assert.Error(t, err)
} else {
assert.Nil(t, err)
}
mockNetLinkOps.AssertExpectations(t)
mockLink.AssertExpectations(t)
})
}
}

func TestGetDefaultGatewayInterfaceDetails(t *testing.T) {
mockNetLinkOps := new(util_mocks.NetLinkOps)
mockLink := new(netlink_mocks.Link)
// below sets the `netLinkOps` in util/net_linux.go to a mock instance for purpose of unit tests execution
util.SetNetLinkOpMockInst(mockNetLinkOps)
defer util.ResetNetLinkOpMockInst()

defaultIf := "testInterface"
defaultGWIPv4 := ovntest.MustParseIP("1.1.1.1")
defaultGWIPv6 := ovntest.MustParseIP("fd99::1")

tests := []struct {
desc string
ipV4Mode bool
ipV6Mode bool
gwIface string
expIntfName string
expGatewayIPs []net.IP
expErr bool
netLinkOpsMockHelper []ovntest.TestifyMockHelper
linkMockHelper []ovntest.TestifyMockHelper
}{
{
desc: "no default routes returns empty values",
ipV4Mode: true,
netLinkOpsMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{}, nil}},
},
},
{
desc: "only ipv4 GW set in dual-stack returns valid interface and one gw",
ipV4Mode: true,
ipV6Mode: true,
expGatewayIPs: []net.IP{defaultGWIPv4},
expIntfName: defaultIf,
netLinkOpsMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{
{
LinkIndex: 1,
Gw: defaultGWIPv4,
},
}, nil}},
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{}, nil}},
{OnCallMethodName: "LinkByIndex", OnCallMethodArgType: []string{"int"}, RetArgList: []interface{}{mockLink, nil}},
},
linkMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
},
},
{
desc: "only ipv6 GW set in dual-stack returns valid interface and one gw",
ipV4Mode: true,
ipV6Mode: true,
expGatewayIPs: []net.IP{defaultGWIPv6},
expIntfName: defaultIf,
netLinkOpsMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{}, nil}},
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{
{
LinkIndex: 1,
Gw: defaultGWIPv6,
},
}, nil}},
{OnCallMethodName: "LinkByIndex", OnCallMethodArgType: []string{"int"}, RetArgList: []interface{}{mockLink, nil}},
},
linkMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
},
},
{
desc: "in dual-stack the function fails if the default GWs are on different interfaces",
ipV4Mode: true,
ipV6Mode: true,
expErr: true,
netLinkOpsMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{
{
LinkIndex: 1,
Gw: defaultGWIPv4,
},
}, nil}},
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{
{
LinkIndex: 2,
Gw: defaultGWIPv6,
},
}, nil}},
{OnCallMethodName: "LinkByIndex", OnCallMethodArgType: []string{"int"}, RetArgList: []interface{}{mockLink, nil}},
{OnCallMethodName: "LinkByIndex", OnCallMethodArgType: []string{"int"}, RetArgList: []interface{}{mockLink, nil}},
},
linkMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: "invalidInterface"}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: "invalidInterface"}}},
},
},
{
desc: "in dual-stack the function returns both GW ips",
ipV4Mode: true,
ipV6Mode: true,
expGatewayIPs: []net.IP{defaultGWIPv4, defaultGWIPv6},
expIntfName: defaultIf,
netLinkOpsMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{
{
LinkIndex: 1,
Gw: defaultGWIPv4,
},
}, nil}},
{OnCallMethodName: "RouteListFiltered", OnCallMethodArgType: []string{"int", "*netlink.Route", "uint64"}, RetArgList: []interface{}{[]netlink.Route{
{
LinkIndex: 1,
Gw: defaultGWIPv6,
},
}, nil}},
{OnCallMethodName: "LinkByIndex", OnCallMethodArgType: []string{"int"}, RetArgList: []interface{}{mockLink, nil}},
{OnCallMethodName: "LinkByIndex", OnCallMethodArgType: []string{"int"}, RetArgList: []interface{}{mockLink, nil}},
},
linkMockHelper: []ovntest.TestifyMockHelper{
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
{OnCallMethodName: "Attrs", OnCallMethodArgType: []string{}, RetArgList: []interface{}{&netlink.LinkAttrs{Name: defaultIf}}},
},
},
}
for i, tc := range tests {
t.Run(fmt.Sprintf("%d:%s", i, tc.desc), func(t *testing.T) {
ovntest.ProcessMockFnList(&mockNetLinkOps.Mock, tc.netLinkOpsMockHelper)
ovntest.ProcessMockFnList(&mockLink.Mock, tc.linkMockHelper)
intfName, gwIPs, err := getDefaultGatewayInterfaceDetails(tc.gwIface, tc.ipV4Mode, tc.ipV6Mode)
if intfName != tc.expIntfName {
t.Fatalf("TestGetDefaultGatewayInterfaceDetails(%d): Default gateway interface should be '%v' but got '%v'",
i, tc.expIntfName, intfName)
}
if !reflect.DeepEqual(tc.expGatewayIPs, gwIPs) {
t.Fatalf("TestGetDefaultGatewayInterfaceDetails(%d): Default gateway IPs should be '%v' but got '%v'",
i, tc.expGatewayIPs, gwIPs)
}

t.Log(err)
if tc.expErr {
assert.Error(t, err)
} else {
assert.Nil(t, err)
}
mockNetLinkOps.AssertExpectations(t)
mockLink.AssertExpectations(t)
})
}
}

0 comments on commit b1774d7

Please sign in to comment.