Skip to content

Commit 7579cf2

Browse files
author
Ali Beyad
committed
Allow comma delimited array settings to have a space after each entry
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
1 parent 20ab4be commit 7579cf2

File tree

10 files changed

+123
-12
lines changed

10 files changed

+123
-12
lines changed

‎core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -973,20 +973,26 @@ public IndexMetaData build() {
973973
filledInSyncAllocationIds.put(i, Collections.emptySet());
974974
}
975975
}
976-
final Map<String, String> requireMap = INDEX_ROUTING_REQUIRE_GROUP_SETTING.get(settings).getAsMap();
976+
final Settings requireGroupSettings = INDEX_ROUTING_REQUIRE_GROUP_SETTING.get(settings);
977+
DiscoveryNodeFilters.IP_VALIDATOR.accept(requireGroupSettings);
978+
final Map<String, String> requireMap = requireGroupSettings.getAsMap();
977979
final DiscoveryNodeFilters requireFilters;
978980
if (requireMap.isEmpty()) {
979981
requireFilters = null;
980982
} else {
981983
requireFilters = DiscoveryNodeFilters.buildFromKeyValue(AND, requireMap);
982984
}
983-
Map<String, String> includeMap = INDEX_ROUTING_INCLUDE_GROUP_SETTING.get(settings).getAsMap();
985+
final Settings includeGroupSettings = INDEX_ROUTING_INCLUDE_GROUP_SETTING.get(settings);
986+
DiscoveryNodeFilters.IP_VALIDATOR.accept(includeGroupSettings);
987+
Map<String, String> includeMap = includeGroupSettings.getAsMap();
984988
final DiscoveryNodeFilters includeFilters;
985989
if (includeMap.isEmpty()) {
986990
includeFilters = null;
987991
} else {
988992
includeFilters = DiscoveryNodeFilters.buildFromKeyValue(OR, includeMap);
989993
}
994+
final Settings excludeGroupSettings = INDEX_ROUTING_EXCLUDE_GROUP_SETTING.get(settings);
995+
DiscoveryNodeFilters.IP_VALIDATOR.accept(excludeGroupSettings);
990996
Map<String, String> excludeMap = INDEX_ROUTING_EXCLUDE_GROUP_SETTING.get(settings).getAsMap();
991997
final DiscoveryNodeFilters excludeFilters;
992998
if (excludeMap.isEmpty()) {

‎core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodeFilters.java‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,15 @@
2121

2222
import org.elasticsearch.common.Nullable;
2323
import org.elasticsearch.common.Strings;
24+
import org.elasticsearch.common.network.InetAddresses;
2425
import org.elasticsearch.common.network.NetworkAddress;
2526
import org.elasticsearch.common.regex.Regex;
2627
import org.elasticsearch.common.settings.Settings;
2728
import org.elasticsearch.common.transport.TransportAddress;
2829

2930
import java.util.HashMap;
3031
import java.util.Map;
32+
import java.util.function.Consumer;
3133

3234
public class DiscoveryNodeFilters {
3335

@@ -36,14 +38,28 @@ public enum OpType {
3638
OR
3739
}
3840

41+
public static final Consumer<Settings> IP_VALIDATOR = (settings) -> {
42+
Map<String, String> settingsMap = settings.getAsMap();
43+
for (Map.Entry<String, String> entry : settingsMap.entrySet()) {
44+
String propertyKey = entry.getKey();
45+
if ("_ip".equals(propertyKey) || "_host_ip".equals(propertyKey) || "_publish_ip".equals(propertyKey)) {
46+
for (String value : Strings.tokenizeToStringArray(entry.getValue(), ",")) {
47+
if (InetAddresses.isInetAddress(value) == false) {
48+
throw new IllegalArgumentException("invalid IP address [" + value + "] for [" + propertyKey + "]");
49+
}
50+
}
51+
}
52+
}
53+
};
54+
3955
public static DiscoveryNodeFilters buildFromSettings(OpType opType, String prefix, Settings settings) {
4056
return buildFromKeyValue(opType, settings.getByPrefix(prefix).getAsMap());
4157
}
4258

4359
public static DiscoveryNodeFilters buildFromKeyValue(OpType opType, Map<String, String> filters) {
4460
Map<String, String[]> bFilters = new HashMap<>();
4561
for (Map.Entry<String, String> entry : filters.entrySet()) {
46-
String[] values = Strings.splitStringByCommaToArray(entry.getValue());
62+
String[] values = Strings.tokenizeToStringArray(entry.getValue(), ",");
4763
if (values.length > 0) {
4864
bFilters.put(entry.getKey(), values);
4965
}

‎core/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/AwarenessAllocationDecider.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,12 @@ public class AwarenessAllocationDecider extends AllocationDecider {
7878
public static final String NAME = "awareness";
7979

8080
public static final Setting<String[]> CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING =
81-
new Setting<>("cluster.routing.allocation.awareness.attributes", "", Strings::splitStringByCommaToArray , Property.Dynamic,
81+
new Setting<>("cluster.routing.allocation.awareness.attributes", "", s -> Strings.tokenizeToStringArray(s, ","), Property.Dynamic,
8282
Property.NodeScope);
8383
public static final Setting<Settings> CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING =
8484
Setting.groupSetting("cluster.routing.allocation.awareness.force.", Property.Dynamic, Property.NodeScope);
8585

86-
private String[] awarenessAttributes;
86+
private volatile String[] awarenessAttributes;
8787

8888
private volatile Map<String, String[]> forcedAwarenessAttributes;
8989

‎core/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import org.elasticsearch.common.settings.Setting.Property;
3131
import org.elasticsearch.common.settings.Settings;
3232

33+
import static org.elasticsearch.cluster.node.DiscoveryNodeFilters.IP_VALIDATOR;
3334
import static org.elasticsearch.cluster.node.DiscoveryNodeFilters.OpType.AND;
3435
import static org.elasticsearch.cluster.node.DiscoveryNodeFilters.OpType.OR;
3536

@@ -83,9 +84,9 @@ public FilterAllocationDecider(Settings settings, ClusterSettings clusterSetting
8384
setClusterRequireFilters(CLUSTER_ROUTING_REQUIRE_GROUP_SETTING.get(settings));
8485
setClusterExcludeFilters(CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING.get(settings));
8586
setClusterIncludeFilters(CLUSTER_ROUTING_INCLUDE_GROUP_SETTING.get(settings));
86-
clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_REQUIRE_GROUP_SETTING, this::setClusterRequireFilters);
87-
clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING, this::setClusterExcludeFilters);
88-
clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_INCLUDE_GROUP_SETTING, this::setClusterIncludeFilters);
87+
clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_REQUIRE_GROUP_SETTING, this::setClusterRequireFilters, IP_VALIDATOR);
88+
clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING, this::setClusterExcludeFilters, IP_VALIDATOR);
89+
clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_INCLUDE_GROUP_SETTING, this::setClusterIncludeFilters, IP_VALIDATOR);
8990
}
9091

