KEP 4770: EndpointSlice Controller Flexibility

[LionelJouin]

• One-line PR description: KEP-4770: Create KEP document (Generic reconciler and disabling controller on particular services)

• Issue link: KEP 4770: EndpointSlice Controller Flexibility #4770

• Other comments:

[k8s-ci-robot]

Welcome @LionelJouin!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @LionelJouin. 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-sigs/prow repository.

[aojea]

we have the label managed-by already on the endpointslice controller and the ipaddress, I think we should be consistent and use the same here

rep -r managed-by staging/src/k8s.io/api
staging/src/k8s.io/api/discovery/v1/well_known_labels.go:       LabelManagedBy = "endpointslice.kubernetes.io/managed-by"
staging/src/k8s.io/api/discovery/v1beta1/well_known_labels.go:  LabelManagedBy = "endpointslice.kubernetes.io/managed-by"
staging/src/k8s.io/api/networking/v1alpha1/well_known_labels.go:        LabelManagedBy = "ipaddress.kubernetes.io/managed-by"
staging/src/k8s.io/api/networking/v1beta1/well_known_labels.go: LabelManagedBy = "ipaddress.kubernetes.io/managed-by"

Also, should we add this label to each Service (if it already does not have the label set) to indicate that is managed by the default endpoint slice controller? I kind of think this will be better

[LionelJouin]

Any name is fine to me. For reference, the service.kubernetes.io/service-proxy-name is not added to the service. As highlighted in #4771 (comment), a default controller name would have to be handled for the kubernetes controller manager.

[aojea]

yes, I added this comment just for opening the debate, I'd like to know if there are more thoughts on this

[robscott]

I think endpointslice.kubernetes.io/managed-by is probably fitting here. I'm not entirely sure whether we want this to apply to Endpoints as well though, if so, that could change what's best here.

[aojea]

This part I can not really understand it, maybe is explained later ...

[robscott]

+1, "Endpoints could be supported for the Kubernetes EndpointSlice Mirroring Controller" is particularly confusing to me.

[aojea]

See https://github.com/kubernetes/enhancements/pull/4771/files#r1685502488 , I think there is 2 things to flesh out here:

1. label name
2. default behavior, do we add a label with the default controller name or we assume no-label means default controller

[LionelJouin]

For reference, kube-proxy has no default controller name, non-labelled services are managed by kube-proxy.

[aojea]

yeah,, but those are decision from the past, if we have to do it again, will we do it the same?
I don't have much feedback from this feature beside using it to workaround cilium conformance tests :)
I think that least we should evaluate the different options and see pros and cons and decide

[aojea]

did a first pass, this KEP is missing some more explanation on the high level details, is too low level, it describe perfectly the changes you want to do in the reconciler logic but is not clear the motivations and how is this planned to be used, some more high level information for the reviewers to understand the changes will help ... an example of how these externals controllers plan to use it will be interesting .... an example of how an external endpoint slice controller consuming this library will be used per example

[LionelJouin]

did a first pass, this KEP is missing some more explanation on the high level details, is too low level, it describe perfectly the changes you want to do in the reconciler logic but is not clear the motivations and how is this planned to be used, some more high level information for the reviewers to understand the changes will help ... an example of how these externals controllers plan to use it will be interesting .... an example of how an external endpoint slice controller consuming this library will be used per example

Thank you for the review. Should I only add high level overview, or should I remove too low level details too? Do we have to provide a way to use the EndpointSlice Reconciler for external EndpointSlice Controller consumers? The example for the usage in the EndpointSlice and EndpointSlice Mirroring Controllers are there already, I was thinking maybe those are sufficient. Also, the usage might change in the future as written in the EndpointSlice Reconciler readme: There are NO compatibility guarantees for this repository, yet. It is in direct support of Kubernetes, so branches will track Kubernetes and be compatible with that repo..

[aojea]

I think is fine to keep it the way you have it, just expand , I'm trying to highlight that without understanding the motivations and the expectations of what your proposing is hard to fully understand all the details

[aojea]

/ok-to-test

[LionelJouin]

