Skip to content

Commit

Permalink
Validating monitoring hosts setting while parsing (#47246)
Browse files Browse the repository at this point in the history 
This commit lifts the validation of the monitoring hosts setting into
the setting itself, rather than when the setting is used. This prevents
a scenario where an invalid value for the setting is accepted, but then
later fails while applying a cluster state with the invalid setting.
  • Loading branch information
@jasontedor
jasontedor authored Oct 4, 2019
1 parent e33a02b commit fbd4ec5
Show file tree
Hide file tree
Showing 10 changed files with 299 additions and 178 deletions.
26 changes: 23 additions & 3 deletions server/src/main/java/org/elasticsearch/common/settings/Setting.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1243,6 +1243,15 @@ public static <T> Setting<List<T>> listSetting(
return listSetting(key, null, singleValueParser, (s) -> defaultStringValue, properties);
}

public static <T> Setting<List<T>> listSetting(
final String key,
final List<String> defaultStringValue,
final Function<String, T> singleValueParser,
final Validator<List<T>> validator,
final Property... properties) {
return listSetting(key, null, singleValueParser, (s) -> defaultStringValue, validator, properties);
}

// TODO this one's two argument get is still broken
public static <T> Setting<List<T>> listSetting(
final String key,
Expand All @@ -1266,13 +1275,23 @@ public static <T> Setting<List<T>> listSetting(
final Function<String, T> singleValueParser,
final Function<Settings, List<String>> defaultStringValue,
final Property... properties) {
return listSetting(key, fallbackSetting, singleValueParser, defaultStringValue, v -> {}, properties);
}

static <T> Setting<List<T>> listSetting(
final String key,
final @Nullable Setting<List<T>> fallbackSetting,
final Function<String, T> singleValueParser,
final Function<Settings, List<String>> defaultStringValue,
final Validator<List<T>> validator,
final Property... properties) {
if (defaultStringValue.apply(Settings.EMPTY) == null) {
throw new IllegalArgumentException("default value function must not return null");
}
Function<String, List<T>> parser = (s) ->
parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList());
parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList());

return new ListSetting<>(key, fallbackSetting, defaultStringValue, parser, properties);
return new ListSetting<>(key, fallbackSetting, defaultStringValue, parser, validator, properties);
}

