Fix failing test "Services should only allow access from service loadbalancer source ranges" #94591

Lion-Wei · 2020-09-07T11:27:54Z

What type of PR is this?

/kind bug
/kind failing-test

What this PR does / why we need it:

kube-proxy IPVS mode will check KUBE-MARK-DROP chain in each sync, which will result in kube-proxy cleanup rules associated to the KUBE-MARK-DROP chain, and this will break firewall logic.

And this result in "Services should only allow access from service loadbalancer source ranges" case keep failing.

This pr removed check KUBE-MARK-DROP in ipvs mode, to make sure kubelet and kube-proxy handle their own rules.

Which issue(s) this PR fixes:
Fixes #94356

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

andrewsykim · 2020-09-07T13:42:30Z

pkg/proxy/ipvs/proxier.go

@@ -117,7 +117,6 @@ var iptablesChains = []struct {
 	{utiliptables.TableNAT, KubeNodePortChain},
 	{utiliptables.TableNAT, KubeLoadBalancerChain},
 	{utiliptables.TableNAT, KubeMarkMasqChain},
-	{utiliptables.TableNAT, KubeMarkDropChain},


If I recall correctly, this is only ensuring the drop chain doesn't exist but I don't think we're writing any rules here that would conflict with kubelet. Can you point me to where we're doing this?

lol, I've introduced the same bug in kube-proxy iptables some time ago :) #85527

9e6a687

Ah I see -- so as part of the save/restore it is also clearing out rules that kubelet created.

The issue that @uablrek mentioned in #82214 still seems valid though? Can we ensure the chain exists but not try any rules to it?

How is the dual-stack situation handled for proxy-mode=iptables? I use a work-around by creating the chain in a script before K8s is started;

/etc/init.d/30kube-prep.rc: ip6tables -t nat -N KUBE-MARK-DROP

Since I start kubelet and kube-proxy from scripts I also get the race. kube-proxy crashes and re-starts a couple of times until the chain is created by kubelet. Not a big deal, but not the most elegant solotion either.

xref #82125 (comment)
😄

Thanks for reviewing 😄

As what we did in #81517 , we chosed to belived nobody change or delete iptables chains created by kubernetes, except system firewall be reloaded.

But we do have race issues like kube-proxy up before kubelet, maybe ensures the chain before is tries to use it is a better way, but not try to change any rule.

After all, this is because KUBE-MARK-DROP chain created by kubelet and used by kube-proxy, as @aojea metioned, kube-proxy is the only consumer, maybe moving KUBE-MARK-DROP to the kube-proxies is another solution.

Overall, I think we have two potential solutions to solve this problem:

Ensure the chain exists but not try any rules to it (Maybe the same with KUBE-MARK-MASQ)

Moving KUBE-MARK-DROP to the kube-proxies.

I think the first one is a better way to solve current failing test, the second way would need further discussion.

The issue that @uablrek mentioned in #82214 still seems valid though? Can we ensure the chain exists but not try any rules to it?

As Antonio said, the problem there is mostly that kube-proxy supports dual-stack and kubelet does not currently, and that should be fixed by #94474. Given #94474, the fix here is probably safe, although yes, I agree it would be better if ipvs could ensure that the chain exists but not delete it.

And then longer-term, #82125

I agree it would be better if ipvs could ensure that the chain exists but not delete it.

Can we do this now for both ipvs and iptables? Seems like the fix is to run EnsureChain for KUBE-MARK-DROP but not add anything to the nat chain buffer.

kubernetes/pkg/proxy/ipvs/proxier.go

Lines 1866 to 1882 in 3fc1bc7

if _, err := proxier.iptables.EnsureChain(ch.table, ch.chain); err != nil {

klog.Errorf("Failed to ensure that %s chain %s exists: %v", ch.table, ch.chain, err)

return

}

if ch.table == utiliptables.TableNAT {

if chain, ok := existingNATChains[ch.chain]; ok {

writeBytesLine(proxier.natChains, chain)

} else {

writeLine(proxier.natChains, utiliptables.MakeChainLine(kubePostroutingChain))

}

} else {

if chain, ok := existingFilterChains[KubeForwardChain]; ok {

writeBytesLine(proxier.filterChains, chain)

} else {

writeLine(proxier.filterChains, utiliptables.MakeChainLine(KubeForwardChain))

}

}

aojea · 2020-09-07T14:45:40Z

/assign @andrewsykim @uablrek
xref #85527

andrewsykim · 2020-09-11T01:12:41Z

pkg/proxy/ipvs/proxier.go

 	{utiliptables.TableNAT, KubeMarkMasqChain},
 	{utiliptables.TableNAT, KubeMarkDropChain},
-	{utiliptables.TableFilter, KubeForwardChain},


What's the reason for changing the order here?

No reason, actually. 😂

andrewsykim · 2020-09-11T01:15:33Z

pkg/proxy/ipvs/proxier.go

@@ -1867,6 +1859,11 @@ func (proxier *Proxier) createAndLinkeKubeChain() {
 			klog.Errorf("Failed to ensure that %s chain %s exists: %v", ch.table, ch.chain, err)
 			return
 		}
+		// KUBE-MARK-MASQ and KUBE-MARK-DROP is created in kubelet
+		// kube-proxy only ensure it exist but not do anything to those chains
+		if ch.chain == KubeMarkDropChain || ch.chain == KubeMarkMasqChain {


Instead of checking specifically for KubeMarkDropChain and KubeMarkMasqChain here and skipping, I would prefer a separate list of chains containing KubeMarkDropChain and KubeMarkMasqChain only (similar to iptablesChains) where we only run proxier.iptables.EnsureChain.

Sure, that might be better. Addressed. : )

andrewsykim · 2020-09-11T01:36:52Z

pkg/proxy/ipvs/proxier.go

@@ -1862,6 +1852,12 @@ func (proxier *Proxier) createAndLinkeKubeChain() {
 	existingNATChains := proxier.getExistingChains(proxier.iptablesData, utiliptables.TableNAT)

 	// Make sure we keep stats for the top-level chains


This comment still belongs below I think.

I think KubeMarkDropChain and KubeMarkMasqChain also chains that we should keep stat?

andrewsykim · 2020-09-11T01:37:58Z

pkg/proxy/ipvs/proxier.go

-	// value should ever change.
-	writeLine(proxier.natRules, []string{
-		"-A", string(KubeMarkMasqChain),
-		"-j", "MARK", "--or-mark", proxier.masqueradeMark,


I don't think it's safe to delete this yet?

Is that so? IMO, KUBE-MARK-MASQ and KUBE-MARK-DROP are similar.

They are similar, but kube-proxy relies on kubelet to apply the drop rules in KUBE-MARK-DROP, where-as kube-proxy adds rules to KUBE-MARK-MASQ like here, which may overlap with kubelet.

aojea · 2020-09-11T22:30:02Z

pkg/proxy/iptables/proxier.go

+	// ensure KUBE-MARK-DROP chain exist but not do any change
+	if _, err := proxier.iptables.EnsureChain(utiliptables.TableNAT, KubeMarkDropChain); err != nil {
+		klog.Errorf("Failed to ensure that %s chain %s exists: %v", utiliptables.TableNAT, KubeMarkDropChain, err)
+		return


in a dual stack cluster, that we know this rule does not exist in one of the ip families,
kube-proxy will stop to work in a one family or will stop to work at all?

Sorry I didn't notice this before. Then both ipvs and iptbales mode have this issue, I think one of the ip familier don't have this rule is a bug, maybe we can wait #94474 to be merged.

Are we still waiting on #94474 for this?

It seems like there are two separate issues here. For IPVS specifically, we are doing what iptables used to do before #85527. Sounds like we should fix that first independent of the dual-stack PR? Am I missing something there?

aojea · 2020-10-01T09:11:09Z

@Lion-Wei I think that this is not the problem, the test is failing with the same error in other jobs with iptables instead of ipvs

#95225

aojea · 2020-10-15T07:55:21Z

/priority important-soon

Lion-Wei · 2020-10-15T08:53:49Z

/test pull-kubernetes-e2e-gci-gce-ipvs

I'll fix this flaky case ASAP 😓

andrewsykim · 2020-10-15T17:50:57Z

pkg/proxy/iptables/proxier.go

@@ -868,6 +868,12 @@ func (proxier *Proxier) syncProxyRules() {
 		}
 	}

+	// ensure KUBE-MARK-DROP chain exist but not do any change
+	if _, err := proxier.iptables.EnsureChain(utiliptables.TableNAT, KubeMarkDropChain); err != nil {


Similar to iptablesJumpChains can we have a variable like iptablesEnsureChain containing only KubeMarkDropChain?

That will be more readable. Addressed : )

andrewsykim

Left some minor comments but overall I think this looks okay -- someone else with more context on kubelet / kube-proxy iptables ownership (#82125) should probably be the approver

andrewsykim · 2020-10-15T22:17:08Z

pkg/proxy/ipvs/proxier.go

@@ -1858,6 +1857,12 @@ func (proxier *Proxier) createAndLinkeKubeChain() {
 	existingFilterChains := proxier.getExistingChains(proxier.filterChainsData, utiliptables.TableFilter)
 	existingNATChains := proxier.getExistingChains(proxier.iptablesData, utiliptables.TableNAT)

+	// ensure KUBE-MARK-DROP chain exist but not do any change


// ensure KUBE-MARK-DROP chain exist but not do any change any rules

aojea · 2020-10-15T22:23:18Z

kubelet / kube-proxy iptables ownership (#82125) should probably be the approver
:)

/assign @danwinship @thockin

andrewsykim · 2020-10-16T03:09:56Z

pkg/proxy/iptables/proxier.go

+	table utiliptables.Table
+	chain utiliptables.Chain
+}{
+	{utiliptables.TableNAT, KubeMarkMasqChain},


This should be KubeMarkDropChain?

andrewsykim · 2020-10-16T03:10:02Z

pkg/proxy/ipvs/proxier.go

+	table utiliptables.Table
+	chain utiliptables.Chain
+}{
+	{utiliptables.TableNAT, KubeMarkMasqChain},


This should be KubeMarkDropChain?

Oops, sorry

danwinship · 2020-10-16T11:53:29Z

/lgtm
/approve

k8s-ci-robot · 2020-10-16T11:53:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, Lion-Wei

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/proxy/OWNERS~~ [danwinship]

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

fejta-bot · 2020-10-16T14:07:00Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot · 2020-10-16T15:47:23Z

@Lion-Wei: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-integration	`1f7ea16`	link	`/test pull-kubernetes-integration`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

cmluciano · 2020-10-16T15:53:27Z

/retest

k8s-ci-robot requested review from danwinship and m1093782566 September 7, 2020 11:28

k8s-ci-robot added area/ipvs sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 7, 2020

andrewsykim reviewed Sep 7, 2020

View reviewed changes

k8s-ci-robot assigned andrewsykim and uablrek Sep 7, 2020

Lion-Wei force-pushed the lb-fw branch from 1d225e1 to 1a87e3f Compare September 10, 2020 02:36

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 10, 2020

andrewsykim reviewed Sep 11, 2020

View reviewed changes

Lion-Wei force-pushed the lb-fw branch from 1a87e3f to a45e8e9 Compare September 11, 2020 01:35

andrewsykim reviewed Sep 11, 2020

View reviewed changes

Lion-Wei force-pushed the lb-fw branch from a45e8e9 to 6a41561 Compare September 11, 2020 09:42

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 11, 2020

Lion-Wei force-pushed the lb-fw branch from 6a41561 to a050b6f Compare September 11, 2020 15:44

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 11, 2020

aojea reviewed Sep 11, 2020

View reviewed changes

k8s-ci-robot assigned aojea Oct 15, 2020

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

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 15, 2020

andrewsykim reviewed Oct 15, 2020

View reviewed changes

k8s-ci-robot assigned danwinship and thockin Oct 15, 2020

Lion-Wei force-pushed the lb-fw branch from 22f96da to 1b5bcea Compare October 16, 2020 02:54

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2020

Lion-Wei force-pushed the lb-fw branch from 1b5bcea to c4617d5 Compare October 16, 2020 03:06

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 16, 2020

andrewsykim reviewed Oct 16, 2020

View reviewed changes

kube-proxy ensure KUBE-MARK-DROP exist but not modify their rules

1f7ea16

Lion-Wei force-pushed the lb-fw branch from c4617d5 to 1f7ea16 Compare October 16, 2020 06:53

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2020

k8s-ci-robot merged commit 5fc8069 into kubernetes:master Oct 16, 2020

k8s-ci-robot added this to the v1.20 milestone Oct 16, 2020

cmluciano mentioned this pull request Oct 16, 2020

loadBalancerSourceRanges not properly applied using IPVS #88698

Closed

aojea mentioned this pull request Oct 26, 2020

Add unit tests for iptables rules in IPVS proxier #95784

Closed

	if _, err := proxier.iptables.EnsureChain(ch.table, ch.chain); err != nil {
	klog.Errorf("Failed to ensure that %s chain %s exists: %v", ch.table, ch.chain, err)
	return
	}
	if ch.table == utiliptables.TableNAT {
	if chain, ok := existingNATChains[ch.chain]; ok {
	writeBytesLine(proxier.natChains, chain)
	} else {
	writeLine(proxier.natChains, utiliptables.MakeChainLine(kubePostroutingChain))
	}
	} else {
	if chain, ok := existingFilterChains[KubeForwardChain]; ok {
	writeBytesLine(proxier.filterChains, chain)
	} else {
	writeLine(proxier.filterChains, utiliptables.MakeChainLine(KubeForwardChain))
	}
	}

		@@ -1862,6 +1852,12 @@ func (proxier *Proxier) createAndLinkeKubeChain() {
		existingNATChains := proxier.getExistingChains(proxier.iptablesData, utiliptables.TableNAT)

		// Make sure we keep stats for the top-level chains

Fix failing test "Services should only allow access from service loadbalancer source ranges" #94591

Fix failing test "Services should only allow access from service loadbalancer source ranges" #94591

Conversation

Lion-Wei commented Sep 7, 2020

Choose a reason for hiding this comment

aojea Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uablrek Sep 7, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Sep 7, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Oct 1, 2020 • edited Loading

aojea commented Oct 15, 2020

Lion-Wei commented Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Oct 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Oct 16, 2020

k8s-ci-robot commented Oct 16, 2020

fejta-bot commented Oct 16, 2020

k8s-ci-robot commented Oct 16, 2020

cmluciano commented Oct 16, 2020

aojea Sep 7, 2020 •

edited

Loading

uablrek Sep 7, 2020 •

edited

Loading

aojea commented Oct 1, 2020 •

edited

Loading

Lion-Wei commented Oct 15, 2020 •

edited

Loading