Drop overlapping configuration in blocking implementation (#1524)

spring-cloud · Nov 30, 2023 · 4bba72f · 4bba72f
1 parent 0e7215e
commit 4bba72f
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 30 deletions.
diff --git a/...org/springframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryProperties.java b/...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,15 +81,28 @@ public KubernetesDiscoveryProperties(@DefaultValue("true") boolean enabled, bool
 			@DefaultValue Map<String, String> 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<String> namespaces, @DefaultValue("true") boolean waitCacheReady,
+			@DefaultValue("60") long cacheLoadingTimeoutSeconds, boolean includeNotReadyAddresses, String filter,
+			@DefaultValue({ "443", "8443" }) Set<Integer> knownSecurePorts,
+			@DefaultValue Map<String, String> 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);
 	}
 
 	/**
 	 * Default instance.
 	 */
 	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/...pringframework/cloud/kubernetes/commons/discovery/KubernetesDiscoveryPropertiesTests.java b/...pringframework/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/...rc/main/java/org/springframework/cloud/kubernetes/discovery/ConfigServerBootstrapper.java b/...rc/main/java/org/springframework/cloud/kubernetes/discovery/ConfigServerBootstrapper.java
@@ -73,14 +73,13 @@ public List<ServiceInstance> 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/...c/main/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClient.java b/...c/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,15 +51,25 @@ 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";
 	}
 
 	@Override
 	public List<ServiceInstance> 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/...mework/cloud/kubernetes/discovery/KubernetesDiscoveryClientBlockingAutoConfiguration.java b/...mework/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/...a/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientProperties.java b/...a/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/...t/java/org/springframework/cloud/kubernetes/discovery/KubernetesDiscoveryClientTests.java b/...t/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,17 +115,19 @@ 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");
 	}
 
 	@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<String, String> metadata = new HashMap<>();
 		metadata.put("spring", "true");
@@ -140,9 +143,9 @@ void getServices() {
 	@MethodSource("servicesFilteredByNamespacesSource")
 	void getServicesFilteredByNamespaces(Set<String> namespaces, List<String> 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,19 +154,19 @@ void getServicesFilteredByNamespaces(Set<String> namespaces, List<String> expect
 	@MethodSource("instancesFilteredByNamespacesSource")
 	void getInstancesFilteredByNamespaces(Set<String> namespaces, String serviceId, List<String> 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);
 	}
 
 	private static Stream<Arguments> 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<Arguments> instancesFilteredByNamespacesSource() {