Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure type exists for all monitoring configuration #57399

Merged
merged 4 commits into from
Jun 4, 2020
Merged

Ensure type exists for all monitoring configuration #57399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf5fc56
Ensure type exists for all monitoring configuration
jakelandis May 31, 2020
2597088
fix test, can't validate dependent settings across node and dynamic
jakelandis May 31, 2020
269854d
another test setup fix
jakelandis May 31, 2020
dfbdc09
more test fixes
jakelandis May 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions server/src/main/java/org/elasticsearch/common/settings/Setting.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,15 @@ private Stream<String> 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<AffixSettingDependency> getDependencies() {
return Collections.unmodifiableSet(dependencies);
}

@Override
public Set<SettingDependency> getSettingsDependencies(String settingsKey) {
if (dependencies.isEmpty()) {
Expand Down
2 changes: 2 additions & 0 deletions ...internalClusterTest/java/org/elasticsearch/xpack/monitoring/integration/MonitoringIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary since this test (and some others here) set the type via node level setting but the settings validation does not validate across node level vs. dynamic settings (they get merged after independent validation).

It also looks like we don't support validation across persistent vs. transient settings either, which makes any of these cross settings dependencies validation a bit terse. This translates to a real change in behavior w.r.t monitoring, but cross level validation is a bigger issue then monitoring. The safety here (imo) outweighs the convenience of cross type setting validation (and already exists for all HTTP type settings).

.put("xpack.monitoring.exporters._local.enabled", true)
.build();

Expand All @@ -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();
Expand Down
13 changes: 8 additions & 5 deletions ...plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@

public abstract class Exporter implements AutoCloseable {

public static Setting.AffixSettingDependency TYPE_DEPENDENCY = () -> Exporter.TYPE_SETTING;

private static final Setting.AffixSetting<Boolean> 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<String> TYPE_SETTING = Setting.affixKeySetting(
"xpack.monitoring.exporters.",
Expand Down Expand Up @@ -82,21 +84,22 @@ public Iterator<Setting<?>> settings() {
*/
public static final Setting.AffixSetting<Boolean> 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<Boolean> 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.
* <p>
* When cluster alerts management is enabled, this should delete anything blacklisted here in addition to not creating it.
*/
public static final Setting.AffixSetting<List<String>> 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.
Expand All @@ -108,7 +111,7 @@ public Iterator<Setting<?>> settings() {
Exporter.INDEX_FORMAT,
DateFormatter::forPattern,
Property.Dynamic,
Property.NodeScope));
Property.NodeScope), TYPE_DEPENDENCY);

private static final String INDEX_FORMAT = "yyyy.MM.dd";

Expand Down
30 changes: 16 additions & 14 deletions ...nitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> getSetting() {
return Exporter.TYPE_SETTING;
Expand Down Expand Up @@ -168,30 +168,32 @@ public Iterator<Setting<?>> settings() {
},
Property.Dynamic,
Property.NodeScope),
TYPE_DEPENDENCY);
HTTP_TYPE_DEPENDENCY);

/**
* Master timeout associated with bulk requests.
*/
public static final Setting.AffixSetting<TimeValue> 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.
*/
public static final Setting.AffixSetting<TimeValue> CONNECTION_TIMEOUT_SETTING =
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.
*/
public static final Setting.AffixSetting<TimeValue> CONNECTION_READ_TIMEOUT_SETTING =
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.
*/
Expand Down Expand Up @@ -235,7 +237,7 @@ public Iterator<Setting<?>> settings() {
Property.Dynamic,
Property.NodeScope,
Property.Filtered),
TYPE_DEPENDENCY);
HTTP_TYPE_DEPENDENCY);
/**
* Secure password for basic auth.
*/
Expand All @@ -244,7 +246,7 @@ public Iterator<Setting<?>> settings() {
"xpack.monitoring.exporters.",
"auth.secure_password",
key -> SecureSetting.secureString(key, null),
TYPE_DEPENDENCY);
HTTP_TYPE_DEPENDENCY);
/**
* The SSL settings.
*
Expand All @@ -255,7 +257,7 @@ public Iterator<Setting<?>> 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.
Expand All @@ -276,13 +278,13 @@ public Iterator<Setting<?>> 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<Boolean> 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.
*/
Expand All @@ -305,7 +307,7 @@ public Iterator<Setting<?>> settings() {
},
Property.Dynamic,
Property.NodeScope),
TYPE_DEPENDENCY);
HTTP_TYPE_DEPENDENCY);
/**
* Blacklist of headers that the user is not allowed to set.
* <p>
Expand All @@ -317,19 +319,19 @@ public Iterator<Setting<?>> settings() {
*/
public static final Setting.AffixSetting<TimeValue> 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<Boolean> 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<TimeValue> 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).
Expand Down
2 changes: 1 addition & 1 deletion ...toring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public class LocalExporter extends Exporter implements ClusterStateListener, Cle
public static final Setting.AffixSetting<TimeValue> 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;
Expand Down
45 changes: 45 additions & 0 deletions .../monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/ExportersTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -337,6 +339,49 @@ public void testConcurrentExports() throws Exception {
exporters.close();
}

/**
* All dynamic monitoring settings should have dependency on type.
*/
public void testSettingsDependency() {
List<Setting.AffixSetting<?>> 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<Setting.AffixSetting<?>> 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.
*
Expand Down
2 changes: 2 additions & 0 deletions .../test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")));
Expand All @@ -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);

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/src/test/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ private void enableMonitoring() throws Exception {
final Map<String, Object> 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(),
Expand Down