From b30f2bac7d46996d58f5cf3a524e03023cca5135 Mon Sep 17 00:00:00 2001 From: Isaac Martinez Aceves Date: Thu, 27 Jun 2019 13:38:10 +0100 Subject: [PATCH 1/4] Allow additional CIDRs to be excluded from SNAT Introduces a mechanism to provide an exclusion list of CIDRs that should not be SNATed. Adds an environment variable `AWS_VPC_K8S_CNI_EXCLUDE_SNAT_CIDRS` to provide a comma separated list of ipv4 CIDRs to exclude from SNAT. The value of this environment variable will only be use when `AWS_VPC_K8S_CNI_EXTERNALSNAT` is false. The SNAT exclusion CIDRs will be used to: - Generate `iptable` rules to prevent SNATing for packets directed to the excluded CIDRs - Generate `ip rule` entries to route packets directed to the excluded CIDRs through the pod's `eni`. These configuration is done both at the `cni` plugin level via the `rpc_handler`, and the runtime `network` api. Given that the list of excluded CIDRs may vary by configuration it was necessary to include a mechanism to clean up stale SNAT rules. If a CIDR is removed from the exclusion list, the corresponding `iptable` rule will be removed as well, and the chain will be adjusted. Connects https://github.com/aws/amazon-vpc-cni-k8s/issues/469 Co-authored-by: @rewiko Co-authored-by: @yorg1st --- README.md | 6 + ipamd/rpc_handler.go | 10 +- ipamd/rpc_handler_test.go | 92 +++++++++ pkg/networkutils/mocks/network_mocks.go | 12 ++ pkg/networkutils/network.go | 152 +++++++++++++-- pkg/networkutils/network_test.go | 238 ++++++++++++++++++++++-- 6 files changed, 476 insertions(+), 34 deletions(-) create mode 100644 ipamd/rpc_handler_test.go diff --git a/README.md b/README.md index 5bdce029f1..9f2feab345 100644 --- a/README.md +++ b/README.md @@ -151,6 +151,12 @@ Disable (`none`) this functionality if you rely on sequential port allocation fo *Note*: Any options other than `none` will cause outbound connections to be assigned a source port that's not necessarily part of the ephemeral port range set at the OS level (/proc/sys/net/ipv4/ip_local_port_range). This is relevant for any customers that might have NACLs restricting traffic based on the port range found in ip_local_port_range +`AWS_VPC_K8S_CNI_EXCLUDE_SNAT_CIDRS` +Type: String +Default: empty +Specify a comma separated list of ipv4 CIDRs to exclude from SNAT. For every item in the list an `iptables` rule and off\-VPC +IP rule will be applied. If an item is not a valid ipv4 range it will be skipped. This should be used when `AWS_VPC_K8S_CNI_EXTERNALSNAT=false`. + `WARM_ENI_TARGET` Type: Integer Default: `1` diff --git a/ipamd/rpc_handler.go b/ipamd/rpc_handler.go index 4f4ede580b..ec65cc0926 100644 --- a/ipamd/rpc_handler.go +++ b/ipamd/rpc_handler.go @@ -54,12 +54,20 @@ func (s *server) AddNetwork(ctx context.Context, in *pb.AddNetworkRequest) (*pb. pbVPCcidrs = append(pbVPCcidrs, *cidr) } + useExternalSNAT := s.ipamContext.networkClient.UseExternalSNAT() + if !useExternalSNAT { + for _, cidr := range s.ipamContext.networkClient.GetExcludeSNATCIDRs() { + log.Debugf("CIDR SNAT Exclusion %s", cidr) + pbVPCcidrs = append(pbVPCcidrs, cidr) + } + } + resp := pb.AddNetworkReply{ Success: err == nil, IPv4Addr: addr, IPv4Subnet: "", DeviceNumber: int32(deviceNumber), - UseExternalSNAT: s.ipamContext.networkClient.UseExternalSNAT(), + UseExternalSNAT: useExternalSNAT, VPCcidrs: pbVPCcidrs, } diff --git a/ipamd/rpc_handler_test.go b/ipamd/rpc_handler_test.go new file mode 100644 index 0000000000..9c4e7c6bd8 --- /dev/null +++ b/ipamd/rpc_handler_test.go @@ -0,0 +1,92 @@ +// Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package ipamd + +import ( + "context" + "github.com/aws/amazon-vpc-cni-k8s/ipamd/datastore" + "github.com/aws/aws-sdk-go/aws" + "testing" + + pb "github.com/aws/amazon-vpc-cni-k8s/rpc" + + "github.com/stretchr/testify/assert" +) + +func TestServer_AddNetwork(t *testing.T) { + ctrl, mockAWS, mockK8S, mockDocker, mockNetwork, _ := setup(t) + defer ctrl.Finish() + + mockContext := &IPAMContext{ + awsClient: mockAWS, + k8sClient: mockK8S, + maxIPsPerENI: 14, + maxENI: 4, + warmENITarget: 1, + warmIPTarget: 3, + dockerClient: mockDocker, + networkClient: mockNetwork, + dataStore: datastore.NewDataStore(), + } + + rpcServer := server{ipamContext: mockContext} + + addNetworkRequest := &pb.AddNetworkRequest{ + Netns: "netns", + K8S_POD_NAME: "pod", + K8S_POD_NAMESPACE: "ns", + K8S_POD_INFRA_CONTAINER_ID: "cid", + IfName: "eni", + } + + vpcCIDRs := []*string{aws.String(vpcCIDR)} + testCases := []struct { + name string + useExternalSNAT bool + vpcCIDRs []*string + snatExclusionCIDRs []string + }{ + { + "VPC CIDRs", + true, + vpcCIDRs, + nil, + }, + { + "SNAT Exclusion CIDRs", + false, + vpcCIDRs, + []string{"10.12.0.0/16", "10.13.0.0/16"}, + }, + } + for _, tc := range testCases { + mockAWS.EXPECT().GetVPCIPv4CIDRs().Return(tc.vpcCIDRs) + mockNetwork.EXPECT().UseExternalSNAT().Return(tc.useExternalSNAT) + if !tc.useExternalSNAT { + mockNetwork.EXPECT().GetExcludeSNATCIDRs().Return(tc.snatExclusionCIDRs) + } + + addNetworkReply, err := rpcServer.AddNetwork(context.TODO(), addNetworkRequest) + assert.NoError(t, err, tc.name) + + assert.Equal(t, tc.useExternalSNAT, addNetworkReply.UseExternalSNAT, tc.name) + + var expectedCIDRs []string + for _, cidr := range tc.vpcCIDRs { + expectedCIDRs = append(expectedCIDRs, *cidr) + } + expectedCIDRs = append([]string{vpcCIDR}, tc.snatExclusionCIDRs...) + assert.Equal(t, expectedCIDRs, addNetworkReply.VPCcidrs, tc.name) + } +} diff --git a/pkg/networkutils/mocks/network_mocks.go b/pkg/networkutils/mocks/network_mocks.go index 2b6707951e..7c019ec6cc 100644 --- a/pkg/networkutils/mocks/network_mocks.go +++ b/pkg/networkutils/mocks/network_mocks.go @@ -133,3 +133,15 @@ func (m *MockNetworkAPIs) UseExternalSNAT() bool { func (mr *MockNetworkAPIsMockRecorder) UseExternalSNAT() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UseExternalSNAT", reflect.TypeOf((*MockNetworkAPIs)(nil).UseExternalSNAT)) } + +// GetExcludeSNATCIDRs mocks base method +func (m *MockNetworkAPIs) GetExcludeSNATCIDRs() []string { + ret := m.ctrl.Call(m, "GetExcludeSNATCIDRs") + ret0, _ := ret[0].([]string) + return ret0 +} + +// GetExcludeSNATCIDRs indicates an expected call of GetExcludeSNATCIDRs +func (mr *MockNetworkAPIsMockRecorder) GetExcludeSNATCIDRs() *gomock.Call { + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetExcludeSNATCIDRs", reflect.TypeOf((*MockNetworkAPIs)(nil).GetExcludeSNATCIDRs)) +} diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index 417b1ce3ce..d0c50b6cbc 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -15,11 +15,13 @@ package networkutils import ( "encoding/binary" + "encoding/csv" "fmt" "io" "math" "net" "os" + "reflect" "strconv" "strings" "syscall" @@ -56,6 +58,11 @@ const ( // be installed and will be removed if they are already installed. Defaults to false. envExternalSNAT = "AWS_VPC_K8S_CNI_EXTERNALSNAT" + // This environment is used to specify a comma separated list of ipv4 CIDRs to exclude from SNAT. An additional rule + // will be written to the iptables for each item. If an item is not an ipv4 range it will be skipped. + // Defaults to empty. + envExcludeSNATCIDRs = "AWS_VPC_K8S_CNI_EXCLUDE_SNAT_CIDRS" + // This environment is used to specify weather the SNAT rule added to iptables should randomize port // allocation for outgoing connections. If set to "hashrandom" the SNAT iptables rule will have the "--random" flag // added to it. Set it to "prng" if you want to use a pseudo random numbers, i.e. "--random-fully". @@ -101,6 +108,7 @@ type NetworkAPIs interface { // SetupENINetwork performs eni level network configuration SetupENINetwork(eniIP string, mac string, table int, subnetCIDR string) error UseExternalSNAT() bool + GetExcludeSNATCIDRs() []string GetRuleList() ([]netlink.Rule, error) GetRuleListBySrc(ruleList []netlink.Rule, src net.IPNet) ([]netlink.Rule, error) UpdateRuleListBySrc(ruleList []netlink.Rule, src net.IPNet, toCIDRs []string, toFlag bool) error @@ -109,6 +117,7 @@ type NetworkAPIs interface { type linuxNetwork struct { useExternalSNAT bool + excludeSNATCIDRs []string typeOfSNAT snatType nodePortSupportEnabled bool connmark uint32 @@ -145,6 +154,7 @@ const ( func New() NetworkAPIs { return &linuxNetwork{ useExternalSNAT: useExternalSNAT(), + excludeSNATCIDRs: getExcludeSNATCIDRs(), typeOfSNAT: typeOfSNAT(), nodePortSupportEnabled: nodePortSupportEnabled(), mainENIMark: getConnmark(), @@ -261,12 +271,30 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, return errors.Wrap(err, "host network setup: failed to create iptables") } - // build IPTABLES chain for SNAT of non-VPC outbound traffic + type snatCIDR struct { + cidr string + isExclusion bool + } + var allCIDRs []snatCIDR + for _, cidr := range vpcCIDRs { + allCIDRs = append(allCIDRs, snatCIDR{cidr: *cidr, isExclusion: false}) + } + for _, cidr := range n.excludeSNATCIDRs { + allCIDRs = append(allCIDRs, snatCIDR{cidr: cidr, isExclusion: true}) + } + + // if excludeSNATCIDRs or vpcCIDRs have changed they need to be cleared + snatStaleRulesToCheck, err := listCurrentSNATRules(ipt) + if err != nil { + return errors.Wrapf(err, "host network setup: failed to get SNAT chain rules to clear") + } + + // build IPTABLES chain for SNAT of non-VPC outbound traffic and excluded CIDRs var chains []string - for i := 0; i <= len(vpcCIDRs); i++ { + for i := 0; i <= len(allCIDRs); i++ { chain := fmt.Sprintf("AWS-SNAT-CHAIN-%d", i) log.Debugf("Setup Host Network: iptables -N %s -t nat", chain) - if err = ipt.NewChain("nat", chain); err != nil && !containChainExistErr(err) { + if err := ipt.NewChain("nat", chain); err != nil && !containChainExistErr(err) { log.Errorf("ipt.NewChain error for chain [%s]: %v", chain, err) return errors.Wrapf(err, "host network setup: failed to add chain") } @@ -274,9 +302,8 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, } // build SNAT rules for outbound non-VPC traffic - log.Debugf("Setup Host Network: iptables -A POSTROUTING -m comment --comment \"AWS SNAT CHAIN\" -j AWS-SNAT-CHAIN-0") - var iptableRules []iptablesRule + log.Debugf("Setup Host Network: iptables -A POSTROUTING -m comment --comment \"AWS SNAT CHAN\" -j AWS-SNAT-CHAIN-0") iptableRules = append(iptableRules, iptablesRule{ name: "first SNAT rules for non-VPC outbound traffic", shouldExist: !n.useExternalSNAT, @@ -286,12 +313,15 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0", }}) - for i, cidr := range vpcCIDRs { + for i, cidr:= range allCIDRs { curChain := chains[i] - nextChain := chains[i+1] curName := fmt.Sprintf("[%d] AWS-SNAT-CHAIN", i) - - log.Debugf("Setup Host Network: iptables -A %s ! -d %s -t nat -j %s", curChain, *cidr, nextChain) + nextChain := chains[i+1] + comment := "AWS SNAT CHAN" + if cidr.isExclusion { + comment += " EXCLUSION" + } + log.Debugf("Setup Host Network: iptables -A %s ! -d %s -t nat -j %s", curChain, cidr, nextChain) iptableRules = append(iptableRules, iptablesRule{ name: curName, @@ -299,11 +329,10 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, table: "nat", chain: curChain, rule: []string{ - "!", "-d", *cidr, "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", nextChain, + "!", "-d", cidr.cidr, "-m", "comment", "--comment", comment, "-j", nextChain, }}) } - lastChain := chains[len(chains)-1] // Prepare the Desired Rule for SNAT Rule snatRule := []string{"-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", @@ -321,6 +350,7 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, } } + lastChain := chains[len(chains)-1] iptableRules = append(iptableRules, iptablesRule{ name: "last SNAT rule for non-VPC outbound traffic", shouldExist: !n.useExternalSNAT, @@ -329,6 +359,24 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, rule: snatRule, }) + var snatStaleRulesToClear []iptablesRule + log.Debugf("Setup Host Network: synchronising SNAT stale rules") + for _, staleRule := range snatStaleRulesToCheck { + keepRule := false + for _, newRule := range iptableRules { + if staleRule.chain == newRule.chain && reflect.DeepEqual(newRule.rule, staleRule.rule) { + log.Debugf("Setup Host Network: active rule found: %s", staleRule) + keepRule = true + break + } + } + if !keepRule { + log.Debugf("Setup Host Network: stale rule found: %s", staleRule) + snatStaleRulesToClear = append(snatStaleRulesToClear, staleRule) + } + } + + iptableRules = append(iptableRules, snatStaleRulesToClear...) log.Debugf("iptableRules: %v", iptableRules) iptableRules = append(iptableRules, iptablesRule{ @@ -393,6 +441,42 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, return nil } +func listCurrentSNATRules(ipt iptablesIface) ([]iptablesRule, error) { + var toClear []iptablesRule + log.Debug("Setup Host Network: loading existing iptables nat SNAT exclusion rules") + + existingChains, err := ipt.ListChains("nat") + if err != nil { + return nil, errors.Wrap(err, "host network setup: failed to list iptables nat chains") + } + for _, chain := range existingChains { + if !strings.HasPrefix(chain, "AWS-SNAT-CHAIN") { + continue + } + rules, err := ipt.List("nat", chain) + if err != nil { + return nil, errors.Wrap(err, fmt.Sprintf("host network setup: failed to list iptables nat chain %s", chain)) + } + for i, rule := range rules { + r := csv.NewReader(strings.NewReader(rule)) + r.Comma = ' ' + ruleSpec, err := r.Read() + if err != nil { + return nil, errors.Wrap(err, fmt.Sprintf("host network setup: failed to parse iptables nat chain %s rule %s", chain, rule)) + } + log.Debugf("host network setup: found potentially stale SNAT rule for chain %s: %v", chain, ruleSpec) + toClear = append(toClear, iptablesRule{ + name: fmt.Sprintf("[%d] %s", i, chain), + shouldExist: false, + table: "nat", + chain: chain, + rule: ruleSpec[2:], //drop action and chain name + }) + } + } + return toClear, nil +} + func containChainExistErr(err error) bool { return strings.Contains(err.Error(), "Chain already exists") } @@ -432,10 +516,11 @@ func containsNoSuchRule(err error) bool { // GetConfigForDebug returns the active values of the configuration env vars (for debugging purposes). func GetConfigForDebug() map[string]interface{} { return map[string]interface{}{ - envExternalSNAT: useExternalSNAT(), - envNodePortSupport: nodePortSupportEnabled(), - envConnmark: getConnmark(), - envRandomizeSNAT: typeOfSNAT(), + envExternalSNAT: useExternalSNAT(), + envExcludeSNATCIDRs: getExcludeSNATCIDRs(), + envNodePortSupport: nodePortSupportEnabled(), + envConnmark: getConnmark(), + envRandomizeSNAT: typeOfSNAT(), } } @@ -450,6 +535,33 @@ func useExternalSNAT() bool { return getBoolEnvVar(envExternalSNAT, false) } +// GetExcludeSNATCIDRs returns a list of cidrs that should be excluded from SNAT if UseExternalSNAT is false, +// otherwise it returns an empty list. +func (n *linuxNetwork) GetExcludeSNATCIDRs() []string { + return getExcludeSNATCIDRs() +} + +func getExcludeSNATCIDRs() []string { + if useExternalSNAT() { + return nil + } + + excludeCIDRs := os.Getenv(envExcludeSNATCIDRs) + if excludeCIDRs == "" { + return nil + } + var cidrs []string + for _, excludeCIDR := range strings.Split(excludeCIDRs, ",") { + _, parseCIDR, err := net.ParseCIDR(excludeCIDR) + if err != nil { + log.Errorf("getExcludeSNATCIDRs : ignoring %v is not a valid IPv4 CIDR", excludeCIDR) + } else { + cidrs = append(cidrs, parseCIDR.String()) + } + } + return cidrs +} + func typeOfSNAT() snatType { defaultValue := randomHashSNAT defaultString := "hashrandom" @@ -749,8 +861,9 @@ func (n *linuxNetwork) DeleteRuleListBySrc(src net.IPNet) error { } // UpdateRuleListBySrc modify IP rules that have a matching source IP -func (n *linuxNetwork) UpdateRuleListBySrc(ruleList []netlink.Rule, src net.IPNet, toCIDRs []string, useExternalSNAT bool) error { - log.Infof("Update Rule List[%v] for source[%v] with toCIDRs[%v], useExternalSNAT[%v]", ruleList, src, toCIDRs, useExternalSNAT) +func (n *linuxNetwork) UpdateRuleListBySrc(ruleList []netlink.Rule, src net.IPNet, toCIDRs []string, requiresSNAT bool) error { + log.Infof("Update Rule List[%v] for source[%v] with toCIDRs[%v], excludeSNATCIDRs[%v], requiresSNAT[%v]", + ruleList, src, toCIDRs, n.excludeSNATCIDRs, requiresSNAT) srcRuleList, err := n.GetRuleListBySrc(ruleList, src) if err != nil { @@ -778,8 +891,9 @@ func (n *linuxNetwork) UpdateRuleListBySrc(ruleList []netlink.Rule, src net.IPNe return nil } - if useExternalSNAT { - for _, cidr := range toCIDRs { + if requiresSNAT { + allCIDRs := append(toCIDRs, n.excludeSNATCIDRs...) + for _, cidr := range allCIDRs { podRule := n.netLink.NewRule() _, podRule.Dst, _ = net.ParseCIDR(cidr) podRule.Src = &src diff --git a/pkg/networkutils/network_test.go b/pkg/networkutils/network_test.go index 209b89a3ae..4760136f8c 100644 --- a/pkg/networkutils/network_test.go +++ b/pkg/networkutils/network_test.go @@ -19,6 +19,7 @@ import ( "net" "os" "reflect" + "strings" "testing" "time" @@ -188,20 +189,22 @@ func TestUpdateRuleListBySrc(t *testing.T) { Table: testTable, } testCases := []struct { - name string - oldRule netlink.Rule - toFlag bool - toCIDRs []string - ruleList []netlink.Rule - newRules []netlink.Rule - expDst []*net.IPNet - expTable []int + name string + oldRule netlink.Rule + requiresSNAT bool + toCIDRs []string + snatExclusionCIDRs []string + ruleList []netlink.Rule + newRules []netlink.Rule + expDst []*net.IPNet + expTable []int }{ { "multiple destinations", origRule, true, []string{"10.10.0.0/16", "10.11.0.0/16"}, + nil, []netlink.Rule{origRule}, make([]netlink.Rule, 2), make([]*net.IPNet, 2), @@ -212,23 +215,37 @@ func TestUpdateRuleListBySrc(t *testing.T) { origRule, false, []string{""}, + nil, []netlink.Rule{origRule}, make([]netlink.Rule, 1), make([]*net.IPNet, 1), []int{origRule.Table}, }, + { + "SNAT exclusions", + origRule, + true, + []string{"10.10.0.0/16", "10.11.0.0/16"}, + []string{"10.12.0.0/16", "10.13.0.0/16"}, + []netlink.Rule{origRule}, + make([]netlink.Rule, 4), + make([]*net.IPNet, 4), + []int{origRule.Table, origRule.Table, origRule.Table, origRule.Table}, + }, } for _, tc := range testCases { + ln.excludeSNATCIDRs = tc.snatExclusionCIDRs var newRuleSize int - if tc.toFlag { - newRuleSize = len(tc.toCIDRs) + if tc.requiresSNAT { + newRuleSize = len(tc.toCIDRs) + len(tc.snatExclusionCIDRs) } else { newRuleSize = 1 } + allCIDRs := append(tc.toCIDRs, tc.snatExclusionCIDRs...) for i := 0; i < newRuleSize; i++ { - _, tc.expDst[i], _ = net.ParseCIDR(tc.toCIDRs[i]) + _, tc.expDst[i], _ = net.ParseCIDR(allCIDRs[i]) } mockNetLink.EXPECT().RuleDel(&tc.oldRule) @@ -238,7 +255,7 @@ func TestUpdateRuleListBySrc(t *testing.T) { mockNetLink.EXPECT().RuleAdd(&tc.newRules[i]) } - err := ln.UpdateRuleListBySrc(tc.ruleList, *testENINetIPNet, tc.toCIDRs, tc.toFlag) + err := ln.UpdateRuleListBySrc(tc.ruleList, *testENINetIPNet, tc.toCIDRs, tc.requiresSNAT) assert.NoError(t, err) for i := 0; i < newRuleSize; i++ { @@ -305,6 +322,183 @@ func TestSetupHostNetworkNodePortEnabled(t *testing.T) { assert.Equal(t, mockFile{closed: true, data: "2"}, mockRPFilter) } +func TestLoadExcludeSNATCIDRsFromEnv(t *testing.T) { + _ = os.Setenv(envExternalSNAT, "false") + _ = os.Setenv(envExcludeSNATCIDRs, "10.12.0.0/16,10.13.0.0/16") + + expected := []string{"10.12.0.0/16", "10.13.0.0/16"} + assert.Equal(t, getExcludeSNATCIDRs(), expected) +} + +func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { + ctrl, mockNetLink, _, mockNS, mockIptables := setup(t) + defer ctrl.Finish() + + var mockRPFilter mockFile + ln := &linuxNetwork{ + useExternalSNAT: false, + excludeSNATCIDRs: []string{"10.12.0.0/16", "10.13.0.0/16"}, + nodePortSupportEnabled: true, + mainENIMark: defaultConnmark, + + netLink: mockNetLink, + ns: mockNS, + newIptables: func() (iptablesIface, error) { + return mockIptables, nil + }, + openFile: func(name string, flag int, perm os.FileMode) (stringWriteCloser, error) { + return &mockRPFilter, 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) + + 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) + assert.NoError(t, err) + assert.Equal(t, + map[string]map[string][][]string{ + "nat": { + "AWS-SNAT-CHAIN-0": [][]string{{"!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-1"}}, + "AWS-SNAT-CHAIN-1": [][]string{{"!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2"}}, + "AWS-SNAT-CHAIN-2": [][]string{{"!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3"}}, + "AWS-SNAT-CHAIN-3": [][]string{{"!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4"}}, + "AWS-SNAT-CHAIN-4": [][]string{{"-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20"}}, + "POSTROUTING": [][]string{{"-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0"}}}, + "mangle": { + "PREROUTING": [][]string{ + {"-m", "comment", "--comment", "AWS, primary ENI", "-i", "lo", "-m", "addrtype", "--dst-type", "LOCAL", "--limit-iface-in", "-j", "CONNMARK", "--set-mark", "0x80/0x80"}, + {"-m", "comment", "--comment", "AWS, primary ENI", "-i", "eni+", "-j", "CONNMARK", "--restore-mark", "--mask", "0x80"}, + }, + }, + }, mockIptables.dataplaneState) +} + +func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { + ctrl, mockNetLink, _, mockNS, mockIptables := setup(t) + defer ctrl.Finish() + + var mockRPFilter mockFile + ln := &linuxNetwork{ + useExternalSNAT: false, + excludeSNATCIDRs: nil, + nodePortSupportEnabled: true, + mainENIMark: defaultConnmark, + + netLink: mockNetLink, + ns: mockNS, + newIptables: func() (iptablesIface, error) { + return mockIptables, nil + }, + openFile: func(name string, flag int, perm os.FileMode) (stringWriteCloser, error) { + return &mockRPFilter, 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) + + 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") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-1", "!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-2", "!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-3", "!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-4", "-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20") + _ = mockIptables.NewChain("nat", "AWS-SNAT-CHAIN-5") + _ = mockIptables.Append("nat", "POSTROUTING", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0") + + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) + assert.NoError(t, err) + + assert.Equal(t, + map[string]map[string][][]string{ + "nat": { + "AWS-SNAT-CHAIN-0": [][]string{{"!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-1"}}, + "AWS-SNAT-CHAIN-1": [][]string{{"!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2"}}, + "AWS-SNAT-CHAIN-2": [][]string{{"-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20"}}, + "AWS-SNAT-CHAIN-3": [][]string{}, + "AWS-SNAT-CHAIN-4": [][]string{}, + "POSTROUTING": [][]string{{"-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0"}}}, + "mangle": { + "PREROUTING": [][]string{ + {"-m", "comment", "--comment", "AWS, primary ENI", "-i", "lo", "-m", "addrtype", "--dst-type", "LOCAL", "--limit-iface-in", "-j", "CONNMARK", "--set-mark", "0x80/0x80"}, + {"-m", "comment", "--comment", "AWS, primary ENI", "-i", "eni+", "-j", "CONNMARK", "--restore-mark", "--mask", "0x80"}, + }, + }, + }, mockIptables.dataplaneState) +} + +func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { + ctrl, mockNetLink, _, mockNS, mockIptables := setup(t) + defer ctrl.Finish() + + var mockRPFilter mockFile + ln := &linuxNetwork{ + useExternalSNAT: false, + excludeSNATCIDRs: []string{"10.12.0.0/16", "10.13.0.0/16"}, + nodePortSupportEnabled: true, + mainENIMark: defaultConnmark, + + netLink: mockNetLink, + ns: mockNS, + newIptables: func() (iptablesIface, error) { + return mockIptables, nil + }, + openFile: func(name string, flag int, perm os.FileMode) (stringWriteCloser, error) { + return &mockRPFilter, 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) + + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-0", "!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-1") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-1", "!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-2", "!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-3", "!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-4", "-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20") + _ = mockIptables.Append("nat", "POSTROUTING", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0") + + // remove exclusions + vpcCIDRs := []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} + err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) + assert.NoError(t, err) + + assert.Equal(t, + map[string]map[string][][]string{ + "nat": { + "AWS-SNAT-CHAIN-0": [][]string{{"!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-1"}}, + "AWS-SNAT-CHAIN-1": [][]string{{"!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2"}}, + "AWS-SNAT-CHAIN-2": [][]string{{"!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3"}}, + "AWS-SNAT-CHAIN-3": [][]string{{"!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4"}}, + "AWS-SNAT-CHAIN-4": [][]string{{"-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20"}}, + "POSTROUTING": [][]string{{"-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0"}}}, + "mangle": { + "PREROUTING": [][]string{ + {"-m", "comment", "--comment", "AWS, primary ENI", "-i", "lo", "-m", "addrtype", "--dst-type", "LOCAL", "--limit-iface-in", "-j", "CONNMARK", "--set-mark", "0x80/0x80"}, + {"-m", "comment", "--comment", "AWS, primary ENI", "-i", "eni+", "-j", "CONNMARK", "--restore-mark", "--mask", "0x80"}, + }, + }, + }, mockIptables.dataplaneState) +} + func TestSetupHostNetworkMultipleCIDRs(t *testing.T) { ctrl, mockNetLink, _, mockNS, mockIptables := setup(t) defer ctrl.Finish() @@ -415,7 +609,19 @@ func (ipt *mockIptables) Delete(table, chainName string, rulespec ...string) err } func (ipt *mockIptables) List(table, chain string) ([]string, error) { - return nil, nil + var chains []string + chainContents := ipt.dataplaneState[table][chain] + for _, ruleSpec := range chainContents { + sanitizedRuleSpec := []string{"-A", chain} + for _, item := range ruleSpec { + if strings.Contains(item, " ") { + item = fmt.Sprintf("%q", item) + } + sanitizedRuleSpec = append(sanitizedRuleSpec, item) + } + chains = append(chains, strings.Join(sanitizedRuleSpec, " ")) + } + return chains, nil } @@ -432,7 +638,11 @@ func (ipt *mockIptables) DeleteChain(table, chain string) error { } func (ipt *mockIptables) ListChains(table string) ([]string, error) { - return nil, nil + var chains []string + for chain := range ipt.dataplaneState[table] { + chains = append(chains, chain) + } + return chains, nil } func (ipt *mockIptables) HasRandomFully() bool { From 773c7cb1682969b0ff71f1de05dfb265914e9acf Mon Sep 17 00:00:00 2001 From: Isaac Martinez Aceves Date: Thu, 11 Jul 2019 09:41:45 +0100 Subject: [PATCH 2/4] Address PR review comments --- README.md | 2 +- pkg/networkutils/network.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 9f2feab345..205250c19f 100644 --- a/README.md +++ b/README.md @@ -154,7 +154,7 @@ Disable (`none`) this functionality if you rely on sequential port allocation fo `AWS_VPC_K8S_CNI_EXCLUDE_SNAT_CIDRS` Type: String Default: empty -Specify a comma separated list of ipv4 CIDRs to exclude from SNAT. For every item in the list an `iptables` rule and off\-VPC +Specify a comma separated list of IPv4 CIDRs to exclude from SNAT. For every item in the list an `iptables` rule and off\-VPC IP rule will be applied. If an item is not a valid ipv4 range it will be skipped. This should be used when `AWS_VPC_K8S_CNI_EXTERNALSNAT=false`. `WARM_ENI_TARGET` diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index d0c50b6cbc..f038f73080 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -58,7 +58,7 @@ const ( // be installed and will be removed if they are already installed. Defaults to false. envExternalSNAT = "AWS_VPC_K8S_CNI_EXTERNALSNAT" - // This environment is used to specify a comma separated list of ipv4 CIDRs to exclude from SNAT. An additional rule + // This environment is used to specify a comma separated list of ipv4 CIDRs to exclude from SNAT. An additional rule // will be written to the iptables for each item. If an item is not an ipv4 range it will be skipped. // Defaults to empty. envExcludeSNATCIDRs = "AWS_VPC_K8S_CNI_EXCLUDE_SNAT_CIDRS" @@ -313,7 +313,7 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0", }}) - for i, cidr:= range allCIDRs { + for i, cidr := range allCIDRs { curChain := chains[i] curName := fmt.Sprintf("[%d] AWS-SNAT-CHAIN", i) nextChain := chains[i+1] @@ -467,7 +467,7 @@ func listCurrentSNATRules(ipt iptablesIface) ([]iptablesRule, error) { log.Debugf("host network setup: found potentially stale SNAT rule for chain %s: %v", chain, ruleSpec) toClear = append(toClear, iptablesRule{ name: fmt.Sprintf("[%d] %s", i, chain), - shouldExist: false, + shouldExist: false, // To trigger ipt.Delete for stale rules table: "nat", chain: chain, rule: ruleSpec[2:], //drop action and chain name From 783dfcb849e9bdf5fd5921cc170fad5248e6f6bf Mon Sep 17 00:00:00 2001 From: Isaac Martinez Aceves Date: Thu, 11 Jul 2019 09:58:34 +0100 Subject: [PATCH 3/4] Fix typo in SNAT chain rules' comment --- pkg/networkutils/network.go | 6 ++--- pkg/networkutils/network_test.go | 46 ++++++++++++++++---------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index f038f73080..0bc47d898d 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -303,21 +303,21 @@ func (n *linuxNetwork) SetupHostNetwork(vpcCIDR *net.IPNet, vpcCIDRs []*string, // build SNAT rules for outbound non-VPC traffic var iptableRules []iptablesRule - log.Debugf("Setup Host Network: iptables -A POSTROUTING -m comment --comment \"AWS SNAT CHAN\" -j AWS-SNAT-CHAIN-0") + log.Debugf("Setup Host Network: iptables -A POSTROUTING -m comment --comment \"AWS SNAT CHAIN\" -j AWS-SNAT-CHAIN-0") iptableRules = append(iptableRules, iptablesRule{ name: "first SNAT rules for non-VPC outbound traffic", shouldExist: !n.useExternalSNAT, table: "nat", chain: "POSTROUTING", rule: []string{ - "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0", + "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-0", }}) for i, cidr := range allCIDRs { curChain := chains[i] curName := fmt.Sprintf("[%d] AWS-SNAT-CHAIN", i) nextChain := chains[i+1] - comment := "AWS SNAT CHAN" + comment := "AWS SNAT CHAIN" if cidr.isExclusion { comment += " EXCLUSION" } diff --git a/pkg/networkutils/network_test.go b/pkg/networkutils/network_test.go index 4760136f8c..e0663cdb40 100644 --- a/pkg/networkutils/network_test.go +++ b/pkg/networkutils/network_test.go @@ -366,12 +366,12 @@ func TestSetupHostNetworkWithExcludeSNATCIDRs(t *testing.T) { assert.Equal(t, map[string]map[string][][]string{ "nat": { - "AWS-SNAT-CHAIN-0": [][]string{{"!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-1"}}, - "AWS-SNAT-CHAIN-1": [][]string{{"!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2"}}, - "AWS-SNAT-CHAIN-2": [][]string{{"!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3"}}, - "AWS-SNAT-CHAIN-3": [][]string{{"!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4"}}, + "AWS-SNAT-CHAIN-0": [][]string{{"!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-1"}}, + "AWS-SNAT-CHAIN-1": [][]string{{"!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-2"}}, + "AWS-SNAT-CHAIN-2": [][]string{{"!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3"}}, + "AWS-SNAT-CHAIN-3": [][]string{{"!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4"}}, "AWS-SNAT-CHAIN-4": [][]string{{"-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20"}}, - "POSTROUTING": [][]string{{"-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0"}}}, + "POSTROUTING": [][]string{{"-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-0"}}}, "mangle": { "PREROUTING": [][]string{ {"-m", "comment", "--comment", "AWS, primary ENI", "-i", "lo", "-m", "addrtype", "--dst-type", "LOCAL", "--limit-iface-in", "-j", "CONNMARK", "--set-mark", "0x80/0x80"}, @@ -411,13 +411,13 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { mockNetLink.EXPECT().RuleAdd(&mainENIRule) 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") - _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-1", "!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2") - _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-2", "!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3") - _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-3", "!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4") + _ = 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 + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-1", "!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-2") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-2", "!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-3", "!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4") _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-4", "-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20") _ = mockIptables.NewChain("nat", "AWS-SNAT-CHAIN-5") - _ = mockIptables.Append("nat", "POSTROUTING", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0") + _ = mockIptables.Append("nat", "POSTROUTING", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-0") err := ln.SetupHostNetwork(testENINetIPNet, vpcCIDRs, "", &testENINetIP) assert.NoError(t, err) @@ -425,12 +425,12 @@ func TestSetupHostNetworkCleansUpStaleSNATRules(t *testing.T) { assert.Equal(t, map[string]map[string][][]string{ "nat": { - "AWS-SNAT-CHAIN-0": [][]string{{"!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-1"}}, - "AWS-SNAT-CHAIN-1": [][]string{{"!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2"}}, + "AWS-SNAT-CHAIN-0": [][]string{{"!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-1"}}, + "AWS-SNAT-CHAIN-1": [][]string{{"!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-2"}}, "AWS-SNAT-CHAIN-2": [][]string{{"-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20"}}, "AWS-SNAT-CHAIN-3": [][]string{}, "AWS-SNAT-CHAIN-4": [][]string{}, - "POSTROUTING": [][]string{{"-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0"}}}, + "POSTROUTING": [][]string{{"-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-0"}}}, "mangle": { "PREROUTING": [][]string{ {"-m", "comment", "--comment", "AWS, primary ENI", "-i", "lo", "-m", "addrtype", "--dst-type", "LOCAL", "--limit-iface-in", "-j", "CONNMARK", "--set-mark", "0x80/0x80"}, @@ -469,12 +469,12 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { mockNetLink.EXPECT().RuleDel(&mainENIRule) mockNetLink.EXPECT().RuleAdd(&mainENIRule) - _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-0", "!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-1") - _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-1", "!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2") - _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-2", "!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3") - _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-3", "!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4") + _ = 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") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-2", "!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3") + _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-3", "!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4") _ = mockIptables.Append("nat", "AWS-SNAT-CHAIN-4", "-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20") - _ = mockIptables.Append("nat", "POSTROUTING", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0") + _ = mockIptables.Append("nat", "POSTROUTING", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-0") // remove exclusions vpcCIDRs := []*string{aws.String("10.10.0.0/16"), aws.String("10.11.0.0/16")} @@ -484,12 +484,12 @@ func TestSetupHostNetworkExcludedSNATCIDRsIdempotent(t *testing.T) { assert.Equal(t, map[string]map[string][][]string{ "nat": { - "AWS-SNAT-CHAIN-0": [][]string{{"!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-1"}}, - "AWS-SNAT-CHAIN-1": [][]string{{"!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-2"}}, - "AWS-SNAT-CHAIN-2": [][]string{{"!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3"}}, - "AWS-SNAT-CHAIN-3": [][]string{{"!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4"}}, + "AWS-SNAT-CHAIN-0": [][]string{{"!", "-d", "10.10.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-1"}}, + "AWS-SNAT-CHAIN-1": [][]string{{"!", "-d", "10.11.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-2"}}, + "AWS-SNAT-CHAIN-2": [][]string{{"!", "-d", "10.12.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN EXCLUSION", "-j", "AWS-SNAT-CHAIN-3"}}, + "AWS-SNAT-CHAIN-3": [][]string{{"!", "-d", "10.13.0.0/16", "-m", "comment", "--comment", "AWS SNAT CHAIN EXCLUSION", "-j", "AWS-SNAT-CHAIN-4"}}, "AWS-SNAT-CHAIN-4": [][]string{{"-m", "comment", "--comment", "AWS, SNAT", "-m", "addrtype", "!", "--dst-type", "LOCAL", "-j", "SNAT", "--to-source", "10.10.10.20"}}, - "POSTROUTING": [][]string{{"-m", "comment", "--comment", "AWS SNAT CHAN", "-j", "AWS-SNAT-CHAIN-0"}}}, + "POSTROUTING": [][]string{{"-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-0"}}}, "mangle": { "PREROUTING": [][]string{ {"-m", "comment", "--comment", "AWS, primary ENI", "-i", "lo", "-m", "addrtype", "--dst-type", "LOCAL", "--limit-iface-in", "-j", "CONNMARK", "--set-mark", "0x80/0x80"}, From 5355b9052f2a986eabfd14fa3d7ba1eec399015d Mon Sep 17 00:00:00 2001 From: Isaac Martinez Aceves Date: Thu, 11 Jul 2019 09:44:47 +0100 Subject: [PATCH 4/4] Add formatting targets to Makefile `format` applies formatting to the project's go files. `check-format` checks that no files require formatting; if they do it will fail the command. It adds the `check-format` target to the travis build so that CI fails if files are not properly formatted. --- .travis.yml | 1 + Makefile | 18 ++++++++++++- ipamd/ipamd.go | 4 +-- ipamd/ipamd_test.go | 22 ++++++++-------- ipamd/rpc_handler_test.go | 3 ++- pkg/awsutils/awsutils_test.go | 4 +-- .../ec2metadatawrapper_test.go | 3 ++- pkg/k8sapi/discovery.go | 2 +- pkg/netlinkwrapper/netlink.go | 3 ++- pkg/networkutils/network_test.go | 4 +-- pkg/utils/utils_test.go | 2 +- plugins/routed-eni/cni_test.go | 26 +++++++++---------- plugins/routed-eni/driver/driver_test.go | 4 +-- 13 files changed, 58 insertions(+), 38 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3168bc4dcc..25cd2b6dbe 100644 --- a/.travis.yml +++ b/.travis.yml @@ -23,6 +23,7 @@ install: # Tests to run script: + - GO111MODULE=on make check-format - GO111MODULE=on make build-linux - GO111MODULE=on make lint - GO111MODULE=on make vet diff --git a/Makefile b/Makefile index 1c0cb07724..22a3bd5c61 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,7 @@ # language governing permissions and limitations under the License. # -.PHONY: all build-linux clean docker docker-build lint unit-test vet download-portmap build-docker-test build-metrics docker-metrics metrics-unit-test docker-metrics-test docker-vet +.PHONY: all build-linux clean format check-format docker docker-build lint unit-test vet download-portmap build-docker-test build-metrics docker-metrics metrics-unit-test docker-metrics-test docker-vet IMAGE ?= amazon/amazon-k8s-cni VERSION ?= $(shell git describe --tags --always --dirty) @@ -123,3 +123,19 @@ clean: rm -f aws-cni rm -f cni-metrics-helper/cni-metrics-helper rm -f portmap + +files := $(shell find . -path ./vendor -prune -or -not -name 'mock_publisher.go' -name '*.go' -print) +unformatted = $(shell goimports -l $(files)) + +format : + @echo "== format" + @goimports -w $(files) + @sync + +check-format : + @echo "== check formatting" +ifneq "$(unformatted)" "" + @echo "needs formatting: $(unformatted)" + @echo "run 'make format'" + @exit 1 +endif \ No newline at end of file diff --git a/ipamd/ipamd.go b/ipamd/ipamd.go index 8a22222eab..ac9bb471c8 100644 --- a/ipamd/ipamd.go +++ b/ipamd/ipamd.go @@ -672,7 +672,7 @@ func (c *IPAMContext) tryAssignIPs() (increasedPool bool, err error) { log.Debugf("Found ENI %s that has less than the maximum number of IP addresses allocated: cur=%d, max=%d", eni.ID, currentNumberOfAllocatedIPs, c.maxIPsPerENI) // Try to allocate all available IPs for this ENI // TODO: Retry with back-off, trying with half the number of IPs each time - err = c.awsClient.AllocIPAddresses(eni.ID, c.maxIPsPerENI- currentNumberOfAllocatedIPs) + err = c.awsClient.AllocIPAddresses(eni.ID, c.maxIPsPerENI-currentNumberOfAllocatedIPs) if err != nil { log.Warnf("failed to allocate all available IP addresses on ENI %s, err: %v", eni.ID, err) // Try to just get one more IP @@ -692,7 +692,7 @@ func (c *IPAMContext) tryAssignIPs() (increasedPool bool, err error) { return true, nil } return false, nil -} +} // setupENI does following: // 1) add ENI to datastore diff --git a/ipamd/ipamd_test.go b/ipamd/ipamd_test.go index c9450b74f8..4e38d28f49 100644 --- a/ipamd/ipamd_test.go +++ b/ipamd/ipamd_test.go @@ -24,13 +24,13 @@ import ( "github.com/aws/amazon-vpc-cni-k8s/ipamd/datastore" "github.com/aws/amazon-vpc-cni-k8s/pkg/apis/crd/v1alpha1" "github.com/aws/amazon-vpc-cni-k8s/pkg/awsutils" - "github.com/aws/amazon-vpc-cni-k8s/pkg/awsutils/mocks" + mock_awsutils "github.com/aws/amazon-vpc-cni-k8s/pkg/awsutils/mocks" "github.com/aws/amazon-vpc-cni-k8s/pkg/docker" - "github.com/aws/amazon-vpc-cni-k8s/pkg/docker/mocks" - "github.com/aws/amazon-vpc-cni-k8s/pkg/eniconfig/mocks" + mock_docker "github.com/aws/amazon-vpc-cni-k8s/pkg/docker/mocks" + mock_eniconfig "github.com/aws/amazon-vpc-cni-k8s/pkg/eniconfig/mocks" "github.com/aws/amazon-vpc-cni-k8s/pkg/k8sapi" - "github.com/aws/amazon-vpc-cni-k8s/pkg/k8sapi/mocks" - "github.com/aws/amazon-vpc-cni-k8s/pkg/networkutils/mocks" + mock_k8sapi "github.com/aws/amazon-vpc-cni-k8s/pkg/k8sapi/mocks" + mock_networkutils "github.com/aws/amazon-vpc-cni-k8s/pkg/networkutils/mocks" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" @@ -237,9 +237,9 @@ func testIncreaseIPPool(t *testing.T, useENIConfig bool) { mockAWS.EXPECT().DescribeENI(eni2).Return( []*ec2.NetworkInterfacePrivateIpAddress{ - { PrivateIpAddress: &testAddr11, Primary: &primary }, - { PrivateIpAddress: &testAddr12, Primary: ¬Primary }, - { PrivateIpAddress: &testAddr12, Primary: ¬Primary }, + {PrivateIpAddress: &testAddr11, Primary: &primary}, + {PrivateIpAddress: &testAddr12, Primary: ¬Primary}, + {PrivateIpAddress: &testAddr12, Primary: ¬Primary}, }, &attachmentID, nil) @@ -302,9 +302,9 @@ func TestTryAddIPToENI(t *testing.T) { mockAWS.EXPECT().GetPrimaryENI().Return(primaryENIid) mockAWS.EXPECT().DescribeENI(secENIid).Return( []*ec2.NetworkInterfacePrivateIpAddress{ - { PrivateIpAddress: &testAddr11, Primary: &primary }, - { PrivateIpAddress: &testAddr12, Primary: ¬Primary }, - { PrivateIpAddress: &testAddr12, Primary: ¬Primary }, + {PrivateIpAddress: &testAddr11, Primary: &primary}, + {PrivateIpAddress: &testAddr12, Primary: ¬Primary}, + {PrivateIpAddress: &testAddr12, Primary: ¬Primary}, }, &attachmentID, nil) diff --git a/ipamd/rpc_handler_test.go b/ipamd/rpc_handler_test.go index 9c4e7c6bd8..b75016094d 100644 --- a/ipamd/rpc_handler_test.go +++ b/ipamd/rpc_handler_test.go @@ -15,9 +15,10 @@ package ipamd import ( "context" + "testing" + "github.com/aws/amazon-vpc-cni-k8s/ipamd/datastore" "github.com/aws/aws-sdk-go/aws" - "testing" pb "github.com/aws/amazon-vpc-cni-k8s/rpc" diff --git a/pkg/awsutils/awsutils_test.go b/pkg/awsutils/awsutils_test.go index f78862bfac..47ad1e697f 100644 --- a/pkg/awsutils/awsutils_test.go +++ b/pkg/awsutils/awsutils_test.go @@ -24,8 +24,8 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" - "github.com/aws/amazon-vpc-cni-k8s/pkg/ec2metadata/mocks" - "github.com/aws/amazon-vpc-cni-k8s/pkg/ec2wrapper/mocks" + mock_ec2metadata "github.com/aws/amazon-vpc-cni-k8s/pkg/ec2metadata/mocks" + mock_ec2wrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/ec2wrapper/mocks" ) const ( diff --git a/pkg/ec2metadatawrapper/ec2metadatawrapper_test.go b/pkg/ec2metadatawrapper/ec2metadatawrapper_test.go index 1a1c628a74..5484f3b1ea 100644 --- a/pkg/ec2metadatawrapper/ec2metadatawrapper_test.go +++ b/pkg/ec2metadatawrapper/ec2metadatawrapper_test.go @@ -1,10 +1,11 @@ package ec2metadatawrapper import ( - mockec2metadatawrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/ec2metadatawrapper/mocks" "testing" "time" + mockec2metadatawrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/ec2metadatawrapper/mocks" + "github.com/aws/aws-sdk-go/aws/ec2metadata" "github.com/golang/mock/gomock" diff --git a/pkg/k8sapi/discovery.go b/pkg/k8sapi/discovery.go index 429c93d0a6..d8e92a77f2 100644 --- a/pkg/k8sapi/discovery.go +++ b/pkg/k8sapi/discovery.go @@ -15,7 +15,7 @@ import ( clientset "k8s.io/client-go/kubernetes" "github.com/operator-framework/operator-sdk/pkg/k8sclient" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" diff --git a/pkg/netlinkwrapper/netlink.go b/pkg/netlinkwrapper/netlink.go index d9d19c4f80..26aa631832 100644 --- a/pkg/netlinkwrapper/netlink.go +++ b/pkg/netlinkwrapper/netlink.go @@ -14,8 +14,9 @@ package netlinkwrapper import ( - "github.com/vishvananda/netlink" "syscall" + + "github.com/vishvananda/netlink" ) // NetLink wraps methods used from the vishvananda/netlink package diff --git a/pkg/networkutils/network_test.go b/pkg/networkutils/network_test.go index e0663cdb40..31a6c93430 100644 --- a/pkg/networkutils/network_test.go +++ b/pkg/networkutils/network_test.go @@ -33,8 +33,8 @@ import ( mocks_ip "github.com/aws/amazon-vpc-cni-k8s/pkg/ipwrapper/mocks" "github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper/mock_netlink" - "github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper/mocks" - "github.com/aws/amazon-vpc-cni-k8s/pkg/nswrapper/mocks" + mock_netlinkwrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper/mocks" + mock_nswrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/nswrapper/mocks" ) const ( diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index e9d32ab339..ce3e14d942 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -20,7 +20,7 @@ import ( "time" "github.com/aws/amazon-vpc-cni-k8s/pkg/utils/ttime" - "github.com/aws/amazon-vpc-cni-k8s/pkg/utils/ttime/mocks" + mock_ttime "github.com/aws/amazon-vpc-cni-k8s/pkg/utils/ttime/mocks" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" ) diff --git a/plugins/routed-eni/cni_test.go b/plugins/routed-eni/cni_test.go index 10a4c24fdf..6914f73260 100644 --- a/plugins/routed-eni/cni_test.go +++ b/plugins/routed-eni/cni_test.go @@ -23,24 +23,24 @@ import ( "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" - "github.com/aws/amazon-vpc-cni-k8s/pkg/grpcwrapper/mocks" - "github.com/aws/amazon-vpc-cni-k8s/pkg/rpcwrapper/mocks" - "github.com/aws/amazon-vpc-cni-k8s/pkg/typeswrapper/mocks" - "github.com/aws/amazon-vpc-cni-k8s/plugins/routed-eni/driver/mocks" + mock_grpcwrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/grpcwrapper/mocks" + mock_rpcwrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/rpcwrapper/mocks" + mock_typeswrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/typeswrapper/mocks" + mock_driver "github.com/aws/amazon-vpc-cni-k8s/plugins/routed-eni/driver/mocks" "github.com/aws/amazon-vpc-cni-k8s/rpc" - "github.com/aws/amazon-vpc-cni-k8s/rpc/mocks" + mock_rpc "github.com/aws/amazon-vpc-cni-k8s/rpc/mocks" "google.golang.org/grpc" ) const ( - containerID = "test-container" - netNS = "/proc/ns/1234" - ifName = "eth0" - cniVersion = "1.0" - cniName = "aws-cni" - cniType = "aws-cni" - ipAddr = "10.0.1.15" - devNum = 4 + containerID = "test-container" + netNS = "/proc/ns/1234" + ifName = "eth0" + cniVersion = "1.0" + cniName = "aws-cni" + cniType = "aws-cni" + ipAddr = "10.0.1.15" + devNum = 4 ) func setup(t *testing.T) (*gomock.Controller, diff --git a/plugins/routed-eni/driver/driver_test.go b/plugins/routed-eni/driver/driver_test.go index 5ba9e0d2b5..e911e5892b 100644 --- a/plugins/routed-eni/driver/driver_test.go +++ b/plugins/routed-eni/driver/driver_test.go @@ -26,8 +26,8 @@ import ( "github.com/aws/amazon-vpc-cni-k8s/pkg/cninswrapper/mock_ns" mocks_ip "github.com/aws/amazon-vpc-cni-k8s/pkg/ipwrapper/mocks" "github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper/mock_netlink" - "github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper/mocks" - "github.com/aws/amazon-vpc-cni-k8s/pkg/nswrapper/mocks" + mock_netlinkwrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper/mocks" + mock_nswrapper "github.com/aws/amazon-vpc-cni-k8s/pkg/nswrapper/mocks" ) const (