Make Kubernetes aware of the LoadBalancer behaviour #92312

Sh4d1 · 2020-06-19T17:11:45Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Implements kubernetes/enhancements#1392
Which issue(s) this PR fixes:
Fixes #79783
Fixes #66607
Special notes for your reviewer:
Naming for the new field still needs to be agreed on
Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/pull/1392

/hold
/cc @thockin
/cc @andrewsykim

aojea · 2020-06-20T13:13:56Z

pkg/apis/core/types.go

@@ -3406,8 +3406,23 @@ type LoadBalancerIngress struct {
 	// (typically AWS load-balancers)
 	// +optional
 	Hostname string
+
+	// RouteType specifies the type of route to use for this ingress
+	// Defaults to `VIP`


does this has to be defaulted here?

kubernetes/pkg/apis/core/v1/defaults.go

Line 99 in 7030246

func SetDefaults_Service(obj *v1.Service) {

Since it needs to get populated by the cloud controller manager, I'm not sure it's the right place. Either we don't default it or we loop through the ingress once EnsureLoadBalancer is called. (Not sure when the SetDefaults_Service is called exactly)

@thockin and/or @andrewsykim would know better,
can you please advice here?

It's a choice.

Do we want every consumer of Service status to find a value in this field? Or do we want the absence of a value to imply a default? Practically speaking, the default is "VIP" since that's what we historically have implemented, so I would be fine to default it as suggested.

pkg/proxy/ipvs/proxier_test.go

aojea · 2020-06-21T08:52:02Z

pkg/proxy/ipvs/proxier.go

-		for _, ingress := range svcInfo.LoadBalancerIPStrings() {
-			if ingress != "" {
+		for _, ingress := range svcInfo.LoadBalancerIngress() {
+			if ingress.IP != "" && ingress.RouteType != v1.LoadBalancerRouteTypeProxy {


if kube-proxy runs against a previous version of the APIserver this field would not be available, should we check first if the field exists?

🤔 in this case the field would just be an empty string no?

In general we say that clients can be older than servers but not newer. If you want to be pedantic, you check for nil and handle it as the default

@thockin do you rather see a IPMode as a string or as a *string. If we set a default on create, I think we can safely use a string and check of empty string instead of nil?

pkg/proxy/iptables/proxier_test.go

fedebongio · 2020-06-23T20:07:57Z

/remove-sig api-machinery

thockin

Overall this looks pretty straight-forward. That said, new fields generally need to be gated and disabled by default, so we can do safe rollbacks. See pkg/features/kube_features.go

thockin · 2020-06-23T23:41:01Z

pkg/apis/core/types.go

@@ -3406,8 +3406,23 @@ type LoadBalancerIngress struct {
 	// (typically AWS load-balancers)
 	// +optional
 	Hostname string
+
+	// RouteType specifies the type of route to use for this ingress
+	// Defaults to `VIP`


It's a choice.

Do we want every consumer of Service status to find a value in this field? Or do we want the absence of a value to imply a default? Practically speaking, the default is "VIP" since that's what we historically have implemented, so I would be fine to default it as suggested.

thockin · 2020-06-23T23:45:18Z

pkg/apis/core/types.go

+	// RouteType specifies the type of route to use for this ingress
+	// Defaults to `VIP`
+	// +optional
+	RouteType LoadBalancerRouteType


names: This only applies when IP is set, right? So maybe call it ipType or ipDeliveryMode or something starting with ip?

ipMode sounds good, wdyt?

pkg/apis/core/types.go

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/meta.go

staging/src/k8s.io/apimachinery/pkg/labels/selector.go

staging/src/k8s.io/code-generator/cmd/informer-gen/generators/generic.go

staging/src/k8s.io/kubectl/pkg/util/templates/markdown.go

pkg/proxy/iptables/proxier.go

thockin · 2020-06-24T00:01:38Z

pkg/proxy/ipvs/proxier.go

-		for _, ingress := range svcInfo.LoadBalancerIPStrings() {
-			if ingress != "" {
+		for _, ingress := range svcInfo.LoadBalancerIngress() {
+			if ingress.IP != "" && ingress.RouteType != v1.LoadBalancerRouteTypeProxy {


In general we say that clients can be older than servers but not newer. If you want to be pedantic, you check for nil and handle it as the default

munnerz · 2020-09-07T10:59:38Z

@Sh4d1 are you able to address Tim's comments above? 😄

Sh4d1 · 2020-11-10T18:03:22Z

@thockin so this will get shipped in 1.20, instead of 1.21 😕 should we rather revert? Or create an exception for the kep ? 🤔

Hence my question, I initially thought it will get shipped with 1.21 (And completly forgot the /hold would get it merged immediatly, and with my messy commits..)

Also wondering how a provisional kep was implemented?
Hence kubernetes/enhancements#2134 but indeed it should have been updated to implementable before.

And I completly missed the ping on kubernetes/enhancements#1860 sorry!

I personally see no objection to create an exception for this feature, was waiting on Tim's opinion.

thockin · 2020-11-10T19:11:21Z

I don't mind creating an exception, but this is not trying to end-run around the rules. I lost track of which features were targetted for which release. I am sorry.

@kikisdeliveryservice No intention to bypass the process, apologies. There's just a lot of cross-checks and I moved too fast.

If you prefer to revert this, I don't object - we can re-propose it and sit on it (though in fact some of this wil make other PRs easier, so maybe we merge some internal parts and sit on the feature).

kikisdeliveryservice · 2020-11-10T23:19:07Z

Thanks @thockin i've brought this to the attention of the Release Leads and we'll let you know shortly.

jeremyrickard · 2020-11-11T00:45:35Z

Commented on the KEP issue for this, but figured I'd drop in here too 👋

I think at this point (we're SUPER close to code freeze..), we should revert this and not push for 1.20. There is a KEP PR in flight in k/enhancements that targets 1.21, I think it would be best if we targeted 1.21 for this feature. @thockin maybe doing a revert and landing the internal parts of it would be acceptable, but we're also 2 days away from code freeze so.....

I totally get that ya'll were not trying to bypass the process and things fall through the cracks...we're just kind of out of runway to get the KEP for this updated and start wrangling docs IMO.

liggitt · 2020-11-11T02:35:48Z

The alpha e2e job started failing ingress tests as soon as this was merged, see https://testgrid.k8s.io/google-gce#gci-gce-alpha-enabled-default&width=20

liggitt · 2020-11-11T02:42:28Z

This is the failure message:

"Ingress.extensions "e2e-example-inggr5wl" is invalid: status.loadBalancer.ingress[0].ipMode: Required value: must be specified when ip is set"

Which seems to indicate that this broke compatibility with a status update that previously worked

liggitt · 2020-11-11T03:06:41Z

pkg/apis/core/validation/validation.go

+
+		if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) {
+			if len(ingress.IP) > 0 && ingress.IPMode == nil {
+				allErrs = append(allErrs, field.Required(idxPath.Child("ipMode"), "must be specified when `ip` is set"))


this is what is failing in the alpha e2e job, so apparently this field is not consistently defaulted. if so, we cannot make it required.

True, it was only defaulted on Service, completly missed the Ingress. Reverted in #96454 and I'll make sure to include ingress in the new one!

This reverts commit ef16faf, reversing changes made to 2343b8a.

Revert "Merge pull request #92312 from Sh4d1/kep_1860"

wilsonehusin · 2020-11-24T18:35:44Z

@Sh4d1 hello! we'd like to remove this entry from 1.20 Release Notes since it was reverted. can you replace the release notes with NONE?

cc: @andrewsykim

Sh4d1 · 2020-11-24T18:37:12Z

@wilsonehusin oops, yes! Done 😄

This reverts commit ef16faf, reversing changes made to 2343b8a.

answer1991 · 2021-01-27T12:53:37Z

pkg/proxy/iptables/proxier.go

-					)
-					// jump to service firewall chain
-					writeLine(proxier.natRules, append(args, "-j", string(fwChain))...)
+					if !utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) || *ingress.IPMode == v1.LoadBalancerIPModeVIP {


why we still need LB forward chain? Is there any rules can jump to LB forward chain if ingress IPMode is Proxy?

k8s-ci-robot requested review from andrewsykim and thockin June 19, 2020 17:11

Sh4d1 force-pushed the kep_1860 branch from 9bf5f2c to 762d8dd Compare June 20, 2020 08:25

aojea reviewed Jun 20, 2020

View reviewed changes

pkg/proxy/ipvs/proxier_test.go Outdated Show resolved Hide resolved

aojea reviewed Jun 21, 2020

View reviewed changes

pkg/proxy/iptables/proxier_test.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 23, 2020

thockin reviewed Jun 24, 2020

View reviewed changes

MatthiasLohr mentioned this pull request Jul 6, 2020

[1.6.1] LoadBalancer does not work with Calico or Cilium hetznercloud/hcloud-cloud-controller-manager#58

Closed

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

Sh4d1 mentioned this pull request Aug 24, 2020

[kube-proxy/ipvs] Add flag to disable ipvs for external ip #79976

Closed

jeremyrickard mentioned this pull request Nov 10, 2020

Make Kubernetes aware of the LoadBalancer behaviour kubernetes/enhancements#1860

Open

liggitt reviewed Nov 11, 2020

View reviewed changes

liggitt mentioned this pull request Nov 11, 2020

Add support for hugepages in downward API #86102

Merged

Sh4d1 added a commit to Sh4d1/kubernetes that referenced this pull request Nov 11, 2020

Revert "Merge pull request kubernetes#92312 from Sh4d1/kep_1860"

d29665c

This reverts commit ef16faf, reversing changes made to 2343b8a.

Sh4d1 mentioned this pull request Nov 11, 2020

Revert "Merge pull request #92312 from Sh4d1/kep_1860" #96454

Merged

k8s-ci-robot added a commit that referenced this pull request Nov 13, 2020

Merge pull request #96454 from Sh4d1/revert_92312

94b1788

Revert "Merge pull request #92312 from Sh4d1/kep_1860"

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 24, 2020

fabstu mentioned this pull request Nov 27, 2020

Waiting for http-01 challenge propagation: failed to perform self check GET request cert-manager/cert-manager#3238

Closed

Sh4d1 mentioned this pull request Jan 4, 2021

Add IP mode field to loadbalancer status ingress #97681

Closed

hidetatz pushed a commit to hidetatz/kubernetes that referenced this pull request Jan 12, 2021

Revert "Merge pull request kubernetes#92312 from Sh4d1/kep_1860"

c78a34f

This reverts commit ef16faf, reversing changes made to 2343b8a.

u-bits pushed a commit to u-bits/kubernetes that referenced this pull request Jan 16, 2021

Revert "Merge pull request kubernetes#92312 from Sh4d1/kep_1860"

f08aba7

This reverts commit ef16faf, reversing changes made to 2343b8a.

answer1991 reviewed Jan 27, 2021

View reviewed changes

process0 mentioned this pull request Jun 22, 2021

Loadbalancer healthchecks report error despite node ports connectable hetznercloud/hcloud-cloud-controller-manager#212

Closed

chess-knight mentioned this pull request Jan 19, 2023

Research: Investigate octavia w/ proxy protocol options SovereignCloudStack/issues#250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Kubernetes aware of the LoadBalancer behaviour #92312

Make Kubernetes aware of the LoadBalancer behaviour #92312

Sh4d1 commented Jun 19, 2020 •

edited

Loading

aojea Jun 20, 2020 •

edited

Loading

Sh4d1 Jun 20, 2020

aojea Jun 21, 2020

thockin Jun 23, 2020

aojea Jun 21, 2020

Sh4d1 Jun 21, 2020

thockin Jun 24, 2020

Sh4d1 Aug 25, 2020

fedebongio commented Jun 23, 2020

thockin left a comment

thockin Jun 23, 2020

thockin Jun 23, 2020

Sh4d1 Aug 25, 2020

thockin Jun 24, 2020

munnerz commented Sep 7, 2020

Sh4d1 commented Nov 10, 2020

thockin commented Nov 10, 2020

kikisdeliveryservice commented Nov 10, 2020

jeremyrickard commented Nov 11, 2020

liggitt commented Nov 11, 2020

liggitt commented Nov 11, 2020

liggitt Nov 11, 2020

Sh4d1 Nov 11, 2020

wilsonehusin commented Nov 24, 2020

Sh4d1 commented Nov 24, 2020

answer1991 Jan 27, 2021

Make Kubernetes aware of the LoadBalancer behaviour #92312

Make Kubernetes aware of the LoadBalancer behaviour #92312

Conversation

Sh4d1 commented Jun 19, 2020 • edited Loading

aojea Jun 20, 2020 • edited Loading

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

fedebongio commented Jun 23, 2020

thockin 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

munnerz commented Sep 7, 2020

Sh4d1 commented Nov 10, 2020

thockin commented Nov 10, 2020

kikisdeliveryservice commented Nov 10, 2020

jeremyrickard commented Nov 11, 2020

liggitt commented Nov 11, 2020

liggitt commented Nov 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonehusin commented Nov 24, 2020

Sh4d1 commented Nov 24, 2020

Choose a reason for hiding this comment

Sh4d1 commented Jun 19, 2020 •

edited

Loading

aojea Jun 20, 2020 •

edited

Loading