Validating monitoring hosts setting while parsing

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

@@ -1243,6 +1243,15 @@ public static <T> Setting<List<T>> listSetting(
         return listSetting(key, null, singleValueParser, (s) -> defaultStringValue, properties);
     }
 
+    public static <T> Setting<List<T>> listSetting(
+        final String key,
+        final List<String> defaultStringValue,
+        final Function<String, T> singleValueParser,
+        final Validator<List<T>> validator,
+        final Property... properties) {
+        return listSetting(key, null, singleValueParser, (s) -> defaultStringValue, validator, properties);
+    }
+
     // TODO this one's two argument get is still broken
     public static <T> Setting<List<T>> listSetting(
             final String key,
@@ -1266,13 +1275,23 @@ public static <T> Setting<List<T>> listSetting(
             final Function<String, T> singleValueParser,
             final Function<Settings, List<String>> defaultStringValue,
             final Property... properties) {
+        return listSetting(key, fallbackSetting, singleValueParser, defaultStringValue, v -> {}, properties);
+    }
+
+    static <T> Setting<List<T>> listSetting(
+        final String key,
+        final @Nullable Setting<List<T>> fallbackSetting,
+        final Function<String, T> singleValueParser,
+        final Function<Settings, List<String>> defaultStringValue,
+        final Validator<List<T>> validator,
+        final Property... properties) {
         if (defaultStringValue.apply(Settings.EMPTY) == null) {
             throw new IllegalArgumentException("default value function must not return null");
         }
         Function<String, List<T>> parser = (s) ->
-                parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList());
+            parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList());
 
