From 1f01633d4994095a3ad1975530f1ce52673c4ced Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1clav=20Muzik=C3=A1=C5=99?= Date: Tue, 3 Dec 2024 16:09:53 +0100 Subject: [PATCH] Incorporate review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Václav Muzikář --- .../configuration/ConfigArgsConfigSource.java | 6 +- .../runtime/configuration/Configuration.java | 2 +- .../configuration/KcEnvConfigSource.java | 2 +- .../PropertyMappingInterceptor.java | 2 +- .../mappers/LoggingPropertyMappers.java | 2 +- .../configuration/mappers/PropertyMapper.java | 84 ++++++++++--------- .../configuration/test/ConfigurationTest.java | 6 ++ .../src/test/resources/META-INF/keycloak.conf | 1 + 8 files changed, 58 insertions(+), 47 deletions(-) diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java index c609459a2af7..2978dce0efd2 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java @@ -131,13 +131,13 @@ public void accept(String key, String value) { if (mapper != null) { String to = mapper.getTo(); - String wildcardValue = mapper.getWildcardValue(key).orElse(null); + String mappedKey = mapper.getMappedKey(key).orElse(null); if (to != null) { - properties.put(mapper.getTo(wildcardValue), value); + properties.put(mapper.getTo(mappedKey), value); } - properties.put(mapper.getFrom(wildcardValue), value); + properties.put(mapper.getFrom(mappedKey), value); } } }, ignored -> {}); diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java index 65ffbb2d2b5a..052534b8de8f 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java @@ -120,7 +120,7 @@ public static ConfigValue getKcConfigValue(String propertyName) { * @return a map of config values where the key is the resolved wildcard (e.g. category) and the value is the config value */ public static Map getKcConfigValues(PropertyMapper mapper) { - return mapper.getWildcardValues().stream() + return mapper.getWildcardKeys().stream() .collect(Collectors.toMap(v -> v, v -> getConfigValue(mapper.getFrom(v)))); } diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/KcEnvConfigSource.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/KcEnvConfigSource.java index 51d74be38112..844d3dfc31ce 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/KcEnvConfigSource.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/KcEnvConfigSource.java @@ -56,7 +56,7 @@ private static Map buildProperties() { properties.put(to, value); } - properties.put(mapper.getFrom(mapper.getWildcardValue(key).orElse(null)), value); + properties.put(mapper.getFrom(mapper.getMappedEnvVarKey(key).orElse(null)), value); } else { // most probably an SPI but could be also something else diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/PropertyMappingInterceptor.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/PropertyMappingInterceptor.java index aa4e9469d9cb..b100ac80a790 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/PropertyMappingInterceptor.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/PropertyMappingInterceptor.java @@ -87,7 +87,7 @@ public Iterator iterateNames(ConfigSourceInterceptorContext context) { disableAdditionalNames.set(true); try { mappedWildcardNames = PropertyMappers.getWildcardMappers().stream() - .map(PropertyMapper::getMappedWildcardOptionNames) + .map(PropertyMapper::getToWithWildcards) .flatMap(Set::stream) .toList(); } finally { diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/LoggingPropertyMappers.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/LoggingPropertyMappers.java index 6eace6fee9a3..4ec5a35c9baf 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/LoggingPropertyMappers.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/LoggingPropertyMappers.java @@ -110,7 +110,7 @@ public static PropertyMapper[] getMappers() { fromOption(LoggingOptions.LOG_LEVEL_CATEGORY) .to("quarkus.log.category.\"\".level") .validator(LoggingPropertyMappers::validateCategoryLogLevel) - .wildcardValuesTransformer(LoggingPropertyMappers::getConfiguredLogCategories) + .wildcardKeysTransformer(LoggingPropertyMappers::getConfiguredLogCategories) .transformer((v,c) -> toLevel(v).getName()) .mapFrom(LoggingOptions.LOG_LEVEL, LoggingPropertyMappers::resolveCategoryLogLevelFromParentLogLevelOption) // a fallback to log-level .paramLabel("level") diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java index c847b4f6570c..562f5fb1d1b5 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java @@ -27,7 +27,6 @@ import java.util.Iterator; import java.util.List; -import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -82,13 +81,13 @@ public class PropertyMapper { private Pattern envVarNameWildcardPattern; private Matcher toWildcardMatcher; private Pattern toWildcardPattern; - private Function, Set> wildcardValuesTransformer; + private Function, Set> wildcardKeysTransformer; PropertyMapper(Option option, String to, BooleanSupplier enabled, String enabledWhen, ValueMapper mapper, String mapFrom, ValueMapper parentMapper, String paramLabel, boolean mask, BiConsumer, ConfigValue> validator, - String description, BooleanSupplier required, String requiredWhen, Function, Set> wildcardValuesTransformer) { + String description, BooleanSupplier required, String requiredWhen, Function, Set> wildcardKeysTransformer) { this.option = option; this.to = to == null ? getFrom() : to; this.enabled = enabled; @@ -127,7 +126,7 @@ public class PropertyMapper { } } - this.wildcardValuesTransformer = wildcardValuesTransformer; + this.wildcardKeysTransformer = wildcardKeysTransformer; } } @@ -136,7 +135,7 @@ ConfigValue getConfigValue(ConfigSourceInterceptorContext context) { } ConfigValue getConfigValue(String name, ConfigSourceInterceptorContext context) { - String from = getFrom(getWildcardValue(name).orElse(null)); + String from = getFrom(getMappedKey(name).orElse(null)); if (to != null && to.endsWith(OPTION_PART_SEPARATOR)) { // in case mapping is based on prefixes instead of full property names @@ -175,7 +174,7 @@ ConfigValue getConfigValue(String name, ConfigSourceInterceptorContext context) return context.proceed(name); } - public Set getWildcardValues() { + public Set getWildcardKeys() { if (!hasWildcard()) { return Set.of(); } @@ -183,24 +182,24 @@ public Set getWildcardValues() { // this is not optimal // TODO find an efficient way to get all values that match the wildcard Set values = StreamSupport.stream(Configuration.getPropertyNames().spliterator(), false) - .map(n -> getWildcardValue(n, false)) + .map(n -> getMappedKey(n, true, false, false)) .filter(Optional::isPresent) .map(Optional::get) .collect(Collectors.toSet()); - if (wildcardValuesTransformer != null) { - return wildcardValuesTransformer.apply(values); + if (wildcardKeysTransformer != null) { + return wildcardKeysTransformer.apply(values); } return values; } - public Set getMappedWildcardOptionNames() { + public Set getToWithWildcards() { if (toWildcardMatcher == null) { return Set.of(); } - return getWildcardValues().stream() + return getWildcardKeys().stream() .map(v -> toWildcardMatcher.replaceFirst(v)) .collect(Collectors.toSet()); } @@ -241,10 +240,10 @@ public Class getType() { return this.option.getType(); } - public String getFrom(String wildcardValue) { + public String getFrom(String wildcardKey) { String from = this.option.getKey(); - if (hasWildcard() && wildcardValue != null) { - from = fromWildcardMatcher.replaceFirst(wildcardValue); + if (hasWildcard() && wildcardKey != null) { + from = fromWildcardMatcher.replaceFirst(wildcardKey); } return MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX + from; } @@ -287,10 +286,10 @@ public boolean isRunTime() { return !this.option.isBuildTime(); } - public String getTo(String wildcardValue) { + public String getTo(String wildcardKey) { String to = this.to; - if (hasWildcard() && wildcardValue != null) { - to = toWildcardMatcher.replaceFirst(wildcardValue); + if (hasWildcard() && wildcardKey != null) { + to = toWildcardMatcher.replaceFirst(wildcardKey); } return to; } @@ -340,28 +339,34 @@ public boolean matchesWildcardOptionName(String name) { } /** - * Extracts the wildcard value from the given option name. + * Returns a mapped key for the given option name if a relevant mapping is available, or empty otherwise. + * Currently, it only attempts to extract the wildcard key from the given option name. * E.g. for the option "log-level-" and the option name "log-level-io.quarkus", * the wildcard value would be "io.quarkus". */ - private Optional getWildcardValue(String option, boolean includeMappedToOptions) { + private Optional getMappedKey(String originalKey, boolean tryFrom, boolean tryEnvVar, boolean tryTo) { if (!hasWildcard()) { return Optional.empty(); } - Matcher matcher = fromWildcardPattern.matcher(option); - if (matcher.matches()) { - return Optional.of(matcher.group(1)); + if (tryFrom) { + Matcher matcher = fromWildcardPattern.matcher(originalKey); + if (matcher.matches()) { + return Optional.of(matcher.group(1)); + } } - matcher = envVarNameWildcardPattern.matcher(option); - if (matcher.matches()) { - String value = matcher.group(1); - value = value.toLowerCase().replace("_", "."); // we opiniotatedly convert env var names to CLI format with dots - return Optional.of(value); + if (tryEnvVar) { + Matcher matcher = envVarNameWildcardPattern.matcher(originalKey); + if (matcher.matches()) { + String value = matcher.group(1); + value = value.toLowerCase().replace("_", "."); // we opiniotatedly convert env var names to CLI format with dots + return Optional.of(value); + } } - if (includeMappedToOptions && toWildcardPattern != null && (matcher = toWildcardPattern.matcher(option)).matches()) { + if (tryTo && toWildcardPattern != null) { + Matcher matcher = toWildcardPattern.matcher(originalKey); if (matcher.matches()) { return Optional.of(matcher.group(1)); } @@ -370,13 +375,12 @@ private Optional getWildcardValue(String option, boolean includeMappedTo return Optional.empty(); } - /** - * Extracts the wildcard value from the given option name. - * E.g. for the option "log-level-" and the option name "log-level-io.quarkus", - * the wildcard value would be "io.quarkus". - */ - public Optional getWildcardValue(String option) { - return getWildcardValue(option, true); + public Optional getMappedKey(String originalKey) { + return getMappedKey(originalKey, true, false, true); + } + + public Optional getMappedEnvVarKey(String originalKey) { + return getMappedKey(originalKey, false, true, false); } private ConfigValue transformValue(String name, ConfigValue configValue, ConfigSourceInterceptorContext context, boolean parentValue) { @@ -386,7 +390,7 @@ private ConfigValue transformValue(String name, ConfigValue configValue, ConfigS boolean mapped = false; var theMapper = parentValue ? this.parentMapper : this.mapper; if (theMapper != null && (!name.equals(getFrom()) || parentValue)) { - String nameForMapper = hasWildcard() ? getWildcardValue(name).orElse(name) : name; + String nameForMapper = getMappedKey(name).orElse(name); mappedValue = theMapper.map(nameForMapper, value, context); mapped = true; } @@ -466,7 +470,7 @@ public static class Builder { private String description; private BooleanSupplier isRequired = () -> false; private String requiredWhen = ""; - private Function, Set> wildcardValuesTransformer; + private Function, Set> wildcardKeysTransformer; public Builder(Option option) { this.option = option; @@ -599,8 +603,8 @@ public Builder addValidateEnabled(BooleanSupplier isEnabled, String enabledWh return this; } - public Builder wildcardValuesTransformer(Function, Set> wildcardValuesTransformer) { - this.wildcardValuesTransformer = wildcardValuesTransformer; + public Builder wildcardKeysTransformer(Function, Set> wildcardValuesTransformer) { + this.wildcardKeysTransformer = wildcardValuesTransformer; return this; } @@ -608,7 +612,7 @@ public PropertyMapper build() { if (paramLabel == null && Boolean.class.equals(option.getType())) { paramLabel = Boolean.TRUE + "|" + Boolean.FALSE; } - return new PropertyMapper<>(option, to, isEnabled, enabledWhen, mapper, mapFrom, parentMapper, paramLabel, isMasked, validator, description, isRequired, requiredWhen, wildcardValuesTransformer); + return new PropertyMapper<>(option, to, isEnabled, enabledWhen, mapper, mapFrom, parentMapper, paramLabel, isMasked, validator, description, isRequired, requiredWhen, wildcardKeysTransformer); } } diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/ConfigurationTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/ConfigurationTest.java index 068b2dbb27c5..e18b02468aef 100644 --- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/ConfigurationTest.java +++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/ConfigurationTest.java @@ -557,4 +557,10 @@ public void testWildcardOptionFromConfigFile() { SmallRyeConfig config = createConfig(); assertEquals("DEBUG", config.getConfigValue("quarkus.log.category.\"io.k8s\".level").getValue()); } + + @Test + public void testWildcardPropertiesDontMatchEnvVarsFormat() { + SmallRyeConfig config = createConfig(); + assertEquals("INFO", config.getConfigValue("quarkus.log.category.\"io.quarkus\".level").getValue()); + } } diff --git a/quarkus/runtime/src/test/resources/META-INF/keycloak.conf b/quarkus/runtime/src/test/resources/META-INF/keycloak.conf index 4244384831d4..f2ee6129cf1a 100644 --- a/quarkus/runtime/src/test/resources/META-INF/keycloak.conf +++ b/quarkus/runtime/src/test/resources/META-INF/keycloak.conf @@ -2,6 +2,7 @@ spi-hostname-default-frontend-url = ${keycloak.frontendUrl:http://filepropdefaul %user-profile.spi-hostname-default-frontend-url = http://filepropprofile.unittest log-level=${SOME_LOG_LEVEL:info} log-level-io.k8s=${SOME_CATEGORY_LOG_LEVEL} +KC_LOG_LEVEL_IO_QUARKUS=trace config-keystore=src/test/resources/keystore config-keystore-password=secret