9192
@Override

‎core/src/test/java/org/elasticsearch/cluster/allocation/FilteringAllocationIT.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,14 @@
2121

2222
import org.apache.logging.log4j.Logger;
2323
import org.elasticsearch.cluster.ClusterState;
24+
import org.elasticsearch.cluster.metadata.IndexMetaData;
2425
import org.elasticsearch.cluster.routing.IndexRoutingTable;
2526
import org.elasticsearch.cluster.routing.IndexShardRoutingTable;
2627
import org.elasticsearch.cluster.routing.ShardRouting;
28+
import org.elasticsearch.cluster.routing.allocation.decider.FilterAllocationDecider;
2729
import org.elasticsearch.cluster.routing.allocation.decider.ThrottlingAllocationDecider;
2830
import org.elasticsearch.common.logging.Loggers;
31+
import org.elasticsearch.common.settings.Setting;
2932
import org.elasticsearch.common.settings.Settings;
3033
import org.elasticsearch.index.query.QueryBuilders;
3134
import org.elasticsearch.test.ESIntegTestCase;
@@ -144,5 +147,16 @@ public void testDisablingAllocationFiltering() throws Exception {
144147
clusterState = client().admin().cluster().prepareState().execute().actionGet().getState();
145148
assertThat(clusterState.routingTable().index("test").numberOfNodesShardsAreAllocatedOn(), equalTo(2));
146149
}
150+
151+
public void testInvalidIPFilterClusterSettings() {
152+
String ipKey = randomFrom("_ip", "_host_ip", "_publish_ip");
153+
Setting<Settings> filterSetting = randomFrom(FilterAllocationDecider.CLUSTER_ROUTING_REQUIRE_GROUP_SETTING,
154+
FilterAllocationDecider.CLUSTER_ROUTING_INCLUDE_GROUP_SETTING, FilterAllocationDecider.CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING);
155+
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> client().admin().cluster().prepareUpdateSettings()
156+
.setTransientSettings(Settings.builder().put(filterSetting.getKey() + ipKey, "192.168.1.1."))
157+
.execute().actionGet());
158+
assertEquals("illegal value can't update [" + filterSetting.getKey() + "] from [{}] to [{" + ipKey + "=192.168.1.1.}]",
159+
e.getMessage());
160+
}
147161
}
148162

