From ab9cc25a8eeb2ec0f8a9c78dc1f68a53ff10f132 Mon Sep 17 00:00:00 2001 From: David Roberts Date: Tue, 10 Oct 2017 15:12:59 +0100 Subject: [PATCH] [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@9c381801d9ffc799fcf3d241efb2b1f3d2da69d4 --- .../xpack/ml/MachineLearning.java | 39 +++++++++- .../xpack/ml/MachineLearningTests.java | 74 +++++++++++++++++++ 2 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 plugin/src/test/java/org/elasticsearch/xpack/ml/MachineLearningTests.java diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java index 4d50a99ec98..156e82eba32 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java +++ b/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"); - additionalSettings.put("node.attr." + MAX_OPEN_JOBS_NODE_ATTR, - AutodetectProcessManager.MAX_OPEN_JOBS_PER_NODE.get(settings)); + addMlNodeAttribute(additionalSettings, mlEnabledNodeAttrName, "true"); + addMlNodeAttribute(additionalSettings, maxOpenJobsPerNodeNodeAttrName, + 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 getNamedWriteables() { return Arrays.asList( // Custom metadata diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/MachineLearningTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/MachineLearningTests.java new file mode 100644 index 00000000000..4eb208fe102 --- /dev/null +++ b/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)); + } +}