Loadbalancer minor refactoring + tests (#1553)

spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/loadbalancer/KubernetesServiceInstanceMapper.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2013-2020 the original author or authors.
+ * Copyright 2013-2023 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.
@@ -16,52 +16,69 @@
 
 package org.springframework.cloud.kubernetes.commons.loadbalancer;
 
-import java.util.HashMap;
 import java.util.Map;
+import java.util.StringJoiner;
+
+import org.apache.commons.logging.LogFactory;
 
 import org.springframework.cloud.kubernetes.commons.discovery.KubernetesServiceInstance;
+import org.springframework.core.log.LogAccessor;
 import org.springframework.util.StringUtils;
 
+import static org.springframework.cloud.kubernetes.commons.config.ConfigUtils.keysWithPrefix;
+
 /**
  * @author Ryan Baxter
  */
 public interface KubernetesServiceInstanceMapper<T> {
 
+	/**
+	 * Logger instance.
+	 */
+	LogAccessor LOG = new LogAccessor(LogFactory.getLog(KubernetesServiceInstanceMapper.class));
+
 	KubernetesServiceInstance map(T service);
 
 	static String createHost(String serviceName, String namespace, String clusterDomain) {
-		return String.format("%s.%s.svc.%s", serviceName, StringUtils.hasText(namespace) ? namespace : "default",
+		String namespaceToUse = StringUtils.hasText(namespace) ? namespace : "default";
-				clusterDomain);
+		return new StringJoiner(".").add(serviceName).add(namespaceToUse).add("svc").add(clusterDomain).toString();
 	}
 
 	static boolean isSecure(Map<String, String> labels, Map<String, String> annotations, String servicePortName,
 			Integer servicePort) {
-		if (labels != null) {
+
-			final String securedLabelValue = labels.getOrDefault("secured", "false");
+		if (hasTrueSecuredValue(labels)) {
-			if (securedLabelValue.equals("true")) {
+			LOG.debug(() -> "Service has a true 'secured' label");
-				return true;
+			return true;
-			}
 		}
 
-		if (annotations != null) {
+		if (hasTrueSecuredValue(annotations)) {
-			final String securedAnnotationValue = annotations.getOrDefault("secured", "false");
+			LOG.debug(() -> "Service has a true 'secured' annotation");
-			if (securedAnnotationValue.equals("true")) {
+			return true;
-				return true;
-			}
 		}
-		return (servicePortName != null && servicePortName.endsWith("https")) || servicePort.toString().endsWith("443");
+
+		if (servicePortName != null && servicePortName.endsWith("https")) {
+			LOG.debug(() -> "Service port name ends with 'https'");
+			return true;
+		}
+
+		if (servicePort != null && servicePort.toString().endsWith("443")) {
+			LOG.debug(() -> "Service port ends with '443'");
+			return true;
+		}
+
+		return false;
 	}
 
 	static Map<String, String> getMapWithPrefixedKeys(Map<String, String> map, String prefix) {
-		if (map == null) {
+		return keysWithPrefix(map, prefix);
-			return new HashMap<>();
+	}
+
+	private static boolean hasTrueSecuredValue(Map<String, String> input) {
+		if (input != null) {
+			return "true".equals(input.get("secured"));
 		}
-		if (!StringUtils.hasText(prefix)) {
+		return false;
-			return map;
-		}
-		final Map<String, String> result = new HashMap<>();
-		map.forEach((k, v) -> result.put(prefix + k, v));
-		return result;
 	}
 
 }

spring-cloud-kubernetes-commons/src/test/java/org/springframework/cloud/kubernetes/commons/loadbalancer/KubernetesServiceInstanceMapperTests.java

@@ -0,0 +1,194 @@
+/*
+ * Copyright 2013-2020 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.commons.loadbalancer;
+
+import java.util.Map;
+
+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 static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * @author wind57
+ */
+@ExtendWith(OutputCaptureExtension.class)
+class KubernetesServiceInstanceMapperTests {
+
+	private static final String SECURED_LABEL_MESSAGE = "Service has a true 'secured' label";
+
+	private static final String SECURED_ANNOTATION_MESSAGE = "Service has a true 'secured' annotation";
+
+	private static final String NAME_ENDS_IN_HTTPS_MESSAGE = "Service port name ends with 'https'";
+
+	private static final String PORT_ENDS_IN_443_MESSAGE = "Service port ends with '443'";
+
+	@Test
+	void testCreateHostWithNamespace() {
+		String namespace = "customNamespace";
+		String host = KubernetesServiceInstanceMapper.createHost("serviceName", namespace, "clusterDomain");
+		assertThat(host).isEqualTo("serviceName.customNamespace.svc.clusterDomain");
+	}
+
+	@Test
+	void testCreateHostWithEmptyNamespace() {
+		String host = KubernetesServiceInstanceMapper.createHost("serviceName", "", "clusterDomain");
+		assertThat(host).isEqualTo("serviceName.default.svc.clusterDomain");
+	}
+
+	@Test
+	void testCreateHostWithNullNamespace() {
+		String host = KubernetesServiceInstanceMapper.createHost("serviceName", null, "clusterDomain");
+		assertThat(host).isEqualTo("serviceName.default.svc.clusterDomain");
+	}
+
+	@Test
+	void testIsSecureWithTrueLabel(CapturedOutput output) {
+		Map<String, String> labels = Map.of("secured", "true");
+		Map<String, String> annotations = Map.of();
+		String servicePortName = null;
+		Integer servicePort = null;
+		assertThat(KubernetesServiceInstanceMapper.isSecure(labels, annotations, servicePortName, servicePort))
+				.isTrue();
+
+		assertThat(output.getOut().contains(SECURED_LABEL_MESSAGE)).isTrue();
+		assertThat(output.getOut().contains(SECURED_ANNOTATION_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(NAME_ENDS_IN_HTTPS_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(PORT_ENDS_IN_443_MESSAGE)).isFalse();
+	}
+
+	@Test
+	void testIsSecureWithTrueAnnotation(CapturedOutput output) {
+		// empty labels
+		Map<String, String> labels = Map.of();
+		Map<String, String> annotations = Map.of("secured", "true");
+		String servicePortName = null;
+		Integer servicePort = null;
+		assertThat(KubernetesServiceInstanceMapper.isSecure(labels, annotations, servicePortName, servicePort))
+				.isTrue();
+
+		assertThat(output.getOut().contains(SECURED_LABEL_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(SECURED_ANNOTATION_MESSAGE)).isTrue();
+		assertThat(output.getOut().contains(NAME_ENDS_IN_HTTPS_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(PORT_ENDS_IN_443_MESSAGE)).isFalse();
+	}
+
+	@Test
+	void testIsSecureWithTrueAnnotationNullLabels(CapturedOutput output) {
+		// null labels
+		Map<String, String> labels = null;
+		Map<String, String> annotations = Map.of("secured", "true");
+		String servicePortName = null;
+		Integer servicePort = null;
+		assertThat(KubernetesServiceInstanceMapper.isSecure(labels, annotations, servicePortName, servicePort))
+				.isTrue();
+
+		assertThat(output.getOut().contains(SECURED_LABEL_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(SECURED_ANNOTATION_MESSAGE)).isTrue();
+		assertThat(output.getOut().contains(NAME_ENDS_IN_HTTPS_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(PORT_ENDS_IN_443_MESSAGE)).isFalse();
+	}
+
+	@Test
+	void testIsNotSecureServicePortNameAndServicePortAreNull(CapturedOutput output) {
+		// null labels
+		Map<String, String> labels = null;
+		// null annotations
+		Map<String, String> annotations = null;
+		String servicePortName = null;
+		Integer servicePort = null;
+		assertThat(KubernetesServiceInstanceMapper.isSecure(labels, annotations, servicePortName, servicePort))
+				.isFalse();
+
+		assertThat(output.getOut().contains(SECURED_LABEL_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(SECURED_ANNOTATION_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(NAME_ENDS_IN_HTTPS_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(PORT_ENDS_IN_443_MESSAGE)).isFalse();
+	}
+
+	@Test
+	void testIsNotSecureServicePortNameDoesNotMatch(CapturedOutput output) {
+		// null labels
+		Map<String, String> labels = null;
+		// null annotations
+		Map<String, String> annotations = null;
+		String servicePortName = "abc_https_def";
+		Integer servicePort = null;
+		assertThat(KubernetesServiceInstanceMapper.isSecure(labels, annotations, servicePortName, servicePort))
+				.isFalse();
+
+		assertThat(output.getOut().contains(SECURED_LABEL_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(SECURED_ANNOTATION_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(NAME_ENDS_IN_HTTPS_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(PORT_ENDS_IN_443_MESSAGE)).isFalse();
+	}
+
+	@Test
+	void testIsSecureServicePortNameMatches(CapturedOutput output) {
+		// null labels
+		Map<String, String> labels = null;
+		// null annotations
+		Map<String, String> annotations = null;
+		String servicePortName = "abc_https";
+		Integer servicePort = null;
+		assertThat(KubernetesServiceInstanceMapper.isSecure(labels, annotations, servicePortName, servicePort))
+				.isTrue();
+
+		assertThat(output.getOut().contains(SECURED_LABEL_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(SECURED_ANNOTATION_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(NAME_ENDS_IN_HTTPS_MESSAGE)).isTrue();
+		assertThat(output.getOut().contains(PORT_ENDS_IN_443_MESSAGE)).isFalse();
+	}
+
+	@Test
+	void testIsNotSecureServicePortDoesNotMatch(CapturedOutput output) {
+		// null labels
+		Map<String, String> labels = null;
+		// null annotations
+		Map<String, String> annotations = null;
+		String servicePortName = null;
+		Integer servicePort = 444;
+		assertThat(KubernetesServiceInstanceMapper.isSecure(labels, annotations, servicePortName, servicePort))
+				.isFalse();
+
+		assertThat(output.getOut().contains(SECURED_LABEL_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(SECURED_ANNOTATION_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(NAME_ENDS_IN_HTTPS_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(PORT_ENDS_IN_443_MESSAGE)).isFalse();
+	}
+
+	@Test
+	void testIsSecureServicePortMatches(CapturedOutput output) {
+		// null labels
+		Map<String, String> labels = null;
+		// null annotations
+		Map<String, String> annotations = null;
+		String servicePortName = null;
+		Integer servicePort = 443;
+		assertThat(KubernetesServiceInstanceMapper.isSecure(labels, annotations, servicePortName, servicePort))
+				.isTrue();
+
+		assertThat(output.getOut().contains(SECURED_LABEL_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(SECURED_ANNOTATION_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(NAME_ENDS_IN_HTTPS_MESSAGE)).isFalse();
+		assertThat(output.getOut().contains(PORT_ENDS_IN_443_MESSAGE)).isTrue();
+	}
+
+}

spring-cloud-kubernetes-commons/src/test/resources/logback-test.xml

@@ -14,5 +14,6 @@
 
 	<!-- needed for CapturedOutput -->
 	<logger name="org.springframework.cloud.kubernetes.commons.discovery" level="DEBUG"/>
+	<logger name="org.springframework.cloud.kubernetes.commons.loadbalancer" level="DEBUG"/>
 
 </configuration>