From c6375c20b711eef0434495b0a77accef23a96920 Mon Sep 17 00:00:00 2001 From: Chun Chen Date: Thu, 9 Nov 2017 15:03:56 +0800 Subject: [PATCH] Add tests to test if legacy chains/rules can be cleaned up --- pkg/kubelet/network/hostport/BUILD | 2 + pkg/kubelet/network/hostport/fake_iptables.go | 24 +++++- .../network/hostport/fake_iptables_test.go | 56 +++++++++++++ .../network/hostport/hostport_manager.go | 11 ++- .../network/hostport/hostport_manager_test.go | 78 ++++++++++++++++++- .../network/hostport/hostport_syncer_test.go | 74 ++++++++++++++++++ pkg/util/iptables/iptables.go | 1 + 7 files changed, 240 insertions(+), 6 deletions(-) create mode 100644 pkg/kubelet/network/hostport/fake_iptables_test.go diff --git a/pkg/kubelet/network/hostport/BUILD b/pkg/kubelet/network/hostport/BUILD index ed448dc8be..a6bb4d0e14 100644 --- a/pkg/kubelet/network/hostport/BUILD +++ b/pkg/kubelet/network/hostport/BUILD @@ -21,12 +21,14 @@ go_library( "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", ], ) go_test( name = "go_default_test", srcs = [ + "fake_iptables_test.go", "hostport_manager_test.go", "hostport_syncer_test.go", "hostport_test.go", diff --git a/pkg/kubelet/network/hostport/fake_iptables.go b/pkg/kubelet/network/hostport/fake_iptables.go index 42f7c68116..ab28a1d874 100644 --- a/pkg/kubelet/network/hostport/fake_iptables.go +++ b/pkg/kubelet/network/hostport/fake_iptables.go @@ -22,6 +22,7 @@ import ( "net" "strings" + "k8s.io/apimachinery/pkg/util/sets" utiliptables "k8s.io/kubernetes/pkg/util/iptables" ) @@ -36,12 +37,18 @@ type fakeTable struct { } type fakeIPTables struct { - tables map[string]*fakeTable + tables map[string]*fakeTable + builtinChains map[string]sets.String } func NewFakeIPTables() *fakeIPTables { return &fakeIPTables{ tables: make(map[string]*fakeTable, 0), + builtinChains: map[string]sets.String{ + string(utiliptables.TableFilter): sets.NewString("INPUT", "FORWARD", "OUTPUT"), + string(utiliptables.TableNAT): sets.NewString("PREROUTING", "INPUT", "OUTPUT", "POSTROUTING"), + string(utiliptables.TableMangle): sets.NewString("PREROUTING", "INPUT", "FORWARD", "OUTPUT", "POSTROUTING"), + }, } } @@ -246,6 +253,7 @@ func (f *fakeIPTables) SaveInto(tableName utiliptables.Table, buffer *bytes.Buff } func (f *fakeIPTables) restore(restoreTableName utiliptables.Table, data []byte, flush utiliptables.FlushFlag) error { + allLines := string(data) buf := bytes.NewBuffer(data) var tableName utiliptables.Table for { @@ -274,6 +282,13 @@ func (f *fakeIPTables) restore(restoreTableName utiliptables.Table, data []byte, } } _, _ = f.ensureChain(tableName, chainName) + // The --noflush option for iptables-restore doesn't work for user-defined chains, only builtin chains. + // We should flush user-defined chains if the chain is not to be deleted + if !f.isBuiltinChain(tableName, chainName) && !strings.Contains(allLines, "-X "+string(chainName)) { + if err := f.FlushChain(tableName, chainName); err != nil { + return err + } + } } else if strings.HasPrefix(line, "-A") { parts := strings.Split(line, " ") if len(parts) < 3 { @@ -329,3 +344,10 @@ func (f *fakeIPTables) AddReloadFunc(reloadFunc func()) { func (f *fakeIPTables) Destroy() { } + +func (f *fakeIPTables) isBuiltinChain(tableName utiliptables.Table, chainName utiliptables.Chain) bool { + if builtinChains, ok := f.builtinChains[string(tableName)]; ok && builtinChains.Has(string(chainName)) { + return true + } + return false +} diff --git a/pkg/kubelet/network/hostport/fake_iptables_test.go b/pkg/kubelet/network/hostport/fake_iptables_test.go new file mode 100644 index 0000000000..bda4c02473 --- /dev/null +++ b/pkg/kubelet/network/hostport/fake_iptables_test.go @@ -0,0 +1,56 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License 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 hostport + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" + utiliptables "k8s.io/kubernetes/pkg/util/iptables" +) + +func TestRestoreFlushRules(t *testing.T) { + iptables := NewFakeIPTables() + rules := [][]string{ + {"-A", "KUBE-HOSTPORTS", "-m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp --dport 8443 -j KUBE-HP-5N7UH5JAXCVP5UJR"}, + {"-A", "POSTROUTING", "-m comment --comment \"SNAT for localhost access to hostports\" -o cbr0 -s 127.0.0.0/8 -j MASQUERADE"}, + } + natRules := bytes.NewBuffer(nil) + writeLine(natRules, "*nat") + for _, rule := range rules { + _, err := iptables.EnsureChain(utiliptables.TableNAT, utiliptables.Chain(rule[1])) + assert.NoError(t, err) + _, err = iptables.ensureRule(utiliptables.RulePosition(rule[0]), utiliptables.TableNAT, utiliptables.Chain(rule[1]), rule[2]) + assert.NoError(t, err) + + writeLine(natRules, utiliptables.MakeChainLine(utiliptables.Chain(rule[1]))) + } + writeLine(natRules, "COMMIT") + assert.NoError(t, iptables.Restore(utiliptables.TableNAT, natRules.Bytes(), utiliptables.NoFlushTables, utiliptables.RestoreCounters)) + natTable, ok := iptables.tables[string(utiliptables.TableNAT)] + assert.True(t, ok) + // check KUBE-HOSTPORTS chain, should have been cleaned up + hostportChain, ok := natTable.chains["KUBE-HOSTPORTS"] + assert.True(t, ok) + assert.Equal(t, 0, len(hostportChain.rules)) + + // check builtin chains, should not been cleaned up + postroutingChain, ok := natTable.chains["POSTROUTING"] + assert.True(t, ok, string(postroutingChain.name)) + assert.Equal(t, 1, len(postroutingChain.rules)) +} diff --git a/pkg/kubelet/network/hostport/hostport_manager.go b/pkg/kubelet/network/hostport/hostport_manager.go index 8bcbf822b7..a31b4da084 100644 --- a/pkg/kubelet/network/hostport/hostport_manager.go +++ b/pkg/kubelet/network/hostport/hostport_manager.go @@ -178,8 +178,8 @@ func (hm *hostportManager) Remove(id string, podPortMapping *PodPortMapping) (er chainsToRemove := []utiliptables.Chain{} for _, pm := range hostportMappings { chainsToRemove = append(chainsToRemove, getHostportChain(id, pm)) - // TODO remove this, please refer https://github.com/kubernetes/kubernetes/pull/55153 - chainsToRemove = append(chainsToRemove, getBugyHostportChain(id, pm)) + // TODO remove this after release 1.9, please refer https://github.com/kubernetes/kubernetes/pull/55153 + chainsToRemove = append(chainsToRemove, getBuggyHostportChain(id, pm)) } // remove rules that consists of target chains @@ -255,8 +255,11 @@ func getHostportChain(id string, pm *PortMapping) utiliptables.Chain { return utiliptables.Chain(kubeHostportChainPrefix + encoded[:16]) } -// TODO remove this, please refer https://github.com/kubernetes/kubernetes/pull/55153 -func getBugyHostportChain(id string, pm *PortMapping) utiliptables.Chain { +// This bugy func does bad conversion on HostPort from int32 to string. +// It may generates same chain names for different ports of the same pod, e.g. port 57119/55429/56833. +// `getHostportChain` fixed this bug. In order to cleanup the legacy chains/rules, it is temporarily left. +// TODO remove this after release 1.9, please refer https://github.com/kubernetes/kubernetes/pull/55153 +func getBuggyHostportChain(id string, pm *PortMapping) utiliptables.Chain { hash := sha256.Sum256([]byte(id + string(pm.HostPort) + string(pm.Protocol))) encoded := base32.StdEncoding.EncodeToString(hash[:]) return utiliptables.Chain(kubeHostportChainPrefix + encoded[:16]) diff --git a/pkg/kubelet/network/hostport/hostport_manager_test.go b/pkg/kubelet/network/hostport/hostport_manager_test.go index bd775ce509..1537d27494 100644 --- a/pkg/kubelet/network/hostport/hostport_manager_test.go +++ b/pkg/kubelet/network/hostport/hostport_manager_test.go @@ -19,12 +19,12 @@ package hostport import ( "bytes" "net" + "strings" "testing" "github.com/stretchr/testify/assert" "k8s.io/api/core/v1" utiliptables "k8s.io/kubernetes/pkg/util/iptables" - "strings" ) func NewFakeHostportManager() HostPortManager { @@ -211,3 +211,79 @@ func TestGetHostportChain(t *testing.T) { t.Fatal(m) } } + +func TestHostPortManagerRemoveLegacyRules(t *testing.T) { + iptables := NewFakeIPTables() + legacyRules := [][]string{ + {"-A", "KUBE-HOSTPORTS", "-m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp --dport 8443 -j KUBE-HP-5N7UH5JAXCVP5UJR"}, + {"-A", "KUBE-HOSTPORTS", "-m comment --comment \"pod1_ns1 hostport 8081\" -m udp -p udp --dport 8081 -j KUBE-HP-7THKRFSEH4GIIXK7"}, + {"-A", "KUBE-HOSTPORTS", "-m comment --comment \"pod1_ns1 hostport 8080\" -m tcp -p tcp --dport 8080 -j KUBE-HP-4YVONL46AKYWSKS3"}, + {"-A", "OUTPUT", "-m comment --comment \"kube hostport portals\" -m addrtype --dst-type LOCAL -j KUBE-HOSTPORTS"}, + {"-A", "PREROUTING", "-m comment --comment \"kube hostport portals\" -m addrtype --dst-type LOCAL -j KUBE-HOSTPORTS"}, + {"-A", "POSTROUTING", "-m comment --comment \"SNAT for localhost access to hostports\" -o cbr0 -s 127.0.0.0/8 -j MASQUERADE"}, + {"-A", "KUBE-HP-4YVONL46AKYWSKS3", "-m comment --comment \"pod1_ns1 hostport 8080\" -s 10.1.1.2/32 -j KUBE-MARK-MASQ"}, + {"-A", "KUBE-HP-4YVONL46AKYWSKS3", "-m comment --comment \"pod1_ns1 hostport 8080\" -m tcp -p tcp -j DNAT --to-destination 10.1.1.2:80"}, + {"-A", "KUBE-HP-7THKRFSEH4GIIXK7", "-m comment --comment \"pod1_ns1 hostport 8081\" -s 10.1.1.2/32 -j KUBE-MARK-MASQ"}, + {"-A", "KUBE-HP-7THKRFSEH4GIIXK7", "-m comment --comment \"pod1_ns1 hostport 8081\" -m udp -p udp -j DNAT --to-destination 10.1.1.2:81"}, + {"-A", "KUBE-HP-5N7UH5JAXCVP5UJR", "-m comment --comment \"pod3_ns1 hostport 8443\" -s 10.1.1.4/32 -j KUBE-MARK-MASQ"}, + {"-A", "KUBE-HP-5N7UH5JAXCVP5UJR", "-m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp -j DNAT --to-destination 10.1.1.4:443"}, + } + for _, rule := range legacyRules { + _, err := iptables.EnsureChain(utiliptables.TableNAT, utiliptables.Chain(rule[1])) + assert.NoError(t, err) + _, err = iptables.ensureRule(utiliptables.RulePosition(rule[0]), utiliptables.TableNAT, utiliptables.Chain(rule[1]), rule[2]) + assert.NoError(t, err) + } + portOpener := NewFakeSocketManager() + manager := &hostportManager{ + hostPortMap: make(map[hostport]closeable), + iptables: iptables, + portOpener: portOpener.openFakeSocket, + } + err := manager.Remove("id", &PodPortMapping{ + Name: "pod1", + Namespace: "ns1", + IP: net.ParseIP("10.1.1.2"), + HostNetwork: false, + PortMappings: []*PortMapping{ + { + HostPort: 8080, + ContainerPort: 80, + Protocol: v1.ProtocolTCP, + }, + { + HostPort: 8081, + ContainerPort: 81, + Protocol: v1.ProtocolUDP, + }, + }, + }) + assert.NoError(t, err) + + err = manager.Remove("id", &PodPortMapping{ + Name: "pod3", + Namespace: "ns1", + IP: net.ParseIP("10.1.1.4"), + HostNetwork: false, + PortMappings: []*PortMapping{ + { + HostPort: 8443, + ContainerPort: 443, + Protocol: v1.ProtocolTCP, + }, + }, + }) + assert.NoError(t, err) + + natTable, ok := iptables.tables[string(utiliptables.TableNAT)] + assert.True(t, ok) + // check KUBE-HOSTPORTS chain should be cleaned up + hostportChain, ok := natTable.chains["KUBE-HOSTPORTS"] + assert.True(t, ok, string(hostportChain.name)) + assert.Equal(t, 0, len(hostportChain.rules), "%v", hostportChain.rules) + // check KUBE-HP-* chains should be deleted + for _, name := range []string{"KUBE-HP-4YVONL46AKYWSKS3", "KUBE-HP-7THKRFSEH4GIIXK7", "KUBE-HP-5N7UH5JAXCVP5UJR"} { + _, ok := natTable.chains[name] + assert.False(t, ok) + } +} diff --git a/pkg/kubelet/network/hostport/hostport_syncer_test.go b/pkg/kubelet/network/hostport/hostport_syncer_test.go index 1cc0f40996..5cd3349ca8 100644 --- a/pkg/kubelet/network/hostport/hostport_syncer_test.go +++ b/pkg/kubelet/network/hostport/hostport_syncer_test.go @@ -22,6 +22,7 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" "k8s.io/api/core/v1" utiliptables "k8s.io/kubernetes/pkg/util/iptables" ) @@ -236,3 +237,76 @@ func TestHostportChainName(t *testing.T) { t.Fatal(m) } } + +func TestHostPortSyncerRemoveLegacyRules(t *testing.T) { + iptables := NewFakeIPTables() + legacyRules := [][]string{ + {"-A", "KUBE-HOSTPORTS", "-m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp --dport 8443 -j KUBE-HP-5N7UH5JAXCVP5UJR"}, + {"-A", "KUBE-HOSTPORTS", "-m comment --comment \"pod1_ns1 hostport 8081\" -m udp -p udp --dport 8081 -j KUBE-HP-7THKRFSEH4GIIXK7"}, + {"-A", "KUBE-HOSTPORTS", "-m comment --comment \"pod1_ns1 hostport 8080\" -m tcp -p tcp --dport 8080 -j KUBE-HP-4YVONL46AKYWSKS3"}, + {"-A", "OUTPUT", "-m comment --comment \"kube hostport portals\" -m addrtype --dst-type LOCAL -j KUBE-HOSTPORTS"}, + {"-A", "PREROUTING", "-m comment --comment \"kube hostport portals\" -m addrtype --dst-type LOCAL -j KUBE-HOSTPORTS"}, + {"-A", "POSTROUTING", "-m comment --comment \"SNAT for localhost access to hostports\" -o cbr0 -s 127.0.0.0/8 -j MASQUERADE"}, + {"-A", "KUBE-HP-4YVONL46AKYWSKS3", "-m comment --comment \"pod1_ns1 hostport 8080\" -s 10.1.1.2/32 -j KUBE-MARK-MASQ"}, + {"-A", "KUBE-HP-4YVONL46AKYWSKS3", "-m comment --comment \"pod1_ns1 hostport 8080\" -m tcp -p tcp -j DNAT --to-destination 10.1.1.2:80"}, + {"-A", "KUBE-HP-7THKRFSEH4GIIXK7", "-m comment --comment \"pod1_ns1 hostport 8081\" -s 10.1.1.2/32 -j KUBE-MARK-MASQ"}, + {"-A", "KUBE-HP-7THKRFSEH4GIIXK7", "-m comment --comment \"pod1_ns1 hostport 8081\" -m udp -p udp -j DNAT --to-destination 10.1.1.2:81"}, + {"-A", "KUBE-HP-5N7UH5JAXCVP5UJR", "-m comment --comment \"pod3_ns1 hostport 8443\" -s 10.1.1.4/32 -j KUBE-MARK-MASQ"}, + {"-A", "KUBE-HP-5N7UH5JAXCVP5UJR", "-m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp -j DNAT --to-destination 10.1.1.4:443"}, + } + for _, rule := range legacyRules { + _, err := iptables.EnsureChain(utiliptables.TableNAT, utiliptables.Chain(rule[1])) + assert.NoError(t, err) + _, err = iptables.ensureRule(utiliptables.RulePosition(rule[0]), utiliptables.TableNAT, utiliptables.Chain(rule[1]), rule[2]) + assert.NoError(t, err) + } + portOpener := NewFakeSocketManager() + h := &hostportSyncer{ + hostPortMap: make(map[hostport]closeable), + iptables: iptables, + portOpener: portOpener.openFakeSocket, + } + // check preserve pod3's rules and remove pod1's rules + pod3PortMapping := &PodPortMapping{ + Name: "pod3", + Namespace: "ns1", + IP: net.ParseIP("10.1.1.4"), + HostNetwork: false, + PortMappings: []*PortMapping{ + { + HostPort: 8443, + ContainerPort: 443, + Protocol: v1.ProtocolTCP, + }, + }, + } + h.SyncHostports("cbr0", []*PodPortMapping{pod3PortMapping}) + + newChainName := string(hostportChainName(pod3PortMapping.PortMappings[0], getPodFullName(pod3PortMapping))) + expectRules := [][]string{ + {"KUBE-HOSTPORTS", "-m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp --dport 8443 -j " + newChainName}, + {newChainName, "-m comment --comment \"pod3_ns1 hostport 8443\" -s 10.1.1.4/32 -j KUBE-MARK-MASQ"}, + {newChainName, "-m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp -j DNAT --to-destination 10.1.1.4:443"}, + } + + natTable, ok := iptables.tables[string(utiliptables.TableNAT)] + assert.True(t, ok) + // check pod1's rules in KUBE-HOSTPORTS chain should be cleaned up + hostportChain, ok := natTable.chains["KUBE-HOSTPORTS"] + assert.True(t, ok, string(hostportChain.name)) + assert.Equal(t, 1, len(hostportChain.rules), "%v", hostportChain.rules) + + // check pod3's rules left + assert.Equal(t, expectRules[0][1], hostportChain.rules[0]) + chain, ok := natTable.chains[newChainName] + assert.True(t, ok) + assert.Equal(t, 2, len(chain.rules)) + assert.Equal(t, expectRules[1][1], chain.rules[0]) + assert.Equal(t, expectRules[2][1], chain.rules[1]) + + // check legacy KUBE-HP-* chains should be deleted + for _, name := range []string{"KUBE-HP-4YVONL46AKYWSKS3", "KUBE-HP-7THKRFSEH4GIIXK7", "KUBE-HP-5N7UH5JAXCVP5UJR"} { + _, ok := natTable.chains[name] + assert.False(t, ok) + } +} diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 342e28ceeb..292288d218 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -82,6 +82,7 @@ type Table string const ( TableNAT Table = "nat" TableFilter Table = "filter" + TableMangle Table = "mangle" ) type Chain string