-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add namespace provider to fabric8 loadbalancer #1597
Changes from all commits
032014e
c3d0ad2
10889fd
8f0375a
96ebf42
db8403e
90a5345
b041c00
9580444
a34ac47
6613f78
1b3eae9
4275382
618f25a
100a9cd
4b9056b
315a85b
ed3c264
446d630
6821a28
9e95a8a
02840b1
7e31d65
926f4d7
344e1d4
e91ac12
ba6e088
49025e8
a647bb3
16b09ab
c8d8a72
591f340
c331bd0
f48b22b
900843d
76ecdd5
b69d4a6
df2ddcc
3ff1cdd
bd89cc8
39ef510
d389953
b676bac
b1473c9
83ff113
33dfbb7
316af58
425d30b
763280a
3afbaef
cb97ba1
b33edc8
7a1fdf1
a8beed9
2e445d9
5b51db1
b0ce76c
918bb7f
c629951
ccfc0bf
0f3fa24
b60a283
901cb66
251002b
fedeb17
004f95b
10168be
dc811cf
3dfe77d
619c8d0
12ca211
a91c1e1
be2e809
1c84e33
1319fe7
9424c2e
cddd550
f8d34f2
31d2fb8
fa9fd09
22f3f51
bb44f4b
9714f2c
0f9cb3d
6e60174
78c5575
5eb5c51
48f7cb0
8a18085
e4cab39
ba4cab0
bbc6e30
0e9f1ad
8a5243c
cbf83c4
3dd0260
1408d44
2925ee1
af3a12b
f5a27dc
494454a
add0f67
b6a4b45
00a559d
882f9c1
6c9071e
09c10bb
3615396
116c199
f1447cd
14b165b
3ce8fac
41eaa8d
4f89298
ee1031d
f6707d5
9930c7d
0043006
4e292ec
3129276
1c90fc2
fa0888a
ab680d1
e896ec4
4023ca3
7ed7f5d
3be81b7
dd710ec
cd93c3d
faa1d85
4fdd06a
e8ba51d
14a93c0
bf61054
5151acd
97028d4
f059697
0ec1230
013cc6a
44a35a5
02fffce
beb0c02
b48edd4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright 2013-2020 the original author or authors. | ||
* Copyright 2013-2024 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
|
@@ -21,14 +21,17 @@ | |
|
||
import io.fabric8.kubernetes.api.model.Service; | ||
import io.fabric8.kubernetes.client.KubernetesClient; | ||
import org.apache.commons.logging.LogFactory; | ||
import reactor.core.publisher.Flux; | ||
|
||
import org.springframework.cloud.client.ServiceInstance; | ||
import org.springframework.cloud.kubernetes.commons.KubernetesNamespaceProvider; | ||
import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties; | ||
import org.springframework.cloud.kubernetes.commons.loadbalancer.KubernetesServicesListSupplier; | ||
import org.springframework.cloud.kubernetes.fabric8.Fabric8Utils; | ||
import org.springframework.cloud.loadbalancer.core.ServiceInstanceListSupplier; | ||
import org.springframework.core.env.Environment; | ||
import org.springframework.util.StringUtils; | ||
import org.springframework.core.log.LogAccessor; | ||
|
||
/** | ||
* Implementation of {@link ServiceInstanceListSupplier} for load balancer in SERVICE | ||
|
@@ -38,30 +41,45 @@ | |
*/ | ||
public class Fabric8ServicesListSupplier extends KubernetesServicesListSupplier<Service> { | ||
|
||
private static final LogAccessor LOG = new LogAccessor(LogFactory.getLog(Fabric8ServicesListSupplier.class)); | ||
|
||
private final KubernetesClient kubernetesClient; | ||
|
||
private final KubernetesNamespaceProvider namespaceProvider; | ||
|
||
Fabric8ServicesListSupplier(Environment environment, KubernetesClient kubernetesClient, | ||
Fabric8ServiceInstanceMapper mapper, KubernetesDiscoveryProperties discoveryProperties) { | ||
super(environment, mapper, discoveryProperties); | ||
this.kubernetesClient = kubernetesClient; | ||
namespaceProvider = new KubernetesNamespaceProvider(environment); | ||
} | ||
|
||
@Override | ||
public Flux<List<ServiceInstance>> get() { | ||
List<ServiceInstance> result = new ArrayList<>(); | ||
String serviceName = getServiceId(); | ||
LOG.debug(() -> "serviceID : " + serviceName); | ||
|
||
if (discoveryProperties.allNamespaces()) { | ||
LOG.debug(() -> "discovering services in all namespaces"); | ||
List<Service> services = kubernetesClient.services().inAnyNamespace() | ||
.withField("metadata.name", getServiceId()).list().getItems(); | ||
.withField("metadata.name", serviceName).list().getItems(); | ||
services.forEach(service -> result.add(mapper.map(service))); | ||
} | ||
else { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the important bit right here : There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean this piece of code, right?
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you saying it is exactly the same because before the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not really, Im saying that:
is a longer version of :
according this this comment from the issue I linked and my testing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct, but "default" namespace is not the literal So that code we currently have, kind of makes no sense. |
||
namespaceProvider); | ||
LOG.debug(() -> "discovering services in namespace : " + namespace); | ||
Service service = kubernetesClient.services().inNamespace(namespace).withName(serviceName).get(); | ||
if (service != null) { | ||
result.add(mapper.map(service)); | ||
} | ||
else { | ||
LOG.debug(() -> "did not find service with name : " + serviceName + " in namespace : " + namespace); | ||
} | ||
} | ||
|
||
LOG.debug(() -> "found services : " + result); | ||
return Flux.defer(() -> Flux.just(result)); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,149 @@ | ||
/* | ||
* Copyright 2013-2024 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.springframework.cloud.kubernetes.fabric8.loadbalancer; | ||
|
||
import java.util.Comparator; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Set; | ||
|
||
import io.fabric8.kubernetes.api.model.Service; | ||
import io.fabric8.kubernetes.api.model.ServiceBuilder; | ||
import io.fabric8.kubernetes.api.model.ServicePortBuilder; | ||
import io.fabric8.kubernetes.api.model.ServiceSpecBuilder; | ||
import io.fabric8.kubernetes.client.Config; | ||
import io.fabric8.kubernetes.client.KubernetesClient; | ||
import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient; | ||
import org.junit.jupiter.api.AfterEach; | ||
import org.junit.jupiter.api.Assertions; | ||
import org.junit.jupiter.api.BeforeAll; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.extension.ExtendWith; | ||
|
||
import org.springframework.boot.test.system.CapturedOutput; | ||
import org.springframework.boot.test.system.OutputCaptureExtension; | ||
import org.springframework.cloud.client.ServiceInstance; | ||
import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties; | ||
import org.springframework.cloud.kubernetes.commons.loadbalancer.KubernetesLoadBalancerProperties; | ||
import org.springframework.core.env.Environment; | ||
import org.springframework.mock.env.MockEnvironment; | ||
|
||
/** | ||
* @author wind57 | ||
*/ | ||
@EnableKubernetesMockClient(crud = true, https = false) | ||
@ExtendWith(OutputCaptureExtension.class) | ||
class Fabric8ServicesListSupplierMockClientTests { | ||
|
||
private static KubernetesClient mockClient; | ||
|
||
@BeforeAll | ||
static void setUpBeforeClass() { | ||
// Configure the kubernetes master url to point to the mock server | ||
System.setProperty(Config.KUBERNETES_MASTER_SYSTEM_PROPERTY, mockClient.getConfiguration().getMasterUrl()); | ||
System.setProperty(Config.KUBERNETES_TRUST_CERT_SYSTEM_PROPERTY, "true"); | ||
System.setProperty(Config.KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY, "false"); | ||
System.setProperty(Config.KUBERNETES_AUTH_TRYSERVICEACCOUNT_SYSTEM_PROPERTY, "false"); | ||
System.setProperty(Config.KUBERNETES_NAMESPACE_SYSTEM_PROPERTY, "test"); | ||
System.setProperty(Config.KUBERNETES_HTTP2_DISABLE, "true"); | ||
} | ||
|
||
@AfterEach | ||
void afterEach() { | ||
mockClient.services().inAnyNamespace().delete(); | ||
} | ||
|
||
@Test | ||
void testAllNamespaces(CapturedOutput output) { | ||
|
||
createService("a", "service-a", 8887); | ||
createService("b", "service-b", 8888); | ||
createService("c", "service-a", 8889); | ||
|
||
Environment environment = new MockEnvironment().withProperty("loadbalancer.client.name", "service-a"); | ||
boolean allNamespaces = true; | ||
Set<String> selectiveNamespaces = Set.of(); | ||
|
||
KubernetesLoadBalancerProperties loadBalancerProperties = new KubernetesLoadBalancerProperties(); | ||
KubernetesDiscoveryProperties discoveryProperties = new KubernetesDiscoveryProperties(true, allNamespaces, | ||
selectiveNamespaces, true, 60, false, null, Set.of(), Map.of(), null, | ||
KubernetesDiscoveryProperties.Metadata.DEFAULT, 0, false, false, null); | ||
|
||
Fabric8ServicesListSupplier supplier = new Fabric8ServicesListSupplier(environment, mockClient, | ||
new Fabric8ServiceInstanceMapper(loadBalancerProperties, discoveryProperties), discoveryProperties); | ||
|
||
List<List<ServiceInstance>> serviceInstances = supplier.get().collectList().block(); | ||
Assertions.assertEquals(serviceInstances.size(), 1); | ||
List<ServiceInstance> inner = serviceInstances.get(0); | ||
|
||
List<ServiceInstance> serviceInstancesSorted = serviceInstances.get(0).stream() | ||
.sorted(Comparator.comparing(ServiceInstance::getServiceId)).toList(); | ||
Assertions.assertEquals(serviceInstancesSorted.size(), 2); | ||
Assertions.assertEquals(inner.get(0).getServiceId(), "service-a"); | ||
Assertions.assertEquals(inner.get(0).getHost(), "service-a.a.svc.cluster.local"); | ||
Assertions.assertEquals(inner.get(0).getPort(), 8887); | ||
|
||
Assertions.assertEquals(inner.get(1).getServiceId(), "service-a"); | ||
Assertions.assertEquals(inner.get(1).getHost(), "service-a.c.svc.cluster.local"); | ||
Assertions.assertEquals(inner.get(1).getPort(), 8889); | ||
|
||
Assertions.assertTrue(output.getOut().contains("discovering services in all namespaces")); | ||
} | ||
|
||
@Test | ||
void testOneNamespace(CapturedOutput output) { | ||
|
||
createService("a", "service-c", 8887); | ||
createService("b", "service-b", 8888); | ||
createService("c", "service-c", 8889); | ||
|
||
Environment environment = new MockEnvironment().withProperty("spring.cloud.kubernetes.client.namespace", "c") | ||
.withProperty("loadbalancer.client.name", "service-c"); | ||
boolean allNamespaces = false; | ||
Set<String> selectiveNamespaces = Set.of(); | ||
|
||
KubernetesLoadBalancerProperties loadBalancerProperties = new KubernetesLoadBalancerProperties(); | ||
KubernetesDiscoveryProperties discoveryProperties = new KubernetesDiscoveryProperties(true, allNamespaces, | ||
selectiveNamespaces, true, 60, false, null, Set.of(), Map.of(), null, | ||
KubernetesDiscoveryProperties.Metadata.DEFAULT, 0, false, false, null); | ||
|
||
Fabric8ServicesListSupplier supplier = new Fabric8ServicesListSupplier(environment, mockClient, | ||
new Fabric8ServiceInstanceMapper(loadBalancerProperties, discoveryProperties), discoveryProperties); | ||
|
||
List<List<ServiceInstance>> serviceInstances = supplier.get().collectList().block(); | ||
Assertions.assertEquals(serviceInstances.size(), 1); | ||
List<ServiceInstance> inner = serviceInstances.get(0); | ||
|
||
List<ServiceInstance> serviceInstancesSorted = serviceInstances.get(0).stream() | ||
.sorted(Comparator.comparing(ServiceInstance::getServiceId)).toList(); | ||
Assertions.assertEquals(serviceInstancesSorted.size(), 1); | ||
Assertions.assertEquals(inner.get(0).getServiceId(), "service-c"); | ||
Assertions.assertEquals(inner.get(0).getHost(), "service-c.c.svc.cluster.local"); | ||
Assertions.assertEquals(inner.get(0).getPort(), 8889); | ||
|
||
Assertions.assertTrue(output.getOut().contains("discovering services in namespace : c")); | ||
} | ||
|
||
private void createService(String namespace, String name, int port) { | ||
Service service = new ServiceBuilder().withNewMetadata().withNamespace(namespace).withName(name).endMetadata() | ||
.withSpec(new ServiceSpecBuilder() | ||
.withPorts(new ServicePortBuilder().withName("http").withPort(port).build()).build()) | ||
.build(); | ||
mockClient.services().resource(service).create(); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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