New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KEP-1435 Mixed Protocol values in LoadBalancer Service GA #112895
Conversation
@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 The 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. |
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 Once the patch is verified, the new status will be reflected by the 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. |
/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 |
/hold cancel |
/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.
/test pull-kubernetes-e2e-kind |
/test pull-kubernetes-e2e-kind |
test/e2e/network/service.go
Outdated
}, | ||
{ | ||
Name: "portname2", | ||
Port: 81, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use the same port for both protocols?
I've seen some bugs on some implementations when this is happening
test/e2e/framework/service/jig.go
Outdated
} | ||
|
||
ginkgo.By("waiting for cluster IP for loadbalancer service " + j.Namespace + "/" + j.Name) | ||
return j.WaitForLoadBalancerClusterIP(timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to wait, ClusterIP assignment is synchronous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving but @aojea is looking at tests
/approve
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are not waiting for the LoadBalancer IP assignment in this test, we only care about the ClusterIP here
[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:
Approvers can indicate their approval by writing |
test/e2e/framework/service/jig.go
Outdated
ginkgo.By("waiting for cluster IP for loadbalancer service " + j.Namespace + "/" + j.Name) | ||
return j.WaitForLoadBalancerClusterIP(timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ginkgo.By("waiting for cluster IP for loadbalancer service " + j.Namespace + "/" + j.Name) | |
return j.WaitForLoadBalancerClusterIP(timeout) | |
return j.sanityCheckService(service, v1.ServiceTypeLoadBalancer) |
test/e2e/framework/service/jig.go
Outdated
|
||
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
some comments to remove unnecessary code and lgtm |
… the protocol, too.
@aojea I think I fixed the comments. |
/test pull-kubernetes-unit |
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it got there due to my copy-paste magic.
/test pull-kubernetes-e2e-gci-gce-ipvs |
/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 |
|
/test pull-kubernetes-unit |
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:
Does this PR introduce a user-facing change?
Yes. The feature flag is locked to true.
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: