From 8b075dbb7501291fc6e751abc57a068964d776f3 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 30 Mar 2016 11:43:04 +0200 Subject: [PATCH] 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 --- .../gradle/test/ClusterFormationTasks.groovy | 2 +- .../cluster/node/DiscoveryNodeService.java | 21 ++++++++----------- .../java/org/elasticsearch/node/Node.java | 4 +--- .../allocation/AwarenessAllocationIT.java | 18 ++++++++-------- .../node/DiscoveryNodeServiceTests.java | 15 ++----------- .../cluster/shards/ClusterSearchShardsIT.java | 4 ++-- .../index/IndexWithShadowReplicasIT.java | 4 ++-- .../indices/recovery/IndexRecoveryIT.java | 4 ++-- .../migration/migrate_5_0/settings.asciidoc | 7 +++++++ 9 files changed, 35 insertions(+), 44 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy index a82fefdc510..25da3588aa6 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy +++ b/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 diff --git a/core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodeService.java b/core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodeService.java index 8dd62e85caa..51c27321dd1 100644 --- a/core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodeService.java +++ b/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 attributes = new HashMap<>(Node.NODE_ATTRIBUTES.get(this.settings).getAsMap()); - if (attributes.containsKey("client")) { - throw new IllegalArgumentException("node.client setting is no longer supported, use " + Node.NODE_MASTER_SETTING.getKey() - + ", " + Node.NODE_DATA_SETTING.getKey() + " and " + Node.NODE_INGEST_SETTING.getKey() + " explicitly instead"); + Set roles = new HashSet<>(); + if (Node.NODE_INGEST_SETTING.get(settings)) { + 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 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 customAttributes = provider.buildAttributes(); diff --git a/core/src/main/java/org/elasticsearch/node/Node.java b/core/src/main/java/org/elasticsearch/node/Node.java index 363250e07cd..3f2a2455fce 100644 --- a/core/src/main/java/org/elasticsearch/node/Node.java +++ b/core/src/main/java/org/elasticsearch/node/Node.java @@ -143,9 +143,7 @@ public class Node implements Closeable { public static final Setting NODE_INGEST_SETTING = Setting.boolSetting("node.ingest", true, Property.NodeScope); public static final Setting NODE_NAME_SETTING = Setting.simpleString("node.name", Property.NodeScope); - // this sucks that folks can mistype data, master or ingest and get away with it. - // TODO: we should move this to node.attribute.${name} = ${value} instead. - public static final Setting NODE_ATTRIBUTES = Setting.groupSetting("node.", Property.NodeScope); + public static final Setting NODE_ATTRIBUTES = Setting.groupSetting("node.attr.", Property.NodeScope); private static final String CLIENT_TYPE = "node"; diff --git a/core/src/test/java/org/elasticsearch/cluster/allocation/AwarenessAllocationIT.java b/core/src/test/java/org/elasticsearch/cluster/allocation/AwarenessAllocationIT.java index cf948366f6a..37f8572f98c 100644 --- a/core/src/test/java/org/elasticsearch/cluster/allocation/AwarenessAllocationIT.java +++ b/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 nodes = internalCluster().startNodesAsync( - Settings.settingsBuilder().put(commonSettings).put("node.zone", "a").build(), - Settings.settingsBuilder().put(commonSettings).put("node.zone", "b").build(), - Settings.settingsBuilder().put(commonSettings).put("node.zone", "b").build(), - 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.attr.zone", "b").build(), + Settings.settingsBuilder().put(commonSettings).put("node.attr.zone", "b").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 nodes = internalCluster().startNodesAsync( - Settings.settingsBuilder().put(commonSettings).put("node.zone", "a").build(), - Settings.settingsBuilder().put(commonSettings).put("node.zone", "b").build() + Settings.settingsBuilder().put(commonSettings).put("node.attr.zone", "a").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(); diff --git a/core/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeServiceTests.java b/core/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeServiceTests.java index 68b2709f281..c03a5ab06dc 100644 --- a/core/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeServiceTests.java +++ b/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 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 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); diff --git a/core/src/test/java/org/elasticsearch/cluster/shards/ClusterSearchShardsIT.java b/core/src/test/java/org/elasticsearch/cluster/shards/ClusterSearchShardsIT.java index 9c5d80d1283..2878204b680 100644 --- a/core/src/test/java/org/elasticsearch/cluster/shards/ClusterSearchShardsIT.java +++ b/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); } diff --git a/core/src/test/java/org/elasticsearch/index/IndexWithShadowReplicasIT.java b/core/src/test/java/org/elasticsearch/index/IndexWithShadowReplicasIT.java index aa3da8fc840..ada7fc950fd 100644 --- a/core/src/test/java/org/elasticsearch/index/IndexWithShadowReplicasIT.java +++ b/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 barSettings = Settings.builder().put(nodeSettings).put("node.affinity", "bar").build(); + Settings fooSettings = Settings.builder().put(nodeSettings).put("node.attr.affinity", "foo").build(); + Settings barSettings = Settings.builder().put(nodeSettings).put("node.attr.affinity", "bar").build(); final InternalTestCluster.Async> fooNodes = internalCluster().startNodesAsync(2, fooSettings); final InternalTestCluster.Async> barNodes = internalCluster().startNodesAsync(2, barSettings); diff --git a/core/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java b/core/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java index 140ff153b9f..e2138938796 100644 --- a/core/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java +++ b/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 blueFuture = internalCluster().startNodeAsync(Settings.builder().put("node.color", "blue").put(nodeSettings).build()); - InternalTestCluster.Async redFuture = internalCluster().startNodeAsync(Settings.builder().put("node.color", "red").put(nodeSettings).build()); + InternalTestCluster.Async blueFuture = internalCluster().startNodeAsync(Settings.builder().put("node.attr.color", "blue").put(nodeSettings).build()); + InternalTestCluster.Async redFuture = internalCluster().startNodeAsync(Settings.builder().put("node.attr.color", "red").put(nodeSettings).build()); final String blueNodeName = blueFuture.get(); final String redNodeName = redFuture.get(); diff --git a/docs/reference/migration/migrate_5_0/settings.asciidoc b/docs/reference/migration/migrate_5_0/settings.asciidoc index c8166d2c228..ec70f261776 100644 --- a/docs/reference/migration/migrate_5_0/settings.asciidoc +++ b/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