From b41dcc146e0ef4171d35c060c8247bb8a0675e2a Mon Sep 17 00:00:00 2001 From: erabii Date: Wed, 10 Nov 2021 15:22:25 -0500 Subject: [PATCH] namespace resolution must work the same across callers (#906) --- ...bernetesClientConfigMapPropertySource.java | 10 +- .../config/KubernetesClientConfigUtils.java | 2 +- ...KubernetesClientSecretsPropertySource.java | 50 +++------- ...tesClientSecretsPropertySourceLocator.java | 4 +- ...tesClientConfigMapPropertySourceTests.java | 25 +++++ ...netesClientSecretsPropertySourceTests.java | 51 +++++----- ...bernetesConfigServerAutoConfiguration.java | 2 +- .../KubernetesEnvironmentRepositoryTests.java | 2 +- .../Fabric8ConfigMapPropertySource.java | 15 ++- .../fabric8/config/Fabric8ConfigUtils.java | 21 +++-- .../config/Fabric8SecretsPropertySource.java | 25 ++--- .../Fabric8ConfigMapPropertySourceTests.java | 77 ++++++++++++++- ...Fabric8SecretsPropertySourceMockTests.java | 94 +++++++++++++++++++ .../Fabric8SecretsPropertySourceTest.java | 45 +-------- ...BasedConfigurationChangeDetectorTests.java | 7 +- 15 files changed, 285 insertions(+), 145 deletions(-) create mode 100644 spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySourceMockTests.java diff --git a/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySource.java b/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySource.java index 0fdd0513..3d2f7f06 100644 --- a/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySource.java +++ b/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySource.java @@ -32,6 +32,8 @@ import org.springframework.cloud.kubernetes.commons.config.ConfigMapPropertySour import org.springframework.core.env.Environment; import org.springframework.util.CollectionUtils; +import static org.springframework.cloud.kubernetes.client.config.KubernetesClientConfigUtils.getApplicationNamespace; + /** * @author Ryan Baxter * @author Isik Erhan @@ -43,13 +45,15 @@ public class KubernetesClientConfigMapPropertySource extends ConfigMapPropertySo @Deprecated public KubernetesClientConfigMapPropertySource(CoreV1Api coreV1Api, String name, String namespace, Environment environment) { - super(getName(name, namespace), getData(coreV1Api, name, namespace, environment, "", true, false)); + super(getName(name, getApplicationNamespace(namespace, "Config Map", null)), getData(coreV1Api, name, + getApplicationNamespace(namespace, "Config Map", null), environment, "", true, false)); } public KubernetesClientConfigMapPropertySource(CoreV1Api coreV1Api, String name, String namespace, Environment environment, String prefix, boolean includeProfileSpecificSources, boolean failFast) { - super(getName(name, namespace), - getData(coreV1Api, name, namespace, environment, prefix, includeProfileSpecificSources, failFast)); + super(getName(name, getApplicationNamespace(namespace, "Config Map", null)), + getData(coreV1Api, name, getApplicationNamespace(namespace, "Config Map", null), environment, prefix, + includeProfileSpecificSources, failFast)); } private static Map getData(CoreV1Api coreV1Api, String name, String namespace, diff --git a/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigUtils.java b/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigUtils.java index 56ff6cea..1b952aee 100644 --- a/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigUtils.java +++ b/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigUtils.java @@ -95,7 +95,7 @@ public final class KubernetesClientConfigUtils { static String getApplicationNamespace(String namespace, String configurationTarget, KubernetesNamespaceProvider provider) { if (StringUtils.hasText(namespace)) { - LOG.debug(configurationTarget + " namespace from normalized source : " + namespace); + LOG.debug(configurationTarget + " namespace from normalized source or passed directly : " + namespace); return namespace; } diff --git a/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySource.java b/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySource.java index 7e17add9..d60e661b 100644 --- a/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySource.java +++ b/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySource.java @@ -20,6 +20,7 @@ import java.util.Base64; import java.util.HashMap; import java.util.Map; import java.util.Optional; +import java.util.stream.Collectors; import io.kubernetes.client.openapi.apis.CoreV1Api; import io.kubernetes.client.openapi.models.V1Secret; @@ -27,9 +28,10 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.cloud.kubernetes.commons.config.SecretsPropertySource; -import org.springframework.core.env.Environment; import org.springframework.util.StringUtils; +import static org.springframework.cloud.kubernetes.client.config.KubernetesClientConfigUtils.getApplicationNamespace; + /** * @author Ryan Baxter * @author Isik Erhan @@ -38,49 +40,32 @@ public class KubernetesClientSecretsPropertySource extends SecretsPropertySource private static final Log LOG = LogFactory.getLog(KubernetesClientSecretsPropertySource.class); - private CoreV1Api coreV1Api; - public KubernetesClientSecretsPropertySource(CoreV1Api coreV1Api, String name, String namespace, - Environment environment, Map labels, boolean failFast) { - super(getSourceName(name, namespace), getSourceData(coreV1Api, environment, name, namespace, labels, failFast)); + Map labels, boolean failFast) { + super(getSourceName(name, getApplicationNamespace(namespace, "Secret", null)), + getSourceData(coreV1Api, name, getApplicationNamespace(namespace, "Secret", null), labels, failFast)); } - private static Map getSourceData(CoreV1Api api, Environment env, String name, String namespace, + private static Map getSourceData(CoreV1Api api, String name, String namespace, Map labels, boolean failFast) { Map result = new HashMap<>(); LOG.info("Loading Secret with name '" + name + "' or with labels [" + labels + "] in namespace '" + namespace + "'"); try { - // Read for secrets api (named) if (StringUtils.hasText(name)) { Optional secret; - if (!StringUtils.hasText(namespace)) { - - // There could technically be more than one, just return the first - secret = api.listSecretForAllNamespaces(null, null, null, null, null, null, null, null, null, null) - .getItems().stream().filter(s -> name.equals(s.getMetadata().getName())).findFirst(); - } - else { - secret = api - .listNamespacedSecret(namespace, null, null, null, null, null, null, null, null, null, null) - .getItems().stream().filter(s -> name.equals(s.getMetadata().getName())).findFirst(); - } + secret = api.listNamespacedSecret(namespace, null, null, null, null, null, null, null, null, null, null) + .getItems().stream().filter(s -> name.equals(s.getMetadata().getName())).findFirst(); secret.ifPresent(s -> putAll(s, result)); } // Read for secrets api (label) if (labels != null && !labels.isEmpty()) { - if (!StringUtils.hasText(namespace)) { - api.listSecretForAllNamespaces(null, null, null, createLabelsSelector(labels), null, null, null, - null, null, null).getItems().forEach(s -> putAll(s, result)); - } - else { - api.listNamespacedSecret(namespace, null, null, null, null, createLabelsSelector(labels), null, - null, null, null, null).getItems().forEach(s -> putAll(s, result)); - } + api.listNamespacedSecret(namespace, null, null, null, null, createLabelsSelector(labels), null, null, + null, null, null).getItems().forEach(s -> putAll(s, result)); } } catch (Exception e) { @@ -97,20 +82,13 @@ public class KubernetesClientSecretsPropertySource extends SecretsPropertySource } private static String createLabelsSelector(Map labels) { - StringBuilder selectorString = new StringBuilder(); - for (String key : labels.keySet()) { - if (selectorString.length() != 0) { - selectorString.append(","); - } - selectorString.append(key + "=" + labels.get(key)); - } - return selectorString.toString(); + return labels.entrySet().stream().map(e -> e.getKey() + "=" + e.getValue()).collect(Collectors.joining(",")); } private static void putAll(V1Secret secret, Map result) { Map secretData = new HashMap<>(); - secret.getData().forEach((key, value) -> secretData.put(key, Base64.getEncoder().encodeToString(value))); - if (secret != null) { + if (secret.getData() != null) { + secret.getData().forEach((key, value) -> secretData.put(key, Base64.getEncoder().encodeToString(value))); putAll(secretData, result); } } diff --git a/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceLocator.java b/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceLocator.java index 333730c6..5b3c17c8 100644 --- a/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceLocator.java +++ b/spring-cloud-kubernetes-client-config/src/main/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceLocator.java @@ -90,8 +90,8 @@ public class KubernetesClientSecretsPropertySourceLocator extends SecretsPropert kubernetesNamespaceProvider); } - return new KubernetesClientSecretsPropertySource(coreV1Api, secretName, namespace, environment, - normalizedSource.getLabels(), this.properties.isFailFast()); + return new KubernetesClientSecretsPropertySource(coreV1Api, secretName, namespace, normalizedSource.getLabels(), + this.properties.isFailFast()); } } diff --git a/spring-cloud-kubernetes-client-config/src/test/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySourceTests.java b/spring-cloud-kubernetes-client-config/src/test/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySourceTests.java index 1bbd563e..404203a5 100644 --- a/spring-cloud-kubernetes-client-config/src/test/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySourceTests.java +++ b/spring-cloud-kubernetes-client-config/src/test/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySourceTests.java @@ -31,6 +31,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.springframework.cloud.kubernetes.commons.config.NamespaceResolutionFailedException; import org.springframework.mock.env.MockEnvironment; import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; @@ -144,6 +145,30 @@ class KubernetesClientConfigMapPropertySourceTests { .isEqualTo("TRACE"); } + @Test + void deprecatedConstructorWithoutNamespaceMustFail() { + assertThatThrownBy(() -> new KubernetesClientConfigMapPropertySource(new CoreV1Api(), "configmap", null, + new MockEnvironment())).isInstanceOf(NamespaceResolutionFailedException.class); + } + + @Test + void constructorWithoutNamespaceMustFail() { + assertThatThrownBy(() -> new KubernetesClientConfigMapPropertySource(new CoreV1Api(), "configmap", null, + new MockEnvironment(), "", false, false)).isInstanceOf(NamespaceResolutionFailedException.class); + } + + @Test + void deprecatedConstructorWithNamespaceMustNotFail() { + assertThat(new KubernetesClientConfigMapPropertySource(new CoreV1Api(), "configmap", "namespace", + new MockEnvironment())).isNotNull(); + } + + @Test + void constructorWithNamespaceMustNotFail() { + assertThat(new KubernetesClientConfigMapPropertySource(new CoreV1Api(), "configmap", "namespace", + new MockEnvironment(), "", false, false)).isNotNull(); + } + @Test public void constructorShouldThrowExceptionOnFailureWhenFailFastIsEnabled() { CoreV1Api api = new CoreV1Api(); diff --git a/spring-cloud-kubernetes-client-config/src/test/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceTests.java b/spring-cloud-kubernetes-client-config/src/test/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceTests.java index cea11b48..c426c4e0 100644 --- a/spring-cloud-kubernetes-client-config/src/test/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceTests.java +++ b/spring-cloud-kubernetes-client-config/src/test/java/org/springframework/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceTests.java @@ -35,7 +35,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; -import org.springframework.mock.env.MockEnvironment; +import org.springframework.cloud.kubernetes.commons.config.NamespaceResolutionFailedException; import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; import static com.github.tomakehurst.wiremock.client.WireMock.get; @@ -63,7 +63,7 @@ class KubernetesClientSecretsPropertySourceTests { private static final String LIST_API = "/api/v1/secrets"; - private static final String LIST_API_WITH_LABEL = "/api/v1/secrets?labelSelector=spring.cloud.kubernetes.secret%3Dtrue"; + private static final String LIST_API_WITH_LABEL = "/api/v1/namespaces/default/secrets?labelSelector=spring.cloud.kubernetes.secret%3Dtrue"; private static final String LIST_BODY = "{\n" + "\t\"kind\": \"SecretList\",\n" + "\t\"apiVersion\": \"v1\",\n" + "\t\"metadata\": {\n" + "\t\t\"selfLink\": \"/api/v1/secrets\",\n" @@ -91,7 +91,7 @@ class KubernetesClientSecretsPropertySourceTests { private static WireMockServer wireMockServer; @BeforeAll - public static void setup() { + static void setup() { wireMockServer = new WireMockServer(options().dynamicPort()); wireMockServer.start(); @@ -103,21 +103,21 @@ class KubernetesClientSecretsPropertySourceTests { } @AfterAll - public static void after() { + static void after() { wireMockServer.stop(); } @AfterEach - public void afterEach() { + void afterEach() { WireMock.reset(); } @Test - public void secretsTest() { + void secretsTest() { CoreV1Api api = new CoreV1Api(); stubFor(get(API).willReturn(aResponse().withStatus(200).withBody(new JSON().serialize(SECRET_LIST)))); KubernetesClientSecretsPropertySource propertySource = new KubernetesClientSecretsPropertySource(api, - "db-secret", "default", new MockEnvironment(), new HashMap<>(), false); + "db-secret", "default", new HashMap<>(), false); assertThat(propertySource.containsProperty("password")).isTrue(); assertThat(propertySource.getProperty("password")).isEqualTo("p455w0rd"); assertThat(propertySource.containsProperty("username")).isTrue(); @@ -125,48 +125,43 @@ class KubernetesClientSecretsPropertySourceTests { } @Test - public void secretsNullNamespaceTest() { - CoreV1Api api = new CoreV1Api(); - stubFor(get(LIST_API).willReturn(aResponse().withStatus(200).withBody(LIST_BODY))); - KubernetesClientSecretsPropertySource propertySource = new KubernetesClientSecretsPropertySource(api, - "db-secret", null, new MockEnvironment(), new HashMap<>(), false); - assertThat(propertySource.containsProperty("password")).isTrue(); - assertThat(propertySource.getProperty("password")).isEqualTo("p455w0rd"); - assertThat(propertySource.containsProperty("username")).isTrue(); - assertThat(propertySource.getProperty("username")).isEqualTo("user"); - } - - @Test - public void secretLabelsTest() { + void secretLabelsTest() { CoreV1Api api = new CoreV1Api(); stubFor(get(LIST_API_WITH_LABEL).willReturn(aResponse().withStatus(200).withBody(LIST_BODY))); Map labels = new HashMap<>(); labels.put("spring.cloud.kubernetes.secret", "true"); KubernetesClientSecretsPropertySource propertySource = new KubernetesClientSecretsPropertySource(api, null, - null, new MockEnvironment(), labels, false); + "default", labels, false); assertThat(propertySource.containsProperty("spring.rabbitmq.password")).isTrue(); assertThat(propertySource.getProperty("spring.rabbitmq.password")).isEqualTo("password"); } @Test - public void constructorShouldThrowExceptionOnFailureWhenFailFastIsEnabled() { + void constructorShouldThrowExceptionOnFailureWhenFailFastIsEnabled() { CoreV1Api api = new CoreV1Api(); stubFor(get(API).willReturn(aResponse().withStatus(500).withBody("Internal Server Error"))); - assertThatThrownBy(() -> new KubernetesClientSecretsPropertySource(api, "db-secret", "default", - new MockEnvironment(), null, true)).isInstanceOf(IllegalStateException.class).hasMessage( - "Unable to read Secret with name 'db-secret' or labels [null] in namespace 'default'"); + assertThatThrownBy(() -> new KubernetesClientSecretsPropertySource(api, "db-secret", "default", null, true)) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Unable to read Secret with name 'db-secret' or labels [null] in namespace 'default'"); verify(getRequestedFor(urlEqualTo(API))); } @Test - public void constructorShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() { + void constructorShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() { CoreV1Api api = new CoreV1Api(); stubFor(get(API).willReturn(aResponse().withStatus(500).withBody("Internal Server Error"))); - assertThatNoException().isThrownBy((() -> new KubernetesClientSecretsPropertySource(api, "db-secret", "default", - new MockEnvironment(), null, false))); + assertThatNoException().isThrownBy( + (() -> new KubernetesClientSecretsPropertySource(api, "db-secret", "default", null, false))); verify(getRequestedFor(urlEqualTo(API))); } + @Test + void constructorMustFailWhenNamespaceIsNoProvided() { + CoreV1Api api = new CoreV1Api(); + assertThatThrownBy((() -> new KubernetesClientSecretsPropertySource(api, "db-secret", null, null, false))) + .isInstanceOf(NamespaceResolutionFailedException.class); + } + } diff --git a/spring-cloud-kubernetes-controllers/spring-cloud-kubernetes-configserver/src/main/java/org/springframework/cloud/kubernetes/configserver/KubernetesConfigServerAutoConfiguration.java b/spring-cloud-kubernetes-controllers/spring-cloud-kubernetes-configserver/src/main/java/org/springframework/cloud/kubernetes/configserver/KubernetesConfigServerAutoConfiguration.java index 09c1e610..f99fa7dc 100644 --- a/spring-cloud-kubernetes-controllers/spring-cloud-kubernetes-configserver/src/main/java/org/springframework/cloud/kubernetes/configserver/KubernetesConfigServerAutoConfiguration.java +++ b/spring-cloud-kubernetes-controllers/spring-cloud-kubernetes-configserver/src/main/java/org/springframework/cloud/kubernetes/configserver/KubernetesConfigServerAutoConfiguration.java @@ -83,7 +83,7 @@ public class KubernetesConfigServerAutoConfiguration { List namespaces = namespaceSplitter(properties.getSecretsNamespaces(), namespace); List propertySources = new ArrayList<>(); namespaces.forEach(space -> propertySources.add(new KubernetesClientSecretsPropertySource(coreApi, - applicationName, space, springEnv, new HashMap<>(), false))); + applicationName, space, new HashMap<>(), false))); return propertySources; }; } diff --git a/spring-cloud-kubernetes-controllers/spring-cloud-kubernetes-configserver/src/test/java/org/springframework/cloud/kubernetes/configserver/KubernetesEnvironmentRepositoryTests.java b/spring-cloud-kubernetes-controllers/spring-cloud-kubernetes-configserver/src/test/java/org/springframework/cloud/kubernetes/configserver/KubernetesEnvironmentRepositoryTests.java index 7feb0df3..78ce8ab9 100644 --- a/spring-cloud-kubernetes-controllers/spring-cloud-kubernetes-configserver/src/test/java/org/springframework/cloud/kubernetes/configserver/KubernetesEnvironmentRepositoryTests.java +++ b/spring-cloud-kubernetes-controllers/spring-cloud-kubernetes-configserver/src/test/java/org/springframework/cloud/kubernetes/configserver/KubernetesEnvironmentRepositoryTests.java @@ -105,7 +105,7 @@ class KubernetesEnvironmentRepositoryTests { kubernetesPropertySourceSuppliers.add((coreApi, applicationName, namespace, springEnv) -> { List propertySources = new ArrayList<>(); propertySources.add(new KubernetesClientSecretsPropertySource(coreApi, applicationName, "default", - springEnv, new HashMap<>(), false)); + new HashMap<>(), false)); return propertySources; }); } diff --git a/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigMapPropertySource.java b/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigMapPropertySource.java index f45b8894..4d22b1b3 100644 --- a/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigMapPropertySource.java +++ b/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigMapPropertySource.java @@ -44,6 +44,11 @@ public class Fabric8ConfigMapPropertySource extends ConfigMapPropertySource { private static final Log LOG = LogFactory.getLog(Fabric8ConfigMapPropertySource.class); + /** + * this constructor is present only for compatibility reasons, its usage is + * discouraged. + */ + @Deprecated public Fabric8ConfigMapPropertySource(KubernetesClient client, String name) { this(client, name, null, null, "", true, false); } @@ -55,15 +60,15 @@ public class Fabric8ConfigMapPropertySource extends ConfigMapPropertySource { @Deprecated public Fabric8ConfigMapPropertySource(KubernetesClient client, String name, String namespace, Environment environment) { - super(getName(name, getApplicationNamespace(client, namespace)), - getData(client, name, getApplicationNamespace(client, namespace), environment, "", true, false)); + super(getName(name, getApplicationNamespace(client, namespace, "Config Map", null)), getData(client, name, + getApplicationNamespace(client, namespace, "Config Map", null), environment, "", true, false)); } public Fabric8ConfigMapPropertySource(KubernetesClient client, String name, String namespace, Environment environment, String prefix, boolean includeProfileSpecificSources, boolean failFast) { - super(getName(name, getApplicationNamespace(client, namespace)), - getData(client, name, getApplicationNamespace(client, namespace), environment, prefix, - includeProfileSpecificSources, failFast)); + super(getName(name, getApplicationNamespace(client, namespace, "Config Map", null)), + getData(client, name, getApplicationNamespace(client, namespace, "Config Map", null), environment, + prefix, includeProfileSpecificSources, failFast)); } private static Map getData(KubernetesClient client, String name, String namespace, diff --git a/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigUtils.java b/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigUtils.java index db4fe693..d130517d 100644 --- a/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigUtils.java +++ b/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigUtils.java @@ -55,6 +55,14 @@ public final class Fabric8ConfigUtils { return namespace; } + /* + * this is not used, it is here for compatibility reasons only. + */ + @Deprecated + public static String getApplicationNamespace(KubernetesClient client, String namespace) { + return !StringUtils.hasLength(namespace) ? client.getNamespace() : namespace; + } + /** * this method does the namespace resolution for both config map and secrets * implementations. It tries these places to find the namespace: @@ -81,7 +89,7 @@ public final class Fabric8ConfigUtils { KubernetesNamespaceProvider provider) { if (StringUtils.hasText(namespace)) { - LOG.debug(configurationTarget + " namespace from normalized source : " + namespace); + LOG.debug(configurationTarget + " namespace from normalized source or passed directly : " + namespace); return namespace; } @@ -102,13 +110,12 @@ public final class Fabric8ConfigUtils { } - public static String getApplicationNamespace(KubernetesClient client, String namespace) { - return !StringUtils.hasLength(namespace) ? client.getNamespace() : namespace; - } - + /* + * namespace that reaches this point is absolutely present, otherwise this would have + * resulted in a NamespaceResolutionFailedException + */ static Map getConfigMapData(KubernetesClient client, String namespace, String name) { - ConfigMap configMap = !StringUtils.hasLength(namespace) ? client.configMaps().withName(name).get() - : client.configMaps().inNamespace(namespace).withName(name).get(); + ConfigMap configMap = client.configMaps().inNamespace(namespace).withName(name).get(); if (configMap == null) { LOG.warn("config-map with name : '" + name + "' not present in namespace : '" + namespace + "'"); diff --git a/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySource.java b/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySource.java index 3fcf06a8..4a18ea9b 100644 --- a/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySource.java +++ b/spring-cloud-kubernetes-fabric8-config/src/main/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySource.java @@ -25,7 +25,8 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.cloud.kubernetes.commons.config.SecretsPropertySource; -import org.springframework.util.StringUtils; + +import static org.springframework.cloud.kubernetes.fabric8.config.Fabric8ConfigUtils.getApplicationNamespace; /** * Kubernetes property source for secrets. @@ -40,40 +41,40 @@ public class Fabric8SecretsPropertySource extends SecretsPropertySource { public Fabric8SecretsPropertySource(KubernetesClient client, String name, String namespace, Map labels, boolean failFast) { - super(getSourceName(name, namespace), getSourceData(client, name, namespace, labels, failFast)); + super(getSourceName(name, getApplicationNamespace(client, namespace, "Secret", null)), getSourceData(client, + name, getApplicationNamespace(client, namespace, "Secret", null), labels, failFast)); } private static Map getSourceData(KubernetesClient client, String name, String namespace, Map labels, boolean failFast) { Map result = new HashMap<>(); - String namespaceToUse = StringUtils.hasLength(namespace) ? namespace : client.getNamespace(); - LOG.info("Loading Secret with name '" + name + "' or with labels [" + labels + "] in namespace '" - + namespaceToUse + "'"); + LOG.info("Loading Secret with name '" + name + "' or with labels [" + labels + "] in namespace '" + namespace + + "'"); try { - Secret secret = client.secrets().inNamespace(namespaceToUse).withName(name).get(); + Secret secret = client.secrets().inNamespace(namespace).withName(name).get(); // the API is documented that it might return null if (secret == null) { - LOG.warn("secret with name : " + name + " in namespace : " + namespaceToUse + " not found"); + LOG.warn("secret with name : " + name + " in namespace : " + namespace + " not found"); } else { - putDataFromSecret(secret, result, namespaceToUse); + putDataFromSecret(secret, result, namespace); } - client.secrets().inNamespace(namespaceToUse).withLabels(labels).list().getItems() - .forEach(s -> putDataFromSecret(s, result, namespaceToUse)); + client.secrets().inNamespace(namespace).withLabels(labels).list().getItems() + .forEach(s -> putDataFromSecret(s, result, namespace)); } catch (Exception e) { if (failFast) { throw new IllegalStateException("Unable to read Secret with name '" + name + "' or labels [" + labels - + "] in namespace '" + namespaceToUse + "'", e); + + "] in namespace '" + namespace + "'", e); } LOG.warn("Can't read secret with name: [" + name + "] or labels [" + labels + "] in namespace: [" - + namespaceToUse + "] (cause: " + e.getMessage() + "). Ignoring"); + + namespace + "] (cause: " + e.getMessage() + "). Ignoring"); } return result; diff --git a/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigMapPropertySourceTests.java b/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigMapPropertySourceTests.java index caeb7f78..62912972 100644 --- a/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigMapPropertySourceTests.java +++ b/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8ConfigMapPropertySourceTests.java @@ -16,13 +16,17 @@ package org.springframework.cloud.kubernetes.fabric8.config; +import io.fabric8.kubernetes.client.DefaultKubernetesClient; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient; import io.fabric8.kubernetes.client.server.mock.KubernetesMockServer; import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.springframework.cloud.kubernetes.commons.config.NamespaceResolutionFailedException; import org.springframework.mock.env.MockEnvironment; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatNoException; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -30,14 +34,16 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; * @author Isik Erhan */ @EnableKubernetesMockClient -public class Fabric8ConfigMapPropertySourceTests { +class Fabric8ConfigMapPropertySourceTests { - KubernetesMockServer mockServer; + private KubernetesMockServer mockServer; - KubernetesClient mockClient; + private KubernetesClient mockClient; + + private final DefaultKubernetesClient client = Mockito.mock(DefaultKubernetesClient.class); @Test - public void constructorShouldThrowExceptionOnFailureWhenFailFastIsEnabled() { + void constructorShouldThrowExceptionOnFailureWhenFailFastIsEnabled() { final String name = "my-config"; final String namespace = "default"; final String path = String.format("/api/v1/namespaces/%s/configmaps/%s", namespace, name); @@ -49,7 +55,7 @@ public class Fabric8ConfigMapPropertySourceTests { } @Test - public void constructorShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() { + void constructorShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() { final String name = "my-config"; final String namespace = "default"; final String path = String.format("/api/v1/namespaces/%s/configmaps/%s", namespace, name); @@ -59,4 +65,65 @@ public class Fabric8ConfigMapPropertySourceTests { new MockEnvironment(), "", false, false)); } + @Test + void deprecatedConstructorWithoutClientNamespaceMustFail() { + + Mockito.when(client.getNamespace()).thenReturn(null); + assertThatThrownBy(() -> new Fabric8ConfigMapPropertySource(client, "configmap")) + .isInstanceOf(NamespaceResolutionFailedException.class); + } + + @Test + void deprecatedConstructorWithClientNamespaceMustNotFail() { + + Mockito.when(client.getNamespace()).thenReturn("some"); + assertThat(new Fabric8ConfigMapPropertySource(client, "configmap")).isNotNull(); + } + + @Test + void anotherDeprecatedConstructorWithoutClientNamespaceMustFail() { + + Mockito.when(client.getNamespace()).thenReturn(null); + assertThatThrownBy(() -> new Fabric8ConfigMapPropertySource(client, "configmap", null, new MockEnvironment())) + .isInstanceOf(NamespaceResolutionFailedException.class); + } + + @Test + void anotherDeprecatedConstructorWithClientNamespaceMustNotFail() { + + Mockito.when(client.getNamespace()).thenReturn("some-namespace"); + assertThat(new Fabric8ConfigMapPropertySource(client, "configmap", null, new MockEnvironment())).isNotNull(); + } + + @Test + void anotherDeprecatedConstructorWithNamespaceMustNotFail() { + + Mockito.when(client.getNamespace()).thenReturn(null); + assertThat(new Fabric8ConfigMapPropertySource(client, "configmap", "namespace", new MockEnvironment())) + .isNotNull(); + } + + @Test + void constructorWithoutClientNamespaceMustFail() { + + Mockito.when(client.getNamespace()).thenReturn(null); + assertThatThrownBy(() -> new Fabric8ConfigMapPropertySource(client, "configmap", null, new MockEnvironment())) + .isInstanceOf(NamespaceResolutionFailedException.class); + } + + @Test + void constructorWithClientNamespaceMustNotFail() { + + Mockito.when(client.getNamespace()).thenReturn("namespace"); + assertThat(new Fabric8ConfigMapPropertySource(client, "configmap", null, new MockEnvironment())).isNotNull(); + } + + @Test + void constructorWithNamespaceMustNotFail() { + + Mockito.when(client.getNamespace()).thenReturn(null); + assertThat(new Fabric8ConfigMapPropertySource(client, "configmap", "namespace", new MockEnvironment())) + .isNotNull(); + } + } diff --git a/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySourceMockTests.java b/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySourceMockTests.java new file mode 100644 index 00000000..b6dc1458 --- /dev/null +++ b/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySourceMockTests.java @@ -0,0 +1,94 @@ +/* + * Copyright 2013-2021 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.fabric8.config; + +import java.util.Collections; + +import io.fabric8.kubernetes.client.DefaultKubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient; +import io.fabric8.kubernetes.client.server.mock.KubernetesMockServer; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +import org.springframework.cloud.kubernetes.commons.config.NamespaceResolutionFailedException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatNoException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * tests that are supposed to use EnableKubernetesMockClient only + * + * @author wind57 + */ +@EnableKubernetesMockClient +class Fabric8SecretsPropertySourceMockTests { + + private static KubernetesMockServer mockServer; + + private static KubernetesClient client; + + private final DefaultKubernetesClient mockClient = Mockito.mock(DefaultKubernetesClient.class); + + @Test + void constructorShouldThrowExceptionOnFailureWhenFailFastIsEnabled() { + final String name = "my-config"; + final String namespace = "default"; + final String path = String.format("/api/v1/namespaces/%s/secrets/%s", namespace, name); + + mockServer.expect().withPath(path).andReturn(500, "Internal Server Error").once(); + assertThatThrownBy( + () -> new Fabric8SecretsPropertySource(client, name, namespace, Collections.emptyMap(), true)) + .isInstanceOf(IllegalStateException.class).hasMessage("Unable to read Secret with name '" + name + + "' or labels [{}] in namespace '" + namespace + "'"); + } + + @Test + void constructorShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() { + final String name = "my-config"; + final String namespace = "default"; + final String path = String.format("/api/v1/namespaces/%s/secrets/%s", namespace, name); + + mockServer.expect().withPath(path).andReturn(500, "Internal Server Error").once(); + assertThatNoException().isThrownBy( + () -> new Fabric8SecretsPropertySource(client, name, namespace, Collections.emptyMap(), false)); + } + + @Test + void constructorWithoutClientNamespaceMustFail() { + Mockito.when(mockClient.getNamespace()).thenReturn(null); + assertThatThrownBy( + () -> new Fabric8SecretsPropertySource(mockClient, "my-secret", null, Collections.emptyMap(), false)) + .isInstanceOf(NamespaceResolutionFailedException.class); + } + + @Test + void constructorWithClientNamespaceMustNotFail() { + Mockito.when(mockClient.getNamespace()).thenReturn("namespace"); + assertThat(new Fabric8SecretsPropertySource(mockClient, "my-secret", null, Collections.emptyMap(), false)) + .isNotNull(); + } + + @Test + void constructorWithNamespaceMustNotFail() { + Mockito.when(mockClient.getNamespace()).thenReturn(null); + assertThat(new Fabric8SecretsPropertySource(mockClient, "my-secret", "ns", Collections.emptyMap(), false)) + .isNotNull(); + } + +} diff --git a/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySourceTest.java b/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySourceTest.java index 50f19b2f..d6816bc5 100644 --- a/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySourceTest.java +++ b/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/Fabric8SecretsPropertySourceTest.java @@ -17,16 +17,12 @@ package org.springframework.cloud.kubernetes.fabric8.config; import java.util.Base64; -import java.util.Collections; import io.fabric8.kubernetes.api.model.Secret; import io.fabric8.kubernetes.api.model.SecretBuilder; import io.fabric8.kubernetes.client.Config; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient; -import io.fabric8.kubernetes.client.server.mock.KubernetesServer; -import org.junit.ClassRule; -import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -41,23 +37,18 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; import static java.util.Collections.singletonMap; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatNoException; -import static org.assertj.core.api.Assertions.assertThatThrownBy; @ExtendWith(SpringExtension.class) @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, classes = App.class, properties = "spring.main.cloud-platform=KUBERNETES") @TestPropertySource("classpath:/application-secrets.properties") @EnableKubernetesMockClient(crud = true, https = false) -public class Fabric8SecretsPropertySourceTest { +class Fabric8SecretsPropertySourceTest { private static final String NAMESPACE = "test"; private static KubernetesClient mockClient; - @ClassRule - public static KubernetesServer mockServer = new KubernetesServer(false); - private static final String SECRET_VALUE = "secretValue"; @Autowired @@ -67,7 +58,7 @@ public class Fabric8SecretsPropertySourceTest { private Environment environment; @BeforeAll - public static void setUpBeforeClass() { + static void setUpBeforeClass() { // Configure the kubernetes master url to point to the mock server System.setProperty(Config.KUBERNETES_MASTER_SYSTEM_PROPERTY, mockClient.getConfiguration().getMasterUrl()); @@ -82,43 +73,13 @@ public class Fabric8SecretsPropertySourceTest { .addToData("secretName", Base64.getEncoder().encodeToString(SECRET_VALUE.getBytes())).build(); mockClient.secrets().inNamespace(NAMESPACE).create(secret); - mockServer.before(); - } - - @AfterAll - public static void tearDown() { - mockServer.after(); } @Test - public void toStringShouldNotExposeSecretValues() { + void toStringShouldNotExposeSecretValues() { PropertySource propertySource = this.propertySourceLocator.locate(this.environment); assertThat(propertySource.toString()).doesNotContain(SECRET_VALUE); assertThat(propertySource.getProperty("secretName")).isEqualTo("secretValue"); } - @Test - public void constructorShouldThrowExceptionOnFailureWhenFailFastIsEnabled() { - final String name = "my-config"; - final String namespace = "default"; - final String path = String.format("/api/v1/namespaces/%s/secrets/%s", namespace, name); - - mockServer.expect().withPath(path).andReturn(500, "Internal Server Error").once(); - assertThatThrownBy(() -> new Fabric8SecretsPropertySource(mockServer.getClient(), name, namespace, - Collections.emptyMap(), true)).isInstanceOf(IllegalStateException.class) - .hasMessage("Unable to read Secret with name '" + name + "' or labels [{}] in namespace '" - + namespace + "'"); - } - - @Test - public void constructorShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() { - final String name = "my-config"; - final String namespace = "default"; - final String path = String.format("/api/v1/namespaces/%s/secrets/%s", namespace, name); - - mockServer.expect().withPath(path).andReturn(500, "Internal Server Error").once(); - assertThatNoException().isThrownBy(() -> new Fabric8SecretsPropertySource(mockServer.getClient(), name, - namespace, Collections.emptyMap(), false)); - } - } diff --git a/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/reload/EventBasedConfigurationChangeDetectorTests.java b/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/reload/EventBasedConfigurationChangeDetectorTests.java index dbffbbbe..738627b4 100644 --- a/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/reload/EventBasedConfigurationChangeDetectorTests.java +++ b/spring-cloud-kubernetes-fabric8-config/src/test/java/org/springframework/cloud/kubernetes/fabric8/config/reload/EventBasedConfigurationChangeDetectorTests.java @@ -55,14 +55,17 @@ public class EventBasedConfigurationChangeDetectorTests { data.put("foo", "bar"); configMap.setData(data); MixedOperation> mixedOperation = mock(MixedOperation.class); + when(k8sClient.configMaps()).thenReturn(mixedOperation); Resource resource = mock(Resource.class); + when(resource.get()).thenReturn(configMap); when(mixedOperation.withName(eq("myconfigmap"))).thenReturn(resource); - when(k8sClient.configMaps()).thenReturn(mixedOperation); + when(mixedOperation.inNamespace("default")).thenReturn(mixedOperation); + when(k8sClient.getNamespace()).thenReturn("default"); Fabric8ConfigMapPropertySource fabric8ConfigMapPropertySource = new Fabric8ConfigMapPropertySource(k8sClient, "myconfigmap"); - env.getPropertySources().addFirst(new BootstrapPropertySource(fabric8ConfigMapPropertySource)); + env.getPropertySources().addFirst(new BootstrapPropertySource<>(fabric8ConfigMapPropertySource)); ConfigurationUpdateStrategy configurationUpdateStrategy = mock(ConfigurationUpdateStrategy.class); Fabric8ConfigMapPropertySourceLocator configMapLocator = mock(Fabric8ConfigMapPropertySourceLocator.class);