Add --ipvs-exclude-cidrs flag to kube-proxy. #62083

rramkumar1 · 2018-04-03T22:55:16Z

What this PR does / why we need it:
Add a flag to kube-proxy called --ipvs-exclude-cidrs. This flag allows a user to specify a list of CIDR ranges that should not be included in the cleanup of IPVS rules.

Fixes: #59507

Release note:

Use --ipvs-exclude-cidrs to specify a list of CIDR's which the IPVS proxier should not touch when cleaning up IPVS rules.

/assign @m1093782566

tengqm · 2018-04-04T02:32:42Z

pkg/proxy/ipvs/proxier.go

+	for cs := range currentServices {
+		svc := currentServices[cs]
+		for _, excludedCidr := range proxier.excludeCIDRs {
+			_, net, err := net.ParseCIDR(excludedCidr)


bad naming of variable ... conflicts with imported package name

tengqm · 2018-04-04T02:33:54Z

pkg/proxy/ipvs/proxier.go

+			}
+			// Only remove if the service VIP is not in the list of CIDR's to exclude
+			// and if it was not processed in the latest sync loop.
+			if net.Contains(svc.Address) && !activeServices[cs] {


net.Contains() or !net.Contains() ?

Good catch!

rramkumar1 · 2018-04-05T17:18:25Z

/assign @m1093782566

rramkumar1 · 2018-04-05T20:40:39Z

/cc @pires @GoelDeepak

k8s-ci-robot · 2018-04-05T20:40:41Z

@rramkumar1: GitHub didn't allow me to request PR reviews from the following users: GoelDeepak.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @pires @GoelDeepak

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.

pires · 2018-04-06T08:40:12Z

pkg/proxy/apis/kubeproxyconfig/validation/validation.go

+	for i := range excludeCIDRs {
+		if _, _, err := net.ParseCIDR(excludeCIDRs[i]); err != nil {
+			allErrs = append(allErrs, field.Invalid(fldPath, excludeCIDRs, "must be a valid IP block"))
+			break


I think it would be a good UX if all CIDRs were validated and all that are invalid were reported, instead of reporting the first invalid and breaking.

pires · 2018-04-06T09:00:24Z

pkg/proxy/ipvs/proxier.go

+			// make sure it does not fall within an excluded CIDR range.
+			okayToDelete := true
+			for _, excludedCIDR := range proxier.excludeCIDRs {
+				_, n, err := net.ParseCIDR(excludedCIDR)


Why are we validating this again? Isn't it supposed to be validated during configuration check?

If we can't ignore the error here, then I'd say we can skip configuration validation completely and just have the validation happen here.

The intent was not validation but rather to convert the string representation of the CIDR to net struct. I guess I could remove the error check since we know from the previous validation that no error is possible? Does that sound okay?

m1093782566 · 2018-04-09T15:07:53Z

pkg/proxy/apis/kubeproxyconfig/validation/validation.go

+	allErrs := field.ErrorList{}
+
+	for i := range excludeCIDRs {
+		if _, _, err := net.ParseCIDR(excludeCIDRs[i]); err != nil {


Single IP should be valid as well?

Single IP as in x.x.x.x/32 is a valid CIDR.

pires · 2018-04-09T15:20:52Z

pkg/proxy/ipvs/proxier_test.go

@@ -2393,11 +2392,93 @@ func Test_syncService(t *testing.T) {
 	}
 }

+<<<<<<< HEAD


pires · 2018-04-09T15:21:01Z

pkg/proxy/ipvs/proxier_test.go

 func buildFakeProxier(nodeIP []net.IP) (*iptablestest.FakeIPTables, *Proxier) {
 	ipt := iptablestest.NewFake()
 	ipvs := ipvstest.NewFake()
 	ipset := ipsettest.NewFake(testIPSetVersion)
 	return ipt, NewFakeProxier(ipt, ipvs, ipset, nil)
+=======


Bad rebase :/

pires · 2018-04-09T15:21:20Z

pkg/proxy/ipvs/proxier_test.go

+			t.Errorf("Expected ipvs5 to be removed after cleanup. It still remains")
+		}
+	}
+>>>>>>> Add --ipvs-exclude-cidrs flag to kube-proxy.


rramkumar1 · 2018-04-09T18:06:54Z

@m1093782566 Seems like the test failures for pull-kubernetes-bazel-test are being caused by something I did not touch.

pires · 2018-04-09T20:43:10Z

pkg/proxy/ipvs/proxier_test.go

-		nodePortLocalSetUDP: NewIPSet(ipset, KubeNodePortLocalSetUDP, utilipset.BitmapPort, false),
-		nodePortSetUDP:      NewIPSet(ipset, KubeNodePortSetUDP, utilipset.BitmapPort, false),
-		nodePortAddresses:   make([]string, 0),
-		networkInterfacer:   proxyutiltest.NewFakeNetwork(),


Why did this change? Maybe this is where the test break comes from.

pires · 2018-04-09T20:43:40Z

pkg/proxy/ipvs/proxier.go

+			// make sure it does not fall within an excluded CIDR range.
+			okayToDelete := true
+			for _, excludedCIDR := range proxier.excludeCIDRs {
+				// Any validation of this CIDR already should have occured.


It seems that verify steps now include spellchecking.

W0409 19:58:08.458] pkg/proxy/ipvs/proxier.go:1474:55: "occured" is a misspelling of "occurred" I0409 19:58:09.113] +++ exit code: 1 I0409 19:58:09.117] +++ error: 1 I0409 19:58:09.235] FAILED verify-spelling.sh 4s

rramkumar1 · 2018-04-12T16:12:11Z

@pires Fixed the test failures. Any more comments on your end?

rramkumar1 · 2018-04-13T03:20:10Z

@m1093782566 Any comments?

m1093782566 · 2018-04-13T03:27:52Z

LGTM except one comment.

m1093782566 · 2018-04-13T06:19:47Z

cmd/kube-proxy/app/server.go

@@ -149,6 +149,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
 	fs.DurationVar(&o.config.IPTables.MinSyncPeriod.Duration, "iptables-min-sync-period", o.config.IPTables.MinSyncPeriod.Duration, "The minimum interval of how often the iptables rules can be refreshed as endpoints and services change (e.g. '5s', '1m', '2h22m').")
 	fs.DurationVar(&o.config.IPVS.SyncPeriod.Duration, "ipvs-sync-period", o.config.IPVS.SyncPeriod.Duration, "The maximum interval of how often ipvs rules are refreshed (e.g. '5s', '1m', '2h22m').  Must be greater than 0.")
 	fs.DurationVar(&o.config.IPVS.MinSyncPeriod.Duration, "ipvs-min-sync-period", o.config.IPVS.MinSyncPeriod.Duration, "The minimum interval of how often the ipvs rules can be refreshed as endpoints and services change (e.g. '5s', '1m', '2h22m').")
+	fs.StringSliceVar(&o.config.IPVS.ExcludeCIDRs, "ipvs-exclude-cidrs", o.config.IPVS.ExcludeCIDRs, "A list of CIDR's which the ipvs proxier should not touch when cleaning up IPVS rules.")


Is that a comma separated list? Comment, please.

pires · 2018-04-13T16:58:09Z

LGTM as well but @m1093782566 comment is spot on.

rramkumar1 · 2018-04-13T17:11:08Z

Done.
/assign @thockin

rramkumar1 · 2018-04-13T17:17:28Z

/retest

pires · 2018-04-15T16:02:44Z

/lgtm

Lion-Wei · 2018-04-24T06:20:03Z

/test pull-kubernetes-integration

thockin · 2018-04-24T15:28:08Z

/approve

k8s-ci-robot · 2018-04-24T15:28:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pires, rramkumar1, 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:

~~cmd/OWNERS~~ [thockin]
~~pkg/proxy/apis/kubeproxyconfig/OWNERS~~ [thockin]
~~pkg/proxy/ipvs/OWNERS~~ [thockin]

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

rramkumar1 · 2018-04-24T15:44:53Z

/test pull-kubernetes-integration

rramkumar1 · 2018-04-24T16:19:17Z

/test pull-kubernetes-integration

k8s-github-robot · 2018-04-24T17:10:41Z

/test all [submit-queue is verifying that this PR is safe to merge]

rramkumar1 · 2018-04-24T17:31:57Z

/test pull-kubernetes-e2e-kops-aws

k8s-github-robot · 2018-04-24T18:13:13Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

voidengineer · 2018-04-27T06:20:36Z

It would be great if this could be cherrypicked into v1.10.x.

pires · 2018-04-27T07:24:48Z

This is a change in behavior, so I don't think that is possible.

k8s-ci-robot requested review from brendandburns, danwinship and thockin April 3, 2018 22:55

tengqm reviewed Apr 4, 2018

View reviewed changes

rramkumar1 force-pushed the ipvs-exclude-cidrs-flag branch 2 times, most recently from fa7db76 to 22943bf Compare April 5, 2018 17:00

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 Apr 5, 2018

rramkumar1 force-pushed the ipvs-exclude-cidrs-flag branch 2 times, most recently from 95109c6 to 516b354 Compare April 5, 2018 17:06

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 5, 2018

rramkumar1 changed the title ~~Add --ipvs-exclude-cidrs flag to kube-proxy. [WIP]~~ Add --ipvs-exclude-cidrs flag to kube-proxy. Apr 5, 2018

k8s-ci-robot assigned m1093782566 Apr 5, 2018

k8s-ci-robot requested a review from pires April 5, 2018 20:40

pires reviewed Apr 6, 2018

View reviewed changes

rramkumar1 force-pushed the ipvs-exclude-cidrs-flag branch from 516b354 to 836fbf0 Compare April 6, 2018 15:48

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

rramkumar1 force-pushed the ipvs-exclude-cidrs-flag branch from 836fbf0 to 39bdd21 Compare April 9, 2018 14:56

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2018

m1093782566 reviewed Apr 9, 2018

View reviewed changes

pires suggested changes Apr 9, 2018

View reviewed changes

rramkumar1 force-pushed the ipvs-exclude-cidrs-flag branch from 39bdd21 to 7bae3f6 Compare April 9, 2018 15:35

rramkumar1 force-pushed the ipvs-exclude-cidrs-flag branch from 7bae3f6 to ac074f9 Compare April 9, 2018 19:35

pires suggested changes Apr 9, 2018

View reviewed changes

rramkumar1 force-pushed the ipvs-exclude-cidrs-flag branch 2 times, most recently from 4afe2b9 to b063079 Compare April 12, 2018 15:57

m1093782566 reviewed Apr 13, 2018

View reviewed changes

Add --ipvs-exclude-cidrs flag to kube-proxy.

056ae44

rramkumar1 force-pushed the ipvs-exclude-cidrs-flag branch from b063079 to 056ae44 Compare April 13, 2018 17:09

k8s-ci-robot assigned thockin Apr 13, 2018

k8s-ci-robot assigned pires Apr 15, 2018

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2018

k8s-github-robot merged commit c0d1ab8 into kubernetes:master Apr 24, 2018

Add --ipvs-exclude-cidrs flag to kube-proxy. #62083

Add --ipvs-exclude-cidrs flag to kube-proxy. #62083

Conversation

rramkumar1 commented Apr 3, 2018 • edited

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

rramkumar1 commented Apr 5, 2018

rramkumar1 commented Apr 5, 2018

k8s-ci-robot commented Apr 5, 2018

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

rramkumar1 commented Apr 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rramkumar1 commented Apr 12, 2018

rramkumar1 commented Apr 13, 2018

m1093782566 commented Apr 13, 2018 • edited

m1093782566 Apr 13, 2018 • edited

Choose a reason for hiding this comment

pires commented Apr 13, 2018

rramkumar1 commented Apr 13, 2018

rramkumar1 commented Apr 13, 2018

pires commented Apr 15, 2018

Lion-Wei commented Apr 24, 2018

thockin commented Apr 24, 2018

k8s-ci-robot commented Apr 24, 2018

rramkumar1 commented Apr 24, 2018

rramkumar1 commented Apr 24, 2018

k8s-github-robot commented Apr 24, 2018

rramkumar1 commented Apr 24, 2018

k8s-github-robot commented Apr 24, 2018

voidengineer commented Apr 27, 2018

pires commented Apr 27, 2018

rramkumar1 commented Apr 3, 2018 •

edited

rramkumar1 commented Apr 9, 2018 •

edited

m1093782566 commented Apr 13, 2018 •

edited

m1093782566 Apr 13, 2018 •

edited