‎core/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeFiltersTests.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,17 @@ public void testIpPublishFilteringNotMatchingOr() {
245245
assertThat(filters.match(node), equalTo(true));
246246
}
247247

248+
public void testCommaSeparatedValuesTrimmed() {
249+
DiscoveryNode node = new DiscoveryNode("", "", "", "", "192.1.1.54", localAddress, singletonMap("tag", "B"), emptySet(), null);
250+
251+
Settings settings = shuffleSettings(Settings.builder()
252+
.put("xxx." + randomFrom("_ip", "_host_ip", "_publish_ip"), "192.1.1.1, 192.1.1.54")
253+
.put("xxx.tag", "A, B")
254+
.build());
255+
DiscoveryNodeFilters filters = DiscoveryNodeFilters.buildFromSettings(OR, "xxx.", settings);
256+
assertTrue(filters.match(node));
257+
}
258+
248259
private Settings shuffleSettings(Settings source) {
249260
Settings.Builder settings = Settings.builder();
250261
List<String> keys = new ArrayList<>(source.getAsMap().keySet());

‎core/src/test/java/org/elasticsearch/cluster/routing/allocation/AwarenessAllocationTests.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,14 @@
3232
import org.elasticsearch.cluster.routing.allocation.command.AllocationCommands;
3333
import org.elasticsearch.cluster.routing.allocation.command.CancelAllocationCommand;
3434
import org.elasticsearch.cluster.routing.allocation.command.MoveAllocationCommand;
35+
import org.elasticsearch.cluster.routing.allocation.decider.AwarenessAllocationDecider;
3536
import org.elasticsearch.cluster.routing.allocation.decider.ClusterRebalanceAllocationDecider;
3637
import org.elasticsearch.common.logging.Loggers;
3738
import org.elasticsearch.common.settings.Settings;
3839

40+
import java.util.HashMap;
41+
import java.util.Map;
42+
3943
import static java.util.Collections.singletonMap;
4044
import static org.elasticsearch.cluster.routing.ShardRoutingState.INITIALIZING;
4145
import static org.elasticsearch.cluster.routing.ShardRoutingState.RELOCATING;
@@ -803,4 +807,50 @@ public void testUnassignedShardsWithUnbalancedZones() {
803807
assertThat(clusterState.getRoutingNodes().shardsWithState(INITIALIZING).size(), equalTo(4)); // +1 for relocating shard.
804808
assertThat(clusterState.getRoutingNodes().shardsWithState(UNASSIGNED).size(), equalTo(1)); // Still 1 unassigned.
805809
}
810+
811+
public void testMultipleAwarenessAttributes() {
812+
AllocationService strategy = createAllocationService(Settings.builder()
813+
.put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack")
814+
.put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "zone.values", "a, b")
815+
.put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "rack.values", "c, d")
816+
.put(ClusterRebalanceAllocationDecider.CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE_SETTING.getKey(), "always")
817+
.build());
818+
819+
logger.info("Building initial routing table for 'testUnbalancedZones'");
820+
821+
MetaData metaData = MetaData.builder()
822+
.put(IndexMetaData.builder("test").settings(settings(Version.CURRENT)).numberOfShards(1).numberOfReplicas(1))
823+
.build();
824+
825+
RoutingTable initialRoutingTable = RoutingTable.builder().addAsNew(metaData.index("test")).build();
826+
827+
ClusterState clusterState = ClusterState.builder(
828+
org.elasticsearch.cluster.ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)
829+
).metaData(metaData).routingTable(initialRoutingTable).build();
830+
831+
logger.info("--> adding two nodes in different zones and do rerouting");
832+
Map<String, String> nodeAAttributes = new HashMap<>();
833+
nodeAAttributes.put("zone", "a");
834+
nodeAAttributes.put("rack", "c");
835+
Map<String, String> nodeBAttributes = new HashMap<>();
836+
nodeBAttributes.put("zone", "b");
837+
nodeBAttributes.put("rack", "d");
838+
clusterState = ClusterState.builder(clusterState).nodes(DiscoveryNodes.builder()
839+
.add(newNode("A-0", nodeAAttributes))
840+
.add(newNode("B-0", nodeBAttributes))
841+
).build();
842+
clusterState = strategy.reroute(clusterState, "reroute");
843+
assertThat(clusterState.getRoutingNodes().shardsWithState(STARTED).size(), equalTo(0));
844+
assertThat(clusterState.getRoutingNodes().shardsWithState(INITIALIZING).size(), equalTo(1));
845+
846+
logger.info("--> start the shards (primaries)");
847+
clusterState = strategy.applyStartedShards(clusterState, clusterState.getRoutingNodes().shardsWithState(INITIALIZING));
848+
assertThat(clusterState.getRoutingNodes().shardsWithState(STARTED).size(), equalTo(1));
849+
assertThat(clusterState.getRoutingNodes().shardsWithState(INITIALIZING).size(), equalTo(1));
850+
851+
clusterState = strategy.applyStartedShards(clusterState, clusterState.getRoutingNodes().shardsWithState(INITIALIZING));
852+
logger.info("--> all replicas are allocated and started since we have one node in each zone and rack");
853+
assertThat(clusterState.getRoutingNodes().shardsWithState(STARTED).size(), equalTo(2));
854+
assertThat(clusterState.getRoutingNodes().shardsWithState(INITIALIZING).size(), equalTo(0));
855+
}
806856
}