-        return new ListSetting<>(key, fallbackSetting, defaultStringValue, parser, properties);
+        return new ListSetting<>(key, fallbackSetting, defaultStringValue, parser, validator, properties);
     }
 
     private static List<String> parseableStringToList(String parsableString) {
@@ -1319,13 +1338,14 @@ private ListSetting(
                 final @Nullable Setting<List<T>> fallbackSetting,
                 final Function<Settings, List<String>> defaultStringValue,
                 final Function<String, List<T>> parser,
+                final Validator<List<T>> validator,
                 final Property... properties) {
             super(
                     new ListKey(key),
                     fallbackSetting,
                     s -> Setting.arrayToParsableString(defaultStringValue.apply(s)),
                     parser,
-                    v -> {},
+                    validator,
                     properties);
             this.defaultStringValue = defaultStringValue;
         }

x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java

@@ -13,11 +13,14 @@
 import org.elasticsearch.common.settings.SettingsException;
 import org.elasticsearch.common.time.DateFormatter;
 import org.elasticsearch.license.XPackLicenseState;
+import org.elasticsearch.xpack.monitoring.exporter.http.HttpExporter;
 
 import java.time.ZoneOffset;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Function;
 
@@ -27,19 +30,52 @@ public abstract class Exporter implements AutoCloseable {
             Setting.affixKeySetting("xpack.monitoring.exporters.","enabled",
                     key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope));
 
-    private static final Setting.AffixSetting<String> TYPE_SETTING =
-            Setting.affixKeySetting("xpack.monitoring.exporters.","type",
-                    key -> Setting.simpleString(key, v -> {
-                        switch (v) {
-                            case "":
-                            case "http":
-                            case "local":
-                                break;
-                            default:
-                                throw new IllegalArgumentException("only exporter types [http] and [local] are allowed [" + v +
-                                        "] is invalid");
-                        }
-                    }, Property.Dynamic, Property.NodeScope));
+    public static final Setting.AffixSetting<String> TYPE_SETTING = Setting.affixKeySetting(
+        "xpack.monitoring.exporters.",
+        "type",
+        key -> Setting.simpleString(
+            key,
+            new Setting.Validator<>() {
+
+                @Override
+                public void validate(final String value) {
+
+                }
+
+                @Override
+                public void validate(final String value, final Map<Setting<?>, Object> settings) {
+                    switch (value) {
+                        case "":
+                            break;
+                        case "http":
+                            // if the type is http, then hosts must be set
+                            final String namespace = TYPE_SETTING.getNamespace(TYPE_SETTING.getConcreteSetting(key));
+                            final Setting<List<String>> hostsSetting = HttpExporter.HOST_SETTING.getConcreteSettingForNamespace(namespace);
+                            @SuppressWarnings("unchecked") final List<String> hosts = (List<String>) settings.get(hostsSetting);
+                            if (hosts.isEmpty()) {
+                                throw new SettingsException("host list for [" + hostsSetting.getKey() + "] is empty");
+                            }
+                            break;
+                        case "local":
+                            break;
+                        default:
+                            throw new SettingsException(
+                                "type [" + value + "] for key [" + key + "] is invalid, only [http] and [local] are allowed");
+                    }
+
+                }
+
+                @Override
+                public Iterator<Setting<?>> settings() {
+                    final String namespace =
+                        Exporter.TYPE_SETTING.getNamespace(Exporter.TYPE_SETTING.getConcreteSetting(key));
+                    final List<Setting<?>> settings = List.of(HttpExporter.HOST_SETTING.getConcreteSettingForNamespace(namespace));
+                    return settings.iterator();
+                }
+
+            },
+            Property.Dynamic,
+            Property.NodeScope));
     /**
      * Every {@code Exporter} adds the ingest pipeline to bulk requests, but they should, at the exporter level, allow that to be disabled.
      * <p>

x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java

@@ -44,10 +44,10 @@
 import org.elasticsearch.xpack.monitoring.exporter.Exporter;
 
 import javax.net.ssl.SSLContext;
-
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -82,9 +82,78 @@ public class HttpExporter extends Exporter {
      * A string array representing the Elasticsearch node(s) to communicate with over HTTP(S).
      */
     public static final Setting.AffixSetting<List<String>> HOST_SETTING =
-            Setting.affixKeySetting("xpack.monitoring.exporters.","host",
-                    (key) -> Setting.listSetting(key, Collections.emptyList(), Function.identity(),
-                            Property.Dynamic, Property.NodeScope));
+            Setting.affixKeySetting(
+                "xpack.monitoring.exporters.",
+                "host",
+                key -> Setting.listSetting(
+                    key,
+                    Collections.emptyList(),
+                    Function.identity(),
+                    new Setting.Validator<>() {
+
+                        @Override
+                        public void validate(final List<String> value) {
+
+                        }
+
+                        @Override
+                        public void validate(final List<String> hosts, final Map<Setting<?>, Object> settings) {
+                            final String namespace =
+                                HttpExporter.HOST_SETTING.getNamespace(HttpExporter.HOST_SETTING.getConcreteSetting(key));
+                            final String type = (String) settings.get(Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace));
+
+                            if (hosts.isEmpty()) {
+                                final String defaultType =
+                                    Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace).get(Settings.EMPTY);
+                                if (Objects.equals(type, defaultType)) {
+                                    // hosts can only be empty if the type is unset
+                                    return;
+                                } else {
+                                    throw new SettingsException("host list for [" + key + "] is empty but type is [" + type + "]");
+                                }
+                            } else if ("http".equals(type) == false) {
+                                // the hosts can only be non-empty if the type is "http"
+                                throw new SettingsException("host list for [" + key + "] is set but type is [" + type + "]");
+                            }
+
+                            boolean httpHostFound = false;
+                            boolean httpsHostFound = false;
+
+                            // every host must be configured
+                            for (final String host : hosts) {
+                                final HttpHost httpHost;
+
+                                try {
+                                    httpHost = HttpHostBuilder.builder(host).build();
+                                } catch (final IllegalArgumentException e) {
+                                    throw new SettingsException("[" + key + "] invalid host: [" + host + "]", e);
+                                }
+
+                                if ("http".equals(httpHost.getSchemeName())) {
+                                    httpHostFound = true;
+                                } else {
+                                    httpsHostFound = true;
+                                }
+
+                                // fail if we find them configuring the scheme/protocol in different ways
+                                if (httpHostFound && httpsHostFound) {
+                                    throw new SettingsException("[" + key + "] must use a consistent scheme: http or https");
+                                }
+                            }
+                        }
+
+                        @Override
+                        public Iterator<Setting<?>> settings() {
+                            final String namespace =
+                                HttpExporter.HOST_SETTING.getNamespace(HttpExporter.HOST_SETTING.getConcreteSetting(key));
+                            final List<Setting<?>> settings = List.of(Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace));
+                            return settings.iterator();
+                        }
+
+                    },
+                    Property.Dynamic,
+                    Property.NodeScope));
+
     /**
      * Master timeout associated with bulk requests.
      */
