Fix 1592 empty source on error part 2 (#1800)

spring-cloud · Nov 25, 2024 · 8eae516 · 8eae516
1 parent ac23677
commit 8eae516
Show file tree

Hide file tree

Showing 21 changed files with 195 additions and 177 deletions.
diff --git a/...rk/cloud/kubernetes/client/config/KubernetesClientConfigMapErrorOnReadingSourceTests.java b/...rk/cloud/kubernetes/client/config/KubernetesClientConfigMapErrorOnReadingSourceTests.java
@@ -39,10 +39,8 @@
 import org.springframework.boot.test.system.OutputCaptureExtension;
 import org.springframework.cloud.kubernetes.commons.KubernetesNamespaceProvider;
 import org.springframework.cloud.kubernetes.commons.config.ConfigMapConfigProperties;
-import org.springframework.cloud.kubernetes.commons.config.Constants;
 import org.springframework.cloud.kubernetes.commons.config.RetryProperties;
 import org.springframework.core.env.CompositePropertySource;
-import org.springframework.core.env.MapPropertySource;
 import org.springframework.core.env.PropertySource;
 import org.springframework.mock.env.MockEnvironment;
 
@@ -100,12 +98,11 @@ public void afterEach() {
 
 	/**
 	 * <pre>
-	 *     we try to read all config maps in a namespace and fail,
-	 *     thus generate a well defined name for the source.
+	 *     we try to read all config maps in a namespace and fail.
 	 * </pre>
 	 */
 	@Test
-	void namedSingleConfigMapFails() {
+	void namedSingleConfigMapFails(CapturedOutput output) {
 		String name = "my-config";
 		String namespace = "spring-k8s";
 		String path = "/api/v1/namespaces/" + namespace + "/configmaps";
@@ -120,13 +117,10 @@ void namedSingleConfigMapFails() {
 				configMapConfigProperties, new KubernetesNamespaceProvider(new MockEnvironment()));
 
 		CompositePropertySource propertySource = (CompositePropertySource) locator.locate(new MockEnvironment());
-		MapPropertySource mapPropertySource = (MapPropertySource) propertySource.getPropertySources()
-			.stream()
-			.findAny()
-			.orElseThrow();
 
-		assertThat(mapPropertySource.getName()).isEqualTo("configmap..spring-k8s");
-		assertThat(propertySource.getProperty(Constants.ERROR_PROPERTY)).isEqualTo("true");
+		assertThat(propertySource.getPropertySources()).isEmpty();
+		assertThat(output.getOut()).contains("Failure in reading named sources");
+		assertThat(output.getOut()).contains("Failed to load source: { config-map name : 'Optional[my-config]'");
 
 	}
 
@@ -168,11 +162,12 @@ void namedTwoConfigMapsOneFails(CapturedOutput output) {
 		CompositePropertySource propertySource = (CompositePropertySource) locator.locate(new MockEnvironment());
 		List<String> names = propertySource.getPropertySources().stream().map(PropertySource::getName).toList();
 
-		// two sources are present, one being empty
-		assertThat(names).containsExactly("configmap.two.default", "configmap..default");
-		assertThat(propertySource.getProperty(Constants.ERROR_PROPERTY)).isEqualTo("true");
+		// one property source is present
+		assertThat(names).containsExactly("configmap.two.default");
 		assertThat(output.getOut())
 			.doesNotContain("sourceName : two was requested, but not found in namespace : default");
+		assertThat(output.getOut()).contains("Failure in reading named sources");
+		assertThat(output.getOut()).contains("Failed to load source: { config-map name : 'Optional[one]'");
 
 	}
 
@@ -212,20 +207,19 @@ void namedTwoConfigMapsBothFail(CapturedOutput output) {
 				configMapConfigProperties, new KubernetesNamespaceProvider(new MockEnvironment()));
 
 		CompositePropertySource propertySource = (CompositePropertySource) locator.locate(new MockEnvironment());
-		List<String> names = propertySource.getPropertySources().stream().map(PropertySource::getName).toList();
 
-		assertThat(names).containsExactly("configmap..default");
-		assertThat(propertySource.getProperty(Constants.ERROR_PROPERTY)).isEqualTo("true");
+		assertThat(propertySource.getPropertySources()).isEmpty();
 		assertThat(output.getOut())
 			.doesNotContain("sourceName : one was requested, but not found in namespace : default");
 		assertThat(output.getOut())
 			.doesNotContain("sourceName : two was requested, but not found in namespace : default");
+		assertThat(output.getOut()).contains("Failure in reading named sources");
+		assertThat(output.getOut()).contains("Failed to load source: { config-map name : 'Optional[one]'");
 	}
 
 	/**
 	 * <pre>
-	 *     we try to read all config maps in a namespace and fail,
-	 *     thus generate a well defined name for the source.
+	 *     we try to read all config maps in a namespace and fail.
 	 * </pre>
 	 */
 	@Test
@@ -256,12 +250,11 @@ void labeledSingleConfigMapFails(CapturedOutput output) {
 				configMapConfigProperties, new KubernetesNamespaceProvider(new MockEnvironment()));
 
 		CompositePropertySource propertySource = (CompositePropertySource) locator.locate(new MockEnvironment());
-		List<String> sourceNames = propertySource.getPropertySources().stream().map(PropertySource::getName).toList();
 
-		assertThat(sourceNames).containsExactly("configmap..spring-k8s");
-		assertThat(propertySource.getProperty(Constants.ERROR_PROPERTY)).isEqualTo("true");
-		assertThat(output).contains("failure in reading labeled sources");
-		assertThat(output).contains("failure in reading named sources");
+		assertThat(propertySource.getPropertySources()).isEmpty();
+		assertThat(output.getOut()).contains("Failure in reading labeled sources");
+		assertThat(output.getOut()).contains("Failure in reading named sources");
+		assertThat(output.getOut()).contains("Failed to load source: { config map labels : '{a=b}'");
 	}
 
 	/**
@@ -311,12 +304,11 @@ void labeledTwoConfigMapsOneFails(CapturedOutput output) {
 		CompositePropertySource propertySource = (CompositePropertySource) locator.locate(new MockEnvironment());
 		List<String> names = propertySource.getPropertySources().stream().map(PropertySource::getName).toList();
 
-		// two sources are present, one being empty
-		assertThat(names).containsExactly("configmap.two.default", "configmap..default");
-		assertThat(propertySource.getProperty(Constants.ERROR_PROPERTY)).isEqualTo("true");
-
-		assertThat(output).contains("failure in reading labeled sources");
-		assertThat(output).contains("failure in reading named sources");
+		// one source is present
+		assertThat(names).containsExactly("configmap.two.default");
+		assertThat(output.getOut()).contains("Failure in reading labeled sources");
+		assertThat(output.getOut()).contains("Failure in reading named sources");
+		assertThat(output.getOut()).contains("Failed to load source: { config map labels : '{one=1}'");
 
 	}
 
@@ -364,15 +356,12 @@ void labeledTwoConfigMapsBothFail(CapturedOutput output) {
 				configMapConfigProperties, new KubernetesNamespaceProvider(new MockEnvironment()));
 
 		CompositePropertySource propertySource = (CompositePropertySource) locator.locate(new MockEnvironment());
-		List<String> names = propertySource.getPropertySources().stream().map(PropertySource::getName).toList();
-
-		// all 3 sources ('application' named source, and two labeled sources)
-		assertThat(names).containsExactly("configmap..default");
-		assertThat(propertySource.getProperty(Constants.ERROR_PROPERTY)).isEqualTo("true");
-
-		assertThat(output).contains("failure in reading labeled sources");
-		assertThat(output).contains("failure in reading named sources");
 
+		assertThat(propertySource.getPropertySources()).isEmpty();
+		assertThat(output).contains("Failure in reading labeled sources");
+		assertThat(output).contains("Failure in reading named sources");
+		assertThat(output.getOut()).contains("Failed to load source: { config map labels : '{one=1}'");
+		assertThat(output.getOut()).contains("Failed to load source: { config map labels : '{two=2}'");
 	}
 
 }
diff --git a/...k/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySourceLocatorTests.java b/...k/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySourceLocatorTests.java
@@ -16,6 +16,7 @@
 
 package org.springframework.cloud.kubernetes.client.config;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -34,12 +35,16 @@
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
 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.KubernetesNamespaceProvider;
 import org.springframework.cloud.kubernetes.commons.config.ConfigMapConfigProperties;
 import org.springframework.cloud.kubernetes.commons.config.Constants;
 import org.springframework.cloud.kubernetes.commons.config.NamespaceResolutionFailedException;
 import org.springframework.cloud.kubernetes.commons.config.RetryProperties;
+import org.springframework.core.env.CompositePropertySource;
 import org.springframework.core.env.PropertySource;
 import org.springframework.mock.env.MockEnvironment;
 
@@ -55,6 +60,7 @@
  * @author Ryan Baxter
  * @author Isik Erhan
  */
+@ExtendWith(OutputCaptureExtension.class)
 class KubernetesClientConfigMapPropertySourceLocatorTests {
 
 	private static final V1ConfigMapList PROPERTIES_CONFIGMAP_LIST = new V1ConfigMapList()
@@ -185,7 +191,7 @@ public void locateShouldThrowExceptionOnFailureWhenFailFastIsEnabled() {
 	}
 
 	@Test
-	public void locateShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() {
+	public void locateShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled(CapturedOutput output) {
 		CoreV1Api api = new CoreV1Api();
 		stubFor(get("/api/v1/namespaces/default/configmaps")
 			.willReturn(aResponse().withStatus(500).withBody("Internal Server Error")));
@@ -196,7 +202,18 @@ public void locateShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() {
 		KubernetesClientConfigMapPropertySourceLocator locator = new KubernetesClientConfigMapPropertySourceLocator(api,
 				configMapConfigProperties, new KubernetesNamespaceProvider(new MockEnvironment()));
 
-		assertThatNoException().isThrownBy(() -> locator.locate(new MockEnvironment()));
+		List<PropertySource<?>> result = new ArrayList<>();
+		assertThatNoException().isThrownBy(() -> {
+			PropertySource<?> source = locator.locate(new MockEnvironment());
+			result.add(source);
+		});
+
+		assertThat(result.get(0)).isInstanceOf(CompositePropertySource.class);
+		CompositePropertySource composite = (CompositePropertySource) result.get(0);
+		assertThat(composite.getPropertySources()).hasSize(0);
+		assertThat(output.getOut()).contains("Failed to load source:");
+
+
 	}
 
 }
diff --git a/...ork/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceLocatorTests.java b/...ork/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceLocatorTests.java
@@ -16,6 +16,7 @@
 
 package org.springframework.cloud.kubernetes.client.config;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -30,11 +31,15 @@
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
 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.KubernetesNamespaceProvider;
 import org.springframework.cloud.kubernetes.commons.config.NamespaceResolutionFailedException;
 import org.springframework.cloud.kubernetes.commons.config.RetryProperties;
 import org.springframework.cloud.kubernetes.commons.config.SecretsConfigProperties;
+import org.springframework.core.env.CompositePropertySource;
 import org.springframework.core.env.PropertySource;
 import org.springframework.mock.env.MockEnvironment;
 
@@ -50,6 +55,7 @@
  * @author Ryan Baxter
  * @author Isik Erhan
  */
+@ExtendWith(OutputCaptureExtension.class)
 class KubernetesClientSecretsPropertySourceLocatorTests {
 
 	private static final String LIST_API = "/api/v1/namespaces/default/secrets";
@@ -200,7 +206,7 @@ void locateShouldThrowExceptionOnFailureWhenFailFastIsEnabled() {
 	}
 
 	@Test
-	void locateShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() {
+	void locateShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled(CapturedOutput output) {
 		CoreV1Api api = new CoreV1Api();
 		stubFor(get(LIST_API).willReturn(aResponse().withStatus(500).withBody("Internal Server Error")));
 
@@ -210,7 +216,16 @@ void locateShouldNotThrowExceptionOnFailureWhenFailFastIsDisabled() {
 		KubernetesClientSecretsPropertySourceLocator locator = new KubernetesClientSecretsPropertySourceLocator(api,
 				new KubernetesNamespaceProvider(new MockEnvironment()), secretsConfigProperties);
 
-		assertThatNoException().isThrownBy(() -> locator.locate(new MockEnvironment()));
+		List<PropertySource<?>> result = new ArrayList<>();
+		assertThatNoException().isThrownBy(() -> {
+			PropertySource<?> source = locator.locate(new MockEnvironment());
+			result.add(source);
+		});
+
+		assertThat(result.get(0)).isInstanceOf(CompositePropertySource.class);
+		CompositePropertySource composite = (CompositePropertySource) result.get(0);
+		assertThat(composite.getPropertySources()).hasSize(0);
+		assertThat(output.getOut()).contains("Failed to load source:");
 	}
 
 }
diff --git a/...rg/springframework/cloud/kubernetes/client/config/reload_it/EventReloadConfigMapTest.java b/...rg/springframework/cloud/kubernetes/client/config/reload_it/EventReloadConfigMapTest.java
@@ -154,11 +154,10 @@ void test(CapturedOutput output) {
 
 		// we fail while reading 'configMapOne'
 		Awaitility.await().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofSeconds(1)).until(() -> {
-			boolean one = output.getOut().contains("failure in reading named sources");
-			boolean two = output.getOut()
-				.contains("there was an error while reading config maps/secrets, no reload will happen");
+			boolean one = output.getOut().contains("Failure in reading named sources");
+			boolean two = output.getOut().contains("Failed to load source");
 			boolean three = output.getOut()
-				.contains("reloadable condition was not satisfied, reload will not be triggered");
+				.contains("Reloadable condition was not satisfied, reload will not be triggered");
 			boolean updateStrategyNotCalled = !strategyCalled[0];
 			return one && two && three && updateStrategyNotCalled;
 		});

diff --git a/...a/org/springframework/cloud/kubernetes/client/config/reload_it/EventReloadSecretTest.java b/...a/org/springframework/cloud/kubernetes/client/config/reload_it/EventReloadSecretTest.java
@@ -156,11 +156,10 @@ void test(CapturedOutput output) {
 
 		// we fail while reading 'configMapOne'
 		Awaitility.await().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofSeconds(1)).until(() -> {
-			boolean one = output.getOut().contains("failure in reading named sources");
-			boolean two = output.getOut()
-				.contains("there was an error while reading config maps/secrets, no reload will happen");
+			boolean one = output.getOut().contains("Failure in reading named sources");
+			boolean two = output.getOut().contains("Failed to load source");
 			boolean three = output.getOut()
-				.contains("reloadable condition was not satisfied, reload will not be triggered");
+				.contains("Reloadable condition was not satisfied, reload will not be triggered");
 			boolean updateStrategyNotCalled = !strategyCalled[0];
 			return one && two && three && updateStrategyNotCalled;
 		});

diff --git a/.../springframework/cloud/kubernetes/client/config/reload_it/PollingReloadConfigMapTest.java b/.../springframework/cloud/kubernetes/client/config/reload_it/PollingReloadConfigMapTest.java
@@ -136,11 +136,10 @@ static void after() {
 	void test(CapturedOutput output) {
 		// we fail while reading 'configMapOne'
 		Awaitility.await().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofSeconds(1)).until(() -> {
-			boolean one = output.getOut().contains("failure in reading named sources");
-			boolean two = output.getOut()
-				.contains("there was an error while reading config maps/secrets, no reload will happen");
+			boolean one = output.getOut().contains("Failure in reading named sources");
+			boolean two = output.getOut().contains("Failed to load source");
 			boolean three = output.getOut()
-				.contains("reloadable condition was not satisfied, reload will not be triggered");
+				.contains("Reloadable condition was not satisfied, reload will not be triggered");
 			boolean updateStrategyNotCalled = !strategyCalled[0];
 			return one && two && three && updateStrategyNotCalled;
 		});

diff --git a/...org/springframework/cloud/kubernetes/client/config/reload_it/PollingReloadSecretTest.java b/...org/springframework/cloud/kubernetes/client/config/reload_it/PollingReloadSecretTest.java
@@ -137,11 +137,10 @@ static void after() {
 	void test(CapturedOutput output) {
 		// we fail while reading 'secretOne'
 		Awaitility.await().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofSeconds(1)).until(() -> {
-			boolean one = output.getOut().contains("failure in reading named sources");
-			boolean two = output.getOut()
-				.contains("there was an error while reading config maps/secrets, no reload will happen");
+			boolean one = output.getOut().contains("Failure in reading named sources");
+			boolean two = output.getOut().contains("Failed to load source");
 			boolean three = output.getOut()
-				.contains("reloadable condition was not satisfied, reload will not be triggered");
+				.contains("Reloadable condition was not satisfied, reload will not be triggered");
 			boolean updateStrategyNotCalled = !strategyCalled[0];
 			return one && two && three && updateStrategyNotCalled;
 		});

diff --git a/...a/org/springframework/cloud/kubernetes/commons/config/ConfigMapPropertySourceLocator.java b/...a/org/springframework/cloud/kubernetes/commons/config/ConfigMapPropertySourceLocator.java
@@ -84,8 +84,13 @@ public PropertySource<?> locate(Environment environment) {
 				LOG.debug("Config Map normalized sources : " + sources);
 				sources.forEach(s -> {
 					MapPropertySource propertySource = getMapPropertySource(s, env);
-					LOG.debug("Adding config map property source " + propertySource.getName());
-					composite.addFirstPropertySource(propertySource);
+					if ("true".equals(propertySource.getProperty(Constants.ERROR_PROPERTY))) {
+						LOG.warn("Failed to load source: " + s);
+					}
+					else {
+						LOG.debug("Adding config map property source " + propertySource.getName());
+						composite.addFirstPropertySource(propertySource);
+					}
 				});
 			}
 

diff --git a/.../src/main/java/org/springframework/cloud/kubernetes/commons/config/LabeledSourceData.java b/.../src/main/java/org/springframework/cloud/kubernetes/commons/config/LabeledSourceData.java
@@ -79,7 +79,7 @@ public final SourceData compute(Map<String, String> labels, ConfigUtils.Prefix p
 			}
 		}
 		catch (Exception e) {
-			LOG.warn("failure in reading labeled sources");
+			LOG.warn("Failure in reading labeled sources");
 			onException(failFast, e);
 			data = new MultipleSourcesContainer(data.names(), Map.of(ERROR_PROPERTY, "true"));
 		}

diff --git a/...ns/src/main/java/org/springframework/cloud/kubernetes/commons/config/NamedSourceData.java b/...ns/src/main/java/org/springframework/cloud/kubernetes/commons/config/NamedSourceData.java
@@ -71,7 +71,7 @@ public final SourceData compute(String sourceName, ConfigUtils.Prefix prefix, St
 
 		}
 		catch (Exception e) {
-			LOG.warn("failure in reading named sources");
+			LOG.warn("Failure in reading named sources");
 			onException(failFast, e);
 			data = new MultipleSourcesContainer(data.names(), Map.of(ERROR_PROPERTY, "true"));
 		}

diff --git a/...ava/org/springframework/cloud/kubernetes/commons/config/SecretsPropertySourceLocator.java b/...ava/org/springframework/cloud/kubernetes/commons/config/SecretsPropertySourceLocator.java
@@ -90,8 +90,14 @@ public PropertySource<?> locate(Environment environment) {
 			if (this.properties.enableApi()) {
 				uniqueSources.forEach(s -> {
 					MapPropertySource propertySource = getSecretsPropertySourceForSingleSecret(env, s);
-					LOG.debug("Adding secret property source " + propertySource.getName());
-					composite.addFirstPropertySource(propertySource);
+
+					if ("true".equals(propertySource.getProperty(Constants.ERROR_PROPERTY))) {
+						LOG.warn("Failed to load source: " + s);
+					}
+					else {
+						LOG.debug("Adding secret property source " + propertySource.getName());
+						composite.addFirstPropertySource(propertySource);
+					}
 				});
 			}