KEP 4770: EndpointSlice Controller Flexibility #4771
Conversation
LionelJouin
commented
Jul 19, 2024
LionelJouin
commented
- One-line PR description: KEP-4770: Create KEP document (Generic reconciler and disabling controller on particular services)
- Issue link: EndpointSlice controller flexibility #4770
- Other comments:
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @LionelJouin! |
k8s-ci-robot
added
needs-ok-to-test
|
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 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-sigs/prow repository. |
k8s-ci-robot
added
sig/network
Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>
|
|
||
| ## Summary | ||
|
|
||
| This proposal adds a new well-known label `service.kubernetes.io/endpoint-controller-name` to Kubernetes Services. This label disables the default Kubernetes EndpointSlice controller for the services where this label is applied and delegates the control of EndpointSlices to a custom EndpointSlice controller. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yes, I added this comment just for opening the debate, I'd like to know if there are more thoughts on this
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This KEP really does need to explain how it interacts with the existing label.
There was a problem hiding this comment.
I am not sure what exactly has to be clarified with existing labels. This KEP proposes a label on the Service object, endpointslice.kubernetes.io/managed-by is already explained in the Kubernetes documentation.
There was a problem hiding this comment.
Make sure that someone reading this KEP is clear on why endpointslice.kubernetes.io/managed-by isn't right and service.kubernetes.io/service-proxy-name isn't right (bear in mind that at the time of writing the docs say you can set service.kubernetes.io/service-proxy-name on a Service and get a useful outcome).
If a KEP isn't clear:
- we shouldn't implement it (we should clarify, then implement)
- someone might implement the wrong thing through a misunderstanding
Making the KEP clear helps to manage that risk.
There was a problem hiding this comment.
Thank you for the explanation, I tried to clarify the differences and how the endpointslice.kubernetes.io/managed-by label is impacted.
|
|
||
| This proposal adds a new well-known label `service.kubernetes.io/endpoint-controller-name` to Kubernetes Services. This label disables the default Kubernetes EndpointSlice controller for the services where this label is applied and delegates the control of EndpointSlices to a custom EndpointSlice controller. | ||
|
|
||
| Additionally, this KEP aims to give more flexibility for the EndpointSlice Reconciler module to allow users to reconcile EndpointSlices with any type of resources (Currently only Service/Pods is supported). For example, Endpoints could be supported for the Kubernetes EndpointSlice Mirroring Controller. |
There was a problem hiding this comment.
This part I can not really understand it, maybe is explained later ...
There was a problem hiding this comment.
+1, "Endpoints could be supported for the Kubernetes EndpointSlice Mirroring Controller" is particularly confusing to me.
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
|
|
||
| ### Well-Known Label | ||
|
|
||
| The kube-controller-manager should pass to the Endpoints, EndpointSlice and EndpointSlice Mirroring Controllers an informer selecting services that are not labelled with `service.kubernetes.io/endpoint-controller-name`. |
There was a problem hiding this comment.
See https://github.com/kubernetes/enhancements/pull/4771/files#r1685502488 , I think there is 2 things to flesh out here:
- label name
- default behavior, do we add a label with the default controller name or we assume no-label means default controller
There was a problem hiding this comment.
For reference, kube-proxy has no default controller name, non-labelled services are managed by kube-proxy.
There was a problem hiding this comment.
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
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
|
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: |
|
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 |
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
|
/ok-to-test |
shaneutt
added
the
do-not-merge/hold
|
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. |
There was a problem hiding this comment.
@LionelJouin this KEP needs to explain how it interacts with the similar, existing label endpointslice.kubernetes.io/managed-by.
See https://kubernetes.io/docs/concepts/services-networking/endpoint-slices/#management
We could add an equivalent label for Endpoints and even automatically mirror the label when mirroring an Endpoints to a set of EndpointSlices.
|
@shaneutt please see #4771 (review) If you agree, please consider cancelling approval. |
|
|
||
| ## Motivation | ||
|
|
||
| As of now, a service can be delegated to a custom Service-Proxy/Gateway if the label `service.kubernetes.io/service-proxy-name` is set. Introduced in [KEP-2447](https://github.com/kubernetes/enhancements/issues/2447), this allows custom Service-Proxies/Gateways to implement services in different ways to address different purposes / use-cases. However, the EndpointSlices attached to this service will still be reconciled in the same way as any other service. Addressing more purposes / use-cases, for example, different pod IP addresses, is therefore not natively possible. |
There was a problem hiding this comment.
Is this true? What if you define a Service without a selector and then do your own reconciliation?
There was a problem hiding this comment.
Thank you for your comment, I clarified that it is not natively possible while using the Service Selector field (The KEP is about this, the fact to use the service as close as possible to how a user would define it for the Kubernetes default network).
About this topic, there are details in this conversation: #4771 (comment)
shaneutt
removed
the
approved
k8s-ci-robot
added
the
approved
| The list of controllers to enable in the Kube-Controller-Manager can be set using the `--controllers` flag ([kube-controller-manager documentation](https://kubernetes.io/docs/reference/command-line-tools-reference/kube-controller-manager/)). The EndpointSlice can then be disabled in the Kube-Controller-Manager and implemented as an external one that will support the label feature described in this KEP. | ||
|
|
||
| This alternative requires significant changes to the cluster management as the cluster level configuration must be modified and a new EndpointSlice controller for the primary network must be developed and deployed to replace the disabled one in the Kube-Controller-Manager. | ||
|
|
There was a problem hiding this comment.
We should - I pretty much think must - document the "don't specify a selector, then do your own thing" option. It is an alternative and we shouldn't pretend otherwise.
There was a problem hiding this comment.
This is covered by the first alternative Well-known label: Empty Selector field and Use Annotation as Selector, I renamed the alternative so it is more clear.
There was a problem hiding this comment.
BTW you can't select on annotations.
There was a problem hiding this comment.
you can create your own selector in the annotation, here is an example.
apiVersion: v1
kind: Service
metadata:
name: service
annotations:
selector: "app=a"
spec: {}
aojea
removed
the
approved
k8s-ci-robot
added
the
approved
| metadata: | ||
| name: service | ||
| spec: | ||
| selector: |
There was a problem hiding this comment.
This isn't a well-known label.
There was a problem hiding this comment.
If you refer to the title, sorry for the confusion, originally, the KEP had 2 proposals (well known label + generic endpointslice reconciler), Well-known Label indicates it is an alternative for the well-known label proposal.
There was a problem hiding this comment.
I removed it from the title of the alternative.
|
|
||
| This alternative potentially leads to confusion among users and inconsistency in how services are managed as each implementation is using its own annotation (see the nokia/danm and projectcalico/vpp-dataplane examples), leading to a fragmented approach. | ||
|
|
||
| ### Well-known label: Use Dummy Selector |
There was a problem hiding this comment.
Try this:
| ### Well-known label: Use Dummy Selector | |
| ### Private (per cluster) dummy selector label, custom controller |
If you don't mean for the dummy selector to use a cluster-private label, you should revise the explanation.
There was a problem hiding this comment.
I am not sure what you mean here. the label in the selector are always within the cluster no?
|
|
||
| ## Alternatives | ||
|
|
||
| ### Well-known label: Empty Selector field and Use Annotation as Selector |
There was a problem hiding this comment.
I'm not sure if you mean
### Empty selector field; pseudo-selector in annotation; no well-known labelThis alternative doesn't seem to use any well-known label.
There was a problem hiding this comment.
I removed it from the title of the alternative.
Originally, the KEP had 2 proposals (well known label + generic endpointslice reconciler), Well-known Label indicates it is an alternative for the well-known label proposal.
keps/sig-network/4770-endpointslice-controller-flexibility/README.md
Outdated
Show resolved
Hide resolved
LionelJouin
force-pushed
the
kep-4770
branch
2 times, most recently
from
03d67fa to
d50bdd0
Compare
|
|
||
| In the Endpoints, EndpointSlice and EndpointSlice Mirroring Controllers, the behavior to create Endpoints/EndpointSlices on service creation and the behavior to delete the Endpoints/EndpointSlices on service deletion is already in place. Only the service informer passed to these controllers must be tweaked for the proposed well-known label (`service.kubernetes.io/endpoint-controller-name`) to work properly. | ||
|
|
||
| This new well-known label will not have any impact on the other well-known label `endpointslice.kubernetes.io/managed-by`. Both labels can coexist and be used together, ``service.kubernetes.io/endpoint-controller-name` on the Service object while `endpointslice.kubernetes.io/managed-by` on the EndpointSlice object. The label `endpointslice.kubernetes.io/managed-by` is used to indicate the controller or entity that manages the EndpointSlice. In contrast, this new label `service.kubernetes.io/endpoint-controller-name`, is used to disable the default Kubernetes EndpointSlice, EndpointSlice Mirroring and Endpoints controllers for a Service and indicates the controller or entity that manages the EndpointSlice for that particular Service. |
There was a problem hiding this comment.
| This new well-known label will not have any impact on the other well-known label `endpointslice.kubernetes.io/managed-by`. Both labels can coexist and be used together, ``service.kubernetes.io/endpoint-controller-name` on the Service object while `endpointslice.kubernetes.io/managed-by` on the EndpointSlice object. The label `endpointslice.kubernetes.io/managed-by` is used to indicate the controller or entity that manages the EndpointSlice. In contrast, this new label `service.kubernetes.io/endpoint-controller-name`, is used to disable the default Kubernetes EndpointSlice, EndpointSlice Mirroring and Endpoints controllers for a Service and indicates the controller or entity that manages the EndpointSlice for that particular Service. | |
| This new well-known label will not have any impact on the other well-known label `endpointslice.kubernetes.io/managed-by`. Both labels can coexist and be used together, `service.kubernetes.io/endpoint-controller-name` on the Service object while `endpointslice.kubernetes.io/managed-by` on the EndpointSlice object. The label `endpointslice.kubernetes.io/managed-by` is used to indicate the controller or entity that manages the EndpointSlice(s) for that Service. In contrast, this new label `service.kubernetes.io/endpoint-controller-name`, is used to disable the default Kubernetes EndpointSlice, EndpointSlice Mirroring and Endpoints controllers for a Service and indicates the controller or entity that manages the EndpointSlices for that particular Service. |
There was a problem hiding this comment.
The label endpointslice.kubernetes.io/managed-by is used to indicate the controller or entity that manages an EndpointSlice. Adding "for that Service" is more confusing to me as the label endpointslice.kubernetes.io/managed-by is applied on an EndpointSlice object, not a Service object.
There was a problem hiding this comment.
OK, try this:
This new well-known label will not have any impact on the other well-known label `endpointslice.kubernetes.io/managed-by`. Both labels can coexist and be used together, `service.kubernetes.io/endpoint-controller-name` on the Service object while `endpointslice.kubernetes.io/managed-by` on the EndpointSlice object. The label `endpointslice.kubernetes.io/managed-by` is used to indicate the controller or entity that manages the EndpointSlice(s) - for, example, the endpoint slice controller managing endpoints on behalf of a Service.
In contrast, this new label `service.kubernetes.io/endpoint-controller-name`, is used to disable the default Kubernetes endpoint slice, Endpoints-to-EndpointSlice mirroring and endpoints controllers for a Service; the value of the new label indicates the controller or entity that manages the EndpointSlices for that particular Service.How does that sound?
|
|
||
| ##### Unit tests | ||
|
|
||
| TDB |
| - Initial unit, integration and e2e tests completed and enabled. | ||
|
|
||
| #### Beta | ||
|
|
There was a problem hiding this comment.
I wonder if we want to test that EndpointSlices created before a Service still work how we want them to (kube-proxy behavior for example), even after that Service is eventually added?
|
|
||
| ### Empty Selector field and Use Annotation as Selector | ||
|
|
||
| Services without selectors will not get any EndpointSlice objects. Therefore, selecting pods can be done in different ways, for example, via annotation. An annotation will be used in the service to select which pods will be used as backend for this service. For example, [nokia/danm](https://github.com/nokia/danm) uses `danm.k8s.io/selector` (e.g. [DANM service declaration](https://github.com/nokia/danm/blob/v4.3.0/example/svcwatcher_demo/services/internal_lb_svc.yaml#L7)), and [projectcalico/vpp-dataplane](https://github.com/projectcalico/vpp-dataplane) uses `extensions.projectcalico.org/selector` (e.g. [Calico-VPP Multinet services](https://github.com/projectcalico/vpp-dataplane/blob/v3.25.1/docs/multinet.md#multinet-services)). To simplify the user experience, a mutating webhook could read the selector, add them to the annotation and clear them from the specs when the type of service is detected. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| metadata: | ||
| name: service | ||
| spec: | ||
| selector: |
| The list of controllers to enable in the Kube-Controller-Manager can be set using the `--controllers` flag ([kube-controller-manager documentation](https://kubernetes.io/docs/reference/command-line-tools-reference/kube-controller-manager/)). The EndpointSlice can then be disabled in the Kube-Controller-Manager and implemented as an external one that will support the label feature described in this KEP. | ||
|
|
||
| This alternative requires significant changes to the cluster management as the cluster level configuration must be modified and a new EndpointSlice controller for the primary network must be developed and deployed to replace the disabled one in the Kube-Controller-Manager. | ||
|
|
|
|
||
| ### Empty Selector field and Use Annotation as Selector | ||
|
|
||
| Services without selectors will not get any EndpointSlice objects. Therefore, selecting pods can be done in different ways, for example, via annotation. An annotation will be used in the service to select which pods will be used as backend for this service. For example, [nokia/danm](https://github.com/nokia/danm) uses `danm.k8s.io/selector` (e.g. [DANM service declaration](https://github.com/nokia/danm/blob/v4.3.0/example/svcwatcher_demo/services/internal_lb_svc.yaml#L7)), and [projectcalico/vpp-dataplane](https://github.com/projectcalico/vpp-dataplane) uses `extensions.projectcalico.org/selector` (e.g. [Calico-VPP Multinet services](https://github.com/projectcalico/vpp-dataplane/blob/v3.25.1/docs/multinet.md#multinet-services)). To simplify the user experience, a mutating webhook could read the selector, add them to the annotation and clear them from the specs when the type of service is detected. |
There was a problem hiding this comment.
| Services without selectors will not get any EndpointSlice objects. Therefore, selecting pods can be done in different ways, for example, via annotation. An annotation will be used in the service to select which pods will be used as backend for this service. For example, [nokia/danm](https://github.com/nokia/danm) uses `danm.k8s.io/selector` (e.g. [DANM service declaration](https://github.com/nokia/danm/blob/v4.3.0/example/svcwatcher_demo/services/internal_lb_svc.yaml#L7)), and [projectcalico/vpp-dataplane](https://github.com/projectcalico/vpp-dataplane) uses `extensions.projectcalico.org/selector` (e.g. [Calico-VPP Multinet services](https://github.com/projectcalico/vpp-dataplane/blob/v3.25.1/docs/multinet.md#multinet-services)). To simplify the user experience, a mutating webhook could read the selector, add them to the annotation and clear them from the specs when the type of service is detected. | |
| Services without selectors will not get any EndpointSlice objects. Therefore, selecting pods can be done in different ways, for example, via annotation. An annotation will be used in the service to select which pods will be used as backend for this service. For example, [nokia/danm](https://github.com/nokia/danm) uses `danm.io/selector`, and [projectcalico/vpp-dataplane](https://github.com/projectcalico/vpp-dataplane) uses `extensions.projectcalico.org/selector` (e.g. [Calico-VPP Multinet services](https://github.com/projectcalico/vpp-dataplane/blob/v3.25.1/docs/multinet.md#multinet-services)). To simplify the user experience, a mutating webhook could read the selector, add them to the annotation and clear them from the specs when the type of service is detected. |
LionelJouin
force-pushed
the
kep-4770
branch
from
d50bdd0 to
d4a9dc9
Compare
|
/approve cancel I will try to avoid starting new work without finishing the current one,
also this is a nice to have feature, since projects already have a solution to this problem working for a long time, and anything we add in core is something we need to support, and AFAIK there is no open issues related to this, so I would infer that is not much of a problem or the use cases are not enough, the README in this repository is really enlightening https://github.com/k8snetworkplumbingwg/multus-service-archived
Also one of the projects that is mentioned has last commit on Sep 2022 https://github.com/nokia/danm/commits/master/ , latest release 2021, kubernetes 1.21 |
aojea
removed
the
approved
|
|
||
| ### Empty Selector field and Use Annotation as Selector | ||
|
|
||
| Services without selectors will not get any EndpointSlice objects. Therefore, selecting pods can be done in different ways, for example, via annotation. An annotation will be used in the service to select which pods will be used as backend for this service. For example, [nokia/danm](https://github.com/nokia/danm) uses `danm.k8s.io/selector` (e.g. [DANM service declaration](https://github.com/nokia/danm/blob/v4.3.0/example/svcwatcher_demo/services/internal_lb_svc.yaml#L7)), and [projectcalico/vpp-dataplane](https://github.com/projectcalico/vpp-dataplane) uses `extensions.projectcalico.org/selector` (e.g. [Calico-VPP Multinet services](https://github.com/projectcalico/vpp-dataplane/blob/v3.25.1/docs/multinet.md#multinet-services)). To simplify the user experience, a mutating webhook could read the selector, add them to the annotation and clear them from the specs when the type of service is detected. |
There was a problem hiding this comment.
This project last commit is from Sep 2022 https://github.com/nokia/danm/commits/master/
There was a problem hiding this comment.
I think for a ruled-out alternative, referencing something stale is OK. The ecosystem doesn't have to buy in to the thing that we, uh, plan to reject.
There was a problem hiding this comment.
I'm trying to gauge the importance of this feature and the demand, it seems there are no OSS implementations of this that are "alive" so I wonder if there are users ... or if there are only private implementations
|
Hi! Quick comment about a case in Cilium that this PR might help with. The initial goal of this PR is entirely different to what we would be using this for but we actually also use the endpointslice controller/reconciler code in Cilium clustermesh to sync EndpointSlice from remote clusters and allowing to have a more generic reconciler would certainly help us. This feature is called "endpointslicesync" (https://docs.cilium.io/en/latest/network/clustermesh/services/#endpointslicesync) and to do that we take the endpoints from remote clusters in our internal format that are pulled from the different clusters and we launch a almost identical EndpointSlice controller to the upstream one/the kubernetes one and fake the informers to masquerade our internal Endpoints data as regular pods and make the controller believe he is in a regular case/cluster while we do some manipulation to actually sync endpoints from remote clusters. All the code to do that is here https://github.com/cilium/cilium/tree/main/pkg/clustermesh/endpointslicesync. We currently have a fork of the EndpointSlice controller/reconciler with some very small patch to help us do that and more importantly isolate the reconciler code so that it's easier to import in our go mod. The forks are here: https://github.com/cilium/endpointslice-controller & https://github.com/cilium/endpointslice-controller (and yes we should also update those fork to keep up with the kubernetes changes 😅). This would most likely not remove all the tricks that we pulled of to be able to do that but it could be a first step to allow us to directly use the upstream code without forking it. And this might help future implementation attempting to add support for MCS-API and implementing this part of the spec: https://github.com/kubernetes/enhancements/blob/master/keps/sig-multicluster/1645-multi-cluster-services-api/README.md#using-endpointslice-objects-to-track-endpoints. While it would help us I entirely understand this might be a niche use case and you wouldn't want to complexify further the EndpointSlice controller for it. |
We can support a community version of an endpoint slice controller out of tree like |
|
/magic-emoji-reaction literally-this 😉 |
