diff --git a/spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClient.java b/spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClient.java index 50210e1d..51d07512 100644 --- a/spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClient.java +++ b/spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClient.java @@ -42,10 +42,8 @@ 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; import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.NAMESPACE_METADATA_KEY; -import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.PRIMARY_PORT_NAME_LABEL_KEY; +import static org.springframework.cloud.kubernetes.fabric8.discovery.KubernetesDiscoveryClientUtils.endpointsPort; import static org.springframework.cloud.kubernetes.fabric8.discovery.KubernetesDiscoveryClientUtils.subsetsFromEndpoints; /** @@ -148,12 +146,6 @@ public class KubernetesDiscoveryClient implements DiscoveryClient { final Map serviceMetadata = this.getServiceMetadata(service); KubernetesDiscoveryProperties.Metadata metadataProps = this.properties.metadata(); - String primaryPortName = this.properties.primaryPortName(); - Map labels = service.getMetadata().getLabels(); - if (labels != null && labels.containsKey(PRIMARY_PORT_NAME_LABEL_KEY)) { - primaryPortName = labels.get(PRIMARY_PORT_NAME_LABEL_KEY); - } - for (EndpointSubset s : subsets) { // Extend the service metadata map with per-endpoint port information (if // requested) @@ -183,7 +175,7 @@ public class KubernetesDiscoveryClient implements DiscoveryClient { } for (EndpointAddress endpointAddress : addresses) { - int endpointPort = findEndpointPort(s, serviceId, primaryPortName); + int endpointPort = endpointsPort(s, serviceId, properties, service); String instanceId = null; if (endpointAddress.getTargetRef() != null) { instanceId = endpointAddress.getTargetRef().getUid(); @@ -223,41 +215,6 @@ public class KubernetesDiscoveryClient implements DiscoveryClient { return serviceMetadata; } - private int findEndpointPort(EndpointSubset s, String serviceId, String primaryPortName) { - List endpointPorts = s.getPorts(); - if (endpointPorts.size() == 1) { - return endpointPorts.get(0).getPort(); - } - else { - Map ports = endpointPorts.stream().filter(p -> StringUtils.hasText(p.getName())) - .collect(Collectors.toMap(EndpointPort::getName, EndpointPort::getPort)); - // This oneliner is looking for a port with a name equal to the primary port - // name specified in the service label - // or in spring.cloud.kubernetes.discovery.primary-port-name, equal to https, - // or equal to http. - // In case no port has been found return -1 to log a warning and fall back to - // the first port in the list. - int discoveredPort = ports.getOrDefault(primaryPortName, - ports.getOrDefault(HTTPS, ports.getOrDefault(HTTP, -1))); - - if (discoveredPort == -1) { - if (StringUtils.hasText(primaryPortName)) { - log.warn("Could not find a port named '" + primaryPortName + "', 'https', or 'http' for service '" - + serviceId + "'."); - } - else { - log.warn("Could not find a port named 'https' or 'http' for service '" + serviceId + "'."); - } - log.warn( - "Make sure that either the primary-port-name label has been added to the service, or that spring.cloud.kubernetes.discovery.primary-port-name has been configured."); - log.warn("Alternatively name the primary port 'https' or 'http'"); - log.warn("An incorrect configuration may result in non-deterministic behaviour."); - discoveredPort = endpointPorts.get(0).getPort(); - } - return discoveredPort; - } - } - @Override public List getServices() { return adapter.apply(client).stream().map(s -> s.getMetadata().getName()).toList(); diff --git a/spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtils.java b/spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtils.java index 0dd26bb5..e3494e33 100644 --- a/spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtils.java +++ b/spring-cloud-kubernetes-fabric8-discovery/src/main/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtils.java @@ -17,15 +17,32 @@ package org.springframework.cloud.kubernetes.fabric8.discovery; import java.util.List; +import java.util.Map; +import java.util.Optional; import java.util.function.Supplier; +import java.util.stream.Collectors; +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; +import org.apache.commons.logging.LogFactory; + +import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties; +import org.springframework.core.log.LogAccessor; +import org.springframework.util.StringUtils; + +import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.HTTP; +import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.HTTPS; +import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.PRIMARY_PORT_NAME_LABEL_KEY; /** * @author wind57 */ final class KubernetesDiscoveryClientUtils { + private static final LogAccessor LOG = new LogAccessor(LogFactory.getLog(KubernetesDiscoveryClientUtils.class)); + private KubernetesDiscoveryClientUtils() { } @@ -37,4 +54,89 @@ final class KubernetesDiscoveryClientUtils { return new EndpointSubsetNS(clientNamespace.get(), List.of()); } + static int endpointsPort(EndpointSubset endpointSubset, String serviceId, KubernetesDiscoveryProperties properties, + Service service) { + + List endpointPorts = endpointSubset.getPorts(); + if (endpointPorts.size() == 1) { + int port = endpointPorts.get(0).getPort(); + LOG.debug(() -> "endpoint ports has a single entry, using port : " + port); + return port; + } + + else { + + Optional port; + String primaryPortName = primaryPortName(properties, service, serviceId); + + Map existingPorts = endpointPorts.stream() + .filter(endpointPort -> StringUtils.hasText(endpointPort.getName())) + .collect(Collectors.toMap(EndpointPort::getName, EndpointPort::getPort)); + + port = fromMap(existingPorts, primaryPortName, "found primary-port-name (with value: '" + primaryPortName + + "') via properties or service labels to match port"); + if (port.isPresent()) { + return port.get(); + } + + port = fromMap(existingPorts, HTTPS, "found primary-port-name via 'https' to match port"); + if (port.isPresent()) { + return port.get(); + } + + port = fromMap(existingPorts, HTTP, "found primary-port-name via 'http' to match port"); + if (port.isPresent()) { + return port.get(); + } + + logWarnings(); + return endpointPorts.get(0).getPort(); + + } + } + + /** + * take primary-port-name from service label "PRIMARY_PORT_NAME_LABEL_KEY" if it + * exists, otherwise from KubernetesDiscoveryProperties if it exists, otherwise null. + */ + static String primaryPortName(KubernetesDiscoveryProperties properties, Service service, String serviceId) { + String primaryPortNameFromProperties = properties.primaryPortName(); + Map serviceLabels = service.getMetadata().getLabels(); + + // the value from labels takes precedence over the one from properties + String primaryPortName = Optional + .ofNullable(Optional.ofNullable(serviceLabels).orElse(Map.of()).get(PRIMARY_PORT_NAME_LABEL_KEY)) + .orElse(primaryPortNameFromProperties); + + if (primaryPortName == null) { + LOG.debug( + () -> "did not find a primary-port-name in neither properties nor service labels for service with ID : " + + serviceId); + return null; + } + + LOG.debug(() -> "will use primaryPortName : " + primaryPortName + " for service with ID = " + serviceId); + return primaryPortName; + } + + private static Optional fromMap(Map existingPorts, String key, String message) { + Integer fromPrimaryPortName = existingPorts.get(key); + if (fromPrimaryPortName == null) { + LOG.debug(() -> "not " + message); + return Optional.empty(); + } + else { + LOG.debug(() -> message + " : " + fromPrimaryPortName); + return Optional.of(fromPrimaryPortName); + } + } + + private static void logWarnings() { + LOG.warn(() -> """ + Make sure that either the primary-port-name label has been added to the service, + or spring.cloud.kubernetes.discovery.primary-port-name has been configured. + Alternatively name the primary port 'https' or 'http' + An incorrect configuration may result in non-deterministic behaviour."""); + } + } diff --git a/spring-cloud-kubernetes-fabric8-discovery/src/test/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtilsTests.java b/spring-cloud-kubernetes-fabric8-discovery/src/test/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtilsTests.java index d586e400..e80ebf7f 100644 --- a/spring-cloud-kubernetes-fabric8-discovery/src/test/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtilsTests.java +++ b/spring-cloud-kubernetes-fabric8-discovery/src/test/java/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtilsTests.java @@ -17,17 +17,32 @@ package org.springframework.cloud.kubernetes.fabric8.discovery; import java.util.List; +import java.util.Map; +import java.util.Set; +import io.fabric8.kubernetes.api.model.EndpointPortBuilder; import io.fabric8.kubernetes.api.model.EndpointSubset; +import io.fabric8.kubernetes.api.model.EndpointSubsetBuilder; import io.fabric8.kubernetes.api.model.Endpoints; import io.fabric8.kubernetes.api.model.EndpointsBuilder; +import io.fabric8.kubernetes.api.model.ObjectMeta; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.api.model.Service; +import io.fabric8.kubernetes.api.model.ServiceBuilder; import org.junit.jupiter.api.Assertions; 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.kubernetes.commons.discovery.KubernetesDiscoveryProperties; + +import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.PRIMARY_PORT_NAME_LABEL_KEY; /** * @author wind57 */ +@ExtendWith(OutputCaptureExtension.class) class KubernetesDiscoveryClientUtilsTests { @Test @@ -58,4 +73,255 @@ class KubernetesDiscoveryClientUtilsTests { Assertions.assertEquals(result.namespace(), "default"); } + /** + *
+	 *     - properties do not have primary-port-name set
+	 *     - service labels do not have primary-port-name set
+	 *
+	 *     As such null is returned.
+	 * 
+ */ + @Test + void testPrimaryPortNameNotFound(CapturedOutput output) { + KubernetesDiscoveryProperties properties = KubernetesDiscoveryProperties.DEFAULT; + Service service = new ServiceBuilder().withMetadata(new ObjectMeta()).build(); + + String result = KubernetesDiscoveryClientUtils.primaryPortName(properties, service, "abc"); + Assertions.assertNull(result); + Assertions.assertTrue(output.getOut().contains( + "did not find a primary-port-name in neither properties nor service labels for service with ID : abc")); + } + + /** + *
+	 *     - properties do have primary-port-name set to "https"
+	 *     - service labels do not have primary-port-name set
+	 *
+	 *     As such "https" is returned.
+	 * 
+ */ + @Test + void testPrimaryPortNameFoundInProperties(CapturedOutput output) { + String primaryPortName = "https"; + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L, + true, "", Set.of(), Map.of(), primaryPortName, null, 0, false); + Service service = new ServiceBuilder().withMetadata(new ObjectMeta()).build(); + + String result = KubernetesDiscoveryClientUtils.primaryPortName(properties, service, "abc"); + Assertions.assertNotNull(result); + Assertions.assertEquals(result, primaryPortName); + Assertions.assertTrue(output.getOut().contains("will use primaryPortName : https for service with ID = abc")); + } + + /** + *
+	 *     - properties do not have primary-port-name set
+	 *     - service labels do have primary-port-name set to "https"
+	 *
+	 *     As such "https" is returned.
+	 * 
+ */ + @Test + void testPrimaryPortNameFoundInLabels(CapturedOutput output) { + Map labels = Map.of(PRIMARY_PORT_NAME_LABEL_KEY, "https"); + KubernetesDiscoveryProperties properties = KubernetesDiscoveryProperties.DEFAULT; + Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder().withLabels(labels).build()).build(); + + String result = KubernetesDiscoveryClientUtils.primaryPortName(properties, service, "abc"); + Assertions.assertNotNull(result); + Assertions.assertEquals(result, "https"); + Assertions.assertTrue(output.getOut().contains("will use primaryPortName : https for service with ID = abc")); + } + + /** + *
+	 *     - properties do have primary-port-name set to "https"
+	 *     - service labels do have primary-port-name set to "http"
+	 *
+	 *     As such "http" is returned (labels win).
+	 * 
+ */ + @Test + void testPrimaryPortNameFoundInBothPropertiesAndLabels(CapturedOutput output) { + String primaryPortName = "https"; + Map labels = Map.of(PRIMARY_PORT_NAME_LABEL_KEY, "http"); + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L, + true, "", Set.of(), Map.of(), primaryPortName, null, 0, false); + Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder().withLabels(labels).build()).build(); + + String result = KubernetesDiscoveryClientUtils.primaryPortName(properties, service, "abc"); + Assertions.assertNotNull(result); + Assertions.assertEquals(result, "http"); + Assertions.assertTrue(output.getOut().contains("will use primaryPortName : http for service with ID = abc")); + } + + /** + *
+	 *     - EndpointSubset has a single entry in getPorts.
+	 * 
+ */ + @Test + void testEndpointsPortSinglePort(CapturedOutput output) { + EndpointSubset endpointSubset = new EndpointSubsetBuilder() + .withPorts(new EndpointPortBuilder().withPort(8080).build()).build(); + String serviceId = "spring-k8s"; + KubernetesDiscoveryProperties properties = KubernetesDiscoveryProperties.DEFAULT; + Service service = new ServiceBuilder().build(); + + Integer port = KubernetesDiscoveryClientUtils.endpointsPort(endpointSubset, serviceId, properties, service); + Assertions.assertEquals(port, 8080); + Assertions.assertTrue(output.getOut().contains("endpoint ports has a single entry, using port : 8080")); + } + + /** + *
+	 *     - primary-port-name is null.
+	 * 
+ */ + @Test + void testEndpointsPortNullPrimaryPortName(CapturedOutput output) { + EndpointSubset endpointSubset = new EndpointSubsetBuilder() + .withPorts(new EndpointPortBuilder().withPort(8080).build(), + new EndpointPortBuilder().withPort(8081).build()) + .build(); + String serviceId = "spring-k8s"; + KubernetesDiscoveryProperties properties = KubernetesDiscoveryProperties.DEFAULT; + Service service = new ServiceBuilder().withMetadata(new ObjectMeta()).build(); + + Integer port = KubernetesDiscoveryClientUtils.endpointsPort(endpointSubset, serviceId, properties, service); + Assertions.assertEquals(port, 8080); + Assertions.assertTrue(output.getOut().contains( + "did not find a primary-port-name in neither properties nor service labels for service with ID : spring-k8s")); + Assertions.assertTrue(output.getOut() + .contains("not found primary-port-name (with value: 'null') via properties or service labels")); + Assertions.assertTrue(output.getOut().contains("not found primary-port-name via 'https' to match port")); + Assertions.assertTrue(output.getOut().contains("not found primary-port-name via 'http' to match port")); + Assertions.assertTrue(output.getOut().contains(""" + Make sure that either the primary-port-name label has been added to the service, + or spring.cloud.kubernetes.discovery.primary-port-name has been configured. + Alternatively name the primary port 'https' or 'http' + An incorrect configuration may result in non-deterministic behaviour.""")); + } + + /** + *
+	 *     - primary-port-name is "three", such a port name does not exist.
+	 * 
+ */ + @Test + void testEndpointsPortPrimaryPortNameIsPresentButNotFound(CapturedOutput output) { + EndpointSubset endpointSubset = new EndpointSubsetBuilder() + .withPorts(new EndpointPortBuilder().withPort(8080).withName("one").build(), + new EndpointPortBuilder().withPort(8081).withName("two").build()) + .build(); + String serviceId = "spring-k8s"; + + String primaryPortName = "three"; + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L, + true, "", Set.of(), Map.of(), primaryPortName, null, 0, false); + + Service service = new ServiceBuilder().withMetadata(new ObjectMeta()).build(); + + Integer port = KubernetesDiscoveryClientUtils.endpointsPort(endpointSubset, serviceId, properties, service); + Assertions.assertEquals(port, 8080); + Assertions.assertTrue( + output.getOut().contains("will use primaryPortName : three for service with ID = spring-k8s")); + Assertions.assertTrue(output.getOut() + .contains("not found primary-port-name (with value: 'three') via properties or service labels")); + Assertions.assertTrue(output.getOut().contains("not found primary-port-name via 'https' to match port")); + Assertions.assertTrue(output.getOut().contains("not found primary-port-name via 'http' to match port")); + Assertions.assertTrue(output.getOut().contains(""" + Make sure that either the primary-port-name label has been added to the service, + or spring.cloud.kubernetes.discovery.primary-port-name has been configured. + Alternatively name the primary port 'https' or 'http' + An incorrect configuration may result in non-deterministic behaviour.""")); + } + + /** + *
+	 *     - primary-port-name is "two", such a port name exists and matches 8081
+	 * 
+ */ + @Test + void testEndpointsPortPrimaryPortNameFound(CapturedOutput output) { + EndpointSubset endpointSubset = new EndpointSubsetBuilder() + .withPorts(new EndpointPortBuilder().withPort(8080).withName("one").build(), + new EndpointPortBuilder().withPort(8081).withName("two").build()) + .build(); + String serviceId = "spring-k8s"; + + String primaryPortName = "two"; + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L, + true, "", Set.of(), Map.of(), primaryPortName, null, 0, false); + + Service service = new ServiceBuilder().withMetadata(new ObjectMeta()).build(); + + Integer port = KubernetesDiscoveryClientUtils.endpointsPort(endpointSubset, serviceId, properties, service); + Assertions.assertEquals(port, 8081); + Assertions.assertTrue( + output.getOut().contains("will use primaryPortName : two for service with ID = spring-k8s")); + Assertions.assertTrue(output.getOut().contains( + "found primary-port-name (with value: 'two') via properties or service labels to match port : 8081")); + } + + /** + *
+	 *     - primary-port-name is "three", such a port name does not exist.
+	 *     - https port exists and this one is returned
+	 * 
+ */ + @Test + void testEndpointsPortPrimaryPortHttps(CapturedOutput output) { + EndpointSubset endpointSubset = new EndpointSubsetBuilder() + .withPorts(new EndpointPortBuilder().withPort(8080).withName("one").build(), + new EndpointPortBuilder().withPort(8081).withName("two").build(), + new EndpointPortBuilder().withPort(8082).withName("https").build()) + .build(); + String serviceId = "spring-k8s"; + + String primaryPortName = "three"; + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L, + true, "", Set.of(), Map.of(), primaryPortName, null, 0, false); + + Service service = new ServiceBuilder().withMetadata(new ObjectMeta()).build(); + + Integer port = KubernetesDiscoveryClientUtils.endpointsPort(endpointSubset, serviceId, properties, service); + Assertions.assertEquals(port, 8082); + Assertions.assertTrue( + output.getOut().contains("will use primaryPortName : three for service with ID = spring-k8s")); + Assertions.assertTrue(output.getOut().contains( + "not found primary-port-name (with value: 'three') via properties or service labels to match port")); + Assertions.assertTrue(output.getOut().contains("found primary-port-name via 'https' to match port : 8082")); + } + + /** + *
+	 *     - primary-port-name is "three", such a port name does not exist.
+	 *     - http port exists and this one is returned
+	 * 
+ */ + @Test + void testEndpointsPortPrimaryPortHttp(CapturedOutput output) { + EndpointSubset endpointSubset = new EndpointSubsetBuilder() + .withPorts(new EndpointPortBuilder().withPort(8080).withName("one").build(), + new EndpointPortBuilder().withPort(8081).withName("two").build(), + new EndpointPortBuilder().withPort(8082).withName("http").build()) + .build(); + String serviceId = "spring-k8s"; + + String primaryPortName = "three"; + KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L, + true, "", Set.of(), Map.of(), primaryPortName, null, 0, false); + + Service service = new ServiceBuilder().withMetadata(new ObjectMeta()).build(); + + Integer port = KubernetesDiscoveryClientUtils.endpointsPort(endpointSubset, serviceId, properties, service); + Assertions.assertEquals(port, 8082); + Assertions.assertTrue( + output.getOut().contains("will use primaryPortName : three for service with ID = spring-k8s")); + Assertions.assertTrue(output.getOut().contains( + "not found primary-port-name (with value: 'three') via properties or service labels to match port")); + Assertions.assertTrue(output.getOut().contains("found primary-port-name via 'http' to match port : 8082")); + } + }