Use endpoint slices for fabric8 catalog watcher by wind57 · Pull Request #1149 · spring-cloud/spring-cloud-kubernetes

wind57 · 2022-11-25T16:58:46Z

No description provided.

wind57 · 2022-11-29T21:49:54Z

 rules:
-  - apiGroups: ["", "extensions", "apps"]
-    resources: ["configmaps", "pods", "services", "endpoints", "secrets"]
+  - apiGroups: ["", "extensions", "apps", "discovery.k8s.io"]


@ryanjbaxter before I even start documentation and explanation on my thought process in this PR, I need your supervisor advice.

Kubernetes recommends using EndpointSlices instead of Endpoints (think of it a @deprecated). Now, we can use EndpointSlices as this PR and integration tests prove. The problem is that by switching to EndpointSlices, users would need to change the roles, as this file shows (though you have already done that by now, since you need endpoints for Endpoints anyway)

So, one hand we have the official k8s recommendation, on the other hand if we do follow it, we kind of introduce a "breaking" change, as users need to adjust to it.

Of course, we could let users opt-in into EndpointSlices and we could explain in the docs the steps needed for that, but then we would kind of go against what k8s says to do.

As a side note, there is nothing wrong in using Endpoints, the code still works... wdyt is the correct path forward here? Thank you.

As far as I understand the Endpoints API is going to continue to be supported and is not planned to be removed at the moment, correct?

Also it seems like Endpoint Slices is only officially supporting in k8s 1.2.1 right? If you are using a version prior to that it was still in beta, correct?

I think if we move forward with this, I would make it an opt in feature. Document how to opt in and the additional configuration that is needed to use it.

As far as I understand the Endpoints API is going to continue to be supported and is not planned to be removed at the moment, correct?

Correct.

Also it seems like Endpoint Slices is only officially supporting in k8s 1.2.1 right? If you are using a version prior to that it was still in beta, correct?

Yeah, they had alpha support in 1.16, then beta, then stable.

I think if we move forward with this, I would make it an opt in feature. Document how to opt in and the additional configuration that is needed to use it.

Excellent! I'll update the PR accordingly.

Thank you for your suggestions. I've updated the PR as suggested. Have a good trip next week!

wind57 · 2022-11-30T09:51:37Z

+	private final Fabric8CatalogWatchContext context;

-	private final KubernetesNamespaceProvider namespaceProvider;
+	private Function<Fabric8CatalogWatchContext, List<EndpointNameAndNamespace>> stateGenerator;


we separate the logic now into two logical pieces: into Endpoints and EndpointSlices. Each of those (Fabric8EndpointsCatalogWatch and Fabric8EndpointSliceV1CatalogWatch) is actually a Function. This function takes as input a Fabric8CatalogWatchContext (which has members as : KubernetesClient, KubernetesDiscoveryProperties and KubernetesNamespaceProvider ) and returns a List<EndpointNameAndNamespace>.

wind57 · 2022-11-30T09:53:29Z

