Make Kubernetes aware of the LoadBalancer behaviour #118895

RyanAoh · 2023-06-27T08:55:17Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implementation of KEP-1860

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add IP mode field to loadbalancer status ingress

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1860-kube-proxy-IP-node-binding

k8s-triage-robot · 2023-06-27T09:55:01Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

danwinship

I wonder if we should reopen the KEP first for a bit of discussion to see if we've changed our minds about anything in the last 3 years? (The version numbers in the kep.yaml need to be updated anyway...)

danwinship · 2023-06-27T14:37:53Z

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"))


I believe defaulting only occurs when creating new objects, but validation will happen when updating existing objects, so this would prevent you from updating existing old Service objects that use IP but don't have IPMode set.

I think overall it might be better to make IPMode be required, and do default-on-read to handle existing objects where it's not set... (?)

Why the backticks here? Error messages aren't Markdown.

AFAIK status is only updated

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status

When a new version of an object is POSTed or PUT, the spec is updated and available immediately. Over time the system will work to bring the status into line with the spec.

but I do not think we should default, do we? https://github.com/kubernetes/kubernetes/pull/118895/files#r1247552003

@thockin @liggitt

I believe defaulting only occurs when creating new objects

Nope! it magically happens any time a versioned object is created, including upon reading from storage. Occasionally this is bad, but mostly it is good for us. In this case it is good! The defaultOnRead() stuff was added because:

// defaultOnRead sets interlinked fields that were not previously set on read. // We can't do this in the normal defaulting path because that same logic // applies on Get, Create, and Update, but we need to distinguish between them.

Why the backticks here?

We wrote this down as a convention a looooooong time back, in an effort to make error messages clearer. It's not to render markdown, but to denote "this is a field name". I'm happy to revisit the convention (seriously!) but we should fix all the places that do it.

We wrote this down as a convention a looooooong time back, in an effort to make error messages clearer. It's not to render markdown, but to denote "this is a field name". I'm happy to revisit the convention (seriously!) but we should fix all the places that do it.

Is that convention documented somewhere user visible? Would be nice.

I think changing it might be a KEP… - possible, but more than trivial effort.

Is that convention documented somewhere user visible?

Probably not - we never considered the error messages to be API and made no guarantees on them. As such, I don't think a KEP would be needed, just a clearly better convention.

x-ref #119452 and #119454

@pacoxu has found some test failures that appear to be related to this (links above). I'm not sure what the solution is, but this specific validation appears to be backwards incompatible in practice.

danwinship · 2023-06-27T14:39:26Z

pkg/apis/core/types.go

+	// this load-balancer is delivered with the destination IP and port set to the load-balancer's IP and port.
+	// Setting this to "Proxy" indicates that the load-balancer acts like a proxy,
+	// delivering traffic with the destination IP and port set to the node's IP and nodePort or to the pod's IP and targetPort.
+	// This field can only be set when the ip field is also set, and defaults to "VIP" if not specified.


Proxy basically describes the behavior for when Hostname is set but IP is not, right? I think it would be better to say that:

it defaults to Proxy if IP is not set

it defaults to VIP if IP is set

it can't be VIP if IP is not set

(This would also allow for clear semantics when both Hostname and IP are set in the future.)

Can there ever be a case where the value here is misleading, because a cloud provider is compatible with v1 (generally available) Service and that code is unaware of the defaulting mechanism?

If so, let's work out how we'll document that. End users need to know about places where the API can say one thing and the truth is another, especially if that behavior arrives in a minor release update to a stable API.

We should have an answer to the docs question before we graduate this feature to beta.

No; the defaulting behavior described here matches the current semantics: if the cloud LB sets IP then kube-proxy will assume it wants VIP behavior, and if it doesn't set IP then kube-proxy will assume it wants proxy behavior.

I don't understand this description well.

Proxy means "internal cluster traffic directed to the loadbalancer is sent to the external loadbalancer because the loadbalancer is expected to act as a proxy" and VIP is "traffic directed to the loadbalancer inside the cluster is sent to the VIP directly instead of sending it back to the external loadbalancer, no?

