Validate kube-proxy options #53780

m1093782566 · 2017-10-12T08:38:12Z

What this PR does / why we need it:

Validate ipvs proxy options

Which issue this PR fixes : fixes #53852

Special notes for your reviewer:

Release note:

NONE

m1093782566 · 2017-10-12T08:38:27Z

/sig network

/area kube-proxy

frodenas · 2017-10-14T08:11:42Z

pkg/apis/componentconfig/types.go

 // the system's kernel or iptables versions are insufficient, this always falls back to the
 // userspace proxy.
 type ProxyMode string

 const (
 	ProxyModeUserspace ProxyMode = "userspace"
 	ProxyModeIPTables  ProxyMode = "iptables"
+	ProxyModeIPVS      ProxyMode = "ipvs"


For your reference, #53860 also add IPVS and Kernelspace.

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>. validate ipvs scheduler **What this PR does / why we need it**: validate ipvs scheduler options **Which issue this PR fixes**: closes #53975 **Special notes for your reviewer**: It depends on work of #53780. **Release note**: ```release-note NONE ``` /sig network /area kube-proxy

m1093782566 · 2017-10-31T02:32:40Z

/retest

m1093782566 · 2017-11-03T06:17:04Z

/cc @xiangpengzhao for review :)

xiangpengzhao · 2017-11-03T06:32:51Z

@m1093782566 so sorry for not noticing this PR when I trying to improve test coverage for kube-proxy validation in #54848. For the IPVS part, this PR is more reasonable than that one.

m1093782566 · 2017-11-03T06:35:18Z

@xiangpengzhao

Never mind. I think you did good job :)

xiangpengzhao

just two nits. otherwise LGTM.

xiangpengzhao · 2017-11-03T06:47:02Z

cmd/kube-proxy/app/validation_test.go

+	}
+}
+
+func TestValidateKubeProxyIPVSScheduler(t *testing.T) {


This test case can be removed here since it's already added in #54848 :)

No problem.

xiangpengzhao · 2017-11-03T06:48:53Z

cmd/kube-proxy/app/validation.go

+		allErrs = append(allErrs, field.Invalid(fldPath.Child("SyncPeriod"), config.SyncPeriod, "must be greater than 0"))
+	}
+
+	if config.MinSyncPeriod.Duration < 0 {


Do we need to check if MinSyncPeriod > SyncPeriod ?

That's a nice finding!

I think we can check MinSyncPeriod > SyncPeriod in validation although currently it's already checked in proxier.go. Let's change it to see if anyone object to it?

although currently it's already checked in proxier.go

IMO, it's better validating earlier than later :)

m1093782566 · 2017-11-05T09:50:31Z

@xiangpengzhao

Comments are fixed now, PTAL. Thanks!

xiangpengzhao · 2017-11-05T12:12:43Z

cmd/kube-proxy/app/validation.go

@@ -87,6 +87,30 @@ func validateKubeProxyIPTablesConfiguration(config componentconfig.KubeProxyIPTa
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("MinSyncPeriod"), config.MinSyncPeriod, "must be greater than or equal to 0"))
 	}

+	if config.MinSyncPeriod.Duration > config.SyncPeriod.Duration {


why checking this both here and L108?

That's an interesting finding :)

Because both iptables and ipvs proxy has the SyncPeriod and MinSyncPeriod - they don't share the same SyncPeriod and MinSyncPeriod.

We need to validate them both in iptables and ipvs mode.

Cool! I didn't notice this :)

xiangpengzhao

please fix the dup code caused by rebase.

xiangpengzhao · 2017-11-07T09:15:18Z

pkg/apis/componentconfig/types.go

@@ -20,6 +20,191 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

+// ClientConnectionConfiguration contains details for constructing a client.


These should be removed from this file as #53645 is merged.

Fixed. Thanks!

xiangpengzhao · 2017-11-07T09:15:38Z

pkg/proxy/apis/kubeproxyconfig/types.go

@@ -163,6 +163,7 @@ const (
 	ProxyModeUserspace ProxyMode = "userspace"
 	ProxyModeIPTables  ProxyMode = "iptables"
 	ProxyModeIPVS      ProxyMode = "ipvs"
+	ProxyModeIPVS      ProxyMode = "ipvs"


dup of L165

nice catch! Done.

xiangpengzhao · 2017-11-07T09:27:57Z

CI is still unhappy due to

I1107 09:23:00.377] pkg/proxy/apis/kubeproxyconfig/validation/validation.go:97:48: undefined: componentconfig
I1107 09:23:00.378] pkg/proxy/apis/kubeproxyconfig/validation/validation.go:112:56: undefined: componentconfig

I guess this is also introduced by rebase. Sorry for the pain caused by #53645. It hurts a lot of PRs.

xiangpengzhao

just a nit now.

xiangpengzhao · 2017-11-07T09:29:01Z

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

