move ports metadata computation to the dedicated method (#1231)

spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClient.java

@@ -17,14 +17,12 @@
 package org.springframework.cloud.kubernetes.fabric8.discovery;
 
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.function.Predicate;
 
 import io.fabric8.kubernetes.api.model.EndpointAddress;
-import io.fabric8.kubernetes.api.model.EndpointPort;
 import io.fabric8.kubernetes.api.model.EndpointSubset;
 import io.fabric8.kubernetes.api.model.Endpoints;
 import io.fabric8.kubernetes.api.model.Service;
@@ -37,10 +35,7 @@ import org.springframework.cloud.kubernetes.commons.discovery.DefaultKubernetesS
 import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties;
 import org.springframework.core.log.LogAccessor;
 import org.springframework.util.CollectionUtils;
-import org.springframework.util.StringUtils;
 
-import static java.util.stream.Collectors.toMap;
-import static org.springframework.cloud.kubernetes.commons.config.ConfigUtils.keysWithPrefix;
 import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.NAMESPACE_METADATA_KEY;
 import static org.springframework.cloud.kubernetes.fabric8.discovery.KubernetesDiscoveryClientUtils.endpoints;
 import static org.springframework.cloud.kubernetes.fabric8.discovery.KubernetesDiscoveryClientUtils.endpointsPort;
@@ -135,52 +130,47 @@ public class KubernetesDiscoveryClient implements DiscoveryClient {
 	}
 
 	private List<ServiceInstance> getNamespaceServiceInstances(EndpointSubsetNS es, String serviceId) {
-		String namespace = es.namespace();
+
 		List<EndpointSubset> subsets = es.endpointSubset();
+		if (subsets.isEmpty()) {
+			LOG.debug(() -> "serviceId : " + serviceId + " does not have any subsets");
+			return List.of();
+		}
+
+		String namespace = es.namespace();
 		List<ServiceInstance> instances = new ArrayList<>();
-		if (!subsets.isEmpty()) {
-			Service service = client.services().inNamespace(namespace).withName(serviceId).get();
-			Map<String, String> serviceMetadata = serviceMetadata(serviceId, service, properties);
-			KubernetesDiscoveryProperties.Metadata metadataProps = properties.metadata();
 
-			for (EndpointSubset s : subsets) {
-				// Extend the service metadata map with per-endpoint port information (if
-				// requested)
-				Map<String, String> endpointMetadata = new HashMap<>(serviceMetadata);
-				if (metadataProps.addPorts()) {
-					Map<String, String> ports = s.getPorts().stream()
-							.filter(port -> StringUtils.hasText(port.getName()))
-							.collect(toMap(EndpointPort::getName, port -> Integer.toString(port.getPort())));
-					Map<String, String> portMetadata = keysWithPrefix(ports, metadataProps.portsPrefix());
-					LOG.debug(() -> "Adding port metadata: " + portMetadata);
-					endpointMetadata.putAll(portMetadata);
+		Service service = client.services().inNamespace(namespace).withName(serviceId).get();
+		Map<String, String> serviceMetadata = serviceMetadata(serviceId, service, properties, subsets);
+
+		for (EndpointSubset endpointSubset : subsets) {
+
+			if (properties.allNamespaces()) {
+				serviceMetadata.put(NAMESPACE_METADATA_KEY, namespace);
+			}
+
+			List<EndpointAddress> addresses = endpointSubset.getAddresses();
+
+			if (properties.includeNotReadyAddresses()
+					&& !CollectionUtils.isEmpty(endpointSubset.getNotReadyAddresses())) {
+				if (addresses == null) {
+					addresses = new ArrayList<>();
 				}
+				addresses.addAll(endpointSubset.getNotReadyAddresses());
+			}
 
-				if (properties.allNamespaces()) {
-					endpointMetadata.put(NAMESPACE_METADATA_KEY, namespace);
-				}
-
-				List<EndpointAddress> addresses = s.getAddresses();
-
-				if (properties.includeNotReadyAddresses() && !CollectionUtils.isEmpty(s.getNotReadyAddresses())) {
-					if (addresses == null) {
-						addresses = new ArrayList<>();
-					}
-					addresses.addAll(s.getNotReadyAddresses());
-				}
-
-				for (EndpointAddress endpointAddress : addresses) {
-					int endpointPort = endpointsPort(s, serviceId, properties, service);
-					String instanceId = null;
-					if (endpointAddress.getTargetRef() != null) {
-						instanceId = endpointAddress.getTargetRef().getUid();
-					}
-					instances.add(new DefaultKubernetesServiceInstance(instanceId, serviceId, endpointAddress.getIp(),
-							endpointPort, endpointMetadata,
-							servicePortSecureResolver.resolve(new ServicePortSecureResolver.Input(endpointPort,
-									service.getMetadata().getName(), service.getMetadata().getLabels(),
-									service.getMetadata().getAnnotations()))));
+			for (EndpointAddress endpointAddress : addresses) {
+				int endpointPort = endpointsPort(endpointSubset, serviceId, properties, service);
+				String instanceId = null;
+				if (endpointAddress.getTargetRef() != null) {
+					instanceId = endpointAddress.getTargetRef().getUid();
 				}
+				instances
+						.add(new DefaultKubernetesServiceInstance(instanceId, serviceId, endpointAddress.getIp(),
+								endpointPort, serviceMetadata,
+								servicePortSecureResolver.resolve(new ServicePortSecureResolver.Input(endpointPort,
+										service.getMetadata().getName(), service.getMetadata().getLabels(),
+										service.getMetadata().getAnnotations()))));
 			}
 		}
 

spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtils.java

@@ -36,6 +36,7 @@ import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscover
 import org.springframework.core.log.LogAccessor;
 import org.springframework.util.StringUtils;
 
+import static java.util.stream.Collectors.toMap;
 import static org.springframework.cloud.kubernetes.commons.config.ConfigUtils.keysWithPrefix;
 import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.HTTP;
 import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.HTTPS;
@@ -121,8 +122,11 @@ final class KubernetesDiscoveryClientUtils {
 		return primaryPortName;
 	}
 
+	/**
+	 * labels, annotations and ports metadata.
+	 */
 	static Map<String, String> serviceMetadata(String serviceId, Service service,
-			KubernetesDiscoveryProperties properties) {
+			KubernetesDiscoveryProperties properties, List<EndpointSubset> endpointSubsets) {
 		Map<String, String> serviceMetadata = new HashMap<>();
 		KubernetesDiscoveryProperties.Metadata metadataProps = properties.metadata();
 		if (metadataProps.addLabels()) {
@@ -138,6 +142,16 @@ final class KubernetesDiscoveryClientUtils {
 			serviceMetadata.putAll(annotationMetadata);
 		}
 
+		if (metadataProps.addPorts()) {
+			Map<String, String> ports = endpointSubsets.stream()
+					.flatMap(endpointSubset -> endpointSubset.getPorts().stream())
+					.filter(port -> StringUtils.hasText(port.getName()))
+					.collect(toMap(EndpointPort::getName, port -> Integer.toString(port.getPort())));
+			Map<String, String> portMetadata = keysWithPrefix(ports, properties.metadata().portsPrefix());
+			LOG.debug(() -> "Adding port metadata: " + portMetadata + " for serviceId : " + serviceId);
+			serviceMetadata.putAll(portMetadata);
+		}
+
 		return serviceMetadata;
 	}
 

spring-cloud-kubernetes-fabric8-discovery/src/test/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtilsTests.java

@@ -333,13 +333,17 @@ class KubernetesDiscoveryClientUtilsTests {
 		String labelsPrefix = "";
 		boolean addAnnotations = false;
 		String annotationsPrefix = "";
+		boolean addPorts = false;
+		String portsPrefix = "";
+
 		KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
-				labelsPrefix, addAnnotations, annotationsPrefix, false, "");
+				labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
 		KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
 				true, "", Set.of(), Map.of(), "", metadata, 0, false);
 		Service service = new ServiceBuilder().build();
 
-		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
+		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
+				List.of());
 		Assertions.assertEquals(result.size(), 0);
 	}
 
@@ -355,14 +359,18 @@ class KubernetesDiscoveryClientUtilsTests {
 		String labelsPrefix = "";
 		boolean addAnnotations = false;
 		String annotationsPrefix = "";
+		boolean addPorts = false;
+		String portsPrefix = "";
+
 		KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
-				labelsPrefix, addAnnotations, annotationsPrefix, false, "");
+				labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
 		KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
 				true, "", Set.of(), Map.of(), "", metadata, 0, false);
 		Service service = new ServiceBuilder()
 				.withMetadata(new ObjectMetaBuilder().withLabels(Map.of("a", "b")).build()).build();
 
-		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
+		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
+				List.of());
 		Assertions.assertEquals(result.size(), 1);
 		Assertions.assertEquals(result, Map.of("a", "b"));
 		Assertions.assertTrue(output.getOut().contains("Adding labels metadata: {a=b} for serviceId: my-service"));
@@ -380,14 +388,18 @@ class KubernetesDiscoveryClientUtilsTests {
 		String labelsPrefix = "prefix-";
 		boolean addAnnotations = false;
 		String annotationsPrefix = "";
+		boolean addPorts = false;
+		String portsPrefix = "";
+
 		KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
-				labelsPrefix, addAnnotations, annotationsPrefix, false, "");
+				labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
 		KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
 				true, "", Set.of(), Map.of(), "", metadata, 0, false);
 		Service service = new ServiceBuilder()
 				.withMetadata(new ObjectMetaBuilder().withLabels(Map.of("a", "b", "c", "d")).build()).build();
 
-		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
+		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
+				List.of());
 		Assertions.assertEquals(result.size(), 2);
 		Assertions.assertEquals(result, Map.of("prefix-a", "b", "prefix-c", "d"));
 		// so that result is deterministic in assertion
