Skip to content

Commit

Permalink
Merge pull request #40079 from gsmet/config-leak-3.8
Browse files Browse the repository at this point in the history
[3.8] Do not record local sources in runtime config defaults
  • Loading branch information
gsmet authored Apr 16, 2024
2 parents dc6e742 + 2b24dc8 commit cb80cee
Show file tree
Hide file tree
Showing 12 changed files with 141 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
import static io.quarkus.deployment.util.ReflectUtil.unwrapInvocationTargetException;
import static io.smallrye.config.ConfigMappings.ConfigClassWithPrefix.configClassWithPrefix;
import static io.smallrye.config.Expressions.withoutExpansion;
import static io.smallrye.config.PropertiesConfigSourceProvider.classPathSources;
import static io.smallrye.config.SmallRyeConfig.SMALLRYE_CONFIG_PROFILE;
import static io.smallrye.config.SmallRyeConfig.SMALLRYE_CONFIG_PROFILE_PARENT;
import static io.smallrye.config.SmallRyeConfigBuilder.META_INF_MICROPROFILE_CONFIG_PROPERTIES;
import static java.util.stream.Collectors.toSet;

import java.io.IOException;
Expand All @@ -29,6 +33,7 @@
import java.util.SortedSet;
import java.util.TreeMap;

import org.eclipse.microprofile.config.Config;
import org.eclipse.microprofile.config.inject.ConfigProperty;
import org.eclipse.microprofile.config.spi.ConfigSource;
import org.eclipse.microprofile.config.spi.Converter;
Expand Down Expand Up @@ -81,6 +86,7 @@
import io.smallrye.config.SmallRyeConfig;
import io.smallrye.config.SmallRyeConfigBuilder;
import io.smallrye.config.SysPropConfigSource;
import io.smallrye.config.common.AbstractConfigSource;
import io.smallrye.config.common.utils.StringUtil;