+	return allErrs
+}
+
+func validateKubeProxyIPVSConfiguration(config componentconfig.KubeProxyIPVSConfiguration, fldPath *field.Path) field.ErrorList {


s/componentconfig/kubeproxyconfig
and other places it appears.

m1093782566 · 2017-11-07T14:30:06Z

CI is happy now :)

thockin · 2017-11-14T00:29:20Z

/lgtm
/approve

k8s-github-robot · 2017-11-14T00:30:22Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 53852

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

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

m1093782566 · 2017-11-14T02:20:53Z

/retest

m1093782566 · 2017-11-15T13:07:25Z

/retest

fejta-bot · 2017-11-15T15:47:14Z

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

Review the full test history for this PR.

k8s-github-robot · 2017-11-15T16:45:42Z

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

k8s-ci-robot · 2017-11-15T17:28:12Z

@m1093782566: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	`c7071ed`	link	`/test pull-kubernetes-unit`
pull-kubernetes-e2e-gce	`c7071ed`	link	`/test pull-kubernetes-e2e-gce`

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.

k8s-github-robot · 2017-11-15T17:30:23Z

Automatic merge from submit-queue (batch tested with PRs 53780, 55663, 55321, 52421, 55659). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 12, 2017

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. area/kube-proxy labels Oct 12, 2017

k8s-github-robot assigned therc and dchen1107 Oct 12, 2017

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 12, 2017

k8s-ci-robot assigned thockin and unassigned therc Oct 12, 2017

m1093782566 mentioned this pull request Oct 14, 2017

Validate kube-proxy IPVS configuration #53901

Closed

k8s-ci-robot assigned therc Oct 14, 2017

frodenas reviewed Oct 14, 2017

View reviewed changes

This was referenced Oct 14, 2017

add supported proxymodes in componentconfig #53860

Closed

validate ipvs scheduler #53973

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2017

m1093782566 force-pushed the validate-ipvs branch from 30f94f9 to 84fbc3b Compare October 30, 2017 11:13

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2017

m1093782566 force-pushed the validate-ipvs branch from 84fbc3b to 9bb7eb3 Compare October 30, 2017 12:07

m1093782566 force-pushed the validate-ipvs branch from 9bb7eb3 to 95a0263 Compare November 3, 2017 06:16

k8s-ci-robot requested a review from xiangpengzhao November 3, 2017 06:17

m1093782566 force-pushed the validate-ipvs branch from 95a0263 to 35ef9a5 Compare November 3, 2017 06:41

xiangpengzhao reviewed Nov 3, 2017

View reviewed changes

m1093782566 force-pushed the validate-ipvs branch from 35ef9a5 to 2ad7efe Compare November 3, 2017 08:29

m1093782566 changed the title ~~Validate ipvs proxy options~~ Validate kube-proxy options Nov 3, 2017

xiangpengzhao reviewed Nov 5, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2017

m1093782566 force-pushed the validate-ipvs branch from 2ad7efe to 8cab993 Compare November 7, 2017 09:06

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2017

xiangpengzhao suggested changes Nov 7, 2017

View reviewed changes

m1093782566 force-pushed the validate-ipvs branch from 8cab993 to 6f57ebb Compare November 7, 2017 09:21

k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 7, 2017

xiangpengzhao reviewed Nov 7, 2017

View reviewed changes

try ipset in ipvs proxy mode

c7071ed

m1093782566 force-pushed the validate-ipvs branch from 6f57ebb to c7071ed Compare November 7, 2017 09:34

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

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

k8s-github-robot merged commit 5e17893 into kubernetes:master Nov 15, 2017

gkudra-msft mentioned this pull request Nov 29, 2017

Kubeproxy's Windows proxy modes don't work. #56522

Closed

Validate kube-proxy options #53780

Validate kube-proxy options #53780

Conversation

m1093782566 commented Oct 12, 2017 • edited

m1093782566 commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 commented Oct 31, 2017

m1093782566 commented Nov 3, 2017 • edited

xiangpengzhao commented Nov 3, 2017

m1093782566 commented Nov 3, 2017

xiangpengzhao 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

m1093782566 Nov 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 commented Nov 5, 2017

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

xiangpengzhao 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

xiangpengzhao commented Nov 7, 2017

xiangpengzhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 commented Nov 7, 2017

thockin commented Nov 14, 2017

k8s-github-robot commented Nov 14, 2017

m1093782566 commented Nov 14, 2017

m1093782566 commented Nov 15, 2017

fejta-bot commented Nov 15, 2017

k8s-github-robot commented Nov 15, 2017

k8s-ci-robot commented Nov 15, 2017 • edited

k8s-github-robot commented Nov 15, 2017

m1093782566 commented Oct 12, 2017 •

edited

m1093782566 commented Nov 3, 2017 •

edited

m1093782566 Nov 3, 2017 •

edited

k8s-ci-robot commented Nov 15, 2017 •

edited