From 21bd4ace257a91c3d7ec55f6512400843a150e9f Mon Sep 17 00:00:00 2001 From: Sri Saran Balaji Vellore Rajakumar Date: Mon, 6 Apr 2020 21:36:02 -0700 Subject: [PATCH] Revert "Update rp_filter only when /proc is accessible" This reverts commit 29474be84292dfb7351dab5605e6ace556be213d. --- pkg/networkutils/network.go | 14 +-- pkg/networkutils/network_test.go | 117 +++++++++++----------- pkg/procsyswrapper/mocks/procsys_mocks.go | 14 --- pkg/procsyswrapper/procsys.go | 8 -- 4 files changed, 64 insertions(+), 89 deletions(-) diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index 26956dd114..59f5b7edf7 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -243,16 +243,10 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, primaryIntfRPFilter := "net/ipv4/conf/" + primaryIntf + "/rp_filter" const rpFilterLoose = "2" - if n.procSys.IsPathWriteAccessible(primaryIntfRPFilter) { - // Setting RPF will be removed from aws-node when we bump our minor version. - log.Debugf("Setting RPF for primary interface: %s", primaryIntfRPFilter) - err = n.procSys.Set(primaryIntfRPFilter, rpFilterLoose) - if err != nil { - return errors.Wrapf(err, "failed to configure %s RPF check", primaryIntf) - } - } else { - // when aws-node run as an un-privileged pod, /proc will be mounted as read only - log.Infof("Skip updating RPF for primary interface: %s", primaryIntfRPFilter) + log.Debugf("Setting RPF for primary interface: %s", primaryIntfRPFilter) + err = n.procSys.Set(primaryIntfRPFilter, rpFilterLoose) + if err != nil { + return errors.Wrapf(err, "failed to configure %s RPF check", primaryIntf) } } diff --git a/pkg/networkutils/network_test.go b/pkg/networkutils/network_test.go index 4b9010b00c..a4488b025e 100644 --- a/pkg/networkutils/network_test.go +++ b/pkg/networkutils/network_test.go @@ -285,8 +285,18 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { procSys: mockProcSys, } - setupNetLinkMocks(ctrl, mockNetLink) - setupProcSysMocks(mockProcSys, true) + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) + + var hostRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&hostRule) + mockNetLink.EXPECT().RuleDel(&hostRule) + var mainENIRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&mainENIRule) + mockNetLink.EXPECT().RuleDel(&mainENIRule) + mockNetLink.EXPECT().RuleAdd(&mainENIRule) + + mockProcSys.EXPECT().Set("net/ipv4/conf/lo/rp_filter", "2").Return(nil) var vpcCIDRs []*string @@ -353,8 +363,18 @@ func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { procSys: mockProcSys, } - setupNetLinkMocks(ctrl, mockNetLink) - setupProcSysMocks(mockProcSys, true) + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) + var hostRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&hostRule) + mockNetLink.EXPECT().RuleDel(&hostRule) + var mainENIRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&mainENIRule) + mockNetLink.EXPECT().RuleDel(&mainENIRule) + mockNetLink.EXPECT().RuleAdd(&mainENIRule) + + mockProcSys.EXPECT().Set("net/ipv4/conf/lo/rp_filter", "2").Return(nil) var vpcCIDRs []*string vpcCIDRs = []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} @@ -396,8 +416,18 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { }, procSys: mockProcSys, } - setupNetLinkMocks(ctrl, mockNetLink) - setupProcSysMocks(mockProcSys, true) + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) + var hostRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&hostRule) + mockNetLink.EXPECT().RuleDel(&hostRule) + var mainENIRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&mainENIRule) + mockNetLink.EXPECT().RuleDel(&mainENIRule) + mockNetLink.EXPECT().RuleAdd(&mainENIRule) + + mockProcSys.EXPECT().Set("net/ipv4/conf/lo/rp_filter", "2").Return(nil) vpcCIDRs := []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-0", "!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-1") //AWS SNAT CHAN proves backwards compatibility @@ -447,8 +477,18 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { }, procSys: mockProcSys, } - setupNetLinkMocks(ctrl, mockNetLink) - setupProcSysMocks(mockProcSys, true) + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) + var hostRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&hostRule) + mockNetLink.EXPECT().RuleDel(&hostRule) + var mainENIRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&mainENIRule) + mockNetLink.EXPECT().RuleDel(&mainENIRule) + mockNetLink.EXPECT().RuleAdd(&mainENIRule) + + mockProcSys.EXPECT().Set("net/ipv4/conf/lo/rp_filter", "2").Return(nil) _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-0", "!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-1") _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-1", "!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-2") @@ -497,8 +537,18 @@ func TestSetupHostNetworkMultipleCIDRs(t *testing.T) { }, procSys: mockProcSys, } - setupNetLinkMocks(ctrl, mockNetLink) - setupProcSysMocks(mockProcSys, true) + mockPrimaryInterfaceLookup(ctrl, mockNetLink) + + mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) + var hostRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&hostRule) + mockNetLink.EXPECT().RuleDel(&hostRule) + var mainENIRule netlink.Rule + mockNetLink.EXPECT().NewRule().Return(&mainENIRule) + mockNetLink.EXPECT().RuleDel(&mainENIRule) + mockNetLink.EXPECT().RuleAdd(&mainENIRule) + + mockProcSys.EXPECT().Set("net/ipv4/conf/lo/rp_filter", "2").Return(nil) var vpcCIDRs []*string vpcCIDRs = []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} @@ -532,53 +582,6 @@ func TestIncrementIPv4Addr(t *testing.T) { } } -func TestSetupHostNetworkForUnprivilegedPod(t *testing.T) { - ctrl, mockNetLink, _, mockNS, mockIptables, mockProcSys := setup(t) - defer ctrl.Finish() - - ln := &linuxNetwork{ - useExternalSNAT: true, - nodePortSupportEnabled: true, - mainENIMark: defaultConnmark, - mtu: testMTU, - - netLink: mockNetLink, - ns: mockNS, - newIptables: func() (iptablesIface, error) { - return mockIptables, nil - }, - procSys: mockProcSys, - } - var vpcCIDRs []*string - setupNetLinkMocks(ctrl, mockNetLink) - setupProcSysMocks(mockProcSys, false) - - err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, loopback, &testENINetIP) - assert.NoError(t, err) -} - -func setupProcSysMocks(mockProcSys *mock_procsyswrapper.MockProcSys, shouldHaveWriteAccess bool) { - rpFilterPath := "net/ipv4/conf/lo/rp_filter" - - mockProcSys.EXPECT().IsPathWriteAccessible(rpFilterPath).Return(shouldHaveWriteAccess) - if shouldHaveWriteAccess { - mockProcSys.EXPECT().Set(rpFilterPath, "2").Return(nil) - } -} - -func setupNetLinkMocks(ctrl *gomock.Controller, mockNetLink *mock_netlinkwrapper.MockNetLink) { - mockPrimaryInterfaceLookup(ctrl, mockNetLink) - mockNetLink.EXPECT().LinkSetMTU(gomock.Any(), testMTU).Return(nil) - - var hostRule netlink.Rule - mockNetLink.EXPECT().NewRule().Return(&hostRule) - mockNetLink.EXPECT().RuleDel(&hostRule) - var mainENIRule netlink.Rule - mockNetLink.EXPECT().NewRule().Return(&mainENIRule) - mockNetLink.EXPECT().RuleDel(&mainENIRule) - mockNetLink.EXPECT().RuleAdd(&mainENIRule) -} - type mockIptables struct { // dataplaneState is a map from table name to chain name to slice of rulespecs dataplaneState map[string]map[string][][]string diff --git a/pkg/procsyswrapper/mocks/procsys_mocks.go b/pkg/procsyswrapper/mocks/procsys_mocks.go index 140e0470ca..ae81b9ecca 100644 --- a/pkg/procsyswrapper/mocks/procsys_mocks.go +++ b/pkg/procsyswrapper/mocks/procsys_mocks.go @@ -62,20 +62,6 @@ func (mr *MockProcSysMockRecorder) Get(arg0 interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockProcSys)(nil).Get), arg0) } -// IsPathWriteAccessible mocks base method -func (m *MockProcSys) IsPathWriteAccessible(arg0 string) bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IsPathWriteAccessible", arg0) - ret0, _ := ret[0].(bool) - return ret0 -} - -// IsPathWriteAccessible indicates an expected call of IsPathWriteAccessible -func (mr *MockProcSysMockRecorder) IsPathWriteAccessible(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsPathWriteAccessible", reflect.TypeOf((*MockProcSys)(nil).IsPathWriteAccessible), arg0) -} - // Set mocks base method func (m *MockProcSys) Set(arg0, arg1 string) error { m.ctrl.T.Helper() diff --git a/pkg/procsyswrapper/procsys.go b/pkg/procsyswrapper/procsys.go index 1f9406f43d..9a952b5d52 100644 --- a/pkg/procsyswrapper/procsys.go +++ b/pkg/procsyswrapper/procsys.go @@ -15,14 +15,11 @@ package procsyswrapper import ( "io/ioutil" - - "golang.org/x/sys/unix" ) type ProcSys interface { Get(key string) (string, error) Set(key, value string) error - IsPathWriteAccessible(key string) bool } type procSys struct { @@ -45,8 +42,3 @@ func (p *procSys) Get(key string) (string, error) { func (p *procSys) Set(key, value string) error { return ioutil.WriteFile(p.path(key), []byte(value), 0644) } - -// IsPathWriteAccessible verifies if aws-node pod can write to the specified path -func (p *procSys) IsPathWriteAccessible(key string) bool { - return unix.Access(p.path(key), unix.W_OK) != nil -}