From 8033c576b71d6577c13fc23fac37787333dd14a5 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 17 Apr 2017 07:03:46 -0400 Subject: [PATCH] Detect remnants of path.data/default.path.data bug In Elasticsearch 5.3.0 a bug was introduced in the merging of default settings when the target setting existed as an array. When this bug concerns path.data and default.path.data, we ended up in a situation where the paths specified in both settings would be used to write index data. Since our packaging sets default.path.data, users that configure multiple data paths via an array and use the packaging are subject to having shards land in paths in default.path.data when that is very likely not what they intended. This commit is an attempt to rectify this situation. If path.data and default.path.data are configured, we check for the presence of indices there. If we find any, we log messages explaining the situation and fail the node. Relates #24099 --- .../org/elasticsearch/bootstrap/Security.java | 28 +++++- .../elasticsearch/env/NodeEnvironment.java | 56 ++++++++++-- .../java/org/elasticsearch/node/Node.java | 59 +++++++++++++ .../org/elasticsearch/node/NodeTests.java | 87 +++++++++++++++++-- 4 files changed, 213 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java index de16bbe76aa42..2af6ee33b374f 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -20,7 +20,6 @@ package org.elasticsearch.bootstrap; import org.elasticsearch.SecureSM; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.network.NetworkModule; @@ -45,11 +44,9 @@ import java.security.Permissions; import java.security.Policy; import java.security.URIParameter; -import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashSet; -import java.util.List; import java.util.Map; import java.util.Set; @@ -269,6 +266,26 @@ static void addFilePermissions(Permissions policy, Environment environment) { for (Path path : environment.dataFiles()) { addPath(policy, Environment.PATH_DATA_SETTING.getKey(), path, "read,readlink,write,delete"); } + /* + * If path.data and default.path.data are set, we need read access to the paths in default.path.data to check for the existence of + * index directories there that could have arisen from a bug in the handling of simultaneous configuration of path.data and + * default.path.data that was introduced in Elasticsearch 5.3.0. + * + * If path.data is not set then default.path.data would take precedence in setting the data paths for the environment and + * permissions would have been granted above. + * + * If path.data is not set and default.path.data is not set, then we would fallback to the default data directory under + * Elasticsearch home and again permissions would have been granted above. + * + * If path.data is set and default.path.data is not set, there is nothing to do here. + */ + if (Environment.PATH_DATA_SETTING.exists(environment.settings()) + && Environment.DEFAULT_PATH_DATA_SETTING.exists(environment.settings())) { + for (final String path : Environment.DEFAULT_PATH_DATA_SETTING.get(environment.settings())) { + // write permissions are not needed here, we are not going to be writing to any paths here + addPath(policy, Environment.DEFAULT_PATH_DATA_SETTING.getKey(), getPath(path), "read,readlink"); + } + } for (Path path : environment.repoFiles()) { addPath(policy, Environment.PATH_REPO_SETTING.getKey(), path, "read,readlink,write,delete"); } @@ -278,6 +295,11 @@ static void addFilePermissions(Permissions policy, Environment environment) { } } + @SuppressForbidden(reason = "read path that is not configured in environment") + private static Path getPath(final String path) { + return PathUtils.get(path); + } + /** * Add dynamic {@link SocketPermission}s based on HTTP and transport settings. * diff --git a/core/src/main/java/org/elasticsearch/env/NodeEnvironment.java b/core/src/main/java/org/elasticsearch/env/NodeEnvironment.java index ab969b17d499b..dec59f97f42fc 100644 --- a/core/src/main/java/org/elasticsearch/env/NodeEnvironment.java +++ b/core/src/main/java/org/elasticsearch/env/NodeEnvironment.java @@ -202,7 +202,7 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce for (int dirIndex = 0; dirIndex < environment.dataFiles().length; dirIndex++) { Path dataDirWithClusterName = environment.dataWithClusterFiles()[dirIndex]; Path dataDir = environment.dataFiles()[dirIndex]; - Path dir = dataDir.resolve(NODES_FOLDER).resolve(Integer.toString(possibleLockId)); + Path dir = resolveNodePath(dataDir, possibleLockId); Files.createDirectories(dir); try (Directory luceneDir = FSDirectory.open(dir, NativeFSLockFactory.INSTANCE)) { @@ -268,6 +268,17 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce } } + /** + * Resolve a specific nodes/{node.id} path for the specified path and node lock id. + * + * @param path the path + * @param nodeLockId the node lock id + * @return the resolved path + */ + public static Path resolveNodePath(final Path path, final int nodeLockId) { + return path.resolve(NODES_FOLDER).resolve(Integer.toString(nodeLockId)); + } + /** Returns true if the directory is empty */ private static boolean dirEmpty(final Path path) throws IOException { try (DirectoryStream stream = Files.newDirectoryStream(path)) { @@ -724,6 +735,14 @@ public NodePath[] nodePaths() { return nodePaths; } + public int getNodeLockId() { + assertEnvIsLocked(); + if (nodePaths == null || locks == null) { + throw new IllegalStateException("node is not configured to store local location"); + } + return nodeLockId; + } + /** * Returns all index paths. */ @@ -736,6 +755,8 @@ public Path[] indexPaths(Index index) { return indexPaths; } + + /** * Returns all shard paths excluding custom shard path. Note: Shards are only allocated on one of the * returned paths. The returned array may contain paths to non-existing directories. @@ -764,19 +785,36 @@ public Set availableIndexFolders() throws IOException { assertEnvIsLocked(); Set indexFolders = new HashSet<>(); for (NodePath nodePath : nodePaths) { - Path indicesLocation = nodePath.indicesPath; - if (Files.isDirectory(indicesLocation)) { - try (DirectoryStream stream = Files.newDirectoryStream(indicesLocation)) { - for (Path index : stream) { - if (Files.isDirectory(index)) { - indexFolders.add(index.getFileName().toString()); - } + indexFolders.addAll(availableIndexFoldersForPath(nodePath)); + } + return indexFolders; + + } + + /** + * Return all directory names in the nodes/{node.id}/indices directory for the given node path. + * + * @param nodePath the path + * @return all directories that could be indices for the given node path. + * @throws IOException if an I/O exception occurs traversing the filesystem + */ + public Set availableIndexFoldersForPath(final NodePath nodePath) throws IOException { + if (nodePaths == null || locks == null) { + throw new IllegalStateException("node is not configured to store local location"); + } + assertEnvIsLocked(); + final Set indexFolders = new HashSet<>(); + Path indicesLocation = nodePath.indicesPath; + if (Files.isDirectory(indicesLocation)) { + try (DirectoryStream stream = Files.newDirectoryStream(indicesLocation)) { + for (Path index : stream) { + if (Files.isDirectory(index)) { + indexFolders.add(index.getFileName().toString()); } } } } return indexFolders; - } /** diff --git a/core/src/main/java/org/elasticsearch/node/Node.java b/core/src/main/java/org/elasticsearch/node/Node.java index bf65f5b94419a..2508872eed14a 100644 --- a/core/src/main/java/org/elasticsearch/node/Node.java +++ b/core/src/main/java/org/elasticsearch/node/Node.java @@ -50,6 +50,7 @@ import org.elasticsearch.cluster.routing.allocation.AllocationService; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.StopWatch; +import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.component.Lifecycle; import org.elasticsearch.common.component.LifecycleComponent; import org.elasticsearch.common.inject.Binder; @@ -58,6 +59,7 @@ import org.elasticsearch.common.inject.Module; import org.elasticsearch.common.inject.ModulesBuilder; import org.elasticsearch.common.inject.util.Providers; +import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.logging.DeprecationLogger; @@ -146,7 +148,9 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Locale; import java.util.Map; +import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; @@ -262,6 +266,9 @@ protected Node(final Environment environment, Collection Logger logger = Loggers.getLogger(Node.class, tmpSettings); final String nodeId = nodeEnvironment.nodeId(); tmpSettings = addNodeNameIfNeeded(tmpSettings, nodeId); + if (DiscoveryNode.nodeRequiresLocalStorage(tmpSettings)) { + checkForIndexDataInDefaultPathData(tmpSettings, nodeEnvironment, logger); + } // this must be captured after the node name is possibly added to the settings final String nodeName = NODE_NAME_SETTING.get(tmpSettings); if (hadPredefinedNodeName == false) { @@ -500,6 +507,58 @@ protected Node(final Environment environment, Collection } } + /** + * Checks for path.data and default.path.data being configured, and there being index data in any of the paths in default.path.data. + * + * @param settings the settings to check for path.data and default.path.data + * @param nodeEnv the current node environment + * @param logger a logger where messages regarding the detection will be logged + * @throws IOException if an I/O exception occurs reading the directory structure + */ + static void checkForIndexDataInDefaultPathData( + final Settings settings, final NodeEnvironment nodeEnv, final Logger logger) throws IOException { + if (!Environment.PATH_DATA_SETTING.exists(settings) || !Environment.DEFAULT_PATH_DATA_SETTING.exists(settings)) { + return; + } + + boolean clean = true; + for (final String defaultPathData : Environment.DEFAULT_PATH_DATA_SETTING.get(settings)) { + final Path nodeDirectory = NodeEnvironment.resolveNodePath(getPath(defaultPathData), nodeEnv.getNodeLockId()); + if (Files.exists(nodeDirectory) == false) { + continue; + } + final NodeEnvironment.NodePath nodePath = new NodeEnvironment.NodePath(nodeDirectory); + final Set availableIndexFolders = nodeEnv.availableIndexFoldersForPath(nodePath); + if (availableIndexFolders.isEmpty()) { + continue; + } + clean = false; + logger.error("detected index data in default.path.data [{}] where there should not be any", nodePath.indicesPath); + for (final String availableIndexFolder : availableIndexFolders) { + logger.info( + "index folder [{}] in default.path.data [{}] must be moved to any of {}", + availableIndexFolder, + nodePath.indicesPath, + Arrays.stream(nodeEnv.nodePaths()).map(np -> np.indicesPath).collect(Collectors.toList())); + } + } + + if (clean) { + return; + } + + final String message = String.format( + Locale.ROOT, + "detected index data in default.path.data %s where there should not be any; check the logs for details", + Environment.DEFAULT_PATH_DATA_SETTING.get(settings)); + throw new IllegalStateException(message); + } + + @SuppressForbidden(reason = "read path that is not configured in environment") + private static Path getPath(final String path) { + return PathUtils.get(path); + } + // visible for testing static void warnIfPreRelease(final Version version, final boolean isSnapshot, final Logger logger) { if (!version.isRelease() || isSnapshot) { diff --git a/test/framework/src/main/java/org/elasticsearch/node/NodeTests.java b/test/framework/src/main/java/org/elasticsearch/node/NodeTests.java index ae4aff917a974..e99c7b9063161 100644 --- a/test/framework/src/main/java/org/elasticsearch/node/NodeTests.java +++ b/test/framework/src/main/java/org/elasticsearch/node/NodeTests.java @@ -19,31 +19,41 @@ package org.elasticsearch.node; import org.apache.logging.log4j.Logger; +import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.Version; import org.elasticsearch.bootstrap.BootstrapCheck; import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.common.UUIDs; import org.elasticsearch.common.network.NetworkModule; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.transport.BoundTransportAddress; import org.elasticsearch.env.Environment; +import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.InternalTestCluster; import org.elasticsearch.transport.MockTcpTransportPlugin; import java.io.IOException; +import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Locale; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasToString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; +@LuceneTestCase.SuppressFileSystems(value = "ExtrasFS") public class NodeTests extends ESTestCase { public void testNodeName() throws IOException { @@ -165,14 +175,81 @@ public void testNodeAttributes() throws IOException { } } + public void testDefaultPathDataSet() throws IOException { + final Path zero = createTempDir().toAbsolutePath(); + final Path one = createTempDir().toAbsolutePath(); + final Path defaultPathData = createTempDir().toAbsolutePath(); + final Settings settings = Settings.builder() + .put("path.home", "/home") + .put("path.data.0", zero) + .put("path.data.1", one) + .put("default.path.data", defaultPathData) + .build(); + try (NodeEnvironment nodeEnv = new NodeEnvironment(settings, new Environment(settings))) { + final Path defaultPathDataWithNodesAndId = defaultPathData.resolve("nodes/0"); + Files.createDirectories(defaultPathDataWithNodesAndId); + final NodeEnvironment.NodePath defaultNodePath = new NodeEnvironment.NodePath(defaultPathDataWithNodesAndId); + final boolean indexExists = randomBoolean(); + final List indices; + if (indexExists) { + indices = IntStream.range(0, randomIntBetween(1, 3)).mapToObj(i -> UUIDs.randomBase64UUID()).collect(Collectors.toList()); + for (final String index : indices) { + Files.createDirectories(defaultNodePath.indicesPath.resolve(index)); + } + } else { + indices = Collections.emptyList(); + } + final Logger mock = mock(Logger.class); + if (indexExists) { + final IllegalStateException e = expectThrows( + IllegalStateException.class, + () -> Node.checkForIndexDataInDefaultPathData(settings, nodeEnv, mock)); + final String message = String.format( + Locale.ROOT, + "detected index data in default.path.data [%s] where there should not be any; check the logs for details", + defaultPathData); + assertThat(e, hasToString(containsString(message))); + verify(mock) + .error("detected index data in default.path.data [{}] where there should not be any", defaultNodePath.indicesPath); + for (final String index : indices) { + verify(mock).info( + "index folder [{}] in default.path.data [{}] must be moved to any of {}", + index, + defaultNodePath.indicesPath, + Arrays.stream(nodeEnv.nodePaths()).map(np -> np.indicesPath).collect(Collectors.toList())); + } + verifyNoMoreInteractions(mock); + } else { + Node.checkForIndexDataInDefaultPathData(settings, nodeEnv, mock); + verifyNoMoreInteractions(mock); + } + } + } + + public void testDefaultPathDataNotSet() throws IOException { + final Path zero = createTempDir().toAbsolutePath(); + final Path one = createTempDir().toAbsolutePath(); + final Settings settings = Settings.builder() + .put("path.home", "/home") + .put("path.data.0", zero) + .put("path.data.1", one) + .build(); + try (NodeEnvironment nodeEnv = new NodeEnvironment(settings, new Environment(settings))) { + final Logger mock = mock(Logger.class); + Node.checkForIndexDataInDefaultPathData(settings, nodeEnv, mock); + verifyNoMoreInteractions(mock); + } + } + private static Settings.Builder baseSettings() { final Path tempDir = createTempDir(); return Settings.builder() - .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", randomLong())) - .put(Environment.PATH_HOME_SETTING.getKey(), tempDir) - .put(NetworkModule.HTTP_ENABLED.getKey(), false) - .put("transport.type", "mock-socket-network") - .put(Node.NODE_DATA_SETTING.getKey(), true); + .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", randomLong())) + .put(Environment.PATH_HOME_SETTING.getKey(), tempDir) + .put(NetworkModule.HTTP_ENABLED.getKey(), false) + .put("transport.type", "mock-socket-network") + .put(Node.NODE_DATA_SETTING.getKey(), true); } + }