@@ -408,15 +420,19 @@ class KubernetesDiscoveryClientUtilsTests {
 		String labelsPrefix = "";
 		boolean addAnnotations = true;
 		String annotationsPrefix = "";
+		boolean addPorts = false;
+		String portsPrefix = "";
+
 		KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
-				labelsPrefix, addAnnotations, annotationsPrefix, false, "");
+				labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
 		KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
 				true, "", Set.of(), Map.of(), "", metadata, 0, false);
 		Service service = new ServiceBuilder().withMetadata(
 				new ObjectMetaBuilder().withAnnotations(Map.of("aa", "bb")).withLabels(Map.of("a", "b")).build())
 				.build();
 
-		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
+		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
+				List.of());
 		Assertions.assertEquals(result.size(), 1);
 		Assertions.assertEquals(result, Map.of("aa", "bb"));
 		Assertions
@@ -430,19 +446,23 @@ class KubernetesDiscoveryClientUtilsTests {
 	 * </pre>
 	 */
 	@Test
-	void testServiceMetadataAddAnnotationsWithPrefixPrefix(CapturedOutput output) {
+	void testServiceMetadataAddAnnotationsWithPrefix(CapturedOutput output) {
 		boolean addLabels = false;
 		String labelsPrefix = "";
 		boolean addAnnotations = true;
 		String annotationsPrefix = "prefix-";
+		boolean addPorts = false;
+		String portsPrefix = "";
+
 		KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
-				labelsPrefix, addAnnotations, annotationsPrefix, false, "");
+				labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
 		KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
 				true, "", Set.of(), Map.of(), "", metadata, 0, false);
 		Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder()
 				.withAnnotations(Map.of("aa", "bb", "cc", "dd")).withLabels(Map.of("a", "b")).build()).build();
 
