Revert "Convert path.data to String setting instead of List (elastic#…

…72282)"

This reverts commit d933ecd.

The revert had two conflicts. The first was very minor in JoinHelper.
The second was several tests in PersistedClusterStateServiceTests.

relates elastic#78525
relates elastic#71205

benchmarks/src/main/java/org/elasticsearch/benchmark/fs/AvailableIndexFoldersBenchmark.java

@@ -44,12 +44,13 @@ public class AvailableIndexFoldersBenchmark {
     @Setup
     public void setup() throws IOException {
         Path path = Files.createTempDirectory("test");
+        String[] paths = new String[] { path.toString() };
         nodePath = new NodeEnvironment.NodePath(path);
 
         LogConfigurator.setNodeName("test");
         Settings settings = Settings.builder()
             .put(Environment.PATH_HOME_SETTING.getKey(), path)
-            .put(Environment.PATH_DATA_SETTING.getKey(), path.resolve("data"))
+            .putList(Environment.PATH_DATA_SETTING.getKey(), paths)
             .build();
         nodeEnv = new NodeEnvironment(settings, new Environment(settings, null));
 

plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureRepositorySettingsTests.java

@@ -30,7 +30,7 @@ public class AzureRepositorySettingsTests extends ESTestCase {
     private AzureRepository azureRepository(Settings settings) {
         Settings internalSettings = Settings.builder()
             .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath())
-            .put(Environment.PATH_DATA_SETTING.getKey(), createTempDir().toAbsolutePath())
+            .putList(Environment.PATH_DATA_SETTING.getKey(), tmpPaths())
             .put(settings)
             .build();
         final AzureRepository azureRepository = new AzureRepository(new RepositoryMetadata("foo", "azure", internalSettings),

qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilSecurityTests.java

@@ -24,6 +24,9 @@
 import java.security.Permissions;
 import java.util.Set;
 
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.hasToString;
+
 @SuppressForbidden(reason = "modifies system properties and attempts to create symbolic links intentionally")
 public class EvilSecurityTests extends ESTestCase {
 
@@ -67,7 +70,8 @@ public void testEnvironmentPaths() throws Exception {
 
         Settings.Builder settingsBuilder = Settings.builder();
         settingsBuilder.put(Environment.PATH_HOME_SETTING.getKey(), esHome.resolve("home").toString());
-        settingsBuilder.put(Environment.PATH_DATA_SETTING.getKey(), esHome.resolve("data1").toString());
+        settingsBuilder.putList(Environment.PATH_DATA_SETTING.getKey(), esHome.resolve("data1").toString(),
+                esHome.resolve("data2").toString());
         settingsBuilder.put(Environment.PATH_SHARED_DATA_SETTING.getKey(), esHome.resolve("custom").toString());
         settingsBuilder.put(Environment.PATH_LOGS_SETTING.getKey(), esHome.resolve("logs").toString());
         settingsBuilder.put(Environment.NODE_PIDFILE_SETTING.getKey(), esHome.resolve("test.pid").toString());
@@ -120,6 +124,31 @@ public void testEnvironmentPaths() throws Exception {
         assertExactPermissions(new FilePermission(environment.pidFile().toString(), "delete"), permissions);
     }
 
+    public void testDuplicateDataPaths() throws IOException {
+        assumeFalse("https://github.com/elastic/elasticsearch/issues/44558", Constants.WINDOWS);
+        final Path path = createTempDir();
+        final Path home = path.resolve("home");
+        final Path data = path.resolve("data");
+        final Path duplicate;
+        if (randomBoolean()) {
+            duplicate = data;
+        } else {
+            duplicate = createTempDir().toAbsolutePath().resolve("link");
+            Files.createSymbolicLink(duplicate, data);
+        }
+
+        final Settings settings =
+                Settings
+                        .builder()
+                        .put(Environment.PATH_HOME_SETTING.getKey(), home.toString())
+                        .putList(Environment.PATH_DATA_SETTING.getKey(), data.toString(), duplicate.toString())
+                        .build();
+
+        final Environment environment = TestEnvironment.newEnvironment(settings);
+        final IllegalStateException e = expectThrows(IllegalStateException.class, () -> Security.createPermissions(environment));
+        assertThat(e, hasToString(containsString("path [" + duplicate.toRealPath() + "] is duplicated by [" + duplicate + "]")));
+    }
+
     public void testEnsureSymlink() throws IOException {
         Path p = createTempDir();
 

qa/evil-tests/src/test/java/org/elasticsearch/env/NodeEnvironmentEvilTests.java

@@ -7,6 +7,7 @@
  */
 package org.elasticsearch.env;
 
+import org.elasticsearch.common.io.PathUtils;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.PosixPermissionsResetter;
@@ -31,13 +32,14 @@ public static void checkPosix() throws IOException {
 
     public void testMissingWritePermission() throws IOException {
         assumeTrue("posix filesystem", isPosix);
-        Path path = createTempDir();
+        final String[] tempPaths = tmpPaths();
+        Path path = PathUtils.get(randomFrom(tempPaths));
         try (PosixPermissionsResetter attr = new PosixPermissionsResetter(path)) {
             attr.setPermissions(new HashSet<>(Arrays.asList(PosixFilePermission.OTHERS_READ, PosixFilePermission.GROUP_READ,
                 PosixFilePermission.OWNER_READ)));
             Settings build = Settings.builder()
                     .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath().toString())
-                    .put(Environment.PATH_DATA_SETTING.getKey(), path).build();
+                    .putList(Environment.PATH_DATA_SETTING.getKey(), tempPaths).build();
             IllegalStateException exception = expectThrows(IllegalStateException.class, () -> {
                 new NodeEnvironment(build, TestEnvironment.newEnvironment(build));
             });
@@ -48,7 +50,8 @@ public void testMissingWritePermission() throws IOException {
 
     public void testMissingWritePermissionOnIndex() throws IOException {
         assumeTrue("posix filesystem", isPosix);
-        Path path = createTempDir();
+        final String[] tempPaths = tmpPaths();
+        Path path = PathUtils.get(randomFrom(tempPaths));
         Path fooIndex = path.resolve(NodeEnvironment.INDICES_FOLDER)
             .resolve("foo");
         Files.createDirectories(fooIndex);
@@ -57,7 +60,7 @@ public void testMissingWritePermissionOnIndex() throws IOException {
                 PosixFilePermission.OWNER_READ)));
             Settings build = Settings.builder()
                 .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath().toString())
-                .put(Environment.PATH_DATA_SETTING.getKey(), path).build();
+                .putList(Environment.PATH_DATA_SETTING.getKey(), tempPaths).build();
             IOException ioException = expectThrows(IOException.class, () -> {
                 new NodeEnvironment(build, TestEnvironment.newEnvironment(build));
             });
@@ -67,7 +70,8 @@ public void testMissingWritePermissionOnIndex() throws IOException {
 
     public void testMissingWritePermissionOnShard() throws IOException {
         assumeTrue("posix filesystem", isPosix);
-        Path path = createTempDir();
+        final String[] tempPaths = tmpPaths();
+        Path path = PathUtils.get(randomFrom(tempPaths));
         Path fooIndex = path.resolve(NodeEnvironment.INDICES_FOLDER)
             .resolve("foo");
         Path fooShard = fooIndex.resolve("0");
@@ -81,7 +85,7 @@ public void testMissingWritePermissionOnShard() throws IOException {
                 PosixFilePermission.OWNER_READ)));
             Settings build = Settings.builder()
                 .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath().toString())
-                .put(Environment.PATH_DATA_SETTING.getKey(), path).build();
+                .putList(Environment.PATH_DATA_SETTING.getKey(), tempPaths).build();
             IOException ioException = expectThrows(IOException.class, () -> {
                 new NodeEnvironment(build, TestEnvironment.newEnvironment(build));
             });

server/src/internalClusterTest/java/org/elasticsearch/env/NodeEnvironmentIT.java

@@ -28,6 +28,7 @@
 import java.nio.file.StandardCopyOption;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
 import static org.elasticsearch.test.NodeRoles.nonDataNode;
@@ -136,7 +137,8 @@ public void testUpgradeDataFolder() throws IOException, InterruptedException {
         internalCluster().stopRandomDataNode();
 
         // simulate older data path layout by moving data under "nodes/0" folder
-        final List<Path> dataPaths = List.of(PathUtils.get(Environment.PATH_DATA_SETTING.get(dataPathSettings)));
+        final List<Path> dataPaths = Environment.PATH_DATA_SETTING.get(dataPathSettings)
+            .stream().map(PathUtils::get).collect(Collectors.toList());
         dataPaths.forEach(path -> {
                 final Path targetPath = path.resolve("nodes").resolve("0");
                 try {

server/src/internalClusterTest/java/org/elasticsearch/plugins/IndexFoldersDeletionListenerIT.java

@@ -235,7 +235,7 @@ public void testListenersInvokedWhenIndexHasLeftOverShard() throws Exception {
         final Path dataDirWithLeftOverShards = createTempDir();
         String dataNode = internalCluster().startDataOnlyNode(
             Settings.builder()
-                .put(Environment.PATH_DATA_SETTING.getKey(), dataDirWithLeftOverShards.toAbsolutePath().toString())
+                .putList(Environment.PATH_DATA_SETTING.getKey(), List.of(dataDirWithLeftOverShards.toAbsolutePath().toString()))
                 .putNull(Environment.PATH_SHARED_DATA_SETTING.getKey())
                 .build()
         );

server/src/main/java/org/elasticsearch/cluster/coordination/JoinHelper.java

@@ -150,7 +150,7 @@ public ClusterTasksResult<JoinTaskExecutor.Task> execute(ClusterState currentSta
             TransportRequest.Empty::new,
             (request, channel, task) -> channel.sendResponse(Empty.INSTANCE));
 
-        final String dataPath = Environment.PATH_DATA_SETTING.get(settings);
+        final List<String> dataPaths = Environment.PATH_DATA_SETTING.get(settings);
         transportService.registerRequestHandler(JOIN_VALIDATE_ACTION_NAME,
             ThreadPool.Names.GENERIC, ValidateJoinRequest::new,
             (request, channel, task) -> {
@@ -161,8 +161,9 @@ public ClusterTasksResult<JoinTaskExecutor.Task> execute(ClusterState currentSta
                             localState.metadata().clusterUUID() + "] and is now trying to join a different cluster with UUID [" +
                             request.getState().metadata().clusterUUID() + "]. This is forbidden and usually indicates an incorrect " +
                             "discovery or cluster bootstrapping configuration. Note that the cluster UUID persists across restarts and " +
-                            "can only be changed by deleting the contents of the node's data path [" + dataPath +
-                            "] which will also remove any data held by this node.");
+                            "can only be changed by deleting the contents of the node's data " +
+                            (dataPaths.size() == 1 ? "path " : "paths ") + dataPaths +
+                            " which will also remove any data held by this node.");
                 }
                 joinValidators.forEach(action -> action.accept(transportService.getLocalNode(), request.getState()));
                 channel.sendResponse(Empty.INSTANCE);

server/src/main/java/org/elasticsearch/env/Environment.java

@@ -40,8 +40,8 @@ public class Environment {
     private static final Path[] EMPTY_PATH_ARRAY = new Path[0];
 
     public static final Setting<String> PATH_HOME_SETTING = Setting.simpleString("path.home", Property.NodeScope);
-    public static final Setting<String> PATH_DATA_SETTING =
-            Setting.simpleString("path.data", Property.NodeScope);
+    public static final Setting<List<String>> PATH_DATA_SETTING =
+            Setting.listSetting("path.data", Collections.emptyList(), Function.identity(), Property.NodeScope);
     public static final Setting<String> PATH_LOGS_SETTING =
             new Setting<>("path.logs", "", Function.identity(), Property.NodeScope);
     public static final Setting<List<String>> PATH_REPO_SETTING =
@@ -52,7 +52,7 @@ public class Environment {
 
     private final Settings settings;
 
-    private final Path dataFile;
+    private final Path[] dataFiles;
 
     private final Path[] repoFiles;
 
@@ -101,10 +101,14 @@ public Environment(final Settings settings, final Path configPath) {
 
         pluginsFile = homeFile.resolve("plugins");
 
-        if (PATH_DATA_SETTING.exists(settings)) {
-            dataFile = PathUtils.get(PATH_DATA_SETTING.get(settings)).toAbsolutePath().normalize();
+        List<String> dataPaths = PATH_DATA_SETTING.get(settings);
+        if (dataPaths.isEmpty() == false) {
+            dataFiles = new Path[dataPaths.size()];
+            for (int i = 0; i < dataPaths.size(); i++) {
+                dataFiles[i] = PathUtils.get(dataPaths.get(i)).toAbsolutePath().normalize();
+            }
         } else {
-            dataFile = homeFile.resolve("data");
+            dataFiles = new Path[]{homeFile.resolve("data")};
         }
         if (PATH_SHARED_DATA_SETTING.exists(settings)) {
             sharedDataFile = PathUtils.get(PATH_SHARED_DATA_SETTING.get(settings)).toAbsolutePath().normalize();
@@ -140,7 +144,12 @@ public Environment(final Settings settings, final Path configPath) {
 
         final Settings.Builder finalSettings = Settings.builder().put(settings);
         if (PATH_DATA_SETTING.exists(settings)) {
-            finalSettings.put(PATH_DATA_SETTING.getKey(), dataFile.toString());
+            if (dataFiles.length == 1) {
+                finalSettings.put(PATH_DATA_SETTING.getKey(), dataFiles[0].toString());
+            } else {
+                finalSettings.putList(PATH_DATA_SETTING.getKey(),
+                    Arrays.stream(dataFiles).map(Path::toString).collect(Collectors.toList()));
+            }
         }
         finalSettings.put(PATH_HOME_SETTING.getKey(), homeFile);
         finalSettings.put(PATH_LOGS_SETTING.getKey(), logsFile.toString());
@@ -171,7 +180,7 @@ public Settings settings() {
      * The data location.
      */
     public Path[] dataFiles() {
-        return new Path[] { dataFile };
+        return dataFiles;
     }
 
     /**

server/src/main/java/org/elasticsearch/node/Node.java

@@ -327,7 +327,9 @@ protected Node(final Environment initialEnvironment,
                 );
             }
 
-            if (Environment.dataPathUsesList(tmpSettings)) {
+            if (initialEnvironment.dataFiles().length > 1) {
+                throw new IllegalArgumentException("Multiple [path.data] values found. Specify a single data path.");
+            } else if (Environment.dataPathUsesList(tmpSettings)) {
                 throw new IllegalArgumentException("[path.data] is a list. Specify as a string value.");
             }
 

server/src/test/java/org/elasticsearch/env/EnvironmentTests.java

@@ -140,8 +140,9 @@ public void testPathNormalization() throws IOException {
 
         final Path home = PathUtils.get(homePath);
 
-        final String dataPath = Environment.PATH_DATA_SETTING.get(environment.settings());
-        assertPath(dataPath, home.resolve("data"));
+        final List<String> dataPaths = Environment.PATH_DATA_SETTING.get(environment.settings());
+        assertThat(dataPaths, hasSize(1));
+        assertPath(dataPaths.get(0), home.resolve("data"));
 
         final String logPath = Environment.PATH_LOGS_SETTING.get(environment.settings());
         assertPath(logPath, home.resolve("logs"));