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

Add unit tests for assignIPToLink in GCS network setup #2309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add unit tests for assignIPToLink in GCS network setup #2309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 11 additions & 4 deletions internal/guest/network/netns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ import (
"github.com/vishvananda/netns"
)

var (
// function definitions for mocking assignIPToLink
netlinkAddrAdd = netlink.AddrAdd
netlinkRuleAdd = netlink.RuleAdd
netlinkRouteAdd = netlink.RouteAdd
)

// MoveInterfaceToNS moves the adapter with interface name `ifStr` to the network namespace
// of `pid`.
func MoveInterfaceToNS(ifStr string, pid int) error {
Expand Down Expand Up @@ -219,7 +226,7 @@ func assignIPToLink(ctx context.Context,
"IP": addr,
}).Debugf("parsed ip address %s/%d", allocatedIP, prefixLen)
ipAddr := &netlink.Addr{IPNet: addr, Label: ""}
if err := netlink.AddrAdd(link, ipAddr); err != nil {
if err := netlinkAddrAdd(link, ipAddr); err != nil {
return errors.Wrapf(err, "netlink.AddrAdd(%#v, %#v) failed", link, ipAddr)
}
if gatewayIP == "" {
Expand All @@ -242,7 +249,7 @@ func assignIPToLink(ctx context.Context,
IP: gw,
Mask: net.CIDRMask(ml, ml)}
ipAddr2 := &netlink.Addr{IPNet: addr2, Label: ""}
if err := netlink.AddrAdd(link, ipAddr2); err != nil {
if err := netlinkAddrAdd(link, ipAddr2); err != nil {
return errors.Wrapf(err, "netlink.AddrAdd(%#v, %#v) failed", link, ipAddr2)
}
}
Expand All @@ -261,7 +268,7 @@ func assignIPToLink(ctx context.Context,
rule.Src = srcNet
rule.Priority = 5

if err := netlink.RuleAdd(rule); err != nil {
if err := netlinkRuleAdd(rule); err != nil {
return errors.Wrapf(err, "netlink.RuleAdd(%#v) failed", rule)
}
table = rule.Table
Expand All @@ -274,7 +281,7 @@ func assignIPToLink(ctx context.Context,
Table: table,
Priority: metric,
}
if err := netlink.RouteAdd(&route); err != nil {
if err := netlinkRouteAdd(&route); err != nil {
return errors.Wrapf(err, "netlink.RouteAdd(%#v) failed", route)
}
return nil
Expand Down
228 changes: 228 additions & 0 deletions internal/guest/network/netns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
//go:build linux
// +build linux

package network

import (
"bytes"
"context"
"fmt"
"net"
"testing"

"github.com/vishvananda/netlink"
)

type fakeLink struct {
attr *netlink.LinkAttrs
}

func (l *fakeLink) Attrs() *netlink.LinkAttrs {
return l.attr
}

func (l *fakeLink) Type() string {
return ""
}

func newFakeLink(name string, index int) *fakeLink {
attr := &netlink.LinkAttrs{
Name: name,
Index: index,
}
return &fakeLink{
attr: attr,
}
}

var _ = (netlink.Link)(&fakeLink{})

func standardNetlinkAddrAdd(expectedIP string, prefixLen, totalMaskSize int) func(_ netlink.Link, _ *netlink.Addr) error {
return func(link netlink.Link, addr *netlink.Addr) error {
if addr.IP.String() != expectedIP {
return fmt.Errorf("expected to add address %s, instead got %s", expectedIP, addr.IP.String())
}
expectedMask := net.CIDRMask(prefixLen, totalMaskSize)
if !bytes.Equal(addr.Mask, expectedMask) {
return fmt.Errorf("expected mask to be %s, instead got %s", expectedMask, addr.Mask)
}
return nil
}
}
Comment on lines +40 to +51
Copy link
Member

Choose a reason for hiding this comment

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

Do we only care about inspecting what values are passed to AddrAdd/RuleAdd/RouteAdd, or do we actually need the ability to modify their return values?

If it's the former, I would suggest an alternate approach here which may be more flexible and readable: Have a single set of AddrAdd/RuleAdd/RouteAdd implementations, which add the arguments they were called with to a tracking slice. Then, after assignIPToLink returns, inspect the tracking slice to ensure the right set of calls were made.

This will help getting rid of the awkward variables that track how many times AddrAdd has been called, as instead you can just do some asserts like:

if len(addrAddCalls) != 2 {
	t.Failf("AddrAdd called %d times instead of 2", len(addrAddCalls))
}

if addrAddCalls[0].IP.String() != tt.allocatedIP {
	t.Errorf("First AddrAdd call has IP %s instead of %s", addrAddCalls[0].IP.String(), tt.allocatedIP)
}
expectedMask := net.CIDRMask(int(tt.prefixLen), tt.totalMaskSize)
if !bytes.Equal(addrAddCalls[0].Mask, expectedMask) {
	t.Errorf("First AddrAdd call has mask %s instead of %s", addrAddCalls[0].Mask, expectedMask)
}

if addrAddCalls[1].IP.String() != tt.gatewayIP {
	t.Errorf("Second AddrAdd call has IP %s instead of %s", addrAddCalls[1].IP.String(), tt.gatewayIP)
}
expectedMask = net.CIDRMask(tt.totalMaskSize, tt.totalMaskSize)
if !bytes.Equal(addrAddCalls[1].Mask, expectedMask) {
	t.Errorf("Second AddrAdd call has mask %s instead of %s", addrAddCalls[1].Mask, expectedMask)
}

You could even generalize further and just add a slice of expected call data to your test matrix, and then validate that the actual calls match that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we want both -- though in these tests, the ability to change the return value is not used. It's still in draft, but I completely redid all of these tests for the work to move to HCN v2 endpoints and support custom routes #2320. In that PR, the tests are set up similar to what you have suggested here and there is a case where we modify the return value from a netlink call as well.

The tests in this current PR were meant to add test coverage in the interim while the changes in my draft PR above are tested and reviewed. Would you still like me to modify the tests here with your suggestion?


func standardNetlinkRouteAdd(gatewayIP string, table, metric int) func(_ *netlink.Route) error {
return func(route *netlink.Route) error {
if route.Gw.String() != gatewayIP {
return fmt.Errorf("expected to add gateway %s, instead got %s", gatewayIP, route.Gw.String())
}
if route.Table != table {
return fmt.Errorf("expected to use table %d, instead got %d", table, route.Table)
}
if route.Priority != metric {
return fmt.Errorf("expected to use metric %d, instead used %d", metric, route.Priority)
}
return nil
}
}

type assignIPToLinkTest struct {
name string
ifStr string
allocatedIP string
gatewayIP string
prefixLen uint8
totalMaskSize int
}

var defaultAssignIPToLinkTests = []assignIPToLinkTest{
{
name: "ipv4 standard",
ifStr: "eth0",
allocatedIP: "192.168.0.5",
gatewayIP: "192.168.0.100",
prefixLen: uint8(24),
Copy link
Member

Choose a reason for hiding this comment

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

nit: Don't need the uint8 qualifier here.

totalMaskSize: 32,
},
{
name: "ipv6 standard",
ifStr: "eth0",
allocatedIP: "9541:a2d4:f0f3:18ff:c868:26ce:e9c4:30a6",
gatewayIP: "9541:a2d4:f0f3:18ff:c868:26ce:e9c4:aaaa",
prefixLen: uint8(64),
totalMaskSize: 128,
},
}

func Test_AssignIPToLink(t *testing.T) {
ctx := context.Background()

for _, tt := range defaultAssignIPToLinkTests {
t.Run(tt.name, func(st *testing.T) {
link1 := newFakeLink(tt.ifStr, 0)

netlinkAddrAdd = standardNetlinkAddrAdd(tt.allocatedIP, int(tt.prefixLen), tt.totalMaskSize)
netlinkRouteAdd = standardNetlinkRouteAdd(tt.gatewayIP, 0, 1)

if err := assignIPToLink(ctx, tt.ifStr, 10, link1, tt.allocatedIP, tt.gatewayIP, tt.prefixLen, false, 1); err != nil {
st.Fatalf("assignIPToLink: %s", err)
}
})
}

}

func Test_AssignIPToLink_No_Gateway(t *testing.T) {
ctx := context.Background()

for _, tt := range defaultAssignIPToLinkTests {
t.Run(tt.name, func(st *testing.T) {
// remove the gateway IP set for the tests
tt.gatewayIP = ""
link1 := newFakeLink(tt.ifStr, 0)

netlinkAddrAdd = standardNetlinkAddrAdd(tt.allocatedIP, int(tt.prefixLen), tt.totalMaskSize)
netlinkRouteAdd = standardNetlinkRouteAdd(tt.gatewayIP, 0, 1)

if err := assignIPToLink(ctx, tt.ifStr, 10, link1, tt.allocatedIP, tt.gatewayIP, tt.prefixLen, false, 1); err != nil {
st.Fatalf("assignIPToLink: %s", err)
}
})
}

}

func Test_AssignIPToLink_GatewayOutsideSubnet(t *testing.T) {
ctx := context.Background()

var assignIPToLinkTestsGateway = []assignIPToLinkTest{
Copy link
Contributor

Choose a reason for hiding this comment

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

am I seeing this wrong or is this same as defaultAssignIPToLinkTests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one has gateway addresses that are outside the subnet of the IP addresses. I figured it was easier to just make new tests than try to change the gateway address based on if the IP was ipv4 or 6.

{
name: "ipv4 standard",
ifStr: "eth0",
allocatedIP: "192.168.0.5",
gatewayIP: "10.0.0.5",
prefixLen: uint8(24),
totalMaskSize: 32,
},
{
name: "ipv6 standard",
ifStr: "eth0",
allocatedIP: "9541:a2d4:f0f3:18ff:c868:26ce:e9c4:30a6",
gatewayIP: "337c:83ab:b4cc:d823:6b5d:6aea:f605:80c5",
prefixLen: uint8(64),
totalMaskSize: 128,
},
}

for _, tt := range assignIPToLinkTestsGateway {
t.Run(tt.name, func(st *testing.T) {
link1 := newFakeLink(tt.ifStr, 0)

netlinkAddCalls := 0
netlinkAddrAdd = func(link netlink.Link, addr *netlink.Addr) error {
expectedIP := tt.allocatedIP
expectedMask := net.CIDRMask(int(tt.prefixLen), tt.totalMaskSize)
if netlinkAddCalls != 0 {
// on the second call, we want to check for the gateway address being added
expectedIP = tt.gatewayIP
expectedMask = net.CIDRMask(tt.totalMaskSize, tt.totalMaskSize)
}
if addr.IP.String() != expectedIP {
return fmt.Errorf("expected to add address %s, instead got %s", expectedIP, addr.IP.String())
}
if !bytes.Equal(addr.Mask, expectedMask) {
return fmt.Errorf("expected mask to be %s, instead got %s", expectedMask, addr.Mask)
}
netlinkAddCalls++
return nil
}

netlinkRouteAdd = standardNetlinkRouteAdd(tt.gatewayIP, 0, 1)

if err := assignIPToLink(ctx, tt.ifStr, 10, link1, tt.allocatedIP, tt.gatewayIP, tt.prefixLen, false, 1); err != nil {
st.Fatalf("assignIPToLink: %s", err)
}

if netlinkAddCalls < 2 {
st.Fatalf("expected to call netlink AddrAdd %d times, instead got %d times", 2, netlinkAddCalls)
}
})
}

}

func Test_AssignIPToLink_EnableLowMetric(t *testing.T) {
ctx := context.Background()
table := 101
metric := 500

for _, tt := range defaultAssignIPToLinkTests {
t.Run(tt.name, func(st *testing.T) {
link1 := newFakeLink(tt.ifStr, 0)

netlinkAddrAdd = standardNetlinkAddrAdd(tt.allocatedIP, int(tt.prefixLen), tt.totalMaskSize)
netlinkRouteAdd = standardNetlinkRouteAdd(tt.gatewayIP, table, metric)

netlinkRuleAddCalled := false
netlinkRuleAdd = func(rule *netlink.Rule) error {
netlinkRuleAddCalled = true
if rule.Src.IP.String() != tt.allocatedIP {
return fmt.Errorf("expected to add rule for address %s, instead got %s", tt.allocatedIP, rule.Src.IP.String())
}
expectedMask := net.CIDRMask(tt.totalMaskSize, tt.totalMaskSize)
if !bytes.Equal(expectedMask, rule.Src.Mask) {
return fmt.Errorf("expected mask to be %s, instead got %s", expectedMask, rule.Src.Mask)
}
return nil
}

if err := assignIPToLink(ctx, tt.ifStr, 10, link1, tt.allocatedIP, tt.gatewayIP, tt.prefixLen, true, metric); err != nil {
t.Fatalf("assignIPToLink: %s", err)
}

if !netlinkRuleAddCalled {
t.Fatal("should have added a rule since enableLowMetric was set")
}
})
}

}
Loading