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 fc92e41befd76..94f0d9b7e627c 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -750,6 +750,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 b58759d5bd258..feb45ce297a83 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 @@ -596,6 +596,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(); @@ -622,6 +623,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 562f427ca2cc6..cdcd4cc10bf93 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 @@ -81,7 +81,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; @@ -168,14 +168,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. */ @@ -183,7 +183,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. */ @@ -191,7 +192,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. */ @@ -242,7 +244,7 @@ public Iterator> settings() { Property.Dynamic, Property.NodeScope, Property.Filtered), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * Password for basic auth. */ @@ -297,7 +299,7 @@ public Iterator> settings() { "xpack.monitoring.exporters.", "auth.secure_password", key -> SecureSetting.secureString(key, null), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * The SSL settings. * @@ -308,7 +310,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. @@ -329,13 +331,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. */ @@ -358,7 +360,7 @@ public Iterator> settings() { }, Property.Dynamic, Property.NodeScope), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * Blacklist of headers that the user is not allowed to set. *

@@ -370,19 +372,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 4b8a310b57507..c896611fa8466 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 10c386dec6ba4..3d64a5c5b837c 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 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(),