only manage kubernetes masquerade and postrouting iptables rules in kubelet #82116

liuxu623 · 2019-08-29T08:30:18Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind cleanup

What this PR does / why we need it:
These iptables rules already managed by kubelet, so we should remove code in iptables/ipvs proxier.

*nat
-A POSTROUTING -m comment --comment "kubernetes postrouting rules" -j KUBE-POSTROUTING
-A KUBE-MARK-MASQ -j MARK --set-xmark 0x4000/0x4000
-A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -m mark --mark 0x4000/0x4000 -j MASQUERADE

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2019-08-29T08:30:26Z

Hi @liuxu623. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2019-08-29T08:31:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liuxu623
To complete the pull request process, please assign dnardo, random-liu
You can assign the PR to them by writing /assign @dnardo @random-liu in a comment when ready.

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

Needs approval from an approver in each of these files:

pkg/kubelet/OWNERS
pkg/proxy/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liuxu623 · 2019-08-29T08:33:10Z

/assign @dnardo @Random-Liu

danwinship · 2019-08-29T13:55:06Z

So I also just filed a bug about this, #82125.

In particular, kube-proxy has a command-line argument related to this, which needs to be dealt with as well, so it's not just a matter of deleting code.

pkg/proxy/iptables/proxier.go

danwinship · 2019-08-29T13:59:24Z

pkg/proxy/ipvs/proxier.go

