From bf5fc5602a86bc54ec48f668f902e7d971504514 Mon Sep 17 00:00:00 2001 From: Jake Landis Date: Sun, 31 May 2020 12:00:28 -0500 Subject: [PATCH 1/4] Ensure type exists for all monitoring configuration --- .../common/settings/Setting.java | 9 ++++ .../xpack/monitoring/exporter/Exporter.java | 13 +++--- .../exporter/http/HttpExporter.java | 30 +++++++------ .../exporter/local/LocalExporter.java | 2 +- .../monitoring/exporter/ExportersTests.java | 45 +++++++++++++++++++ 5 files changed, 79 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 614a72f3a0d90..8fbe6903af4b9 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/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 f212c3cb81480..7966dda4c555f 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 @@ -82,7 +82,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. */ @@ -235,7 +237,7 @@ public Iterator> settings() { Property.Dynamic, Property.NodeScope, Property.Filtered), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * Secure password for basic auth. */ @@ -244,7 +246,7 @@ public Iterator> settings() { "xpack.monitoring.exporters.", "auth.secure_password", key -> SecureSetting.secureString(key, null), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * The SSL settings. * @@ -255,7 +257,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. @@ -276,13 +278,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. */ @@ -305,7 +307,7 @@ public Iterator> settings() { }, Property.Dynamic, Property.NodeScope), - TYPE_DEPENDENCY); + HTTP_TYPE_DEPENDENCY); /** * Blacklist of headers that the user is not allowed to set. *

@@ -317,19 +319,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. * From 25970889337a76356fa563d42523c295fbd0d17a Mon Sep 17 00:00:00 2001 From: Jake Landis Date: Sun, 31 May 2020 14:27:40 -0500 Subject: [PATCH 2/4] fix test, can't validate dependent settings across node and dynamic --- .../monitoring/exporter/local/LocalExporterIntegTests.java | 2 ++ 1 file changed, 2 insertions(+) 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); From 269854dba99c38ed712d725f2bb82732a49bddb1 Mon Sep 17 00:00:00 2001 From: Jake Landis Date: Sun, 31 May 2020 15:21:47 -0500 Subject: [PATCH 3/4] another test setup fix --- .../test/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java | 1 + 1 file changed, 1 insertion(+) 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(), From dfbdc097627fcc1cdbf390f84563b0e565c9be87 Mon Sep 17 00:00:00 2001 From: Jake Landis Date: Sun, 31 May 2020 15:53:26 -0500 Subject: [PATCH 4/4] more test fixes --- .../xpack/monitoring/integration/MonitoringIT.java | 2 ++ 1 file changed, 2 insertions(+) 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();