Validate the settings key if it's simple chars separated by `.`

This adds additional safety to the settings since we don't have a full list of them
yet and it caught bugs already in tests.
This commit is contained in:
Simon Willnauer 2016-01-20 13:44:22 +01:00
parent d11a11e9f0
commit 0941604919
5 changed files with 54 additions and 5 deletions

View File

@ -33,6 +33,7 @@ import java.util.Map;
import java.util.Set; import java.util.Set;
import java.util.function.BiConsumer; import java.util.function.BiConsumer;
import java.util.function.Consumer; import java.util.function.Consumer;
import java.util.regex.Pattern;
/** /**
* A basic setting service that can be used for per-index and per-cluster settings. * A basic setting service that can be used for per-index and per-cluster settings.
@ -44,6 +45,9 @@ public abstract class AbstractScopedSettings extends AbstractComponent {
private final Map<String, Setting<?>> complexMatchers = new HashMap<>(); private final Map<String, Setting<?>> complexMatchers = new HashMap<>();
private final Map<String, Setting<?>> keySettings = new HashMap<>(); private final Map<String, Setting<?>> keySettings = new HashMap<>();
private final Setting.Scope scope; private final Setting.Scope scope;
private static final Pattern KEY_PATTERN = Pattern.compile("^([\\w\\d_-]+[.])*[\\w\\d_-]+$");
private static final Pattern GROUP_KEY_PATTERN = Pattern.compile("^([\\w\\d_-]+[.])+");
protected AbstractScopedSettings(Settings settings, Set<Setting<?>> settingsSet, Setting.Scope scope) { protected AbstractScopedSettings(Settings settings, Set<Setting<?>> settingsSet, Setting.Scope scope) {
super(settings); super(settings);
@ -67,6 +71,9 @@ public abstract class AbstractScopedSettings extends AbstractComponent {
if (setting.getScope() != scope) { if (setting.getScope() != scope) {
throw new IllegalArgumentException("Setting must be a " + scope + " setting but was: " + setting.getScope()); throw new IllegalArgumentException("Setting must be a " + scope + " setting but was: " + setting.getScope());
} }
if (isValidKey(setting.getKey()) == false && (setting.isGroupSetting() && isValidGroupKey(setting.getKey())) == false) {
throw new IllegalArgumentException("illegal settings key: [" + setting.getKey() + "]");
}
if (setting.hasComplexMatcher()) { if (setting.hasComplexMatcher()) {
complexMatchers.putIfAbsent(setting.getKey(), setting); complexMatchers.putIfAbsent(setting.getKey(), setting);
} else { } else {
@ -74,6 +81,17 @@ public abstract class AbstractScopedSettings extends AbstractComponent {
} }
} }
/**
* Returns <code>true</code> iff the given key is a valid settings key otherwise <code>false</code>
*/
public static boolean isValidKey(String key) {
return KEY_PATTERN.matcher(key).matches();
}
private static boolean isValidGroupKey(String key) {
return GROUP_KEY_PATTERN.matcher(key).matches();
}
public Setting.Scope getScope() { public Setting.Scope getScope() {
return this.scope; return this.scope;
} }

View File

@ -61,6 +61,8 @@ public class SettingsModule extends AbstractModule {
for (Map.Entry<String, String> entry : settings.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE.negate()).getAsMap().entrySet()) { for (Map.Entry<String, String> entry : settings.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE.negate()).getAsMap().entrySet()) {
if (clusterSettings.get(entry.getKey()) != null) { if (clusterSettings.get(entry.getKey()) != null) {
clusterSettings.validate(entry.getKey(), settings); clusterSettings.validate(entry.getKey(), settings);
} else if (AbstractScopedSettings.isValidKey(entry.getKey()) == false) {
throw new IllegalArgumentException("illegal settings key: [" + entry.getKey() + "]");
} }
} }

View File

@ -162,8 +162,8 @@ public class ClusterRerouteIT extends ESIntegTestCase {
public void testDelayWithALargeAmountOfShards() throws Exception { public void testDelayWithALargeAmountOfShards() throws Exception {
Settings commonSettings = settingsBuilder() Settings commonSettings = settingsBuilder()
.put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_INCOMING_RECOVERIES_SETTING, 1) .put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_INCOMING_RECOVERIES_SETTING.getKey(), 1)
.put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_OUTGOING_RECOVERIES_SETTING, 1) .put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_OUTGOING_RECOVERIES_SETTING.getKey(), 1)
.build(); .build();
logger.info("--> starting 4 nodes"); logger.info("--> starting 4 nodes");
String node_1 = internalCluster().startNode(commonSettings); String node_1 = internalCluster().startNode(commonSettings);

View File

@ -225,4 +225,33 @@ public class ScopedSettingsTests extends ESTestCase {
return metaData; return metaData;
} }
public void testKeyPattern() {
assertTrue(AbstractScopedSettings.isValidKey("a.b.c-b.d"));
assertTrue(AbstractScopedSettings.isValidKey("a.b.c.d"));
assertTrue(AbstractScopedSettings.isValidKey("a.b_012.c_b.d"));
assertTrue(AbstractScopedSettings.isValidKey("a"));
assertFalse(AbstractScopedSettings.isValidKey("a b"));
assertFalse(AbstractScopedSettings.isValidKey(""));
assertFalse(AbstractScopedSettings.isValidKey("\""));
try {
new IndexScopedSettings(
Settings.EMPTY, Collections.singleton(Setting.groupSetting("boo .", false, Setting.Scope.INDEX)));
fail();
} catch (IllegalArgumentException e) {
assertEquals("illegal settings key: [boo .]", e.getMessage());
}
new IndexScopedSettings(
Settings.EMPTY, Collections.singleton(Setting.groupSetting("boo.", false, Setting.Scope.INDEX)));
try {
new IndexScopedSettings(
Settings.EMPTY, Collections.singleton(Setting.boolSetting("boo.", true, false, Setting.Scope.INDEX)));
fail();
} catch (IllegalArgumentException e) {
assertEquals("illegal settings key: [boo.]", e.getMessage());
}
new IndexScopedSettings(
Settings.EMPTY, Collections.singleton(Setting.boolSetting("boo", true, false, Setting.Scope.INDEX)));
}
} }

View File

@ -331,10 +331,10 @@ public class RecoveryFromGatewayIT extends ESIntegTestCase {
public void testReusePeerRecovery() throws Exception { public void testReusePeerRecovery() throws Exception {
final Settings settings = settingsBuilder() final Settings settings = settingsBuilder()
.put("action.admin.cluster.node.shutdown.delay", "10ms") .put("action.admin.cluster.node.shutdown.delay", "10ms")
.put(MockFSIndexStore.INDEX_CHECK_INDEX_ON_CLOSE_SETTING, false) .put(MockFSIndexStore.INDEX_CHECK_INDEX_ON_CLOSE_SETTING.getKey(), false)
.put("gateway.recover_after_nodes", 4) .put("gateway.recover_after_nodes", 4)
.put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_INCOMING_RECOVERIES_SETTING, 4) .put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_INCOMING_RECOVERIES_SETTING.getKey(), 4)
.put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_OUTGOING_RECOVERIES_SETTING, 4) .put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_OUTGOING_RECOVERIES_SETTING.getKey(), 4)
.put(MockFSDirectoryService.CRASH_INDEX_SETTING.getKey(), false).build(); .put(MockFSDirectoryService.CRASH_INDEX_SETTING.getKey(), false).build();
internalCluster().startNodesAsync(4, settings).get(); internalCluster().startNodesAsync(4, settings).get();