From e0328655c885f3bbeecc15b5460bc4253ea11df7 Mon Sep 17 00:00:00 2001 From: Jake Landis Date: Thu, 4 Jun 2020 16:47:09 -0500 Subject: [PATCH] Ensure type exists for all monitoring configuration (#57399) rejected at time of setting the monitoring settings. Else an invalid monitoring setting can find it's way into the cluster state and result in an exception thrown [1] on the cluster state application (there by causing significant issues). Some additional monitoring settings have been identified that can result in invalid cluster state that also result in exceptions thrown on cluster state application. All settings require a type of either http or local to be applicable. When a setting is changed, the exporters are automatically updated with the new settings. However, if the old or new settings lack of a type setting an exception will be thrown (since exporters are always of type 'http' or 'local'). Arguably we shouldn't blindly create and destroy new exporters on each monitoring setting update, but the lifecycle of the exporters is abit out the scope this PR is trying to address. This commit introduces a similar methodology to check for validity as Monitoring settings are not useful unless there an exporter with a type defined. The type is used as dependent setting, such that it must exist to set the value. This ensures that when any monitoring settings changes that they can only get added to cluster state if the type exists. If the type exists (and the other validations pass) then the exporters will get re-built and the cluster state remains valid. Tests have been included to ensure that all dynamic monitoring settings have the type as dependent settings. [1] org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?] --- .../common/settings/Setting.java | 9 ++++ .../monitoring/integration/MonitoringIT.java | 2 + .../xpack/monitoring/exporter/Exporter.java | 13 +++--- .../exporter/http/HttpExporter.java | 30 +++++++------ .../monitoring/exporter/ExportersTests.java | 45 +++++++++++++++++++ .../local/LocalExporterIntegTests.java | 2 + .../xpack/test/rest/XPackRestIT.java | 1 + 7 files changed, 83 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index 796123d9cb659..c94e0b60347e6 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -742,6 +742,15 @@ private Stream matchStream(Settings settings) { return settings.keySet().stream().filter(this::match).map(key::getConcreteString); } + /** + * Get the raw list of dependencies. This method is exposed for testing purposes and {@link #getSettingsDependencies(String)} + * should be preferred for most all cases. + * @return the raw list of dependencies for this setting + */ + public Set getDependencies() { + return Collections.unmodifiableSet(dependencies); + } + @Override public Set getSettingsDependencies(String settingsKey) { if (dependencies.isEmpty()) { diff --git a/x-pack/plugin/monitoring/src/internalClusterTest/java/org/elasticsearch/xpack/monitoring/integration/MonitoringIT.java b/x-pack/plugin/monitoring/src/internalClusterTest/java/org/elasticsearch/xpack/monitoring/integration/MonitoringIT.java index 7a631d6f488c3..6da56338e81e7 100644 --- a/x-pack/plugin/monitoring/src/internalClusterTest/java/org/elasticsearch/xpack/monitoring/integration/MonitoringIT.java +++ b/x-pack/plugin/monitoring/src/internalClusterTest/java/org/elasticsearch/xpack/monitoring/integration/MonitoringIT.java @@ -595,6 +595,7 @@ public void enableMonitoring() throws Exception { final Settings settings = Settings.builder() .put("xpack.monitoring.collection.enabled", true) + .put("xpack.monitoring.exporters._local.type", "local") .put("xpack.monitoring.exporters._local.enabled", true) .build(); @@ -621,6 +622,7 @@ public void enableMonitoring() throws Exception { public void disableMonitoring() throws Exception { final Settings settings = Settings.builder() .putNull("xpack.monitoring.collection.enabled") + .putNull("xpack.monitoring.exporters._local.type") .putNull("xpack.monitoring.exporters._local.enabled") .putNull("cluster.metadata.display_name") .build(); diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java index 8922d17affcfa..ac39be4a94827 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java @@ -25,9 +25,11 @@ public abstract class Exporter implements AutoCloseable { + public static Setting.AffixSettingDependency TYPE_DEPENDENCY = () -> Exporter.TYPE_SETTING; + private static final Setting.AffixSetting ENABLED_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","enabled", - key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); + key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); public static final Setting.AffixSetting TYPE_SETTING = Setting.affixKeySetting( "xpack.monitoring.exporters.", @@ -83,13 +85,13 @@ public Iterator> settings() { */ public static final Setting.AffixSetting USE_INGEST_PIPELINE_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","use_ingest", - key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); + key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); /** * Every {@code Exporter} allows users to explicitly disable cluster alerts. */ public static final Setting.AffixSetting CLUSTER_ALERTS_MANAGEMENT_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.", "cluster_alerts.management.enabled", - key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); + key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); /** * Every {@code Exporter} allows users to explicitly disable specific cluster alerts. *

@@ -97,7 +99,8 @@ public Iterator> settings() { */ public static final Setting.AffixSetting> CLUSTER_ALERTS_BLACKLIST_SETTING = Setting .affixKeySetting("xpack.monitoring.exporters.", "cluster_alerts.management.blacklist", - key -> Setting.listSetting(key, Collections.emptyList(), Function.identity(), Property.Dynamic, Property.NodeScope)); + key -> Setting.listSetting(key, Collections.emptyList(), Function.identity(), Property.Dynamic, Property.NodeScope), + TYPE_DEPENDENCY); /** * Every {@code Exporter} allows users to use a different index time format. @@ -109,7 +112,7 @@ public Iterator> settings() { Exporter.INDEX_FORMAT, DateFormatter::forPattern, Property.Dynamic, - Property.NodeScope)); + Property.NodeScope), TYPE_DEPENDENCY); private static final String INDEX_FORMAT = "yyyy.MM.dd"; 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 ad340c751e4e1..193c61ed53d4a 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 @@ -80,7 +80,7 @@ public class HttpExporter extends Exporter { public static final String TYPE = "http"; - private static Setting.AffixSettingDependency TYPE_DEPENDENCY = new Setting.AffixSettingDependency() { + private static Setting.AffixSettingDependency HTTP_TYPE_DEPENDENCY = new Setting.AffixSettingDependency() { @Override public Setting.AffixSetting getSetting() { return Exporter.TYPE_SETTING; @@ -167,14 +167,14 @@ public Iterator> settings() { }, Property.Dynamic, Property.NodeScope), - TYPE_DEPENDENCY); + HTTP_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), TYPE_DEPENDENCY); + (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope), HTTP_TYPE_DEPENDENCY); /** * Timeout used for initiating a connection. */ @@ -182,7 +182,8 @@ public Iterator> settings() { Setting.affixKeySetting( "xpack.monitoring.exporters.", "connection.timeout", - (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(6), Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); + (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(6), Property.Dynamic, Property.NodeScope), + HTTP_TYPE_DEPENDENCY); /** * Timeout used for reading from the connection. */ @@ -190,7 +191,8 @@ public Iterator> settings() { Setting.affixKeySetting( "xpack.monitoring.exporters.", "connection.read_timeout", - (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(60), Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY); + (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(60), Property.Dynamic, Property.NodeScope), + HTTP_TYPE_DEPENDENCY); /** * Username for basic auth. */ @@ -241,7 +243,7 @@ public Iterator> settings() { Property.Dynamic, Property.NodeScope, Property.Filtered), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * Password for basic auth. */ @@ -296,7 +298,7 @@ public Iterator> settings() { "xpack.monitoring.exporters.", "auth.secure_password", key -> SecureSetting.secureString(key, null), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * The SSL settings. * @@ -307,7 +309,7 @@ public Iterator> settings() { "xpack.monitoring.exporters.", "ssl", (key) -> Setting.groupSetting(key + ".", Property.Dynamic, Property.NodeScope, Property.Filtered), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * Proxy setting to allow users to send requests to a remote cluster that requires a proxy base path. @@ -328,13 +330,13 @@ public Iterator> settings() { }, Property.Dynamic, Property.NodeScope), - TYPE_DEPENDENCY); + HTTP_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), TYPE_DEPENDENCY); + (key) -> Setting.boolSetting(key, false, Property.Dynamic, Property.NodeScope), HTTP_TYPE_DEPENDENCY); /** * A parent setting to header key/value pairs, whose names are user defined. */ @@ -357,7 +359,7 @@ public Iterator> settings() { }, Property.Dynamic, Property.NodeScope), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * Blacklist of headers that the user is not allowed to set. *

