Skip to content

Fix 1592 empty source on error part 2 #1800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryanjbaxter merged 8 commits into from
Nov 25, 2024
Merged

Fix 1592 empty source on error part 2 #1800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5d593c1
dirty
wind57 Nov 22, 2024
b08280c
fix
wind57 Nov 22, 2024
2ecb77b
checkstyle
wind57 Nov 22, 2024
06e5b86
fix issue
wind57 Nov 23, 2024
63c5834
fix issue
wind57 Nov 23, 2024
c97dc85
fix issue
wind57 Nov 23, 2024
a9bc7c2
checkstyle
wind57 Nov 23, 2024
4b6846b
placeholder commit
wind57 Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 27 additions & 38 deletions ...rk/cloud/kubernetes/client/config/KubernetesClientConfigMapErrorOnReadingSourceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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";
Expand All @@ -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]'");

}

Expand Down Expand Up @@ -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]'");

}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}'");
}

/**
Expand Down Expand Up @@ -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}'");

}

Expand Down Expand Up @@ -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}'");
}

}
21 changes: 19 additions & 2 deletions ...k/cloud/kubernetes/client/config/KubernetesClientConfigMapPropertySourceLocatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -55,6 +60,7 @@
* @author Ryan Baxter
* @author Isik Erhan
*/
@ExtendWith(OutputCaptureExtension.class)
class KubernetesClientConfigMapPropertySourceLocatorTests {

private static final V1ConfigMapList PROPERTIES_CONFIGMAP_LIST = new V1ConfigMapList()
Expand Down Expand Up @@ -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")));
Expand All @@ -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:");


}

}
19 changes: 17 additions & 2 deletions ...ork/cloud/kubernetes/client/config/KubernetesClientSecretsPropertySourceLocatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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";
Expand Down Expand Up @@ -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")));

Expand All @@ -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:");
}

}
7 changes: 3 additions & 4 deletions ...rg/springframework/cloud/kubernetes/client/config/reload_it/EventReloadConfigMapTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
Expand Down
7 changes: 3 additions & 4 deletions ...a/org/springframework/cloud/kubernetes/client/config/reload_it/EventReloadSecretTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
Expand Down
7 changes: 3 additions & 4 deletions .../springframework/cloud/kubernetes/client/config/reload_it/PollingReloadConfigMapTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
Expand Down
7 changes: 3 additions & 4 deletions ...org/springframework/cloud/kubernetes/client/config/reload_it/PollingReloadSecretTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
Expand Down
9 changes: 7 additions & 2 deletions ...a/org/springframework/cloud/kubernetes/commons/config/ConfigMapPropertySourceLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))) {
Copy link
Contributor Author

@wind57 wind57 Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix should be here and not in the reload. We should not be saving in the environment any property source that was computed on error.

Since reload code, calls this method anyway, it takes care to fix the issue.

There are two parts in the bug:

  • (a) when we try to see if we need to reload in the first place
  • (b) when we actually reload

In both cases, we call this locate and in both cases we need to skip a property source that was computed on error. Since reload code calls this one, the skipping of the source is done here

Copy link
Contributor Author

@wind57 wind57 Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing is done in SecretsPropertySourceLocator, everything else is related to changing the tests and proper logging

LOG.warn("Failed to load source: " + s);
}
else {
LOG.debug("Adding config map property source " + propertySource.getName());
composite.addFirstPropertySource(propertySource);
}
});
}

Expand Down
2 changes: 1 addition & 1 deletion .../src/main/java/org/springframework/cloud/kubernetes/commons/config/LabeledSourceData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
}
Expand Down
2 changes: 1 addition & 1 deletion ...ns/src/main/java/org/springframework/cloud/kubernetes/commons/config/NamedSourceData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
}
Expand Down
10 changes: 8 additions & 2 deletions ...ava/org/springframework/cloud/kubernetes/commons/config/SecretsPropertySourceLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
});
}

Expand Down
Loading
Loading