Skip to content

Config Server Controller Should Return All PropertySources #1600

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
merged 17 commits into from
Mar 28, 2024
Merged

Config Server Controller Should Return All PropertySources #1600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0b7e2ef
Prototype
ryanjbaxter Jan 22, 2024
c6c4aea
Merge remote-tracking branch 'Upstream/main' into property-source-nam…
ryanjbaxter Mar 13, 2024
e089b51
Initial implementation to return all property sources
ryanjbaxter Mar 14, 2024
bedc7ce
Merge branch 'main' into property-source-name-includes-profiles
ryanjbaxter Mar 14, 2024
bfc76c1
Removing testing config
ryanjbaxter Mar 14, 2024
2e112a5
fixing checkstyle
ryanjbaxter Mar 14, 2024
29128eb
Slightly different approach to changing PropertySource name that does…
ryanjbaxter Mar 15, 2024
9e5032d
Removing comment
ryanjbaxter Mar 15, 2024
8c86bd5
Merge branch 'main' into property-source-name-includes-profiles
ryanjbaxter Mar 18, 2024
f0844e1
New version the limits breaking changes
ryanjbaxter Mar 21, 2024
19c20eb
Fixing checkstyle
ryanjbaxter Mar 21, 2024
12ae0b3
Remove public static method to clear cache
ryanjbaxter Mar 21, 2024
9bb5287
Code review comments
ryanjbaxter Mar 22, 2024
de5391b
Merge branch 'main' into property-source-name-includes-profiles
ryanjbaxter Mar 22, 2024
86b8f12
Code review feedback
ryanjbaxter Mar 23, 2024
13efa61
Add flag to context
ryanjbaxter Mar 26, 2024
6d52d37
Cleaning up code
ryanjbaxter Mar 27, 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
7 changes: 6 additions & 1 deletion ...ava/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,10 @@
* @author wind57
*/
public record KubernetesClientConfigContext(CoreV1Api client, NormalizedSource normalizedSource, String namespace,
Environment environment) {
Environment environment, boolean includeDefaultProfileData) {

public KubernetesClientConfigContext(CoreV1Api client, NormalizedSource normalizedSource, String namespace,
Environment environment) {
this(client, normalizedSource, namespace, environment, true);
}
}
2 changes: 1 addition & 1 deletion ...a/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapsCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
*
* @author wind57
*/
final class KubernetesClientConfigMapsCache implements ConfigMapCache {
public final class KubernetesClientConfigMapsCache implements ConfigMapCache {

private static final LogAccessor LOG = new LogAccessor(LogFactory.getLog(KubernetesClientConfigMapsCache.class));

Expand Down
20 changes: 16 additions & 4 deletions .../java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,23 @@ static MultipleSourcesContainer configMapsDataByLabels(CoreV1Api coreV1Api, Stri
* </pre>
*/
static MultipleSourcesContainer secretsDataByName(CoreV1Api coreV1Api, String namespace,
LinkedHashSet<String> sourceNames, Environment environment) {
LinkedHashSet<String> sourceNames, Environment environment, boolean includeDefaultProfileData) {
List<StrippedSourceContainer> strippedSecrets = strippedSecrets(coreV1Api, namespace);
if (strippedSecrets.isEmpty()) {
return MultipleSourcesContainer.empty();
}
return ConfigUtils.processNamedData(strippedSecrets, environment, sourceNames, namespace, DECODE);
return ConfigUtils.processNamedData(strippedSecrets, environment, sourceNames, namespace, DECODE,
includeDefaultProfileData);
}

static MultipleSourcesContainer secretsDataByName(CoreV1Api coreV1Api, String namespace,
LinkedHashSet<String> sourceNames, Environment environment) {
return secretsDataByName(coreV1Api, namespace, sourceNames, environment, true);
}

static MultipleSourcesContainer configMapsDataByName(CoreV1Api coreV1Api, String namespace,
LinkedHashSet<String> sourceNames, Environment environment) {
return configMapsDataByName(coreV1Api, namespace, sourceNames, environment, true);
}

/**
Expand All @@ -125,12 +136,13 @@ static MultipleSourcesContainer secretsDataByName(CoreV1Api coreV1Api, String na
* </pre>
*/
static MultipleSourcesContainer configMapsDataByName(CoreV1Api coreV1Api, String namespace,
LinkedHashSet<String> sourceNames, Environment environment) {
LinkedHashSet<String> sourceNames, Environment environment, boolean includeDefaultProfileData) {
List<StrippedSourceContainer> strippedConfigMaps = strippedConfigMaps(coreV1Api, namespace);
if (strippedConfigMaps.isEmpty()) {
return MultipleSourcesContainer.empty();
}
return ConfigUtils.processNamedData(strippedConfigMaps, environment, sourceNames, namespace, DECODE);
return ConfigUtils.processNamedData(strippedConfigMaps, environment, sourceNames, namespace, DECODE,
includeDefaultProfileData);
}

private static List<StrippedSourceContainer> strippedConfigMaps(CoreV1Api coreV1Api, String namespace) {
Expand Down
12 changes: 11 additions & 1 deletion ...ngframework/cloud/kubernetes/client/config/NamedConfigMapContextToSourceDataProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.LinkedHashSet;
import java.util.function.Supplier;

import org.springframework.cloud.kubernetes.commons.config.ConfigUtils;
import org.springframework.cloud.kubernetes.commons.config.MultipleSourcesContainer;
import org.springframework.cloud.kubernetes.commons.config.NamedConfigMapNormalizedSource;
import org.springframework.cloud.kubernetes.commons.config.NamedSourceData;
Expand All @@ -42,10 +43,19 @@ public KubernetesClientContextToSourceData get() {
NamedConfigMapNormalizedSource source = (NamedConfigMapNormalizedSource) context.normalizedSource();

return new NamedSourceData() {
@Override
protected String generateSourceName(String target, String sourceName, String namespace,
String[] activeProfiles) {
if (source.appendProfileToName()) {
return ConfigUtils.sourceName(target, sourceName, namespace, activeProfiles);
}
return super.generateSourceName(target, sourceName, namespace, activeProfiles);
}

@Override
public MultipleSourcesContainer dataSupplier(LinkedHashSet<String> sourceNames) {
return KubernetesClientConfigUtils.configMapsDataByName(context.client(), context.namespace(),
sourceNames, context.environment());
sourceNames, context.environment(), context.includeDefaultProfileData());
}
}.compute(source.name().orElseThrow(), source.prefix(), source.target(), source.profileSpecificSources(),
source.failFast(), context.namespace(), context.environment().getActiveProfiles());
Expand Down
12 changes: 11 additions & 1 deletion ...pringframework/cloud/kubernetes/client/config/NamedSecretContextToSourceDataProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.LinkedHashSet;
import java.util.function.Supplier;

import org.springframework.cloud.kubernetes.commons.config.ConfigUtils;
import org.springframework.cloud.kubernetes.commons.config.MultipleSourcesContainer;
import org.springframework.cloud.kubernetes.commons.config.NamedSecretNormalizedSource;
import org.springframework.cloud.kubernetes.commons.config.NamedSourceData;
Expand All @@ -41,10 +42,19 @@ public KubernetesClientContextToSourceData get() {
NamedSecretNormalizedSource source = (NamedSecretNormalizedSource) context.normalizedSource();

return new NamedSourceData() {
@Override
protected String generateSourceName(String target, String sourceName, String namespace,
String[] activeProfiles) {
if (source.appendProfileToName()) {
return ConfigUtils.sourceName(target, sourceName, namespace, activeProfiles);
}
return super.generateSourceName(target, sourceName, namespace, activeProfiles);
}

@Override
public MultipleSourcesContainer dataSupplier(LinkedHashSet<String> sourceNames) {
return KubernetesClientConfigUtils.secretsDataByName(context.client(), context.namespace(),
sourceNames, context.environment());
sourceNames, context.environment(), context.includeDefaultProfileData());
}
}.compute(source.name().orElseThrow(), source.prefix(), source.target(), source.profileSpecificSources(),
source.failFast(), context.namespace(), context.environment().getActiveProfiles());
Expand Down
10 changes: 5 additions & 5 deletions ...mework/cloud/kubernetes/client/config/NamedConfigMapContextToSourceDataProviderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,17 +164,17 @@ void matchIncludeSingleProfile() {
stubCall(configMapList);
CoreV1Api api = new CoreV1Api();

NormalizedSource source = new NamedConfigMapNormalizedSource(RED_CONFIG_MAP_NAME, NAMESPACE, true, true);
NormalizedSource source = new NamedConfigMapNormalizedSource(RED_CONFIG_MAP_NAME, NAMESPACE, true,
ConfigUtils.Prefix.DEFAULT, true, true);
MockEnvironment environment = new MockEnvironment();
environment.setActiveProfiles("with-profile");
KubernetesClientConfigContext context = new KubernetesClientConfigContext(api, source, NAMESPACE, environment);
KubernetesClientConfigContext context = new KubernetesClientConfigContext(api, source, NAMESPACE, environment, false);

KubernetesClientContextToSourceData data = new NamedConfigMapContextToSourceDataProvider().get();
SourceData sourceData = data.apply(context);

Assertions.assertEquals(sourceData.sourceName(), "configmap.red.red-with-profile.default");
Assertions.assertEquals(sourceData.sourceData().size(), 2);
Assertions.assertEquals(sourceData.sourceData().get("color"), "really-red");
Assertions.assertEquals(sourceData.sourceName(), "configmap.red.red-with-profile.default.with-profile");
Copy link
Contributor

Choose a reason for hiding this comment

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

to be honest, I find this miss-leading and kind of don't agree with it, the name here should be:

configmap.red-with-profile.default.with-profile

at least for me.

I mean, this is a request for config server, something like account/with-profile. In the end we will make two requests in this case, one for the default config server profile and one for with-profile. This one here, simulates the with-profile one, so the name should not include red on its own, because red will be included in the default config server profile.

The name here btw is <SOURCE_TYPE>.<SOURCE_NAME>.<NAMESPACE>.<ACTIVE_PROFILE>.

But in general this "pain" comes from the fact that IMHO, we are expressing things here a bit in reverse, and again IMHO (very humble), we should change the PR slightly. I am going to present my idea in separate bigger comment, which is going to be the last one btw :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So why was it previously configmap.red.red-with-profile.default?

Assertions.assertEquals(sourceData.sourceData().size(), 1);
Assertions.assertEquals(sourceData.sourceData().get("taste"), "mango");

}
Expand Down
10 changes: 5 additions & 5 deletions ...framework/cloud/kubernetes/client/config/NamedSecretContextToSourceDataProviderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,17 +217,17 @@ void matchIncludeSingleProfile() {
stubCall(secretList);
CoreV1Api api = new CoreV1Api();

NormalizedSource source = new NamedSecretNormalizedSource("red", NAMESPACE, false, true);
NormalizedSource source = new NamedSecretNormalizedSource("red", NAMESPACE, false, ConfigUtils.Prefix.DEFAULT,
true, true);
MockEnvironment environment = new MockEnvironment();
environment.addActiveProfile("with-profile");
KubernetesClientConfigContext context = new KubernetesClientConfigContext(api, source, NAMESPACE, environment);
KubernetesClientConfigContext context = new KubernetesClientConfigContext(api, source, NAMESPACE, environment, false);

KubernetesClientContextToSourceData data = new NamedSecretContextToSourceDataProvider().get();
SourceData sourceData = data.apply(context);

Assertions.assertEquals(sourceData.sourceName(), "secret.red.red-with-profile.default");
Assertions.assertEquals(sourceData.sourceData().size(), 2);
Assertions.assertEquals(sourceData.sourceData().get("color"), "really-red");
Assertions.assertEquals(sourceData.sourceName(), "secret.red.red-with-profile.default.with-profile");
Assertions.assertEquals(sourceData.sourceData().size(), 1);
Assertions.assertEquals(sourceData.sourceData().get("taste"), "mango");

}
Expand Down
42 changes: 39 additions & 3 deletions ...ommons/src/main/java/org/springframework/cloud/kubernetes/commons/config/ConfigUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.springframework.cloud.kubernetes.commons.config;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Base64;
import java.util.HashMap;
import java.util.LinkedHashSet;
Expand Down Expand Up @@ -161,13 +162,27 @@ public static String sourceName(String target, String applicationName, String na
return target + PROPERTY_SOURCE_NAME_SEPARATOR + applicationName + PROPERTY_SOURCE_NAME_SEPARATOR + namespace;
}

public static String sourceName(String target, String applicationName, String namespace, String[] profiles) {
String name = sourceName(target, applicationName, namespace);
if (profiles != null && profiles.length > 0) {
name = name + PROPERTY_SOURCE_NAME_SEPARATOR + StringUtils.arrayToDelimitedString(profiles, "-");
}
return name;
}

public static MultipleSourcesContainer processNamedData(List<StrippedSourceContainer> strippedSources,
Environment environment, LinkedHashSet<String> sourceNames, String namespace, boolean decode) {
return processNamedData(strippedSources, environment, sourceNames, namespace, decode, true);
}

/**
* transforms raw data from one or multiple sources into an entry of source names and
* flattened data that they all hold (potentially overriding entries without any
* defined order).
*/
public static MultipleSourcesContainer processNamedData(List<StrippedSourceContainer> strippedSources,
Environment environment, LinkedHashSet<String> sourceNames, String namespace, boolean decode) {
Environment environment, LinkedHashSet<String> sourceNames, String namespace, boolean decode,
boolean includeDefaultProfileData) {

Map<String, StrippedSourceContainer> hashByName = strippedSources.stream()
.collect(Collectors.toMap(StrippedSourceContainer::name, Function.identity()));
Expand All @@ -189,14 +204,35 @@ public static MultipleSourcesContainer processNamedData(List<StrippedSourceConta
if (decode) {
rawData = decodeData(rawData);
}
data.putAll(SourceDataEntriesProcessor.processAllEntries(rawData == null ? Map.of() : rawData,
environment));

// In some cases we want to include properties from the default profile along with any active profiles
// In these cases includeDefaultProfileData will be true
// If includeDefaultProfileData is false then we want to make sure that we only return properties from any active profiles

//Check the source to see if it contains any active profiles
boolean containsActiveProfile = environment.getActiveProfiles().length == 0
|| Arrays.stream(environment.getActiveProfiles())
.anyMatch(p -> source.contains("-" + p) || "default".equals(p));
if (includeDefaultProfileData || containsActiveProfile
|| containsDataWithProfile(rawData, environment.getActiveProfiles())) {
data.putAll(SourceDataEntriesProcessor.processAllEntries(rawData == null ? Map.of() : rawData,
environment, includeDefaultProfileData));
}
}
});

return new MultipleSourcesContainer(foundSourceNames, data);
}

/*
* In the case there the data contains yaml or properties files we need to check their names to see if they
* contain any active profiles.
*/
private static boolean containsDataWithProfile(Map<String, String> rawData, String[] activeProfiles) {
return rawData.keySet().stream().anyMatch(
key -> Arrays.stream(activeProfiles).anyMatch(p -> key.contains("-" + p) || "default".equals(p)));
}

/**
* transforms raw data from one or multiple sources into an entry of source names and
* flattened data that they all hold (potentially overriding entries without any
Expand Down
23 changes: 19 additions & 4 deletions ...a/org/springframework/cloud/kubernetes/commons/config/NamedConfigMapNormalizedSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,25 @@ public final class NamedConfigMapNormalizedSource extends NormalizedSource {

private final boolean includeProfileSpecificSources;

private final boolean appendProfileToName;

public NamedConfigMapNormalizedSource(String name, String namespace, boolean failFast, ConfigUtils.Prefix prefix,
boolean includeProfileSpecificSources) {
super(name, namespace, failFast);
this.prefix = Objects.requireNonNull(prefix);
this.includeProfileSpecificSources = includeProfileSpecificSources;
this(name, namespace, failFast, prefix, includeProfileSpecificSources, false);
}

public NamedConfigMapNormalizedSource(String name, String namespace, boolean failFast,
boolean includeProfileSpecificSources) {
this(name, namespace, failFast, ConfigUtils.Prefix.DEFAULT, includeProfileSpecificSources);
}

public NamedConfigMapNormalizedSource(String name, String namespace, boolean failFast, ConfigUtils.Prefix prefix,
boolean includeProfileSpecificSources, boolean appendProfileToName) {
super(name, namespace, failFast);
this.prefix = ConfigUtils.Prefix.DEFAULT;
this.prefix = Objects.requireNonNull(prefix);
this.includeProfileSpecificSources = includeProfileSpecificSources;
this.appendProfileToName = appendProfileToName;

}

public ConfigUtils.Prefix prefix() {
Expand All @@ -51,6 +58,14 @@ public boolean profileSpecificSources() {
return includeProfileSpecificSources;
}

/**
* append or not the active profiles to the name of the generated source.
* At the moment this is true only for config server generated sources.
*/
public boolean appendProfileToName() {
return appendProfileToName;
}

@Override
public NormalizedSourceType type() {
return NormalizedSourceType.NAMED_CONFIG_MAP;
Expand Down
22 changes: 18 additions & 4 deletions ...java/org/springframework/cloud/kubernetes/commons/config/NamedSecretNormalizedSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,38 @@ public final class NamedSecretNormalizedSource extends NormalizedSource {

private final boolean includeProfileSpecificSources;

private final boolean appendProfileToName;

public NamedSecretNormalizedSource(String name, String namespace, boolean failFast, ConfigUtils.Prefix prefix,
boolean includeProfileSpecificSources) {
boolean includeProfileSpecificSources, boolean appendProfileToName) {
super(name, namespace, failFast);
this.prefix = Objects.requireNonNull(prefix);
this.includeProfileSpecificSources = includeProfileSpecificSources;
this.appendProfileToName = appendProfileToName;
}

public NamedSecretNormalizedSource(String name, String namespace, boolean failFast,
boolean includeProfileSpecificSources) {
super(name, namespace, failFast);
this.prefix = ConfigUtils.Prefix.DEFAULT;
this.includeProfileSpecificSources = includeProfileSpecificSources;
this(name, namespace, failFast, ConfigUtils.Prefix.DEFAULT, includeProfileSpecificSources, false);
}

public NamedSecretNormalizedSource(String name, String namespace, boolean failFast, ConfigUtils.Prefix prefix,
boolean includeProfileSpecificSources) {
this(name, namespace, failFast, prefix, includeProfileSpecificSources, false);
}

public boolean profileSpecificSources() {
return includeProfileSpecificSources;
}

/**
* append or not the active profiles to the name of the generated source.
* At the moment this is true only for config server generated sources.
*/
public boolean appendProfileToName() {
return appendProfileToName;
}

public ConfigUtils.Prefix prefix() {
return prefix;
}
Expand Down
6 changes: 5 additions & 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 @@ -67,7 +67,11 @@ public final SourceData compute(String sourceName, ConfigUtils.Prefix prefix, St
}

String names = data.names().stream().sorted().collect(Collectors.joining(PROPERTY_SOURCE_NAME_SEPARATOR));
return new SourceData(ConfigUtils.sourceName(target, names, namespace), data.data());
return new SourceData(generateSourceName(target, names, namespace, activeProfiles), data.data());
}

protected String generateSourceName(String target, String sourceName, String namespace, String[] activeProfiles) {
return ConfigUtils.sourceName(target, sourceName, namespace);
}

/**
Expand Down
Loading