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

ketkulka · 2017-04-17T09:30:35Z

Install ICMP Reject Rules for externalIP and svc port
if no endpoints are present
Includes Unit Test case
Fixes Need ICMP reject rules for externalIPs and port as well #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:

k8s-reviewable · 2017-04-17T09:30:42Z

This change is

k8s-ci-robot · 2017-04-17T09:30:43Z

Hi @ketkulka. 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 @k8s-bot 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.

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.

ketkulka · 2017-04-17T17:35:55Z

/approve @bprashanth @justinsb @thockin

thockin · 2017-04-17T19:13:27Z

@k8s-bot ok to test

thockin

We talked about adding a utility KUBE-MARK-REJECT rule (like KUBE-MARK-DROP) and simply inserting that into the service-chain, which would simplify and catch all these cases.

@freehan did we kill that idea?

thockin · 2017-04-17T19:17:47Z

pkg/proxy/iptables/proxier.go

@@ -1017,6 +1017,20 @@ func (proxier *Proxier) syncProxyRules(reason syncReason) {
 			// Allow traffic bound for external IPs that happen to be recognized as local IPs to stay local.
 			// This covers cases like GCE load-balancers which get added to the local routing table.
 			writeLine(natRules, append(dstLocalOnlyArgs, "-j", string(svcChain))...)
+
+			// If the service has no endpoints then reject packets coming from externalIP


fixed in new version

thockin · 2017-04-17T19:18:07Z

pkg/proxy/iptables/proxier.go

+			if len(newEndpoints[svcName]) == 0 {
+				writeLine(filterRules,
+					"-A", string(kubeServicesChain),
+					"-m", "comment", "--comment", fmt.Sprintf(`"%s has no endpoints from externalIPs"`, svcNameString),


s/from externalIPs// - simpler is better

fixed in new version

thockin · 2017-04-17T19:19:25Z

pkg/proxy/iptables/proxier.go

+				writeLine(filterRules,
+					"-A", string(kubeServicesChain),
+					"-m", "comment", "--comment", fmt.Sprintf(`"%s has no endpoints from externalIPs"`, svcNameString),
+					"-m", "addrtype", "--dst-type", "LOCAL",


why are you checking addrtype?

removed addrtype. fixed in new version

cblecker · 2017-04-17T20:03:15Z

@ketkulka:

I0417 12:41:49.583] Verifying hack/make-rules/../../hack/verify-gofmt.sh
I0417 12:42:02.606] diff ./pkg/proxy/iptables/proxier_test.go gofmt/./pkg/proxy/iptables/proxier_test.go
I0417 12:42:02.606] --- /tmp/gofmt965435796	2017-04-17 12:41:50.807482217 -0700
I0417 12:42:02.606] +++ /tmp/gofmt953671651	2017-04-17 12:41:50.807482217 -0700
I0417 12:42:02.606] @@ -775,9 +775,9 @@
I0417 12:42:02.607]  			svc.Spec.ClusterIP = svcIP
I0417 12:42:02.607]  			svc.Spec.ExternalIPs = []string{svcExternalIPs}
I0417 12:42:02.607]  			svc.Spec.Ports = []api.ServicePort{{
I0417 12:42:02.607] -				Name: svcPortName.Port,
I0417 12:42:02.607] -				Port: int32(svcPort),
I0417 12:42:02.607] -				Protocol: api.ProtocolTCP,
I0417 12:42:02.607] +				Name:       svcPortName.Port,
I0417 12:42:02.607] +				Port:       int32(svcPort),
I0417 12:42:02.607] +				Protocol:   api.ProtocolTCP,
I0417 12:42:02.607]  				TargetPort: intstr.FromInt(svcPort),
I0417 12:42:02.608]  			}}
I0417 12:42:02.608]  		}),
I0417 12:42:02.608] FAILED   hack/make-rules/../../hack/verify-gofmt.sh	13s

