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

abeyad · 2017-01-12T15:23:40Z

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

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 elastic#22297

s1monw

I left some questions Looks good though

s1monw · 2017-01-12T15:26:58Z

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

@@ -36,14 +38,28 @@
        OR
    }

+    public static final Consumer<Settings> IP_VALIDATOR = (settings) -> {


can you put some javadocs on this

s1monw · 2017-01-12T15:27:57Z

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

@@ -973,20 +973,26 @@ public IndexMetaData build() {
                    filledInSyncAllocationIds.put(i, Collections.emptySet());
                }
            }
-            final Map<String, String> requireMap = INDEX_ROUTING_REQUIRE_GROUP_SETTING.get(settings).getAsMap();
+            final Settings requireGroupSettings = INDEX_ROUTING_REQUIRE_GROUP_SETTING.get(settings);
+            DiscoveryNodeFilters.IP_VALIDATOR.accept(requireGroupSettings);


why don't you use this as a validator on the actual setting?

thanks for alerting me to this, i didn't realize a validator could be set on group settings until you mentioned it. I fixed this and all other occurrences below

s1monw · 2017-01-12T15:28:03Z

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

            final DiscoveryNodeFilters requireFilters;
            if (requireMap.isEmpty()) {
                requireFilters = null;
            } else {
                requireFilters = DiscoveryNodeFilters.buildFromKeyValue(AND, requireMap);
            }
-            Map<String, String> includeMap = INDEX_ROUTING_INCLUDE_GROUP_SETTING.get(settings).getAsMap();
+            final Settings includeGroupSettings = INDEX_ROUTING_INCLUDE_GROUP_SETTING.get(settings);
+            DiscoveryNodeFilters.IP_VALIDATOR.accept(includeGroupSettings);


s1monw · 2017-01-12T15:28:08Z

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

            final DiscoveryNodeFilters includeFilters;
            if (includeMap.isEmpty()) {
                includeFilters = null;
            } else {
                includeFilters = DiscoveryNodeFilters.buildFromKeyValue(OR, includeMap);
            }
+            final Settings excludeGroupSettings = INDEX_ROUTING_EXCLUDE_GROUP_SETTING.get(settings);
+            DiscoveryNodeFilters.IP_VALIDATOR.accept(excludeGroupSettings);


s1monw · 2017-01-12T15:28:14Z

...in/java/org/elasticsearch/cluster/routing/allocation/decider/AwarenessAllocationDecider.java

            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;


s1monw · 2017-01-12T15:28:41Z

.../main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java

-        clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_REQUIRE_GROUP_SETTING, this::setClusterRequireFilters);
-        clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING, this::setClusterExcludeFilters);
-        clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_INCLUDE_GROUP_SETTING, this::setClusterIncludeFilters);
+        clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_REQUIRE_GROUP_SETTING, this::setClusterRequireFilters, IP_VALIDATOR);


hmm use the validator on the actual setting instead?

abeyad · 2017-01-12T16:38:21Z

@s1monw I pushed 1ca2d47 to address your feedback

s1monw

LGTM

abeyad · 2017-01-13T14:57:08Z

retest this please

abeyad · 2017-01-16T23:21:10Z

@s1monw I pushed 953f016 which also prohibits node attributes to have leading or trailing whitespace, if you can take a quick look
/cc @jasontedor

jasontedor

LGTM.

s1monw

LGTM 2

…#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

abeyad · 2017-01-17T14:56:31Z

5.x commit: c763b1e

abeyad · 2017-01-17T14:56:41Z

thanks @s1monw @jasontedor

Fixes the broken usage of wildcards for IP-based allocation filtering (introduced by PR #22591), which is documented at https://www.elastic.co/guide/en/elasticsearch/reference/current/shard-allocation-filtering.html Closes #26184

abeyad added :Core/Infra/Settings Settings infrastructure and APIs >enhancement review v5.2.1 v6.0.0-alpha1 labels Jan 12, 2017

s1monw added v5.3.0 and removed v5.2.1 labels Jan 12, 2017

s1monw suggested changes Jan 12, 2017

View reviewed changes

address code review

1ca2d47

s1monw approved these changes Jan 13, 2017

View reviewed changes

disallow node attributes with leading or trailing whitespace

953f016

jasontedor approved these changes Jan 16, 2017

View reviewed changes

s1monw approved these changes Jan 17, 2017

View reviewed changes

abeyad merged commit e297788 into elastic:master Jan 17, 2017

abeyad deleted the trim_filter_allocation_settings branch January 17, 2017 14:51

danielmitterdorfer mentioned this pull request May 16, 2017

Cannot 'reset' index settings to default #24709

Closed

This was referenced Aug 14, 2017

Shard IP filtering does not support wildcards (which breaks 2.x to 5.5.1 upgrade) #26184

Closed

Allow wildcards for shard IP filtering #26187

Merged

jasontedor mentioned this pull request Dec 8, 2017

Fix routing with leading or trailing whitespace #27712

Merged

ywelsch mentioned this pull request Jan 15, 2018

NPE when upgrading from 5.2 to 5.6 with index.routing.allocation.exclude.tag: null #28213

Closed

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

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

Uh oh!

Conversation

abeyad commented Jan 12, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abeyad commented Jan 12, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

abeyad commented Jan 13, 2017

Uh oh!

abeyad commented Jan 16, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

abeyad commented Jan 17, 2017

Uh oh!

abeyad commented Jan 17, 2017

Uh oh!

Uh oh!