Simplify node attributes check

Author: ywelsch

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java

@@ -210,22 +210,13 @@ private static boolean alreadyContainsXPackCustomMetadata(ClusterState clusterSt
     public Settings additionalSettings() {
         final String xpackInstalledNodeAttrSetting = "node.attr." + XPACK_INSTALLED_NODE_ATTR;
 
-        if (transportClientMode) {
-            if (settings.get(xpackInstalledNodeAttrSetting) != null) {
-                throw new IllegalArgumentException("Directly setting [" + xpackInstalledNodeAttrSetting + "] is not permitted");
-            }
+        if (settings.get(xpackInstalledNodeAttrSetting) != null) {
+            throw new IllegalArgumentException("Directly setting [" + xpackInstalledNodeAttrSetting + "] is not permitted");
+        }
 
+        if (transportClientMode) {
             return super.additionalSettings();
         } else {
-            // Unfortunately we cannot simply disallow any value for xpackInstalledNodeAttrSetting, because the
-            // internal cluster integration test framework will restart nodes with settings copied from the node
-            // immediately before it was stopped.  The best we can do is reject inconsistencies.
-            // TODO: fix the test framework not to copy derived node settings upon restart.
-            if (settings.get(xpackInstalledNodeAttrSetting) != null &&
-                settings.get(xpackInstalledNodeAttrSetting).equals("true") == false) {
-                throw new IllegalArgumentException("Conflicting setting [" + xpackInstalledNodeAttrSetting + "]");
-            }
-
             return Settings.builder().put(super.additionalSettings()).put(xpackInstalledNodeAttrSetting, "true").build();
         }
     }

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/XPackPluginTests.java

@@ -25,25 +25,18 @@
 
 public class XPackPluginTests extends ESTestCase {
 
-    public void testXPackInstalledAttrClashOnTransport() throws Exception {
+    public void testXPackInstalledAttrClash() throws Exception {
         Settings.Builder builder = Settings.builder();
-        builder.put("node.attr." + XPackPlugin.XPACK_INSTALLED_NODE_ATTR, "true");
-        builder.put(Client.CLIENT_TYPE_SETTING_S.getKey(), "transport");
+        builder.put("node.attr." + XPackPlugin.XPACK_INSTALLED_NODE_ATTR, randomBoolean());
+        if (randomBoolean()) {
+            builder.put(Client.CLIENT_TYPE_SETTING_S.getKey(), "transport");
+        }
         XPackPlugin xpackPlugin = createXPackPlugin(builder.put("path.home", createTempDir()).build());
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, xpackPlugin::additionalSettings);
         assertThat(e.getMessage(),
             containsString("Directly setting [node.attr." + XPackPlugin.XPACK_INSTALLED_NODE_ATTR + "] is not permitted"));
     }
 
-    public void testXPackInstalledAttrClashOnNode() throws Exception {
-        Settings.Builder builder = Settings.builder();
-        builder.put("node.attr." + XPackPlugin.XPACK_INSTALLED_NODE_ATTR, "false");
-        XPackPlugin xpackPlugin = createXPackPlugin(builder.put("path.home", createTempDir()).build());
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, xpackPlugin::additionalSettings);
-        assertThat(e.getMessage(),
-            containsString("Conflicting setting [node.attr." + XPackPlugin.XPACK_INSTALLED_NODE_ATTR + "]"));
-    }
-
     public void testXPackInstalledAttrExists() throws Exception {
         XPackPlugin xpackPlugin = createXPackPlugin(Settings.builder().put("path.home", createTempDir()).build());
         assertEquals("true", xpackPlugin.additionalSettings().get("node.attr." + XPackPlugin.XPACK_INSTALLED_NODE_ATTR));