spring-cloud
Implement A CatalogWatch for Kubernetes Java Client
I am trying to understand what is the purpose of KubernetesCatalogWatch in the fabric8 implementation of discovery. It seems to me that KubernetesDiscoveryClient is an implementation of DiscoveryClient and KubernetesCatalogWatch was an initial implementation before that one existed.
As such, I am very inclined to delete it from the repo... unless I am missing something. what do you think? thank you
seems like spring-boot-admin relies on this for getting proper heartbeat events, so no, we can't delete it.
what I find interesting is that such an implementation does not exist in k8s-native. May be someone can explain...
I dont know the exact history here, but this was modeled about ConsulCatalogWatch and it appears to publish a HeartBeat event when the services change. What that is used for I am not sure. @spencergibb can you describe how ConsulCatalogWatch is used?
It is not a DiscoveryClient implementation as far as I can tell.
it appears to publish a HeartBeat event when the services change.
That's exactly right. Then something can use that event to do something like spring cloud gateway can refresh routes and if there is a new one it can add it.
May be someone can explain...
I think the reason why it doesn't exist in the k8s java client is simply we never implemented it
I started working on this, and started by refactoring a bit the existing fabric8 KubernetesCatalogWatch implementation. While doing that, I have observed a minor thing. We return inside the heartbeat the list of endpoints as a List<String>. We also support the search against "any namespace", which means that in theory we can return a List that would contain duplicates (same endpoint name across different namespace).
Do you think we should refactor this one to publish a Tuple like endpoint name and namespace where it came from? This would mean a breaking change for clients, but, imho, we would be fixing a bug here. @ryanjbaxter wdyt? thank you.
Should we be returning endpoints across all namespaces or should that be enabled via a property, kind of like service discovery?
We already have that implemented in KubernetesCatalogWatch, we do check for "all-namespaces" property:
List<Endpoints> endpoints;
if (properties.allNamespaces()) {
endpoints = kubernetesClient.endpoints().inAnyNamespace().withLabels(properties.serviceLabels()).list()
.getItems();
}
else {
endpoints = kubernetesClient.endpoints().withLabels(properties.serviceLabels()).list().getItems();
}
In theory, we can leave the code as-is, as no one complained (yet), but overall this is still a tiny issue.
So its only when all-namespaces is true that we run into this issue then?
If so I think we should make the breaking change. I would just make it in #1138
So its only when all-namespaces is true that we run into this issue then?
Currently. There is a PR (that I am yet to take a closer look at) here that adds the possibility to specify specific namespaces. And once we have that (and implement the same in CatalogWatcher), we might run into the same problem.