-		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
+		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
+				List.of());
 		Assertions.assertEquals(result.size(), 2);
 		Assertions.assertEquals(result, Map.of("prefix-aa", "bb", "prefix-cc", "dd"));
 		// so that result is deterministic in assertion
@@ -463,15 +483,19 @@ class KubernetesDiscoveryClientUtilsTests {
 		String labelsPrefix = "label-";
 		boolean addAnnotations = true;
 		String annotationsPrefix = "annotation-";
+		boolean addPorts = false;
+		String portsPrefix = "";
+
 		KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
-				labelsPrefix, addAnnotations, annotationsPrefix, false, "");
+				labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
 		KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
 				true, "", Set.of(), Map.of(), "", metadata, 0, false);
 		Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder()
 				.withAnnotations(Map.of("aa", "bb", "cc", "dd")).withLabels(Map.of("a", "b", "c", "d")).build())
 				.build();
 
-		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
+		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
+				List.of());
 		Assertions.assertEquals(result.size(), 4);
 		Assertions.assertEquals(result,
 				Map.of("annotation-aa", "bb", "annotation-cc", "dd", "label-a", "b", "label-c", "d"));
@@ -486,4 +510,74 @@ class KubernetesDiscoveryClientUtilsTests {
 				output.getOut().contains("Adding annotations metadata: " + annotations + " for serviceId: my-service"));
 	}
 
