Skip to content

Commit

Permalink
Validate exporter type is HTTP for HTTP exporter (elastic#49992)
Browse files Browse the repository at this point in the history 
Today the HTTP exporter settings without the exporter type having been
configured to HTTP. When it is time to initialize the exporter, we can
blow up. Since this initialization happens on the cluster state applier
thread, it is quite problematic that we do not reject settings updates
where the type is not configured to HTTP, but there are HTTP exporter
settings configured. This commit addresses this by validating that the
exporter type is not only set, but is set to HTTP.
  • Loading branch information
@jasontedor @SivagurunathanV
jasontedor authored and SivagurunathanV committed Jan 21, 2020
1 parent 244a8e7 commit 33af9c5
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 30 deletions.
74 changes: 52 additions & 22 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 @@ -78,6 +78,20 @@ public class HttpExporter extends Exporter {

public static final String TYPE = "http";

private static Setting.AffixSettingDependency TYPE_DEPENDENCY = new Setting.AffixSettingDependency() {
@Override
public Setting.AffixSetting getSetting() {
return Exporter.TYPE_SETTING;
}

@Override
public void validate(final String key, final Object value, final Object dependency) {
if (TYPE.equals(dependency) == false) {
throw new SettingsException("[" + key + "] is set but type is [" + dependency + "]");
}
}
};

/**
* A string array representing the Elasticsearch node(s) to communicate with over HTTP(S).
*/
Expand Down Expand Up @@ -111,9 +125,6 @@ public void validate(final List<String> hosts, final Map<Setting<?>, Object> set
} 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;
Expand All @@ -129,7 +140,7 @@ public void validate(final List<String> hosts, final Map<Setting<?>, Object> set
throw new SettingsException("[" + key + "] invalid host: [" + host + "]", e);
}

if ("http".equals(httpHost.getSchemeName())) {
if (TYPE.equals(httpHost.getSchemeName())) {
httpHostFound = true;
} else {
httpsHostFound = true;
Expand All @@ -152,26 +163,31 @@ public Iterator<Setting<?>> settings() {

},
Property.Dynamic,
Property.NodeScope));
Property.NodeScope),
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));
(key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope), 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));
Setting.affixKeySetting(
"xpack.monitoring.exporters.",
"connection.timeout",
(key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(6), Property.Dynamic, Property.NodeScope), 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));
Setting.affixKeySetting(
"xpack.monitoring.exporters.",
"connection.read_timeout",
(key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(60), Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY);
/**
* Username for basic auth.
*/
Expand All @@ -181,12 +197,12 @@ public Iterator<Setting<?>> settings() {
key,
new Setting.Validator<String>() {
@Override
public void validate(String password) {
public void validate(final String password) {
// no username validation that is independent of other settings
}

@Override
public void validate(String username, Map<Setting<?>, Object> settings) {
public void validate(final String username, final Map<Setting<?>, Object> settings) {
final String namespace =
HttpExporter.AUTH_USERNAME_SETTING.getNamespace(
HttpExporter.AUTH_USERNAME_SETTING.getConcreteSetting(key));
Expand All @@ -201,6 +217,11 @@ public void validate(String username, Map<Setting<?>, Object> settings) {
"but [" + AUTH_PASSWORD_SETTING.getConcreteSettingForNamespace(namespace).getKey() + "] is " +
"missing");
}
final String type =
(String) settings.get(Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace));
if ("http".equals(type) == false) {
throw new SettingsException("username for [" + key + "] is set but type is [" + type + "]");
}
}
}

Expand All @@ -209,15 +230,18 @@ public Iterator<Setting<?>> settings() {
final String namespace =
HttpExporter.AUTH_USERNAME_SETTING.getNamespace(
HttpExporter.AUTH_USERNAME_SETTING.getConcreteSetting(key));

final List<Setting<?>> settings = List.of(
Exporter.TYPE_SETTING.getConcreteSettingForNamespace(namespace),
HttpExporter.AUTH_PASSWORD_SETTING.getConcreteSettingForNamespace(namespace));
return settings.iterator();
}

},
Property.Dynamic,
Property.NodeScope,
Property.Filtered));
Property.Filtered),
TYPE_DEPENDENCY);
/**
* Password for basic auth.
*/
Expand Down Expand Up @@ -261,15 +285,19 @@ public Iterator<Setting<?>> settings() {
},
Property.Dynamic,
Property.NodeScope,
Property.Filtered));
Property.Filtered),
TYPE_DEPENDENCY);
/**
* The SSL settings.
*
* @see SSLService
*/
public static final Setting.AffixSetting<Settings> SSL_SETTING =
Setting.affixKeySetting("xpack.monitoring.exporters.","ssl",
(key) -> Setting.groupSetting(key + ".", Property.Dynamic, Property.NodeScope, Property.Filtered));
Setting.affixKeySetting(
"xpack.monitoring.exporters.",
"ssl",
(key) -> Setting.groupSetting(key + ".", Property.Dynamic, Property.NodeScope, Property.Filtered),
TYPE_DEPENDENCY);
/**
* Proxy setting to allow users to send requests to a remote cluster that requires a proxy base path.
*/
Expand All @@ -288,13 +316,14 @@ public Iterator<Setting<?>> settings() {
}
},
Property.Dynamic,
Property.NodeScope));
Property.NodeScope),
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));
(key) -> Setting.boolSetting(key, false, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY);
/**
* A parent setting to header key/value pairs, whose names are user defined.
*/
Expand All @@ -316,7 +345,8 @@ public Iterator<Setting<?>> settings() {
}
},
Property.Dynamic,
Property.NodeScope));
Property.NodeScope),
TYPE_DEPENDENCY);
/**
* Blacklist of headers that the user is not allowed to set.
* <p>
Expand All @@ -328,19 +358,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));
(key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope), 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));
(key) -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope), 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));
(key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope), TYPE_DEPENDENCY);

/**
* Minimum supported version of the remote monitoring cluster (same major).
Expand Down
2 changes: 2 additions & 0 deletions ...ing/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterSslIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ private Exporter getExporter(String name) {
private ActionFuture<ClusterUpdateSettingsResponse> setVerificationMode(String name, VerificationMode mode) {
final ClusterUpdateSettingsRequest updateSettings = new ClusterUpdateSettingsRequest();
final Settings settings = Settings.builder()
.put("xpack.monitoring.exporters." + name + ".type", HttpExporter.TYPE)
.put("xpack.monitoring.exporters." + name + ".host", "https://" + webServer.getHostName() + ":" + webServer.getPort())
.put("xpack.monitoring.exporters." + name + ".ssl.verification_mode", mode.name())
.build();
updateSettings.transientSettings(settings);
Expand Down
13 changes: 5 additions & 8 deletions ...ing/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.common.unit.TimeValue;
Expand All @@ -38,6 +39,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -133,14 +135,9 @@ public void testHostListIsRejectedIfTypeIsNotHttp() {
final Settings.Builder builder = Settings.builder().put(prefix + ".type", "local");
builder.putList(prefix + ".host", List.of("https://example.com:443"));
final Settings settings = builder.build();
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> HttpExporter.HOST_SETTING.getConcreteSetting(prefix + ".host").get(settings));
assertThat(
e,
hasToString(containsString("Failed to parse value [[\"https://example.com:443\"]] for setting [" + prefix + ".host]")));
assertThat(e.getCause(), instanceOf(SettingsException.class));
assertThat(e.getCause(), hasToString(containsString("host list for [" + prefix + ".host] is set but type is [local]")));
final ClusterSettings clusterSettings = new ClusterSettings(settings, Set.of(HttpExporter.HOST_SETTING, Exporter.TYPE_SETTING));
final SettingsException e = expectThrows(SettingsException.class, () -> clusterSettings.validate(settings, true));
assertThat(e, hasToString(containsString("[" + prefix + ".host] is set but type is [local]")));
}

public void testInvalidHost() {
Expand Down

0 comments on commit 33af9c5

Please sign in to comment.