Merge pull request #17402 from s1monw/issues/17280

Remove ability to specify arbitrary node attributes with `node.` prefix

Today the basic node settings like `node.data` and `node.master` can't really be fully validated
since we allow to specify custom user attributes on the node level. We have to, in order to
support that, add a wildcard setting for `node.*` to let these setting pass validation.
Instead we should require a more contraint prefix like `node.attr.` that defines a namespace
that is reserved for user attributes.
This commit adds a new namespace for attributes in `node.attr`.

Closes #17280

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy

@@ -258,7 +258,7 @@ class ClusterFormationTasks {
                 'path.repo'                    : "${node.sharedDir}/repo",
                 'path.shared_data'             : "${node.sharedDir}/",
                 // Define a node attribute so we can test that it exists
-                'node.testattr'                : 'test',
+                'node.attr.testattr'                : 'test',
                 'repositories.url.allowed_urls': 'http://snapshot.test*'
         ]
         esConfig['http.port'] = node.config.httpPort

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

@@ -68,20 +68,17 @@ public class DiscoveryNodeService extends AbstractComponent {
     public DiscoveryNode buildLocalNode(TransportAddress publishAddress) {
         final String nodeId = generateNodeId(settings);
         Map<String, String> attributes = new HashMap<>(Node.NODE_ATTRIBUTES.get(this.settings).getAsMap());
-        if (attributes.containsKey("client")) {
+        Set<DiscoveryNode.Role> roles = new HashSet<>();
-            throw new IllegalArgumentException("node.client setting is no longer supported, use " + Node.NODE_MASTER_SETTING.getKey()
+        if (Node.NODE_INGEST_SETTING.get(settings)) {
-                    + ", " + Node.NODE_DATA_SETTING.getKey() + " and " + Node.NODE_INGEST_SETTING.getKey() + " explicitly instead");
+            roles.add(DiscoveryNode.Role.INGEST);
+        }
+        if (Node.NODE_MASTER_SETTING.get(settings)) {
+            roles.add(DiscoveryNode.Role.MASTER);
+        }
+        if (Node.NODE_DATA_SETTING.get(settings)) {
+            roles.add(DiscoveryNode.Role.DATA);
         }
 
-        attributes.remove("name"); // name is extracted in other places
-        Set<DiscoveryNode.Role> roles = new HashSet<>();
-        for (DiscoveryNode.Role role : DiscoveryNode.Role.values()) {
-            String isRoleEnabled = attributes.remove(role.getRoleName());
-            //all existing roles default to true
-            if (isRoleEnabled == null || Booleans.parseBooleanExact(isRoleEnabled)) {
-                roles.add(role);
-            }
-        }
         for (CustomAttributesProvider provider : customAttributesProviders) {
             try {
                 Map<String, String> customAttributes = provider.buildAttributes();

core/src/main/java/org/elasticsearch/node/Node.java

@@ -143,9 +143,7 @@ public class Node implements Closeable {
     public static final Setting<Boolean> NODE_INGEST_SETTING =
         Setting.boolSetting("node.ingest", true, Property.NodeScope);
     public static final Setting<String> NODE_NAME_SETTING = Setting.simpleString("node.name", Property.NodeScope);
-    // this sucks that folks can mistype data, master or ingest and get away with it.
+    public static final Setting<Settings> NODE_ATTRIBUTES = Setting.groupSetting("node.attr.", Property.NodeScope);
-    // TODO: we should move this to node.attribute.${name} = ${value} instead.
-    public static final Setting<Settings> NODE_ATTRIBUTES = Setting.groupSetting("node.", Property.NodeScope);
 
 
     private static final String CLIENT_TYPE = "node";

core/src/test/java/org/elasticsearch/cluster/allocation/AwarenessAllocationIT.java

@@ -61,7 +61,7 @@ public class AwarenessAllocationIT extends ESIntegTestCase {
 
 
         logger.info("--> starting 2 nodes on the same rack");
-        internalCluster().startNodesAsync(2, Settings.settingsBuilder().put(commonSettings).put("node.rack_id", "rack_1").build()).get();
+        internalCluster().startNodesAsync(2, Settings.settingsBuilder().put(commonSettings).put("node.attr.rack_id", "rack_1").build()).get();
 
         createIndex("test1");
         createIndex("test2");
@@ -74,7 +74,7 @@ public class AwarenessAllocationIT extends ESIntegTestCase {
         ensureGreen();
 
         logger.info("--> starting 1 node on a different rack");
-        final String node3 = internalCluster().startNode(Settings.settingsBuilder().put(commonSettings).put("node.rack_id", "rack_2").build());
+        final String node3 = internalCluster().startNode(Settings.settingsBuilder().put(commonSettings).put("node.attr.rack_id", "rack_2").build());
 
         // On slow machines the initial relocation might be delayed
         assertThat(awaitBusy(
@@ -113,10 +113,10 @@ public class AwarenessAllocationIT extends ESIntegTestCase {
 
         logger.info("--> starting 4 nodes on different zones");
         List<String> nodes = internalCluster().startNodesAsync(
-                Settings.settingsBuilder().put(commonSettings).put("node.zone", "a").build(),
+                Settings.settingsBuilder().put(commonSettings).put("node.attr.zone", "a").build(),
-                Settings.settingsBuilder().put(commonSettings).put("node.zone", "b").build(),
+                Settings.settingsBuilder().put(commonSettings).put("node.attr.zone", "b").build(),
-                Settings.settingsBuilder().put(commonSettings).put("node.zone", "b").build(),
+                Settings.settingsBuilder().put(commonSettings).put("node.attr.zone", "b").build(),
-                Settings.settingsBuilder().put(commonSettings).put("node.zone", "a").build()
+                Settings.settingsBuilder().put(commonSettings).put("node.attr.zone", "a").build()
         ).get();
         String A_0 = nodes.get(0);
         String B_0 = nodes.get(1);
@@ -159,8 +159,8 @@ public class AwarenessAllocationIT extends ESIntegTestCase {
 
         logger.info("--> starting 2 nodes on zones 'a' & 'b'");
         List<String> nodes = internalCluster().startNodesAsync(
-                Settings.settingsBuilder().put(commonSettings).put("node.zone", "a").build(),
+                Settings.settingsBuilder().put(commonSettings).put("node.attr.zone", "a").build(),
-                Settings.settingsBuilder().put(commonSettings).put("node.zone", "b").build()
+                Settings.settingsBuilder().put(commonSettings).put("node.attr.zone", "b").build()
         ).get();
         String A_0 = nodes.get(0);
         String B_0 = nodes.get(1);
@@ -183,7 +183,7 @@ public class AwarenessAllocationIT extends ESIntegTestCase {
         assertThat(counts.get(B_0), equalTo(5));
         logger.info("--> starting another node in zone 'b'");
 
-        String B_1 = internalCluster().startNode(Settings.settingsBuilder().put(commonSettings).put("node.zone", "b").build());
+        String B_1 = internalCluster().startNode(Settings.settingsBuilder().put(commonSettings).put("node.attr.zone", "b").build());
         health = client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus().setWaitForNodes("3").execute().actionGet();
         assertThat(health.isTimedOut(), equalTo(false));
         client().admin().cluster().prepareReroute().get();

core/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeServiceTests.java

@@ -33,23 +33,12 @@ import static org.hamcrest.CoreMatchers.equalTo;
 
 public class DiscoveryNodeServiceTests extends ESTestCase {
 
-    public void testClientNodeSettingIsProhibited() {
-        Settings settings = Settings.builder().put("node.client", randomBoolean()).build();
-        try {
-            new DiscoveryNodeService(settings, Version.CURRENT).buildLocalNode(DummyTransportAddress.INSTANCE);
-            fail("build attributes should have failed");
-        } catch(IllegalArgumentException e) {
-            assertThat(e.getMessage(), equalTo("node.client setting is no longer supported, use node.master, " +
-                    "node.data and node.ingest explicitly instead"));
-        }
-    }
-
     public void testBuildLocalNode() {
         Map<String, String> expectedAttributes = new HashMap<>();
         int numCustomSettings = randomIntBetween(0, 5);
         Settings.Builder builder = Settings.builder();
         for (int i = 0; i < numCustomSettings; i++) {
-            builder.put("node.attr" + i, "value" + i);
+            builder.put("node.attr.attr" + i, "value" + i);
             expectedAttributes.put("attr" + i, "value" + i);
         }
         Set<DiscoveryNode.Role> selectedRoles = new HashSet<>();
@@ -76,7 +65,7 @@ public class DiscoveryNodeServiceTests extends ESTestCase {
         int numCustomSettings = randomIntBetween(0, 5);
         Settings.Builder builder = Settings.builder();
         for (int i = 0; i < numCustomSettings; i++) {
-            builder.put("node.attr" + i, "value" + i);
+            builder.put("node.attr.attr" + i, "value" + i);
             expectedAttributes.put("attr" + i, "value" + i);
         }
         DiscoveryNodeService discoveryNodeService = new DiscoveryNodeService(builder.build(), Version.CURRENT);

core/src/test/java/org/elasticsearch/cluster/shards/ClusterSearchShardsIT.java

@@ -46,9 +46,9 @@ public class ClusterSearchShardsIT extends ESIntegTestCase {
     protected Settings nodeSettings(int nodeOrdinal) {
         switch(nodeOrdinal) {
         case 1:
-            return settingsBuilder().put(super.nodeSettings(nodeOrdinal)).put("node.tag", "B").build();
+            return settingsBuilder().put(super.nodeSettings(nodeOrdinal)).put("node.attr.tag", "B").build();
         case 0:
-            return settingsBuilder().put(super.nodeSettings(nodeOrdinal)).put("node.tag", "A").build();
+            return settingsBuilder().put(super.nodeSettings(nodeOrdinal)).put("node.attr.tag", "A").build();
         }
         return super.nodeSettings(nodeOrdinal);
     }

core/src/test/java/org/elasticsearch/index/IndexWithShadowReplicasIT.java

@@ -703,8 +703,8 @@ public class IndexWithShadowReplicasIT extends ESIntegTestCase {
     public void testIndexOnSharedFSRecoversToAnyNode() throws Exception {
         Path dataPath = createTempDir();
         Settings nodeSettings = nodeSettings(dataPath);
-        Settings fooSettings = Settings.builder().put(nodeSettings).put("node.affinity", "foo").build();
+        Settings fooSettings = Settings.builder().put(nodeSettings).put("node.attr.affinity", "foo").build();
-        Settings barSettings = Settings.builder().put(nodeSettings).put("node.affinity", "bar").build();
+        Settings barSettings = Settings.builder().put(nodeSettings).put("node.attr.affinity", "bar").build();
 
         final InternalTestCluster.Async<List<String>> fooNodes = internalCluster().startNodesAsync(2, fooSettings);
         final InternalTestCluster.Async<List<String>> barNodes = internalCluster().startNodesAsync(2, barSettings);

core/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java

@@ -535,8 +535,8 @@ public class IndexRecoveryIT extends ESIntegTestCase {
         // start a master node
         internalCluster().startNode(nodeSettings);
 
-        InternalTestCluster.Async<String> blueFuture = internalCluster().startNodeAsync(Settings.builder().put("node.color", "blue").put(nodeSettings).build());
+        InternalTestCluster.Async<String> blueFuture = internalCluster().startNodeAsync(Settings.builder().put("node.attr.color", "blue").put(nodeSettings).build());
-        InternalTestCluster.Async<String> redFuture = internalCluster().startNodeAsync(Settings.builder().put("node.color", "red").put(nodeSettings).build());
+        InternalTestCluster.Async<String> redFuture = internalCluster().startNodeAsync(Settings.builder().put("node.attr.color", "red").put(nodeSettings).build());
         final String blueNodeName = blueFuture.get();
         final String redNodeName = redFuture.get();
 

docs/reference/index-modules/allocation/filtering.asciidoc

@@ -14,7 +14,7 @@ attribute as follows:
 
 [source,sh]
 ------------------------
-bin/elasticsearch -Ees.node.rack=rack1 -Ees.node.size=big  <1>
+bin/elasticsearch -Ees.node.attr.rack=rack1 -Ees.node.attr.size=big  <1>
 ------------------------
 <1> These attribute settings can also be specified in the `elasticsearch.yml` config file.
 

docs/reference/migration/migrate_5_0/settings.asciidoc

@@ -26,6 +26,13 @@ should be used instead.
 The `name` setting has been removed and is replaced by `node.name`. Usage of
 `-Dname=some_node_name` is not supported anymore.
 
+==== Node attribute settings
+
+Node level attributes used for allocation filtering, forced awareness or other node identification / grouping
+must be prefixed with `node.attr`. In previous versions it was possible to specify node attributes with the `node.`
+prefix. All node attributes except of `node.master`, `node.data` and `node.ingest` must be moved to the new `node.attr.`
+namespace.
+
 ==== Node types settings
 
 The `node.client` setting has been removed. A node with such a setting set will not

docs/reference/modules/cluster/allocation_awareness.asciidoc

@@ -21,7 +21,7 @@ attribute called `rack_id` -- we could use any attribute name.  For example:
 
 [source,sh]
 ----------------------
-./bin/elasticsearch -Ees.node.rack_id=rack_one <1>
+./bin/elasticsearch -Ees.node.attr.rack_id=rack_one <1>
 ----------------------
 <1> This setting could also be specified in the `elasticsearch.yml` config file.
 
@@ -37,12 +37,12 @@ For our example, we'll set the value in the config file:
 cluster.routing.allocation.awareness.attributes: rack_id
 --------------------------------------------------------
 
-With this config in place, let's say we start two nodes with `node.rack_id`
+With this config in place, let's say we start two nodes with `node.attr.rack_id`
 set to `rack_one`, and we create an index with 5 primary shards and 1 replica
 of each primary.  All primaries and replicas are allocated across the two
 nodes.
 
-Now, if we start two more nodes with `node.rack_id` set to `rack_two`,
+Now, if we start two more nodes with `node.attr.rack_id` set to `rack_two`,
 Elasticsearch will move shards across to the new nodes, ensuring (if possible)
 that no two copies of the same shard will be in the same rack. However if `rack_two`
 were to fail, taking down both of its nodes, Elasticsearch will still allocate the lost
@@ -102,10 +102,10 @@ cluster.routing.allocation.awareness.attributes: zone
 -------------------------------------------------------------------
 <1> We must list all possible values that the `zone` attribute can have.
 
-Now, if we start 2 nodes with `node.zone` set to `zone1` and create an index
+Now, if we start 2 nodes with `node.attr.zone` set to `zone1` and create an index
 with 5 shards and 1 replica. The index will be created, but only the 5 primary
 shards will be allocated (with no replicas). Only when we start more shards
-with `node.zone` set to `zone2` will the replicas be allocated.
+with `node.attr.zone` set to `zone2` will the replicas be allocated.
 
 The `cluster.routing.allocation.awareness.*` settings can all be updated
 dynamically on a live cluster with the