Add possibility to disable kube-proxy #2088

brb · 2021-02-24T18:00:51Z

This commit introduces the new kube-proxy mode "none" which is used
to disable kube-proxy when provisioning (via kubeadm init's
skip-phase=addon/kube-proxy).

The motivation for the change is to use Kind for testing Cilium's
kube-proxy replacement [1].

[1]: https://docs.cilium.io/en/v1.9/gettingstarted/kubeproxy-free/

k8s-ci-robot · 2021-02-24T18:00:58Z

Welcome @brb!

It looks like this is your first PR to kubernetes-sigs/kind 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kind has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-02-24T18:00:59Z

Hi @brb. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

This commit introduces the new kube-proxy mode "none" which is used to disable kube-proxy when provisioning (via kubeadm init's skip-phase=addon/kube-proxy). The motivation for the change is to use Kind for testing Cilium's kube-proxy replacement [1]. [1]: https://docs.cilium.io/en/v1.9/gettingstarted/kubeproxy-free/ Signed-off-by: Martynas Pumputis <[email protected]>

aojea · 2021-02-24T18:26:10Z

/ok-to-test
I'm +1 on this,
defer to Ben to review the mechanics of introducing the skip phases changed

neolit123 · 2021-02-24T18:37:20Z

pkg/internal/apis/config/types.go

@@ -167,6 +167,8 @@ const (
 	IPTablesMode ProxyMode = "iptables"
 	// IPVSMode sets ProxyMode to iptables
 	IPVSMode ProxyMode = "ipvs"
+	// NoneMode disables kube-proxy
+	NoneMode ProxyMode = "none"


what is the alternative value of this field?
if "" implies kube-proxy mode this should be reflected in a comment above.

for the API change, as is, this could be useful if one day kind decides to implement it's own proxy ALA Cilium.
adding another ProxyMode.

but if there are no plans for that and if the demand to skip kube-proxy is not high, the better alternative would have been to configure this via kubeadm's v1beta3 ClusterConfiguration.

of course, a couple of problems with that are:

kubeadm v1beta3 is not ready yet, but this exact skip phases feature is tracked here

it would only work with kubeadm / k8s latest

a more flexible feature in kind would be to allow passing extra args to the kubeadm init/join commands the same way kubeadm allows passing such to the kubelet / control-plane. as per how cobra, go's CLI works, subsequent flags with the same key would override prior ones.

what is the alternative value of this field?
if "" implies kube-proxy mode this should be reflected in a comment above.

"" implies iptables mode.

but if there are no plans for that and if the demand to skip kube-proxy is not high, the better alternative would have been to configure this via kubeadm's v1beta3 ClusterConfiguration.

Once it's ready, we can change the way we disable kube-proxy.

but if there are no plans for that and if the demand to skip kube-proxy is not high, the better alternative would have been to configure this via kubeadm's v1beta3 ClusterConfiguration.

If we need to pass more flags to kubeadm, then it makes sense. At the moment, it's only one flag.

for the API of this field I think it should be

"iptables" : default value

"ipvs"

"none"

if there are other proxies they should provide their installation, same we are doing with the cni

a more flexible feature in kind would be to allow passing extra args to the kubeadm init/join commands the same way kubeadm allows passing such to the kubelet / control-plane. as per how cobra, go's CLI works, subsequent flags with the same key would override prior ones.

I think we already discussed this in another issue 🤔

a more flexible feature in kind would be to allow passing extra args to the kubeadm init/join commands the same way kubeadm allows passing such to the kubelet / control-plane. as per how cobra, go's CLI works, subsequent flags with the same key would override prior ones.

kubelet etc though are moving away from this in favor of config, which also seems to be the case for kubeadm (phases in v1beta3), I don't think we should promote reliance on dependency flags versus structured configuration.

flags are indeed the riskier option.

brb · 2021-02-24T18:42:45Z

The CI failure seems to be unrelated:

ERROR: An error occurred during the fetch of repository 'debian-iptables-amd64':
   Pull command failed: 2021/02/24 18:31:24 Running the Image Puller to pull images from a Docker Registry...
2021/02/24 18:31:25 Image pull was unsuccessful: unable to save remote image k8s.gcr.io/build-image/debian-iptables@sha256:e30919918299988b318f0208e7fd264dee21a6be9d74bbd9f7fc15e78eade9b4: unable to write image layers: unable to write image layer: unable to write the contents of layer 0 to /bazel-scratch/.cache/bazel/_bazel_root/cae228f2a89ef5ee47c2085e441a3561/external/debian-iptables-amd64/image/000.tar.gz: read tcp 10.60.213.98:43602->172.217.214.128:443: read: connection reset by peer
 (/bazel-scratch/.cache/bazel/_bazel_root/cae228f2a89ef5ee47c2085e441a3561/external/go_puller_linux/file/downloaded -directory /bazel-scratch/.cache/bazel/_bazel_root/cae228f2a89ef5ee47c2085e441a3561/external/debian-iptables-amd64/image -os linux -os-version  -os-features  -architecture amd64 -variant  -features  -name k8s.gcr.io/build-image/debian-iptables@sha256:e30919918299988b318f0208e7fd264dee21a6be9d74bbd9f7fc15e78eade9b4)

BenTheElder

Ordinarily I'd ask that a feature request issue be filed first per our contributing guide 1

This feature seems relatively straightforward and well contained within the existing API surface though.

In concept this seems reasonable enough to support, and difficult to work around cleanly without it 👍

Some nits (see comments)

pkg/internal/apis/config/types.go

BenTheElder · 2021-02-24T18:54:57Z

pkg/internal/apis/config/types.go

@@ -167,6 +167,8 @@ const (
 	IPTablesMode ProxyMode = "iptables"
 	// IPVSMode sets ProxyMode to iptables
 	IPVSMode ProxyMode = "ipvs"
+	// NoneMode disables kube-proxy
+	NoneMode ProxyMode = "none"


a more flexible feature in kind would be to allow passing extra args to the kubeadm init/join commands the same way kubeadm allows passing such to the kubelet / control-plane. as per how cobra, go's CLI works, subsequent flags with the same key would override prior ones.

kubelet etc though are moving away from this in favor of config, which also seems to be the case for kubeadm (phases in v1beta3), I don't think we should promote reliance on dependency flags versus structured configuration.

This changes {IPTables,IPVS,None}Mode => {IPTables,IPVS,None}ProxyMode for the sake of readability. Suggested-by: Benjamin Elder <[email protected]> Signed-off-by: Martynas Pumputis <[email protected]>

brb · 2021-02-25T10:14:22Z

@BenTheElder I addressed your comment. PTAL.

brb · 2021-02-25T17:29:39Z

Both CI failures above look unrelated to my changes.

BenTheElder

/lgtm
/approve
thanks for your patience, it's been super busy with code freeze in kubernetes and kubernetes/kubernetes#99561

k8s-ci-robot · 2021-03-03T07:48:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, brb

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:

~~OWNERS~~ [BenTheElder]

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

aojea · 2021-03-03T09:33:00Z

dockerhub rate limits on github actions

"kindest/node:v1.20.2@sha256:8f7ea6e7642c0da54f04a7ee10431549c0257315b3a634f6ef2fecaaedb19bab": command "docker pull kindest/node:v1.20.2@sha256:8f7ea6e7642c0da54f04a7ee10431549c0257315b3a634f6ef2fecaaedb19bab" failed with error: exit status 1

Command Output: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

flake on 1.19
connection reset on 1.20 read tcp 10.61.33.216:53166->108.177.112.128:443: read: connection reset by peer
/retest

aojea · 2021-03-03T10:17:34Z

@BenTheElder

Kubernetes e2e suite: [sig-network] Service endpoints latency should not be very high [Conformance] expand_less | 1m22s
/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/framework.go:597 Mar 3 10:03:07.425: Tail (99 percentile) latency should be less than 50s 50, 90, 99 percentiles: 755.062239ms 3.879176932s 50.4752086s

50seconds latency for endpoints is not a healthy environment to run e2e tests
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/2088/pull-kind-e2e-kubernetes-1-19/1367045340850556928
/retest

k8s-ci-robot · 2021-03-03T10:23:19Z

@brb: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kind-e2e-kubernetes-1-20	`abf42e2`	link	`/test pull-kind-e2e-kubernetes-1-20`

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-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2021

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 Feb 24, 2021

k8s-ci-robot requested review from aojea and krzyzacy February 24, 2021 18:00

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 24, 2021

brb mentioned this pull request Feb 24, 2021

ci: Run smoke tests in GH actions with kube-proxy-replacement=strict cilium/cilium#14957

Closed

5 tasks

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 24, 2021

neolit123 reviewed Feb 24, 2021

View reviewed changes

BenTheElder reviewed Feb 24, 2021

View reviewed changes

config: Change kube-proxy Mode postfix to ProxyMode

abf42e2

This changes {IPTables,IPVS,None}Mode => {IPTables,IPVS,None}ProxyMode for the sake of readability. Suggested-by: Benjamin Elder <[email protected]> Signed-off-by: Martynas Pumputis <[email protected]>

brb requested a review from BenTheElder March 3, 2021 07:28

BenTheElder approved these changes Mar 3, 2021

View reviewed changes

k8s-ci-robot assigned BenTheElder Mar 3, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 3, 2021

BenTheElder merged commit ca88477 into kubernetes-sigs:master Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to disable kube-proxy #2088

Add possibility to disable kube-proxy #2088

brb commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

aojea commented Feb 24, 2021

neolit123 Feb 24, 2021 •

edited

Loading

brb Feb 24, 2021

aojea Feb 24, 2021

BenTheElder Feb 24, 2021

neolit123 Feb 24, 2021

brb commented Feb 24, 2021

BenTheElder left a comment

BenTheElder Feb 24, 2021

brb commented Feb 25, 2021

brb commented Feb 25, 2021

BenTheElder left a comment

k8s-ci-robot commented Mar 3, 2021

aojea commented Mar 3, 2021

aojea commented Mar 3, 2021 •

edited

Loading

k8s-ci-robot commented Mar 3, 2021 •

edited

Loading

Add possibility to disable kube-proxy #2088

Add possibility to disable kube-proxy #2088

Conversation

brb commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

aojea commented Feb 24, 2021

neolit123 Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

brb Feb 24, 2021

Choose a reason for hiding this comment

aojea Feb 24, 2021

Choose a reason for hiding this comment

BenTheElder Feb 24, 2021

Choose a reason for hiding this comment

neolit123 Feb 24, 2021

Choose a reason for hiding this comment

brb commented Feb 24, 2021

BenTheElder left a comment

Choose a reason for hiding this comment

BenTheElder Feb 24, 2021

Choose a reason for hiding this comment

brb commented Feb 25, 2021

brb commented Feb 25, 2021

BenTheElder left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 3, 2021

aojea commented Mar 3, 2021

aojea commented Mar 3, 2021 • edited Loading

k8s-ci-robot commented Mar 3, 2021 • edited Loading

neolit123 Feb 24, 2021 •

edited

Loading

aojea commented Mar 3, 2021 •

edited

Loading

k8s-ci-robot commented Mar 3, 2021 •

edited

Loading