Mixed protocol support for Services with type=LoadBalancer #94028

janosi · 2020-08-15T20:55:58Z

What type of PR is this?
/kind feature
/kind api-change

What this PR does / why we need it:
This PR implements the following KEP: kubernetes/enhancements@4ccf8f5

It enables the usage of different protocol values in the same LoadBalancer Service.
It extends the Service's Status definition so cloud provider implementations can include more details about the status of the cloud load balancer into that.

KEP issue: kubernetes/enhancements#1435

Which issue(s) this PR fixes:
Fixes #23880

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The usage of mixed protocol values in the same LoadBalancer Service is possible if the new feature gate MixedProtocolLBService is enabled.
"action required"
The feature gate is disabled by default. The user has to enable it for the API Server.

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

- [KEP]: https://github.com/kubernetes/enhancements/commit/4ccf8f53d9ffa4ffdb98afecfc5eb0ed4f5550f3

k8s-ci-robot · 2020-08-15T20:56:07Z

Hi @janosi. Thanks for your PR.

I'm waiting for a kubernetes 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.

fejta-bot · 2020-08-15T23:11:24Z

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.

janosi · 2020-08-16T09:32:12Z

/label api-review

janosi · 2020-08-16T09:32:35Z

/assign @thockin

pontiyaraja · 2020-08-17T18:13:28Z

/ok-to-test

thockin · 2020-08-17T19:07:16Z

pkg/apis/core/types.go

+
+	//Current service state
+	// +optional
+	Conditions []ServiceCondition


The KEP says: "Our feature does not introduce new values or new fields"

am not saying that adding a field is wrong, but it is not part of the committed KEP. I re-read the discussion on the KEP and we agreed to this in theory, but it was never updated

We should use metav1.Condition from staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go

but it is not part of the committed KEP. I re-read the discussion on the KEP and we agreed to this in theory, but it was never updated

Hmmm, we have in the KEP the following:

In order to provide a way for the CPI to indicate port statuses to the user we would add the following new portStatus list of port statuses to Service.status.loadBalancer.ingress, so the CPI can indicate the status of the LB. Also we add a conditions list of conditions to the Service.status.loadBalancer, with the first official condition LoadBalancerMixedProtocolNotSupported defined.

And then there is the specification of these fields, too.

Am I missing something?

So, should I remove this API change (ServiceStatus) from this PR? I put it in here, as it is part of the KEP.

You are right, I guess I skimmed too fast. Sorry.

So, I started using metav1.Condition there instead of ServiceConditions, but as I can see, bazel-test fails due to that https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/94028/pull-kubernetes-bazel-test/1296919306033958912

So, I started using metav1.Condition there instead of ServiceConditions, but as I can see, bazel-test fails due to that https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/94028/pull-kubernetes-bazel-test/1296919306033958912

chatted in slack, but for posterity, looks like metav1.Condition types should be added to

kubernetes/pkg/controlplane/import_known_versions_test.go

Lines 53 to 73 in c3b888f

// These types are registered in external versions, and therefore include json tags,

// but are also registered in internal versions (or referenced from internal types),

// so we explicitly allow tags for them

var typesAllowedTags = map[reflect.Type]bool{

reflect.TypeOf(intstr.IntOrString{}): true,

reflect.TypeOf(metav1.Time{}): true,

reflect.TypeOf(metav1.MicroTime{}): true,

reflect.TypeOf(metav1.Duration{}): true,

reflect.TypeOf(metav1.TypeMeta{}): true,

reflect.TypeOf(metav1.ListMeta{}): true,

reflect.TypeOf(metav1.ObjectMeta{}): true,

reflect.TypeOf(metav1.OwnerReference{}): true,

reflect.TypeOf(metav1.LabelSelector{}): true,

reflect.TypeOf(metav1.GetOptions{}): true,

reflect.TypeOf(metav1.ExportOptions{}): true,

reflect.TypeOf(metav1.ListOptions{}): true,

reflect.TypeOf(metav1.DeleteOptions{}): true,

reflect.TypeOf(metav1.GroupVersionKind{}): true,

reflect.TypeOf(metav1.GroupVersionResource{}): true,

reflect.TypeOf(metav1.Status{}): true,

}

@liggitt Thank you for your help. Indeed, it is OK now.

thockin · 2020-08-17T19:14:08Z

pkg/apis/core/types.go

