From 29474be84292dfb7351dab5605e6ace556be213d Mon Sep 17 00:00:00 2001 From: Sri Saran Balaji Vellore Rajakumar Date: Wed, 1 Apr 2020 07:57:58 -0700 Subject: [PATCH] Update rp_filter only when /proc is accessible --- 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, 89 insertions(+), 64 deletions(-) diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index 59f5b7edf7..26956dd114 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -243,10 +243,16 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, primaryIntfRPFilter := "net/ipv4/conf/" + primaryIntf + "/rp_filter" const rpFilterLoose = "2" - 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) + 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) } } diff --git a/pkg/networkutils/network_test.go b/pkg/networkutils/network_test.go index a4488b025e..4b9010b00c 100644 --- a/pkg/networkutils/network_test.go +++ b/pkg/networkutils/network_test.go @@ -285,18 +285,8 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { procSys: mockProcSys, } - 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) + setupNetLinkMocks(ctrl, mockNetLink) + setupProcSysMocks(mockProcSys, true) var vpcCIDRs []*string @@ -363,18 +353,8 @@ func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { procSys: mockProcSys, } - 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) + setupNetLinkMocks(ctrl, mockNetLink) + setupProcSysMocks(mockProcSys, true) var vpcCIDRs []*string vpcCIDRs = []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} @@ -416,18 +396,8 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { }, procSys: mockProcSys, } - 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) + setupNetLinkMocks(ctrl, mockNetLink) + setupProcSysMocks(mockProcSys, true) 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 @@ -477,18 +447,8 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { }, procSys: mockProcSys, } - 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) + setupNetLinkMocks(ctrl, mockNetLink) + setupProcSysMocks(mockProcSys, true) _ = 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") @@ -537,18 +497,8 @@ func TestSetupHostNetworkMultipleCIDRs(t *testing.T) { }, procSys: mockProcSys, } - 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) + setupNetLinkMocks(ctrl, mockNetLink) + setupProcSysMocks(mockProcSys, true) var vpcCIDRs []*string vpcCIDRs = []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} @@ -582,6 +532,53 @@ 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 ae81b9ecca..140e0470ca 100644 --- a/pkg/procsyswrapper/mocks/procsys_mocks.go +++ b/pkg/procsyswrapper/mocks/procsys_mocks.go @@ -62,6 +62,20 @@ 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 9a952b5d52..1f9406f43d 100644 --- a/pkg/procsyswrapper/procsys.go +++ b/pkg/procsyswrapper/procsys.go @@ -15,11 +15,14 @@ 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 { @@ -42,3 +45,8 @@ 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 +}