Skip to content

Commit

Permalink
This implementation is sufficient becuase
Browse files Browse the repository at this point in the history
* It generically solves the cluster state application across all of
the settings by catching the exception and logging it.

This implementation is not ideal becuase
* It allow invalid configuration to be persisted to cluster state,
with only a message to the log
* Does not notify the user that the configuration maybe incorrect
via the REST API.

To notify the the user that the configuration is incorrect via the
REST API and prevent persisting config to cluster state, one would
need to implement a validator via
` clusterService.getClusterSettings().addAffixUpdateConsumer(HOST_SETTING , consumer, validator)`

However, this is not done becuase
* It requires calling initExporters to find any exceptions that may be found.
	* Calling initExporters is not feasible due to
		* It would require alot of work on cluster update (even if we
		refactored out the validation bits)
		* We don't have easy access to the set of settings that are currently
		 being set, just easy access to the single setting. This is an affix
		 setting with other highly correlated settings needed to determine
		 correctness. The validator sees settings 1 by 1, not the full set
		 of settings being set.
		* On node startup this validation is also run, so if by some means
		[1] invalid config got into cluster state the exception would be thrown
		to the cluster state applier, not the REST layer.
* HOST_SETTINGS is not unique in it's behavior here. For example
`xpack.monitoring.exporters.foo.use_ingest` will exibit the same behavior
if `foo` has not been defined.

[1] not sure if this a bug or feature ... but when monitoring
(and i assume any other xpack plugin) is not enabled, you can still set the
settings, but the validators will not run, allowing cluster state to be applied
without any settings validation. This likely isn't a problem, until something
gets enabled to use that un-validated cluster state (and the state is incorrect).

```
GET _xpack
...
    "monitoring" : {
      "available" : true,
      "enabled" : false
    }
```
Fixes elastic#47125
  • Loading branch information
jakelandis committed Sep 27, 2019
1 parent 4568d30 commit 95fc96c
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,13 @@ public Exporters(Settings settings, Map<String, Exporter.Factory> factories,

private void setExportersSetting(Settings exportersSetting) {
if (this.lifecycle.started()) {
Map<String, Exporter> updated = initExporters(exportersSetting);
closeExporters(logger, this.exporters.getAndSet(updated));
try {
Map<String, Exporter> updated = initExporters(exportersSetting);
closeExporters(logger, this.exporters.getAndSet(updated));
}catch (Exception e){
//swallow exception here since this can be called from cluster state applier
logger.error("Unable set monitoring exporter settings", e);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,12 @@ public void testDynamicIndexFormatChange() throws Exception {
assertThat(newIndex.get("_index"), equalTo(expectedMonitoringIndex));
}

public void testInvalidConfig() {
//bad config could result in cluster state not applying properly see: https://github.com/elastic/elasticsearch/issues/47125
client().admin().cluster().prepareUpdateSettings()
.setTransientSettings(Collections.singletonMap("xpack.monitoring.exporters.foo.use_ingest", true)).execute().actionGet(5000);
}

private void assertMonitorVersion(final MockWebServer webServer) throws Exception {
assertMonitorVersion(webServer, null, null);
}
Expand Down

0 comments on commit 95fc96c

Please sign in to comment.