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/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