Needs a gofmt -w -s pkg/proxy/iptables/proxier_test.go run.

thockin · 2017-04-17T21:55:03Z

/approve

@freehan or @dcbw for final

ketkulka · 2017-04-20T21:27:51Z

ping @freehan @dcbw

freehan · 2017-04-20T22:27:04Z

Thanks! /lgtm

ketkulka · 2017-04-20T23:44:00Z

Thanks for approving. I think the label was not added properly. Thanks Ketan

…

On Apr 20, 2017, at 3:27 PM, Minhan Xia ***@***.***> wrote: Thanks! /lgtm — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44547 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_K0qSprn_iM-iWM3Zed2gbAA4IPwSYks5rx9vYgaJpZM4M_AoK>.

cblecker · 2017-04-20T23:49:46Z

/lgtm
Adding on @freehan's behalf :)

freehan · 2017-04-21T17:37:33Z

@k8s-bot test this

cblecker · 2017-04-21T18:04:33Z

I0421 17:41:49.199] k8s.io/kubernetes/pkg/proxy/iptables/proxier.go:1105: undefined: newEndpoints

ketkulka · 2017-04-21T18:24:21Z

got conflict with 2f25043
will submit a corrected patch.

ketkulka · 2017-04-21T20:38:41Z

uploaded a new patch. hopefully this passes.

freehan · 2017-04-21T21:22:05Z

/lgtm

ketkulka · 2017-04-21T21:38:26Z

Thanks!

cblecker · 2017-04-21T22:50:46Z

@ketkulka I'd make sure you've rebased off master, and that go test ./pkg/proxy/iptables works locally.

ketkulka · 2017-04-21T23:19:51Z

actually i think that's what i did and it passed locally.

git remote add upstream https://github.com/kubernetes/kubernetes.git
git fetch upstream
git rebase upstream/master

i will do it once again.
let me know if i am mistaken

freehan · 2017-04-21T23:33:47Z

@k8s-bot cvm gce e2e test this
@k8s-bot unit test this
@k8s-bot bazel test this

ketkulka · 2017-04-21T23:37:27Z

Looking into failures. Will send a new patch. Sorry for the trouble. Looks like I missed one more line in tests.

…

On Apr 21, 2017, at 4:36 PM, k8s-ci-robot ***@***.***> wrote: @ketkulka <https://github.com/ketkulka>: The following test(s) failed: Test name Commit Details Rerun command Jenkins unit/integration b0978b3 <b0978b3> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/44547/pull-kubernetes-unit/27356/> @k8s-bot unit test this Jenkins GCE e2e b0978b3 <b0978b3> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/44547/pull-kubernetes-e2e-gce/27376/> @k8s-bot cvm gce e2e test this Jenkins Bazel Build b0978b3 <b0978b3> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/44547/pull-kubernetes-bazel/14426/> @k8s-bot bazel test this Full PR test history <https://k8s-gubernator.appspot.com/pr/44547>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/ketkulka>. Please help us cut down on flakes by linking to an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-request-commands.md> <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> <https://github.com/kubernetes/test-infra/blob/master/prow/commands.md> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44547 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_K0pY72cBAw2dEEcwYTAliTTgCFKdFks5ryT10gaJpZM4M_AoK>.

- Install ICMP Reject Rules for externalIP and svc port if no endpoints are present - Includes Unit Test case - Fixes kubernetes#44516

ketkulka · 2017-04-21T23:50:11Z

uploaded a new patch. this time on my local test; i saw this OK for iptable test ok k8s.io/kubernetes/pkg/proxy/iptables 0.072s Apologies for too many small fixes.

…