‎core/src/test/java/org/elasticsearch/cluster/routing/allocation/FilterAllocationDeciderTests.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import org.elasticsearch.cluster.routing.allocation.decider.ReplicaAfterPrimaryActiveAllocationDecider;
3838
import org.elasticsearch.cluster.routing.allocation.decider.SameShardAllocationDecider;
3939
import org.elasticsearch.common.settings.ClusterSettings;
40+
import org.elasticsearch.common.settings.Setting;
4041
import org.elasticsearch.common.settings.Settings;
4142
import org.elasticsearch.snapshots.Snapshot;
4243
import org.elasticsearch.snapshots.SnapshotId;
@@ -191,4 +192,14 @@ private ClusterState createInitialClusterState(AllocationService service, Settin
191192
.build();
192193
return service.reroute(clusterState, "reroute", false);
193194
}
195+
196+
public void testInvalidIPFilter() {
197+
String ipKey = randomFrom("_ip", "_host_ip", "_publish_ip");
198+
Setting<Settings> filterSetting = randomFrom(IndexMetaData.INDEX_ROUTING_REQUIRE_GROUP_SETTING,
199+
IndexMetaData.INDEX_ROUTING_INCLUDE_GROUP_SETTING, IndexMetaData.INDEX_ROUTING_EXCLUDE_GROUP_SETTING);
200+
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
201+
IndexMetaData.builder("test").settings(settings(Version.CURRENT).put(filterSetting + ipKey, "192.168.1.1."))
202+
.numberOfShards(1).numberOfReplicas(0).build());
203+
assertEquals("invalid IP address [192.168.1.1.] for [" + ipKey + "]", e.getMessage());
204+
}
194205
}

