Merge pull request #11106 from rmuir/symlink

Improve path mgmt on init, better error messages, symlink support

pom.xml

@@ -774,7 +774,9 @@
                         <configuration>
                             <includes>
                                 <include>org/elasticsearch/test/**/*</include>
-                                <include>org/apache/lucene/util/AbstractRandomizedTest.java</include>
+                                <include>org/elasticsearch/bootstrap/BootstrapForTesting.class</include>
+                                <include>org/elasticsearch/common/cli/CliToolTestCase.class</include>
+                                <include>org/elasticsearch/common/cli/CliToolTestCase$*.class</include>
                             </includes>
                             <excludes>
                                 <!-- unit tests for yaml suite parser & rest spec parser need to be excluded -->
@@ -1535,9 +1537,7 @@
                         <configuration>
                             <includes>
                                 <include>org/elasticsearch/test/**/*</include>
-                                <include>org/elasticsearch/cache/recycler/MockPageCacheRecycler.class</include>
+                                <include>org/elasticsearch/bootstrap/BootstrapForTesting.class</include>
-                                <include>org/apache/lucene/util/AbstractRandomizedTest.class</include>
-                                <include>org/apache/lucene/util/AbstractRandomizedTest$*.class</include>
                                 <include>org/elasticsearch/common/cli/CliToolTestCase.class</include>
                                 <include>org/elasticsearch/common/cli/CliToolTestCase$*.class</include>
                             </includes>

src/main/java/org/elasticsearch/bootstrap/Bootstrap.java