@@ -1591,7 +1576,7 @@ func (proxier *Proxier) createAndLinkeKubeChain() {
 			if chain, ok := existingNATChains[ch.chain]; ok {
 				writeBytesLine(proxier.natChains, chain)
 			} else {
-				writeLine(proxier.natChains, utiliptables.MakeChainLine(kubePostroutingChain))
+				writeLine(proxier.natChains, utiliptables.MakeChainLine(ch.chain))


um... I'm not sure what this code is doing but this clearly changes its behavior...

I think this code is copy form iptables/proxier.go#L763-L778, it means if chain not exist, create it.

I'm not sure what the original intent here was (maybe @m1093782566 knows), but this does seem wrong but I don't think it broke anyone because we already ensure the chain exists in line 1605.

I write some test code, iptables_test1.go and iptables_test2.go, when all kube iptables chains not exist, iptables_test1.go output is

:KUBE-POSTROUTING - [0:0] :KUBE-POSTROUTING - [0:0] :KUBE-POSTROUTING - [0:0] :KUBE-POSTROUTING - [0:0]

iptables_test2.go output is

:KUBE-SERVICES - [0:0] :KUBE-FIREWALL - [0:0] :KUBE-NODE-PORT - [0:0] :KUBE-LOAD-BALANCER - [0:0]

And you are right, we already ensure the chain exists in line 1605, so I think we don't actually need line 1609-1622 anymore.

We can do this in iptables proxier too, use iptables.EnsureChain to create not exist chains instead of iptables.RestoreAll.

Letting RestoreAll ensure the chains is more efficient than making extra EnsureChain calls. We should only call EnsureChain for the chains that we can't ensure from RestoreAll because we aren't filling in their contents. (eg, the RestoreAll input might include -j KUBE-MARK-DROP in some rule, but we can't actually define the KUBE-MARK-DROP chain in RestoreAll because we don't know exactly what the rule in that chain is supposed to be. So we should EnsureChain it.)

ok, so we should remove EnsureChain ? Becase we already use RestoreAll to ensure the chains exists.

fejta-bot · 2019-12-04T14:32:48Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

danwinship · 2019-12-13T15:34:51Z

Tim has raised some concerns about this idea in #82125, and there is also related discussion in #84945 around these rules. I think this needs a KEP to iron out the details. I'm closing this PR for now but we may be able to use some of this code later when we agree on a path forward.
/close

k8s-ci-robot · 2019-12-13T15:34:53Z

@danwinship: Closed this PR.

In response to this:

Tim has raised some concerns about this idea in #82125, and there is also related discussion in #84945 around these rules. I think this needs a KEP to iron out the details. I'm closing this PR for now but we may be able to use some of this code later when we agree on a path forward.
/close

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.

k8s-ci-robot · 2019-12-13T15:34:58Z

@liuxu623: PR needs rebase.

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.

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 29, 2019

k8s-ci-robot requested review from derekwaynecarr and nicksardo August 29, 2019 08:32

k8s-ci-robot added area/ipvs area/kubelet sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 29, 2019

k8s-ci-robot assigned dnardo and Random-Liu Aug 29, 2019

aojea mentioned this pull request Aug 29, 2019

fix KUBE-MARK-MASQ ownership (kubelet vs kube-proxy) #82125

Closed

danwinship reviewed Aug 29, 2019

View reviewed changes

liuxu623 force-pushed the master branch from 4d4d6a3 to da342b6 Compare August 29, 2019 15:19

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 Aug 29, 2019

liuxu623 added 5 commits August 30, 2019 08:49

only manage kubernetes masquerade iptables rule in kubelet

cfa6726

only manage kubernetes postrouting iptables rule in kubelet

f9dfc51

small fix

0d13642

fix comment

d05d21f

remove const KubeMarkMasqChain in kube-proxy

6aeaa71

remove const KubeMarkDropChain in kube-proxy

72c8ca2

liuxu623 force-pushed the master branch 2 times, most recently from b9d291a to e6e7200 Compare August 30, 2019 01:07

remove const kubePostroutingChain in kube-proxy

a48b785

liuxu623 force-pushed the master branch from e6e7200 to a48b785 Compare August 30, 2019 01:07

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 4, 2019

k8s-ci-robot closed this Dec 13, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

only manage kubernetes masquerade and postrouting iptables rules in kubelet #82116

only manage kubernetes masquerade and postrouting iptables rules in kubelet #82116

liuxu623 commented Aug 29, 2019 •

edited

Loading

k8s-ci-robot commented Aug 29, 2019

k8s-ci-robot commented Aug 29, 2019

liuxu623 commented Aug 29, 2019

danwinship commented Aug 29, 2019

danwinship Aug 29, 2019

liuxu623 Aug 29, 2019 •

edited

Loading

andrewsykim Sep 4, 2019

liuxu623 Sep 5, 2019

liuxu623 Sep 5, 2019 •

edited

Loading

liuxu623 Sep 5, 2019

danwinship Sep 5, 2019

liuxu623 Sep 5, 2019

fejta-bot commented Dec 4, 2019

danwinship commented Dec 13, 2019

k8s-ci-robot commented Dec 13, 2019

k8s-ci-robot commented Dec 13, 2019

only manage kubernetes masquerade and postrouting iptables rules in kubelet #82116

only manage kubernetes masquerade and postrouting iptables rules in kubelet #82116

Conversation

liuxu623 commented Aug 29, 2019 • edited Loading

k8s-ci-robot commented Aug 29, 2019

k8s-ci-robot commented Aug 29, 2019

liuxu623 commented Aug 29, 2019

danwinship commented Aug 29, 2019

danwinship Aug 29, 2019

Choose a reason for hiding this comment

liuxu623 Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

andrewsykim Sep 4, 2019

Choose a reason for hiding this comment

liuxu623 Sep 5, 2019

Choose a reason for hiding this comment

liuxu623 Sep 5, 2019 • edited Loading

Choose a reason for hiding this comment

liuxu623 Sep 5, 2019

Choose a reason for hiding this comment

danwinship Sep 5, 2019

Choose a reason for hiding this comment

liuxu623 Sep 5, 2019

Choose a reason for hiding this comment

fejta-bot commented Dec 4, 2019

danwinship commented Dec 13, 2019

k8s-ci-robot commented Dec 13, 2019

k8s-ci-robot commented Dec 13, 2019

liuxu623 commented Aug 29, 2019 •

edited

Loading

liuxu623 Aug 29, 2019 •

edited

Loading

liuxu623 Sep 5, 2019 •

edited

Loading