@aojea You are right. "Proxy" means the traffic will actually pass through the external loadbalancer and "VIP" means that kube-proxy will create a forwarding rule on the node using the ingress.ip, directly forwarding the traffic to the backend pods.
The default value for this field is "VIP" which keeping the current behaviour, it does not have any impact on the existing cloud providers. BTW, we don't care about the value of ingress.hostname here. @danwinship @sftim

Comments in the code may not be very clear. I'll try to update it accordingly.

We'll (eventually) need docs updates to:

https://kubernetes.io/docs/reference/networking/virtual-ips/

https://kubernetes.io/docs/tasks/access-application-cluster/create-external-load-balancer/

https://kubernetes.io/docs/tutorials/services/source-ip/

I like Dan's thinking, with a tweak:

it defaults to "VIP" when ip is set

otherwise it defaults to "Proxy" when hostname is set

otherwise it is not defaulted

it may not be "VIP" if ip is not set

I don't understand this description well.

So this can be described either from the CloudProvider's perspective or from kube-proxy's perspective. Probably the docs need to explain both.

danwinship · 2023-06-27T14:41:12Z

pkg/proxy/service.go

-	// Obtain Load Balancer Ingress IPs
-	var ips []string
+	// Obtain Load Balancer Ingress
+	var invalidIps []string


invalidIPs (ie, capitalize the "P" if you capitalize the "I")

danwinship · 2023-06-27T14:44:25Z

