KEP-1860: Make Kubernetes aware of the LoadBalancer behavior (reopen) #4114

RyanAoh · 2023-06-29T13:02:22Z

One-line PR description: Add an IP mode field to service.status.loadBalancer.ingress to determine whether the rule (iptables or ipvs) for loadBalancer.ip should be added to the node by kube-proxy.

Issue link: Make Kubernetes aware of the LoadBalancer behaviour #1860

Other comments: Earlier discussions Make Kubernetes aware of the LoadBalancer behaviour #1392

RyanAoh · 2023-06-29T13:03:46Z

cc @thockin @danwinship @sftim

thockin · 2023-06-29T22:17:02Z

Thanks!

/lgtm
/approve

RyanAoh · 2023-06-30T09:02:00Z

/hold for some new changes according to the comments on kubernetes/kubernetes#118895

aojea · 2023-06-30T14:53:02Z

/hold for some new changes according to the comments on kubernetes/kubernetes#118895

what changes? I think theKEP is ok, comments does not question the KEP itself, is more on the implementation details, can you elaborate?

RyanAoh · 2023-06-30T15:31:40Z

Got it.
/unhold

RyanAoh · 2023-06-30T15:35:18Z

/assign @johnbelamaric

danwinship · 2023-07-03T13:09:47Z

I had suggested reopening the KEP because it was approved 2 years ago and I didn't even remember it, and I thought we might want to make sure that we still agree with what 2-years-ago us decided.

aojea · 2023-07-03T13:26:01Z

I had suggested reopening the KEP because it was approved 2 years ago and I didn't even remember it, and I thought we might want to make sure that we still agree with what 2-years-ago us decided.

oh, I missed that, I thought that the person driving the PR was assuming all the comments there were pushing back ... sorry for the confusion 😅

aojea · 2023-07-04T08:50:07Z

/lgtm

thockin · 2023-07-07T20:45:34Z

I think you can proceed with implementation while this is pending PRR. I know it's hard to make PRR time happen during the code window.

keps/prod-readiness/sig-network/1860.yaml

wojtek-t · 2023-07-25T13:59:49Z

keps/prod-readiness/sig-network/1860.yaml

+# of http://git.k8s.io/enhancements/OWNERS_ALIASES
+kep-number: 1860
+alpha:
+  approver: "@wojtek-t"


I didn't realize initially, but you didn't fill in PRR section.

Please copy the whole PRR section from the template:
https://github.com/kubernetes/enhancements/blob/master/keps/NNNN-kep-template/README.md#production-readiness-review-questionnaire

and fill-in at least the ones that are required for Alpha (others may be left empty for now).

And ping me once done :)

Done. @wojtek-t

wojtek-t · 2023-07-26T07:50:58Z

keps/sig-network/1860-kube-proxy-IP-node-binding/README.md

+
+- [x] Feature gate (also fill in values in `kep.yaml`)
+  - Feature gate name: LoadBalancerIPMode
+  - Components depending on the feature gate: kube-proxy, kube-apiserver


Also cloud-controller-manager?

It will be [potentially] setting this field, right?

Yes. I'll also add it in kep.yaml.

wojtek-t · 2023-07-26T07:53:43Z

keps/sig-network/1860-kube-proxy-IP-node-binding/README.md

+
+###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?
+
+Yes.


nit: Yes, by disabling the feature gate.

Probably worth mentioning that to get user-visible impact, disabling it in kube-proxy is enough, right?

Yes. Thanks for your advice.

wojtek-t · 2023-07-26T07:54:15Z

keps/sig-network/1860-kube-proxy-IP-node-binding/README.md

+
+###### Are there any tests for feature enablement/disablement?
+
+Yes. There are some unit tests and an integration test added for this feature enablement/disablement.


Are there? Or will be added?