@@ -58,7 +58,7 @@ public class Bootstrap {
 
     private static volatile Bootstrap INSTANCE;
 
-    private Node node;
+    private volatile Node node;
     private final CountDownLatch keepAliveLatch = new CountDownLatch(1);
     private final Thread keepAliveThread;
 
@@ -144,19 +144,22 @@ public class Bootstrap {
         initializeNatives(settings.getAsBoolean("bootstrap.mlockall", false), 
                           settings.getAsBoolean("bootstrap.ctrlhandler", true));
 
-        NodeBuilder nodeBuilder = NodeBuilder.nodeBuilder().settings(settings).loadConfigSettings(false);
-        node = nodeBuilder.build();
         if (addShutdownHook) {
             Runtime.getRuntime().addShutdownHook(new Thread() {
                 @Override
                 public void run() {
+                    if (node != null) {
                         node.close();
                     }
+                }
             });
         }
         
         // install SM after natives, shutdown hooks, etc.
         setupSecurity(settings, environment);
+        
+        NodeBuilder nodeBuilder = NodeBuilder.nodeBuilder().settings(settings).loadConfigSettings(false);
+        node = nodeBuilder.build();
     }
     
     /** 

src/main/java/org/elasticsearch/bootstrap/Security.java

@@ -23,7 +23,10 @@ import org.elasticsearch.common.SuppressForbidden;
 import org.elasticsearch.env.Environment;
 
 import java.io.*;
+import java.nio.file.AccessMode;
+import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.Files;
+import java.nio.file.NotDirectoryException;
 import java.nio.file.Path;
 import java.security.Permissions;
 import java.security.Policy;
@@ -34,7 +37,7 @@ import java.security.Policy;
  * We use a template file (the one we test with), and add additional 
  * permissions based on the environment (data paths, etc)
  */
-public class Security {
+class Security {
        
     /** 
      * Initializes securitymanager for the environment
@@ -74,22 +77,49 @@ public class Security {
     }
     
     /** Add access to path (and all files underneath it */
-    public static void addPath(Permissions policy, Path path, String permissions) throws IOException {
+    static void addPath(Permissions policy, Path path, String permissions) throws IOException {
         // paths may not exist yet
-        Files.createDirectories(path);
+        ensureDirectoryExists(path);
+
         // add each path twice: once for itself, again for files underneath it
         policy.add(new FilePermission(path.toString(), permissions));
         policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", permissions));
     }
     
+    /**
+     * Ensures configured directory {@code path} exists.
+     * @throws IOException if {@code path} exists, but is not a directory, not accessible, or broken symbolic link.
+     */
+    static void ensureDirectoryExists(Path path) throws IOException {
+        // this isn't atomic, but neither is createDirectories.
+        if (Files.isDirectory(path)) {
+            // verify access, following links (throws exception if something is wrong)
+            // we only check READ as a sanity test
+            path.getFileSystem().provider().checkAccess(path.toRealPath(), AccessMode.READ);
+        } else {
+            // doesn't exist, or not a directory
+            try {
+                Files.createDirectories(path);
+            } catch (FileAlreadyExistsException e) {
+                // convert optional specific exception so the context is clear
+                IOException e2 = new NotDirectoryException(path.toString());
+                e2.addSuppressed(e);
+                throw e2;
+            }
+        }
+    }
+
     /** Simple checks that everything is ok */
     @SuppressForbidden(reason = "accesses jvm default tempdir as a self-test")
-    public static void selfTest() {
+    static void selfTest() throws IOException {
         // check we can manipulate temporary files
         try {
-            Files.delete(Files.createTempFile(null, null));
+            Path p = Files.createTempFile(null, null);
+            try {
+                Files.delete(p);
             } catch (IOException ignored) {
                 // potentially virus scanner
+            }
         } catch (SecurityException problem) {
             throw new SecurityException("Security misconfiguration: cannot access java.io.tmpdir", problem);
         }

src/main/java/org/elasticsearch/common/jna/Natives.java

@@ -72,9 +72,9 @@ public class Natives {
         }
         try {
             return CLibrary.geteuid() == 0;
-        } catch (Throwable error) {
+        } catch (UnsatisfiedLinkError e) {
-            logger.warn("unable to determine euid", error);
+            // this will have already been logged by Kernel32Library, no need to repeat it
-            return false; // don't know
+            return false;
         }
     }
 

src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java

@@ -17,7 +17,7 @@
  * under the License.
  */
 
-package org.elasticsearch.test;
+package org.elasticsearch.bootstrap;
 
 import org.apache.lucene.util.TestSecurityManager;
 import org.elasticsearch.bootstrap.Bootstrap;
@@ -33,13 +33,14 @@ import java.util.Objects;
 import static com.carrotsearch.randomizedtesting.RandomizedTest.systemPropertyAsBoolean;
 
 /** 
- * Installs test security manager (ensures it happens regardless of which
+ * Initializes natives and installs test security manager
+ * (init'd early by base classes to ensure it happens regardless of which
  * test case happens to be first, test ordering, etc). 
  * <p>
  * The idea is to mimic as much as possible what happens with ES in production
  * mode (e.g. assign permissions and install security manager the same way)
  */
-class SecurityBootstrap {
+public class BootstrapForTesting {
     
     // TODO: can we share more code with the non-test side here
     // without making things complex???
@@ -77,5 +78,5 @@ class SecurityBootstrap {
     }
 
     // does nothing, just easy way to make sure the class is loaded.
-    static void ensureInitialized() {}
+    public static void ensureInitialized() {}
 }

src/test/java/org/elasticsearch/bootstrap/SecurityTests.java

@@ -25,6 +25,8 @@ import org.elasticsearch.env.Environment;
 import org.elasticsearch.test.ElasticsearchTestCase;
 
 import java.io.FilePermission;
+import java.io.IOException;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.security.Permissions;
 
@@ -110,4 +112,68 @@ public class SecurityTests extends ElasticsearchTestCase {
         // PID file: r/w
         assertTrue(permissions.implies(new FilePermission(environment.pidFile().toString(), "read,readlink,write,delete")));
     }
+    
+    public void testEnsureExists() throws IOException {
+        Path p = createTempDir();
+
+        // directory exists
+        Path exists = p.resolve("exists");
+        Files.createDirectory(exists);
+        Security.ensureDirectoryExists(exists);
+        Files.createTempFile(exists, null, null);
+    }
+    
+    public void testEnsureNotExists() throws IOException { 
+        Path p = createTempDir();
+
+        // directory does not exist: create it
+        Path notExists = p.resolve("notexists");
+        Security.ensureDirectoryExists(notExists);
+        Files.createTempFile(notExists, null, null);
+    }
+    
+    public void testEnsureRegularFile() throws IOException {
+        Path p = createTempDir();
+
+        // regular file
+        Path regularFile = p.resolve("regular");
+        Files.createFile(regularFile);
+        try {
+            Security.ensureDirectoryExists(regularFile);
+            fail("didn't get expected exception");
+        } catch (IOException expected) {}
+    }
+    
+    public void testEnsureSymlink() throws IOException {
+        Path p = createTempDir();
+        
+        Path exists = p.resolve("exists");
+        Files.createDirectory(exists);
+
+        // symlink
+        Path linkExists = p.resolve("linkExists");
+        try {
+            Files.createSymbolicLink(linkExists, exists);
+        } catch (UnsupportedOperationException | IOException e) {
+            assumeNoException("test requires filesystem that supports symbolic links", e);
+        }
+        Security.ensureDirectoryExists(linkExists);
+        Files.createTempFile(linkExists, null, null);
+    }
+    
+    public void testEnsureBrokenSymlink() throws IOException {
+        Path p = createTempDir();
+
+        // broken symlink
+        Path brokenLink = p.resolve("brokenLink");
+        try {
+            Files.createSymbolicLink(brokenLink, p.resolve("nonexistent"));
+        } catch (UnsupportedOperationException | IOException e) {
+            assumeNoException("test requires filesystem that supports symbolic links", e);
+        }
+        try {
+            Security.ensureDirectoryExists(brokenLink);
+            fail("didn't get expected exception");
+        } catch (IOException expected) {}
+    }
 }

src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java

@@ -30,12 +30,14 @@ import com.carrotsearch.randomizedtesting.generators.RandomPicks;
 import com.carrotsearch.randomizedtesting.generators.RandomStrings;
 import com.carrotsearch.randomizedtesting.rules.TestRuleAdapter;
 import com.google.common.base.Predicate;
+
 import org.apache.lucene.uninverting.UninvertingReader;
 import org.apache.lucene.util.LuceneTestCase;
 import org.apache.lucene.util.LuceneTestCase.SuppressCodecs;
 import org.apache.lucene.util.TestUtil;
 import org.apache.lucene.util.TimeUnits;
 import org.elasticsearch.Version;
+import org.elasticsearch.bootstrap.BootstrapForTesting;
 import org.elasticsearch.client.Requests;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.cluster.routing.DjbHashFunction;
@@ -93,7 +95,7 @@ import static com.google.common.collect.Lists.newArrayList;
 public abstract class ElasticsearchTestCase extends LuceneTestCase {
 
     static {
-        SecurityBootstrap.ensureInitialized();
+        BootstrapForTesting.ensureInitialized();
     }
 
     protected final ESLogger logger = Loggers.getLogger(getClass());

src/test/java/org/elasticsearch/test/ElasticsearchTokenStreamTestCase.java

@@ -26,6 +26,7 @@ import org.apache.lucene.analysis.BaseTokenStreamTestCase;
 import org.apache.lucene.util.LuceneTestCase;
 import org.apache.lucene.util.TimeUnits;
 import org.elasticsearch.Version;
+import org.elasticsearch.bootstrap.BootstrapForTesting;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.test.junit.listeners.ReproduceInfoPrinter;
@@ -43,7 +44,7 @@ import org.elasticsearch.test.junit.listeners.ReproduceInfoPrinter;
 public abstract class ElasticsearchTokenStreamTestCase extends BaseTokenStreamTestCase {
 
     static {
-        SecurityBootstrap.ensureInitialized();
+        BootstrapForTesting.ensureInitialized();
     }
     
     public static Version randomVersion() {