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

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.
      *

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<Path> 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<String> availableIndexFolders() throws IOException {
         assertEnvIsLocked();
         Set<String> indexFolders = new HashSet<>();
         for (NodePath nodePath : nodePaths) {
-            Path indicesLocation = nodePath.indicesPath;
-            if (Files.isDirectory(indicesLocation)) {
-                try (DirectoryStream<Path> 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<String> 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<String> indexFolders = new HashSet<>();
+        Path indicesLocation = nodePath.indicesPath;
+        if (Files.isDirectory(indicesLocation)) {
+            try (DirectoryStream<Path> stream = Files.newDirectoryStream(indicesLocation)) {
+                for (Path index : stream) {
+                    if (Files.isDirectory(index)) {
+                        indexFolders.add(index.getFileName().toString());
                     }
                 }
             }
         }
         return indexFolders;
-
     }
 
     /**

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<Class<? extends Plugin>
             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<Class<? extends Plugin>
         }
     }
 
+    /**
+     * 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<String> 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) {

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<String> 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);
     }
 
+
 }