From 3e06d4a38aa18416ca938b23b7675e7830faaf67 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Fri, 13 Dec 2019 16:55:10 -0500 Subject: [PATCH] Fix failing Keystore Passphrase test for feature branch (#50154) One problem with the passphrase-from-file tests, as written, is that they would leave a SystemD environment variable set when they failed, and this setting would cause elasticsearch startup to fail for other tests as well. By using a try-finally, I hope that these tests will fail more gracefully. It appears that our Fedora and Ubuntu environments may be configured to store journald information under /var rather than under /run, so that it will persist between boots. Our destructive tests that read from the journal need to account for this in order to avoid trying to limit the output we check in tests. --- .../test/KeystoreManagementTests.java | 138 +++++++----------- .../packaging/util/Distribution.java | 44 ------ .../packaging/util/Packages.java | 18 ++- 3 files changed, 70 insertions(+), 130 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/KeystoreManagementTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/KeystoreManagementTests.java index 9366985b88d8d..03bd3f5cd4a13 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/KeystoreManagementTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/KeystoreManagementTests.java @@ -58,11 +58,11 @@ public class KeystoreManagementTests extends PackagingTestCase { public void test10InstallArchiveDistribution() throws Exception { assumeTrue(distribution().isArchive()); - installation = installArchive(distribution); + installation = installArchive(sh, distribution); verifyArchiveInstallation(installation, distribution()); final Installation.Executables bin = installation.executables(); - Shell.Result r = sh.runIgnoreExitCode(bin.elasticsearchKeystore + " has-passwd"); + Shell.Result r = sh.runIgnoreExitCode(bin.keystoreTool.toString() + " has-passwd"); assertThat("has-passwd should fail", r.exitCode, not(is(0))); assertThat("has-passwd should fail", r.stderr, containsString("ERROR: Elasticsearch keystore not found")); } @@ -72,12 +72,12 @@ public void test11InstallPackageDistribution() throws Exception { assumeTrue(distribution().isPackage()); assertRemoved(distribution); - installation = installPackage(distribution); + installation = installPackage(sh, distribution); assertInstalled(distribution); verifyPackageInstallation(installation, distribution, sh); final Installation.Executables bin = installation.executables(); - Shell.Result r = sh.runIgnoreExitCode(bin.elasticsearchKeystore + " has-passwd"); + Shell.Result r = sh.runIgnoreExitCode(bin.keystoreTool.toString() + " has-passwd"); assertThat("has-passwd should fail", r.exitCode, not(is(0))); assertThat("has-passwd should fail", r.stderr, containsString("ERROR: Keystore is not password-protected")); } @@ -90,10 +90,7 @@ public void test20CreateKeystoreManually() throws Exception { final Installation.Executables bin = installation.executables(); verifyKeystorePermissions(); - String possibleSudo = distribution().isArchive() && Platforms.LINUX - ? "sudo -u " + ARCHIVE_OWNER + " " - : ""; - Shell.Result r = sh.run(possibleSudo + bin.elasticsearchKeystore + " list"); + Shell.Result r = bin.keystoreTool.run("list"); assertThat(r.stdout, containsString("keystore.seed")); } @@ -109,10 +106,7 @@ public void test30AutoCreateKeystore() throws Exception { verifyKeystorePermissions(); final Installation.Executables bin = installation.executables(); - String possibleSudo = distribution().isArchive() && Platforms.LINUX - ? "sudo -u " + ARCHIVE_OWNER + " " - : ""; - Shell.Result r = sh.run(possibleSudo + bin.elasticsearchKeystore + " list"); + Shell.Result r = bin.keystoreTool.run("list"); assertThat(r.stdout, containsString("keystore.seed")); } @@ -192,87 +186,57 @@ public void test50KeystorePasswordFromFile() throws Exception { assertPasswordProtectedKeystore(); - sh.getEnv().put("ES_KEYSTORE_PASSPHRASE_FILE", esKeystorePassphraseFile.toString()); - distribution().packagingConditional() - .forPackage( - () -> sh.run("sudo systemctl set-environment ES_KEYSTORE_PASSPHRASE_FILE=$ES_KEYSTORE_PASSPHRASE_FILE") - ) - .forArchive(Platforms.NO_ACTION) - .forDocker(/* TODO */ Platforms.NO_ACTION) - .run(); + try { + sh.run("sudo systemctl set-environment ES_KEYSTORE_PASSPHRASE_FILE=" + esKeystorePassphraseFile); - Files.createFile(esKeystorePassphraseFile); - Files.write(esKeystorePassphraseFile, - (password + System.lineSeparator()).getBytes(StandardCharsets.UTF_8), - StandardOpenOption.WRITE); + Files.createFile(esKeystorePassphraseFile); + Files.write(esKeystorePassphraseFile, + (password + System.lineSeparator()).getBytes(StandardCharsets.UTF_8), + StandardOpenOption.WRITE); - startElasticsearch(); - ServerUtils.runElasticsearchTests(); - stopElasticsearch(); - - distribution().packagingConditional() - .forPackage( - () -> sh.run("sudo systemctl unset-environment ES_KEYSTORE_PASSPHRASE_FILE") - ) - .forArchive(Platforms.NO_ACTION) - .forDocker(/* TODO */ Platforms.NO_ACTION) - .run(); + startElasticsearch(); + ServerUtils.runElasticsearchTests(); + stopElasticsearch(); + } finally { + sh.run("sudo systemctl unset-environment ES_KEYSTORE_PASSPHRASE_FILE"); + } } - @Ignore /* Ignored for feature branch, awaits fix: https://github.com/elastic/elasticsearch/issues/50079 */ public void test51WrongKeystorePasswordFromFile() throws Exception { assumeTrue("only for systemd", Platforms.isSystemd() && distribution().isPackage()); Path esKeystorePassphraseFile = installation.config.resolve("eks"); assertPasswordProtectedKeystore(); - sh.getEnv().put("ES_KEYSTORE_PASSPHRASE_FILE", esKeystorePassphraseFile.toString()); - distribution().packagingConditional() - .forPackage( - () -> sh.run("sudo systemctl set-environment ES_KEYSTORE_PASSPHRASE_FILE=$ES_KEYSTORE_PASSPHRASE_FILE") - ) - .forArchive(Platforms.NO_ACTION) - .forDocker(/* TODO */ Platforms.NO_ACTION) - .run(); - - if (Files.exists(esKeystorePassphraseFile)) { - rm(esKeystorePassphraseFile); - } + try { + sh.run("sudo systemctl set-environment ES_KEYSTORE_PASSPHRASE_FILE=" + esKeystorePassphraseFile); - Files.createFile(esKeystorePassphraseFile); - Files.write(esKeystorePassphraseFile, - ("wrongpassword" + System.lineSeparator()).getBytes(StandardCharsets.UTF_8), - StandardOpenOption.WRITE); + if (Files.exists(esKeystorePassphraseFile)) { + rm(esKeystorePassphraseFile); + } - Shell.Result result = runElasticsearchStartCommand(); - assertElasticsearchFailure(result, PASSWORD_ERROR_MESSAGE); + Files.createFile(esKeystorePassphraseFile); + Files.write(esKeystorePassphraseFile, + ("wrongpassword" + System.lineSeparator()).getBytes(StandardCharsets.UTF_8), + StandardOpenOption.WRITE); - distribution().packagingConditional() - .forPackage( - () -> sh.run("sudo systemctl unset-environment ES_KEYSTORE_PASSPHRASE_FILE") - ) - .forArchive(Platforms.NO_ACTION) - .forDocker(/* TODO */ Platforms.NO_ACTION) - .run(); + Shell.Result result = runElasticsearchStartCommand(); + assertElasticsearchFailure(result, PASSWORD_ERROR_MESSAGE); + } finally { + sh.run("sudo systemctl unset-environment ES_KEYSTORE_PASSPHRASE_FILE"); + } } private void createKeystore() throws Exception { Path keystore = installation.config("elasticsearch.keystore"); final Installation.Executables bin = installation.executables(); - Platforms.onLinux(() -> { - distribution().packagingConditional() - .forPackage(() -> sh.run(bin.elasticsearchKeystore + " create")) - .forArchive(() -> sh.run("sudo -u " + ARCHIVE_OWNER + " " + bin.elasticsearchKeystore + " create")) - .forDocker(/* TODO */ Platforms.NO_ACTION) - .run(); - }); + bin.keystoreTool.run("create"); // this is a hack around the fact that we can't run a command in the same session as the same user but not as administrator. // the keystore ends up being owned by the Administrators group, so we manually set it to be owned by the vagrant user here. // from the server's perspective the permissions aren't really different, this is just to reflect what we'd expect in the tests. // when we run these commands as a role user we won't have to do this Platforms.onWindows(() -> { - sh.run(bin.elasticsearchKeystore + " create"); sh.chown(keystore); }); } @@ -288,31 +252,37 @@ private void setKeystorePassword(String password) throws Exception { final Installation.Executables bin = installation.executables(); // set the password by passing it to stdin twice - Platforms.onLinux(() -> distribution().packagingConditional() - .forPackage(() -> sh.run("( echo \'" + password + "\' ; echo \'" + password + "\' ) | " + - bin.elasticsearchKeystore + " passwd")) - .forArchive(() -> sh.run("( echo \'" + password + "\' ; echo \'" + password + "\' ) | " + - "sudo -u " + ARCHIVE_OWNER + " " + bin.elasticsearchKeystore + " passwd")) - .forDocker(/* TODO */ Platforms.NO_ACTION) - .run() - ); + Platforms.onLinux(() -> { + bin.keystoreTool.run("passwd", password + "\n" + password + "\n"); + }); + Platforms.onWindows(() -> { sh.run("Invoke-Command -ScriptBlock {echo \'" + password + "\'; echo \'" + password + "\'} | " - + bin.elasticsearchKeystore + " passwd"); + + bin.keystoreTool + " passwd"); }); } private void assertPasswordProtectedKeystore() { - Shell.Result r = sh.runIgnoreExitCode(installation.executables().elasticsearchKeystore.toString() + " has-passwd"); + Shell.Result r = installation.executables().keystoreTool.run("has-passwd"); assertThat("keystore should be password protected", r.exitCode, is(0)); } private void verifyKeystorePermissions() throws Exception { Path keystore = installation.config("elasticsearch.keystore"); - distribution().packagingConditional() - .forPackage(() -> assertThat(keystore, file(File, "root", "elasticsearch", p660))) - .forArchive(() -> assertThat(keystore, file(File, ARCHIVE_OWNER, ARCHIVE_OWNER, p660))) - .forDocker(/* TODO */ Platforms.NO_ACTION) - .run(); + switch (distribution.packaging) { + case TAR: + case ZIP: + assertThat(keystore, file(File, ARCHIVE_OWNER, ARCHIVE_OWNER, p660)); + break; + case DEB: + case RPM: + assertThat(keystore, file(File, "root", "elasticsearch", p660)); + break; + case DOCKER: + // TODO #49469 + break; + default: + throw new IllegalStateException("Unknown Elasticsearch packaging type."); + } } } diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Distribution.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Distribution.java index e9d7052cd6755..13b2f31c7e4fd 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Distribution.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Distribution.java @@ -20,11 +20,7 @@ package org.elasticsearch.packaging.util; import java.nio.file.Path; -import java.util.Arrays; -import java.util.HashMap; -import java.util.HashSet; import java.util.Locale; -import java.util.Map; public class Distribution { @@ -88,46 +84,6 @@ public enum Packaging { } } - public static class PackagingConditional { - Distribution distribution; - - public PackagingConditional(Distribution distribution) { - this.distribution = distribution; - } - - private final Map conditions = new HashMap<>(); - - public PackagingConditional forArchive(Platforms.PlatformAction action) { - conditions.put(Packaging.TAR, action); - conditions.put(Packaging.ZIP, action); - return this; - } - - public PackagingConditional forPackage(Platforms.PlatformAction action) { - conditions.put(Packaging.RPM, action); - conditions.put(Packaging.DEB, action); - return this; - } - - public PackagingConditional forDocker(Platforms.PlatformAction action) { - conditions.put(Packaging.DOCKER, action); - return this; - } - - public void run() throws Exception { - HashSet missingPackaging = new HashSet<>(Arrays.asList(Packaging.values())); - missingPackaging.removeAll(conditions.keySet()); - if (missingPackaging.isEmpty() == false) { - throw new IllegalArgumentException("No condition specified for " + missingPackaging); - } - conditions.get(this.distribution.packaging).run(); - } - } - - public PackagingConditional packagingConditional() { - return new PackagingConditional(this); - } - public enum Platform { LINUX, WINDOWS, diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Packages.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Packages.java index e039d67ffcc6c..44b3edc305e5f 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Packages.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Packages.java @@ -286,8 +286,22 @@ public static Shell.Result runElasticsearchStartCommand(Shell sh) throws IOExcep */ public static void clearJournal(Shell sh) { if (isSystemd()) { - sh.run("rm -rf /run/log/journal/"); - sh.run("systemctl restart systemd-journald"); + sh.run("rm -rf /run/log/journal/ /var/log/journal/"); + final Result result = sh.runIgnoreExitCode("systemctl restart systemd-journald"); + + // Sometimes the restart fails on Debian 10 with: + // Job for systemd-journald.service failed because the control process exited with error code. + // See "systemctl status systemd-journald.service" and "journalctl -xe" for details.] + // + // ...so run these commands in an attempt to figure out what's going on. + if (result.isSuccess() == false) { + logger.error("Failed to restart systemd-journald: " + result); + + logger.error(sh.runIgnoreExitCode("systemctl status systemd-journald.service")); + logger.error(sh.runIgnoreExitCode("journalctl -xe")); + + fail("Couldn't clear the systemd journal as restarting systemd-journald failed"); + } } }