On Apr 21, 2017, at 4:37 PM, Ketan Kulkarni ***@***.***> wrote: Looking into failures. Will send a new patch. Sorry for the trouble. Looks like I missed one more line in tests. > On Apr 21, 2017, at 4:36 PM, k8s-ci-robot ***@***.*** ***@***.***>> wrote: > > @ketkulka <https://github.com/ketkulka>: The following test(s) failed: > > Test name Commit Details Rerun command > Jenkins unit/integration b0978b3 <b0978b3> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/44547/pull-kubernetes-unit/27356/> @k8s-bot unit test this > Jenkins GCE e2e b0978b3 <b0978b3> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/44547/pull-kubernetes-e2e-gce/27376/> @k8s-bot cvm gce e2e test this > Jenkins Bazel Build b0978b3 <b0978b3> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/44547/pull-kubernetes-bazel/14426/> @k8s-bot bazel test this > Full PR test history <https://k8s-gubernator.appspot.com/pr/44547>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/ketkulka>. Please help us cut down on flakes by linking to an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. > > <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-request-commands.md> <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> <https://github.com/kubernetes/test-infra/blob/master/prow/commands.md> > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub <#44547 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_K0pY72cBAw2dEEcwYTAliTTgCFKdFks5ryT10gaJpZM4M_AoK>. >

cblecker · 2017-04-22T00:35:32Z

okay, main tests are passing now, and it looks good locally for me too
/lgtm

k8s-github-robot · 2017-04-22T00:37:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, freehan, ketkulka, 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-04-22T01:58:38Z

Automatic merge from submit-queue

ketkulka · 2017-06-01T19:18:22Z

should this be picked for upcoming release?

cblecker · 2017-06-01T19:20:59Z

@ketkulka This should make it into 1.7

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>. Split KUBE-SERVICES chain to re-shrink the INPUT chain **What this PR does / why we need it**: #43972 added an iptables rule "`-A INPUT -j KUBE-SERVICES`" to make NodePort ICMP rejection work. (Previously the KUBE-SERVICES chain was only run from OUTPUT, not INPUT.) #44547 extended that patch for ExternalIP rejection as well. However, the KUBE-SERVICES chain may potentially have a very large number of ICMP reject rules for plain ClusterIP services (the ones that get run from OUTPUT), and it seems that for some reason the kernel is much more sensitive to the length of the INPUT chain than it is to the length of the OUTPUT chain. So a node that worked fine with kube 1.6 (when KUBE-SERVICES was only run from OUTPUT) might fall over with kube 1.7 (with KUBE-SERVICES being run from both INPUT and OUTPUT). (Specifically, a node with about 5000 ClusterIP reject rules that ran fine with OpenShift 3.6 [kube 1.6] slowed almost to a complete halt with OpenShift 3.7 [kube 1.7].) This PR fixes things by splitting out the "new" part of KUBE-SERVICES (NodePort and ExternalIP reject rules) into a separate KUBE-EXTERNAL-SERVICES chain run from INPUT, and moves KUBE-SERVICES back to being only run from OUTPUT. (So, yes, this assumes that you don't have 5000 NodePort/ExternalIP services, but, if you do, there's not much we can do, since those rules *have* to be run on the INPUT side.) Oh, and I left in the code to clean up the "`-A INPUT -j KUBE-SERVICES`" rule even though we don't generate it any more, so it gets fixed on upgrade. **Release note**: ```release-note Reorganized iptables rules to fix a performance regression on clusters with thousands of services. ``` @kubernetes/sig-network-bugs @kubernetes/rh-networking

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 17, 2017

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 Apr 17, 2017

k8s-github-robot assigned justinsb and bprashanth Apr 17, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Apr 17, 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 Apr 17, 2017

thockin reviewed Apr 17, 2017

View reviewed changes

thockin assigned freehan and unassigned bprashanth Apr 17, 2017

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

ketkulka force-pushed the icmp-reject branch from bd349b6 to 1b1a6a5 Compare April 17, 2017 21:04

thockin unassigned justinsb Apr 17, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2017

k8s-ci-robot assigned cblecker Apr 20, 2017

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

