Fix hostport duplicate chain names #55153

chenchun · 2017-11-06T11:25:24Z

Fixes bad conversion from int32 to string. Without this patch, getHostportChain/hostportChainName generates the same chain names for ports 57119/55429/56833 of the same pod.

closes #55771

Fixes bad conversion in host port chain name generating func which leads to some unreachable host ports.

MrHohn · 2017-11-06T19:05:20Z

/ok-to-test

MrHohn · 2017-11-06T19:01:54Z

pkg/kubelet/network/hostport/hostport_manager_test.go

@@ -198,3 +198,16 @@ func TestHostportManager(t *testing.T) {
 		assert.EqualValues(t, true, port.closed)
 	}
 }
+
+func TestGetHostportChain(t *testing.T) {
+	m := make(map[string]int)


nit: what about map[string]bool?

MrHohn · 2017-11-06T19:04:35Z

pkg/kubelet/network/hostport/hostport_manager.go

@@ -247,7 +248,7 @@ 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.


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

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

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

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?

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...

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

chenchun · 2017-11-08T07:10:09Z

/retest

MrHohn

Thanks, seems to be a viable solution. Some comments about testing.

MrHohn · 2017-11-09T03:52:04Z

pkg/kubelet/network/hostport/hostport_manager.go

+}
+
+// TODO remove this, please refer https://github.com/kubernetes/kubernetes/pull/55153
+func getBugyHostportChain(id string, pm *PortMapping) utiliptables.Chain {


Might be worth to also add an one-line description about the issue?

MrHohn · 2017-11-09T04:21:09Z

pkg/kubelet/network/hostport/hostport_manager_test.go

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


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

MrHohn · 2017-11-09T04:25:23Z

pkg/kubelet/network/hostport/hostport_syncer.go

@@ -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))


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

chenchun · 2017-11-09T10:24:53Z

@MrHohn Addressed all your comments, PTAL

MrHohn · 2017-11-10T04:50:38Z

pkg/kubelet/network/hostport/hostport_manager_test.go

+	assert.True(t, ok)
+	// check KUBE-HOSTPORTS chain should be cleaned up
+	hostportChain, ok := natTable.chains["KUBE-HOSTPORTS"]
+	assert.True(t, ok, "%s %v", string(hostportChain.name))


nit:

assert.True(t, ok, "%s", string(hostportChain.name))

MrHohn · 2017-11-10T04:51:36Z

pkg/kubelet/network/hostport/hostport_syncer_test.go

+	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, "%s %v", string(hostportChain.name))


nit:

assert.True(t, ok, "%s", string(hostportChain.name))

MrHohn · 2017-11-13T00:55:07Z

pkg/kubelet/network/hostport/fake_iptables.go

@@ -274,6 +275,11 @@ func (f *fakeIPTables) restore(restoreTableName utiliptables.Table, data []byte,
 					}
 				}
 				_, _ = f.ensureChain(tableName, chainName)