+	@PostConstruct
+	void postConstruct() {
+
+		if (context.properties().useEndpointSlices()) {


we need to make a distinction of when to use Endpoints vs EndpointSlices, because the latter might be not support on the k8s cluster. As such, we try to find out if it is supported or not and based on that return the proper stateGenerator instance

I think I would rather just let things fail if they enable it but its not enabled on the cluster. We don't generally try to do these types of things when someone has to explicitly enable a feature

updated the PR. thank you very much for the feedback, I did not know which direction to take : exception or defaulting, so this was much appreciated

No problem!

wind57 · 2022-11-30T09:58:05Z

+ *
+ * @author wind57
+ */
+record Fabric8CatalogWatchContext(KubernetesClient kubernetesClient, KubernetesDiscoveryProperties properties,


a record that acts as a Tuple for all the parameters needed in order to compute the List<EndpointNameAndNamespace>. Please notice both Endpoints and EndpointSlices implementations will delegate to its state method here.

This method takes as input a Stream<ObjectReference> because both Endpoints and EndpointSlices can be narrowed to this, meaning :

Stream<ObjectReference> references = endpointSlices.stream().map(EndpointSlice::getEndpoints) .flatMap(List::stream).map(Endpoint::getTargetRef);

and

Stream<ObjectReference> references = endpoints.stream().map(Endpoints::getSubsets).filter(Objects::nonNull) .flatMap(List::stream).map(EndpointSubset::getAddresses).filter(Objects::nonNull).flatMap(List::stream) .map(EndpointAddress::getTargetRef);

I hope this makes sense.

wind57 · 2022-11-30T10:00:49Z

+ *
+ * @author wind57
+ */
+final class Fabric8EndpointSliceV1CatalogWatch


implementation that uses EndpointSlices

wind57 · 2022-11-30T10:01:23Z

+ *
+ * @author wind57
+ */
+final class Fabric8EndpointsCatalogWatch


implementation that uses Endpoints - this is what we had until now. Nothing has changed logically here

wind57 · 2022-11-30T10:03:38Z

+			LOG.debug(() -> "discovering endpoints in all namespaces");
+
+			try (KubernetesClient client = context.kubernetesClient()) {
+				endpointSlices = client.discovery().v1().endpointSlices().inAnyNamespace()


I am using client.discovery().v1() here. There is also an option to use v1beta1 for EndpointSlices, but since its beta, I left it out... If you disagree, please let me know.

wind57 · 2022-11-30T10:04:14Z

+ *
+ * @author wind57
+ */
+@EnableKubernetesMockClient


fabric tests for EndpointSlices

wind57 · 2022-11-30T10:09:17Z

+          env:
+            - name: LOGGING_LEVEL_ORG_SPRINGFRAMEWORK_CLOUD_KUBERNETES_FABRIC8_DISCOVERY
+              value: DEBUG
+            - name: SPRING_CLOUD_KUBERNETES_DISCOVERY_USE_ENDPOINT_SLICES


property that enables using EndpointSlices

wind57 · 2022-12-02T14:39:50Z

@ryanjbaxter this is ready to be looked at too. thank you

wind57 added 12 commits November 16, 2022 18:36

move method

51f460d

commit so that I could ask a question

729a896

dirty

9700cc2

added tests

309d552

Merge branch 'main' into fix-1139

7841560

trigger build

904bbbe

wip

5e10e4b

merged main

0a1e3fc

add integration test

7fc10fd

add documentation

e23a309

upgrade istiocli

f7dfa27

started work on using slices

0bc6b8a

spring-cloud-issues added the waiting-for-triage label Nov 25, 2022

wind57 added 8 commits November 29, 2022 10:04

WIP

42c7a84

fix

32d9e36

remove a tab

20feee5

merged main

c337e3d

wip

b26e731

wip

0d290b5

merge main

7e05b3e

fix tests

56d1caa

wind57 commented Nov 29, 2022

View reviewed changes

wind57 added 3 commits November 30, 2022 11:00

fix tests

c766aae

fix wiremock dependency

1136ccb

merge fix for wiremock

a994fe6

wind57 commented Nov 30, 2022

View reviewed changes

wind57 added 5 commits November 30, 2022 12:23

fix test

9167486

tighther tests

99f5235

change order + documentation

7ed47e4

a few fixes

adc6bf9

fix tests

4141ee3

wind57 marked this pull request as ready for review November 30, 2022 23:11

wind57 added 3 commits December 1, 2022 13:58

merge main

9481a94

merge main

357a1e9

Merge branch 'main' into use-endpoint-slices

265a12c

wind57 requested a review from ryanjbaxter December 2, 2022 14:39

wind57 added 5 commits December 6, 2022 21:11

fail is endpointslices are not supported

701fa71

fix test

3c580b6

formatting

d23dfe9

Merge branch 'main' into use-endpoint-slices

36faf15

fix tests

0f81e06

ryanjbaxter added enhancement and removed waiting-for-triage labels Dec 6, 2022

ryanjbaxter added this to the 3.0.0 milestone Dec 6, 2022

ryanjbaxter merged commit 3dcab2b into spring-cloud:main Dec 6, 2022

ryanjbaxter linked an issue Dec 14, 2022 that may be closed by this pull request

Use endpoint slices ( if available ) in KubernetesCatalogWatch #1146

Closed

Conversation

wind57 commented Nov 25, 2022

Uh oh!

wind57 Nov 29, 2022 • 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wind57 Nov 30, 2022 • 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!

wind57 commented Dec 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wind57 Nov 29, 2022 •

edited

Loading

wind57 Nov 30, 2022 •

edited

Loading