These tests have added in the implemention(kubernetes/kubernetes#118895).

Please correct me if I'm wrong, but I don't see enablement/disablement features there.
I see tests that exercise the feature-gate being on and off, but I don't see tests that change it in the middle of the test [which is what the enablement/disablement is doing.]

On unrelated note - how the implementation PR got merged, given the feature didn't went through PRR and wasn't tracked as part of the release:
https://github.com/orgs/kubernetes/projects/140/views/1?filterQuery=

@thockin ^^

On unrelated note - how the implementation PR got merged, given the feature didn't went through PRR and wasn't tracked as part of the release

I think the KEP was originally approved to be implemented before PRR existed (and then just didn't get implemented). I'm not sure what that means process-wise.

@wojtek-t The test TestUpdateServiceLoadBalancerStatus in pkg/registry/core/service/storage/storage_test.go did this.
https://github.com/RyanAoh/kubernetes/blob/e6863757f4fd9b6997ca38dea549e3b370463db9/pkg/registry/core/service/storage/storage_test.go#L11935-L11947

On unrelated note - how the implementation PR got merged, given the feature didn't went through PRR and wasn't tracked as part of the release: https://github.com/orgs/kubernetes/projects/140/views/1?filterQuery=

@wojtek-t kubernetes/kubernetes#118895 was reverted by kubernetes/kubernetes#119876 for release resons. And I have opened a new one kubernetes/kubernetes#119937, we can make this run into the right process.

wojtek-t · 2023-07-26T07:59:25Z

keps/sig-network/1860-kube-proxy-IP-node-binding/README.md

@@ -149,3 +159,284 @@ On downgrade, the feature gate will simply be disabled, and as long as `kube-pro
 ### Version Skew Strategy

 Version skew from the control plane to `kube-proxy` should be trivial since `kube-proxy` will simply ignore the `ipMode` field.
+


Since I can't comment on unchanged code, commenting here:

For Upgrade/Downgrade section:

"all the previous LoadBalancer service will get an ipMode of VIP."

Will cloud-controller-manager be explicitly setting those?
Or are we going to rely on this being defaulted to VIP on the first write by kube-apiserver?

It will be defaulted to VIP when we get it from kube-apiserver in this case. Ref https://github.com/kubernetes/kubernetes/pull/118895/files#r1248316868

Can you add that explicitly there (with the link - it seems helpful here too).

wojtek-t · 2023-08-24T10:37:01Z

Sorry for delay - was OOO for quite some time.

Thanks for all your answers - this LGTM to me now.

/lgtm
/approve

k8s-ci-robot · 2023-08-24T10:37:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RyanAoh, thockin, wojtek-t

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:

~~keps/prod-readiness/OWNERS~~ [wojtek-t]
~~keps/sig-network/OWNERS~~ [thockin]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 29, 2023

k8s-ci-robot requested a review from dcbw June 29, 2023 13:02

k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jun 29, 2023

k8s-ci-robot requested a review from shaneutt June 29, 2023 13:02

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jun 29, 2023

RyanAoh force-pushed the kep-1860 branch 3 times, most recently from 30b44f9 to 47dfa7a Compare June 29, 2023 13:23

thockin changed the title ~~KEP-1860: Make Kubernetes aware of the LoadBalancer behaviou(reopen)~~ KEP-1860: Make Kubernetes aware of the LoadBalancer behavior (reopen) Jun 29, 2023

k8s-ci-robot assigned thockin Jun 29, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2023

k8s-ci-robot assigned johnbelamaric Jun 30, 2023

k8s-ci-robot assigned aojea Jul 4, 2023

wojtek-t reviewed Jul 10, 2023

View reviewed changes

keps/prod-readiness/sig-network/1860.yaml Outdated Show resolved Hide resolved

RyanAoh force-pushed the kep-1860 branch from 47dfa7a to abf0214 Compare July 10, 2023 12:15

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2023

wojtek-t reviewed Jul 25, 2023

View reviewed changes

RyanAoh force-pushed the kep-1860 branch from abf0214 to 30f6073 Compare July 26, 2023 06:45

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 Jul 26, 2023

RyanAoh force-pushed the kep-1860 branch from 30f6073 to 63d1ce8 Compare July 26, 2023 07:03

wojtek-t reviewed Jul 26, 2023

View reviewed changes

RyanAoh force-pushed the kep-1860 branch from 63d1ce8 to dfb8ff7 Compare July 28, 2023 13:18

RyanAoh requested a review from wojtek-t July 28, 2023 13:19

RyanAoh force-pushed the kep-1860 branch 2 times, most recently from 604170e to bbaa47a Compare August 4, 2023 13:04

reopen KEP 1860

eea659a

RyanAoh force-pushed the kep-1860 branch from bbaa47a to eea659a Compare August 4, 2023 13:11

k8s-ci-robot assigned wojtek-t Aug 24, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2023

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

k8s-ci-robot merged commit df246c8 into kubernetes:master Aug 24, 2023
4 checks passed

k8s-ci-robot added this to the v1.29 milestone Aug 24, 2023

RyanAoh mentioned this pull request Sep 26, 2023

Make Kubernetes aware of the LoadBalancer behaviour #1860

Open

Nils98Ar mentioned this pull request Oct 25, 2023

Modify LB IP with dns suffix when the proxy protocol is used SovereignCloudStack/k8s-cluster-api-provider#323

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-1860: Make Kubernetes aware of the LoadBalancer behavior (reopen) #4114

KEP-1860: Make Kubernetes aware of the LoadBalancer behavior (reopen) #4114

RyanAoh commented Jun 29, 2023

RyanAoh commented Jun 29, 2023

thockin commented Jun 29, 2023

RyanAoh commented Jun 30, 2023

aojea commented Jun 30, 2023

RyanAoh commented Jun 30, 2023

RyanAoh commented Jun 30, 2023

danwinship commented Jul 3, 2023

aojea commented Jul 3, 2023

aojea commented Jul 4, 2023

thockin commented Jul 7, 2023

wojtek-t Jul 25, 2023

RyanAoh Jul 26, 2023

wojtek-t Jul 26, 2023

RyanAoh Jul 28, 2023

wojtek-t Jul 26, 2023

RyanAoh Jul 28, 2023

wojtek-t Jul 26, 2023

RyanAoh Jul 28, 2023

wojtek-t Jul 31, 2023

danwinship Jul 31, 2023

RyanAoh Aug 4, 2023

RyanAoh Aug 14, 2023

wojtek-t Jul 26, 2023

RyanAoh Jul 28, 2023

wojtek-t Jul 31, 2023

RyanAoh Aug 17, 2023

wojtek-t commented Aug 24, 2023

k8s-ci-robot commented Aug 24, 2023


		###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?

		Yes.


		###### Are there any tests for feature enablement/disablement?

		Yes. There are some unit tests and an integration test added for this feature enablement/disablement.

		@@ -149,3 +159,284 @@ On downgrade, the feature gate will simply be disabled, and as long as `kube-pro
		### Version Skew Strategy

		Version skew from the control plane to `kube-proxy` should be trivial since `kube-proxy` will simply ignore the `ipMode` field.

KEP-1860: Make Kubernetes aware of the LoadBalancer behavior (reopen) #4114

KEP-1860: Make Kubernetes aware of the LoadBalancer behavior (reopen) #4114

Conversation

RyanAoh commented Jun 29, 2023

RyanAoh commented Jun 29, 2023

thockin commented Jun 29, 2023

RyanAoh commented Jun 30, 2023

aojea commented Jun 30, 2023

RyanAoh commented Jun 30, 2023

RyanAoh commented Jun 30, 2023

danwinship commented Jul 3, 2023

aojea commented Jul 3, 2023

aojea commented Jul 4, 2023

thockin commented Jul 7, 2023

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

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

wojtek-t commented Aug 24, 2023

k8s-ci-robot commented Aug 24, 2023