@@ -369,19 +371,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), TYPE_DEPENDENCY); + (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope), HTTP_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), TYPE_DEPENDENCY); + (key) -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope), HTTP_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), TYPE_DEPENDENCY); + (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope), HTTP_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/ExportersTests.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/ExportersTests.java index 40e865cd964a5..56d459de2bd61 100644 --- a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/ExportersTests.java +++ b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/ExportersTests.java @@ -49,7 +49,9 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; @@ -337,6 +339,49 @@ public void testConcurrentExports() throws Exception { exporters.close(); } + /** + * All dynamic monitoring settings should have dependency on type. + */ + public void testSettingsDependency() { + List> settings = Exporters.getSettings().stream().filter(Setting::isDynamic).collect(Collectors.toList()); + settings.stream().filter(s -> s.getKey().equals("xpack.monitoring.exporters.*.type") == false) + .forEach(setting -> assertThat(setting.getKey() + " does not have a dependency on type", + setting.getDependencies().stream().map(Setting.AffixSettingDependency::getSetting).distinct().collect(Collectors.toList()), + contains(Exporter.TYPE_SETTING))); + } + + /** + * This is a variation of testing that all settings validated at cluster state update ensure that the type is set. This workflow + * emulates adding valid settings, then attempting to remove the type. This should never be allowed since type if type is null + * then any associated settings are extraneous and thus invalid (and can cause validation issues on cluster state application). + */ + public void testRemoveType() { + //run the update for all dynamic settings and ensure that they correctly throw an exception + List> settings = Exporters.getSettings().stream().filter(Setting::isDynamic).collect(Collectors.toList()); + settings.stream().filter(s -> s.getKey().equals("xpack.monitoring.exporters.*.type") == false) + .forEach(setting -> { + String fullSettingName = setting.getKey().replace("*", "foobar"); + Settings nodeSettings = Settings.builder() + .put("xpack.monitoring.exporters.foobar.type", randomFrom("local, http")) //actual type should not matter + .put(fullSettingName, "") + .build(); + + clusterSettings = new ClusterSettings(nodeSettings, new HashSet<>(Exporters.getSettings())); + when(clusterService.getClusterSettings()).thenReturn(clusterSettings); + + Settings update = Settings.builder() + .put("xpack.monitoring.exporters.foobar.type", (String) null) + .build(); + + Settings.Builder target = Settings.builder().put(nodeSettings); + clusterSettings.updateDynamicSettings(update, target, Settings.builder(), "persistent"); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> clusterSettings.validate(target.build(), true)); + assertThat(e.getMessage(), + containsString("missing required setting [xpack.monitoring.exporters.foobar.type] for setting [" + fullSettingName)); + }); + } + /** * Attempt to export a random number of documents via {@code exporters} from multiple threads. * diff --git a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java index ed5d3ef40ae24..6e7f23e7e2cb9 100644 --- a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java +++ b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java @@ -66,6 +66,7 @@ private void stopMonitoring() { // Now disabling the monitoring service, so that no more collection are started assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings( Settings.builder().putNull(MonitoringService.ENABLED.getKey()) + .putNull("xpack.monitoring.exporters._local.type") .putNull("xpack.monitoring.exporters._local.enabled") .putNull("xpack.monitoring.exporters._local.cluster_alerts.management.enabled") .putNull("xpack.monitoring.exporters._local.index.name.time_format"))); @@ -86,6 +87,7 @@ public void testExport() throws Exception { // start the monitoring service so that /_monitoring/bulk is not ignored final Settings.Builder exporterSettings = Settings.builder() .put(MonitoringService.ENABLED.getKey(), true) + .put("xpack.monitoring.exporters._local.type", LocalExporter.TYPE) .put("xpack.monitoring.exporters._local.enabled", true) .put("xpack.monitoring.exporters._local.cluster_alerts.management.enabled", false); diff --git a/x-pack/plugin/src/test/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java b/x-pack/plugin/src/test/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java index 18a13c131894d..4e1243ed0965f 100644 --- a/x-pack/plugin/src/test/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java +++ b/x-pack/plugin/src/test/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java @@ -123,6 +123,7 @@ private void enableMonitoring() throws Exception { final Map settings = new HashMap<>(); settings.put("xpack.monitoring.collection.enabled", true); settings.put("xpack.monitoring.collection.interval", "1s"); + settings.put("xpack.monitoring.exporters._local.type", "local"); settings.put("xpack.monitoring.exporters._local.enabled", true); awaitCallApi("cluster.put_settings", emptyMap(),