As discussed during the sig-network meeting on 1st August 2024 (Kubernetes SIG Network meeting 20240801), I removed the part making the EndpointSlice Reconciler generic. This could still be re-introduced later as part of a future improvement of this KEP or via a completely new KEP. For reference, the commits containing the description of these changes are still available. A large part of the implementation is also available here: kubernetes/kubernetes#126304

Otherwise the part about the label, the question is about whether or not to keep the possibility to add/remove/update the label without re-creating the service. To me, keeping the mutability of the label simplifies the implementation as the EndpointSlice Controller would consider the service as deleted if the label is added (as in the implementation we should change the informer passed to the controller to not select labeled services). So, the deletion of the EndpointSlices should already be covered by other tests (Service creation/deletion).

For the use-cases on the mutability of the label, we could say the service must be switched from a controller to another, to, for example, switch between network providers (Multus to Multi-Network), but I am not sure if this will really be used, so I haven't added it.

Another argument about keeping the mutability was that it will probably be used together with service.kubernetes.io/service-proxy-name, and this label can be mutated.

[danwinship]

You should explain what KEP-3685 is rather than just referring to it by number, but I think this is mostly talking about the withdrawn plan to extend the existing reconciler anyway, so probably it should have been deleted?

[LionelJouin]

Thank you, I added the short description of the KEP. No, I think this is an additional argument as an implementation is required when using the label, so the EndpointSlice Reconciler in staging could be used as reference.

[danwinship]

I think it would be useful as a clarification to add "Changing the behavior of kube-proxy" as a Non-Goal: Kube-proxy doesn't need to care who creates the EndpointSlices for a Service or what logic they use to do so. It just knows that, unless the service has a service-proxy-name label indicating otherwise, it should just create rules pointing to its endpoints. Right?

And likewise cloud load balancers?

[LionelJouin]

Thank you, I added it. Yes, this applies to any service (described in the next section: "When set on a service, no matter the service specs...")

[danwinship]

Suggested change

	The Kubernetes Controller Manager will implement this label both at object creation and on dynamic addition/removal/updates of this label.
	The endpoint-related controllers will obey this label both at object creation and on dynamic addition/removal/updates of this label.

[danwinship]

It's not clear where you're using "service" to mean v1.Service and where you're using it in a generic sense...

How is the Service configured? Is it type: ClusterIP? Does it have the service-proxy-name label set?