@@ -3508,6 +3512,14 @@ type LoadBalancerIngress struct {
 	// (typically AWS load-balancers)
 	// +optional
 	Hostname string
+
+	//The list has one entry per service port in the manifest


nit: comments want a " " between // and text

Godoc style starts with the field name

We need to tighten the spec. This field is optional, but if it is provided, there must be one entry per service port, mapped by port number and protocol (avoid the word "manifest, tooooo vague).

Updated the comment here.

thockin · 2020-08-17T19:19:31Z

pkg/apis/core/types.go

+
+	//The list has one entry per service port in the manifest
+	// +optional
+	PortStatuses []PortStatus


name "ports"

thockin · 2020-08-17T19:20:15Z

pkg/apis/core/types.go

+	// +optional
+	PortStatuses []PortStatus
+
+	//A human readable message indicating details about the condition of the load-balancer.


Is human-readable what we want here? We have conditions and ports which have messages - what is THIS for that isn't better in one of those?

(I just wonder how I should interpret all caps here).
Well, this is the part of the agreed and accepted KEP. Which has been reviewed and accepted by people much better in K8s API design than I.
Of course things change and I am OK to remove it from here. Just like the whole API implementation as said above.

/KEPs are not written in stone, and API reviews can tweak the exact structure. I am asking what this particular field is for that is not already covered by the other messages?

I think the original purpose was to have a "message" on this level, too. I.e. we have a condition with a message on Service level, we have a message on portstatus level, but no message on LB level, in the middle.
Though, I just removed it.

thockin · 2020-08-17T19:20:37Z

pkg/apis/core/types.go

+
+// PortStatus represents the condition of a service port condition
+type PortStatus struct {
+	Port     int32


comments on all these

Fixed, please check.

pkg/apis/core/types.go

thockin · 2020-08-17T19:36:11Z

pkg/apis/core/validation/conditional_validation.go

+	var protocol api.Protocol = "init"
+	for _, port := range service.Spec.Ports {
+		if protocol == "init" {
+			if port.Protocol == "" {


You should not need to handle "" - that will be defaulted

This whole thing should be as easy as:

protos := map[string]bool{} for _, port := range service.Spec.Ports { protos[string(port.Protocol)] = true } return len(protos) > 1

Thanks. Replaced my code with this one.

thockin · 2020-08-17T19:44:42Z

pkg/apis/core/validation/validation.go

@@ -4232,18 +4232,12 @@ func ValidateService(service *core.Service, allowAppProtocol bool) field.ErrorLi

 	if service.Spec.Type == core.ServiceTypeLoadBalancer {


I think this whole block can go away - the same check (supportedPortProtocols.Has) exists in validateServicePort() already. Please double-check that I am not missing something

Indeed. Service ports are checked before this place for any Service. Removed this one here.

thockin · 2020-08-17T19:47:16Z

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

@@ -3971,6 +3977,15 @@ type LoadBalancerIngress struct {
 	// (typically AWS load-balancers)
 	// +optional
 	Hostname string `json:"hostname,omitempty" protobuf:"bytes,2,opt,name=hostname"`
+
+	//The list has one entry per service port in the manifest
+	// +listType=map


Why not atomic? Do we really need to patch individual records? The key for the map is multi-field, which is unpleasant

To tell the truth, it was more like a guess. Modified to atomic.

thockin · 2020-08-17T19:49:01Z

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

+	Port int32 `json:"port" protobuf:"varint,1,opt,name=port"`
+	// The IP protocol for this port. Supports "TCP", "UDP", and "SCTP".
+	// Default is TCP.
+	// +optional


If this is optional with a default, you need to write the rule in defaults.go to set it.

Made it required. As you commented above port + protocol should be the key.

janosi · 2020-11-13T17:16:28Z

FWIW regarding squash, you can set thetide/merge-method-squash and let the bots handle it for you, if the goal is a complete squash. Just make sure your first commit message is good :)

Good to know. Thank you! :)

thockin · 2020-11-13T17:21:13Z

/retest

KEP: https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20200103-mixed-protocol-lb.md Add new feature gate to control the support of mixed protocols in Services with type=LoadBalancer Add new fields to the ServiceStatus Add Ports to the LoadBalancerIngress, so cloud provider implementations can report the status of the requested load balanc er ports Add ServiceCondition to the ServiceStatus so Service controllers can indicate the conditions of the Service

janosi · 2020-11-13T17:53:40Z

@thockin new rebase was required

thockin · 2020-11-13T18:01:35Z

/lgtm

janosi · 2020-11-13T19:18:01Z

/retest

janosi · 2020-11-13T20:57:11Z

/retest

janosi · 2020-11-13T21:30:14Z

@andrewsykim @thockin @kikisdeliveryservice @liggitt Thank you all for your help!

zimmertr · 2020-11-13T22:36:47Z

Thanks everyone for this!

…s#94028) * Mixed protocol support for Services with type=LoadBalancer KEP: https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20200103-mixed-protocol-lb.md Add new feature gate to control the support of mixed protocols in Services with type=LoadBalancer Add new fields to the ServiceStatus Add Ports to the LoadBalancerIngress, so cloud provider implementations can report the status of the requested load balanc er ports Add ServiceCondition to the ServiceStatus so Service controllers can indicate the conditions of the Service * regenerate conflicting stuff

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 15, 2020

k8s-ci-robot requested review from dchen1107 and thockin August 15, 2020 20:56

janosi mentioned this pull request Aug 15, 2020

Support of mixed protocols in Services with type=LoadBalancer kubernetes/enhancements#1435

Closed

janosi changed the title ~~WIP Mixed protocol support for Services with type=LoadBalancer~~ Mixed protocol support for Services with type=LoadBalancer Aug 15, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 15, 2020

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Aug 16, 2020

k8s-ci-robot assigned thockin Aug 16, 2020

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 Aug 17, 2020

thockin reviewed Aug 17, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Aug 21, 2020

janosi force-pushed the mixedprotocollb branch from 3a76398 to 5e34c49 Compare September 5, 2020 17:43

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Sep 5, 2020

feiskyer mentioned this pull request Sep 9, 2020

Mixed protocols doesn't work on Azure kubernetes-sigs/cloud-provider-azure#295

Closed

Laszlo Janosi added 2 commits November 13, 2020 17:31

regenerate conflicting stuff

382f6f5

janosi force-pushed the mixedprotocollb branch from 75a3e30 to 382f6f5 Compare November 13, 2020 17:53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2020

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

k8s-ci-robot merged commit c970a46 into kubernetes:master Nov 13, 2020

janosi deleted the mixedprotocollb branch November 13, 2020 21:23

jbg mentioned this pull request Nov 14, 2020

[EKS] [request]: Network Load Balancer with mixed protocols aws/containers-roadmap#841

Open

0xThiebaut mentioned this pull request Nov 19, 2020

Revert split LoadBalancers to single LoadBalancer with mixed protocols traefik/traefik-helm-chart#311

Closed

aledbf mentioned this pull request Dec 4, 2020

Documentation is lacking for UDP set up kubernetes/ingress-nginx#6573

Closed

TBBle mentioned this pull request Jan 12, 2021

Support mixed protocols in service.type=loadbalancer #23880

Closed

liggitt added this to Backlog in API Reviews Feb 16, 2021

liggitt moved this from Backlog to API review completed, 1.20 in API Reviews Feb 16, 2021

jetersen mentioned this pull request Feb 19, 2021

[INFRA-2743] add traefik as a secondary ingress controller jenkins-infra/kubernetes-management#867

Merged

jnoordsij mentioned this pull request Oct 24, 2022

Merge TCP and UDP ports into a single service traefik/traefik-helm-chart#656

Merged

3 tasks

liggitt mentioned this pull request Oct 27, 2022

KEP-1435 Mixed Protocol values in LoadBalancer Service GA #112895

Merged

renklus mentioned this pull request Jan 4, 2024

Remove outdated information about k8s mixed protocol support metallb/metallb#2228

Merged

	// These types are registered in external versions, and therefore include json tags,
	// but are also registered in internal versions (or referenced from internal types),
	// so we explicitly allow tags for them
	var typesAllowedTags = map[reflect.Type]bool{
	reflect.TypeOf(intstr.IntOrString{}): true,
	reflect.TypeOf(metav1.Time{}): true,
	reflect.TypeOf(metav1.MicroTime{}): true,
	reflect.TypeOf(metav1.Duration{}): true,
	reflect.TypeOf(metav1.TypeMeta{}): true,
	reflect.TypeOf(metav1.ListMeta{}): true,
	reflect.TypeOf(metav1.ObjectMeta{}): true,
	reflect.TypeOf(metav1.OwnerReference{}): true,
	reflect.TypeOf(metav1.LabelSelector{}): true,
	reflect.TypeOf(metav1.GetOptions{}): true,
	reflect.TypeOf(metav1.ExportOptions{}): true,
	reflect.TypeOf(metav1.ListOptions{}): true,
	reflect.TypeOf(metav1.DeleteOptions{}): true,
	reflect.TypeOf(metav1.GroupVersionKind{}): true,
	reflect.TypeOf(metav1.GroupVersionResource{}): true,
	reflect.TypeOf(metav1.Status{}): true,
	}

		@@ -4232,18 +4232,12 @@ func ValidateService(service *core.Service, allowAppProtocol bool) field.ErrorLi

		if service.Spec.Type == core.ServiceTypeLoadBalancer {

Mixed protocol support for Services with type=LoadBalancer #94028

Mixed protocol support for Services with type=LoadBalancer #94028

Conversation

janosi commented Aug 15, 2020 • edited

k8s-ci-robot commented Aug 15, 2020

fejta-bot commented Aug 15, 2020

janosi commented Aug 16, 2020

janosi commented Aug 16, 2020

pontiyaraja commented Aug 17, 2020

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

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

janosi commented Nov 13, 2020

thockin commented Nov 13, 2020

janosi commented Nov 13, 2020

thockin commented Nov 13, 2020

janosi commented Nov 13, 2020

janosi commented Nov 13, 2020

janosi commented Nov 13, 2020

zimmertr commented Nov 13, 2020

janosi commented Aug 15, 2020 •

edited