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

janosi · 2022-10-06T09:49:27Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

This PR is to move the implementation of kubernetes/enhancements#1435 to GA

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

Removed the unit tests that test the cases when the MixedProtocolLBService feature flag was false - the feature flag is locked to true with GA
Added an integration test to test whether the API server accepts an LB Service with different protocols.
Added an e2e test case to the "Network/Services" part to test whether a Service which is exposed via a multi-protocol LB Service is reachable via all ports
Removed the conditional validation that compared the new and the old Service definitions during an update - the feature flag is locked to true with GA.

Does this PR introduce a user-facing change?

Yes. The feature flag is locked to true.

Moving MixedProtocolLBService from beta to GA

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

- [KEP]: link comes here once the KEP update is merged

k8s-ci-robot · 2022-10-06T09:49:35Z

@janosi: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2022-10-06T09:49:36Z

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.

aojea · 2022-10-06T09:51:16Z

/hold

I've added some comments to the KEP, I think e2e should be part of the GA criteria to verify Service proxies implement correctly the new behavior

aojea · 2022-10-06T23:29:23Z

/hold cancel
/ok-to-test
KEPs has merged , all questions answered
Thanks

janosi · 2022-10-07T06:38:57Z

/retest

Removed the unit tests that test the cases when the MixedProtocolLBService feature flag was false - the feature flag is locked to true with GA Added an integration test to test whether the API server accepts an LB Service with different protocols. Added an e2e test to test whether a service which is exposed by a multi-protocol LB Service is accessible via both ports. Removed the conditional validation that compared the new and the old Service definitions during an update - the feature flag is locked to true with GA.

janosi · 2022-11-02T12:49:23Z

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-gce-ubuntu-containerd
/test pull-kubernetes-e2e-gce-100-performance

janosi · 2022-11-02T14:44:54Z

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-gce-ubuntu-containerd

aojea · 2022-11-02T22:43:44Z

test/e2e/network/service.go

+				},
+				{
+					Name:       "portname2",
+					Port:       81,


can we use the same port for both protocols?
I've seen some bugs on some implementations when this is happening

aojea · 2022-11-02T22:44:58Z

test/e2e/framework/service/jig.go

+	}
+
+	ginkgo.By("waiting for cluster IP for loadbalancer service " + j.Namespace + "/" + j.Name)
+	return j.WaitForLoadBalancerClusterIP(timeout)


you don't need to wait, ClusterIP assignment is synchronous

thockin

I'm approving but @aojea is looking at tests

/approve

thockin · 2022-11-02T22:49:12Z

test/e2e/network/service.go

