Complete alignment between fabric8 and k8s discovery clients #1500
Conversation
…fabric8-k8s-discovery-clients-part-2
wind57
changed the title
| @@ -116,20 +116,25 @@ public void createAndWait(String namespace, String name, V1Deployment deployment | |||
| @Nullable V1Ingress ingress, boolean changeVersion) { | |||
| try { | |||
|
|
|||
| String imageFromDeployment = deployment.getSpec().getTemplate().getSpec().getContainers().get(0).getImage(); | |||
There was a problem hiding this comment.
changes here are needed because of integration tests that have to do with "ExternalName" service type. We do not need deployments or ingress for such tests, but the utility methods we put in place where expecting those. So the changes here simply take care of a potential null deployment.
| @@ -69,7 +70,7 @@ void testSimple() { | |||
| Assertions.assertEquals(serviceInstance.getServiceId(), "spring-cloud-kubernetes-k8s-client-discovery"); | |||
| Assertions.assertNotNull(serviceInstance.getHost()); | |||
| Assertions.assertEquals(serviceInstance.getMetadata(), | |||
| Map.of("http", "8080", "k8s_namespace", "default", "type", "ClusterIP", "label-app", | |||
| Map.of("port.http", "8080", "k8s_namespace", "default", "type", "ClusterIP", "label-app", | |||
There was a problem hiding this comment.
the default prefix for port is port.. Fabric8 already had this in place properly, in k8s-client this was a defect. It was never opened, though. Now that we have aligned implementation, such tests needed to be aligned also.
| @@ -82,7 +86,8 @@ void testBlockingConfiguration(K3sContainer container) { | |||
| Assertions.assertThat(BASIC_JSON_TESTER.from(healthResult)) | |||
| .extractingJsonPathArrayValue( | |||
| "$.components.discoveryComposite.components.discoveryClient.details.services") | |||
| .containsExactlyInAnyOrder("spring-cloud-kubernetes-k8s-client-discovery", "kubernetes"); | |||
There was a problem hiding this comment.
add one more service as assertion: external-name-service
| /** | ||
| * @author wind57 | ||
| */ | ||
| final class K8sInstanceIdHostPodNameSupplier implements Supplier<InstanceIdHostPodName> { |
There was a problem hiding this comment.
a fabric8 equivalent of this was introduced a while ago, time to do the same for k8s-client
|
|
||
| import io.kubernetes.client.informer.SharedInformer; | ||
| import io.kubernetes.client.informer.SharedInformerFactory; | ||
| import io.kubernetes.client.informer.cache.Lister; | ||
| import io.kubernetes.client.openapi.models.CoreV1EndpointPort; | ||
| import io.kubernetes.client.openapi.apis.CoreV1Api; |
There was a problem hiding this comment.
the most important changes are here. You can think about of what was done here in this manner: "take the fabric8 discovery client implementation, almost do a copy paste in here and replace all fabric8 specific code with k8s-client code".
This is exactly what I did, keeping in mind that k8s discovery works based on Listers, unlike fabric8.
If you take the code flow in fabric8, you can see that it is replicated in here too, almost verbatim, with k8s-client specific classes.
There was a problem hiding this comment.
This was "easy" to achieve since we did a lot of work in this direction this year, lost of things have been refactored and moved to commons, so that both clients can delegate to that code.
| /** | ||
| * @author wind57 | ||
| */ | ||
| class K8sInstanceIdHostPodNameSupplierTests { |
There was a problem hiding this comment.
same tests exist in fabric8
|
we were going to this PR all year, tbh. This finally aligns fabric8 and k8s-client discovery implementations. They have the same tests, same integration tests and most important same code flow. Event if one works with Listers and one not. |
| @@ -76,6 +80,13 @@ public class KubernetesInformerDiscoveryClient implements DiscoveryClient { | |||
|
|
|||
| private final Predicate<V1Service> filter; | |||
|
|
|||
| private final ServicePortSecureResolver servicePortSecureResolver; | |||
|
|
|||
| // visible only for testing and | |||
There was a problem hiding this comment.
I could inject this via the constructor, but then : a) lots of deprecations b) deprecated constructor would not support all the features. It would have got messy. In a future release, this will be refactored and changed.
|
@ryanjbaxter ready to be looked at. thank you. |
No description provided.