private static List<String> parseableStringToList(String parsableString) {
Expand Down Expand Up @@ -1319,13 +1338,14 @@ private ListSetting(
final @Nullable Setting<List<T>> fallbackSetting,
final Function<Settings, List<String>> defaultStringValue,
final Function<String, List<T>> parser,
final Validator<List<T>> validator,
final Property... properties) {
super(
new ListKey(key),
fallbackSetting,
s -> Setting.arrayToParsableString(defaultStringValue.apply(s)),
parser,
v -> {},
validator,
properties);
this.defaultStringValue = defaultStringValue;
}
Expand Down
62 changes: 49 additions & 13 deletions ...plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,14 @@
import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.xpack.monitoring.exporter.http.HttpExporter;

import java.time.ZoneOffset;
import java.util.Arrays;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Function;

Expand All @@ -27,19 +30,52 @@ public abstract class Exporter implements AutoCloseable {
Setting.affixKeySetting("xpack.monitoring.exporters.","enabled",
key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope));

private static final Setting.AffixSetting<String> TYPE_SETTING =
Setting.affixKeySetting("xpack.monitoring.exporters.","type",
key -> Setting.simpleString(key, v -> {
switch (v) {
case "":
case "http":
case "local":
break;
default:
throw new IllegalArgumentException("only exporter types [http] and [local] are allowed [" + v +
"] is invalid");
}
}, Property.Dynamic, Property.NodeScope));
public static final Setting.AffixSetting<String> TYPE_SETTING = Setting.affixKeySetting(
"xpack.monitoring.exporters.",
"type",
key -> Setting.simpleString(
key,
new Setting.Validator<>() {

@Override
public void validate(final String value) {

}

@Override
public void validate(final String value, final Map<Setting<?>, Object> settings) {
switch (value) {
case "":
break;
case "http":
// if the type is http, then hosts must be set
final String namespace = TYPE_SETTING.getNamespace(TYPE_SETTING.getConcreteSetting(key));
final Setting<List<String>> hostsSetting = HttpExporter.HOST_SETTING.getConcreteSettingForNamespace(namespace);
@SuppressWarnings("unchecked") final List<String> hosts = (List<String>) settings.get(hostsSetting);
if (hosts.isEmpty()) {
throw new SettingsException("host list for [" + hostsSetting.getKey() + "] is empty");
}
break;
case "local":
break;
default:
throw new SettingsException(
"type [" + value + "] for key [" + key + "] is invalid, only [http] and [local] are allowed");
}

}

@Override
public Iterator<Setting<?>> settings() {
final String namespace =
Exporter.TYPE_SETTING.getNamespace(Exporter.TYPE_SETTING.getConcreteSetting(key));
final List<Setting<?>> settings = List.of(HttpExporter.HOST_SETTING.getConcreteSettingForNamespace(namespace));
return settings.iterator();
}

},
Property.Dynamic,
Property.NodeScope));
/**
* Every {@code Exporter} adds the ingest pipeline to bulk requests, but they should, at the exporter level, allow that to be disabled.
* <p>
Expand Down
107 changes: 75 additions & 32 deletions ...nitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@
import org.elasticsearch.xpack.monitoring.exporter.Exporter;

import javax.net.ssl.SSLContext;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -82,9 +82,78 @@ public class HttpExporter extends Exporter {
* A string array representing the Elasticsearch node(s) to communicate with over HTTP(S).
*/
public static final Setting.AffixSetting<List<String>> HOST_SETTING =
Setting.affixKeySetting("xpack.monitoring.exporters.","host",
(key) -> Setting.listSetting(key, Collections.emptyList(), Function.identity(),
Property.Dynamic, Property.NodeScope));
Setting.affixKeySetting(
"xpack.monitoring.exporters.",
"host",
key -> Setting.listSetting(
key,
Collections.emptyList(),
Function.identity(),
new Setting.Validator<>() {

@Override
public void validate(final List<String> value) {

}

@Override
public void validate(final List<String> hosts, final Map<Setting<?>, Object> settings) {
final String namespace =
HttpExporter.HOST_SETTING.getNamespace(HttpExporter.HOST_SETTING.getConcreteSetting(key));
final String type = (String) settings.get(Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace));

if (hosts.isEmpty()) {
final String defaultType =
Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace).get(Settings.EMPTY);
if (Objects.equals(type, defaultType)) {
// hosts can only be empty if the type is unset
return;
} else {
throw new SettingsException("host list for [" + key + "] is empty but type is [" + type + "]");
}
} else if ("http".equals(type) == false) {
// the hosts can only be non-empty if the type is "http"
throw new SettingsException("host list for [" + key + "] is set but type is [" + type + "]");
}

boolean httpHostFound = false;
boolean httpsHostFound = false;

// every host must be configured
for (final String host : hosts) {
final HttpHost httpHost;

try {
httpHost = HttpHostBuilder.builder(host).build();
} catch (final IllegalArgumentException e) {
throw new SettingsException("[" + key + "] invalid host: [" + host + "]", e);
}

if ("http".equals(httpHost.getSchemeName())) {
httpHostFound = true;
} else {
httpsHostFound = true;
}

// fail if we find them configuring the scheme/protocol in different ways
if (httpHostFound && httpsHostFound) {
throw new SettingsException("[" + key + "] must use a consistent scheme: http or https");
}
}
}

@Override
public Iterator<Setting<?>> settings() {
final String namespace =
HttpExporter.HOST_SETTING.getNamespace(HttpExporter.HOST_SETTING.getConcreteSetting(key));
final List<Setting<?>> settings = List.of(Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace));
return settings.iterator();
}

},
Property.Dynamic,
Property.NodeScope));

