Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable TTY password OS tests, plus refactoring (#57759) #58200

Merged
merged 5 commits into from
Jun 25, 2020
Merged

Enable TTY password OS tests, plus refactoring (#57759) #58200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fcd797f
Enable TTY password OS tests, plus refactoring (#57759)
williamrandolph Jun 16, 2020
5faa81d
Merge branch '7.x' into backport/7.x/enable-tty-tests-and-refactor
williamrandolph Jun 17, 2020
6f30abf
Remove -d option for tty startup
williamrandolph Jun 18, 2020
bb85667
Test daemonization and non-daemonization case for tty
williamrandolph Jun 24, 2020
700efba
Merge branch '7.x' into backport/7.x/enable-tty-tests-and-refactor
williamrandolph Jun 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 47 additions & 21 deletions qa/os/src/test/java/org/elasticsearch/packaging/test/KeystoreManagementTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import java.util.List;
import java.util.Map;

import static com.carrotsearch.randomizedtesting.RandomizedTest.randomBoolean;
import static java.util.Collections.singletonList;
import static org.elasticsearch.packaging.util.Archives.ARCHIVE_OWNER;
import static org.elasticsearch.packaging.util.Archives.installArchive;
Expand Down Expand Up @@ -124,7 +125,7 @@ public void test12InstallDockerDistribution() throws Exception {

public void test20CreateKeystoreManually() throws Exception {
rmKeystoreIfExists();
createKeystore();
createKeystore(null);

final Installation.Executables bin = installation.executables();
verifyKeystorePermissions();
Expand Down Expand Up @@ -156,54 +157,76 @@ public void test40KeystorePasswordOnStandardInput() throws Exception {
String password = "^|<>\\&exit"; // code insertion on Windows if special characters are not escaped

rmKeystoreIfExists();
createKeystore();
setKeystorePassword(password);
createKeystore(password);

assertPasswordProtectedKeystore();

awaitElasticsearchStartup(startElasticsearchStandardInputPassword(password, true));
awaitElasticsearchStartup(runElasticsearchStartCommand(password, true, false));
ServerUtils.runElasticsearchTests();
stopElasticsearch();
}

public void test41WrongKeystorePasswordOnStandardInput() {
public void test41WrongKeystorePasswordOnStandardInput() throws Exception {
assumeTrue("packages will use systemd, which doesn't handle stdin", distribution.isArchive());
assumeThat(installation, is(notNullValue()));

assertPasswordProtectedKeystore();

Shell.Result result = startElasticsearchStandardInputPassword("wrong", false);
Shell.Result result = runElasticsearchStartCommand("wrong", false, false);
assertElasticsearchFailure(result, Arrays.asList(ERROR_INCORRECT_PASSWORD, ERROR_CORRUPTED_KEYSTORE), null);
}

@Ignore /* Ignored for feature branch, awaits fix: https://github.com/elastic/elasticsearch/issues/49340 */
public void test42KeystorePasswordOnTty() throws Exception {
/**
* This test simulates a user starting Elasticsearch on the command line without daemonizing
*/
public void test42KeystorePasswordOnTtyRunningInForeground() throws Exception {
/* Windows issue awaits fix: https://github.com/elastic/elasticsearch/issues/49340 */
assumeTrue("expect command isn't on Windows", distribution.platform != Distribution.Platform.WINDOWS);
assumeTrue("packages will use systemd, which doesn't handle stdin", distribution.isArchive());
assumeThat(installation, is(notNullValue()));

String password = "keystorepass";

rmKeystoreIfExists();
createKeystore(password);

assertPasswordProtectedKeystore();

awaitElasticsearchStartup(runElasticsearchStartCommand(password, false, true));
ServerUtils.runElasticsearchTests();
stopElasticsearch();
}

@Ignore // awaits fix: https://github.com/elastic/elasticsearch/issues/49340
public void test43KeystorePasswordOnTtyDaemonized() throws Exception {
/* Windows issue awaits fix: https://github.com/elastic/elasticsearch/issues/49340 */
assumeTrue("expect command isn't on Windows", distribution.platform != Distribution.Platform.WINDOWS);
assumeTrue("packages will use systemd, which doesn't handle stdin", distribution.isArchive());
assumeThat(installation, is(notNullValue()));

String password = "keystorepass";

rmKeystoreIfExists();
createKeystore();
setKeystorePassword(password);
createKeystore(password);

assertPasswordProtectedKeystore();

awaitElasticsearchStartup(startElasticsearchTtyPassword(password, true));
awaitElasticsearchStartup(runElasticsearchStartCommand(password, true, true));
ServerUtils.runElasticsearchTests();
stopElasticsearch();
}

@Ignore /* Ignored for feature branch, awaits fix: https://github.com/elastic/elasticsearch/issues/49340 */
public void test43WrongKeystorePasswordOnTty() throws Exception {
public void test44WrongKeystorePasswordOnTty() throws Exception {
/* Windows issue awaits fix: https://github.com/elastic/elasticsearch/issues/49340 */
assumeTrue("expect command isn't on Windows", distribution.platform != Distribution.Platform.WINDOWS);
assumeTrue("packages will use systemd, which doesn't handle stdin", distribution.isArchive());
assumeThat(installation, is(notNullValue()));

assertPasswordProtectedKeystore();

Shell.Result result = startElasticsearchTtyPassword("wrong", false);
// daemonization shouldn't matter for this test
boolean daemonize = randomBoolean();
Shell.Result result = runElasticsearchStartCommand("wrong", daemonize, true);
// error will be on stdout for "expect"
assertThat(result.stdout, anyOf(containsString(ERROR_INCORRECT_PASSWORD), containsString(ERROR_CORRUPTED_KEYSTORE)));
}
Expand All @@ -212,14 +235,13 @@ public void test43WrongKeystorePasswordOnTty() throws Exception {
* If we have an encrypted keystore, we shouldn't require a password to
* view help information.
*/
public void test44EncryptedKeystoreAllowsHelpMessage() throws Exception {
public void test45EncryptedKeystoreAllowsHelpMessage() throws Exception {
assumeTrue("users call elasticsearch directly in archive case", distribution.isArchive());

String password = "keystorepass";

rmKeystoreIfExists();
createKeystore();
setKeystorePassword(password);
createKeystore(password);

assertPasswordProtectedKeystore();
Shell.Result r = installation.executables().elasticsearch.run("--help");
Expand All @@ -232,8 +254,7 @@ public void test50KeystorePasswordFromFile() throws Exception {
Path esKeystorePassphraseFile = installation.config.resolve("eks");

rmKeystoreIfExists();
createKeystore();
setKeystorePassword(password);
createKeystore(password);

assertPasswordProtectedKeystore();

Expand Down Expand Up @@ -268,7 +289,7 @@ public void test51WrongKeystorePasswordFromFile() throws Exception {
Files.write(esKeystorePassphraseFile, singletonList("wrongpassword"));

Packages.JournaldWrapper journaldWrapper = new Packages.JournaldWrapper(sh);
Shell.Result result = runElasticsearchStartCommand(false);
Shell.Result result = runElasticsearchStartCommand(null, false, false);
assertElasticsearchFailure(result, Arrays.asList(ERROR_INCORRECT_PASSWORD, ERROR_CORRUPTED_KEYSTORE), journaldWrapper);
} finally {
sh.run("sudo systemctl unset-environment ES_KEYSTORE_PASSPHRASE_FILE");
Expand Down Expand Up @@ -399,7 +420,8 @@ private Path getKeystoreFileFromDockerContainer(String password, Path dockerKeys
return tempDirectory.resolve("elasticsearch.keystore");
}

private void createKeystore() throws Exception {
/** Create a keystore. Provide a password to password-protect it, otherwise use null */
private void createKeystore(String password) throws Exception {
Path keystore = installation.config("elasticsearch.keystore");
final Installation.Executables bin = installation.executables();
bin.keystoreTool.run("create");
Expand All @@ -417,6 +439,10 @@ private void createKeystore() throws Exception {
throw new RuntimeException(e);
}
}

if (password != null) {
setKeystorePassword(password);
}
}

private void rmKeystoreIfExists() {
Expand Down
2 changes: 1 addition & 1 deletion qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ public void test90DoNotCloseStderrWhenQuiet() throws Exception {

// Make sure we don't pick up the journal entries for previous ES instances.
Packages.JournaldWrapper journald = new Packages.JournaldWrapper(sh);
runElasticsearchStartCommand(true);
runElasticsearchStartCommand(null, true, false);
final Result logs = journald.getLogs();

assertThat(logs.stdout, containsString("Failed to load settings from [elasticsearch.yml]"));
Expand Down
33 changes: 18 additions & 15 deletions qa/os/src/test/java/org/elasticsearch/packaging/test/PackagingTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ protected static void cleanup() throws Exception {
*/
protected void assertWhileRunning(Platforms.PlatformAction assertions) throws Exception {
try {
awaitElasticsearchStartup(runElasticsearchStartCommand(true));
awaitElasticsearchStartup(runElasticsearchStartCommand(null, true, false));
} catch (Exception e) {
if (Files.exists(installation.home.resolve("elasticsearch.pid"))) {
String pid = FileUtils.slurp(installation.home.resolve("elasticsearch.pid")).trim();
Expand Down Expand Up @@ -249,14 +249,27 @@ protected void assertWhileRunning(Platforms.PlatformAction assertions) throws Ex
* Run the command to start Elasticsearch, but don't wait or test for success.
* This method is useful for testing failure conditions in startup. To await success,
* use {@link #startElasticsearch()}.
* @param password Password for password-protected keystore, null for no password;
* this option will fail for non-archive distributions
* @param daemonize Run Elasticsearch in the background
* @param useTty Use a tty for inputting the password rather than standard input;
* this option will fail for non-archive distributions
* @return Shell results of the startup command.
* @throws Exception when command fails immediately.
*/
public Shell.Result runElasticsearchStartCommand(boolean daemonize) throws Exception {
public Shell.Result runElasticsearchStartCommand(String password, boolean daemonize, boolean useTty) throws Exception {
if (password != null) {
assertTrue("Only archives support user-entered passwords", distribution().isArchive());
}

switch (distribution.packaging) {
case TAR:
case ZIP:
return Archives.runElasticsearchStartCommand(installation, sh, null, daemonize);
if (useTty) {
return Archives.startElasticsearchWithTty(installation, sh, password, daemonize);
} else {
return Archives.runElasticsearchStartCommand(installation, sh, password, daemonize);
}
case DEB:
case RPM:
return Packages.runElasticsearchStartCommand(sh);
Expand Down Expand Up @@ -307,21 +320,11 @@ public void awaitElasticsearchStartup(Shell.Result result) throws Exception {

/**
* Start Elasticsearch and wait until it's up and running. If you just want to run
* the start command, use {@link #runElasticsearchStartCommand(boolean)}.
* the start command, use {@link #runElasticsearchStartCommand(String, boolean, boolean)}.
* @throws Exception if Elasticsearch can't start
*/
public void startElasticsearch() throws Exception {
awaitElasticsearchStartup(runElasticsearchStartCommand(true));
}

public Shell.Result startElasticsearchStandardInputPassword(String password, boolean daemonize) {
assertTrue("Only archives support passwords on standard input", distribution().isArchive());
return Archives.runElasticsearchStartCommand(installation, sh, password, daemonize);
}

public Shell.Result startElasticsearchTtyPassword(String password, boolean daemonize) throws Exception {
assertTrue("Only archives support passwords on TTY", distribution().isArchive());
return Archives.startElasticsearchWithTty(installation, sh, password, daemonize);
awaitElasticsearchStartup(runElasticsearchStartCommand(null, true, false));
}

public void assertElasticsearchFailure(Shell.Result result, String expectedMessage, Packages.JournaldWrapper journaldWrapper) {
Expand Down
6 changes: 5 additions & 1 deletion qa/os/src/test/java/org/elasticsearch/packaging/util/Archives.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,12 +241,16 @@ public static Shell.Result startElasticsearchWithTty(Installation installation,
final Path pidFile = installation.home.resolve("elasticsearch.pid");
final Installation.Executables bin = installation.executables();

// requires the "expect" utility to be installed
List<String> command = new ArrayList<>();
command.add("sudo -E -u %s %s -p %s");

// requires the "expect" utility to be installed
// TODO: daemonization isn't working with expect versions prior to 5.45, but centos-6 has 5.45.1.15
// TODO: try using pty4j to make daemonization work
if (daemonize) {
command.add("-d");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should be removing daemonize here, since the method still takes in the flag. Wouldn't this cause any tests using tty but not expecting failure to fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here is that I want spawn -ignore HUP to let the process keep running in the background after the expect program ends. This is how it works on centos 7, and that's why the tests are passing after this change: it's as if a keyboard user is running without daemonization.

On centos 6, it seems that when expect terminates, it's terminating Elasticsearch too when the -d flag is added.

But I agree that the code shouldn't be merged in this state. I think I can improve it in a couple of ways, including not taking and disregarding a boolean flag in this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've restored the "daemonize" block, but put a TODO/ignore on the test that would cause the problem. Hopefully the specific issue will be clear to anyone who works on the code. I'm hoping to find a different workaround after more investigation.

}

String script = String.format(
Locale.ROOT,
"expect -c \"$(cat<<EXPECT\n"
Expand Down