‎modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -420,14 +420,14 @@ static Netty4CorsConfig buildCorsConfig(Settings settings) {
420420
if (SETTING_CORS_ALLOW_CREDENTIALS.get(settings)) {
421421
builder.allowCredentials();
422422
}
423-
String[] strMethods = Strings.splitStringByCommaToArray(SETTING_CORS_ALLOW_METHODS.get(settings));
423+
String[] strMethods = Strings.tokenizeToStringArray(SETTING_CORS_ALLOW_METHODS.get(settings), ",");
424424
HttpMethod[] methods = Arrays.asList(strMethods)
425425
.stream()
426426
.map(HttpMethod::valueOf)
427427
.toArray(size -> new HttpMethod[size]);
428428
return builder.allowedRequestMethods(methods)
429429
.maxAge(SETTING_CORS_MAX_AGE.get(settings))
430-
.allowedRequestHeaders(Strings.splitStringByCommaToArray(SETTING_CORS_ALLOW_HEADERS.get(settings)))
430+
.allowedRequestHeaders(Strings.tokenizeToStringArray(SETTING_CORS_ALLOW_HEADERS.get(settings), ","))
431431
.shortCircuit()
432432
.build();
433433
}

‎modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import java.util.Set;
5252
import java.util.stream.Collectors;
5353

54+
import static org.elasticsearch.common.Strings.collectionToDelimitedString;
5455
import static org.elasticsearch.http.HttpTransportSettings.SETTING_CORS_ALLOW_CREDENTIALS;
5556
import static org.elasticsearch.http.HttpTransportSettings.SETTING_CORS_ALLOW_HEADERS;
5657
import static org.elasticsearch.http.HttpTransportSettings.SETTING_CORS_ALLOW_METHODS;
@@ -89,11 +90,12 @@ public void shutdown() throws Exception {
8990
public void testCorsConfig() {
9091
final Set<String> methods = new HashSet<>(Arrays.asList("get", "options", "post"));
9192
final Set<String> headers = new HashSet<>(Arrays.asList("Content-Type", "Content-Length"));
93+
final String suffix = randomBoolean() ? " " : ""; // sometimes have a leading whitespace between comma delimited elements
9294
final Settings settings = Settings.builder()
9395
.put(SETTING_CORS_ENABLED.getKey(), true)
9496
.put(SETTING_CORS_ALLOW_ORIGIN.getKey(), "*")
95-
.put(SETTING_CORS_ALLOW_METHODS.getKey(), Strings.collectionToCommaDelimitedString(methods))
96-
.put(SETTING_CORS_ALLOW_HEADERS.getKey(), Strings.collectionToCommaDelimitedString(headers))
97+
.put(SETTING_CORS_ALLOW_METHODS.getKey(), collectionToDelimitedString(methods, ",", suffix, ""))
98+
.put(SETTING_CORS_ALLOW_HEADERS.getKey(), collectionToDelimitedString(headers, ",", suffix, ""))
9799
.put(SETTING_CORS_ALLOW_CREDENTIALS.getKey(), true)
98100
.build();
99101
final Netty4CorsConfig corsConfig = Netty4HttpServerTransport.buildCorsConfig(settings);

0 commit comments

Comments
 (0)