Who is calling the Service and from where? (Pods that share the same secondary network? Pods that don't share the same secondary network? Nodes? Cluster-external hosts?) What is the expectation for if the "wrong" people try to connect to it?

(You have both "for example, [Multus]" and "e.g. [Multus]" there btw. Second one can probably go away.)

[LionelJouin]

I always meant the Kubernetes service, I added it for clarity, thank you.
I also added the mention to the service-proxy label. But I think the rest of the configuration of the service doesn't really matter, it could be both internal service traffic or traffic originating from outside of the cluster. I am not sure we should try to fully describe secondary networks and services here.

[danwinship]

But I think the rest of the configuration of the service doesn't really matter

Ideally, user stories should help the reader understand why you made some decisions and not others. But they can only do that if the user stories are sufficiently clear and evocative.

(And you especially don't want your user stories to be making the reviewers think about things like "but how will type: LoadBalancer services work if the load balancer doesn't have access to the secondary network?")

[danwinship]

(if it's "N/A" you can just remove the section, since it's "(Optional)")

[LionelJouin]

I will remove them later, if anyone has any concern that some are in fact applicable.

[danwinship]

Some of these aren't N/A.

e.g., if a cluster is making use of this feature, and you roll back to a version of kcm that doesn't know about the label, then that kcm will start overwriting your custom endpoints. So that's something we need to warn users about.

[LionelJouin]

I added it for disabling and re-enabling the feature, I will have to check the testing.

[LionelJouin]

I added a test in the integration tests for when the feature gate is enabled and disabled.

[aojea]

@robscott @danwinship @thockin The mutability here is a friction point, as this is "the same" as removing the selector on a Service , something we didn't reach a consensus yet and this is defining a behavior that is inconsistent with current state kubernetes/kubernetes#118376.

I'm leaning more to avoid to deal with Updates on Services, specially because I can not understand why one Service with one selector managed by the cluster endpoint slice controller will switch to an external one slice controller, it seems that should be better handled by deleting and recreating the Service.

This will be straight forward to implement, just validating on the Service API that this special label can not be added or mutated after the Service has been created

[danwinship]

If the annotation is mutable (which it has to be) then we have to deal with changes to it. Even if kcm tries to ignore changes, what happens if you restart kcm after changing the annotation?

I feel like 118376 was mostly waiting for Rob to bless a solution, and then we all stopped thinking about it.

[LionelJouin]

I am not sure if I miss something. To me, 118376 is different issue than the one describe in this KEP.

Here is how I see the implementation for this KEP: The service informer passed to the EndpointSlice Controller will have a selector to not include the services that are labeled with service.kubernetes.io/endpoint-controller-name. This means the controller will consider adding the label as if the service has been removed (which should already be covered as of now: when you remove a service, the EndpointSlices for it are removed).

If KCM is restarting, then the EndpointSlice controller will see the previous EndpointSlice(s) and enqueue the service associated (service name is in the EndpointSlice annotation). The service will be considered as non-existing (as the informer is not selecting labeled service), so the EndpointSlice(s) will be deleted.

Here is the implementation: kubernetes/kubernetes@master...LionelJouin:kubernetes:custom-endpointslice-controller

[danwinship]

The similarity between this and 118376 is that in both cases someone (potentially) has a Service with endpoints that have been generated in a particular way, and then later they want to change the Service so that the endpoints will be generated in a different way. Antonio was suggesting that if we couldn't figure out a solution for 118376, then we won't be able to come up with a solution for this either (and so we should forbid the case where you change things after the endpoints have already been generated), and I was disputing that we couldn't figure out a solution for 118376.

[LionelJouin]

I saw this comment: kubernetes/kubernetes#103576 (comment). The behavior that I see with this KEP is different. To me, the first interpretation seems more logical for this KEP, so if we add the label, controllers are disabled for the service, and objects (Endpoints and EndpointSlices) previously created by the KCM are deleted.

[aojea]

If the annotation is mutable (which it has to be)

I think you mean label instead of annotation ... why it has to be mutable?

if we add the label, controllers are disabled for the service, and objects (Endpoints and EndpointSlices) previously created by the KCM are deleted

Here is the implementation: kubernetes/kubernetes@master...LionelJouin:kubernetes:custom-endpointslice-controller

I see, so this is using this property of the informers so the controller "sees" a Delete event when the label is added, with 118376 the problem is that it "sees" an Update event ... I can not think of any edge cases from the top of my head, I think it may work ...

@robscott WDYT? maybe should we modify the controllers to process the selector removal as a delete event too?

[aojea]

I suggest another name, ExternalEndpointSlicesController or similar

[LionelJouin]

I renamed it to ExternalEndpointController (to be aligned with the label service.kubernetes.io/endpoint-controller-name)

[aojea]

There has to be some e2e conformance of these out of tree controllers, since is an external controller adding an endpointslice, how do we validate these out of tree controllers are generating the endpointslices accordingly ? what if these controllers does not respect the selectors and generete the endpointslices anyway?

@robscott I really like to hear your thoughts on this

[robscott]

That's an interesting thought. I'd assumed that the purpose of this KEP was to build controllers that would intentionally have different behavior than the default in-tree EndpointSlice controller. In some cases that might be to select a different IP for a Pod, but in other cases that might be to select an entirely different type of resource, for example a custom backend type.

This does certainly make it more likely that unexpected variations of EndpointSlices will exist, which is certainly something to watch out for. Since the outset of the EndpointSlice API anyone has been able to create EndpointSlices, and sometimes unexpected variations have led to bugs in controllers consuming those EndpointSlices, but I don't think that's necessarily new with this KEP, just potentially a more common set of edge cases that we'll need to be sure that controller authors are aware of.

For example, nodeName is optional in EndpointSlices, but some controllers likely assume it exists because the EndpointSlice controllers in k/k generally populate it. The same is largely true for zone and hostname (see podToEndpoint for a reference).

I think this risk is unavoidable with this KEP, so I'd like to see it called out + a corresponding commitment to improved documentation around this as a requirement for graduating past alpha. Unfortunately I can't think of a way to write conformance tests that would help us here though.

/cc @swetharepakula @gauravkghildiyal

[aojea]

overall this LGTM, there may be some edges to flesh out, but this feature looks clear and scoped now

[maiqueb]

/cc

[k8s-ci-robot]

@maiqueb: GitHub didn't allow me to request PR reviews from the following users: maiqueb.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc

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.

[LionelJouin]

Thank you @aojea for the review. Some of your comments might be resolved, but I am not sure, could you mark them as resolved if they are, so it's more clear what I still need to answer and fix:

• KEP 4770: EndpointSlice Controller Flexibility #4771 (comment) - Use endpointslice.kubernetes.io/managed-by instead of service.kubernetes.io/endpoint-controller-name
  • related to KEP 4770: EndpointSlice Controller Flexibility #4771 (comment)
• KEP 4770: EndpointSlice Controller Flexibility #4771 (comment) - Unclear summary
• KEP 4770: EndpointSlice Controller Flexibility #4771 (comment) - Only unlabelled services are handled by KCM / Default label value for KCM
• KEP 4770: EndpointSlice Controller Flexibility #4771 (comment) - E2E tests
• KEP 4770: EndpointSlice Controller Flexibility #4771 (comment) - Mutability of the well-known label

[thockin]

I'm -1 on this KEP, because I don't see why it is better than the existing mechanism (empty selector).

[thockin]

My initial reaction to this KEP is that we should not do it. This alternative is better for a bunch of reasons, but mostly because it already exists.

We have a way of expressing "don't generate endpoints for me" - leave the selector blank. Fortunately for you, it means exactly what you want.

The argument of inconsistency across controllers doesn't hold a lot of water for me. If you specify a pod selector, you're going to get pods as endpoints. If you don't specify it, you get something else. IMO, being able to specify that something else in your own schema is an advantage, not "fragmentation". Suppose you need an expression that is more flexible than the legacy selector-map (e.g. modern selectors can use "In" and "NotIn" and other operators).

This is not an accidental side-effect that I am trying to shoe-horn you into, it's literally why this mechanism was designed.

[LionelJouin]

The empty selector feature exists to cover use-cases that are not covered by the existing selector, i.e. selecting backends that are not selectable via labels. For example (from the Service documentation), selecting backends in a different namespace/cluster, selecting backends that are running outside of a Kubernetes, or as you mentioned, selecting backends with a more flexible expression covered with modern selectors. In these cases I agree the empty selector must be used and the users must define their own way to select backends, via, for example, annotations.

I realized that the goals and non-goals are not clear enough, so I fixed them in the latest commit. The goal is to disable the Kubernetes EndpointSlice controller and allow the user to re-use the selector field from the Service Spec. Custom EndpointSlice Controller should be able to replicate the usage of the selector, by selecting workloads by labels (labels specified in the selector field) but the difference with the Kubernetes EndpointSlice Controller is the compilation of these EndpointSlices, in this KEP the main use case is to select different IPs than the Pod.Status.PodIPs.

A well-known label would also provide an explicit and standard way to inform which component is managing the EndpointSlices for a particular service.

[thockin]

I am still struggling with this one. I spoke with Rob a little, since he seemed "pro". The argument that carried the most weight for me was that there are (or might be?) cases where the alternative controller is doing exactly what the "real" controller does but extracting some different information (e.g. the multi-network case). In that case, you have to specify the network(s) somewhere right? Another example, albeit hypothetical, was something doing different topology hinting.

I'm not sure how I feel about this, still. It seems "simple" to implement, but I am worried about the tail of side-effects.

As a thought experiment - what if you could specify a function, e.g. CEL, which took a pod as argument and returned a list of IPs to use -- would that eliminate the need for this?

[LionelJouin]

Yes, that's correct, multi-network is my primary use case, I have personally no plan, at least for now, to do something different such as topology hinting. But, multi-network also means more than just picking the IP for the pods, it also means the network must be attached and up for the selected pods.

Yes, I need to specify which network(s) must be used for the selected pods. Having this label (service.kubernetes.io/endpoint-controller-name) will give me freedom to choose the way to specify those networks. For my case, I am currently thinking of using Services together with Gateway API (I made a PoC about it: https://github.com/LionelJouin/l-3-4-gateway-api-poc). Gateway in Gateway API allows me to specify new Service-Proxies (Secondary Network Service (load-balancing) acts directly in the Gateway instance) and attach networks via the Gateway.Spec.Infrastructure field, this way I can also specify which networks must be selected. Here is an example on how the Service/Gateway could look like: https://github.com/LionelJouin/l-3-4-gateway-api-poc/blob/main/examples/kpng-gateway-api.yaml#L59-L62.

If I understand correctly, CEL would select a specific field based on what the user would specify via a new field in the Service. I think this would be more complex to maintain, more difficult to use, and less flexible. As of today, the most used solution is Multus, the secondary networks are attached and reported via pod annotation in a json format, I guess it would be quite challenging to match the IPs for a particular network. This could have worked with the Multi-Network KEP changes since the Secondary Network IPs would have been reported in Pod.Status. Now the discussion for multi-network is about DRA and ResourceClaim.Status to report the Secondary Network IPs, I am not sure how CEL could fit here.

For reference, at the beginning, another goal for this KEP was to improve the Kubernetes EndpointSlice Reconciler to make it generic enough so it can be re-used for Secondary Network IPs (No matter if Multus, KEP#3698 or DRA is used). This way, the custom EndpointSlice Controllers for secondary networks would be quite easy to implement. I kept it in the commit history (removed in this commit) and I am still planning to propose again these changes somehow in a better way in the future.

[aojea]

For reference, at the beginning, another goal for this KEP was to improve the Kubernetes EndpointSlice Reconciler to make it generic enough so it can be re-used for Secondary Network IPs (No matter if Multus, KEP#3698 or DRA is used). This way, the custom EndpointSlice Controllers for secondary networks would be quite easy to implement. I kept it in the commit history (removed in this commit) and I am still planning to propose again these changes somehow in a better way in the future.

This is a big no for me, if someone wants to create EndpointSlices controllers out of tree is ok to share some common types and logic but adding more load to the core maintainers just to remove from the custom endpointslice controllers maintainers is simply unfair.

[LionelJouin]

This is a big no for me, if someone wants to create EndpointSlices controllers out of tree is ok to share some common types and logic but adding more load to the core maintainers just to remove from the custom endpointslice controllers maintainers is simply unfair.

I agree with this, the idea with this part (generic EndpointSlice Reconciler) of the KEP was to solve what is stated in the readme of EndpointSlice Reconciler: This EndpointSlice reconciler library is not sufficiently generic to be used by the EndpointSlice Mirroring controller.... But also keeping what is written for compatibility: There are NO compatibility guarantees for this repository, yet. It is in direct support of Kubernetes, so branches will track Kubernetes and be compatible with that repo..

[thockin]

How to proceed? Time is running for KEPs.

@robscott @aojea @danwinship @shaneutt

[thockin]

This counts as a point in favor of "use an empty selector". But also, that just seems broken. :)

[thockin]

I am still struggling with this one. I spoke with Rob a little, since he seemed "pro". The argument that carried the most weight for me was that there are (or might be?) cases where the alternative controller is doing exactly what the "real" controller does but extracting some different information (e.g. the multi-network case). In that case, you have to specify the network(s) somewhere right? Another example, albeit hypothetical, was something doing different topology hinting.

I'm not sure how I feel about this, still. It seems "simple" to implement, but I am worried about the tail of side-effects.

As a thought experiment - what if you could specify a function, e.g. CEL, which took a pod as argument and returned a list of IPs to use -- would that eliminate the need for this?

[shaneutt]

I'm generally in favor of this one, since I see it as door opening for a lot of implementations that don't want to be a complete replacement, and I think this optionality is very good for users at a reasonably low maintenance cost.

/approve
/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LionelJouin, shaneutt

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:

• keps/sig-network/OWNERS [shaneutt]

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

[thockin]

Let's hold off on merge until everyone has a minute to think about it :)

[aojea]

I'm not in favor of merging because we are constantly bitten by edge problems with Endpoints and EndpointSlices, more recently kubernetes/kubernetes#127370 , this just adds another permutation to maintain in an already complicated area and there are alternatives, like completely replacing the endpointslice controller on the kube-controller-manager or empty selector.

However, in case we move forward, I expect beta criteria to have at least 2 implementations of these endpointslices controller out of tree in use, adding features is expensive, at least we need to be sure that the cost we are paying is worth it.