Make docker tests more reliable (#68512)

Closes #66656.

Current, `Docker#verifyOssInstallation(...)` checks the permissions of
`elasticsearch.keystore`, however this often causes problems when a test
forgets to wait for ES and the keystore doesn't exist yet.

Instead, add a test specifically to check `elasticsearch.keystore` and
remove the check from `verifyOssInstallation()`.

buildSrc/src/main/java/org/elasticsearch/gradle/docker/DockerSupportService.java

@@ -311,7 +311,7 @@ public abstract class DockerSupportService implements BuildService<DockerSupport
          * <ul>
          *     <li>Installed</li>
          *     <li>Executable</li>
-         *     <li>Is at least version compatibile with minimum version</li>
+         *     <li>Is at least version compatible with minimum version</li>
          *     <li>Can execute a command that requires privileges</li>
          * </ul>
          */

qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java

@@ -34,6 +34,7 @@ import java.util.Set;
 import java.util.stream.Collectors;
 
 import static java.nio.file.attribute.PosixFilePermissions.fromString;
+import static org.elasticsearch.packaging.util.Docker.assertPermissionsAndOwnership;
 import static org.elasticsearch.packaging.util.Docker.chownWithPrivilegeEscalation;
 import static org.elasticsearch.packaging.util.Docker.copyFromContainer;
 import static org.elasticsearch.packaging.util.Docker.existsInContainer;
@@ -97,9 +98,7 @@ public class DockerTests extends PackagingTestCase {
     /**
      * Checks that the Docker image can be run, and that it passes various checks.
      */
-    public void test010Install() throws Exception {
-        // Wait for the container to come up, because we assert the state of some files that Elasticsearch creates on startup.
-        waitForElasticsearch(installation);
+    public void test010Install() {
         verifyContainerInstallation(installation, distribution());
     }
 
@@ -146,6 +145,15 @@ public class DockerTests extends PackagingTestCase {
         assertTrue("Expected Amazon trusted cert in cacerts", matches);
     }
 
+    /**
+     * Check that when the keystore is created on startup, it is created with the correct permissions.
+     */
+    public void test042KeystorePermissionsAreCorrect() throws Exception {
+        waitForElasticsearch(installation);
+
+        assertPermissionsAndOwnership(installation.config("elasticsearch.keystore"), p660);
+    }
+
     /**
      * Send some basic index, count and delete requests, in order to check that the installation
      * is minimally functional.
@@ -183,7 +191,7 @@ public class DockerTests extends PackagingTestCase {
 
         waitForElasticsearch(installation);
 
-        final JsonNode nodes = getJson("_nodes").get("nodes");
+        final JsonNode nodes = getJson("/_nodes").get("nodes");
         final String nodeId = nodes.fieldNames().next();
 
         final int heapSize = nodes.at("/" + nodeId + "/jvm/mem/heap_init_in_bytes").intValue();
@@ -211,7 +219,7 @@ public class DockerTests extends PackagingTestCase {
 
             waitForElasticsearch(installation);
 
-            final JsonNode nodes = getJson("_nodes");
+            final JsonNode nodes = getJson("/_nodes");
 
             assertThat(nodes.at("/_nodes/total").intValue(), equalTo(1));
             assertThat(nodes.at("/_nodes/successful").intValue(), equalTo(1));
@@ -718,7 +726,7 @@ public class DockerTests extends PackagingTestCase {
     public void test140CgroupOsStatsAreAvailable() throws Exception {
         waitForElasticsearch(installation);
 
-        final JsonNode nodes = getJson("_nodes/stats/os").get("nodes");
+        final JsonNode nodes = getJson("/_nodes/stats/os").get("nodes");
 
         final String nodeId = nodes.fieldNames().next();
 

qa/os/src/test/java/org/elasticsearch/packaging/test/PackagingTestCase.java

@@ -58,7 +58,6 @@ import java.util.Locale;
 import static org.elasticsearch.packaging.util.Cleanup.cleanEverything;
 import static org.elasticsearch.packaging.util.Docker.ensureImageIsLoaded;
 import static org.elasticsearch.packaging.util.Docker.removeContainer;
-import static org.elasticsearch.packaging.util.Docker.waitForElasticsearch;
 import static org.elasticsearch.packaging.util.FileExistenceMatchers.fileExists;
 import static org.elasticsearch.packaging.util.FileUtils.append;
 import static org.hamcrest.CoreMatchers.anyOf;
@@ -214,7 +213,6 @@ public abstract class PackagingTestCase extends Assert {
             case DOCKER:
             case DOCKER_UBI:
                 installation = Docker.runContainer(distribution);
-                waitForElasticsearch(installation);
                 Docker.verifyContainerInstallation(installation, distribution);
                 break;
             default:

qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java

@@ -23,12 +23,12 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Stream;
 
 import static java.nio.file.attribute.PosixFilePermissions.fromString;
 import static org.elasticsearch.packaging.util.FileMatcher.p644;
-import static org.elasticsearch.packaging.util.FileMatcher.p660;
 import static org.elasticsearch.packaging.util.FileMatcher.p664;
 import static org.elasticsearch.packaging.util.FileMatcher.p755;
 import static org.elasticsearch.packaging.util.FileMatcher.p770;
@@ -81,6 +81,7 @@ public class Docker {
      * successfully.
      *
      * @param distribution details about the docker image being tested
+     * @return an installation that models the running container
      */
     public static Installation runContainer(Distribution distribution) {
         return runContainer(distribution, DockerRun.builder());
@@ -92,6 +93,7 @@ public class Docker {
      *
      * @param distribution details about the docker image being tested
      * @param builder the command to run
+     * @return an installation that models the running container
      */
     public static Installation runContainer(Distribution distribution, DockerRun builder) {
         executeDockerRun(distribution, builder);
@@ -289,6 +291,8 @@ public class Docker {
 
     /**
      * Checks whether a path exists in the Docker container.
+     * @param path the path that ought to exist
+     * @return whether the path exists
      */
     public static boolean existsInContainer(Path path) {
         return existsInContainer(path.toString());
@@ -296,6 +300,8 @@ public class Docker {
 
     /**
      * Checks whether a path exists in the Docker container.
+     * @param path the path that ought to exist
+     * @return whether the path exists
      */
     public static boolean existsInContainer(String path) {
         logger.debug("Checking whether file " + path + " exists in container");
@@ -394,6 +400,8 @@ public class Docker {
 
     /**
      * Checks that the specified path's permissions and ownership match those specified.
+     * @param path the path to check
+     * @param expectedPermissions the unix permissions that the path ought to have
      */
     public static void assertPermissionsAndOwnership(Path path, Set<PosixFilePermission> expectedPermissions) {
         logger.debug("Checking permissions and ownership of [" + path + "]");
@@ -415,6 +423,7 @@ public class Docker {
 
     /**
      * Waits for up to 20 seconds for a path to exist in the container.
+     * @param path the path to await
      */
     public static void waitForPathToExist(Path path) throws InterruptedException {
         int attempt = 0;
@@ -432,6 +441,8 @@ public class Docker {
 
     /**
      * Perform a variety of checks on an installation. If the current distribution is not OSS, additional checks are carried out.
+     * @param installation the installation to verify
+     * @param distribution the distribution to verify
      */
     public static void verifyContainerInstallation(Installation installation, Distribution distribution) {
         verifyOssInstallation(installation);
@@ -452,8 +463,6 @@ public class Docker {
 
         Stream.of(es.modules).forEach(dir -> assertPermissionsAndOwnership(dir, p755));
 
-        assertPermissionsAndOwnership(es.config("elasticsearch.keystore"), p660);
-
         Stream.of("elasticsearch.yml", "jvm.options", "log4j2.properties")
             .forEach(configFile -> assertPermissionsAndOwnership(es.config(configFile), p664));
 
@@ -538,14 +547,31 @@ public class Docker {
         return containerId;
     }
 
+    /**
+     * Performs an HTTP GET to <code>http://localhost:9200/</code> with the supplied path.
+     * @param path the path to fetch, which must start with <code>/</code>
+     * @return the parsed response
+     */
     public static JsonNode getJson(String path) throws Exception {
-        final String pluginsResponse = makeRequest(Request.Get("http://localhost:9200/" + path));
+        path = Objects.requireNonNull(path).trim();
+        if (path.isEmpty()) {
+            throw new IllegalArgumentException("path must be supplied");
+        }
+        if (path.startsWith("/") == false) {
+            throw new IllegalArgumentException("path must start with /");
+        }
+        final String pluginsResponse = makeRequest(Request.Get("http://localhost:9200" + path));
 
         ObjectMapper mapper = new ObjectMapper();
 
         return mapper.readTree(pluginsResponse);
     }
 
+    /**
+     * Fetches all the labels for a Docker image
+     * @param distribution required to derive the image name
+     * @return a mapping from label name to value
+     */
     public static Map<String, String> getImageLabels(Distribution distribution) throws Exception {
         // The format below extracts the .Config.Labels value, and prints it as json. Without the json
         // modifier, a stringified Go map is printed instead, which isn't helpful.