EndpointSlice object is not removed when service selector is made to be empty

[pperiyasamy]

What happened?

Implementations like kube-proxy might direct service traffic to stale endpoint slice as if it was a valid endpoint.

What did you expect to happen?

The EndpointSlice (have service object as owner reference) object must get removed when service selector is set to empty.

How can we reproduce it (as minimally and precisely as possible)?

1. Create a Pod Deployment and Service with a corresponding selector label.
2. Ensure Endpoint and EndpointSlice (with service object as the owner reference) is created.
3. Edit the Service object by removing selector label.
4. Check how Endpoint and EndpointSlice objects are updated.

Anything else we need to know?

Actual Result:

The Endpoint and EndpointSlice (having service object as owner reference) object is untouched, a new EndpointSlice mirror object (having Endpoint object as owner reference) is created.

An update on the Endpoint object also modifies EndpointSlice mirror object and not the old endpointslice object (which is a desired behavior).

Observation:

The PR #105997 adds/removes mirrored EndpointSlice when selector is updated from non empty to empty and vice versa.
But EndpointSlice controller doesn't delete EndpointSlice object when service's selector is set to empty.

Kubernetes version

# kubectl version
Client Version: version.Info{Major:"1", Minor:"23", GitVersion:"v1.23.3", GitCommit:"816c97ab8cff8a1c72eccca1026f7820e93e0d25", GitTreeState:"clean", BuildDate:"2022-01-25T21:25:17Z", GoVersion:"go1.17.6", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"26", GitVersion:"v1.26.2", GitCommit:"fc04e732bb3e7198d2fa44efa5457c7c6f8c0f5b", GitTreeState:"clean", BuildDate:"2023-02-22T13:32:22Z", GoVersion:"go1.19.6", Compiler:"gc", Platform:"linux/amd64"}

Cloud provider

Any

OS version

# On Linux:
$ cat /etc/os-release
NAME=Fedora
VERSION="33 (Server Edition)"
ID=fedora
VERSION_ID=33
VERSION_CODENAME=""
PLATFORM_ID="platform:f33"
PRETTY_NAME="Fedora 33 (Server Edition)"
ANSI_COLOR="0;38;2;60;110;180"
LOGO=fedora-logo-icon
CPE_NAME="cpe:/o:fedoraproject:fedora:33"
HOME_URL="https://fedoraproject.org/"
DOCUMENTATION_URL="https://docs.fedoraproject.org/en-US/fedora/f33/system-administrators-guide/"
SUPPORT_URL="https://fedoraproject.org/wiki/Communicating_and_getting_help"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_BUGZILLA_PRODUCT="Fedora"
REDHAT_BUGZILLA_PRODUCT_VERSION=33
REDHAT_SUPPORT_PRODUCT="Fedora"
REDHAT_SUPPORT_PRODUCT_VERSION=33
PRIVACY_POLICY_URL="https://fedoraproject.org/wiki/Legal:PrivacyPolicy"
VARIANT="Server Edition"
VARIANT_ID=server
$ uname -a
Linux fedora-ovk 5.8.15-301.fc33.x86_64 #1 SMP Thu Oct 15 16:58:06 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

[pperiyasamy]

/sig network

[HirazawaUi]

/assign

[aojea]

yeah, this was discussed in several places

#103576

and as far as I remember we settled in leaving the object orphan since removing the selector of the Service is a conscious decision that removes the ownership of the endpoint or endpointslice from the controller ... it is a complex problem with a lot of edges, can you elaborate on the use case to remove the selector of the service @pperiyasamy ?

/cc @robscott

[danwinship]

The description here isn't very clear about what the problem is. The bug is:

1. Create a Service with a selector. The Endpoints controller will create a corresponding Endpoints, and the EndpointSlice controller will create an EndpointSlice. Good so far.
2. Remove the selector. Both the Endpoints and the EndpointSlice will be left in place, orphaned. Should update endpoints when delete service selector  #103576 explains the reasons for this.
3. Modify the Endpoints. Since the Endpoints object you modified is not owned by a Service, the EndpointSlice mirroring controller will now jump in and mirror it to a new EndpointSlice. Now you have two EndpointSlices, both pointing back to the original Service, but with different endpoints.

