Port aggregated apiserver discovery to EndpointSlices #129837

danwinship · 2025-01-27T12:51:40Z

What this PR does / why we need it:

Belatedly ports the aggregated apiserver code to use EndpointSlices rather than Endpoints.

Does this PR introduce a user-facing change?

When proxying to an aggregated API server, kube-apiserver now uses the
EndpointSlices of the `service` indicated by the `APIServer`, rather than
using Endpoints.

If you are using the aggregated API server feature, and you are writing out
the endpoints for it by hand (rather than letting kube-controller-manager
generate Endpoints and EndpointSlices for it automatically based on the
Service definition), then you should write out an EndpointSlice object rather
than (or in addition to) an Endpoints object.

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/4974-deprecate-endpoints

~~(The KEP is not yet merged, but this PR is correct/desirable in terms of internal consistency even if the KEP is rejected.)~~

/kind cleanup
/kind feature
/sig network
/sig api-machinery

aojea · 2025-01-27T13:47:32Z

(rather than letting
kube-controller-manager generate Endpoints and EndpointSlices for it
automatically based on the Service definition), then you need to write out an
EndpointSlice object rather than (or in addition to) an Endpoints object.

will not be solved by the mirroring controller?

aojea · 2025-01-27T13:59:14Z

one suggestion, in the eventual situation something goes wrong with one of the changes, having all of them in the same commit will revert all, should we have one commit for aggregator, other for proxy and other for remote available controller so we avoid that possible problem?

danwinship · 2025-01-27T19:38:35Z

will not be solved by the mirroring controller?

yes, but

I want the mirroring controller to eventually be optional too.
There's no good reason for writing out an Endpoints and then letting the mirroring controller rewrite it to an EndpointSlice for you, rather than just writing out the EndpointSlice yourself.

danwinship · 2025-01-27T19:42:25Z

one commit for aggregator, other for proxy and other for remote available controller

They're all basically a single feature (proxy.ResolveEndpoints is only called by the aggregator code). I don't think we want to ship a state where part of the feature uses Endpoints and part uses EndpointSlices.

aojea · 2025-01-27T20:38:06Z

/lgtm

/assign @liggitt @deads2k

k8s-ci-robot · 2025-01-27T20:38:13Z

LGTM label has been added.

Git tree hash: cbc6a1a992fedf76a537663543b4993d4eee61aa

staging/src/k8s.io/apiserver/pkg/util/proxy/proxy.go

liggitt · 2025-01-28T13:51:56Z

staging/src/k8s.io/apiserver/pkg/util/proxy/proxy.go

