Add namespace provider to fabric8 loadbalancer #1597
Conversation
Configure Renovate
| @@ -46,6 +46,7 @@ public static KubernetesClientServicesFunction servicesFunction(KubernetesDiscov | |||
|
|
|||
| } | |||
|
|
|||
| @Deprecated(forRemoval = true) | |||
There was a problem hiding this comment.
un-related to the current PR, just saw that this one is un-used, so deprecated it. Later, when its allowed, I will be searching for these for clean-up, so it will useful to easy find them
| Service service = StringUtils.hasText(kubernetesClient.getNamespace()) ? kubernetesClient.services() | ||
| .inNamespace(kubernetesClient.getNamespace()).withName(getServiceId()).get() | ||
| : kubernetesClient.services().withName(getServiceId()).get(); | ||
| String namespace = Fabric8Utils.getApplicationNamespace(kubernetesClient, null, "loadbalancer-service", |
There was a problem hiding this comment.
this is the important bit right here : String namespace = Fabric8Utils.getApplicationNamespace .... This adds support for namespace resolution that we have in various other places too. Besides that, everything else are just log additions. Of course, when I am done with both clients, everything will be documented properly.
There was a problem hiding this comment.
Doesn't this change the functionality a bit? Right now if we don't find the service in the namespace we look for it across all namespaces. With this change we will just return an empty Flux if we don't find it.
There was a problem hiding this comment.
You mean this piece of code, right?
Service service = StringUtils.hasText(kubernetesClient.getNamespace()) ? kubernetesClient.services()
.inNamespace(kubernetesClient.getNamespace()).withName(getServiceId()).get()
: kubernetesClient.services().withName(getServiceId()).get();
If so, this code is miss-leading, at best. It's actually exactly the same. It's counter intuitive, but its the same. The change was done a lot time ago in fabric8 client: fabric8io/kubernetes-client#123
So the fact that you can omit the namespace, creates this confusion.
There was a problem hiding this comment.
Are you saying it is exactly the same because before the KubernetesClient that was passed into the class already had the namespace set so when calling .inNamespace it didn't make a difference in the services returned?
There was a problem hiding this comment.
not really, Im saying that:
kubernetesClient.services().inNamespace(kubernetesClient.getNamespace()).get()
is a longer version of :
kubernetesClient.services().get()
according this this comment from the issue I linked and my testing
There was a problem hiding this comment.
I think I understand now, when you specify the namespace and it does not find the service it will also return the service if it exists in the default namespace?
There was a problem hiding this comment.
correct, but "default" namespace is not the literal default. meaning if this client is running in namespace a, this will be the "default namespace" : a, not default. Or, in other words, kubernetesClient.getNamespace() will return a.
So that code we currently have, kind of makes no sense.
| @@ -128,7 +128,8 @@ | |||
| <!-- configuration watcher is based on k8s-client, there's no fabric8 counterpart --> | |||
| <module>spring-cloud-kubernetes-k8s-client-configuration-watcher</module> | |||
|
|
|||
| <!-- at the moment we have no integration test for the fabric8 client, only k8s native client --> | |||
| <!-- loadbalancer --> | |||
| <module>spring-cloud-kubernetes-fabric8-client-loadbalancer</module> | |||
There was a problem hiding this comment.
I'm adding an integration test. I know you are not very fond of adding, but it's really the case that we need them here. We never had them for fabric8 load balancer in the first place
There was a problem hiding this comment.
Have you investigated setting up Wiremock in a normal junit test to mock the k8s APIs to efficiently test this?
There was a problem hiding this comment.
I did not, though your advise makes sense. The thing, I still think we need some integration tests for this functionality, as we have none. In the future, I will see how I can write plain unit tests with wiremock indeed. I'm fully aware of the pain point here that you told me about jenkins, still, some integration tests are needed, imho.
Any future tests (for example against "selective namespaces" that I plan to add support for) will have plain junit tests
There was a problem hiding this comment.
I would like us to take the opposite approach moving forward. I would like to use wiremock and unit tests to test functionality and then if we find a case where its just not possible to test something using that approach and we need an integration test then we can add one.
Out build is taking way to long to run and its becoming problematic for us, so I only want to add more integration tests where we are starting a test container if it is absolutely necessary.
There was a problem hiding this comment.
I see, I'll refactor then, thx for the input
There was a problem hiding this comment.
Sorry to be a pain in the butt
There was a problem hiding this comment.
oh no, this makes perfect sense and it's kind of an interesting challenge to try to mimic as much as possible the integration test. I actually can't wait to start working into it :) so no worries at all!
There was a problem hiding this comment.
I refactored as suggested. thank you
| spec: | ||
| containers: | ||
| - name: httpd | ||
| image: httpd:2.4 |
There was a problem hiding this comment.
I'm also adding a new image for testing purposes to our stack: httpd. This is needed so that we have different services in different namespaces, so that our integration tests are very realistic.
|
@ryanjbaxter this one only adds namespace resolution support for fabric8 load balancer, but the integration tests make it look like its far more, which is not |
No description provided.