ketkulka force-pushed the icmp-reject branch from 1b1a6a5 to b0978b3 Compare April 21, 2017 20:38

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

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

ketkulka closed this Apr 21, 2017

ketkulka reopened this Apr 21, 2017

Reject Rules for ExternalIP and svc port if no ep

ac7c026

- Install ICMP Reject Rules for externalIP and svc port if no endpoints are present - Includes Unit Test case - Fixes kubernetes#44516

ketkulka force-pushed the icmp-reject branch from b0978b3 to ac7c026 Compare April 21, 2017 23:48

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

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

k8s-github-robot merged commit 12c6b0c into kubernetes:master Apr 22, 2017

danwinship mentioned this pull request Nov 21, 2017

Split KUBE-SERVICES chain to re-shrink the INPUT chain #56164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ketkulka commented Apr 17, 2017

k8s-reviewable commented Apr 17, 2017

k8s-ci-robot commented Apr 17, 2017

ketkulka commented Apr 17, 2017

thockin commented Apr 17, 2017

thockin left a comment

thockin Apr 17, 2017

ketkulka Apr 17, 2017

thockin Apr 17, 2017

ketkulka Apr 17, 2017

thockin Apr 17, 2017

ketkulka Apr 17, 2017

cblecker commented Apr 17, 2017

thockin commented Apr 17, 2017

ketkulka commented Apr 20, 2017

freehan commented Apr 20, 2017

ketkulka commented Apr 20, 2017 via email

cblecker commented Apr 20, 2017

freehan commented Apr 21, 2017

cblecker commented Apr 21, 2017

ketkulka commented Apr 21, 2017

ketkulka commented Apr 21, 2017

freehan commented Apr 21, 2017

ketkulka commented Apr 21, 2017

cblecker commented Apr 21, 2017

ketkulka commented Apr 21, 2017

freehan commented Apr 21, 2017

ketkulka commented Apr 21, 2017 via email

ketkulka commented Apr 21, 2017 via email

cblecker commented Apr 22, 2017

k8s-github-robot commented Apr 22, 2017

k8s-github-robot commented Apr 22, 2017

ketkulka commented Jun 1, 2017

cblecker commented Jun 1, 2017

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

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

Conversation

ketkulka commented Apr 17, 2017

k8s-reviewable commented Apr 17, 2017

k8s-ci-robot commented Apr 17, 2017

ketkulka commented Apr 17, 2017

thockin commented Apr 17, 2017

thockin left a comment

Choose a reason for hiding this comment

thockin Apr 17, 2017

Choose a reason for hiding this comment

ketkulka Apr 17, 2017

Choose a reason for hiding this comment

thockin Apr 17, 2017

Choose a reason for hiding this comment

ketkulka Apr 17, 2017

Choose a reason for hiding this comment

thockin Apr 17, 2017

Choose a reason for hiding this comment

ketkulka Apr 17, 2017

Choose a reason for hiding this comment

cblecker commented Apr 17, 2017

thockin commented Apr 17, 2017

ketkulka commented Apr 20, 2017

freehan commented Apr 20, 2017

ketkulka commented Apr 20, 2017 via email

cblecker commented Apr 20, 2017

freehan commented Apr 21, 2017

cblecker commented Apr 21, 2017

ketkulka commented Apr 21, 2017

ketkulka commented Apr 21, 2017

freehan commented Apr 21, 2017

ketkulka commented Apr 21, 2017

cblecker commented Apr 21, 2017

ketkulka commented Apr 21, 2017

freehan commented Apr 21, 2017

ketkulka commented Apr 21, 2017 via email

ketkulka commented Apr 21, 2017 via email

cblecker commented Apr 22, 2017

k8s-github-robot commented Apr 22, 2017

k8s-github-robot commented Apr 22, 2017

ketkulka commented Jun 1, 2017

cblecker commented Jun 1, 2017