Make iptables and ipvs modes of kube-proxy MASQUERADE --random-fully if possible #78547

MikeSpreitzer · 2019-05-30T20:42:42Z

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 api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This PR makes kube-proxy in iptables or ipvs mode include --random-fully in the iptables rule it emits to -j MASQUERADE, if the local version of iptables supports that additional flag. This greatly reduces the problems caused by a Linux kernel bug that bites when multiple flows are started at the same time. Due to this kernel bug the flows can get mapped to the same port, and all but one of them will suffer packet drops. This flag makes the port choice fully random, greatly reducing the probability of this port collision and consequent packet drop.

Which issue(s) this PR fixes:

Fixes #76699

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

MikeSpreitzer · 2019-05-30T20:45:57Z

@kubernetes/sig-network-bugs

MikeSpreitzer · 2019-05-31T03:16:04Z

/remove-label needs-sig

MikeSpreitzer · 2019-05-31T03:18:10Z

/woof

k8s-ci-robot · 2019-05-31T03:18:10Z

@MikeSpreitzer:

In response to this:

/woof

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.

cmluciano · 2019-06-04T19:58:23Z

/assign @cmluciano

cmluciano · 2019-06-05T18:36:40Z

pkg/proxy/iptables/proxier.go

@@ -308,6 +313,16 @@ func NewProxier(ipt utiliptables.Interface,
 		klog.Warning("missing br-netfilter module or unset sysctl br-nf-call-iptables; proxy may not work as intended")
 	}

+	masqueradeRandomFully := false
+	iptVersionString, err := ipt.GetVersion()


Can we log this error on startup?

cmluciano · 2019-06-05T18:42:55Z

pkg/proxy/iptables/proxier.go

