Install a REJECT rule for nodeport with no backend #43415

thockin · 2017-03-20T22:57:43Z

Rather than actually accepting the connection, REJECT. This will avoid
CLOSE_WAIT.

Fixes #43212

@justinsb @felipejfc @Spiddy

k8s-reviewable · 2017-03-20T22:58:49Z

This change is

justinsb

Some nits, but I haven't figured out how >= 0 is correct yet (but e2e didn't fail catastrophically, so...)

justinsb · 2017-03-21T00:47:47Z

pkg/proxy/iptables/proxier.go

@@ -1108,6 +1108,18 @@ func (proxier *Proxier) syncProxyRules() {
 				// Currently we only create it for loadbalancers (#33586).
 				writeLine(natRules, append(args, "-j", string(svcXlbChain))...)
 			}
+
+			// If the service has no endpoints then reject packets.
+			if len(proxier.endpointsMap[svcName]) >= 0 {


Do you mean == 0 ?

dangit, vestigial from something else I tried.

justinsb · 2017-03-21T00:48:03Z

pkg/proxy/iptables/proxier.go

+			if len(proxier.endpointsMap[svcName]) >= 0 {
+				writeLine(filterRules,
+					"-A", string(kubeServicesChain),
+					"-m", "comment", "--comment", fmt.Sprintf(`"%s has no endpoints"`, svcName.String()),


The comment here is inconsistent with >= 0

Just wasn't sure which one was wrong :-)

justinsb · 2017-03-21T00:53:25Z

pkg/proxy/iptables/proxier.go

@@ -1108,6 +1108,18 @@ func (proxier *Proxier) syncProxyRules() {
 				// Currently we only create it for loadbalancers (#33586).
 				writeLine(natRules, append(args, "-j", string(svcXlbChain))...)
 			}
+


We could do the "no endpoints" check before writing the MARK/SVC or XLB rules. But I presume we want to keep the rules less volatile?

I have an idea for future refactoring that will make this nice, so I made the smallest change I could.

You're saying you have a cunning plan? 👍

justinsb · 2017-03-21T00:56:22Z

pkg/proxy/iptables/proxier.go

@@ -1108,6 +1108,18 @@ func (proxier *Proxier) syncProxyRules() {
 				// Currently we only create it for loadbalancers (#33586).
 				writeLine(natRules, append(args, "-j", string(svcXlbChain))...)
 			}
+
+			// If the service has no endpoints then reject packets.


It took me a while to figure out that this is the analog of the nodeport entry chain below (e.g. I was wondering why you were changing kubeServicesChain and not the NodePort chain). Expanding the comment to that effect may be helpful.

e.g.

If the service has no endpoints then reject packets. We add a rule to the service chain that is the equivalent of the "tail-call to nodeports chain", but immediately rejects the packet.

will add words

thockin · 2017-03-21T03:34:35Z

new push is up

justinsb · 2017-03-21T04:21:18Z

Sorry ... I see a new commit, but I don't see anything different?

justinsb · 2017-03-21T04:22:36Z

BTW any idea why all the e2e tests aren't failing? Aren't we blocking all NodePorts?

Rather than actually accepting the connection, REJECT. This will avoid CLOSE_WAIT.

thockin · 2017-03-21T04:40:56Z

I botched the amend. Fixed.

It didn't trigger the e2es because the nat PREROUTING table triggers first, so the original packet never hits the filter table.

justinsb · 2017-03-21T05:11:34Z

/lgtm

Nice - and the explanation for how the e2e tests are not blowing up makes sense now you've explained it!

k8s-github-robot · 2017-03-21T05:11:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, thockin

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

~~pkg/proxy/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-03-21T06:22:21Z

Automatic merge from submit-queue

justinsb · 2017-03-30T13:39:11Z

Going to propose this for cherry picking to 1.4 and 1.5.

justinsb · 2017-04-03T02:35:29Z

So I'm afraid I think this is still happening in 1.6.0. I can scale down a deployment to size 0 and I see CLOSE_WAIT accumulating ~~, and I don't see any has no endpoints rule, either nodeport or clusterip.~~

~~I think that the problem is maybe that we don't set activeNATChains, but I'm trying to get a test to fail.~~

justinsb · 2017-04-03T02:49:17Z

Edit: It does have the drop rules, in iptables -t filter (not -t nat). But the CLOSE_WAIT sockets are still accumulating. Digging...

justinsb · 2017-04-03T02:59:14Z

Figured it out I think - put details into #43969

ketkulka · 2017-04-13T20:25:04Z

@justinsb @thockin the reject rules should also be installed for externalIPs and ports in addition to service IPs and node port. I am hitting a case where reject with icmp rule get installed for destination=service ip but not for destination=externalIPs. And when no pods are present; kube-proxy accepts the connection coming via externalIPs.

thockin · 2017-04-14T00:26:55Z

Can you file a new bug with a full repro, please?

…

On Thu, Apr 13, 2017 at 1:25 PM, Ketan Kulkarni ***@***.***> wrote: @thockin <https://github.com/thockin> the reject rules should also be installed for externalIPs and ports in addition to service IPs and node port. I am hitting a case where reject with icmp rule get installed for destination=service ip but not for destination=externalIPs. And when no pods are present; kube-proxy accepts the connection coming via externalIPs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#43415 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVLTrp-qJuRx6oJJDvNbwzwAet5H-ks5rvoS1gaJpZM4MjHzg> .

ketkulka · 2017-04-14T21:45:14Z

Thanks @thockin .
Opened #44516 to track it. I can give a shot for the patch but I must admit I have no way of verifying all cases.

Automatic merge from submit-queue Reject Rules for ExternalIP and svc port if no ep - Install ICMP Reject Rules for externalIP and svc port if no endpoints are present - Includes Unit Test case - Fixes #44516 **What this PR does / why we need it**: Explained in issue #44516 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # `Fixes #44516` **Special notes for your reviewer**: Similar to #43415 Feedback welcome. Will be happy to improve the patch. Unit Test done and passing. **Release note**: ```release-note ```

thockin added this to the v1.6 milestone Mar 20, 2017

thockin assigned justinsb and dcbw Mar 20, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 20, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Mar 20, 2017

thockin added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Mar 20, 2017

justinsb reviewed Mar 21, 2017

View reviewed changes

thockin force-pushed the fix-nodeport-close-wait branch from b4e4c68 to d49370c Compare March 21, 2017 03:34

Install a REJECT rule for nodeport with no backend

2ec8799

Rather than actually accepting the connection, REJECT. This will avoid CLOSE_WAIT.

thockin force-pushed the fix-nodeport-close-wait branch from d49370c to 2ec8799 Compare March 21, 2017 04:37

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

k8s-github-robot merged commit 1e5fa8f into kubernetes:master Mar 21, 2017

justinsb added the cherrypick-candidate label Mar 30, 2017

This was referenced Mar 30, 2017

Automated cherry pick of #43415 #43858

Closed

Automated cherry pick of #43415 #43859

Closed

justinsb mentioned this pull request Apr 3, 2017

NodePort rejection should be on input chain #43969

Closed

ketkulka mentioned this pull request Apr 14, 2017

Need ICMP reject rules for externalIPs and port as well #44516

Closed

ketkulka mentioned this pull request Apr 17, 2017

Reject Rules for ExternalIP and svc port if no ep #44547

Merged

justinsb mentioned this pull request May 11, 2017

etcd cluster losing quorum, causing cluster outage kubernetes/kops#2235

Closed

k8s-cherrypick-bot removed the cherrypick-candidate label Jul 31, 2017

thockin deleted the fix-nodeport-close-wait branch August 14, 2019 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install a REJECT rule for nodeport with no backend #43415

Install a REJECT rule for nodeport with no backend #43415

thockin commented Mar 20, 2017

k8s-reviewable commented Mar 20, 2017

justinsb left a comment

justinsb Mar 21, 2017

thockin Mar 21, 2017

justinsb Mar 21, 2017

thockin Mar 21, 2017

justinsb Mar 21, 2017

justinsb Mar 21, 2017

thockin Mar 21, 2017

justinsb Mar 21, 2017

justinsb Mar 21, 2017 •

edited

thockin Mar 21, 2017

thockin commented Mar 21, 2017

justinsb commented Mar 21, 2017

justinsb commented Mar 21, 2017

thockin commented Mar 21, 2017

justinsb commented Mar 21, 2017

k8s-github-robot commented Mar 21, 2017

k8s-github-robot commented Mar 21, 2017

justinsb commented Mar 30, 2017

justinsb commented Apr 3, 2017 •

edited

justinsb commented Apr 3, 2017

justinsb commented Apr 3, 2017

ketkulka commented Apr 13, 2017 •

edited

thockin commented Apr 14, 2017 via email

ketkulka commented Apr 14, 2017

Install a REJECT rule for nodeport with no backend #43415

Install a REJECT rule for nodeport with no backend #43415

Conversation

thockin commented Mar 20, 2017

k8s-reviewable commented Mar 20, 2017

justinsb left a 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

justinsb Mar 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 21, 2017

justinsb commented Mar 21, 2017

justinsb commented Mar 21, 2017

thockin commented Mar 21, 2017

justinsb commented Mar 21, 2017

k8s-github-robot commented Mar 21, 2017

k8s-github-robot commented Mar 21, 2017

justinsb commented Mar 30, 2017

justinsb commented Apr 3, 2017 • edited

justinsb commented Apr 3, 2017

justinsb commented Apr 3, 2017

ketkulka commented Apr 13, 2017 • edited

thockin commented Apr 14, 2017 via email

ketkulka commented Apr 14, 2017

justinsb Mar 21, 2017 •

edited

justinsb commented Apr 3, 2017 •

edited

ketkulka commented Apr 13, 2017 •

edited