Note that step 3 (modifying the Endpoints by hand after removing the selector from the Service) is exactly the use case that was argued for in #103576 as a reason to not delete the Endpoints/EndpointSlice in step 2.

My theory on fixing this was that in step 2, the ownership of the EndpointSlice should be transferred from the EndpointSlice controller to the EndpointSlice mirroring controller.

[HirazawaUi]

I looked at some of the relevant issues and the document for EndpointSlices, do we need to show the declaration of this behavior in the document, i.e. remove the selector, the Endpoints and EndpointSlice will be left in place.

[HirazawaUi]

3. Modify the Endpoints. Since the Endpoints object you modified is not owned by a Service, the EndpointSlice mirroring controller will now jump in and mirror it to a new EndpointSlice. Now you have two EndpointSlices, both pointing back to the original Service, but with different endpoints.

May I submit a PR to fix the issue as you suggested?

[sftim]

I'm not sure the Endpoints should own the EndpointSlices - that means we have a (semi)deprecated object owning a stable, recommended API kind.

[sftim]

For any case where the behavior is likely to be surprising, and we can't use code to reduce the surprise directly, we could return a Warning header to let the client know they have done something that might need need thinking about.

[palonsoro]

I'm not sure the Endpoints should own the EndpointSlices - that means we have a (semi)deprecated object owning a stable, recommended API kind.

Why not? In fact, this is already the case: When you create a service without selector and the Endpoints are created manually, those Endpoints own the EndpointSlices. When you delete those Endpoints, you expect the mirrored EndpointSlices to be deleted.

If we are doing this already, why would it be a problem to just transfer the ownership as @danwinship suggested?

[palonsoro]

Basically, if I have a this

Service ---selector updates---> Endpoints ---are mirrored as---> EndpointSlices

And I remove the selector, I expect to cut only the first edge but keep the other one, i.e. to end up with

Service     (no edge here)      Endpoints ---are mirrored as---> EndpointSlices

Honestly, I can see neither a good use case nor a valid reason to keep the current odd behavior.

[aojea]

Modify the Endpoints. Since the Endpoints object you modified is not owned by a Service, the EndpointSlice mirroring controller will now jump in and mirror it to a new EndpointSlice. Now you have two EndpointSlices, both pointing back to the original Service, but with different endpoints.

So the mirroring controller creates a new EndpointSlice label but the managed-by label is different from the original

kind: EndpointSlice
metadata:
  labels:
    endpointslice.kubernetes.io/managed-by: endpointslicemirroring-controller.k8s.io
    kubernetes.io/service-name: myservice

and

  labels:
    endpointslice.kubernetes.io/managed-by: endpointslice-controller.k8s.io
    kubernetes.io/service-name: myservice

The problem with this triangle of dependencies between Services, Endpoints and EndpointSlices is that it has a lot of edge cases, and there can be more endpoint slice controllers or owners, I think that what we want is:

1. if an EndpointSlice exist pointing to a Service without selector
2. and is owned by the endpointslice.kubernetes.io/managed-by: endpointslice-controller.k8s.io
3. and an Endpoint already exist for this Service

Then the endpointslice mirroring controller takes ownership of this EndpointSlice

do we see any corner case we can break something @robscott @thockin ?

[sftim]

Why is it useful to have an EndpointSlice owned by an Endpoints?

[palonsoro]

Why is it useful to have an EndpointSlice owned by an Endpoints?

Reasons that come to my head:

• If we delete the Endpoints, we want to delete the EndpointSlices.
  • At least this has been the behavior so far.
  • It makes sense to me: Those EndpointSlices just exist to adapt the older Endpoints to the consumers who should only consume EndpointSlices from now on, so if those Endpoints are deleted, mirrored EndpointSlices should go away with them.
  • In short, mirroring mirrors whatever happens to the mirrored object, that should include its deletion.
• To properly track where do these EndpointSlices come from.

[sftim]

In short, mirroring mirrors whatever happens to the mirrored object, that should include its deletion.

I have this ambition that we reverse the mirroring: EndpointSlices (plural) that have a related Service would one day mirror to an Endpoints. That feels like the better approach because it makes it clear that Endpoints is the legacy API.

However, that ownership if we do it could get in the way. It's weird to have things mirror into their owner.

[aojea]

Why is it useful to have an EndpointSlice owned by an Endpoints?

