Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix hostport duplicate chain names #55153

Merged
merged 3 commits into from
Nov 15, 2017
Merged

Fix hostport duplicate chain names #55153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
932cf07
Fix hostport duplicate chain names
chenchun Nov 6, 2017
5d23dd8
Remove bugy chains as well to clean up old chain/rule
chenchun Nov 8, 2017
c6375c2
Add tests to test if legacy chains/rules can be cleaned up
chenchun Nov 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/kubelet/network/hostport/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
24 changes: 23 additions & 1 deletion pkg/kubelet/network/hostport/fake_iptables.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"net"
"strings"

"k8s.io/apimachinery/pkg/util/sets"
utiliptables "k8s.io/kubernetes/pkg/util/iptables"
)

Expand All @@ -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"),
},
}
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
56 changes: 56 additions & 0 deletions pkg/kubelet/network/hostport/fake_iptables_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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))
}
13 changes: 13 additions & 0 deletions pkg/kubelet/network/hostport/hostport_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"crypto/sha256"
"encoding/base32"
"fmt"
"strconv"
"strings"
"sync"

Expand Down Expand Up @@ -177,6 +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 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
Expand Down Expand Up @@ -247,6 +250,16 @@ func (hm *hostportManager) closeHostports(hostportMappings []*PortMapping) error
// WARNING: Please do not change this function. Otherwise, HostportManager may not be able to
// identify existing iptables chains.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean by changing the chain name, we are orphaning the old chain/rule? Is that acceptable or anyway to workaround?

@kubernetes/sig-network-pr-reviews

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a commit to clean up these old chains/rules as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, thought about this again, I wonder why it would be an issue changing this function. Can't we assume upon node/kubelet upgrade, all iptables rules will not be retained because node has been restarted? Am I suggesting an unnecessary cleanup? Would be great to have guidance from @thockin and @freehan

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we assume upon node/kubelet upgrade, all iptables rules will not be retained because node has been restarted?

😕 Is this true? I don't think everyone will restart node upon upgrade. Is this the required step of upgrading to the next release or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least that is what happens with gce/upgrade.sh :)

I'm not aware of any supported per-system-component upgrade mechanism in k8s yet, not sure if someone already support that...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is better. It's low cost.

func getHostportChain(id string, pm *PortMapping) utiliptables.Chain {
hash := sha256.Sum256([]byte(id + strconv.Itoa(int(pm.HostPort)) + string(pm.Protocol)))
encoded := base32.StdEncoding.EncodeToString(hash[:])
return utiliptables.Chain(kubeHostportChainPrefix + encoded[:16])
}