/**
Expand Down Expand Up @@ -508,18 +514,19 @@ ReadResult run() {
nameBuilder.setLength(0);
}

SmallRyeConfig runtimeConfig = getConfigForRuntimeRecording();

// Register defaults for Roots
allBuildTimeValues.putAll(getDefaults(buildTimePatternMap));
buildTimeRunTimeValues.putAll(getDefaults(buildTimeRunTimePatternMap));
runTimeDefaultValues.putAll(getDefaults(runTimePatternMap));
allBuildTimeValues.putAll(getDefaults(config, buildTimePatternMap));
buildTimeRunTimeValues.putAll(getDefaults(config, buildTimeRunTimePatternMap));
runTimeDefaultValues.putAll(getDefaults(runtimeConfig, runTimePatternMap));

// Register defaults for Mappings
// Runtime defaults are added in ConfigGenerationBuildStep.generateBuilders to include user mappings
for (ConfigClassWithPrefix buildTimeRunTimeMapping : buildTimeRunTimeMappings) {
buildTimeRunTimeValues.putAll(ConfigMappings.getDefaults(buildTimeRunTimeMapping));
}

SmallRyeConfig runtimeDefaultsConfig = getConfigForRuntimeDefaults();
Set<String> registeredRoots = allRoots.stream().map(RootDefinition::getName).collect(toSet());
registeredRoots.add("quarkus");
Set<String> allProperties = getAllProperties(registeredRoots);
Expand Down Expand Up @@ -599,7 +606,7 @@ ReadResult run() {
knownProperty = knownProperty || matched != null;
if (matched != null) {
// it's a run-time default (record for later)
ConfigValue configValue = withoutExpansion(() -> runtimeDefaultsConfig.getConfigValue(propertyName));
ConfigValue configValue = withoutExpansion(() -> runtimeConfig.getConfigValue(propertyName));
if (configValue.getValue() != null) {
runTimeValues.put(configValue.getNameProfiled(), configValue.getValue());
}
Expand All @@ -610,7 +617,7 @@ ReadResult run() {
}
} else {
// it's not managed by us; record it
ConfigValue configValue = withoutExpansion(() -> runtimeDefaultsConfig.getConfigValue(propertyName));
ConfigValue configValue = withoutExpansion(() -> runtimeConfig.getConfigValue(propertyName));
if (configValue.getValue() != null) {
runTimeValues.put(configValue.getNameProfiled(), configValue.getValue());
}
Expand All @@ -637,7 +644,7 @@ ReadResult run() {
for (String property : mappedProperties) {
unknownBuildProperties.remove(property);
ConfigValue value = config.getConfigValue(property);
if (value != null && value.getRawValue() != null) {
if (value.getRawValue() != null) {
allBuildTimeValues.put(property, value.getRawValue());
}
}
Expand All @@ -649,7 +656,7 @@ ReadResult run() {
for (String property : mappedProperties) {
unknownBuildProperties.remove(property);
ConfigValue value = config.getConfigValue(property);
if (value != null && value.getRawValue() != null) {
if (value.getRawValue() != null) {
allBuildTimeValues.put(property, value.getRawValue());
buildTimeRunTimeValues.put(property, value.getRawValue());
}
Expand All @@ -661,8 +668,8 @@ ReadResult run() {
Set<String> mappedProperties = ConfigMappings.mappedProperties(mapping, allProperties);
for (String property : mappedProperties) {
unknownBuildProperties.remove(property);
ConfigValue value = config.getConfigValue(property);
if (value != null && value.getRawValue() != null) {
ConfigValue value = runtimeConfig.getConfigValue(property);
if (value.getRawValue() != null) {
runTimeValues.put(property, value.getRawValue());
}
}
Expand Down Expand Up @@ -1066,25 +1073,57 @@ private Set<String> getAllProperties(final Set<String> registeredRoots) {

/**
* Use this Config instance to record the runtime default values. We cannot use the main Config
* instance because it may record values coming from the EnvSource in build time. Environment variable values
* may be completely different between build and runtime, so it doesn't make sense to record these.
* <br>
* We do exclude the properties coming from the EnvSource, but a call to getValue may still provide a result
* coming from the EnvSource, so we need to exclude it from the sources when recording values for runtime.
* <br>
* We also do not want to completely exclude the EnvSource, because it may provide values for the build. This
* is only specific when recording the defaults.
* instance because it may record values coming from local development sources (Environment Variables,
* System Properties, etc.) in build time. Local config source values may be completely different between the
* build environment and the runtime environment, so it doesn't make sense to record these.
*
* @return a new SmallRye instance without the EnvSources.
*/
private SmallRyeConfig getConfigForRuntimeDefaults() {
private SmallRyeConfig getConfigForRuntimeRecording() {
SmallRyeConfigBuilder builder = ConfigUtils.emptyConfigBuilder();
builder.getSources().clear();
builder.getSourceProviders().clear();
builder.setAddDefaultSources(false)
// Customizers may duplicate sources, but not much we can do about it, we need to run them
.addDiscoveredCustomizers()
// Readd microprofile-config.properties, because we disabled the default sources
.withSources(classPathSources(META_INF_MICROPROFILE_CONFIG_PROPERTIES, classLoader));

// TODO - Should we reset quarkus.config.location to not record from these sources?
for (ConfigSource configSource : config.getConfigSources()) {
if (configSource instanceof SysPropConfigSource) {
continue;
}
if (configSource instanceof EnvConfigSource) {
continue;
}
if ("PropertiesConfigSource[source=Build system]".equals(configSource.getName())) {
continue;
}
builder.withSources(configSource);
}
builder.withSources(new AbstractConfigSource("Profiles", Integer.MAX_VALUE) {
private final Set<String> profiles = Set.of(
"quarkus.profile",
"quarkus.config.profile.parent",
"quarkus.test.profile",
SMALLRYE_CONFIG_PROFILE,
SMALLRYE_CONFIG_PROFILE_PARENT,
Config.PROFILE);

@Override
public Set<String> getPropertyNames() {
return Collections.emptySet();
}

@Override
public String getValue(final String propertyName) {
if (profiles.contains(propertyName)) {
return config.getConfigValue(propertyName).getValue();
}
return null;
};
});
return builder.build();
}

Expand All @@ -1102,13 +1141,15 @@ private Map<String, String> filterActiveProfileProperties(final Map<String, Stri
return properties;
}

private Map<String, String> getDefaults(final ConfigPatternMap<Container> patternMap) {
private static Map<String, String> getDefaults(final SmallRyeConfig config,
final ConfigPatternMap<Container> patternMap) {
Map<String, String> defaultValues = new TreeMap<>();
getDefaults(defaultValues, new StringBuilder(), patternMap);
getDefaults(config, defaultValues, new StringBuilder(), patternMap);
return defaultValues;
}

private void getDefaults(
private static void getDefaults(
final SmallRyeConfig config,
final Map<String, String> defaultValues,
final StringBuilder propertyName,
final ConfigPatternMap<Container> patternMap) {
Expand All @@ -1135,7 +1176,7 @@ private void getDefaults(
}

for (String childName : patternMap.childNames()) {
getDefaults(defaultValues,
getDefaults(config, defaultValues,
new StringBuilder(propertyName).append(childName.equals(ConfigPatternMap.WILD_CARD) ? "*" : childName),
patternMap.getChild(childName));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,7 @@
@IfBuildProfile("foo")
@ApplicationScoped
public class HelloService {

@ConfigProperty(name = "name")
String name;

public String name() {
return name;
return "from foo";
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
package org.acme;

import io.quarkus.arc.profile.IfBuildProfile;
import org.eclipse.microprofile.config.inject.ConfigProperty;

import jakarta.enterprise.context.ApplicationScoped;

@IfBuildProfile("foo")
@ApplicationScoped
public class HelloService {

@ConfigProperty(name = "name")
String name;

public String name() {
return name;
return "from foo";
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ quarkus.arc.unremovable-types=foo
# The YAML source may add an indexed property (depending on how the YAML is laid out). This is not supported by @ConfigRoot
quarkus.arc.unremovable-types[0]=foo

### Do not record env values in build time
bt.do.not.record=properties
### recording
bt.ok.to.record=from-app
%test.bt.profile.record=properties

### mappings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.extest.runtime.config.AnotherPrefixConfig;
import io.quarkus.extest.runtime.config.DoNotRecordEnvConfigSource;
import io.quarkus.extest.runtime.config.MyEnum;
import io.quarkus.extest.runtime.config.NestedConfig;
import io.quarkus.extest.runtime.config.ObjectOfValue;
Expand All @@ -52,9 +53,9 @@ public class ConfiguredBeanTest {
static final QuarkusUnitTest TEST = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(ConfiguredBean.class)
// Don't change this to types, because of classloader class cast exception.
.addAsServiceProvider("org.eclipse.microprofile.config.spi.ConfigSource",
"io.quarkus.extest.runtime.config.OverrideBuildTimeConfigSource")
.addAsServiceProvider(ConfigSource.class,
OverrideBuildTimeConfigSource.class,
DoNotRecordEnvConfigSource.class)
.addAsResource("application.properties"));

@Inject
Expand Down Expand Up @@ -363,6 +364,16 @@ public void testProfileDefaultValuesSource() {
assertEquals("5678", defaultValues.getValue("%dev.my.prop"));
assertEquals("1234", defaultValues.getValue("%test.my.prop"));
assertEquals("1234", config.getValue("my.prop", String.class));

// runtime properties coming from env must not be recorded
assertNull(defaultValues.getValue("should.not.be.recorded"));
assertNull(defaultValues.getValue("SHOULD_NOT_BE_RECORDED"));
assertNull(defaultValues.getValue("quarkus.rt.do-not-record"));
assertEquals("value", config.getRawValue("quarkus.rt.do-not-record"));
assertNull(defaultValues.getValue("quarkus.mapping.rt.do-not-record"));
assertNull(defaultValues.getValue("%prod.quarkus.mapping.rt.do-not-record"));
assertNull(defaultValues.getValue("%dev.quarkus.mapping.rt.do-not-record"));
assertEquals("value", config.getRawValue("quarkus.mapping.rt.do-not-record"));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,15 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.extest.runtime.config.EnvBuildTimeConfigSource;
import io.quarkus.test.QuarkusUnitTest;
import io.smallrye.config.SmallRyeConfig;

public class RuntimeDefaultsTest {
@RegisterExtension
static final QuarkusUnitTest TEST = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
// Don't change this to types, because of classloader class cast exception.
.addAsServiceProvider("org.eclipse.microprofile.config.spi.ConfigSource",
"io.quarkus.extest.runtime.config.EnvBuildTimeConfigSource")
.addAsServiceProvider(ConfigSource.class, EnvBuildTimeConfigSource.class)
.addAsResource("application.properties"));

@Inject
Expand All @@ -31,16 +30,33 @@ public class RuntimeDefaultsTest {
void doNotRecordEnvRuntimeDefaults() {
Optional<ConfigSource> defaultValues = config.getConfigSource("DefaultValuesConfigSource");
assertTrue(defaultValues.isPresent());
assertEquals("rtStringOptValue", defaultValues.get().getValue("quarkus.rt.rt-string-opt"));
assertEquals("properties", defaultValues.get().getValue("bt.do.not.record"));
// Do not record Env values for runtime
assertNull(defaultValues.get().getValue("quarkus.mapping.rt.do-not-record"));
assertEquals("value", config.getRawValue("quarkus.mapping.rt.do-not-record"));
// Property available in both Env and application.properties, ok to record application.properties value
assertEquals("from-app", defaultValues.get().getValue("bt.ok.to.record"));
// You still get the value from Env
assertEquals("from-env", config.getRawValue("bt.ok.to.record"));
// Do not record any of the other properties
assertNull(defaultValues.get().getValue(("do.not.record")));
assertNull(defaultValues.get().getValue(("DO_NOT_RECORD")));
assertEquals("value", config.getRawValue("do.not.record"));
}

@Test
void doNotRecordActiveUnprofiledPropertiesDefaults() {
Optional<ConfigSource> defaultValues = config.getConfigSource("DefaultValuesConfigSource");
assertTrue(defaultValues.isPresent());
assertEquals("properties", config.getRawValue("bt.profile.record"));
// Property needs to be recorded as is, including the profile name
assertEquals("properties", defaultValues.get().getValue("%test.bt.profile.record"));
assertNull(defaultValues.get().getValue("bt.profile.record"));
}

@Test
void recordProfile() {
Optional<ConfigSource> defaultValues = config.getConfigSource("DefaultValuesConfigSource");
assertTrue(defaultValues.isPresent());
assertEquals("record", config.getRawValue("quarkus.profile"));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package io.quarkus.extest.runtime.config;

import java.util.Map;

import io.quarkus.runtime.annotations.StaticInitSafe;
import io.smallrye.config.EnvConfigSource;

@StaticInitSafe
public class DoNotRecordEnvConfigSource extends EnvConfigSource {
public DoNotRecordEnvConfigSource() {
super(Map.of(
"SHOULD_NOT_BE_RECORDED", "value",
"should.not.be.recorded", "value",
"quarkus.rt.do-not-record", "value",
"quarkus.mapping.rt.do-not-record", "value",
"%dev.quarkus.mapping.rt.do-not-record", "dev",
"_PROD_QUARKUS_MAPPING_RT_DO_NOT_RECORD", "prod"), 300);
}
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
package io.quarkus.extest.runtime.config;

import java.util.HashMap;
import java.util.Map;

import io.smallrye.config.EnvConfigSource;

public class EnvBuildTimeConfigSource extends EnvConfigSource {
public EnvBuildTimeConfigSource() {
super(new HashMap<>() {
{
put("QUARKUS_RT_RT_STRING_OPT", "changed");
put("BT_DO_NOT_RECORD", "env-source");
}
}, Integer.MAX_VALUE);
super(Map.of(
"QUARKUS_PROFILE", "record",
"QUARKUS_MAPPING_RT_DO_NOT_RECORD", "value",
"BT_OK_TO_RECORD", "from-env",
"BT_DO_NOT_RECORD", "value",
"DO_NOT_RECORD", "value"), Integer.MAX_VALUE);
}
}
Loading

0 comments on commit cb80cee

Please sign in to comment.