+	masqueradeRandomFully := false
+	iptVersionString, err := ipt.GetVersion()
+	if err == nil {
+		iptVersion, err := utilversion.ParseGeneric(iptVersionString)


This call is duplicated with the first check on L317.

I am having trouble understanding the words here; this line invokes a different function than line 317. Can you please elaborate on the problem?

I got my lines confused. I meant to comment on L318-328 where we run ParseGeneric a few times in different nested ifs. Can we consolidate these lines and utilize something like version.Compare. It might just be me but I got a little lost walking through all of these nested conditions. I also had to reread the final comparison on L326 a few times before I understood that we are setting that flag to true based on the other conditions.

Perhaps @dcbw or @danwinship will have other thoughts on if it looks ok to them.

@cmluciano : in lines 318--328 there are two different strings parsed with utilversion.ParseGeneric. We coud remove one of those by using version.Compare --- which does a parse internally --- instead of version.AtLeast.

@danwinship : it looks like #78547 (comment) was intended to be attached here, right?

I am a little surprised that pkg/util/iptables does not cache the version string. Is that deliberate preparation for coping with live kernel patching? Should we continue to eschew caching the version string and things derived from it for that reason?

There is no direct external consumer for the "supports check" bit, so "SupportsRandomFully" (data bit or query function) would be the first of its kind.

@danwinship : it looks like #78547 (comment) was intended to be attached here, right?

oops. yes

I am a little surprised that pkg/util/iptables does not cache the version string. Is that deliberate preparation for coping with live kernel patching?

Nothing so planned. Originally it was only needed in one place. Later whoever added the second usage must have felt it would be easier to just call the "get version" function again rather than cache it. it's not like it takes that long to run "iptables --version", and you only end up doing it a few times anyway...

There is no direct external consumer for the "supports check" bit, so "SupportsRandomFully" (data bit or query function) would be the first of its kind.

There are external consumers (like pkg/proxy/iptables/proxier.go), they just have to implement it by getting the iptables version and manually comparing it against utiliptables.MinCheckVersion...

I have refactored, consolidating the version string parsing and comparison logic.

cmluciano · 2019-06-05T21:07:33Z

pkg/proxy/iptables/proxier.go

 		"-A", string(kubePostroutingChain),
 		"-m", "comment", "--comment", `"kubernetes service traffic requiring SNAT"`,
 		"-m", "mark", "--mark", proxier.masqueradeMark,
 		"-j", "MASQUERADE",
-	}...)
+	}
+	if proxier.masqueradeRandomFully {


Please add an e2e to be sure this is parsed correctly when this feature is enabled

In the existing code, if there is any error in any generated rule then (a) none of the generate rules will take effect (and thus the kube-proxy will be completely ineffective) and (b) an error message will be logged.

Presumably there are already e2e tests that exercise kube-proxy functionality. So those will test the rules that the revised code generates.

That may be the case, could you reference the kube-proxy test that leverages this generated code for the test cases

I'm mainly trying to find unit test that just validates the expected lines added by this flag or an e2e that enables the flag and ensures the baseline iptables functionality is correct. I can imagine that having the e2e for the scale tests would be useful to see if we can replicate the issue without this flag enabled.

The code isn't that confusing, though maybe it would be nice to just have pkg/util/iptables/ just check the version once and export a bunch of variables like SupportsCheck and SupportsRandomFully for everyone else to use.

On the original point, of testing:

I added a unit test that the generated -j MASQUERADE includes --random-fully if the iptables version supports it.

The existing e2e tests surely test that all the generated rules parse, for the version of iptables where the tests run. If any rule does not parse then none are installed, which causes massive damage --- for example, no client will be able to reach the kubernetes Service.

MikeSpreitzer · 2019-06-21T05:54:38Z

/retest

MikeSpreitzer · 2019-06-29T01:54:57Z

/test pull-kubernetes-verify

MikeSpreitzer · 2019-06-29T03:47:18Z

/test pull-kubernetes-verify

MikeSpreitzer · 2019-06-29T14:23:47Z

/test pull-kubernetes-verify

MikeSpreitzer · 2019-07-01T18:17:22Z

/retest

MikeSpreitzer · 2019-07-02T15:54:49Z

@danwinship , @cmluciano : I think I have addressed all the comments. Look good?

k8s-ci-robot · 2019-09-02T00:07:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, MikeSpreitzer, thockin

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

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

MikeSpreitzer · 2019-09-02T00:11:54Z

@cblecker : thanks!

MikeSpreitzer · 2019-09-02T00:12:11Z

/retest

…ng if possible Work around Linux kernel bug that sometimes causes multiple flows to get mapped to the same IP:PORT and consequently some suffer packet drops. Also made the same update in kubelet. Also added cross-pointers between the two bodies of code, in comments. Some day we should eliminate the duplicate code. But today is not that day.

MikeSpreitzer · 2019-09-02T02:08:56Z

Dang! I had to push another revision to fix a mis-spelling.

MikeSpreitzer · 2019-09-02T02:09:18Z

@cblecker

cblecker · 2019-09-02T02:40:13Z

/lgtm

MikeSpreitzer · 2019-09-02T02:43:01Z

thanks again!

lachie83 · 2019-09-02T03:30:47Z

👋 1.16 release lead here. I've been asked to consider this PR to be added to the 1.16 milestone. Can you please answer the following? Does this fix address a bug that was introduced by an enhancement in 1.16? If not, are you planning on getting this cherry-picked in the current supported releases? What are the associated risks of merging this?

MikeSpreitzer · 2019-09-02T14:43:04Z

@lachie83 : the bug that this fixes was reported in April, but has been around longer than that. This is a work-around for a Linux kernel deficiency or bug that has been there forever (as far as I know). Yes, cherry-picking this to supported releases would be good. Regarding risks: this is the usual work-around for the kernel issue, and I am not aware of any reported problems with this work-around.

MikeSpreitzer · 2019-09-02T15:01:28Z

@lachie83, another thought regarding risks: I am not sure how -j MASQUERADE --random-fully behaves when the kernel is running low on available ports; there could be a risk there.

dannyk81 · 2019-09-02T18:08:49Z

@lachie83 @MikeSpreitzer I'd also comment that this flag has become standard (for a while now) in recent releases of CNI plugins such as Calico and Flannel that create iptables rules.

Calico: projectcalico/felix#1901
Flannel: flannel-io/flannel#1040

Edit: considering the "illusive" impact of the underlying issue this flag fixes, I would be very pleased to see this in 1.16 as well as 1.15 🙏

MikeSpreitzer · 2019-09-03T15:59:18Z

Also FYI: Here is an explanation of the problem and how --random-fully works around it: https://tech.xing.com/a-reason-for-unexplained-connection-timeouts-on-kubernetes-docker-abd041cf7e02 . The code cited in reference (3) includes the loop at https://github.com/torvalds/linux/blob/24de3d377539e384621c5b8f8f8d8d01852dddc8/net/netfilter/nf_nat_proto_common.c#L87-L94 , which answers the question of how --random-fully reacts to high utilization of the port range: no worse than the alternatives. The only difference is where the search starts.

MikeSpreitzer · 2019-09-03T16:02:11Z

That same code structure is in the latest kernel in the 4.X line.
The 5.X line is different.

lachie83 · 2019-09-03T17:30:27Z

Thanks for the updates. Can we get issue #76699 appropriately labelled with the correct priority from sig-network so that I can evaluate the urgency of getting this milestoned in 1.16?

MikeSpreitzer · 2019-09-03T17:35:11Z

@lachie83 : can you please elaborate on what that means / how to get it done?

MikeSpreitzer · 2019-09-03T18:19:52Z

BTW, on further investigation: it looks like in the 5.X kernel they have made this randomization the normal behavior; the --random-fully option is still accepted but no longer makes a difference.

lachie83 · 2019-09-03T18:32:02Z

/milestone v1.16

k8s-ci-robot requested review from cmluciano and dnardo May 30, 2019 20:43

MikeSpreitzer force-pushed the fix-76699 branch from 30b4317 to dc97021 Compare May 30, 2019 20:44

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/ipvs and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 31, 2019

k8s-ci-robot assigned cmluciano Jun 4, 2019

cmluciano suggested changes Jun 5, 2019

View reviewed changes

MikeSpreitzer force-pushed the fix-76699 branch 3 times, most recently from ad402d1 to 7740a36 Compare June 29, 2019 00:27

MikeSpreitzer changed the title ~~Make iptables mode of kube-proxy MASQUERADE --random-fully if possible~~ Make iptables and ipvs modes of kube-proxy MASQUERADE --random-fully if possible Jul 2, 2019

MikeSpreitzer force-pushed the fix-76699 branch from 89d3019 to d86d1de Compare September 2, 2019 02:07

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2019

k8s-ci-robot added this to the v1.16 milestone Sep 3, 2019

k8s-ci-robot merged commit 542f3c6 into kubernetes:master Sep 3, 2019

MikeSpreitzer deleted the fix-76699 branch September 4, 2019 02:00

danwinship mentioned this pull request Sep 11, 2019

kube-proxy fails to restore iptable rules #82587

Closed

xmudrii mentioned this pull request Oct 2, 2019

Rework the Bug Triage handbook kubernetes/sig-release#807

Merged

jsmilani mentioned this pull request Oct 31, 2019

Added/Removing pod interrupts network for completely unrelated pod #84603

Closed

danwinship mentioned this pull request Mar 20, 2020

Don't log whether we're using iptables --random-fully #89296

Merged

This was referenced May 7, 2020

Additional 1s latency in host -> service IP -> pod when upgrading from 1.15.3 -> 1.18.1 on RHEL 8.1 #90854

Closed

Super slow access to service IP from host (& host-networked pods) with Flannel CNI flannel-io/flannel#1245

Closed

Make iptables and ipvs modes of kube-proxy MASQUERADE --random-fully if possible #78547

Make iptables and ipvs modes of kube-proxy MASQUERADE --random-fully if possible #78547

Conversation

MikeSpreitzer commented May 30, 2019 • edited Loading

MikeSpreitzer commented May 30, 2019

MikeSpreitzer commented May 31, 2019

MikeSpreitzer commented May 31, 2019

k8s-ci-robot commented May 31, 2019

cmluciano commented Jun 4, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeSpreitzer Jun 20, 2019 • edited Loading

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

MikeSpreitzer commented Jun 21, 2019

MikeSpreitzer commented Jun 29, 2019

MikeSpreitzer commented Jun 29, 2019

MikeSpreitzer commented Jun 29, 2019

MikeSpreitzer commented Jul 1, 2019

MikeSpreitzer commented Jul 2, 2019

k8s-ci-robot commented Sep 2, 2019

MikeSpreitzer commented Sep 2, 2019

MikeSpreitzer commented Sep 2, 2019

MikeSpreitzer commented Sep 2, 2019

MikeSpreitzer commented Sep 2, 2019

cblecker commented Sep 2, 2019

MikeSpreitzer commented Sep 2, 2019

lachie83 commented Sep 2, 2019 • edited Loading

MikeSpreitzer commented Sep 2, 2019

MikeSpreitzer commented Sep 2, 2019

dannyk81 commented Sep 2, 2019 • edited Loading

MikeSpreitzer commented Sep 3, 2019

MikeSpreitzer commented Sep 3, 2019

lachie83 commented Sep 3, 2019

MikeSpreitzer commented Sep 3, 2019

MikeSpreitzer commented Sep 3, 2019

lachie83 commented Sep 3, 2019

MikeSpreitzer commented May 30, 2019 •

edited

Loading

MikeSpreitzer Jun 20, 2019 •

edited

Loading

lachie83 commented Sep 2, 2019 •

edited

Loading

dannyk81 commented Sep 2, 2019 •

edited

Loading