Add conformance tests for EndpointSlices #79

tpantelis · 2024-10-02T22:42:32Z

Look for an MCS EndpointSlice using the criteria whereby the endpointslice.kubernetes.io/managed-by label is not equal to endpointslice-controller.k8s.io or any MCS label is present. If not found then fail the test as non-conformant. Otherwise verify the required MCS labels are correctly set.

The tests are labeled with "EndpointSlice" so, if EndpointSlices aren't supported by the implementation, the user can use the "EndpointSlice" label to filter out the tests.

Fixes #70

k8s-ci-robot · 2024-10-02T22:42:41Z

Hi @tpantelis. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

tpantelis · 2024-10-02T22:43:04Z

/cc @skitt

tpantelis · 2024-10-02T22:46:30Z

/cc @MrFreezeex When you get a chance, could you test this with Cilium (assuming Cilium uses EndpointSlices)? I've tested it with Submariner but Submariner doesn't add the ServiceImport ownership (currently, I'll be adding that at some point) so I haven't tested the ServiceImport ownership check passing.

MrFreezeex · 2024-10-03T08:20:38Z

/cc @MrFreezeex When you get a chance, could you test this with Cilium (assuming Cilium uses EndpointSlices)? I've tested it with Submariner but Submariner doesn't add the ServiceImport ownership (currently, I'll be adding that at some point) so I haven't tested the ServiceImport ownership check passing.

Actually we don't do that too, our EndpointSlice have an OwnerReference to a Service which have an OwnerReference to the actual ServiceImport. We are doing it this way because while the EndpointSlice synchronization was done with MCS-API in mind it is actually tied to the annotation system of global Services (aka the way we are doing multicluster networking without MCS-API) so that even non MCS-API users can use this feature (as we are the CNI + replacing kube-proxy before this EndpointSlice synchronization feature we were keeping the remote endpoints internally and not exposing it anywhere on the Kubernetes API).

I mentioned that two sig meeting ago as while reading the associated issue to this I thought the OwnerReference was explicitly stated as something mandatory in the KEP and people that were on the call were all agreeing that the KEP shoudn't mandate to have an OwnerReference to a ServiceImport as long as there is any cleanup mechanism in place and that the conformance tests may check that EndpointSlices are cleaned up correctly but not necessarily in what way it does this. But after that call when I wanted to PR the KEP, I realized that it wasn't technically stated explicitly in the KEP actually x).

tpantelis · 2024-10-03T12:11:27Z

I mentioned that two sig meeting ago as while reading the associated issue to this I thought the OwnerReference was explicitly stated as something mandatory in the KEP and people that were on the call were all agreeing that the KEP shoudn't mandate to have an OwnerReference to a ServiceImport as long as there is any cleanup mechanism in place and that the conformance tests may check that EndpointSlices are cleaned up correctly but not necessarily in what way it does this.

OK - I can remove the OwnerReference check. That would help Submariner compliance too since we don't use it for cleanup either 😄 I'll expand the test to un-export and ensure the EPS is cleaned up.

But after that call when I wanted to PR the KEP, I realized that it wasn't technically stated explicitly in the KEP actually x).

I think it is in this section at the end of the second paragraph: "EndpointSlices will have an owner reference to their associated ServiceImport.". I can push a PR to remove that line.

tpantelis · 2024-10-03T13:00:41Z

I can push a PR to remove that line.

kubernetes/enhancements#4900

tpantelis · 2024-10-08T13:11:59Z

/ok-to-test

tpantelis · 2024-10-08T15:53:33Z

/test pull-mcs-api-verify

tpantelis · 2024-10-21T12:40:30Z

/cc @skitt @MrFreezeex

MrFreezeex · 2024-10-22T10:19:05Z

conformance/endpoint_slice.go

+				eps := &list.Items[i]
+
+				if slices.IndexFunc(eps.OwnerReferences, func(r metav1.OwnerReference) bool {
+					return r.Kind == "Service"


As our owner reference for those endpointslices in Cilium is a Service too this wouldn't work for us. IIUC this check is to ensure that we don't end picking up regular/non multi cluster endpointslice so could this be replaced by checking the label discoveryv1.LabelManagedBy (like it's already checked above actually) perhaps 🙏?

That leads to a Catch-22 — one of the purposes of the test is to verify that EndpointSlices created by the MCS controller have the appropriate “managed by” label 😉. Given the leniency of the spec though I suspect we won’t be able to come up with a guaranteed test. Perhaps checking that the “managed by” label isn’t endpointslice-controller.k8s.io is good enough: if an MCS controller uses that label value, it’s asking for trouble anyway, so it’s fair game to ignore such EndpointSlices IMO; and we can specify that the conformance suite shouldn’t be run with non-default EndpointSlice controllers. Another option would be to allow the user to specify the “managed-by” value to check for.

I was going by the original statement in the spec, "EndpointSlices will have an owner reference to their associated ServiceImport", but that's been since removed. I didn't check for the endpointslice.kubernetes.io/managed-by label here in order to verify this statement in the spec, "These EndpointSlice objects will be marked as managed by the clusterset service controller, so that the endpoint slice controller doesn’t delete them". But I guess checking that the endpointslice.kubernetes.io/managed-by label is not equal to endpointslice-controller.k8s.io is the only reliable way to distinguish a K8s EndpointSlice from an MCS one.

@skitt Our comments crossed. It sounds like we concluded the same thing re: checking endpointslice.kubernetes.io/managed-by here.

Another option is to fail the test as non-conformant if we don't find an EndpointSlice not managed by endpointslice-controller.k8s.io. If the implementation doesn't support MCS EndpointSlices then the user can filter out the test via the EndpointSlice Ginkgo label. But the test is also labeled as Optional so, by default, it would effectively be treated as required which could be viewed as inconsistent although it's just a categorization for the report.

@skitt @MrFreezeex WDYT?

That sounds about right that was also very similar to what I had in mind too, thanks 🙏!

OK so the MCS labels get propagated to the local EPS by the K8s EPS controller via the service labels. In this case, Cilium is still compliant b/c the EPS isn't actually created/managed by an MCS controller although your kubernetes/enhancements#4930 PR further clarifies it in the spec.

So there's really no way to tell if the EPS was created by an MCS controller or the K8s controller, hence cannot verify the endpointslice.kubernetes.io/managed-by label. We could add a flag to try to guide the test as to who manages the EPS but Cilium muddies that further b/c some are MCS-managed and some not. So I think the only thing we can search for is the presence of at least one MCS label.

BTW, with #81 we could verify the value of the multicluster.kubernetes.io/source-cluster label although I'm not entirely sure if the cluster name extracted from the kubeconfig is guaranteed to match what the MCS implementation views as the cluster name (@skitt ).

OK so the MCS labels get propagated to the local EPS by the K8s EPS controller via the service labels. In this case, Cilium is still compliant b/c the EPS isn't actually created/managed by an MCS controller although your kubernetes/enhancements#4930 PR further clarifies it in the spec.

So there's really no way to tell if the EPS was created by an MCS controller or the K8s controller, hence cannot verify the endpointslice.kubernetes.io/managed-by label. We could add a flag to try to guide the test as to who manages the EPS but Cilium muddies that further b/c some are MCS-managed and some not. So I think the only thing we can search for is the presence of at least one MCS label.

Yeah I am thinking that the managed-by check could be an opt-out check somehow. IMO this should be fine as is, Cilium is probably the only one doing that and to integrate this test to Cilium correctly I would also have to change a bit the test because endpointslice syncing is opt-in in Cilium so I would probably need to add a way to hook into the Service creation or some kind of argument . This means that I could also add something to optionally ignore the managed-by check at the ~ same time.

BTW, with #81 we could verify the value of the multicluster.kubernetes.io/source-cluster label although I'm not entirely sure if the cluster name extracted from the kubeconfig is guaranteed to match what the MCS implementation views as the cluster name (@skitt ).

What's in kubeconfig is kind of free form so I would guess that you could provide a kubeconfig that is different on many implementations x). Although the conformance test could probably just assume that the user provides something that actually match if it's clearly stated somewhere 🤷‍♂️.

IMO this should be fine as is

I assume you mean the way it is now in my latest commit even tho it would fail on Cilium. Of course, you could always filter out this test since it's optional.

endpointslice syncing is opt-in in Cilium

We have a similar situation with Submariner re: ServiceImport VIP. By default we don't set this so the test fails. However we recently added an option to set the VIP so when running the conformance tests we set the option. However this causes the DNS test to fail as we rely on some external component to handle that. But the DNS test is optional so I can filter it out 😄 I assume you could do similar, ie filter out the EPS test according to your opt-in setting.

add something to optionally ignore the managed-by check at the ~ same time.

I could do that with this PR.

BTW, with #81 we could verify the value of the multicluster.kubernetes.io/source-cluster label although I'm not entirely sure if the cluster name extracted from the kubeconfig is guaranteed to match what the MCS implementation views as the cluster name (@skitt ).

What's in kubeconfig is kind of free form so I would guess that you could provide a kubeconfig that is different on many implementations x). Although the conformance test could probably just assume that the user provides something that actually match if it's clearly stated somewhere 🤷‍♂️.

Yup, the cluster name in the kubeconfig is only relevant on the client, it doesn’t have to be related to the cluster id used by the MCS controller.

I added a skip-verify-eps-managed-by CLI flag defaulting to false. if true, it still checks that the endpointslice.kubernetes.io/managed-by label is present but does not check that value.

conformance/endpoint_slice.go

Look for an MCS EndpointSlice using the criteria whereby the "endpointslice.kubernetes.io/managed-by" label is not equal to endpointslice-controller.k8s.io" or any MCS label is present. If not found then fail the test as non-conformant. Otherwise verify the required MCS labels are correctly set. The tests are labeled with "EndpointSlice" so, if EndpointSlices aren't supported by the implmentation, the user can use the "EndpointSlice" label to filter out the tests. Fixes kubernetes-sigs#70 Signed-off-by: Tom Pantelis <[email protected]>

MrFreezeex

Thanks a lot for bearing with me that looks great 🙇
/lgtm

tpantelis · 2024-10-28T23:47:52Z

Thanks a lot for bearing with me that looks great 🙇 /lgtm

@skitt ? 🙏

skitt · 2024-10-31T12:40:28Z

/approve

k8s-ci-robot · 2024-10-31T12:40:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrFreezeex, skitt, tpantelis

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:

~~OWNERS~~ [skitt]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 2, 2024

k8s-ci-robot requested review from JeremyOT and lauralorenz October 2, 2024 22:42

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 2, 2024

k8s-ci-robot requested a review from skitt October 2, 2024 22:43

tpantelis force-pushed the eps_conformance branch from 78c80df to f1f4bcd Compare October 3, 2024 04:11

tpantelis force-pushed the eps_conformance branch from f1f4bcd to defaed4 Compare October 3, 2024 12:36

tpantelis mentioned this pull request Oct 3, 2024

REQUEST: New membership for tpantelis kubernetes/org#5205

Closed

11 tasks

tpantelis force-pushed the eps_conformance branch from defaed4 to 91f8187 Compare October 3, 2024 20:36

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 8, 2024

tpantelis force-pushed the eps_conformance branch from 91f8187 to 1179bc9 Compare October 15, 2024 12:37

k8s-ci-robot requested a review from MrFreezeex October 21, 2024 12:40

MrFreezeex reviewed Oct 22, 2024

View reviewed changes

tpantelis force-pushed the eps_conformance branch from 1179bc9 to d8a4297 Compare October 24, 2024 12:58

MrFreezeex reviewed Oct 24, 2024

View reviewed changes

conformance/endpoint_slice.go Show resolved Hide resolved

tpantelis force-pushed the eps_conformance branch 2 times, most recently from b05a597 to dd51285 Compare October 25, 2024 12:41

tpantelis force-pushed the eps_conformance branch from dd51285 to 25ba403 Compare October 25, 2024 13:04

MrFreezeex approved these changes Oct 25, 2024

View reviewed changes

k8s-ci-robot assigned MrFreezeex Oct 25, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2024

k8s-ci-robot merged commit 3e8e407 into kubernetes-sigs:master Oct 31, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conformance tests for EndpointSlices #79

Add conformance tests for EndpointSlices #79

tpantelis commented Oct 2, 2024 •

edited

Loading

k8s-ci-robot commented Oct 2, 2024

tpantelis commented Oct 2, 2024

tpantelis commented Oct 2, 2024

MrFreezeex commented Oct 3, 2024 •

edited

Loading

tpantelis commented Oct 3, 2024

tpantelis commented Oct 3, 2024

tpantelis commented Oct 8, 2024

tpantelis commented Oct 8, 2024

tpantelis commented Oct 21, 2024

MrFreezeex Oct 22, 2024 •

edited

Loading

skitt Oct 22, 2024

tpantelis Oct 22, 2024

tpantelis Oct 22, 2024 •

edited

Loading

MrFreezeex Oct 22, 2024

tpantelis Oct 24, 2024 •

edited

Loading

MrFreezeex Oct 24, 2024 •

edited

Loading

tpantelis Oct 24, 2024

skitt Oct 25, 2024

tpantelis Oct 25, 2024 •

edited

Loading

MrFreezeex left a comment

tpantelis commented Oct 28, 2024

skitt commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

Add conformance tests for EndpointSlices #79

Add conformance tests for EndpointSlices #79

Conversation

tpantelis commented Oct 2, 2024 • edited Loading

k8s-ci-robot commented Oct 2, 2024

tpantelis commented Oct 2, 2024

tpantelis commented Oct 2, 2024

MrFreezeex commented Oct 3, 2024 • edited Loading

tpantelis commented Oct 3, 2024

tpantelis commented Oct 3, 2024

tpantelis commented Oct 8, 2024

tpantelis commented Oct 8, 2024

tpantelis commented Oct 21, 2024

MrFreezeex Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

skitt Oct 22, 2024

Choose a reason for hiding this comment

tpantelis Oct 22, 2024

Choose a reason for hiding this comment

tpantelis Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

MrFreezeex Oct 22, 2024

Choose a reason for hiding this comment

tpantelis Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

MrFreezeex Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

tpantelis Oct 24, 2024

Choose a reason for hiding this comment

skitt Oct 25, 2024

Choose a reason for hiding this comment

tpantelis Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

MrFreezeex left a comment

Choose a reason for hiding this comment

tpantelis commented Oct 28, 2024

skitt commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

tpantelis commented Oct 2, 2024 •

edited

Loading

MrFreezeex commented Oct 3, 2024 •

edited

Loading

MrFreezeex Oct 22, 2024 •

edited

Loading

tpantelis Oct 22, 2024 •

edited

Loading

tpantelis Oct 24, 2024 •

edited

Loading

MrFreezeex Oct 24, 2024 •

edited

Loading

tpantelis Oct 25, 2024 •

edited

Loading