Settings: Introduce settings updater for a list of settings (#28338)

This introduces a settings updater that allows to specify a list of
settings. Whenever one of those settings changes, the whole block of
settings is passed to the consumer.

This also fixes an issue with affix settings, when used in combination
with group settings, which could result in no found settings when used
to get a setting for a namespace.

Lastly logging has been slightly changed, so that filtered settings now
only log the setting key.

Another bug has been fixed for the mock log appender, which did not
work, when checking for the exact message.

Closes #28047

Author: spinscale

server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

@@ -194,6 +194,16 @@ public synchronized <T> void addSettingsUpdateConsumer(Setting<T> setting, Consu
         addSettingsUpdater(setting.newUpdater(consumer, logger, validator));
     }
 
+    /**
+     * Adds a settings consumer that is only executed if any setting in the supplied list of settings is changed. In that case all the
+     * settings are specified in the argument are returned.
+     *
+     * Also automatically adds empty consumers for all settings in order to activate logging
+     */
+    public synchronized void addSettingsUpdateConsumer(Consumer<Settings> consumer, List<? extends Setting<?>> settings) {
+        addSettingsUpdater(Setting.groupedSettingsUpdater(consumer, logger, settings));
+    }
+
     /**
      * Adds a settings consumer for affix settings. Affix settings have a namespace associated to it that needs to be available to the
      * consumer in order to be processed correctly.

server/src/main/java/org/elasticsearch/common/settings/Setting.java

@@ -509,10 +509,10 @@ public Tuple<A, B> getValue(Settings current, Settings previous) {
             @Override
             public void apply(Tuple<A, B> value, Settings current, Settings previous) {
                 if (aSettingUpdater.hasChanged(current, previous)) {
-                    logger.info("updating [{}] from [{}] to [{}]", aSetting.key, aSetting.getRaw(previous), aSetting.getRaw(current));
+                    logSettingUpdate(aSetting, current, previous, logger);
                 }
                 if (bSettingUpdater.hasChanged(current, previous)) {
-                    logger.info("updating [{}] from [{}] to [{}]", bSetting.key, bSetting.getRaw(previous), bSetting.getRaw(current));
+                    logSettingUpdate(bSetting, current, previous, logger);
                 }
                 consumer.accept(value.v1(), value.v2());
             }
@@ -524,6 +524,46 @@ public String toString() {
         };
     }
 
+    static AbstractScopedSettings.SettingUpdater<Settings> groupedSettingsUpdater(Consumer<Settings> consumer, Logger logger,
+                                                                                  final List<? extends Setting<?>> configuredSettings) {
+
+        return new AbstractScopedSettings.SettingUpdater<Settings>() {
+
+            private Settings get(Settings settings) {
+                return settings.filter(s -> {
+                    for (Setting<?> setting : configuredSettings) {
+                        if (setting.key.match(s)) {
+                            return true;
+                        }
+                    }
+                    return false;
+                });
+            }
+
+            @Override
+            public boolean hasChanged(Settings current, Settings previous) {
+                Settings currentSettings = get(current);
+                Settings previousSettings = get(previous);
+                return currentSettings.equals(previousSettings) == false;
+            }
+
+            @Override
+            public Settings getValue(Settings current, Settings previous) {
+                return get(current);
+            }
+
+            @Override
+            public void apply(Settings value, Settings current, Settings previous) {
+                consumer.accept(value);
+            }
+
+            @Override
+            public String toString() {
+                return "Updater grouped: " + configuredSettings.stream().map(Setting::getKey).collect(Collectors.joining(", "));
+            }
+        };
+    }
+
     public static class AffixSetting<T> extends Setting<T> {
         private final AffixKey key;
         private final Function<String, Setting<T>> delegateFactory;
@@ -541,7 +581,7 @@ boolean isGroupSetting() {
         }
 
         private Stream<String> matchStream(Settings settings) {
-            return settings.keySet().stream().filter((key) -> match(key)).map(settingKey -> key.getConcreteString(settingKey));
+            return settings.keySet().stream().filter(this::match).map(key::getConcreteString);
         }
 
         public Set<String> getSettingsDependencies(String settingsKey) {
@@ -812,9 +852,7 @@ public Settings getValue(Settings current, Settings previous) {
 
                 @Override
                 public void apply(Settings value, Settings current, Settings previous) {
-                    if (logger.isInfoEnabled()) { // getRaw can create quite some objects
-                        logger.info("updating [{}] from [{}] to [{}]", key, getRaw(previous), getRaw(current));
-                    }
+                    Setting.logSettingUpdate(GroupSetting.this, current, previous, logger);
                     consumer.accept(value);
                 }
 
@@ -902,7 +940,7 @@ public T getValue(Settings current, Settings previous) {
 
         @Override
         public void apply(T value, Settings current, Settings previous) {
-            logger.info("updating [{}] from [{}] to [{}]", key, getRaw(previous), getRaw(current));
+            logSettingUpdate(Setting.this, current, previous, logger);
             consumer.accept(value);
         }
     }
@@ -1138,6 +1176,16 @@ private static String arrayToParsableString(List<String> array) {
         }
     }
 
+    static void logSettingUpdate(Setting setting, Settings current, Settings previous, Logger logger) {
+        if (logger.isInfoEnabled()) {
+            if (setting.isFiltered()) {
+                logger.info("updating [{}]", setting.key);
+            } else {
+                logger.info("updating [{}] from [{}] to [{}]", setting.key, setting.getRaw(previous), setting.getRaw(current));
+            }
+        }
+    }
+
     public static Setting<Settings> groupSetting(String key, Property... properties) {
         return groupSetting(key, (s) -> {}, properties);
     }
@@ -1308,8 +1356,8 @@ public static final class AffixKey implements Key {
             if (suffix == null) {
                 pattern = Pattern.compile("(" + Pattern.quote(prefix) + "((?:[-\\w]+[.])*[-\\w]+$))");
             } else {
-                // the last part of this regexp is for lists since they are represented as x.${namespace}.y.1, x.${namespace}.y.2
-                pattern = Pattern.compile("(" + Pattern.quote(prefix) + "([-\\w]+)\\." + Pattern.quote(suffix) + ")(?:\\.\\d+)?");
+                // the last part of this regexp is to support both list and group keys
+                pattern = Pattern.compile("(" + Pattern.quote(prefix) + "([-\\w]+)\\." + Pattern.quote(suffix) + ")(?:\\..*)?");
             }
         }
 

server/src/test/java/org/elasticsearch/common/settings/SettingTests.java

@@ -38,6 +38,7 @@
 
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
@@ -712,4 +713,79 @@ public void testTimeValue() {
         assertThat(setting.get(Settings.EMPTY).getMillis(), equalTo(random.getMillis() * factor));
     }
 
+    public void testSettingsGroupUpdater() {
+        Setting<Integer> intSetting = Setting.intSetting("prefix.foo", 1, Property.NodeScope, Property.Dynamic);
+        Setting<Integer> intSetting2 = Setting.intSetting("prefix.same", 1, Property.NodeScope, Property.Dynamic);
+        AbstractScopedSettings.SettingUpdater<Settings> updater = Setting.groupedSettingsUpdater(s -> {}, logger,
+            Arrays.asList(intSetting, intSetting2));
+
+        Settings current = Settings.builder().put("prefix.foo", 123).put("prefix.same", 5555).build();
+        Settings previous = Settings.builder().put("prefix.foo", 321).put("prefix.same", 5555).build();
+        assertTrue(updater.apply(current, previous));
+    }
+
+    public void testSettingsGroupUpdaterRemoval() {
+        Setting<Integer> intSetting = Setting.intSetting("prefix.foo", 1, Property.NodeScope, Property.Dynamic);
+        Setting<Integer> intSetting2 = Setting.intSetting("prefix.same", 1, Property.NodeScope, Property.Dynamic);
+        AbstractScopedSettings.SettingUpdater<Settings> updater = Setting.groupedSettingsUpdater(s -> {}, logger,
+            Arrays.asList(intSetting, intSetting2));
+
+        Settings current = Settings.builder().put("prefix.same", 5555).build();
+        Settings previous = Settings.builder().put("prefix.foo", 321).put("prefix.same", 5555).build();
+        assertTrue(updater.apply(current, previous));
+    }
+
+    public void testSettingsGroupUpdaterWithAffixSetting() {
+        Setting<Integer> intSetting = Setting.intSetting("prefix.foo", 1, Property.NodeScope, Property.Dynamic);
+        Setting.AffixSetting<String> prefixKeySetting =
+            Setting.prefixKeySetting("prefix.foo.bar.", key -> Setting.simpleString(key, Property.NodeScope, Property.Dynamic));
+        Setting.AffixSetting<String> affixSetting =
+            Setting.affixKeySetting("prefix.foo.", "suffix", key -> Setting.simpleString(key,Property.NodeScope, Property.Dynamic));
+
+        AbstractScopedSettings.SettingUpdater<Settings> updater = Setting.groupedSettingsUpdater(s -> {}, logger,
+            Arrays.asList(intSetting, prefixKeySetting, affixSetting));
+
+        Settings.Builder currentSettingsBuilder = Settings.builder()
+            .put("prefix.foo.bar.baz", "foo")
+            .put("prefix.foo.infix.suffix", "foo");
+        Settings.Builder previousSettingsBuilder = Settings.builder()
+            .put("prefix.foo.bar.baz", "foo")
+            .put("prefix.foo.infix.suffix", "foo");
+        boolean removePrefixKeySetting = randomBoolean();
+        boolean changePrefixKeySetting = randomBoolean();
+        boolean removeAffixKeySetting = randomBoolean();
+        boolean changeAffixKeySetting = randomBoolean();
+        boolean removeAffixNamespace = randomBoolean();
+
+        if (removePrefixKeySetting) {
+            previousSettingsBuilder.remove("prefix.foo.bar.baz");
+        }
+        if (changePrefixKeySetting) {
+            currentSettingsBuilder.put("prefix.foo.bar.baz", "bar");
+        }
+        if (removeAffixKeySetting) {
+            previousSettingsBuilder.remove("prefix.foo.infix.suffix");
+        }
+        if (changeAffixKeySetting) {
+            currentSettingsBuilder.put("prefix.foo.infix.suffix", "bar");
+        }
+        if (removeAffixKeySetting == false && changeAffixKeySetting == false && removeAffixNamespace) {
+            currentSettingsBuilder.remove("prefix.foo.infix.suffix");
+            currentSettingsBuilder.put("prefix.foo.infix2.suffix", "bar");
+            previousSettingsBuilder.put("prefix.foo.infix2.suffix", "bar");
+        }
+
+        boolean expectedChange = removeAffixKeySetting || removePrefixKeySetting || changeAffixKeySetting || changePrefixKeySetting
+            || removeAffixNamespace;
+        assertThat(updater.apply(currentSettingsBuilder.build(), previousSettingsBuilder.build()), is(expectedChange));
+    }
+
+    public void testAffixNamespacesWithGroupSetting() {
+        final Setting.AffixSetting<Settings> affixSetting =
+            Setting.affixKeySetting("prefix.","suffix",
+                (key) -> Setting.groupSetting(key + ".", Setting.Property.Dynamic, Setting.Property.NodeScope));
+
+        assertThat(affixSetting.getNamespaces(Settings.builder().put("prefix.infix.suffix", "anything").build()), hasSize(1));
+        assertThat(affixSetting.getNamespaces(Settings.builder().put("prefix.infix.suffix.anything", "anything").build()), hasSize(1));
+    }
 }

server/src/test/java/org/elasticsearch/common/settings/SettingsFilterTests.java

@@ -18,16 +18,22 @@
  */
 package org.elasticsearch.common.settings;
 
-import org.elasticsearch.common.Strings;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Logger;
+import org.elasticsearch.common.logging.Loggers;
+import org.elasticsearch.common.logging.ServerLoggers;
+import org.elasticsearch.common.settings.Setting.Property;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.MockLogAppender;
 import org.elasticsearch.test.rest.FakeRestRequest;
 
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.HashSet;
+import java.util.function.Consumer;
 
 import static org.hamcrest.CoreMatchers.equalTo;
 
@@ -100,7 +106,43 @@ public void testSettingsFiltering() throws IOException {
                 .build(),
             "a.b.*.d"
         );
+    }
+
+    public void testFilteredSettingIsNotLogged() throws Exception {
+        Settings oldSettings = Settings.builder().put("key", "old").build();
+        Settings newSettings = Settings.builder().put("key", "new").build();
+
+        Setting<String> filteredSetting = Setting.simpleString("key", Property.Filtered);
+        assertExpectedLogMessages((testLogger) -> Setting.logSettingUpdate(filteredSetting, newSettings, oldSettings, testLogger),
+            new MockLogAppender.SeenEventExpectation("secure logging", "org.elasticsearch.test", Level.INFO, "updating [key]"),
+            new MockLogAppender.UnseenEventExpectation("unwanted old setting name", "org.elasticsearch.test", Level.INFO, "*old*"),
+            new MockLogAppender.UnseenEventExpectation("unwanted new setting name", "org.elasticsearch.test", Level.INFO, "*new*")
+        );
+    }
+
+    public void testRegularSettingUpdateIsFullyLogged() throws Exception {
+        Settings oldSettings = Settings.builder().put("key", "old").build();
+        Settings newSettings = Settings.builder().put("key", "new").build();
+
+        Setting<String> regularSetting = Setting.simpleString("key");
+        assertExpectedLogMessages((testLogger) -> Setting.logSettingUpdate(regularSetting, newSettings, oldSettings, testLogger),
+            new MockLogAppender.SeenEventExpectation("regular logging", "org.elasticsearch.test", Level.INFO,
+            "updating [key] from [old] to [new]"));
+    }
 
+    private void assertExpectedLogMessages(Consumer<Logger> consumer,
+                                           MockLogAppender.LoggingExpectation ... expectations) throws IllegalAccessException {
+        Logger testLogger = Loggers.getLogger("org.elasticsearch.test");
+        MockLogAppender appender = new MockLogAppender();
+        ServerLoggers.addAppender(testLogger, appender);
+        try {
+            appender.start();
+            Arrays.stream(expectations).forEach(appender::addExpectation);
+            consumer.accept(testLogger);
+            appender.assertAllExpectationsMatched();
+        } finally {
+            ServerLoggers.removeAppender(testLogger, appender);
+        }
     }
 
     private void testFiltering(Settings source, Settings filtered, String... patterns) throws IOException {

test/framework/src/main/java/org/elasticsearch/test/MockLogAppender.java

@@ -92,7 +92,7 @@ public void match(LogEvent event) {
                         saw = true;
                     }
                 } else {
-                    if (event.getMessage().toString().contains(message)) {
+                    if (event.getMessage().getFormattedMessage().contains(message)) {
                         saw = true;
                     }
                 }