This is an automated email from the ASF dual-hosted git repository. rsivaram pushed a commit to branch 2.3 in repository https://gitbox.apache.org/repos/asf/kafka.git
commit dd763fbd6a9b1d712b1c3dbd1fbb497943cd28a6 Author: tadsul <43974298+tad...@users.noreply.github.com> AuthorDate: Mon Jun 3 04:43:11 2019 -0700 KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) (#6795) Since the originals map passed to AbstractConfig constructor may be immutable, avoid updating this map while resolving indirect config variables. Instead a new ResolvingMap instance is now used to store resolved configs. Reviewers: Randall Hauch <rha...@gmail.com>, Boyang Chen <bche...@outlook.com>, Rajini Sivaram <rajinisiva...@googlemail.com> --- .../apache/kafka/common/config/AbstractConfig.java | 37 +++++++++++++++++++--- .../kafka/common/config/AbstractConfigTest.java | 14 ++++++++ gradle/spotbugs-exclude.xml | 7 ++++ 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java b/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java index 555b634..13865d0 100644 --- a/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java +++ b/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java @@ -103,7 +103,6 @@ public class AbstractConfig { throw new ConfigException(entry.getKey().toString(), entry.getValue(), "Key must be a string."); this.originals = resolveConfigVariables(configProviderProps, (Map<String, Object>) originals); - this.values = definition.parse(this.originals); Map<String, Object> configUpdates = postProcessParsedConfig(Collections.unmodifiableMap(this.values)); for (Map.Entry<String, Object> update : configUpdates.entrySet()) { @@ -459,10 +458,11 @@ public class AbstractConfig { private Map<String, ?> resolveConfigVariables(Map<String, ?> configProviderProps, Map<String, Object> originals) { Map<String, String> providerConfigString; Map<String, ?> configProperties; - + Map<String, Object> resolvedOriginals = new HashMap<>(); // As variable configs are strings, parse the originals and obtain the potential variable configs. Map<String, String> indirectVariables = extractPotentialVariables(originals); + resolvedOriginals.putAll(originals); if (configProviderProps == null || configProviderProps.isEmpty()) { providerConfigString = indirectVariables; configProperties = originals; @@ -475,10 +475,12 @@ public class AbstractConfig { if (!providers.isEmpty()) { ConfigTransformer configTransformer = new ConfigTransformer(providers); ConfigTransformerResult result = configTransformer.transform(indirectVariables); - originals.putAll(result.data()); + if (!result.data().isEmpty()) { + resolvedOriginals.putAll(result.data()); + } } - return originals; + return new ResolvingMap<>(resolvedOriginals, originals); } private Map<String, Object> configProviderProperties(String configProviderPrefix, Map<String, ?> providerConfigProperties) { @@ -585,4 +587,31 @@ public class AbstractConfig { return super.get(key); } } + + /** + * ResolvingMap keeps a track of the original map instance and the resolved configs. + * The originals are tracked in a separate nested map and may be a `RecordingMap`; thus + * any access to a value for a key needs to be recorded on the originals map. + * The resolved configs are kept in the inherited map and are therefore mutable, though any + * mutations are not applied to the originals. + */ + private static class ResolvingMap<V> extends HashMap<String, V> { + + private final Map<String, ?> originals; + + ResolvingMap(Map<String, ? extends V> resolved, Map<String, ?> originals) { + super(resolved); + this.originals = Collections.unmodifiableMap(originals); + } + + @Override + public V get(Object key) { + if (key instanceof String && originals.containsKey(key)) { + // Intentionally ignore the result; call just to mark the original entry as used + originals.get(key); + } + // But always use the resolved entry + return super.get(key); + } + } } diff --git a/clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java b/clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java index a007fd3..33aaac5 100644 --- a/clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java +++ b/clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java @@ -364,6 +364,20 @@ public class AbstractConfigTest { } @Test + public void testImmutableOriginalsWithConfigProvidersProps() { + // Test Case: Valid Test Case for ConfigProviders as a separate variable + Properties providers = new Properties(); + providers.put("config.providers", "file"); + providers.put("config.providers.file.class", "org.apache.kafka.common.config.provider.MockFileConfigProvider"); + Properties props = new Properties(); + props.put("sasl.kerberos.key", "${file:/usr/kerberos:key}"); + Map<?, ?> immutableMap = Collections.unmodifiableMap(props); + Map<String, ?> provMap = convertPropertiesToMap(providers); + TestIndirectConfigResolution config = new TestIndirectConfigResolution(immutableMap, provMap); + assertEquals(config.originals().get("sasl.kerberos.key"), "testKey"); + } + + @Test public void testAutoConfigResolutionWithMultipleConfigProviders() { // Test Case: Valid Test Case With Multiple ConfigProviders as a separate variable Properties providers = new Properties(); diff --git a/gradle/spotbugs-exclude.xml b/gradle/spotbugs-exclude.xml index 721b05e..60e2eb2 100644 --- a/gradle/spotbugs-exclude.xml +++ b/gradle/spotbugs-exclude.xml @@ -332,4 +332,11 @@ For a detailed description of spotbugs bug categories, see https://spotbugs.read <!-- END Suppress warnings for unused members that are undetectably used by Jackson --> + <Match> + <!-- Suppress a warning about ignoring return value because this is intentional. --> + <Class name="org.apache.kafka.common.config.AbstractConfig$ResolvingMap"/> + <Method name="get"/> + <Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/> + </Match> + </FindBugsFilter>