[ML] Prevent ML node attributes being set directly (elastic/x-pack-elasticsearch#2725)

ML uses node attributes to ensure that the master node knows how many
ML jobs may be allocated to each node.  This change prevents a user
messing up the way these attributes are used by setting them differently
using node.attr.* entries in their elasticsearch.yml.

This covers the "very short term" change outlined in elastic/x-pack-elasticsearch#2649

Original commit: elastic/x-pack-elasticsearch@9c381801d9

plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java

@@ -203,7 +203,11 @@ public class MachineLearning implements ActionPlugin {
     }
 
     public Settings additionalSettings() {
+        String mlEnabledNodeAttrName = "node.attr." + ML_ENABLED_NODE_ATTR;
+        String maxOpenJobsPerNodeNodeAttrName = "node.attr." + MAX_OPEN_JOBS_NODE_ATTR;
+
         if (enabled == false || transportClientMode || tribeNode || tribeNodeClient) {
+            disallowMlNodeAttributes(mlEnabledNodeAttrName, maxOpenJobsPerNodeNodeAttrName);
             return Settings.EMPTY;
         }
 
@@ -211,13 +215,42 @@ public class MachineLearning implements ActionPlugin {
         Boolean allocationEnabled = ML_ENABLED.get(settings);
         if (allocationEnabled != null && allocationEnabled) {
             // TODO: the simple true/false flag will not be required once all supported versions have the number - consider removing in 7.0
-            additionalSettings.put("node.attr." + ML_ENABLED_NODE_ATTR, "true");
+            addMlNodeAttribute(additionalSettings, mlEnabledNodeAttrName, "true");
-            additionalSettings.put("node.attr." + MAX_OPEN_JOBS_NODE_ATTR,
+            addMlNodeAttribute(additionalSettings, maxOpenJobsPerNodeNodeAttrName,
-                    AutodetectProcessManager.MAX_OPEN_JOBS_PER_NODE.get(settings));
+                    String.valueOf(AutodetectProcessManager.MAX_OPEN_JOBS_PER_NODE.get(settings)));
+        } else {
+            disallowMlNodeAttributes(mlEnabledNodeAttrName, maxOpenJobsPerNodeNodeAttrName);
         }
         return additionalSettings.build();
     }
 
+    private void addMlNodeAttribute(Settings.Builder additionalSettings, String attrName, String value) {
+        // Unfortunately we cannot simply disallow any value, 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, and report this in a way that
+        // makes clear that setting the node attribute directly is not allowed.
+        String oldValue = settings.get(attrName);
+        if (oldValue == null || oldValue.equals(value)) {
+            additionalSettings.put(attrName, value);
+        } else {
+            reportClashingNodeAttribute(attrName);
+        }
+    }
+
+    private void disallowMlNodeAttributes(String... mlNodeAttributes) {
+        for (String attrName : mlNodeAttributes) {
+            if (settings.get(attrName) != null) {
+                reportClashingNodeAttribute(attrName);
+            }
+        }
+    }
+
+    private void reportClashingNodeAttribute(String attrName) {
+        throw new IllegalArgumentException("Directly setting [" + attrName + "] is not permitted - " +
+                "it is reserved for machine learning. If your intention was to customize machine learning, set the [" +
+                attrName.replace("node.attr.", "xpack.") + "] setting instead.");
+    }
+
     public List<NamedWriteableRegistry.Entry> getNamedWriteables() {
         return Arrays.asList(
                 // Custom metadata

plugin/src/test/java/org/elasticsearch/xpack/ml/MachineLearningTests.java

@@ -0,0 +1,74 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+package org.elasticsearch.xpack.ml;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.env.Environment;
+import org.elasticsearch.license.XPackLicenseState;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.startsWith;
+import static org.mockito.Mockito.mock;
+
+public class MachineLearningTests extends ESTestCase {
+
+    public void testNoAttributes_givenNoClash() {
+        Settings.Builder builder = Settings.builder();
+        if (randomBoolean()) {
+            builder.put("xpack.ml.enabled", randomBoolean());
+        }
+        if (randomBoolean()) {
+            builder.put("xpack.ml.max_open_jobs", randomIntBetween(9, 12));
+        }
+        builder.put("node.attr.foo", "abc");
+        builder.put("node.attr.ml.bar", "def");
+        MachineLearning machineLearning = createMachineLearning(builder.build());
+        assertNotNull(machineLearning.additionalSettings());
+    }
+
+    public void testNoAttributes_givenSame() {
+        Settings.Builder builder = Settings.builder();
+        if (randomBoolean()) {
+            boolean enabled = randomBoolean();
+            builder.put("xpack.ml.enabled", enabled);
+            builder.put("node.attr.ml.enabled", enabled);
+        }
+        if (randomBoolean()) {
+            int maxOpenJobs = randomIntBetween(5, 15);
+            builder.put("xpack.ml.max_open_jobs", maxOpenJobs);
+            builder.put("node.attr.ml.max_open_jobs", maxOpenJobs);
+        }
+        MachineLearning machineLearning = createMachineLearning(builder.build());
+        assertNotNull(machineLearning.additionalSettings());
+    }
+
+    public void testNoAttributes_givenClash() {
+        Settings.Builder builder = Settings.builder();
+        boolean enabled = true;
+        if (randomBoolean()) {
+            enabled = randomBoolean();
+            builder.put("xpack.ml.enabled", enabled);
+        }
+        if (randomBoolean()) {
+            builder.put("xpack.ml.max_open_jobs", randomIntBetween(9, 12));
+        }
+        if (randomBoolean()) {
+            builder.put("node.attr.ml.enabled", !enabled);
+        } else {
+            builder.put("node.attr.ml.max_open_jobs", randomIntBetween(13, 15));
+        }
+        MachineLearning machineLearning = createMachineLearning(builder.build());
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, machineLearning::additionalSettings);
+        assertThat(e.getMessage(), startsWith("Directly setting [node.attr.ml."));
+        assertThat(e.getMessage(), containsString("] is not permitted - " +
+                "it is reserved for machine learning. If your intention was to customize machine learning, set the [xpack.ml."));
+    }
+
+    private MachineLearning createMachineLearning(Settings settings) {
+        return new MachineLearning(settings, mock(Environment.class), mock(XPackLicenseState.class));
+    }
+}