Allow comma delimited array settings to have a space after each entry (#22591)

Previously, certain settings that could take multiple comma delimited
values would pick up incorrect values for all entries but the first if
each comma separated value was followed by a whitespace character.  For
example, the multi-value "A,B,C" would be correctly parsed as
["A", "B", "C"] but the multi-value "A, B, C" would be incorrectly parsed
as ["A", " B", " C"].

This commit allows a comma separated list to have whitespace characters
after each entry.  The specific settings that were affected by this are:

  cluster.routing.allocation.awareness.attributes
  index.routing.allocation.require.*
  index.routing.allocation.include.*
  index.routing.allocation.exclude.*
  cluster.routing.allocation.require.*
  cluster.routing.allocation.include.*
  cluster.routing.allocation.exclude.*
  http.cors.allow-methods
  http.cors.allow-headers

For the allocation filtering related settings, this commit also provides
validation of each specified entry if the filtering is done by _ip,
_host_ip, or _publish_ip, to ensure that each entry is a valid IP
address.

Closes #22297
This commit is contained in:
Ali Beyad 2017-01-17 08:51:04 -06:00 committed by GitHub
parent f5542ed47f
commit e2977889b8
12 changed files with 172 additions and 14 deletions

View File

@ -70,6 +70,7 @@ import java.util.Map;
import java.util.Set;
import java.util.function.Function;
import static org.elasticsearch.cluster.node.DiscoveryNodeFilters.IP_VALIDATOR;
import static org.elasticsearch.cluster.node.DiscoveryNodeFilters.OpType.AND;
import static org.elasticsearch.cluster.node.DiscoveryNodeFilters.OpType.OR;
import static org.elasticsearch.common.settings.Settings.readSettingsFromStream;
@ -242,11 +243,11 @@ public class IndexMetaData implements Diffable<IndexMetaData>, ToXContent {
public static final String INDEX_ROUTING_INCLUDE_GROUP_PREFIX = "index.routing.allocation.include";
public static final String INDEX_ROUTING_EXCLUDE_GROUP_PREFIX = "index.routing.allocation.exclude";
public static final Setting<Settings> INDEX_ROUTING_REQUIRE_GROUP_SETTING =
Setting.groupSetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", Property.Dynamic, Property.IndexScope);
Setting.groupSetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", IP_VALIDATOR, Property.Dynamic, Property.IndexScope);
public static final Setting<Settings> INDEX_ROUTING_INCLUDE_GROUP_SETTING =
Setting.groupSetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", Property.Dynamic, Property.IndexScope);
Setting.groupSetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", IP_VALIDATOR, Property.Dynamic, Property.IndexScope);
public static final Setting<Settings> INDEX_ROUTING_EXCLUDE_GROUP_SETTING =
Setting.groupSetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", Property.Dynamic, Property.IndexScope);
Setting.groupSetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", IP_VALIDATOR, Property.Dynamic, Property.IndexScope);
public static final Setting<Settings> INDEX_ROUTING_INITIAL_RECOVERY_GROUP_SETTING =
Setting.groupSetting("index.routing.allocation.initial_recovery."); // this is only setable internally not a registered setting!!

View File

@ -21,6 +21,7 @@ package org.elasticsearch.cluster.node;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.network.InetAddresses;
import org.elasticsearch.common.network.NetworkAddress;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.common.settings.Settings;
@ -28,6 +29,7 @@ import org.elasticsearch.common.transport.TransportAddress;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Consumer;
public class DiscoveryNodeFilters {
@ -36,6 +38,25 @@ public class DiscoveryNodeFilters {
OR
}
/**
* Validates the IP addresses in a group of {@link Settings} by looking for the keys
* "_ip", "_host_ip", and "_publish_ip" and ensuring each of their comma separated values
* is a valid IP address.
*/
public static final Consumer<Settings> IP_VALIDATOR = (settings) -> {
Map<String, String> settingsMap = settings.getAsMap();
for (Map.Entry<String, String> entry : settingsMap.entrySet()) {
String propertyKey = entry.getKey();
if ("_ip".equals(propertyKey) || "_host_ip".equals(propertyKey) || "_publish_ip".equals(propertyKey)) {
for (String value : Strings.tokenizeToStringArray(entry.getValue(), ",")) {
if (InetAddresses.isInetAddress(value) == false) {
throw new IllegalArgumentException("invalid IP address [" + value + "] for [" + propertyKey + "]");
}
}
}
}
};
public static DiscoveryNodeFilters buildFromSettings(OpType opType, String prefix, Settings settings) {
return buildFromKeyValue(opType, settings.getByPrefix(prefix).getAsMap());
}
@ -43,7 +64,7 @@ public class DiscoveryNodeFilters {
public static DiscoveryNodeFilters buildFromKeyValue(OpType opType, Map<String, String> filters) {
Map<String, String[]> bFilters = new HashMap<>();
for (Map.Entry<String, String> entry : filters.entrySet()) {
String[] values = Strings.splitStringByCommaToArray(entry.getValue());
String[] values = Strings.tokenizeToStringArray(entry.getValue(), ",");
if (values.length > 0) {
bFilters.put(entry.getKey(), values);
}

View File

@ -78,12 +78,12 @@ public class AwarenessAllocationDecider extends AllocationDecider {
public static final String NAME = "awareness";
public static final Setting<String[]> CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING =
new Setting<>("cluster.routing.allocation.awareness.attributes", "", Strings::splitStringByCommaToArray , Property.Dynamic,
new Setting<>("cluster.routing.allocation.awareness.attributes", "", s -> Strings.tokenizeToStringArray(s, ","), Property.Dynamic,
Property.NodeScope);
public static final Setting<Settings> CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING =
Setting.groupSetting("cluster.routing.allocation.awareness.force.", Property.Dynamic, Property.NodeScope);
private String[] awarenessAttributes;
private volatile String[] awarenessAttributes;
private volatile Map<String, String[]> forcedAwarenessAttributes;

View File

@ -30,6 +30,7 @@ import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import static org.elasticsearch.cluster.node.DiscoveryNodeFilters.IP_VALIDATOR;
import static org.elasticsearch.cluster.node.DiscoveryNodeFilters.OpType.AND;
import static org.elasticsearch.cluster.node.DiscoveryNodeFilters.OpType.OR;
@ -68,11 +69,11 @@ public class FilterAllocationDecider extends AllocationDecider {
private static final String CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX = "cluster.routing.allocation.include";
private static final String CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX = "cluster.routing.allocation.exclude";
public static final Setting<Settings> CLUSTER_ROUTING_REQUIRE_GROUP_SETTING =
Setting.groupSetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", Property.Dynamic, Property.NodeScope);
Setting.groupSetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", IP_VALIDATOR, Property.Dynamic, Property.NodeScope);
public static final Setting<Settings> CLUSTER_ROUTING_INCLUDE_GROUP_SETTING =
Setting.groupSetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", Property.Dynamic, Property.NodeScope);
Setting.groupSetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", IP_VALIDATOR, Property.Dynamic, Property.NodeScope);
public static final Setting<Settings> CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING =
Setting.groupSetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", Property.Dynamic, Property.NodeScope);
Setting.groupSetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", IP_VALIDATOR, Property.Dynamic, Property.NodeScope);
private volatile DiscoveryNodeFilters clusterRequireFilters;
private volatile DiscoveryNodeFilters clusterIncludeFilters;

View File

@ -181,7 +181,16 @@ public class Node implements Closeable {
*/
public static final Setting<Boolean> NODE_LOCAL_STORAGE_SETTING = Setting.boolSetting("node.local_storage", true, Property.NodeScope);
public static final Setting<String> NODE_NAME_SETTING = Setting.simpleString("node.name", Property.NodeScope);
public static final Setting<Settings> NODE_ATTRIBUTES = Setting.groupSetting("node.attr.", Property.NodeScope);
public static final Setting<Settings> NODE_ATTRIBUTES = Setting.groupSetting("node.attr.", (settings) -> {
Map<String, String> settingsMap = settings.getAsMap();
for (Map.Entry<String, String> entry : settingsMap.entrySet()) {
String value = entry.getValue();
if (Character.isWhitespace(value.charAt(0)) || Character.isWhitespace(value.charAt(value.length() - 1))) {
throw new IllegalArgumentException("node.attr." + entry.getKey() + " cannot have leading or trailing whitespace " +
"[" + value + "]");
}
}
}, Property.NodeScope);
public static final Setting<String> BREAKER_TYPE_KEY = new Setting<>("indices.breaker.type", "hierarchy", (s) -> {
switch (s) {
case "hierarchy":

View File

@ -24,8 +24,10 @@ import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.routing.IndexRoutingTable;
import org.elasticsearch.cluster.routing.IndexShardRoutingTable;
import org.elasticsearch.cluster.routing.ShardRouting;
import org.elasticsearch.cluster.routing.allocation.decider.FilterAllocationDecider;
import org.elasticsearch.cluster.routing.allocation.decider.ThrottlingAllocationDecider;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.test.ESIntegTestCase;
@ -144,5 +146,15 @@ public class FilteringAllocationIT extends ESIntegTestCase {
clusterState = client().admin().cluster().prepareState().execute().actionGet().getState();
assertThat(clusterState.routingTable().index("test").numberOfNodesShardsAreAllocatedOn(), equalTo(2));
}
public void testInvalidIPFilterClusterSettings() {
String ipKey = randomFrom("_ip", "_host_ip", "_publish_ip");
Setting<Settings> filterSetting = randomFrom(FilterAllocationDecider.CLUSTER_ROUTING_REQUIRE_GROUP_SETTING,
FilterAllocationDecider.CLUSTER_ROUTING_INCLUDE_GROUP_SETTING, FilterAllocationDecider.CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> client().admin().cluster().prepareUpdateSettings()
.setTransientSettings(Settings.builder().put(filterSetting.getKey() + ipKey, "192.168.1.1."))
.execute().actionGet());
assertEquals("invalid IP address [192.168.1.1.] for [" + ipKey + "]", e.getMessage());
}
}

View File

@ -245,6 +245,17 @@ public class DiscoveryNodeFiltersTests extends ESTestCase {
assertThat(filters.match(node), equalTo(true));
}
public void testCommaSeparatedValuesTrimmed() {
DiscoveryNode node = new DiscoveryNode("", "", "", "", "192.1.1.54", localAddress, singletonMap("tag", "B"), emptySet(), null);
Settings settings = shuffleSettings(Settings.builder()
.put("xxx." + randomFrom("_ip", "_host_ip", "_publish_ip"), "192.1.1.1, 192.1.1.54")
.put("xxx.tag", "A, B")
.build());
DiscoveryNodeFilters filters = DiscoveryNodeFilters.buildFromSettings(OR, "xxx.", settings);
assertTrue(filters.match(node));
}
private Settings shuffleSettings(Settings source) {
Settings.Builder settings = Settings.builder();
List<String> keys = new ArrayList<>(source.getAsMap().keySet());

View File

@ -32,10 +32,14 @@ import org.elasticsearch.cluster.routing.ShardRoutingState;
import org.elasticsearch.cluster.routing.allocation.command.AllocationCommands;
import org.elasticsearch.cluster.routing.allocation.command.CancelAllocationCommand;
import org.elasticsearch.cluster.routing.allocation.command.MoveAllocationCommand;
import org.elasticsearch.cluster.routing.allocation.decider.AwarenessAllocationDecider;
import org.elasticsearch.cluster.routing.allocation.decider.ClusterRebalanceAllocationDecider;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.settings.Settings;
import java.util.HashMap;
import java.util.Map;
import static java.util.Collections.singletonMap;
import static org.elasticsearch.cluster.routing.ShardRoutingState.INITIALIZING;
import static org.elasticsearch.cluster.routing.ShardRoutingState.RELOCATING;
@ -803,4 +807,50 @@ public class AwarenessAllocationTests extends ESAllocationTestCase {
assertThat(clusterState.getRoutingNodes().shardsWithState(INITIALIZING).size(), equalTo(4)); // +1 for relocating shard.
assertThat(clusterState.getRoutingNodes().shardsWithState(UNASSIGNED).size(), equalTo(1)); // Still 1 unassigned.
}
public void testMultipleAwarenessAttributes() {
AllocationService strategy = createAllocationService(Settings.builder()
.put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack")
.put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "zone.values", "a, b")
.put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "rack.values", "c, d")
.put(ClusterRebalanceAllocationDecider.CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE_SETTING.getKey(), "always")
.build());
logger.info("Building initial routing table for 'testUnbalancedZones'");
MetaData metaData = MetaData.builder()
.put(IndexMetaData.builder("test").settings(settings(Version.CURRENT)).numberOfShards(1).numberOfReplicas(1))
.build();
RoutingTable initialRoutingTable = RoutingTable.builder().addAsNew(metaData.index("test")).build();
ClusterState clusterState = ClusterState.builder(
org.elasticsearch.cluster.ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)
).metaData(metaData).routingTable(initialRoutingTable).build();
logger.info("--> adding two nodes in different zones and do rerouting");
Map<String, String> nodeAAttributes = new HashMap<>();
nodeAAttributes.put("zone", "a");
nodeAAttributes.put("rack", "c");
Map<String, String> nodeBAttributes = new HashMap<>();
nodeBAttributes.put("zone", "b");
nodeBAttributes.put("rack", "d");
clusterState = ClusterState.builder(clusterState).nodes(DiscoveryNodes.builder()
.add(newNode("A-0", nodeAAttributes))
.add(newNode("B-0", nodeBAttributes))
).build();
clusterState = strategy.reroute(clusterState, "reroute");
assertThat(clusterState.getRoutingNodes().shardsWithState(STARTED).size(), equalTo(0));
assertThat(clusterState.getRoutingNodes().shardsWithState(INITIALIZING).size(), equalTo(1));
logger.info("--> start the shards (primaries)");
clusterState = strategy.applyStartedShards(clusterState, clusterState.getRoutingNodes().shardsWithState(INITIALIZING));
assertThat(clusterState.getRoutingNodes().shardsWithState(STARTED).size(), equalTo(1));
assertThat(clusterState.getRoutingNodes().shardsWithState(INITIALIZING).size(), equalTo(1));
clusterState = strategy.applyStartedShards(clusterState, clusterState.getRoutingNodes().shardsWithState(INITIALIZING));
logger.info("--> all replicas are allocated and started since we have one node in each zone and rack");
assertThat(clusterState.getRoutingNodes().shardsWithState(STARTED).size(), equalTo(2));
assertThat(clusterState.getRoutingNodes().shardsWithState(INITIALIZING).size(), equalTo(0));
}
}

View File

@ -37,6 +37,8 @@ import org.elasticsearch.cluster.routing.allocation.decider.FilterAllocationDeci
import org.elasticsearch.cluster.routing.allocation.decider.ReplicaAfterPrimaryActiveAllocationDecider;
import org.elasticsearch.cluster.routing.allocation.decider.SameShardAllocationDecider;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.IndexScopedSettings;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.snapshots.Snapshot;
import org.elasticsearch.snapshots.SnapshotId;
@ -191,4 +193,16 @@ public class FilterAllocationDeciderTests extends ESAllocationTestCase {
.build();
return service.reroute(clusterState, "reroute", false);
}
public void testInvalidIPFilter() {
String ipKey = randomFrom("_ip", "_host_ip", "_publish_ip");
Setting<Settings> filterSetting = randomFrom(IndexMetaData.INDEX_ROUTING_REQUIRE_GROUP_SETTING,
IndexMetaData.INDEX_ROUTING_INCLUDE_GROUP_SETTING, IndexMetaData.INDEX_ROUTING_EXCLUDE_GROUP_SETTING);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
IndexScopedSettings indexScopedSettings = new IndexScopedSettings(Settings.EMPTY, IndexScopedSettings.BUILT_IN_INDEX_SETTINGS);
indexScopedSettings.updateDynamicSettings(Settings.builder().put(filterSetting.getKey() + ipKey, "192..168.1.1").build(),
Settings.builder().put(Settings.EMPTY), Settings.builder(), "test ip validation");
});
assertEquals("invalid IP address [192..168.1.1] for [" + ipKey + "]", e.getMessage());
}
}

View File

@ -406,14 +406,14 @@ public class Netty4HttpServerTransport extends AbstractLifecycleComponent implem
if (SETTING_CORS_ALLOW_CREDENTIALS.get(settings)) {
builder.allowCredentials();
}
String[] strMethods = Strings.splitStringByCommaToArray(SETTING_CORS_ALLOW_METHODS.get(settings));
String[] strMethods = Strings.tokenizeToStringArray(SETTING_CORS_ALLOW_METHODS.get(settings), ",");
HttpMethod[] methods = Arrays.asList(strMethods)
.stream()
.map(HttpMethod::valueOf)
.toArray(size -> new HttpMethod[size]);
return builder.allowedRequestMethods(methods)
.maxAge(SETTING_CORS_MAX_AGE.get(settings))
.allowedRequestHeaders(Strings.splitStringByCommaToArray(SETTING_CORS_ALLOW_HEADERS.get(settings)))
.allowedRequestHeaders(Strings.tokenizeToStringArray(SETTING_CORS_ALLOW_HEADERS.get(settings), ","))
.shortCircuit()
.build();
}

View File

@ -51,6 +51,7 @@ import java.util.HashSet;
import java.util.Set;
import java.util.stream.Collectors;
import static org.elasticsearch.common.Strings.collectionToDelimitedString;
import static org.elasticsearch.http.HttpTransportSettings.SETTING_CORS_ALLOW_CREDENTIALS;
import static org.elasticsearch.http.HttpTransportSettings.SETTING_CORS_ALLOW_HEADERS;
import static org.elasticsearch.http.HttpTransportSettings.SETTING_CORS_ALLOW_METHODS;
@ -89,11 +90,12 @@ public class Netty4HttpServerTransportTests extends ESTestCase {
public void testCorsConfig() {
final Set<String> methods = new HashSet<>(Arrays.asList("get", "options", "post"));
final Set<String> headers = new HashSet<>(Arrays.asList("Content-Type", "Content-Length"));
final String suffix = randomBoolean() ? " " : ""; // sometimes have a leading whitespace between comma delimited elements
final Settings settings = Settings.builder()
.put(SETTING_CORS_ENABLED.getKey(), true)
.put(SETTING_CORS_ALLOW_ORIGIN.getKey(), "*")
.put(SETTING_CORS_ALLOW_METHODS.getKey(), Strings.collectionToCommaDelimitedString(methods))
.put(SETTING_CORS_ALLOW_HEADERS.getKey(), Strings.collectionToCommaDelimitedString(headers))
.put(SETTING_CORS_ALLOW_METHODS.getKey(), collectionToDelimitedString(methods, ",", suffix, ""))
.put(SETTING_CORS_ALLOW_HEADERS.getKey(), collectionToDelimitedString(headers, ",", suffix, ""))
.put(SETTING_CORS_ALLOW_CREDENTIALS.getKey(), true)
.build();
final Netty4CorsConfig corsConfig = Netty4HttpServerTransport.buildCorsConfig(settings);

View File

@ -138,4 +138,41 @@ public class NodeTests extends ESTestCase {
}
public void testNodeAttributes() throws IOException {
String attr = randomAsciiOfLength(5);
Settings.Builder settings = baseSettings().put(Node.NODE_ATTRIBUTES.getKey() + "test_attr", attr);
try (Node node = new MockNode(settings.build(), Collections.singleton(MockTcpTransportPlugin.class))) {
final Settings nodeSettings = randomBoolean() ? node.settings() : node.getEnvironment().settings();
assertEquals(attr, Node.NODE_ATTRIBUTES.get(nodeSettings).getAsMap().get("test_attr"));
}
// leading whitespace not allowed
attr = " leading";
settings = baseSettings().put(Node.NODE_ATTRIBUTES.getKey() + "test_attr", attr);
try (Node node = new MockNode(settings.build(), Collections.singleton(MockTcpTransportPlugin.class))) {
fail("should not allow a node attribute with leading whitespace");
} catch (IllegalArgumentException e) {
assertEquals("node.attr.test_attr cannot have leading or trailing whitespace [ leading]", e.getMessage());
}
// trailing whitespace not allowed
attr = "trailing ";
settings = baseSettings().put(Node.NODE_ATTRIBUTES.getKey() + "test_attr", attr);
try (Node node = new MockNode(settings.build(), Collections.singleton(MockTcpTransportPlugin.class))) {
fail("should not allow a node attribute with trailing whitespace");
} catch (IllegalArgumentException e) {
assertEquals("node.attr.test_attr cannot have leading or trailing whitespace [trailing ]", e.getMessage());
}
}
private static Settings.Builder baseSettings() {
final Path tempDir = createTempDir();
return Settings.builder()
.put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", randomLong()))
.put(Environment.PATH_HOME_SETTING.getKey(), tempDir)
.put(NetworkModule.HTTP_ENABLED.getKey(), false)
.put("transport.type", "mock-socket-network")
.put(Node.NODE_DATA_SETTING.getKey(), true);
}
}