Add tests to test if legacy chains/rules can be cleaned up

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",

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
+}

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))
+}

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
+		// TODO remove this after release 1.9, please refer https://github.com/kubernetes/kubernetes/pull/55153
-		chainsToRemove = append(chainsToRemove, getBugyHostportChain(id, pm))
+		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
+// This bugy func does bad conversion on HostPort from int32 to string.
-func getBugyHostportChain(id string, pm *PortMapping) utiliptables.Chain {
+// 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])

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)
+	}
+}

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)
+	}
+}

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