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()`.

Author: pugnascotia

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

@@ -311,7 +311,7 @@ public static class DockerAvailability {
          * <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

@@ -38,6 +38,7 @@
 
 import static java.nio.file.attribute.PosixFilePermissions.fromString;
 import static java.util.Collections.singletonMap;
+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;
@@ -151,6 +152,15 @@ public void test041AmazonCaCertsAreInTheKeystore() {
         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.
@@ -189,7 +199,7 @@ public void test070BindMountCustomPathConfAndJvmOptions() throws Exception {
 
         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();
@@ -217,7 +227,7 @@ public void test071BindMountCustomPathWithDifferentUID() throws Exception {
 
             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));
@@ -729,7 +739,7 @@ public void test131InitProcessHasCorrectPID() {
     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 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;
@@ -218,7 +217,6 @@ protected static void install() throws Exception {
             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

@@ -24,12 +24,12 @@
 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;
@@ -84,6 +84,7 @@ public static void ensureImageIsLoaded(Distribution distribution) {
      * 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());
@@ -95,6 +96,7 @@ public static Installation runContainer(Distribution distribution) {
      *
      * @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);
@@ -261,13 +263,17 @@ protected String[] getScriptCommand(String script) {
 
     /**
      * 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());
     }
 
     /**
      * 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");
@@ -366,6 +372,8 @@ public static void chownWithPrivilegeEscalation(Path localPath, String ownership
 
     /**
      * 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 + "]");
@@ -387,6 +395,7 @@ public static void assertPermissionsAndOwnership(Path path, Set<PosixFilePermiss
 
     /**
      * 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;
@@ -404,6 +413,8 @@ public static void waitForPathToExist(Path path) throws InterruptedException {
 
     /**
      * 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);
@@ -424,8 +435,6 @@ private static void verifyOssInstallation(Installation es) {
 
         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));
 
@@ -508,14 +517,31 @@ public static String getContainerId() {
         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.