Config Server Controller Should Return All PropertySources #1600
Conversation
| addApplicationConfiguration(environment, springEnv, "application"); | ||
| return environment; | ||
| ArrayUtils.reverse(profiles); | ||
| for (String activeProfile : profiles) { |
There was a problem hiding this comment.
Before this change the KubernetesPropertySourceSuppliers would return a single PropertySource with all the properties aggregated together. This makes sense when the app is using the K8S API to directly fetch its configuration. However when the app is leveraging the Kuberentes Config Server we actually want to return a separate PropertySource for not only each configmap/secret we find but also each data element inside the config map/secret.
So instead of creating one StandardEnvironment with all the requested profiles and getting back a single PropertySource we create a StandardEnvironment for each profile so we add a PropertySource for each one.
In addition we were not also fetching the default profile in the case where a profile is requested from the config server so I have added some logic to make sure we return that data as well.
There was a problem hiding this comment.
About your last sentence, I have a question please. So if I request the dev profile from config server, we need to return stuff for two profiles: dev and default. Because I see you are adding this profile to in KubernetesEnvironmentRepository. Seems like that is the case, just want to make sure
There was a problem hiding this comment.
Correct. See the example from the config server in the PR description that I added. When you request k8s,prod the configuration for the default profile is also returned
There was a problem hiding this comment.
that makes sense logically, yes, my miss-understanding still exists though.
what is the configmap (for example) that would correspond to the default profile? Let me explain myself. If your config request "matches" account and you request two more profiles: k8s and prod, under the hood we will return data from 3 configmaps:
account
account-k8s
account-prod
where k8s and prod would be the active profiles.
How does default profile fits here? We do not have account-default and that is normal and I highly doubt this is what you want.
For me, the default profile should match account configmap, I'd assume. But if so, we already do that
There was a problem hiding this comment.
All of what you said is 100% correct.
The problem with the was the code works today at least for the config server is that the property sources for account-k8s and account-prod would include the properties from account as well and that is not technically correct because those property sources don't actually contain those properties.
This is why I needed to make changes here
https://github.com/spring-cloud/spring-cloud-kubernetes/pull/1600/files#diff-7fd0d91605f84c420115afd2e5ace4fa142b14471c72c82499c36b17cdc4fb17R208
and here
In the case where the we are looking at config map or secret that has an active profile appended to it we should not include any properties from the default profile (because we will have a separate property source for that).
…'t have widespread effects
|
|
||
| NormalizedSource devSource = new NamedConfigMapNormalizedSource(applicationName, "dev", false, true); | ||
| KubernetesClientConfigContext devContext = new KubernetesClientConfigContext(coreApi, devSource, "dev", | ||
| if ("dev".equals(namespace)) { |
There was a problem hiding this comment.
There was a bug here. These property sources for the dev namespace were being added even if the namespace was not enabled.
|
@wind57 I am trying to understand something that I can't figure out.. Why does the |
|
I've been through the history and I can't explain this to myself. Indeed I added profile specific support and I made those changes, but it seems I missed it for secrets. This seems to be also true since for config, |
...java/org/springframework/cloud/kubernetes/client/config/KubernetesClientConfigMapsCache.java
Outdated
Show resolved
Hide resolved
...in/java/org/springframework/cloud/kubernetes/commons/config/NamedSecretNormalizedSource.java
Show resolved
Hide resolved
...java/org/springframework/cloud/kubernetes/commons/config/NamedConfigMapNormalizedSource.java
Show resolved
Hide resolved
...mmons/src/main/java/org/springframework/cloud/kubernetes/commons/config/NamedSourceData.java
Show resolved
Hide resolved
| @Override | ||
| public MultipleSourcesContainer dataSupplier(LinkedHashSet<String> sourceNames) { | ||
| if (source.appendProfileToName()) { |
There was a problem hiding this comment.
I have a few questions here cause I'm really confused.
-
In my understanding,
source.appendProfileToName()would betrueonly for config server usage, right? Meaning the property sources created for config server would have their names appended with profiles, the others will not. -
Only when we compute sources for config server is when we want to see if the
defaultprofile is present in the active ones, right? Cause we always want to include this profile for config server. We don't want to see if thisdefaultprofile is present in active profiles otherwise. Am I correct?
There was a problem hiding this comment.
Yes both of those assumptions are correct.
There was a problem hiding this comment.
This is my last comment on this PR, sorry for taking so much, but I had to use quite a lot of pen/paper and head scratching to fully get the picture here. Usually when I make PRs this time is "invisible", but it was quite visible here :( I'm also sorry for making it so big upfront.
We are trying to express the fact that the calling code is coming from config-server or not via appendProfileToName. Why can't we express this intention directly then? Why can't we create an enum in commons that would precisely express that:
package org.springframework.cloud.kubernetes.commons.config;
/**
* denotes the calling code.
*/
public enum Caller {
/**
* configuration server is the caller.
*/
CONFIG_SERVER,
/**
* plain property sources is the caller.
*/
CONFIG
}
Then in NormalizedSource create a new method:
protected Caller caller() {
return Caller.CONFIG;
}
Then in NamedConfigMapNormalizedSource instead of appendProfileToName use Caller.
By default everyone will inherit it and by default, code will act exactly the same as it used to be. For fabric8 client that is what we want:
In fabric8 NamedConfigMapContextToSourceDataProvider, we could have:
@Override
protected String generateSourceName(String target, String sourceName, String namespace,
String[] activeProfiles) {
// for fabric8 client this will always be false, since config server
// is based on the k8s-native client.
if (source.caller() == Caller.CONFIG_SERVER) {
return ConfigUtils.sourceName(target, sourceName, namespace, activeProfiles);
}
return super.generateSourceName(target, sourceName, namespace, activeProfiles);
}
In k8s-client NamedConfigMapContextToSourceDataProvider can be simplified to:
return new NamedSourceData() {
@Override
protected String generateSourceName(String target, String sourceName, String namespace,
String[] activeProfiles) {
if (source.caller() == Caller.CONFIG_SERVER) {
return ConfigUtils.sourceName(target, sourceName, namespace, activeProfiles);
}
// this one does not uses the last argument
return super.generateSourceName(target, sourceName, namespace, activeProfiles);
}
@Override
public MultipleSourcesContainer dataSupplier(LinkedHashSet<String> sourceNames) {
return KubernetesClientConfigUtils.configMapsDataByName(context.client(), context.namespace(),
sourceNames, context.environment(), source.caller());
}
}.compute(source.name().orElseThrow(), source.prefix(), source.target(), source.profileSpecificSources(),
source.failFast(), context.namespace(), context.environment().getActiveProfiles());
and the call to : KubernetesClientConfigUtils::configMapsDataByName:
/**
* <pre>
* 1. read all config maps in the provided namespace
* 2. from the above, filter the ones that we care about (by name)
* 3. see if any of the config maps has a single yaml/properties file
* 4. gather all the names of the config maps + data they hold
* </pre>
*/
static MultipleSourcesContainer configMapsDataByName(CoreV1Api coreV1Api, String namespace,
LinkedHashSet<String> sourceNames, Environment environment, Caller caller) {
List<StrippedSourceContainer> strippedConfigMaps = strippedConfigMaps(coreV1Api, namespace);
if (strippedConfigMaps.isEmpty()) {
return MultipleSourcesContainer.empty();
}
LOG.debug("caller is : " + caller);
if (caller == Caller.CONFIG) {
return ConfigUtils.processNamedData(strippedConfigMaps, environment, sourceNames, namespace, DECODE);
}
else {
return ConfigUtils.processNamedData(strippedConfigMaps, environment, sourceNames, namespace, DECODE, caller);
}
}
(we would not need two configMapsDataByName here, as the method was not public, so OK to be changed).
There are changes that I am proposing in ConfigUtils::processNamedData:
/**
* 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,
Caller caller) {
Map<String, StrippedSourceContainer> hashByName = strippedSources.stream()
.collect(Collectors.toMap(StrippedSourceContainer::name, Function.identity()));
LinkedHashSet<String> foundSourceNames = new LinkedHashSet<>();
Map<String, Object> data = new HashMap<>();
Set<String> activeProfiles = Arrays.stream(environment.getActiveProfiles()).collect(Collectors.toSet());
// this is an ordered stream, and it means that non-profile based sources will be
// processed before profile based sources. This way, we replicate that
// "application-dev.yaml"
// overrides properties from "application.yaml"
sourceNames.forEach(sourceName -> {
StrippedSourceContainer stripped = hashByName.get(sourceName);
if (stripped != null) {
LOG.debug("Found source with name : '" + sourceName + " in namespace: '" + namespace + "'");
foundSourceNames.add(sourceName);
// see if data is a single yaml/properties file and if it needs decoding
Map<String, String> rawData = stripped.data();
if (decode) {
rawData = decodeData(rawData);
}
if (skipSource(caller, sourceName, environment.getActiveProfiles(), rawData.keySet())) {
return;
}
data.putAll(dataFromSource(caller, rawData, environment));
}
});
return new MultipleSourcesContainer(foundSourceNames, data);
}
It calls :
/*
* This method is only relevant for config server callers, and it's meant to skip some sources
* that config server should not process.
*/
private static boolean skipSource(Caller caller, String sourceName, String[] activeProfiles,
Set<String> rawDataKeys) {
if (caller == Caller.CONFIG_SERVER) {
// for config server this is safe, because it contains a single active profile
String activeProfile = activeProfiles[0];
/*
* the idea here is simple: this logically matches "account" and "account-default",
* and we only want to take "account", since this one matches the
* "config server default profile". As such, "account-default" must be skipped.
*/
if (activeProfile.equals("default")) {
if (sourceName.contains("-default")) {
return true;
}
return false;
}
/*
* here we matched "account" and "account-k8s".
*
* "account", in general, must be processed in the case above, but rawDataKeys
* can contain data like this: "account-k8s.yaml : | ..."
* (a single entry that is a yaml file). Since k8s is not an active profile in the case
* above, we need to do that processing here.
*/
else if (!sourceName.contains("-" + activeProfile)) {
for (String keyValue : rawDataKeys) {
// "-k8s.yaml" for example
String token = "-" + activeProfile;
if (keyValue.endsWith(token + ".yml") ||
keyValue.endsWith(token + ".yaml") || keyValue.endsWith(token + ".properties")) {
return false;
}
}
return true;
}
/*
* "account-k8s" is never skipped. If we reached this point,
* it means it exists in the namespace and must be processed.
*/
else {
return false;
}
}
else {
return false;
}
}
and
private static Map<String, Object> dataFromSource(Caller caller, Map<String, String> rawData,
Environment environment) {
switch (caller) {
case CONFIG -> {
return SourceDataEntriesProcessor.processAllEntries(rawData == null ? Map.of() : rawData, environment);
}
case CONFIG_SERVER -> {
return SourceDataEntriesProcessor.processAllEntries(rawData == null ? Map.of() : rawData,
environment, Caller.CONFIG_SERVER);
}
}
return Map.of();
}
And we would also need to change SourceDataEntriesProcessor::sorted:
/**
* <pre>
* we want to sort entries coming from the k8s source in a specific way:
*
* 1. "application.yaml/yml/properties" have to come first
* (or the value from spring.application.name)
* 2. then profile specific entries, like "application-dev.yaml"
* 3. then plain properties
* </pre>
*/
static List<Map.Entry<String, String>> sorted(Map<String, String> rawData, Environment environment, Caller caller) {
record WeightedEntry(Map.Entry<String, String> entry, int weight) {
}
// we pass empty Strings on purpose, the logic here is either the value of
// "spring.application.name" or literal "application".
String applicationName = ConfigUtils.getApplicationName(environment, "", "");
String[] activeProfiles = environment.getActiveProfiles();
// the order here is important, first has to come "application.yaml" and then
// "application-dev.yaml"
List<String> orderedFileNames = new ArrayList<>();
// this denotes entries that are non profile-based. These are either for plain CONFIG
// queries, or config server queries, but only when "default" profile is present.
boolean includeDataEntry = false;
// we include the source that is == applicationName in two cases:
// 1. when the request comes from CONFIG, meaning how it used to work until now
if (caller == Caller.CONFIG) {
orderedFileNames.add(applicationName);
includeDataEntry = true;
}
/*
* 2. request comes from config server, and it wants to get the "default"
* configuration. The idea here is that config server requests data individually,
* for each separate profile, and 'default' must be always returned too, as a
* separate one. In such a case, there would be a single profile in active
* profiles, called 'default'.
*/
else if (caller == Caller.CONFIG_SERVER && Arrays.asList(environment.getActiveProfiles()).contains("default")) {
orderedFileNames.add(applicationName);
includeDataEntry = true;
}
orderedFileNames.addAll(Arrays.stream(activeProfiles).map(profile -> applicationName + "-" + profile).toList());
int current = orderedFileNames.size() - 1;
List<WeightedEntry> weightedEntries = new ArrayList<>();
if (rawData.keySet().stream().noneMatch(ENDS_WITH)) {
for (Map.Entry<String, String> entry : rawData.entrySet()) {
weightedEntries.add(new WeightedEntry(entry, ++current));
}
}
else {
for (Map.Entry<String, String> entry : rawData.entrySet()) {
String key = entry.getKey();
if (key.endsWith(".yml") || key.endsWith(".yaml") || key.endsWith(".properties")) {
String withoutExtension = key.split("\\.", 2)[0];
int index = orderedFileNames.indexOf(withoutExtension);
if (index >= 0) {
weightedEntries.add(new WeightedEntry(entry, index));
}
else {
LOG.warn("entry : " + key + " will be skipped");
}
}
else {
if (includeDataEntry) {
weightedEntries.add(new WeightedEntry(entry, ++current));
}
}
}
}
return weightedEntries.stream().sorted(Comparator.comparing(WeightedEntry::weight)).map(WeightedEntry::entry)
.toList();
}
There was a problem hiding this comment.
The reason I want to propose such changes is future support. If we add one more "caller" in the future, things are going to get un-manageable, imho. This code :
boolean containsActiveProfile = environment.getActiveProfiles().length == 0
|| Arrays.stream(environment.getActiveProfiles())
.anyMatch(p -> source.contains("-" + p) || "default".equals(p));
if (includeDefaultProfile || containsActiveProfile
|| containsDataWithProfile(rawData, environment.getActiveProfiles())) {
data.putAll(SourceDataEntriesProcessor.processAllEntries(rawData == null ? Map.of() : rawData,
environment, includeDefaultProfile));
}
while readable, its a kind of tricky to get around. At least that was the case for me while trying to understand what it is supposed to do. Also if you agree, I can make these changes in a different PR, whatever works for you
There was a problem hiding this comment.
I agree that the flag appendProfileToName is a bit overloaded in the PR. It is actually the final thing I was trying to make better in the PR.
I thought about an approach similar to what you are proposing here. Ultimately I decided against it because who is to say that next "Caller" does not want to also do the same thing the config server needs? In which case having a "caller" API seems to be a bit fragile.
What I dislike about my approach now is that you could also imaging a situation where you want to append the profile to the property source name, but don't want to exclude the default profile. Right now both pieces of functionality are tied to the one flag appendProfileToName.
It actually feels more "right" to me if we modify KubernetesClientConfigContext and add more to the "context to compute property sources". I think here it might make sense to add a flag to include the default profile.
There was a problem hiding this comment.
well if the next caller wants to do the same thing as the existing config server, I still find it easier to maintain in the long run...
but that is my personal view, at this point in time. It not necessarily correct or it does not mean that it will not change. The good news is that there are at least two people that know exactly what it does :)
I'm more then OK if you want to go with what we already have, without the caller API.
...s-commons/src/main/java/org/springframework/cloud/kubernetes/commons/config/ConfigUtils.java
Show resolved
Hide resolved
| 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"); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
So why was it previously configmap.red.red-with-profile.default?
Some more details for some context...
Assume we have this config map
The k8s config server prior to this change would return this for http://localhost:8888/account/default
There is nothing wrong with this in itself. However if we then make the request http://localhost:8888/account/k8s,prod
This is not how the config server would normally respond. It should return a property source for each yaml file that it found in the configmap. For example this is how the config server would respond when configured to use the native file system with similar files.
This idea is that the config server supplies all the property sources and lets the client resolve which properties take precedence.
The k8s config sever property source basically does what the client should be doing. This makes sense when the app is using the k8s API directly but not for the config server.
The second problem is the fact that the property source name in the response is the same no matter if you make a request for the default profile and for the k8s and prod profiles. This causes an issue on the client because boot uses the property source name to know whether it already has that property source in the environment and will exclude it if it does. Since boot will load configuration for the default profile and then for any active profiles, the default property source gets added to the environment first and then the other property sources will not be added.