+				if !strings.Contains(allLines, "-X "+string(chainName)) {
+					if err := f.FlushChain(tableName, chainName); err != nil {


Sorry I don't quite get the logic here, why should we flush the chain when there is no -X CHAIN_NAME? Explanation would be appreciated.

I think I should update here to just flush user defined chains.
The --noflush option for iptables-restore doesn't work for user-defined chains, such as TESTCHAIN, only builtin chains. https://unix.stackexchange.com/questions/134687/how-to-combine-iptables-rulesets

I did a test to confirm this. See below example, the POSTROUTING chain didn't get flush, but KUBE-NODEPORT chain did.

[root@kubernetes-master vagrant]# iptables -t nat -S -P PREROUTING ACCEPT -P INPUT ACCEPT -P OUTPUT ACCEPT -P POSTROUTING ACCEPT -N KUBE-HP-5N7UH5JAXCVP5UJR -N KUBE-NODEPORT -A POSTROUTING -p tcp -m comment --comment "pod3_ns1 hostport 8443" -m tcp --dport 8443 -j KUBE-HP-5N7UH5JAXCVP5UJR -A KUBE-NODEPORT -p tcp -m comment --comment "pod3_ns1 hostport 8443" -m tcp --dport 8443 -j KUBE-HP-5N7UH5JAXCVP5UJR [root@kubernetes-master vagrant]# cat nat.log # Generated by iptables-save v1.4.21 on Mon Nov 13 03:14:59 2017 *nat :PREROUTING ACCEPT [65:3900] :INPUT ACCEPT [65:3900] :OUTPUT ACCEPT [0:0] :POSTROUTING ACCEPT [0:0] :KUBE-HP-5N7UH5JAXCVP5UJR - [0:0] :KUBE-NODEPORT - [0:0] COMMIT # Completed on Mon Nov 13 03:14:59 2017 [root@kubernetes-master vagrant]# iptables-restore --table=nat --noflush < nat.log [root@kubernetes-master vagrant]# iptables -t nat -S -P PREROUTING ACCEPT -P INPUT ACCEPT -P OUTPUT ACCEPT -P POSTROUTING ACCEPT -N KUBE-HP-5N7UH5JAXCVP5UJR -N KUBE-NODEPORT -A POSTROUTING -p tcp -m comment --comment "pod3_ns1 hostport 8443" -m tcp --dport 8443 -j KUBE-HP-5N7UH5JAXCVP5UJR

And this behavior is needed to cleanup rules in KUBE-NODEPORT in my tests.

Thanks for the investigation, now I get it. Interesting that this behavior is not documented in iptables manual.

Though I just learned that user-defined chains won't always be flushed, unless they are explicitly mentioned in the input of iptables-restore --noflush, as what below comment is referring to:

kubernetes/pkg/kubelet/network/hostport/hostport_syncer.go

Lines 263 to 267 in f281404

// We must (as per iptables) write a chain-line for it, which has

// the nice effect of flushing the chain. Then we can remove the

// chain.

writeLine(natChains, existingNATChains[chain])

writeLine(natRules, "-X", chainString)

In short I believe your fix will work as expected, but the implementation of fakeIPTables.restore() still seems problematic as it flushes more than it should...

Oops I scan too fast, looks like that is exactly what you have implemented. Could you update it to just flush user defined chains? Thanks.

Done. Also added a test TestRestoreFlushRules for fackIPtables.restore().

Awesome, thanks!

MrHohn · 2017-11-13T01:00:59Z

pkg/kubelet/network/hostport/hostport_manager.go

@@ -247,7 +248,7 @@ 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.


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

MrHohn

Thanks, appreciate your works!

/lgtm

MrHohn · 2017-11-14T05:09:28Z

This might worth a release note.
/release-note

/assign @thockin
for approval.

k8s-ci-robot · 2017-11-14T05:09:28Z

@MrHohn: the /release-note and /release-note-action-required commands have been deprecated.
Please edit the release-note block in the PR body text to include the release note. If the release note requires additional action include the string action required in the release note. For example:

```release-note
Some release note with action required.
```

In response to this:

This might worth a release note.
/release-note

/assign @thockin
for approval.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

thockin · 2017-11-15T05:40:09Z

pkg/util/iptables/iptables.go

@@ -82,6 +82,7 @@ type Table string
 const (
 	TableNAT    Table = "nat"
 	TableFilter Table = "filter"
+	TableMangle Table = "mangle"


Why is this part of the same PR? Seems unrelated?

It's used by NewFakeIPTables to cache builtin chains.

thockin · 2017-11-15T05:40:23Z

pkg/kubelet/network/hostport/hostport_manager.go

@@ -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, please refer https://github.com/kubernetes/kubernetes/pull/55153
+		chainsToRemove = append(chainsToRemove, getBugyHostportChain(id, pm))


s/Bugy/Buggy/

thockin · 2017-11-15T05:41:08Z

pkg/kubelet/network/hostport/hostport_manager.go

@@ -247,7 +248,7 @@ 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.


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

thockin · 2017-11-15T05:43:12Z

pkg/kubelet/network/hostport/hostport_manager.go

@@ -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, please refer https://github.com/kubernetes/kubernetes/pull/55153


... after release 1.9

thockin · 2017-11-15T06:35:57Z

This needs an associated issue and a closes #12345 in the FIRST COMMENT, or it can not be approved.

thockin · 2017-11-15T06:36:13Z

/lgtm

closes kubernetes#55771

MrHohn · 2017-11-15T18:07:56Z

Comments are addressed and an associated issue is created.

/lgtm

k8s-github-robot · 2017-11-15T18:08:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chenchun, MrHohn, thockin

Associated issue: 55771

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubelet/network/OWNERS~~ [thockin]
~~pkg/util/iptables/OWNERS~~ [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-15T20:57:58Z

Automatic merge from submit-queue (batch tested with PRs 54436, 53148, 55153, 55614, 55484). If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. remove duplicate function getBuggyHostportChain **What this PR does / why we need it**: remove `TODO remove this after release 1.9, please refer #55153 function `getBuggyHostportChain` does bad conversion on HostPort from int32 to string, now that `getHostportChain` does right, we remove function `getBuggyHostportChain` . **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes # **Special notes for your reviewer**: **Release note**: ```release-note NONE ```

k8s-github-robot assigned MrHohn and nicksardo Nov 6, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 6, 2017

MrHohn reviewed Nov 6, 2017

View reviewed changes

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Nov 6, 2017

MrHohn reviewed Nov 9, 2017

View reviewed changes

chenchun force-pushed the fix branch from d67cfce to 82711b1 Compare November 9, 2017 10:23

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 9, 2017

MrHohn reviewed Nov 13, 2017

View reviewed changes

chenchun force-pushed the fix branch 3 times, most recently from b4da86d to 58f44a9 Compare November 14, 2017 01:49

MrHohn approved these changes Nov 14, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2017

k8s-ci-robot assigned thockin Nov 14, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 14, 2017

thockin reviewed Nov 15, 2017

View reviewed changes

chenchun force-pushed the fix branch from 58f44a9 to 61b3bea Compare November 15, 2017 06:18

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2017

chenchun force-pushed the fix branch from 61b3bea to 283ca8c Compare November 15, 2017 07:12

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 15, 2017

chenchun added 3 commits November 15, 2017 15:15

Fix hostport duplicate chain names

932cf07

closes kubernetes#55771

Remove bugy chains as well to clean up old chain/rule

5d23dd8

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

c6375c2

chenchun force-pushed the fix branch from 283ca8c to c6375c2 Compare November 15, 2017 07:15

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2017

k8s-github-robot merged commit 7ad180a into kubernetes:master Nov 15, 2017

chenchun deleted the fix branch November 16, 2017 01:32

		@@ -247,7 +248,7 @@ 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.

	// We must (as per iptables) write a chain-line for it, which has
	// the nice effect of flushing the chain. Then we can remove the
	// chain.
	writeLine(natChains, existingNATChains[chain])
	writeLine(natRules, "-X", chainString)

Fix hostport duplicate chain names #55153

Fix hostport duplicate chain names #55153

Conversation

chenchun commented Nov 6, 2017 • edited

MrHohn commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenchun commented Nov 8, 2017

MrHohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn Nov 9, 2017 • edited

Choose a reason for hiding this comment

chenchun commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenchun Nov 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn left a comment

Choose a reason for hiding this comment

MrHohn commented Nov 14, 2017

k8s-ci-robot commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Nov 15, 2017

thockin commented Nov 15, 2017

MrHohn commented Nov 15, 2017

k8s-github-robot commented Nov 15, 2017

k8s-github-robot commented Nov 15, 2017

chenchun commented Nov 6, 2017 •

edited

MrHohn Nov 9, 2017 •

edited

chenchun Nov 14, 2017 •

edited