// 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])
Expand Down
115 changes: 102 additions & 13 deletions pkg/kubelet/network/hostport/hostport_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -144,21 +144,21 @@ func TestHostportManager(t *testing.T) {
`:OUTPUT - [0:0]`: true,
`:PREROUTING - [0:0]`: true,
`:POSTROUTING - [0:0]`: true,
`:KUBE-HP-4YVONL46AKYWSKS3 - [0:0]`: true,
`:KUBE-HP-7THKRFSEH4GIIXK7 - [0:0]`: true,
`:KUBE-HP-5N7UH5JAXCVP5UJR - [0:0]`: true,
"-A KUBE-HOSTPORTS -m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp --dport 8443 -j KUBE-HP-5N7UH5JAXCVP5UJR": true,
"-A KUBE-HOSTPORTS -m comment --comment \"pod1_ns1 hostport 8081\" -m udp -p udp --dport 8081 -j KUBE-HP-7THKRFSEH4GIIXK7": true,
"-A KUBE-HOSTPORTS -m comment --comment \"pod1_ns1 hostport 8080\" -m tcp -p tcp --dport 8080 -j KUBE-HP-4YVONL46AKYWSKS3": true,
`:KUBE-HP-IJHALPHTORMHHPPK - [0:0]`: true,
`:KUBE-HP-63UPIDJXVRSZGSUZ - [0:0]`: true,
`:KUBE-HP-WFBOALXEP42XEMJK - [0:0]`: true,
"-A KUBE-HOSTPORTS -m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp --dport 8443 -j KUBE-HP-WFBOALXEP42XEMJK": true,
"-A KUBE-HOSTPORTS -m comment --comment \"pod1_ns1 hostport 8081\" -m udp -p udp --dport 8081 -j KUBE-HP-63UPIDJXVRSZGSUZ": true,
"-A KUBE-HOSTPORTS -m comment --comment \"pod1_ns1 hostport 8080\" -m tcp -p tcp --dport 8080 -j KUBE-HP-IJHALPHTORMHHPPK": true,
"-A OUTPUT -m comment --comment \"kube hostport portals\" -m addrtype --dst-type LOCAL -j KUBE-HOSTPORTS": true,
"-A PREROUTING -m comment --comment \"kube hostport portals\" -m addrtype --dst-type LOCAL -j KUBE-HOSTPORTS": true,
"-A POSTROUTING -m comment --comment \"SNAT for localhost access to hostports\" -o cbr0 -s 127.0.0.0/8 -j MASQUERADE": true,
"-A KUBE-HP-4YVONL46AKYWSKS3 -m comment --comment \"pod1_ns1 hostport 8080\" -s 10.1.1.2/32 -j KUBE-MARK-MASQ": true,
"-A KUBE-HP-4YVONL46AKYWSKS3 -m comment --comment \"pod1_ns1 hostport 8080\" -m tcp -p tcp -j DNAT --to-destination 10.1.1.2:80": true,
"-A KUBE-HP-7THKRFSEH4GIIXK7 -m comment --comment \"pod1_ns1 hostport 8081\" -s 10.1.1.2/32 -j KUBE-MARK-MASQ": true,
"-A KUBE-HP-7THKRFSEH4GIIXK7 -m comment --comment \"pod1_ns1 hostport 8081\" -m udp -p udp -j DNAT --to-destination 10.1.1.2:81": true,
"-A KUBE-HP-5N7UH5JAXCVP5UJR -m comment --comment \"pod3_ns1 hostport 8443\" -s 10.1.1.4/32 -j KUBE-MARK-MASQ": true,
"-A KUBE-HP-5N7UH5JAXCVP5UJR -m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp -j DNAT --to-destination 10.1.1.4:443": true,
"-A KUBE-HP-IJHALPHTORMHHPPK -m comment --comment \"pod1_ns1 hostport 8080\" -s 10.1.1.2/32 -j KUBE-MARK-MASQ": true,
"-A KUBE-HP-IJHALPHTORMHHPPK -m comment --comment \"pod1_ns1 hostport 8080\" -m tcp -p tcp -j DNAT --to-destination 10.1.1.2:80": true,
"-A KUBE-HP-63UPIDJXVRSZGSUZ -m comment --comment \"pod1_ns1 hostport 8081\" -s 10.1.1.2/32 -j KUBE-MARK-MASQ": true,
"-A KUBE-HP-63UPIDJXVRSZGSUZ -m comment --comment \"pod1_ns1 hostport 8081\" -m udp -p udp -j DNAT --to-destination 10.1.1.2:81": true,
"-A KUBE-HP-WFBOALXEP42XEMJK -m comment --comment \"pod3_ns1 hostport 8443\" -s 10.1.1.4/32 -j KUBE-MARK-MASQ": true,
"-A KUBE-HP-WFBOALXEP42XEMJK -m comment --comment \"pod3_ns1 hostport 8443\" -m tcp -p tcp -j DNAT --to-destination 10.1.1.4:443": true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we validate the cleanup logic in unit test so that we will have more confidences?

`COMMIT`: true,
}
for _, line := range lines {
Expand Down Expand Up @@ -198,3 +198,92 @@ func TestHostportManager(t *testing.T) {
assert.EqualValues(t, true, port.closed)
}
}

func TestGetHostportChain(t *testing.T) {
m := make(map[string]int)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: what about map[string]bool?

chain := getHostportChain("testrdma-2", &PortMapping{HostPort: 57119, Protocol: "TCP", ContainerPort: 57119})
m[string(chain)] = 1
chain = getHostportChain("testrdma-2", &PortMapping{HostPort: 55429, Protocol: "TCP", ContainerPort: 55429})
m[string(chain)] = 1
chain = getHostportChain("testrdma-2", &PortMapping{HostPort: 56833, Protocol: "TCP", ContainerPort: 56833})
m[string(chain)] = 1
if len(m) != 3 {
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)
}
}
3 changes: 2 additions & 1 deletion pkg/kubelet/network/hostport/hostport_syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"crypto/sha256"
"encoding/base32"
"fmt"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -142,7 +143,7 @@ func writeLine(buf *bytes.Buffer, words ...string) {
// this because IPTables Chain Names must be <= 28 chars long, and the longer
// they are the harder they are to read.
func hostportChainName(pm *PortMapping, podFullName string) utiliptables.Chain {
hash := sha256.Sum256([]byte(string(pm.HostPort) + string(pm.Protocol) + podFullName))
hash := sha256.Sum256([]byte(strconv.Itoa(int(pm.HostPort)) + string(pm.Protocol) + podFullName))
Copy link
Member

@MrHohn MrHohn Nov 9, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For hostport_syncer, is the cleanup logic originally baked in? Might be great to verify it in unit test as well.

encoded := base32.StdEncoding.EncodeToString(hash[:])
return utiliptables.Chain(kubeHostportChainPrefix + encoded[:16])
}
Expand Down