From 019390412fe81d378b7a8c92be5aadb3b4b344f6 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 5 Feb 2021 20:48:41 +0000 Subject: [PATCH] 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()`. --- .../gradle/docker/DockerSupportService.java | 2 +- .../packaging/test/DockerTests.java | 16 +++++++-- .../packaging/test/PackagingTestCase.java | 2 -- .../elasticsearch/packaging/util/Docker.java | 34 ++++++++++++++++--- 4 files changed, 44 insertions(+), 10 deletions(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/docker/DockerSupportService.java b/buildSrc/src/main/java/org/elasticsearch/gradle/docker/DockerSupportService.java index abc8219efd086..bc455a6e0608d 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/docker/DockerSupportService.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/docker/DockerSupportService.java @@ -311,7 +311,7 @@ public static class DockerAvailability { * */ diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index 2d935a20e4e5e..b04645bc228e3 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/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(); diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackagingTestCase.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackagingTestCase.java index f83b04889fe58..28d6e43f86e13 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackagingTestCase.java +++ b/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: diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index cf2177ad4c259..999d1d38b65f5 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/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,6 +263,8 @@ 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()); @@ -268,6 +272,8 @@ public static boolean existsInContainer(Path path) { /** * 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 expectedPermissions) { logger.debug("Checking permissions and ownership of [" + path + "]"); @@ -387,6 +395,7 @@ public static void assertPermissionsAndOwnership(Path path, Set 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 http://localhost:9200/ with the supplied path. + * @param path the path to fetch, which must start with / + * @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 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.