@@ -383,43 +452,17 @@ static MultiHttpResource createResources(final Config config) {
      */
     private static HttpHost[] createHosts(final Config config) {
         final List<String> hosts = HOST_SETTING.getConcreteSettingForNamespace(config.name()).get(config.settings());
-        String configKey = HOST_SETTING.getConcreteSettingForNamespace(config.name()).getKey();
-
-        if (hosts.isEmpty()) {
-            throw new SettingsException("missing required setting [" + configKey + "]");
-        }
 
         final List<HttpHost> httpHosts = new ArrayList<>(hosts.size());
-        boolean httpHostFound = false;
-        boolean httpsHostFound = false;
 
-        // every host must be configured
         for (final String host : hosts) {
-            final HttpHost httpHost;
-
-            try {
-                httpHost = HttpHostBuilder.builder(host).build();
-            } catch (IllegalArgumentException e) {
-                throw new SettingsException("[" + configKey + "] invalid host: [" + host + "]", e);
-            }
-
-            if ("http".equals(httpHost.getSchemeName())) {
-                httpHostFound = true;
-            } else {
-                httpsHostFound = true;
-            }
-
-            // fail if we find them configuring the scheme/protocol in different ways
-            if (httpHostFound && httpsHostFound) {
-                throw new SettingsException("[" + configKey + "] must use a consistent scheme: http or https");
-            }
-
+            final HttpHost httpHost = HttpHostBuilder.builder(host).build();
             httpHosts.add(httpHost);
         }
 
         logger.debug("exporter [{}] using hosts {}", config.name(), hosts);
 
-        return httpHosts.toArray(new HttpHost[httpHosts.size()]);
+        return httpHosts.toArray(new HttpHost[0]);
     }
 
     /**

x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/ExportersTests.java

@@ -27,6 +27,7 @@
 import org.elasticsearch.xpack.core.monitoring.exporter.MonitoringDoc;
 import org.elasticsearch.xpack.monitoring.MonitoringService;
 import org.elasticsearch.xpack.monitoring.cleaner.CleanerService;
+import org.elasticsearch.xpack.monitoring.exporter.http.HttpExporter;
 import org.elasticsearch.xpack.monitoring.exporter.local.LocalExporter;
 import org.junit.Before;
 
@@ -53,6 +54,7 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasKey;
 import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
@@ -98,6 +100,17 @@ public void init() {
         exporters = new Exporters(Settings.EMPTY, factories, clusterService, licenseState, threadContext);
     }
 
+    public void testHostsMustBeSetIfTypeIsHttp() {
+        final String prefix = "xpack.monitoring.exporters.example";
+        final Settings settings  = Settings.builder().put(prefix + ".type", "http").build();
+        final IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> HttpExporter.TYPE_SETTING.getConcreteSetting(prefix + ".type").get(settings));
+        assertThat(e, hasToString(containsString("Failed to parse value [http] for setting [" + prefix + ".type]")));
+        assertThat(e.getCause(), instanceOf(SettingsException.class));
+        assertThat(e.getCause(), hasToString(containsString("host list for [" + prefix + ".host] is empty")));
+    }
+
     public void testExporterIndexPattern() {
         Exporter.Config config = mock(Exporter.Config.class);
         when(config.name()).thenReturn("anything");
@@ -241,7 +254,7 @@ public void testExporterBlocksOnClusterState() {
         } else {
             when(state.version()).thenReturn(ClusterState.UNKNOWN_VERSION);
         }
-        
+
         final int nbExporters = randomIntBetween(1, 5);
         final Settings.Builder settings = Settings.builder();
 

x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterIT.java

@@ -113,25 +113,15 @@ protected boolean ignoreExternalCluster() {
         return true;
     }
 
-    @Override
-    protected Settings nodeSettings(int nodeOrdinal) {
-        // we create and disable the exporter to avoid the cluster actually using it (thus speeding up tests)
-        // we make an exporter on demand per test
-        return Settings.builder()
-                       .put(super.nodeSettings(nodeOrdinal))
-                       .put("xpack.monitoring.exporters._http.type", "http")
-                       .put("xpack.monitoring.exporters._http.ssl.truststore.password", "foobar") // ensure that ssl can be used by settings
-                       .put("xpack.monitoring.exporters._http.headers.ignored", "value") // ensure that headers can be used by settings
-                       .put("xpack.monitoring.exporters._http.enabled", false)
-                       .build();
-    }
-
     private Settings.Builder baseSettings() {
         return Settings.builder()
-                       .put("xpack.monitoring.exporters._http.type", "http")
-                       .put("xpack.monitoring.exporters._http.host", getFormattedAddress(webServer))
-                       .putList("xpack.monitoring.exporters._http.cluster_alerts.management.blacklist", clusterAlertBlacklist)
-                       .put("xpack.monitoring.exporters._http.index.template.create_legacy_templates", includeOldTemplates);
+            .put("xpack.monitoring.exporters._http.enabled", false)
+            .put("xpack.monitoring.exporters._http.type", "http")
+            .put("xpack.monitoring.exporters._http.ssl.truststore.password", "foobar") // ensure that ssl can be used by settings
+            .put("xpack.monitoring.exporters._http.headers.ignored", "value") // ensure that headers can be used by settings
+            .put("xpack.monitoring.exporters._http.host", getFormattedAddress(webServer))
+            .putList("xpack.monitoring.exporters._http.cluster_alerts.management.blacklist", clusterAlertBlacklist)
+            .put("xpack.monitoring.exporters._http.index.template.create_legacy_templates", includeOldTemplates);
     }
 
     public void testExport() throws Exception {