+		Testname: Service, same ports with different protocols on a Load Balancer Service
+		Description: Create a LoadBalancer service with two ports that have the same value but use different protocols. Add a Pod that listens on both ports. The Pod must be reachable via the ClusterIP and both ports
+	*/
+	ginkgo.It("should serve endpoints on same port and different protocol for internal traffic on Type LoadBalancer ", func() {


Once upon a time we tried to flatten all LB tests into one run (because LB creation can be very slow). I sort of lost touch - is that still something we care about?

@aojea

we are not waiting for the LoadBalancer IP assignment in this test, we only care about the ClusterIP here

k8s-ci-robot · 2022-11-02T22:53:07Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~pkg/apis/OWNERS~~ [thockin]
~~pkg/features/OWNERS~~ [thockin]
~~pkg/registry/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

aojea · 2022-11-02T23:04:41Z

test/e2e/framework/service/jig.go

+	ginkgo.By("waiting for cluster IP for loadbalancer service " + j.Namespace + "/" + j.Name)
+	return j.WaitForLoadBalancerClusterIP(timeout)


Suggested change

ginkgo.By("waiting for cluster IP for loadbalancer service " + j.Namespace + "/" + j.Name)

return j.WaitForLoadBalancerClusterIP(timeout)

return j.sanityCheckService(service, v1.ServiceTypeLoadBalancer)

aojea · 2022-11-02T23:08:18Z

test/e2e/framework/service/jig.go

+
+// CreateLoadBalancerServiceWaitForClusterIPOnly creates a loadbalancer service and waits
+// for it to acquire a cluster IP
+func (j *TestJig) CreateLoadBalancerServiceWaitForClusterIPOnly(timeout time.Duration, tweak func(svc *v1.Service)) (*v1.Service, error) {


Suggested change

func (j *TestJig) CreateLoadBalancerServiceWaitForClusterIPOnly(timeout time.Duration, tweak func(svc *v1.Service)) (*v1.Service, error) {

func (j *TestJig) CreateLoadBalancerServiceWaitForClusterIPOnly(tweak func(svc *v1.Service)) (*v1.Service, error) {

we don't need the timeout

aojea · 2022-11-03T01:42:02Z

some comments to remove unnecessary code and lgtm

… the protocol, too.

janosi · 2022-11-03T09:02:38Z

@aojea I think I fixed the comments.
On the other hand, I modified the e2e case so, that all ports (LB Service, container ports) have the same port value but different protocols, as you requested. In turn, I had to add some new util functions to be able to validate the list of ports that has the same port number but different protocols.

janosi · 2022-11-03T10:32:24Z

/test pull-kubernetes-unit

aojea · 2022-11-03T10:40:37Z

test/e2e/network/service.go

+		// If EndpointSlice API is enabled, then validate if appropriate EndpointSlice objects
+		// were also create/updated/deleted.
+		if _, err := c.Discovery().ServerResourcesForGroupVersion(discoveryv1.SchemeGroupVersion.String()); err == nil {


I see, this is a legacy thing, it is always enabled since it is GA, don't worry we can change it in a follow up

Yep, it got there due to my copy-paste magic.

janosi · 2022-11-03T10:52:41Z

/test pull-kubernetes-e2e-gci-gce-ipvs

aojea · 2022-11-03T11:15:26Z

/lgtm

thanks for reducing the technical debt, we were only validation by port number on the e2e , and now we can validate per port and protocol

aojea · 2022-11-03T12:10:31Z

#112895

k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/storagebackend/factory: TestCreateReadycheck/timeouts_if_response_time_higher_than_custom_timeout expand_less

janosi · 2022-11-03T12:12:49Z

/test pull-kubernetes-unit

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 6, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 6, 2022

janosi mentioned this pull request Oct 6, 2022

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

Closed

k8s-ci-robot requested review from aojea and caesarxuchao October 6, 2022 09:50

k8s-ci-robot added area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 6, 2022

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

janosi force-pushed the kep-1435-GA branch from 5d51656 to 2e185d7 Compare October 25, 2022 19:50

k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework sig/network Categorizes an issue or PR as relevant to SIG Network. labels Oct 25, 2022

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

janosi force-pushed the kep-1435-GA branch from 40b5b85 to 82ce61a Compare November 2, 2022 11:45

k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 2, 2022

janosi mentioned this pull request Nov 2, 2022

Update docs given that Services with type=LoadBalancer is stable kubernetes/website#37676

Merged

aojea reviewed Nov 2, 2022

View reviewed changes

thockin reviewed Nov 2, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2022

aojea reviewed Nov 2, 2022

View reviewed changes

Fix review comments. Implement endpoint port validation that verifies…

9d75c95

… the protocol, too.

aojea reviewed Nov 3, 2022

View reviewed changes

k8s-ci-robot assigned aojea Nov 3, 2022

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

k8s-ci-robot merged commit c98aef4 into kubernetes:master Nov 3, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 3, 2022

alibo mentioned this pull request Feb 13, 2023

HTTP3 and QUIC support kubernetes-sigs/gateway-api#687

Closed

liggitt moved this from Assigned to API review completed, 1.26 in API Reviews Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

janosi commented Oct 6, 2022 •

edited

k8s-ci-robot commented Oct 6, 2022

k8s-ci-robot commented Oct 6, 2022

aojea commented Oct 6, 2022

aojea commented Oct 6, 2022

janosi commented Oct 7, 2022

janosi commented Nov 2, 2022

janosi commented Nov 2, 2022

aojea Nov 2, 2022

aojea Nov 2, 2022

thockin left a comment

thockin Nov 2, 2022

aojea Nov 2, 2022

k8s-ci-robot commented Nov 2, 2022

aojea Nov 2, 2022

aojea Nov 2, 2022

aojea commented Nov 3, 2022

janosi commented Nov 3, 2022

janosi commented Nov 3, 2022

aojea Nov 3, 2022

janosi Nov 3, 2022

janosi commented Nov 3, 2022

aojea commented Nov 3, 2022

aojea commented Nov 3, 2022

janosi commented Nov 3, 2022

		ginkgo.By("waiting for cluster IP for loadbalancer service " + j.Namespace + "/" + j.Name)
		return j.WaitForLoadBalancerClusterIP(timeout)

	ginkgo.By("waiting for cluster IP for loadbalancer service " + j.Namespace + "/" + j.Name)
	return j.WaitForLoadBalancerClusterIP(timeout)
	return j.sanityCheckService(service, v1.ServiceTypeLoadBalancer)

	func (j TestJig) CreateLoadBalancerServiceWaitForClusterIPOnly(timeout time.Duration, tweak func(svc v1.Service)) (*v1.Service, error) {
	func (j TestJig) CreateLoadBalancerServiceWaitForClusterIPOnly(tweak func(svc v1.Service)) (*v1.Service, error) {

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

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

Conversation

janosi commented Oct 6, 2022 • edited

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-ci-robot commented Oct 6, 2022

k8s-ci-robot commented Oct 6, 2022

aojea commented Oct 6, 2022

aojea commented Oct 6, 2022

janosi commented Oct 7, 2022

janosi commented Nov 2, 2022

janosi commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

k8s-ci-robot commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Nov 3, 2022

janosi commented Nov 3, 2022

janosi commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janosi commented Nov 3, 2022

aojea commented Nov 3, 2022

aojea commented Nov 3, 2022

janosi commented Nov 3, 2022

janosi commented Oct 6, 2022 •

edited