-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only allow x-pack metadata if all nodes are ready #30743
Changes from 1 commit
12110b9
ba94bdd
5f25ea3
8911179
e1437e8
87ad80f
cec9704
ed235a5
1b6ae1d
a70ea64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,15 +9,20 @@ | |
import org.apache.lucene.util.SetOnce; | ||
import org.bouncycastle.operator.OperatorCreationException; | ||
import org.elasticsearch.SpecialPermission; | ||
import org.elasticsearch.Version; | ||
import org.elasticsearch.action.ActionRequest; | ||
import org.elasticsearch.action.ActionResponse; | ||
import org.elasticsearch.action.GenericAction; | ||
import org.elasticsearch.action.support.ActionFilter; | ||
import org.elasticsearch.client.Client; | ||
import org.elasticsearch.client.transport.TransportClient; | ||
import org.elasticsearch.cluster.ClusterState; | ||
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; | ||
import org.elasticsearch.cluster.metadata.MetaData; | ||
import org.elasticsearch.cluster.node.DiscoveryNode; | ||
import org.elasticsearch.cluster.node.DiscoveryNodes; | ||
import org.elasticsearch.cluster.service.ClusterService; | ||
import org.elasticsearch.common.Booleans; | ||
import org.elasticsearch.common.inject.Binder; | ||
import org.elasticsearch.common.inject.Module; | ||
import org.elasticsearch.common.inject.multibindings.Multibinder; | ||
|
@@ -33,6 +38,7 @@ | |
import org.elasticsearch.env.Environment; | ||
import org.elasticsearch.env.NodeEnvironment; | ||
import org.elasticsearch.license.LicenseService; | ||
import org.elasticsearch.license.LicensesMetaData; | ||
import org.elasticsearch.license.Licensing; | ||
import org.elasticsearch.license.XPackLicenseState; | ||
import org.elasticsearch.plugins.ExtensiblePlugin; | ||
|
@@ -46,10 +52,13 @@ | |
import org.elasticsearch.xpack.core.action.TransportXPackUsageAction; | ||
import org.elasticsearch.xpack.core.action.XPackInfoAction; | ||
import org.elasticsearch.xpack.core.action.XPackUsageAction; | ||
import org.elasticsearch.xpack.core.ml.MLMetadataField; | ||
import org.elasticsearch.xpack.core.rest.action.RestXPackInfoAction; | ||
import org.elasticsearch.xpack.core.rest.action.RestXPackUsageAction; | ||
import org.elasticsearch.xpack.core.security.authc.TokenMetaData; | ||
import org.elasticsearch.xpack.core.ssl.SSLConfigurationReloader; | ||
import org.elasticsearch.xpack.core.ssl.SSLService; | ||
import org.elasticsearch.xpack.core.watcher.WatcherMetaData; | ||
|
||
import javax.security.auth.DestroyFailedException; | ||
|
||
|
@@ -62,14 +71,19 @@ | |
import java.time.Clock; | ||
import java.util.ArrayList; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.function.Supplier; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.StreamSupport; | ||
|
||
public class XPackPlugin extends XPackClientPlugin implements ScriptPlugin, ExtensiblePlugin { | ||
|
||
private static Logger logger = ESLoggerFactory.getLogger(XPackPlugin.class); | ||
private static DeprecationLogger deprecationLogger = new DeprecationLogger(logger); | ||
|
||
public static final String XPACK_INSTALLED_NODE_ATTR = "xpack.installed"; | ||
|
||
// TODO: clean up this library to not ask for write access to all system properties! | ||
static { | ||
// invoke this clinit in unbound with permissions to access all system properties | ||
|
@@ -138,6 +152,78 @@ protected Clock getClock() { | |
public static LicenseService getSharedLicenseService() { return licenseService.get(); } | ||
public static XPackLicenseState getSharedLicenseState() { return licenseState.get(); } | ||
|
||
/** | ||
* Checks if the cluster state allows this node to add x-pack metadata to the cluster state, | ||
* and throws an exception otherwise. | ||
* This check should be called before installing any x-pack metadata to the cluster state, | ||
* to ensure that the other nodes that are part of the cluster will be able to deserialize | ||
* that metadata. | ||
* Having this check properly in place everywhere allows to install x-pack into a cluster | ||
* using a rolling restart. | ||
*/ | ||
public static void checkReadyForXPackCustomMetadata(ClusterState clusterState) { | ||
List<DiscoveryNode> notReadyNodes = nodesNotReadyForXPackCustomMetadata(clusterState); | ||
if (notReadyNodes.isEmpty() == false) { | ||
throw new IllegalStateException("The following nodes are not ready yet for enabling x-pack custom metadata: " + notReadyNodes); | ||
} | ||
} | ||
|
||
/** | ||
* Checks if the cluster state allows this node to add x-pack metadata to the cluster state. | ||
* See {@link #checkReadyForXPackCustomMetadata} for more details. | ||
*/ | ||
public static boolean isReadyForXPackCustomMetadata(ClusterState clusterState) { | ||
return nodesNotReadyForXPackCustomMetadata(clusterState).isEmpty(); | ||
} | ||
|
||
/** | ||
* Returns the list of nodes that won't allow this node from adding x-pack metadata to the cluster state. | ||
* See {@link #checkReadyForXPackCustomMetadata} for more details. | ||
*/ | ||
public static List<DiscoveryNode> nodesNotReadyForXPackCustomMetadata(ClusterState clusterState) { | ||
// check if there's already x-pack metadata in the cluster state; if so, any further metadata won't hurt | ||
final MetaData metaData = clusterState.metaData(); | ||
if (metaData.custom(LicensesMetaData.TYPE) != null || | ||
metaData.custom(MLMetadataField.TYPE) != null || | ||
metaData.custom(WatcherMetaData.TYPE) != null || | ||
clusterState.custom(TokenMetaData.TYPE) != null) { | ||
return Collections.emptyList(); | ||
} | ||
|
||
// if there's no x-pack metadata yet in the cluster state, check that all nodes would be capable | ||
// of deserializing newly added x-pack metadata | ||
final List<DiscoveryNode> notReadyNodes = StreamSupport.stream(clusterState.nodes().spliterator(), false).filter(node -> { | ||
final String xpackInstalledAttr = node.getAttributes().getOrDefault(XPACK_INSTALLED_NODE_ATTR, "false"); | ||
|
||
// The node attribute XPACK_INSTALLED_NODE_ATTR was only introduced in 6.3.0, so when | ||
// we have an older node in this mixed-version cluster without any x-pack metadata, | ||
// we want to prevent x-pack from adding custom metadata | ||
return node.getVersion().before(Version.V_6_3_0) || Booleans.parseBoolean(xpackInstalledAttr) == false; | ||
}).collect(Collectors.toList()); | ||
|
||
return notReadyNodes; | ||
} | ||
|
||
@Override | ||
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"); | ||
} | ||
|
||
return super.additionalSettings(); | ||
} else { | ||
if (settings.get(xpackInstalledNodeAttrSetting) != null && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we allow this setting to be already set in this case? shouldn't we lock it down? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think its because the internal cluster integration test framework will restart nodes with settings copied from the node immediately before it was stopped. There's a comment here for the same problem that could be copied over to prevent confusion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exactly. I wanted to fix that behavior, but did not want to make it part of this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Makes sense. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a comment in ba94bdd There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've opened #30780 which would allow me to make this check more strict. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've merged that PR and made the check more strict in 1b6ae1d |
||
settings.get(xpackInstalledNodeAttrSetting).equals("true") == false) { | ||
throw new IllegalArgumentException("Conflicting setting [" + xpackInstalledNodeAttrSetting + "]"); | ||
} | ||
|
||
return Settings.builder().put(super.additionalSettings()).put(xpackInstalledNodeAttrSetting, "true").build(); | ||
} | ||
} | ||
|
||
@Override | ||
public Collection<Module> createGuiceModules() { | ||
ArrayList<Module> modules = new ArrayList<>(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/* | ||
* 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.core; | ||
|
||
import org.elasticsearch.client.Client; | ||
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.license.XPackLicenseState; | ||
import org.elasticsearch.test.ESTestCase; | ||
import org.elasticsearch.xpack.core.ssl.SSLService; | ||
|
||
import static org.hamcrest.Matchers.containsString; | ||
|
||
public class XPackPluginTests extends ESTestCase { | ||
|
||
public void testXPackInstalledAttrClashOnTransport() 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"); | ||
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)); | ||
} | ||
|
||
private XPackPlugin createXPackPlugin(Settings settings) throws Exception { | ||
return new XPackPlugin(settings, null){ | ||
|
||
@Override | ||
protected void setSslService(SSLService sslService) { | ||
// disable | ||
} | ||
|
||
@Override | ||
protected void setLicenseState(XPackLicenseState licenseState) { | ||
// disable | ||
} | ||
}; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.gateway.GatewayService; | ||
import org.elasticsearch.threadpool.ThreadPool; | ||
import org.elasticsearch.xpack.core.XPackPlugin; | ||
import org.elasticsearch.xpack.core.ml.MLMetadataField; | ||
import org.elasticsearch.xpack.core.ml.MlMetadata; | ||
|
||
|
@@ -48,6 +49,11 @@ public void clusterChanged(ClusterChangedEvent event) { | |
} | ||
|
||
if (event.localNodeMaster()) { | ||
if (XPackPlugin.isReadyForXPackCustomMetadata(event.state()) == false) { | ||
logger.debug("cannot add ML metadata to cluster as the following nodes might not understand the ML metadata: {}", | ||
() -> XPackPlugin.nodesNotReadyForXPackCustomMetadata(event.state())); | ||
return; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following the changes I made in #30751 there's no need to change this file in this PR. We no longer eagerly install the ML metadata on startup. |
||
MetaData metaData = event.state().metaData(); | ||
installMlMetadata(metaData); | ||
installDailyMaintenanceService(); | ||
|
@@ -63,6 +69,7 @@ private void installMlMetadata(MetaData metaData) { | |
clusterService.submitStateUpdateTask("install-ml-metadata", new ClusterStateUpdateTask() { | ||
@Override | ||
public ClusterState execute(ClusterState currentState) throws Exception { | ||
XPackPlugin.checkReadyForXPackCustomMetadata(currentState); | ||
// If the metadata has been added already don't try to update | ||
if (currentState.metaData().custom(MLMetadataField.TYPE) != null) { | ||
return currentState; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.threadpool.ThreadPool; | ||
import org.elasticsearch.transport.TransportService; | ||
import org.elasticsearch.xpack.core.XPackPlugin; | ||
import org.elasticsearch.xpack.core.ml.action.FinalizeJobExecutionAction; | ||
import org.elasticsearch.xpack.core.ml.MLMetadataField; | ||
import org.elasticsearch.xpack.core.ml.MlMetadata; | ||
|
@@ -57,6 +58,7 @@ protected void masterOperation(FinalizeJobExecutionAction.Request request, Clust | |
clusterService.submitStateUpdateTask(source, new ClusterStateUpdateTask() { | ||
@Override | ||
public ClusterState execute(ClusterState currentState) throws Exception { | ||
XPackPlugin.checkReadyForXPackCustomMetadata(currentState); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @droberts195 this feels strange given the name of the method. If we get so far that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think the only endpoints that need protecting are the ones that put jobs and datafeeds. Until the user has created an ML entity the endpoints that operate on them should be no-ops. (There is actually a bug in this action in that if you passed it a non-existent job ID it would currently fail with an NPE. That's not a disaster as it's an undocumented action intended for internal use, but I will make it more defensive in another PR.) |
||
MlMetadata mlMetadata = currentState.metaData().custom(MLMetadataField.TYPE); | ||
MlMetadata.Builder mlMetadataBuilder = new MlMetadata.Builder(mlMetadata); | ||
Date finishedTime = new Date(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you document that if the cluster state already contains xpack metadata it is always considered "ready"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed 5f25ea3