From 1134a1434b6acdda7beb29aeb32331fe14f16803 Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Wed, 2 Oct 2019 11:47:20 +0300 Subject: [PATCH 1/7] Made timeouts exponential --- pkg/networkutils/network.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index 38478e047a..f44bd05486 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -645,7 +645,7 @@ func LinkByMac(mac string, netLink netlinkwrapper.NetLink, retryInterval time.Du if attempt > maxAttemptsLinkByMac { return nil, lastErr } else if attempt > 1 { - time.Sleep(retryInterval) + time.Sleep(retryInterval * time.Duration(attempt)) } links, err := netLink.LinkList() @@ -773,7 +773,7 @@ func setupENINetwork(eniIP string, eniMAC string, eniTable int, eniSubnetCIDR st } log.Debugf("Not able to add route route %s/0 via %s table %d (attempt %d/%d)", r.Dst.IP.String(), gw.String(), eniTable, retry, maxRetryRouteAdd) - time.Sleep(retryRouteAddInterval) + time.Sleep(retryRouteAddInterval * time.Duration(retry)) } else if netlinkwrapper.IsRouteExistsError(err) { if err := netLink.RouteReplace(&r); err != nil { return errors.Wrapf(err, "setupENINetwork: unable to replace route entry %s", r.Dst.IP.String()) From 03e761fa034a9bd8048a91d9e71b96f2fa9bc7d0 Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Thu, 3 Oct 2019 11:31:19 +0300 Subject: [PATCH 2/7] Added RetryNWithBackoff Signed-off-by: bpopovschi --- pkg/networkutils/network.go | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index f44bd05486..5b74017c36 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -17,6 +17,7 @@ import ( "encoding/binary" "encoding/csv" "fmt" + "github.com/aws/amazon-vpc-cni-k8s/pkg/utils/retry" "io" "math" "net" @@ -645,7 +646,7 @@ func LinkByMac(mac string, netLink netlinkwrapper.NetLink, retryInterval time.Du if attempt > maxAttemptsLinkByMac { return nil, lastErr } else if attempt > 1 { - time.Sleep(retryInterval * time.Duration(attempt)) + time.Sleep(retryInterval) } links, err := netLink.LinkList() @@ -759,36 +760,28 @@ func setupENINetwork(eniIP string, eniMAC string, eniTable int, eniSubnetCIDR st return errors.Wrap(err, "setupENINetwork: failed to clean up old routes") } - // In case of route dependency, retry few times - retry := 0 - for { + err = retry.RetryNWithBackoff(retry.NewSimpleBackoff(time.Millisecond*500, retryRouteAddInterval*time.Duration(maxRetryRouteAdd), 0.15, 2.0), maxRetryRouteAdd, func() error { if err := netLink.RouteAdd(&r); err != nil { if netlinkwrapper.IsNetworkUnreachableError(err) { - retry++ - if retry > maxRetryRouteAdd { - log.Errorf("Failed to add route %s/0 via %s table %d", - r.Dst.IP.String(), gw.String(), eniTable) - return errors.Wrapf(err, "setupENINetwork: failed to add route %s/0 via %s table %d", - r.Dst.IP.String(), gw.String(), eniTable) - } - log.Debugf("Not able to add route route %s/0 via %s table %d (attempt %d/%d)", - r.Dst.IP.String(), gw.String(), eniTable, retry, maxRetryRouteAdd) - time.Sleep(retryRouteAddInterval * time.Duration(retry)) + log.Debugf("Not able to add route route %s/0 via %s table %d ", + r.Dst.IP.String(), gw.String(), eniTable, maxRetryRouteAdd) + time.Sleep(retryRouteAddInterval) } else if netlinkwrapper.IsRouteExistsError(err) { if err := netLink.RouteReplace(&r); err != nil { return errors.Wrapf(err, "setupENINetwork: unable to replace route entry %s", r.Dst.IP.String()) } log.Debugf("Successfully replaced route to be %s/0", r.Dst.IP.String()) - break + return nil } else { return errors.Wrapf(err, "setupENINetwork: unable to add route %s/0 via %s table %d", r.Dst.IP.String(), gw.String(), eniTable) } } else { log.Debugf("Successfully added route route %s/0 via %s table %d", r.Dst.IP.String(), gw.String(), eniTable) - break + return nil } - } + return nil + }) } // Remove the route that default out to ENI-x out of main route table From 7f65d54ec8cbcdd4c89b222f1b0c914fb14f5ccb Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Mon, 7 Oct 2019 11:17:06 +0300 Subject: [PATCH 3/7] Simplified ENI network setup Signed-off-by: bpopovschi --- pkg/networkutils/network.go | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index 5b74017c36..ef1d6d3b8f 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -17,7 +17,6 @@ import ( "encoding/binary" "encoding/csv" "fmt" - "github.com/aws/amazon-vpc-cni-k8s/pkg/utils/retry" "io" "math" "net" @@ -28,6 +27,8 @@ import ( "syscall" "time" + "github.com/aws/amazon-vpc-cni-k8s/pkg/utils/retry" + "github.com/pkg/errors" "golang.org/x/sys/unix" @@ -760,26 +761,13 @@ func setupENINetwork(eniIP string, eniMAC string, eniTable int, eniSubnetCIDR st return errors.Wrap(err, "setupENINetwork: failed to clean up old routes") } - err = retry.RetryNWithBackoff(retry.NewSimpleBackoff(time.Millisecond*500, retryRouteAddInterval*time.Duration(maxRetryRouteAdd), 0.15, 2.0), maxRetryRouteAdd, func() error { - if err := netLink.RouteAdd(&r); err != nil { - if netlinkwrapper.IsNetworkUnreachableError(err) { - log.Debugf("Not able to add route route %s/0 via %s table %d ", - r.Dst.IP.String(), gw.String(), eniTable, maxRetryRouteAdd) - time.Sleep(retryRouteAddInterval) - } else if netlinkwrapper.IsRouteExistsError(err) { - if err := netLink.RouteReplace(&r); err != nil { - return errors.Wrapf(err, "setupENINetwork: unable to replace route entry %s", r.Dst.IP.String()) - } - log.Debugf("Successfully replaced route to be %s/0", r.Dst.IP.String()) - return nil - } else { - return errors.Wrapf(err, "setupENINetwork: unable to add route %s/0 via %s table %d", - r.Dst.IP.String(), gw.String(), eniTable) - } - } else { - log.Debugf("Successfully added route route %s/0 via %s table %d", r.Dst.IP.String(), gw.String(), eniTable) - return nil + err = retry.RetryNWithBackoff(retry.NewSimpleBackoff(500*time.Microsecond, retryRouteAddInterval, 0.15, 2.0), maxRetryRouteAdd, func() error { + if err := netLink.RouteReplace(&r); err != nil { + log.Debugf("Not able to add/replace route route %s/0 via %s table %d ", + r.Dst.IP.String(), gw.String(), eniTable, maxRetryRouteAdd) + return errors.Wrapf(err, "setupENINetwork: unable to replace route entry %s", r.Dst.IP.String()) } + log.Debugf("Successfully added/replaced route to be %s/0", r.Dst.IP.String()) return nil }) } From c28d4dde67197fc597f62907c60531b25afd84e0 Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Wed, 9 Oct 2019 17:00:02 +0300 Subject: [PATCH 4/7] Unit tests fix Signed-off-by: bpopovschi --- pkg/networkutils/network_test.go | 78 +++++++++----------------------- 1 file changed, 22 insertions(+), 56 deletions(-) diff --git a/pkg/networkutils/network_test.go b/pkg/networkutils/network_test.go index bb50df8d46..ee1991906c 100644 --- a/pkg/networkutils/network_test.go +++ b/pkg/networkutils/network_test.go @@ -38,7 +38,6 @@ import ( ) const ( - loopback = "" testMAC1 = "01:23:45:67:89:a0" testMAC2 = "01:23:45:67:89:a1" testTable = 10 @@ -73,14 +72,17 @@ func TestSetupENINetwork(t *testing.T) { hwAddr, err := net.ParseMAC(testMAC1) assert.NoError(t, err) + mockLinkAttrs1 := &netlink.LinkAttrs{ HardwareAddr: hwAddr, } hwAddr, err = net.ParseMAC(testMAC2) assert.NoError(t, err) + mockLinkAttrs2 := &netlink.LinkAttrs{ HardwareAddr: hwAddr, } + lo := mock_netlink.NewMockLink(ctrl) eth1 := mock_netlink.NewMockLink(ctrl) // Emulate a delay attaching the ENI so a retry is necessary @@ -92,11 +94,14 @@ func TestSetupENINetwork(t *testing.T) { lo.EXPECT().Attrs().Return(mockLinkAttrs1) eth1.EXPECT().Attrs().Return(mockLinkAttrs2) gomock.InOrder(firstlistSet, secondlistSet) + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) mockNetLink.EXPECT().LinkSetUp(gomock.Any()).Return(nil) + // eth1's device eth1.EXPECT().Attrs().Return(mockLinkAttrs2) eth1.EXPECT().Attrs().Return(mockLinkAttrs2) + // eth1's IP address testeniAddr := &net.IPNet{ IP: net.ParseIP(testeniIP), @@ -113,7 +118,7 @@ func TestSetupENINetwork(t *testing.T) { mockNetLink.EXPECT().RouteDel(gomock.Any()).Return(nil) - err = setupENINetwork(testeniIP, testMAC2, testTable, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second, testMTU) + err = setupENINetwork(testeniIP, testMAC2, testTable, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second) assert.NoError(t, err) } @@ -127,7 +132,7 @@ func TestSetupENINetworkMACFail(t *testing.T) { mockNetLink.EXPECT().LinkList().Return(nil, fmt.Errorf("simulated failure")) } - err := setupENINetwork(testeniIP, testMAC2, testTable, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second, testMTU) + err := setupENINetwork(testeniIP, testMAC2, testTable, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second) assert.Errorf(t, err, "simulated failure") } @@ -135,7 +140,7 @@ func TestSetupENINetworkPrimary(t *testing.T) { ctrl, mockNetLink, _, _, _ := setup(t) defer ctrl.Finish() - err := setupENINetwork(testeniIP, testMAC2, 0, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second, testMTU) + err := setupENINetwork(testeniIP, testMAC2, 0, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second) assert.NoError(t, err) } @@ -145,16 +150,14 @@ func TestSetupHostNetworkNodePortDisabled(t *testing.T) { ln := &linuxNetwork{ mainENIMark: 0x80, - mtu: testMTU, - netLink: mockNetLink, - ns: mockNS, + + netLink: mockNetLink, + ns: mockNS, newIptables: func() (iptablesIface, error) { return mockIptables, nil }, } - mockPrimaryInterfaceLookup(ctrl, mockNetLink) - mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -163,19 +166,10 @@ func TestSetupHostNetworkNodePortDisabled(t *testing.T) { mockNetLink.EXPECT().RuleDel(&mainENIRule) var vpcCIDRs []*string - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) assert.NoError(t, err) } -func mockPrimaryInterfaceLookup(ctrl *gomock.Controller, mockNetLink *mock_netlinkwrapper.MockNetLink) { - lo := mock_netlink.NewMockLink(ctrl) - mockLinkAttrs1 := &netlink.LinkAttrs{ - HardwareAddr: net.HardwareAddr{}, - } - mockNetLink.EXPECT().LinkList().Return([]netlink.Link{lo}, nil) - lo.EXPECT().Attrs().AnyTimes().Return(mockLinkAttrs1) -} - func TestUpdateRuleListBySrc(t *testing.T) { ctrl, mockNetLink, _, _, _ := setup(t) defer ctrl.Finish() @@ -273,7 +267,6 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { useExternalSNAT: true, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, - mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -285,9 +278,6 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { }, } - mockPrimaryInterfaceLookup(ctrl, mockNetLink) - mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) - var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -298,7 +288,11 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { var vpcCIDRs []*string - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) + // loopback for primary device is a little bit hacky. But the test is stable and it should be + // OK for test purpose. + LoopBackMac := "" + + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, LoopBackMac, &testENINetIP) assert.NoError(t, err) assert.Equal(t, map[string]map[string][][]string{ @@ -320,21 +314,6 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { assert.Equal(t, mockFile{closed: true, data: "2"}, mockRPFilter) } -func TestLoadMTUFromEnvTooLow(t *testing.T) { - _ = os.Setenv(envMTU, "1") - assert.Equal(t, GetEthernetMTU(""), minimumMTU) -} - -func TestLoadMTUFromEnv1500(t *testing.T) { - _ = os.Setenv(envMTU, "1500") - assert.Equal(t, GetEthernetMTU(""), 1500) -} - -func TestLoadMTUFromEnvTooHigh(t *testing.T) { - _ = os.Setenv(envMTU, "65536") - assert.Equal(t, GetEthernetMTU(""), maximumMTU) -} - func TestLoadExcludeSNATCIDRsFromEnv(t *testing.T) { _ = os.Setenv(envExternalSNAT, "false") _ = os.Setenv(envExcludeSNATCIDRs, "10.12.0.0/16,10.13.0.0/16") @@ -353,7 +332,6 @@ func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { excludeSNATCIDRs: []string{"10.12.0.0/16", "10.13.0.0/16"}, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, - mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -365,9 +343,6 @@ func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { }, } - mockPrimaryInterfaceLookup(ctrl, mockNetLink) - - mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -378,7 +353,7 @@ func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { var vpcCIDRs []*string vpcCIDRs = []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) assert.NoError(t, err) assert.Equal(t, map[string]map[string][][]string{ @@ -408,7 +383,6 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { excludeSNATCIDRs: nil, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, - mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -419,9 +393,7 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { return &mockRPFilter, nil }, } - mockPrimaryInterfaceLookup(ctrl, mockNetLink) - mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -439,7 +411,7 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { _ = mockIptables.NewChain("nat", "AWS-SNAT-CHAIN-5") _ = mockIptables.Append("nat", "POSTROUTING", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-0") - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) assert.NoError(t, err) assert.Equal(t, @@ -470,7 +442,6 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { excludeSNATCIDRs: []string{"10.12.0.0/16", "10.13.0.0/16"}, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, - mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -481,9 +452,7 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { return &mockRPFilter, nil }, } - mockPrimaryInterfaceLookup(ctrl, mockNetLink) - mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -501,7 +470,7 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { // remove exclusions vpcCIDRs := []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) assert.NoError(t, err) assert.Equal(t, @@ -531,7 +500,6 @@ func TestSetupHostNetworkMultipleCIDRs(t *testing.T) { useExternalSNAT: true, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, - mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -542,9 +510,7 @@ func TestSetupHostNetworkMultipleCIDRs(t *testing.T) { return &mockRPFilter, nil }, } - mockPrimaryInterfaceLookup(ctrl, mockNetLink) - mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -555,7 +521,7 @@ func TestSetupHostNetworkMultipleCIDRs(t *testing.T) { var vpcCIDRs []*string vpcCIDRs = []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) assert.NoError(t, err) } From 68571f69f5219ee5df88b5fc0caec99629822315 Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Tue, 29 Oct 2019 16:58:30 +0200 Subject: [PATCH 5/7] comments fix Signed-off-by: bpopovschi --- pkg/networkutils/network.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index ef1d6d3b8f..87285b4566 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -761,15 +761,19 @@ func setupENINetwork(eniIP string, eniMAC string, eniTable int, eniSubnetCIDR st return errors.Wrap(err, "setupENINetwork: failed to clean up old routes") } - err = retry.RetryNWithBackoff(retry.NewSimpleBackoff(500*time.Microsecond, retryRouteAddInterval, 0.15, 2.0), maxRetryRouteAdd, func() error { + err = retry.RetryNWithBackoff(retry.NewSimpleBackoff(500*time.Millisecond, retryRouteAddInterval, 0.15, 2.0), maxRetryRouteAdd, func() error { if err := netLink.RouteReplace(&r); err != nil { - log.Debugf("Not able to add/replace route route %s/0 via %s table %d ", - r.Dst.IP.String(), gw.String(), eniTable, maxRetryRouteAdd) + log.Debugf("Not able to set route %s/0 via %s table %d", + r.Dst.IP.String(), gw.String(), eniTable) return errors.Wrapf(err, "setupENINetwork: unable to replace route entry %s", r.Dst.IP.String()) } + log.Debugf("Successfully added/replaced route to be %s/0", r.Dst.IP.String()) return nil }) + if err != nil { + return err + } } // Remove the route that default out to ENI-x out of main route table From 9b5348d846ddd9a1b46a3f6806e109c3a2d6400c Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Tue, 29 Oct 2019 19:36:03 +0200 Subject: [PATCH 6/7] Tests fix Signed-off-by: bpopovschi --- pkg/networkutils/network_test.go | 78 +++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 22 deletions(-) diff --git a/pkg/networkutils/network_test.go b/pkg/networkutils/network_test.go index ee1991906c..bb50df8d46 100644 --- a/pkg/networkutils/network_test.go +++ b/pkg/networkutils/network_test.go @@ -38,6 +38,7 @@ import ( ) const ( + loopback = "" testMAC1 = "01:23:45:67:89:a0" testMAC2 = "01:23:45:67:89:a1" testTable = 10 @@ -72,17 +73,14 @@ func TestSetupENINetwork(t *testing.T) { hwAddr, err := net.ParseMAC(testMAC1) assert.NoError(t, err) - mockLinkAttrs1 := &netlink.LinkAttrs{ HardwareAddr: hwAddr, } hwAddr, err = net.ParseMAC(testMAC2) assert.NoError(t, err) - mockLinkAttrs2 := &netlink.LinkAttrs{ HardwareAddr: hwAddr, } - lo := mock_netlink.NewMockLink(ctrl) eth1 := mock_netlink.NewMockLink(ctrl) // Emulate a delay attaching the ENI so a retry is necessary @@ -94,14 +92,11 @@ func TestSetupENINetwork(t *testing.T) { lo.EXPECT().Attrs().Return(mockLinkAttrs1) eth1.EXPECT().Attrs().Return(mockLinkAttrs2) gomock.InOrder(firstlistSet, secondlistSet) - mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) mockNetLink.EXPECT().LinkSetUp(gomock.Any()).Return(nil) - // eth1's device eth1.EXPECT().Attrs().Return(mockLinkAttrs2) eth1.EXPECT().Attrs().Return(mockLinkAttrs2) - // eth1's IP address testeniAddr := &net.IPNet{ IP: net.ParseIP(testeniIP), @@ -118,7 +113,7 @@ func TestSetupENINetwork(t *testing.T) { mockNetLink.EXPECT().RouteDel(gomock.Any()).Return(nil) - err = setupENINetwork(testeniIP, testMAC2, testTable, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second) + err = setupENINetwork(testeniIP, testMAC2, testTable, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second, testMTU) assert.NoError(t, err) } @@ -132,7 +127,7 @@ func TestSetupENINetworkMACFail(t *testing.T) { mockNetLink.EXPECT().LinkList().Return(nil, fmt.Errorf("simulated failure")) } - err := setupENINetwork(testeniIP, testMAC2, testTable, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second) + err := setupENINetwork(testeniIP, testMAC2, testTable, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second, testMTU) assert.Errorf(t, err, "simulated failure") } @@ -140,7 +135,7 @@ func TestSetupENINetworkPrimary(t *testing.T) { ctrl, mockNetLink, _, _, _ := setup(t) defer ctrl.Finish() - err := setupENINetwork(testeniIP, testMAC2, 0, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second) + err := setupENINetwork(testeniIP, testMAC2, 0, testeniSubnet, mockNetLink, 0*time.Second, 0*time.Second, testMTU) assert.NoError(t, err) } @@ -150,14 +145,16 @@ func TestSetupHostNetworkNodePortDisabled(t *testing.T) { ln := &linuxNetwork{ mainENIMark: 0x80, - - netLink: mockNetLink, - ns: mockNS, + mtu: testMTU, + netLink: mockNetLink, + ns: mockNS, newIptables: func() (iptablesIface, error) { return mockIptables, nil }, } + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -166,10 +163,19 @@ func TestSetupHostNetworkNodePortDisabled(t *testing.T) { mockNetLink.EXPECT().RuleDel(&mainENIRule) var vpcCIDRs []*string - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) assert.NoError(t, err) } +func mockPrimaryInterfaceLookup(ctrl *gomock.Controller, mockNetLink *mock_netlinkwrapper.MockNetLink) { + lo := mock_netlink.NewMockLink(ctrl) + mockLinkAttrs1 := &netlink.LinkAttrs{ + HardwareAddr: net.HardwareAddr{}, + } + mockNetLink.EXPECT().LinkList().Return([]netlink.Link{lo}, nil) + lo.EXPECT().Attrs().AnyTimes().Return(mockLinkAttrs1) +} + func TestUpdateRuleListBySrc(t *testing.T) { ctrl, mockNetLink, _, _, _ := setup(t) defer ctrl.Finish() @@ -267,6 +273,7 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { useExternalSNAT: true, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, + mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -278,6 +285,9 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { }, } + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) + var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -288,11 +298,7 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { var vpcCIDRs []*string - // loopback for primary device is a little bit hacky. But the test is stable and it should be - // OK for test purpose. - LoopBackMac := "" - - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, LoopBackMac, &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) assert.NoError(t, err) assert.Equal(t, map[string]map[string][][]string{ @@ -314,6 +320,21 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { assert.Equal(t, mockFile{closed: true, data: "2"}, mockRPFilter) } +func TestLoadMTUFromEnvTooLow(t *testing.T) { + _ = os.Setenv(envMTU, "1") + assert.Equal(t, GetEthernetMTU(""), minimumMTU) +} + +func TestLoadMTUFromEnv1500(t *testing.T) { + _ = os.Setenv(envMTU, "1500") + assert.Equal(t, GetEthernetMTU(""), 1500) +} + +func TestLoadMTUFromEnvTooHigh(t *testing.T) { + _ = os.Setenv(envMTU, "65536") + assert.Equal(t, GetEthernetMTU(""), maximumMTU) +} + func TestLoadExcludeSNATCIDRsFromEnv(t *testing.T) { _ = os.Setenv(envExternalSNAT, "false") _ = os.Setenv(envExcludeSNATCIDRs, "10.12.0.0/16,10.13.0.0/16") @@ -332,6 +353,7 @@ func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { excludeSNATCIDRs: []string{"10.12.0.0/16", "10.13.0.0/16"}, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, + mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -343,6 +365,9 @@ func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { }, } + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -353,7 +378,7 @@ func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { var vpcCIDRs []*string vpcCIDRs = []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) assert.NoError(t, err) assert.Equal(t, map[string]map[string][][]string{ @@ -383,6 +408,7 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { excludeSNATCIDRs: nil, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, + mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -393,7 +419,9 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { return &mockRPFilter, nil }, } + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -411,7 +439,7 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { _ = mockIptables.NewChain("nat", "AWS-SNAT-CHAIN-5") _ = mockIptables.Append("nat", "POSTROUTING", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-0") - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) assert.NoError(t, err) assert.Equal(t, @@ -442,6 +470,7 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { excludeSNATCIDRs: []string{"10.12.0.0/16", "10.13.0.0/16"}, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, + mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -452,7 +481,9 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { return &mockRPFilter, nil }, } + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -470,7 +501,7 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { // remove exclusions vpcCIDRs := []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) assert.NoError(t, err) assert.Equal(t, @@ -500,6 +531,7 @@ func TestSetupHostNetworkMultipleCIDRs(t *testing.T) { useExternalSNAT: true, nodePortSupportEnabled: true, mainENIMark: defaultConnmark, + mtu: testMTU, netLink: mockNetLink, ns: mockNS, @@ -510,7 +542,9 @@ func TestSetupHostNetworkMultipleCIDRs(t *testing.T) { return &mockRPFilter, nil }, } + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) var hostRule netlink.Rule mockNetLink.EXPECT().NewRule().Return(&hostRule) mockNetLink.EXPECT().RuleDel(&hostRule) @@ -521,7 +555,7 @@ func TestSetupHostNetworkMultipleCIDRs(t *testing.T) { var vpcCIDRs []*string vpcCIDRs = []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) assert.NoError(t, err) } From e339c2b310078acff2bc590b68b99450482ba640 Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Tue, 29 Oct 2019 22:18:30 +0200 Subject: [PATCH 7/7] TestSetupENINetwork Fix Signed-off-by: bpopovschi --- pkg/networkutils/network_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/networkutils/network_test.go b/pkg/networkutils/network_test.go index bb50df8d46..a95a3b9e8b 100644 --- a/pkg/networkutils/network_test.go +++ b/pkg/networkutils/network_test.go @@ -106,10 +106,10 @@ func TestSetupENINetwork(t *testing.T) { mockNetLink.EXPECT().AddrAdd(gomock.Any(), &netlink.Addr{IPNet: testeniAddr}).Return(nil) mockNetLink.EXPECT().RouteDel(gomock.Any()) - mockNetLink.EXPECT().RouteAdd(gomock.Any()).Return(nil) + mockNetLink.EXPECT().RouteReplace(gomock.Any()).Return(nil) mockNetLink.EXPECT().RouteDel(gomock.Any()) - mockNetLink.EXPECT().RouteAdd(gomock.Any()).Return(nil) + mockNetLink.EXPECT().RouteReplace(gomock.Any()).Return(nil) mockNetLink.EXPECT().RouteDel(gomock.Any()).Return(nil)