The endpointslicemirroring mirrors Endpoints to EndpointSlices, that is its job and I
think we should think very careful how to implement this new relation slice-> endpoint, we already have these corner cases that are not well handled.

AFAIK the problem here is to decide if the EndpointSlice orphaned should be considered by the EndpointSliceMirroring controller or not, but is not really orphan since still keeps the "managed-by" label 🤷

On one side we can say, the controller is smart enough to detect this problem and reconcile, but now that I think it out loud, it does not look a good idea to open this path of "taking ownership" from other controllers #118376 (comment) with a lot more of thinking, if we not define clear guidelines for this behavior we'll end on hotlooping situations between controllers stealing objects from others back and forth.

In other side we can argument that is a user problem, "you leave these objects orphan on purpose you have to deal with the existing behavior", that is why I asked in #118376 (comment) what is the use case and how did we end in this situation, because it seems that here we only have a consequence without full context on why we ended in this situation, can we have more details on the full picture?

[danwinship]

I have this ambition that we reverse the mirroring: EndpointSlices (plural) that have a related Service would one day mirror to an Endpoints. That feels like the better approach because it makes it clear that Endpoints is the legacy API.

No, we don't want that. Translating an EndpointSlice to an Endpoints is a lossy operation. (eg, you can't preserve dual-stack). If someone creates an EndpointSlice by hand, and they don't create a corresponding Endpoints object, then we just assume that they don't care about legacy Endpoints-only clients (because they absolutely shouldn't care about them at this point).

But the opposite is not the case; if someone creates an Endpoints by hand, and they don't create a corresponding EndpointSlice, we assume that they are a legacy client and don't realize that they should have created an EndpointSlice, so we create the EndpointSlice for them, because otherwise their Endpoints is useless because kube-proxy, etc, won't look at it.

If you are a modern client and you for some reason want to create an Endpoints which will not get mirrored to an EndpointSlice, then you are required to set the endpointslice.kubernetes.io/skip-mirror label on it. This is basically part of the EndpointSlice API.

The question here is what the expectation is for an Endpoints object that was orphaned by the EndpointController.

However, that ownership if we do it could get in the way. It's weird to have things mirror into their owner.

The EndpointSlice isn't owned by the Endpoints, it's owned by the EndpointSlice mirroring controller.

[danwinship]

maybe we should start emitting a warning on all manual Endpoints creations...

[palonsoro]

On one side we can say, the controller is smart enough to detect this problem and reconcile, but now that I think it out loud, it does not look a good idea to open this path of "taking ownership" from other controllers #118376 (comment) with a lot more of thinking, if we not define clear guidelines for this behavior we'll end on hotlooping situations between controllers stealing objects from others back and forth.

@aojea Maybe it would be feasible/simpler if we had the endpointslice controller delete the endpoint slices when the service has endpoints and selector is removed? Under the assumption that the endpointslicemirroring controller would create its own EndpointSlices by mirroring the Endpoints that have been kept around.

[palonsoro]

@danwinship if you mean a warning in kube-apiserver logs, I am afraid that it will be seen by almost nobody other than few very careful cluster-admins (which often are teams different to the users who create them). An event might be better seen by the end users, but...

[sftim]

if someone creates an Endpoints by hand, and they don't create a corresponding EndpointSlice, we assume that they are a legacy client and don't realize that they should have created an EndpointSlice, so we create the EndpointSlice for them, because otherwise their Endpoints is useless because kube-proxy, etc, won't look at it.

Could we emit a Warning at HTTP level? (see linked article for more details)

[palonsoro]

@sftim oh, such a warning would be perfectly visible to end-user

[thockin]

/triage accepted

[pperiyasamy]

@danwinship @robscott any update on the proposed fix ? Is anyone of you working on it ?

[palonsoro]

@pperiyasamy the warning should be something good to have (so that people prefer creating manual endpointslices instead of manual endpoints). But once somebody has decided to create the Endpoints, the mapping between EndpointSlices and Endpoints should be always correct. We should not have wrong mappings just because the sequence of actions was as described in the issue.

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[danwinship]

/triage accepted

Still a problem. Some of the discussion here (eg, warnings if people use Endpoints) ties into the idea that we should "deprecate Endpoints harder" as discussed in SIG Network on May 9.