/**
* Master timeout associated with bulk requests.
*/
Expand Down Expand Up @@ -383,43 +452,17 @@ static MultiHttpResource createResources(final Config config) {
*/
private static HttpHost[] createHosts(final Config config) {
final List<String> hosts = HOST_SETTING.getConcreteSettingForNamespace(config.name()).get(config.settings());
String configKey = HOST_SETTING.getConcreteSettingForNamespace(config.name()).getKey();

if (hosts.isEmpty()) {
throw new SettingsException("missing required setting [" + configKey + "]");
}

final List<HttpHost> httpHosts = new ArrayList<>(hosts.size());
boolean httpHostFound = false;
boolean httpsHostFound = false;

// every host must be configured
for (final String host : hosts) {
final HttpHost httpHost;

try {
httpHost = HttpHostBuilder.builder(host).build();
} catch (IllegalArgumentException e) {
throw new SettingsException("[" + configKey + "] invalid host: [" + host + "]", e);
}

if ("http".equals(httpHost.getSchemeName())) {
httpHostFound = true;
} else {
httpsHostFound = true;
}

// fail if we find them configuring the scheme/protocol in different ways
if (httpHostFound && httpsHostFound) {
throw new SettingsException("[" + configKey + "] must use a consistent scheme: http or https");
}

final HttpHost httpHost = HttpHostBuilder.builder(host).build();
httpHosts.add(httpHost);
}

logger.debug("exporter [{}] using hosts {}", config.name(), hosts);

return httpHosts.toArray(new HttpHost[httpHosts.size()]);
return httpHosts.toArray(new HttpHost[0]);
}

/**
Expand Down
15 changes: 14 additions & 1 deletion .../monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/ExportersTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.elasticsearch.xpack.core.monitoring.exporter.MonitoringDoc;
import org.elasticsearch.xpack.monitoring.MonitoringService;
import org.elasticsearch.xpack.monitoring.cleaner.CleanerService;
import org.elasticsearch.xpack.monitoring.exporter.http.HttpExporter;
import org.elasticsearch.xpack.monitoring.exporter.local.LocalExporter;
import org.junit.Before;

Expand All @@ -53,6 +54,7 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasKey;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.hasToString;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
Expand Down Expand Up @@ -98,6 +100,17 @@ public void init() {
exporters = new Exporters(Settings.EMPTY, factories, clusterService, licenseState, threadContext);
}

public void testHostsMustBeSetIfTypeIsHttp() {
final String prefix = "xpack.monitoring.exporters.example";
final Settings settings = Settings.builder().put(prefix + ".type", "http").build();
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> HttpExporter.TYPE_SETTING.getConcreteSetting(prefix + ".type").get(settings));
assertThat(e, hasToString(containsString("Failed to parse value [http] for setting [" + prefix + ".type]")));
assertThat(e.getCause(), instanceOf(SettingsException.class));
assertThat(e.getCause(), hasToString(containsString("host list for [" + prefix + ".host] is empty")));
}

public void testExporterIndexPattern() {
Exporter.Config config = mock(Exporter.Config.class);
when(config.name()).thenReturn("anything");
Expand Down Expand Up @@ -241,7 +254,7 @@ public void testExporterBlocksOnClusterState() {
} else {
when(state.version()).thenReturn(ClusterState.UNKNOWN_VERSION);
}

final int nbExporters = randomIntBetween(1, 5);
final Settings.Builder settings = Settings.builder();

Expand Down
24 changes: 7 additions & 17 deletions ...toring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,25 +113,15 @@ protected boolean ignoreExternalCluster() {
return true;
}

@Override
protected Settings nodeSettings(int nodeOrdinal) {
// we create and disable the exporter to avoid the cluster actually using it (thus speeding up tests)
// we make an exporter on demand per test
return Settings.builder()
.put(super.nodeSettings(nodeOrdinal))
.put("xpack.monitoring.exporters._http.type", "http")
.put("xpack.monitoring.exporters._http.ssl.truststore.password", "foobar") // ensure that ssl can be used by settings
.put("xpack.monitoring.exporters._http.headers.ignored", "value") // ensure that headers can be used by settings
.put("xpack.monitoring.exporters._http.enabled", false)
.build();
}

private Settings.Builder baseSettings() {
return Settings.builder()
.put("xpack.monitoring.exporters._http.type", "http")
.put("xpack.monitoring.exporters._http.host", getFormattedAddress(webServer))
.putList("xpack.monitoring.exporters._http.cluster_alerts.management.blacklist", clusterAlertBlacklist)
.put("xpack.monitoring.exporters._http.index.template.create_legacy_templates", includeOldTemplates);
.put("xpack.monitoring.exporters._http.enabled", false)
.put("xpack.monitoring.exporters._http.type", "http")
.put("xpack.monitoring.exporters._http.ssl.truststore.password", "foobar") // ensure that ssl can be used by settings
.put("xpack.monitoring.exporters._http.headers.ignored", "value") // ensure that headers can be used by settings
.put("xpack.monitoring.exporters._http.host", getFormattedAddress(webServer))
.putList("xpack.monitoring.exporters._http.cluster_alerts.management.blacklist", clusterAlertBlacklist)
.put("xpack.monitoring.exporters._http.index.template.create_legacy_templates", includeOldTemplates);
}

public void testExport() throws Exception {
Expand Down
Loading

0 comments on commit fbd4ec5

Please sign in to comment.