From d855010d08e196c4fd0f58f118ac704b8b9fee7f Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 6 Dec 2019 15:02:08 -0500 Subject: [PATCH 1/2] Validate exporter type is HTTP for HTTP exporter Today the HTTP exporter settings without the exporter type having been configured to HTTP. When it is time to initialize the exporter, we can blow up. Since this initialization happens on the cluster state applier thread, it is quite problematic that we do not reject settings updates where the type is not configured to HTTP, but there are HTTP exporter settings configured. This commit addresses this by validating that the exporter type is not only set, but is set to HTTP. --- .../exporter/http/HttpExporter.java | 74 +++++++++++++------ .../exporter/http/HttpExporterTests.java | 13 ++-- 2 files changed, 57 insertions(+), 30 deletions(-) diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java index dd9041ffd070c..a73cee446c280 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java @@ -78,6 +78,20 @@ public class HttpExporter extends Exporter { public static final String TYPE = "http"; + private static Setting.AffixSettingDependency TYPE_DEPENDENCY = new Setting.AffixSettingDependency() { + @Override + public Setting.AffixSetting getSetting() { + return Exporter.TYPE_SETTING; + } + + @Override + public void validate(final String key, final Object value, final Object dependency) { + if (TYPE.equals(dependency) == false) { + throw new SettingsException("[" + key + "] is set but type is [" + dependency + "]"); + } + } + }; + /** * A string array representing the Elasticsearch node(s) to communicate with over HTTP(S). */ @@ -111,9 +125,6 @@ public void validate(final List hosts, final Map, Object> set } 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; @@ -129,7 +140,7 @@ public void validate(final List hosts, final Map, Object> set throw new SettingsException("[" + key + "] invalid host: [" + host + "]", e); } - if ("http".equals(httpHost.getSchemeName())) { + if (TYPE.equals(httpHost.getSchemeName())) { httpHostFound = true; } else { httpsHostFound = true; @@ -152,26 +163,31 @@ public Iterator> settings() { }, Property.Dynamic, - Property.NodeScope)); + Property.NodeScope), + TYPE_DEPENDENCY); /** * Master timeout associated with bulk requests. */ public static final Setting.AffixSetting BULK_TIMEOUT_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","bulk.timeout", - (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope)); + (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); /** * Timeout used for initiating a connection. */ public static final Setting.AffixSetting CONNECTION_TIMEOUT_SETTING = - Setting.affixKeySetting("xpack.monitoring.exporters.","connection.timeout", - (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(6), Property.Dynamic, Property.NodeScope)); + Setting.affixKeySetting( + "xpack.monitoring.exporters.", + "connection.timeout", + (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(6), Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); /** * Timeout used for reading from the connection. */ public static final Setting.AffixSetting CONNECTION_READ_TIMEOUT_SETTING = - Setting.affixKeySetting("xpack.monitoring.exporters.","connection.read_timeout", - (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(60), Property.Dynamic, Property.NodeScope)); + Setting.affixKeySetting( + "xpack.monitoring.exporters.", + "connection.read_timeout", + (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(60), Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); /** * Username for basic auth. */ @@ -181,12 +197,12 @@ public Iterator> settings() { key, new Setting.Validator() { @Override - public void validate(String password) { + public void validate(final String password) { // no username validation that is independent of other settings } @Override - public void validate(String username, Map, Object> settings) { + public void validate(final String username, final Map, Object> settings) { final String namespace = HttpExporter.AUTH_USERNAME_SETTING.getNamespace( HttpExporter.AUTH_USERNAME_SETTING.getConcreteSetting(key)); @@ -201,6 +217,11 @@ public void validate(String username, Map, Object> settings) { "but [" + AUTH_PASSWORD_SETTING.getConcreteSettingForNamespace(namespace).getKey() + "] is " + "missing"); } + final String type = + (String) settings.get(Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace)); + if ("http".equals(type) == false) { + throw new SettingsException("username for [" + key + "] is set but type is [" + type + "]"); + } } } @@ -209,7 +230,9 @@ public Iterator> settings() { final String namespace = HttpExporter.AUTH_USERNAME_SETTING.getNamespace( HttpExporter.AUTH_USERNAME_SETTING.getConcreteSetting(key)); + final List> settings = List.of( + Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace), HttpExporter.AUTH_PASSWORD_SETTING.getConcreteSettingForNamespace(namespace)); return settings.iterator(); } @@ -217,7 +240,8 @@ public Iterator> settings() { }, Property.Dynamic, Property.NodeScope, - Property.Filtered)); + Property.Filtered), + TYPE_DEPENDENCY); /** * Password for basic auth. */ @@ -261,15 +285,19 @@ public Iterator> settings() { }, Property.Dynamic, Property.NodeScope, - Property.Filtered)); + Property.Filtered), + TYPE_DEPENDENCY); /** * The SSL settings. * * @see SSLService */ public static final Setting.AffixSetting SSL_SETTING = - Setting.affixKeySetting("xpack.monitoring.exporters.","ssl", - (key) -> Setting.groupSetting(key + ".", Property.Dynamic, Property.NodeScope, Property.Filtered)); + Setting.affixKeySetting( + "xpack.monitoring.exporters.", + "ssl", + (key) -> Setting.groupSetting(key + ".", Property.Dynamic, Property.NodeScope, Property.Filtered), + TYPE_DEPENDENCY); /** * Proxy setting to allow users to send requests to a remote cluster that requires a proxy base path. */ @@ -288,13 +316,14 @@ public Iterator> settings() { } }, Property.Dynamic, - Property.NodeScope)); + Property.NodeScope), + TYPE_DEPENDENCY); /** * A boolean setting to enable or disable sniffing for extra connections. */ public static final Setting.AffixSetting SNIFF_ENABLED_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","sniff.enabled", - (key) -> Setting.boolSetting(key, false, Property.Dynamic, Property.NodeScope)); + (key) -> Setting.boolSetting(key, false, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); /** * A parent setting to header key/value pairs, whose names are user defined. */ @@ -316,7 +345,8 @@ public Iterator> settings() { } }, Property.Dynamic, - Property.NodeScope)); + Property.NodeScope), + TYPE_DEPENDENCY); /** * Blacklist of headers that the user is not allowed to set. *

@@ -328,19 +358,19 @@ public Iterator> settings() { */ public static final Setting.AffixSetting TEMPLATE_CHECK_TIMEOUT_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","index.template.master_timeout", - (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope)); + (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); /** * A boolean setting to enable or disable whether to create placeholders for the old templates. */ public static final Setting.AffixSetting TEMPLATE_CREATE_LEGACY_VERSIONS_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","index.template.create_legacy_templates", - (key) -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); + (key) -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); /** * ES level timeout used when checking and writing pipelines (used to speed up tests) */ public static final Setting.AffixSetting PIPELINE_CHECK_TIMEOUT_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","index.pipeline.master_timeout", - (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope)); + (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); /** * Minimum supported version of the remote monitoring cluster (same major). diff --git a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java index bfa1e16d860b0..a50839ffbadc2 100644 --- a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java +++ b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java @@ -17,6 +17,7 @@ import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.common.unit.TimeValue; @@ -38,6 +39,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; @@ -133,14 +135,9 @@ public void testHostListIsRejectedIfTypeIsNotHttp() { final Settings.Builder builder = Settings.builder().put(prefix + ".type", "local"); builder.putList(prefix + ".host", List.of("https://example.com:443")); final Settings settings = builder.build(); - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, - () -> HttpExporter.HOST_SETTING.getConcreteSetting(prefix + ".host").get(settings)); - assertThat( - e, - hasToString(containsString("Failed to parse value [[\"https://example.com:443\"]] for setting [" + prefix + ".host]"))); - assertThat(e.getCause(), instanceOf(SettingsException.class)); - assertThat(e.getCause(), hasToString(containsString("host list for [" + prefix + ".host] is set but type is [local]"))); + final ClusterSettings clusterSettings = new ClusterSettings(settings, Set.of(HttpExporter.HOST_SETTING, Exporter.TYPE_SETTING)); + final SettingsException e = expectThrows(SettingsException.class, () -> clusterSettings.validate(settings, true)); + assertThat(e, hasToString(containsString("[" + prefix + ".host] is set but type is [local]"))); } public void testInvalidHost() { From 425b96a04e39e3d636998d59f50234530efaa0ce Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Thu, 12 Dec 2019 18:19:47 -0500 Subject: [PATCH 2/2] Fix tests --- .../xpack/monitoring/exporter/http/HttpExporterSslIT.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterSslIT.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterSslIT.java index 3aa07a58a7666..fb0da753be3b5 100644 --- a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterSslIT.java +++ b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterSslIT.java @@ -177,6 +177,8 @@ private Exporter getExporter(String name) { private ActionFuture setVerificationMode(String name, VerificationMode mode) { final ClusterUpdateSettingsRequest updateSettings = new ClusterUpdateSettingsRequest(); final Settings settings = Settings.builder() + .put("xpack.monitoring.exporters." + name + ".type", HttpExporter.TYPE) + .put("xpack.monitoring.exporters." + name + ".host", "https://" + webServer.getHostName() + ":" + webServer.getPort()) .put("xpack.monitoring.exporters." + name + ".ssl.verification_mode", mode.name()) .build(); updateSettings.transientSettings(settings);