From 7dde9d48e6cb7aa945c63672330d00188b1a8fcd 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) #47711 and #47246 helped to validate that monitoring settings are 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 #47711 and #47246 but this time for ALL (including non-http) settings. 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 +++++++------ .../exporter/local/LocalExporter.java | 2 +- .../monitoring/exporter/ExportersTests.java | 45 +++++++++++++++++++ .../local/LocalExporterIntegTests.java | 2 + .../xpack/test/rest/XPackRestIT.java | 1 + 8 files changed, 84 insertions(+), 20 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 bba96575fe79f..8bb2f78366060 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -748,6 +748,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 4b90c0ef851e8..e4a7095826cff 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 @@ -364,6 +364,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(); @@ -390,6 +391,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 cd9fbfe5fd1eb..c2d8cea408814 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.", @@ -82,13 +84,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. *

@@ -96,7 +98,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. @@ -108,7 +111,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 bd89d3c290efa..f9e90189b42dd 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 @@ -83,7 +83,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; @@ -169,14 +169,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. */ @@ -184,7 +184,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. */ @@ -192,7 +193,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. */ @@ -236,7 +238,7 @@ public Iterator> settings() { Property.Dynamic, Property.NodeScope, Property.Filtered), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * Secure password for basic auth. */ @@ -245,7 +247,7 @@ public Iterator> settings() { "xpack.monitoring.exporters.", "auth.secure_password", key -> SecureSetting.secureString(key, null), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * The SSL settings. * @@ -256,7 +258,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. @@ -277,13 +279,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. */ @@ -306,7 +308,7 @@ public Iterator> settings() { }, Property.Dynamic, Property.NodeScope), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * Blacklist of headers that the user is not allowed to set. *

@@ -318,19 +320,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/main/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporter.java index b4f3831fb2445..7529c7f229adb 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporter.java @@ -96,7 +96,7 @@ public class LocalExporter extends Exporter implements ClusterStateListener, Cle public static final Setting.AffixSetting WAIT_MASTER_TIMEOUT_SETTING = Setting.affixKeySetting( "xpack.monitoring.exporters.", "wait_master.timeout", - (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(30), Property.Dynamic, Property.NodeScope) + (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(30), Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY ); private final Client client; 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 113e79cd48f97..c12c05bcedfbf 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 f9cdf307f3829..18e433b550f99 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 @@ -65,6 +65,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"))); @@ -85,6 +86,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 78c6605c67f30..625639505d458 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(),