From 4bba72fc823c5b2bbd57504170f24669d6577919 Mon Sep 17 00:00:00 2001 From: erabii Date: Thu, 30 Nov 2023 23:14:23 +0200 Subject: [PATCH] Drop overlapping configuration in blocking implementation (#1524) --- .../KubernetesDiscoveryProperties.java | 20 +++++++++++-- .../KubernetesDiscoveryPropertiesTests.java | 5 +++- .../discovery/ConfigServerBootstrapper.java | 11 ++++--- .../discovery/KubernetesDiscoveryClient.java | 20 ++++++++++--- ...coveryClientBlockingAutoConfiguration.java | 6 ++-- .../KubernetesDiscoveryClientProperties.java | 4 +++ .../KubernetesDiscoveryClientTests.java | 29 ++++++++++--------- 7 files changed, 65 insertions(+), 30 deletions(-) diff --git a/spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryProperties.java b/spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryProperties.java index 0d8b803222..a07f367ae4 100644 --- a/spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryProperties.java +++ b/spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryProperties.java @@ -60,7 +60,8 @@ public record KubernetesDiscoveryProperties( @DefaultValue Metadata metadata, @DefaultValue("" + DEFAULT_ORDER) int order, boolean useEndpointSlices, - boolean includeExternalNameServices) { + boolean includeExternalNameServices, + String discoveryServerUrl) { // @formatter:on /** @@ -80,7 +81,20 @@ public KubernetesDiscoveryProperties(@DefaultValue("true") boolean enabled, bool @DefaultValue Map serviceLabels, String primaryPortName, @DefaultValue Metadata metadata, @DefaultValue("" + DEFAULT_ORDER) int order, boolean useEndpointSlices) { this(enabled, allNamespaces, namespaces, waitCacheReady, cacheLoadingTimeoutSeconds, includeNotReadyAddresses, - filter, knownSecurePorts, serviceLabels, primaryPortName, metadata, order, useEndpointSlices, false); + filter, knownSecurePorts, serviceLabels, primaryPortName, metadata, order, useEndpointSlices, false, + null); + } + + public KubernetesDiscoveryProperties(@DefaultValue("true") boolean enabled, boolean allNamespaces, + @DefaultValue Set namespaces, @DefaultValue("true") boolean waitCacheReady, + @DefaultValue("60") long cacheLoadingTimeoutSeconds, boolean includeNotReadyAddresses, String filter, + @DefaultValue({ "443", "8443" }) Set knownSecurePorts, + @DefaultValue Map serviceLabels, String primaryPortName, @DefaultValue Metadata metadata, + @DefaultValue("" + DEFAULT_ORDER) int order, boolean useEndpointSlices, + boolean includeExternalNameServices) { + this(enabled, allNamespaces, namespaces, waitCacheReady, cacheLoadingTimeoutSeconds, includeNotReadyAddresses, + filter, knownSecurePorts, serviceLabels, primaryPortName, metadata, order, useEndpointSlices, + includeExternalNameServices, null); } /** @@ -88,7 +102,7 @@ public KubernetesDiscoveryProperties(@DefaultValue("true") boolean enabled, bool */ public static final KubernetesDiscoveryProperties DEFAULT = new KubernetesDiscoveryProperties(true, false, Set.of(), true, 60, false, null, Set.of(), Map.of(), null, KubernetesDiscoveryProperties.Metadata.DEFAULT, 0, false, - false); + false, null); /** * @param addLabels include labels as metadata diff --git a/spring-cloud-kubernetes-commons/src/test/java/org/springframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryPropertiesTests.java b/spring-cloud-kubernetes-commons/src/test/java/org/springframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryPropertiesTests.java index f3d62f1cda..74e9f5e54e 100644 --- a/spring-cloud-kubernetes-commons/src/test/java/org/springframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryPropertiesTests.java +++ b/spring-cloud-kubernetes-commons/src/test/java/org/springframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryPropertiesTests.java @@ -54,6 +54,7 @@ void testBindingWhenNoPropertiesProvided() { assertThat(props.order()).isZero(); assertThat(props.useEndpointSlices()).isFalse(); assertThat(props.includeExternalNameServices()).isFalse(); + assertThat(props.discoveryServerUrl()).isNull(); }); } @@ -66,7 +67,8 @@ void testBindingWhenSomePropertiesProvided() { "spring.cloud.kubernetes.discovery.use-endpoint-slices=true", "spring.cloud.kubernetes.discovery.namespaces[0]=ns1", "spring.cloud.kubernetes.discovery.namespaces[1]=ns2", - "spring.cloud.kubernetes.discovery.include-external-name-services=true") + "spring.cloud.kubernetes.discovery.include-external-name-services=true", + "spring.cloud.kubernetes.discovery.discovery-server-url=http://example") .run(context -> { KubernetesDiscoveryProperties props = context.getBean(KubernetesDiscoveryProperties.class); assertThat(props).isNotNull(); @@ -87,6 +89,7 @@ void testBindingWhenSomePropertiesProvided() { assertThat(props.order()).isZero(); assertThat(props.useEndpointSlices()).isTrue(); assertThat(props.includeExternalNameServices()).isTrue(); + assertThat(props.discoveryServerUrl()).isEqualTo("http://example"); }); } diff --git a/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/ConfigServerBootstrapper.java b/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/ConfigServerBootstrapper.java index 66158e7fe2..2e96d8b667 100644 --- a/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/ConfigServerBootstrapper.java +++ b/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/ConfigServerBootstrapper.java @@ -73,14 +73,13 @@ public List apply(String serviceId, Binder binder, BindHandler } private KubernetesConfigServerInstanceProvider getInstanceProvider(Binder binder, BindHandler bindHandler) { - KubernetesDiscoveryClientProperties kubernetesDiscoveryClientProperties = binder - .bind(KubernetesDiscoveryProperties.PREFIX, Bindable.of(KubernetesDiscoveryClientProperties.class), + KubernetesDiscoveryProperties kubernetesDiscoveryProperties = binder + .bind(KubernetesDiscoveryProperties.PREFIX, Bindable.of(KubernetesDiscoveryProperties.class), bindHandler) - .orElseGet(KubernetesDiscoveryClientProperties::new); - KubernetesDiscoveryClientBlockingAutoConfiguration autoConfiguration = - new KubernetesDiscoveryClientBlockingAutoConfiguration(); + .orElseGet(() -> KubernetesDiscoveryProperties.DEFAULT); + KubernetesDiscoveryClientBlockingAutoConfiguration autoConfiguration = new KubernetesDiscoveryClientBlockingAutoConfiguration(); DiscoveryClient discoveryClient = autoConfiguration - .kubernetesDiscoveryClient(autoConfiguration.restTemplate(), kubernetesDiscoveryClientProperties); + .kubernetesDiscoveryClient(autoConfiguration.restTemplate(), kubernetesDiscoveryProperties); return discoveryClient::getInstances; } diff --git a/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClient.java b/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClient.java index 207919519c..415ecda9d8 100644 --- a/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClient.java +++ b/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClient.java @@ -23,6 +23,7 @@ import org.springframework.cloud.client.ServiceInstance; import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties; import org.springframework.util.StringUtils; import org.springframework.web.client.RestTemplate; @@ -39,6 +40,7 @@ public class KubernetesDiscoveryClient implements DiscoveryClient { private final String discoveryServerUrl; + @Deprecated(forRemoval = true) public KubernetesDiscoveryClient(RestTemplate rest, KubernetesDiscoveryClientProperties properties) { if (!StringUtils.hasText(properties.getDiscoveryServerUrl())) { throw new DiscoveryServerUrlInvalidException(); @@ -49,6 +51,16 @@ public KubernetesDiscoveryClient(RestTemplate rest, KubernetesDiscoveryClientPro this.discoveryServerUrl = properties.getDiscoveryServerUrl(); } + KubernetesDiscoveryClient(RestTemplate rest, KubernetesDiscoveryProperties kubernetesDiscoveryProperties) { + if (!StringUtils.hasText(kubernetesDiscoveryProperties.discoveryServerUrl())) { + throw new DiscoveryServerUrlInvalidException(); + } + this.rest = rest; + this.emptyNamespaces = kubernetesDiscoveryProperties.namespaces().isEmpty(); + this.namespaces = kubernetesDiscoveryProperties.namespaces(); + this.discoveryServerUrl = kubernetesDiscoveryProperties.discoveryServerUrl(); + } + @Override public String description() { return "Kubernetes Discovery Client"; @@ -56,8 +68,8 @@ public String description() { @Override public List getInstances(String serviceId) { - KubernetesServiceInstance[] responseBody = rest.getForEntity( - discoveryServerUrl + "/apps/" + serviceId, KubernetesServiceInstance[].class).getBody(); + KubernetesServiceInstance[] responseBody = rest + .getForEntity(discoveryServerUrl + "/apps/" + serviceId, KubernetesServiceInstance[].class).getBody(); if (responseBody != null && responseBody.length > 0) { return Arrays.stream(responseBody).filter(this::matchNamespaces).collect(Collectors.toList()); } @@ -79,8 +91,8 @@ private boolean matchNamespaces(KubernetesServiceInstance kubernetesServiceInsta } private boolean matchNamespaces(Service service) { - return service.getServiceInstances().isEmpty() || - service.getServiceInstances().stream().anyMatch(this::matchNamespaces); + return service.getServiceInstances().isEmpty() + || service.getServiceInstances().stream().anyMatch(this::matchNamespaces); } } diff --git a/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientBlockingAutoConfiguration.java b/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientBlockingAutoConfiguration.java index 35f3490407..da4512f6bc 100644 --- a/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientBlockingAutoConfiguration.java +++ b/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientBlockingAutoConfiguration.java @@ -24,6 +24,7 @@ import org.springframework.cloud.kubernetes.commons.discovery.ConditionalOnSpringCloudKubernetesBlockingDiscovery; import org.springframework.cloud.kubernetes.commons.discovery.ConditionalOnSpringCloudKubernetesBlockingDiscoveryHealthInitializer; import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryClientHealthIndicatorInitializer; +import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties; import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -35,8 +36,7 @@ @Configuration(proxyBeanMethods = false) @ConditionalOnSpringCloudKubernetesBlockingDiscovery -@EnableConfigurationProperties({ DiscoveryClientHealthIndicatorProperties.class, - KubernetesDiscoveryClientProperties.class }) +@EnableConfigurationProperties({ DiscoveryClientHealthIndicatorProperties.class, KubernetesDiscoveryProperties.class }) class KubernetesDiscoveryClientBlockingAutoConfiguration { @Bean @@ -48,7 +48,7 @@ RestTemplate restTemplate() { @Bean @ConditionalOnMissingBean KubernetesDiscoveryClient kubernetesDiscoveryClient(RestTemplate restTemplate, - KubernetesDiscoveryClientProperties properties) { + KubernetesDiscoveryProperties properties) { return new KubernetesDiscoveryClient(restTemplate, properties); } diff --git a/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientProperties.java b/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientProperties.java index 91c1d9b344..c93de0d1e9 100644 --- a/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientProperties.java +++ b/spring-cloud-kubernetes-discovery/src/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientProperties.java @@ -22,7 +22,11 @@ /** * @author Ryan Baxter + * @deprecated use + * {@link org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties} + * instead. */ +@Deprecated(forRemoval = true) @ConfigurationProperties("spring.cloud.kubernetes.discovery") public class KubernetesDiscoveryClientProperties { diff --git a/spring-cloud-kubernetes-discovery/src/test/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientTests.java b/spring-cloud-kubernetes-discovery/src/test/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientTests.java index c64de7ec6f..ef9ba4be1c 100644 --- a/spring-cloud-kubernetes-discovery/src/test/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientTests.java +++ b/spring-cloud-kubernetes-discovery/src/test/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientTests.java @@ -33,6 +33,7 @@ import org.springframework.boot.web.client.RestTemplateBuilder; import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties; import org.springframework.web.client.RestTemplate; import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; @@ -114,8 +115,9 @@ static void beforeAll() { @Test void getInstances() { RestTemplate rest = new RestTemplateBuilder().build(); - KubernetesDiscoveryClientProperties properties = new KubernetesDiscoveryClientProperties(); - properties.setDiscoveryServerUrl(wireMockServer.baseUrl()); + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60, + false, null, Set.of(), Map.of(), null, KubernetesDiscoveryProperties.Metadata.DEFAULT, 0, false, false, + wireMockServer.baseUrl()); KubernetesDiscoveryClient discoveryClient = new KubernetesDiscoveryClient(rest, properties); assertThat(discoveryClient.getServices()).contains("test-svc-1", "test-svc-3"); } @@ -123,8 +125,9 @@ void getInstances() { @Test void getServices() { RestTemplate rest = new RestTemplateBuilder().build(); - KubernetesDiscoveryClientProperties properties = new KubernetesDiscoveryClientProperties(); - properties.setDiscoveryServerUrl(wireMockServer.baseUrl()); + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60, + false, null, Set.of(), Map.of(), null, KubernetesDiscoveryProperties.Metadata.DEFAULT, 0, false, false, + wireMockServer.baseUrl()); KubernetesDiscoveryClient discoveryClient = new KubernetesDiscoveryClient(rest, properties); Map metadata = new HashMap<>(); metadata.put("spring", "true"); @@ -140,9 +143,9 @@ void getServices() { @MethodSource("servicesFilteredByNamespacesSource") void getServicesFilteredByNamespaces(Set namespaces, List expectedServices) { RestTemplate rest = new RestTemplateBuilder().build(); - KubernetesDiscoveryClientProperties properties = new KubernetesDiscoveryClientProperties(); - properties.setNamespaces(namespaces); - properties.setDiscoveryServerUrl(wireMockServer.baseUrl()); + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, namespaces, true, 60, + false, null, Set.of(), Map.of(), null, KubernetesDiscoveryProperties.Metadata.DEFAULT, 0, false, false, + wireMockServer.baseUrl()); KubernetesDiscoveryClient discoveryClient = new KubernetesDiscoveryClient(rest, properties); assertThat(discoveryClient.getServices()).containsExactlyInAnyOrderElementsOf(expectedServices); } @@ -151,9 +154,9 @@ void getServicesFilteredByNamespaces(Set namespaces, List expect @MethodSource("instancesFilteredByNamespacesSource") void getInstancesFilteredByNamespaces(Set namespaces, String serviceId, List expectedInstances) { RestTemplate rest = new RestTemplateBuilder().build(); - KubernetesDiscoveryClientProperties properties = new KubernetesDiscoveryClientProperties(); - properties.setNamespaces(namespaces); - properties.setDiscoveryServerUrl(wireMockServer.baseUrl()); + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, namespaces, true, 60, + false, null, Set.of(), Map.of(), null, KubernetesDiscoveryProperties.Metadata.DEFAULT, 0, false, false, + wireMockServer.baseUrl()); KubernetesDiscoveryClient discoveryClient = new KubernetesDiscoveryClient(rest, properties); assertThat(discoveryClient.getInstances(serviceId)).map(ServiceInstance::getInstanceId) .containsExactlyInAnyOrderElementsOf(expectedInstances); @@ -161,9 +164,9 @@ void getInstancesFilteredByNamespaces(Set namespaces, String serviceId, private static Stream servicesFilteredByNamespacesSource() { return Stream.of(Arguments.of(Set.of(), List.of("test-svc-1", "test-svc-3")), - Arguments.of(Set.of("namespace1", "namespace2"), List.of("test-svc-1", "test-svc-3")), - Arguments.of(Set.of("namespace1"), List.of("test-svc-1")), - Arguments.of(Set.of("namespace2", "does-not-exist"), List.of("test-svc-3"))); + Arguments.of(Set.of("namespace1", "namespace2"), List.of("test-svc-1", "test-svc-3")), + Arguments.of(Set.of("namespace1"), List.of("test-svc-1")), + Arguments.of(Set.of("namespace2", "does-not-exist"), List.of("test-svc-3"))); } private static Stream instancesFilteredByNamespacesSource() {