Skip to content

[release-4.13] OCPBUGS-15496: Fix default GW IPs retrieval #1738

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

Merged
openshift-merge-robot merged 3 commits into from
Aug 3, 2023
Merged

[release-4.13] OCPBUGS-15496: Fix default GW IPs retrieval #1738

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
9cbdd3b
Allow a scenario where there is only IPv4 default GW specified in a d…
kyrtapz May 4, 2023
7b17395
getDefaultGatewayInterfaceDetails: do not return empty gateway IPs
kyrtapz May 4, 2023
b332d91
Add unit test for getDefaultGatewayInterfaceByFamily and getDefaultGa…
kyrtapz May 4, 2023
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
2 changes: 1 addition & 1 deletion go-controller/pkg/node/gateway_init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
})
}
}