+				ep := &slice.Endpoints[(epi+offset)%len(slice.Endpoints)]
+				if ep.Conditions.Ready == nil || *ep.Conditions.Ready {
+					ip := ep.Addresses[0]
+					port := int(*slice.Ports[i].Port)


godoc on the Port field sounds like Port can be nil... should we check / require a non-nil port value above when deciding if the Port is valid for our use?

Huh. This is another unused future-extensibility point. Port is allowed to be unset (and thus not validated as required) even though it's required to be set for EndpointSlices corresponding to Services. Grumble.

Yeah... I just don't know the EndpointSlice API surface well enough to know which of these "allowed by the spec" things we have to handle, so my default is to check everything we could possibly encounter

maybe I'm missing it, but it still looks like we can hit a nil port and panic here? should we add || slice.Ports[i].Port == nil to the conditions above where we just continue?

staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller.go

BenTheElder · 2025-01-28T21:17:00Z

/triage accepted
/cc @seans3

aojea · 2025-03-17T19:41:35Z

/lgtm

k8s-ci-robot · 2025-03-17T19:41:41Z

LGTM label has been added.

Git tree hash: 626403b39cc1e94c2349cfd9ae9b5f4d95fbc5a9

liggitt · 2025-03-18T13:57:06Z

staging/src/k8s.io/apiserver/pkg/util/proxy/proxy.go

+		// If there are multiple slices, we want to look at them in a random
+		// order, but we need to look at all of the slices of the primary IP
+		// family first.
+		preferredAddressType := discoveryv1.AddressType(svc.Spec.IPFamilies[0])


ensure svc.Spec.IPFamilies is non-zero length before indexing into [0], since there are circumstances where IPFamilies is blank

IPFamilies can only be blank if the Service is ExternalName or headless, but it appears that ResolveEndpoint does allow you to use a headless Service. OK.

EDIT: Actually, from what I can tell, IPFamilies seems to get defaulted (via registry, not apis) even for headless services. However, I definitely couldn't prove that that's the case so I'll add the check anyway...

liggitt · 2025-03-18T14:19:29Z

staging/src/k8s.io/apiserver/pkg/util/proxy/proxy.go

+				ep := &slice.Endpoints[(epi+offset)%len(slice.Endpoints)]
+				if ep.Conditions.Ready == nil || *ep.Conditions.Ready {
+					ip := ep.Addresses[0]
+					port := int(*slice.Ports[i].Port)


maybe I'm missing it, but it still looks like we can hit a nil port and panic here? should we add || slice.Ports[i].Port == nil to the conditions above where we just continue?

liggitt · 2025-03-18T14:49:15Z

staging/src/k8s.io/apiserver/pkg/util/proxy/proxy.go

@@ -70,32 +76,50 @@ func ResolveEndpoint(services listersv1.ServiceLister, endpoints listersv1.Endpo
 		return nil, err
 	}

-	eps, err := endpoints.Endpoints(namespace).Get(svc.Name)
+	endpointSliceSelector := labels.SelectorFromSet(labels.Set{discoveryv1.LabelServiceName: svc.Name})
+	slices, err := endpointSlices.EndpointSlices(namespace).List(endpointSliceSelector)


This changes an O(1) lookup by name into a O(n) selection.

I don't expect this to be an issue under normal circumstances, but when there are bananas numbers of services in a single namespace, this could be observably more expensive. We sort of won't know until this change gets into the wild what clusters with unexpectedly large numbers of endpointslices in a namespace this lookup degrades.

Should we add a namespaceName+svcName indexer onto the endpointslice informer/lister, and make use of that to keep from having to traverse / do selection on all endpointslices in a namespace on every webhook / aggregated server dial?

The top commit of https://github.com/liggitt/kubernetes/commits/aggregated-apiserver-endpointslices/ has a sketch of what that could look like. I'm on the fence...

do selection on all endpointslices in a namespace on every webhook

oh... huh, I was assuming that because this function is only called from within staging/src/k8s.io/kube-aggregator, that that meant it was only used for aggregated apiserver stuff (and thus "bananas numbers of services in the same namespace" was unlikely to be a problem). Looks like the API outgrew its original use case.

I'm on the fence...

Yeah, I understand the general concern, but I don't have any sense of if it matters in this specific case...

(Though if we add EndpointSliceGetter it should eventually migrate somewhere more public/generic, possibly as part of #124777.)

We are limited by IPs in Services so it will unlikely to have 1M of Services (famous last words :) ) but with 100k we already take a 7 ms hits that is really high for http requests #130967

1M Services isn't unreasonable - folks have reached a third of that using Knative and ended up re-architecting it to remove the use of services

See: https://render.com/blog/knative

folks have reached a third of that using Knative

glad to know, and I also heard good feedback from that community with the ServiceCIDR feature since they seems to consume those IPs very quickly, but still feel is a long shot to get there, those are a lot of ips , 2^20 = 1,048,576 , means they have to use a /12 mask ... I'll be the most happy person in the world if we get to the 1M of Services

liggitt · 2025-03-18T15:20:55Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller.go

-			return err
-		} else if err != nil {
+		endpointSliceSelector := labels.SelectorFromSet(labels.Set{discoveryv1.LabelServiceName: apiService.Spec.Service.Name})
+		endpointSlices, err := c.endpointSliceLister.EndpointSlices(apiService.Spec.Service.Namespace).List(endpointSliceSelector)


same comment about wondering if this should make use of an indexer, the POC commit I linked to also fixes up this call site

staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller.go

k8s-ci-robot · 2025-03-18T18:04:07Z

New changes are detected. LGTM label has been removed.

Co-Authored-by: Jordan Liggitt <[email protected]>

…Slices

k8s-ci-robot · 2025-05-26T17:21:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danwinship
Once this PR has been reviewed and has the lgtm label, please ask for approval from liggitt. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

cmd/kube-apiserver/OWNERS
staging/src/k8s.io/apiserver/OWNERS
~~staging/src/k8s.io/endpointslice/OWNERS~~ [danwinship]
staging/src/k8s.io/kube-aggregator/OWNERS
~~test/e2e/network/OWNERS~~ [danwinship]

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

danwinship · 2025-05-26T17:28:22Z

Now includes an updated version of Jordan's "Sketch of indexing on endpointslice namespace/service-name". As compared to that:

The code is moved to k8s.io/endpointslice/util.
Fixed a race condition if two different goroutines tried to create an EndpointSliceIndexerGetter off the same indexer at the same time.
The new indexer now skips EndpointSlices that don't have the service-name label.
Has unit tests and slightly-improved documentation.

Plus the new push also includes the e2e test update that got bumped from #131732 because it depends specifically on whether the proxying code uses Endpoints or EndpointSlices.

k8s-ci-robot · 2025-05-26T18:25:29Z

@danwinship: The following tests 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-dependencies	`c141385`	link	true	`/test pull-kubernetes-dependencies`
pull-kubernetes-verify	`c141385`	link	true	`/test pull-kubernetes-verify`

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-sigs/prow repository. I understand the commands that are listed here.

danwinship · 2025-05-26T19:38:54Z

The code is moved to k8s.io/endpointslice/util.

which makes verify-imports.sh fail... not sure if I should update import-restrictions.yaml or move the code somewhere else. It's only used by apiserver stuff as of this commit, but it's theoretically more generically useful. (Though, FWIW, it would not ever be used by kube-proxy, because it would be inefficient there to re-construct the []*discoveryv1.EndpointSlice every time we needed the slices for a Service; we really need a modified Informer there, not a modified Lister.)

danwinship · 2025-06-03T13:11:02Z

@liggitt this is ready for review (though we need to decide where to put the code / how to deal with the verify-imports failure)

BenTheElder · 2025-06-03T18:18:02Z

Jordan is OOO through 6/6 (From slack status, github just says on vacation)

Prajyot-Parab · 2025-06-30T11:50:56Z

Hello @danwinship @liggitt
This PR has not been updated for a month, so I'd like to check what's the status. If there's anything we can do, please let us know. The code freeze is starting 02:00 UTC Friday 25th July 2025 (about 4 weeks from now). Please make sure the PR has both lgtm and approved labels before the code freeze.
Thanks!

k8s-ci-robot requested review from alexzielenski and enj January 27, 2025 12:52

k8s-ci-robot added the area/apiserver label Jan 27, 2025

danwinship force-pushed the aggregated-apiserver-endpointslices branch from 6e33fa6 to 3aed1e3 Compare January 27, 2025 20:03

k8s-ci-robot assigned deads2k, liggitt and aojea Jan 27, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 27, 2025

liggitt reviewed Jan 28, 2025

View reviewed changes

k8s-ci-robot requested a review from seans3 January 28, 2025 21:17

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 28, 2025

danwinship mentioned this pull request Jan 29, 2025

Try to clarify EndpointSlice semantics some more #129886

Merged

danwinship force-pushed the aggregated-apiserver-endpointslices branch from 3aed1e3 to 139a933 Compare January 29, 2025 18:45

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

liggitt reviewed Mar 18, 2025

View reviewed changes

danwinship force-pushed the aggregated-apiserver-endpointslices branch from 139a933 to f79dcf9 Compare March 18, 2025 18:04

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 18, 2025

k8s-ci-robot requested review from aojea and liggitt March 18, 2025 18:04

aojea mentioned this pull request Mar 21, 2025

benchmarks inefficiency on listers linear search lookup #130967

Merged

danwinship mentioned this pull request May 14, 2025

Fix two conformance tests to use EndpointSlices rather than Endpoints #131732

Merged

danwinship and others added 3 commits May 26, 2025 13:02

Add utilities for getting EndpointSlices for a Service

bded37f

Co-Authored-by: Jordan Liggitt <[email protected]>

Port aggregated apiserver discovery to EndpointSlices

09e6e85

Co-Authored-by: Jordan Liggitt <[email protected]>

Update "should proxy through a service and a pod" to look at Endpoint…

c141385

…Slices

danwinship force-pushed the aggregated-apiserver-endpointslices branch from f79dcf9 to c141385 Compare May 26, 2025 17:21

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 26, 2025

liggitt added this to the v1.34 milestone Jun 10, 2025

k8s-infra-ci-robot added this to [sig-release] Bug Triage Jun 10, 2025

Rajalakshmi-Girish moved this to Pending inclusion in [sig-release] Bug Triage Jun 30, 2025

Prajyot-Parab moved this from Pending inclusion to Tracked in [sig-release] Bug Triage Jun 30, 2025

Port aggregated apiserver discovery to EndpointSlices #129837

Are you sure you want to change the base?

Port aggregated apiserver discovery to EndpointSlices #129837

Uh oh!

Conversation

danwinship commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Does this PR introduce a user-facing change?

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

Uh oh!

aojea commented Jan 27, 2025

Uh oh!

aojea commented Jan 27, 2025

Uh oh!

danwinship commented Jan 27, 2025

Uh oh!

danwinship commented Jan 27, 2025

Uh oh!

aojea commented Jan 27, 2025

Uh oh!

k8s-ci-robot commented Jan 27, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BenTheElder commented Jan 28, 2025

Uh oh!

aojea commented Mar 17, 2025

Uh oh!

k8s-ci-robot commented Mar 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danwinship Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 18, 2025

Uh oh!

k8s-ci-robot commented May 26, 2025

Uh oh!

danwinship commented May 26, 2025

Uh oh!

k8s-ci-robot commented May 26, 2025

Uh oh!

danwinship commented May 26, 2025

Uh oh!

danwinship commented Jun 3, 2025

danwinship commented Jan 27, 2025 •

edited

Loading

danwinship Mar 18, 2025 •

edited

Loading

liggitt Mar 18, 2025 •

edited

Loading

aojea Mar 21, 2025 •

edited

Loading