+	/**
+	 * <pre>
+	 *     - ports without prefix are added
+	 * </pre>
+	 */
+	@Test
+	void testServiceMetadataAddPortsWithoutPrefix(CapturedOutput output) {
+		boolean addLabels = false;
+		String labelsPrefix = "";
+		boolean addAnnotations = false;
+		String annotationsPrefix = "prefix-";
+		boolean addPorts = true;
+		String portsPrefix = "";
+
+		KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
+				labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
+		KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
+				true, "", Set.of(), Map.of(), "", metadata, 0, false);
+		Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder()
+				.withAnnotations(Map.of("aa", "bb", "cc", "dd")).withLabels(Map.of("a", "b")).build()).build();
+
+		List<EndpointSubset> endpointSubsets = List.of(
+				new EndpointSubsetBuilder().withPorts(new EndpointPortBuilder().withPort(8081).withName("").build())
+						.build(),
+				new EndpointSubsetBuilder()
+						.withPorts(new EndpointPortBuilder().withPort(8080).withName("https").build()).build());
+
+		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
+				endpointSubsets);
+		Assertions.assertEquals(result.size(), 1);
+		Assertions.assertEquals(result, Map.of("https", "8080"));
+		Assertions
+				.assertTrue(output.getOut().contains("Adding port metadata: {https=8080} for serviceId : my-service"));
+	}
+
+	/**
+	 * <pre>
+	 *     - ports without prefix are added
+	 * </pre>
+	 */
+	@Test
+	void testServiceMetadataAddPortsWithPrefix(CapturedOutput output) {
+		boolean addLabels = false;
+		String labelsPrefix = "";
+		boolean addAnnotations = false;
+		String annotationsPrefix = "prefix-";
+		boolean addPorts = true;
+		String portsPrefix = "prefix-";
+
+		KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
+				labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
+		KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
+				true, "", Set.of(), Map.of(), "", metadata, 0, false);
+		Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder()
+				.withAnnotations(Map.of("aa", "bb", "cc", "dd")).withLabels(Map.of("a", "b")).build()).build();
+
+		List<EndpointSubset> endpointSubsets = List.of(
+				new EndpointSubsetBuilder().withPorts(new EndpointPortBuilder().withPort(8081).withName("http").build())
+						.build(),
+				new EndpointSubsetBuilder()
+						.withPorts(new EndpointPortBuilder().withPort(8080).withName("https").build()).build());
+
+		Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
+				endpointSubsets);
+		Assertions.assertEquals(result.size(), 2);
+		Assertions.assertEquals(result, Map.of("prefix-https", "8080", "prefix-http", "8081"));
+		Assertions.assertTrue(output.getOut()
+				.contains("Adding port metadata: {prefix-http=8081, prefix-https=8080} for serviceId : my-service"));
+	}
+
 }