pkg/proxy/service.go

 	var ips []string
 	for _, ing := range bsvcPortInfo.loadBalancerStatus.Ingress {
-		ips = append(ips, ing.IP)
+		if proxyutil.IsVIPMode(ing) {
+			ips = append(ips, ing.IP)


So I noticed recently that pkg/proxy/service.go is sort of dumb about LoadBalancerIPs: it copies the entire LoadBalancerStatus object into BaseServicePortInfo, but then only allows callers to access the individual IPs. So if you're changing this code anyway, can you change BaseServicePortInfo.loadBalancerStatus v1.LoadBalancerStatus to BaseServicePortInfo.loadBalancerVIPs []string and do the filtering by IsVIPMode in newBaseServiceInfo rather than here?

alexzielenski · 2023-06-27T20:02:28Z

/remove-sig api-machinery

sftim · 2023-06-28T15:28:00Z

pkg/apis/core/types.go

+	// this load-balancer is delivered with the destination IP and port set to the load-balancer's IP and port.
+	// Setting this to "Proxy" indicates that the load-balancer acts like a proxy,
+	// delivering traffic with the destination IP and port set to the node's IP and nodePort or to the pod's IP and targetPort.
+	// This field can only be set when the ip field is also set, and defaults to "VIP" if not specified.


Can there ever be a case where the value here is misleading, because a cloud provider is compatible with v1 (generally available) Service and that code is unaware of the defaulting mechanism?

If so, let's work out how we'll document that. End users need to know about places where the API can say one thing and the truth is another, especially if that behavior arrives in a minor release update to a stable API.

We should have an answer to the docs question before we graduate this feature to beta.

sftim · 2023-06-28T15:28:26Z

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"))


Why the backticks here? Error messages aren't Markdown.

sftim · 2023-06-28T15:28:36Z

pkg/apis/core/validation/validation.go

+
+		if ingress.IPMode != nil {
+			if len(ingress.IP) == 0 {
+				allErrs = append(allErrs, field.Forbidden(idxPath.Child("ipMode"), "may not be used when `ip` is not set"))


Why the backticks here? Error messages aren't Markdown.

sftim · 2023-06-28T15:30:05Z

staging/src/k8s.io/api/core/v1/types.go

+
+const (
+	// LoadBalancerIPModeVIP indicates that the traffic passing through this LoadBalancer
+	// is delivered with the destination IP set to the specified LoadBalancer IP


Suggested change

// is delivered with the destination IP set to the specified LoadBalancer IP

// is delivered with the destination IP address set to the specified LoadBalancer IP address

sftim · 2023-06-28T15:31:39Z

staging/src/k8s.io/api/core/v1/types.go

+	// is delivered with the destination IP set to the specified LoadBalancer IP
+	LoadBalancerIPModeVIP LoadBalancerIPMode = "VIP"
+	// LoadBalancerIPModeProxy indicates that the specified LoadBalancer acts like a proxy,
+	// changing the destination IP to the node IP and the source IP to the LoadBalancer (mostly private) IP


Suggested change

// changing the destination IP to the node IP and the source IP to the LoadBalancer (mostly private) IP

// so that packets arrive at nodes with the destination IP address set as the node's IP address,

// and the source IP address set to the load balancer's IP address. The load balancer's IP address

// might be a private / internal IP address rather than the same IP address that the client sees.

Could we ever support a configuration where the destination IP address belongs to a specific pod, rather than a node? If it could, I think I'd prefer the value to be ProxyToNode here, which leaves room for another proxy-to-pod option later on.

I didn't see the related KEP to provide this feedback before we started on implementation.

As mentioned above, "Proxy" means that the traffic will actually be sent to the external load balancer without caring about whether the backend consists of nodes or pods. I will update the code comment here.

Right, got it.

RyanAoh · 2023-06-30T00:13:00Z

cc @thockin

aojea · 2023-06-30T07:19:50Z

pkg/apis/core/validation/validation_test.go

@@ -22575,3 +22575,87 @@ func TestValidateDynamicResourceAllocation(t *testing.T) {
 		}
 	})
 }
+
+func TestValidateLoadBalancerStatus(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LoadBalancerIPMode, true)()


you should add another variable to the testCase to test with the feature gate to true and false

pkg/apis/core/validation/validation_test.go

pkg/apis/core/v1/defaults.go

pkg/registry/core/service/strategy.go

aojea · 2023-06-30T07:56:24Z

This needs api-review and add integration tests on https://github.com/kubernetes/kubernetes/blob/bd1435dccdd3f06de36b20d49afdd04ce431fcc5/test/integration/service/loadbalancer_test.go , check the git history to find examples and adapt to your change, per example #103129

pkg/apis/core/types.go

thockin · 2023-07-10T15:23:36Z

pkg/apis/core/types.go

+type LoadBalancerIPMode string
+
+const (
+	// LoadBalancerIPModeVIP indicates that kube-proxy will create a forwarding rule


Same here. Doc it in terms of the meaning, not how kube-proxy reacts to the meaning

pkg/apis/core/types.go

pkg/apis/core/validation/validation.go

pkg/proxy/util/utils.go

pkg/registry/core/service/storage/storage_test.go

pkg/registry/core/service/strategy_test.go

aojea · 2023-07-13T12:54:19Z

this needs rebase

k8s-ci-robot · 2023-07-17T08:52:22Z

@RyanAoh: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gci-gce-ipvs	`b185049`	link	false	`/test pull-kubernetes-e2e-gci-gce-ipvs`

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.

thockin

Just one small nit, which can be fixed in a followup

/lgtm
/approve

thockin · 2023-07-17T17:10:45Z

pkg/proxy/util/utils.go

 		return v1.IPv6Protocol
 	}
-	return v1.IPv4Protocol
+
+	return ""


We have IPFamilyUnknown for this

pkg/registry/core/service/strategy_test.go

k8s-ci-robot · 2023-07-17T17:21:25Z

LGTM label has been added.

Git tree hash: 442f8346692ecc4d2f62a601eaf4d9a3c08c4f6c

k8s-ci-robot · 2023-07-17T17:21:49Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~api/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~pkg/features/OWNERS~~ [thockin]
~~pkg/generated/openapi/OWNERS~~ [thockin]
~~pkg/proxy/OWNERS~~ [thockin]
~~pkg/registry/core/service/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/client-go/applyconfigurations/OWNERS~~ [thockin]
~~staging/src/k8s.io/cloud-provider/OWNERS~~ [thockin]
~~test/OWNERS~~ [thockin]

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

This reverts commit 890a6c8, reversing changes made to 4f60a8d.

Revert "Merge pull request #118895 from RyanAoh/kep-1860"

k8s-ci-robot requested review from aojea and bowei June 27, 2023 08:56

danwinship reviewed Jun 27, 2023

View reviewed changes

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

sftim reviewed Jun 28, 2023

View reviewed changes

aojea reviewed Jun 30, 2023

View reviewed changes

pkg/apis/core/validation/validation_test.go Show resolved Hide resolved

aojea reviewed Jun 30, 2023

View reviewed changes

pkg/apis/core/v1/defaults.go Show resolved Hide resolved

aojea reviewed Jun 30, 2023

View reviewed changes

pkg/registry/core/service/strategy.go Outdated Show resolved Hide resolved

thockin reviewed Jul 10, 2023

View reviewed changes

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

thockin self-assigned this Jul 14, 2023

RyanAoh force-pushed the kep-1860 branch from b2fbc2d to 0e17179 Compare July 15, 2023 06:35

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 15, 2023

RyanAoh added 4 commits July 17, 2023 15:51

Add IP mode field

e686375

Generated code for IP mode field

7c6e399

Proxy changes for IP mode field

7eab0d7

Integration tests for IP mode field

b185049

RyanAoh force-pushed the kep-1860 branch from 0e17179 to b185049 Compare July 17, 2023 08:05

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

thockin reviewed Jul 17, 2023

View reviewed changes

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

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

k8s-ci-robot merged commit 890a6c8 into kubernetes:master Jul 17, 2023

k8s-ci-robot added this to the v1.28 milestone Jul 17, 2023

This was referenced Jul 20, 2023

Failure cluster [878e052f...] ipMode: Required value: must be specified when ip is set #119452

Closed

use LoadBalancer type service for e2e service test to patch ingress status #119454

Merged

hongliangl mentioned this pull request Aug 2, 2023

Implement KEP-1860 in AntreaProxy: Make Kubernetes aware of the LoadBalancer behavior antrea-io/antrea#5342

Closed

1 task

marosset added a commit to marosset/kubernetes that referenced this pull request Aug 9, 2023

Revert "Merge pull request kubernetes#118895 from RyanAoh/kep-1860"

0d90d1f

This reverts commit 890a6c8, reversing changes made to 4f60a8d.

marosset mentioned this pull request Aug 9, 2023

Revert "Merge pull request #118895 from RyanAoh/kep-1860" #119876

Merged

k8s-ci-robot added a commit that referenced this pull request Aug 10, 2023

Merge pull request #119876 from marosset/revert-118895

e6218f3

Revert "Merge pull request #118895 from RyanAoh/kep-1860"

RyanAoh mentioned this pull request Aug 14, 2023

Make Kubernetes aware of the LoadBalancer behaviour #119937

Merged

jaskaransarkaria mentioned this pull request Jun 5, 2024

Planning upgrade to EKS 1.28 ministryofjustice/cloud-platform#5570

Closed

pacoxu mentioned this pull request Oct 12, 2024

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

Open

12 tasks

	// is delivered with the destination IP set to the specified LoadBalancer IP
	// is delivered with the destination IP address set to the specified LoadBalancer IP address

-	// changing the destination IP to the node IP and the source IP to the LoadBalancer (mostly private) IP
+	// so that packets arrive at nodes with the destination IP address set as the node's IP address,
+	// and the source IP address set to the load balancer's IP address. The load balancer's IP address
+	// might be a private / internal IP address rather than the same IP address that the client sees.

Make Kubernetes aware of the LoadBalancer behaviour #118895

Make Kubernetes aware of the LoadBalancer behaviour #118895

Conversation

RyanAoh commented Jun 27, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-triage-robot commented Jun 27, 2023

danwinship 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

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

danwinship Jun 27, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzielenski commented Jun 27, 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

RyanAoh commented Jun 30, 2023

Choose a reason for hiding this comment

aojea commented Jun 30, 2023

Choose a reason for hiding this comment

aojea commented Jul 13, 2023

k8s-ci-robot commented Jul 17, 2023

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

danwinship Jun 27, 2023 •

edited

Loading