From 2b549e7342ebb59ca4ad35dd5109f610ad411350 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Wed, 21 Aug 2019 14:46:07 -0400 Subject: [PATCH] CLI tools: write errors to stderr instead of stdout (#45586) Most of our CLI tools use the Terminal class, which previously did not provide methods for writing to standard output. When all output goes to standard out, there are two basic problems. First, errors and warnings are "swallowed" in pipelines, making it hard for a user to know when something's gone wrong. Second, errors and warnings are intermingled with legitimate output, making it difficult to pass the results of interactive scripts to other tools. This commit adds a second set of print commands to Terminal for printing to standard error, with errorPrint corresponding to print and errorPrintln corresponding to println. This leaves it to developers to decide which output should go where. It also adjusts existing commands to send errors and warnings to stderr. Usage is printed to standard output when it's correctly requested (e.g., bin/elasticsearch-keystore --help) but goes to standard error when a command is invoked incorrectly (e.g. bin/elasticsearch-keystore list-with-a-typo | sort). --- .../plugins/ListPluginsCommand.java | 2 +- .../plugins/InstallPluginCommandTests.java | 8 +- .../plugins/ListPluginsCommandTests.java | 7 +- .../plugins/RemovePluginCommandTests.java | 7 +- .../java/org/elasticsearch/cli/Command.java | 29 ++-- .../java/org/elasticsearch/cli/Terminal.java | 46 +++++- .../bootstrap/EvilElasticsearchCliTests.java | 4 +- .../elasticsearch/cli/EvilCommandTests.java | 2 +- .../packaging/test/ArchiveTests.java | 2 +- .../elasticsearch/plugins/PluginSecurity.java | 12 +- .../bootstrap/ElasticsearchCliTests.java | 36 ++-- .../org/elasticsearch/cli/CommandTests.java | 33 +++- .../org/elasticsearch/cli/TerminalTests.java | 34 ++++ .../bootstrap/ESElasticsearchCliTestCase.java | 9 +- .../org/elasticsearch/cli/MockTerminal.java | 21 ++- .../security/cli/CertificateGenerateTool.java | 6 +- .../xpack/security/cli/CertificateTool.java | 13 +- .../security/cli/CertificateToolTests.java | 4 +- .../esnative/tool/SetupPasswordTool.java | 154 +++++++++--------- .../security/authc/file/tool/UsersTool.java | 8 +- .../authc/saml/SamlMetadataCommand.java | 19 ++- .../support/FileAttributesChecker.java | 8 +- .../esnative/tool/SetupPasswordToolTests.java | 2 +- .../authc/saml/SamlMetadataCommandTests.java | 8 +- .../authc/file/tool/UsersToolTests.java | 2 +- .../support/FileAttributesCheckerTests.java | 9 +- .../snapshots/S3CleanupTests.java | 1 + 27 files changed, 309 insertions(+), 177 deletions(-) diff --git a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/ListPluginsCommand.java b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/ListPluginsCommand.java index ffb9adee8e2..2acfbad5101 100644 --- a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/ListPluginsCommand.java +++ b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/ListPluginsCommand.java @@ -66,7 +66,7 @@ class ListPluginsCommand extends EnvironmentAwareCommand { PluginInfo info = PluginInfo.readFromProperties(env.pluginsFile().resolve(plugin)); terminal.println(Terminal.Verbosity.VERBOSE, info.toString(prefix)); if (info.getElasticsearchVersion().equals(Version.CURRENT) == false) { - terminal.println("WARNING: plugin [" + info.getName() + "] was built for Elasticsearch version " + info.getVersion() + + terminal.errorPrintln("WARNING: plugin [" + info.getName() + "] was built for Elasticsearch version " + info.getVersion() + " but version " + Version.CURRENT + " is required"); } } diff --git a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java index 8a890d8d7ff..45fbd3133d1 100644 --- a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java +++ b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java @@ -789,7 +789,7 @@ public class InstallPluginCommandTests extends ESTestCase { public void testBatchFlag() throws Exception { MockTerminal terminal = new MockTerminal(); installPlugin(terminal, true); - assertThat(terminal.getOutput(), containsString("WARNING: plugin requires additional permissions")); + assertThat(terminal.getErrorOutput(), containsString("WARNING: plugin requires additional permissions")); assertThat(terminal.getOutput(), containsString("-> Downloading")); // No progress bar in batch mode assertThat(terminal.getOutput(), not(containsString("100%"))); @@ -1225,7 +1225,7 @@ public class InstallPluginCommandTests extends ESTestCase { UserException e = expectThrows(UserException.class, () -> installPlugin(pluginZip, env.v1())); assertEquals("installation aborted by user", e.getMessage()); - assertThat(terminal.getOutput(), containsString("WARNING: " + warning)); + assertThat(terminal.getErrorOutput(), containsString("WARNING: " + warning)); try (Stream fileStream = Files.list(env.v2().pluginsFile())) { assertThat(fileStream.collect(Collectors.toList()), empty()); } @@ -1238,7 +1238,7 @@ public class InstallPluginCommandTests extends ESTestCase { terminal.addTextInput("n"); e = expectThrows(UserException.class, () -> installPlugin(pluginZip, env.v1())); assertEquals("installation aborted by user", e.getMessage()); - assertThat(terminal.getOutput(), containsString("WARNING: " + warning)); + assertThat(terminal.getErrorOutput(), containsString("WARNING: " + warning)); try (Stream fileStream = Files.list(env.v2().pluginsFile())) { assertThat(fileStream.collect(Collectors.toList()), empty()); } @@ -1251,7 +1251,7 @@ public class InstallPluginCommandTests extends ESTestCase { } installPlugin(pluginZip, env.v1()); for (String warning : warnings) { - assertThat(terminal.getOutput(), containsString("WARNING: " + warning)); + assertThat(terminal.getErrorOutput(), containsString("WARNING: " + warning)); } } diff --git a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java index 8144c5f0600..bb839008d91 100644 --- a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java +++ b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java @@ -247,8 +247,11 @@ public class ListPluginsCommandTests extends ESTestCase { MockTerminal terminal = listPlugins(home); String message = "plugin [fake_plugin1] was built for Elasticsearch version 1.0 but version " + Version.CURRENT + " is required"; assertEquals( - "fake_plugin1\n" + "WARNING: " + message + "\n" + "fake_plugin2\n", - terminal.getOutput()); + "fake_plugin1\nfake_plugin2\n", + terminal.getOutput()); + assertEquals( + "WARNING: " + message + "\n", + terminal.getErrorOutput()); String[] params = {"-s"}; terminal = listPlugins(home, params); diff --git a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java index c62d37a4e28..40f17196472 100644 --- a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java +++ b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java @@ -237,11 +237,14 @@ public class RemovePluginCommandTests extends ESTestCase { return false; } }.main(new String[] { "-Epath.home=" + home, "fake" }, terminal); - try (BufferedReader reader = new BufferedReader(new StringReader(terminal.getOutput()))) { + try (BufferedReader reader = new BufferedReader(new StringReader(terminal.getOutput())); + BufferedReader errorReader = new BufferedReader(new StringReader(terminal.getErrorOutput())) + ) { assertEquals("-> removing [fake]...", reader.readLine()); assertEquals("ERROR: plugin [fake] not found; run 'elasticsearch-plugin list' to get list of installed plugins", - reader.readLine()); + errorReader.readLine()); assertNull(reader.readLine()); + assertNull(errorReader.readLine()); } } diff --git a/libs/cli/src/main/java/org/elasticsearch/cli/Command.java b/libs/cli/src/main/java/org/elasticsearch/cli/Command.java index 34ede7ccf94..2a270153f47 100644 --- a/libs/cli/src/main/java/org/elasticsearch/cli/Command.java +++ b/libs/cli/src/main/java/org/elasticsearch/cli/Command.java @@ -73,7 +73,7 @@ public abstract class Command implements Closeable { StringWriter sw = new StringWriter(); PrintWriter pw = new PrintWriter(sw)) { e.printStackTrace(pw); - terminal.println(sw.toString()); + terminal.errorPrintln(sw.toString()); } catch (final IOException impossible) { // StringWriter#close declares a checked IOException from the Closeable interface but the Javadocs for StringWriter // say that an exception here is impossible @@ -89,14 +89,15 @@ public abstract class Command implements Closeable { try { mainWithoutErrorHandling(args, terminal); } catch (OptionException e) { - printHelp(terminal); - terminal.println(Terminal.Verbosity.SILENT, "ERROR: " + e.getMessage()); + // print help to stderr on exceptions + printHelp(terminal, true); + terminal.errorPrintln(Terminal.Verbosity.SILENT, "ERROR: " + e.getMessage()); return ExitCodes.USAGE; } catch (UserException e) { if (e.exitCode == ExitCodes.USAGE) { - printHelp(terminal); + printHelp(terminal, true); } - terminal.println(Terminal.Verbosity.SILENT, "ERROR: " + e.getMessage()); + terminal.errorPrintln(Terminal.Verbosity.SILENT, "ERROR: " + e.getMessage()); return e.exitCode; } return ExitCodes.OK; @@ -109,7 +110,7 @@ public abstract class Command implements Closeable { final OptionSet options = parser.parse(args); if (options.has(helpOption)) { - printHelp(terminal); + printHelp(terminal, false); return; } @@ -125,11 +126,17 @@ public abstract class Command implements Closeable { } /** Prints a help message for the command to the terminal. */ - private void printHelp(Terminal terminal) throws IOException { - terminal.println(description); - terminal.println(""); - printAdditionalHelp(terminal); - parser.printHelpOn(terminal.getWriter()); + private void printHelp(Terminal terminal, boolean toStdError) throws IOException { + if (toStdError) { + terminal.errorPrintln(description); + terminal.errorPrintln(""); + parser.printHelpOn(terminal.getErrorWriter()); + } else { + terminal.println(description); + terminal.println(""); + printAdditionalHelp(terminal); + parser.printHelpOn(terminal.getWriter()); + } } /** Prints additional help information, specific to the command */ diff --git a/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java b/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java index a0ebff5d670..718b4796c02 100644 --- a/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java +++ b/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java @@ -39,9 +39,17 @@ import java.util.Locale; */ public abstract class Terminal { + /** Writer to standard error - not supplied by the {@link Console} API, so we share with subclasses */ + private static final PrintWriter ERROR_WRITER = newErrorWriter(); + /** The default terminal implementation, which will be a console if available, or stdout/stderr if not. */ public static final Terminal DEFAULT = ConsoleTerminal.isSupported() ? new ConsoleTerminal() : new SystemTerminal(); + @SuppressForbidden(reason = "Writer for System.err") + private static PrintWriter newErrorWriter() { + return new PrintWriter(System.err); + } + /** Defines the available verbosity levels of messages to be printed. */ public enum Verbosity { SILENT, /* always printed */ @@ -70,9 +78,14 @@ public abstract class Terminal { /** Reads password text from the terminal input. See {@link Console#readPassword()}}. */ public abstract char[] readSecret(String prompt); - /** Returns a Writer which can be used to write to the terminal directly. */ + /** Returns a Writer which can be used to write to the terminal directly using standard output. */ public abstract PrintWriter getWriter(); + /** Returns a Writer which can be used to write to the terminal directly using standard error. */ + public PrintWriter getErrorWriter() { + return ERROR_WRITER; + } + /** Prints a line to the terminal at {@link Verbosity#NORMAL} verbosity level. */ public final void println(String msg) { println(Verbosity.NORMAL, msg); @@ -83,14 +96,35 @@ public abstract class Terminal { print(verbosity, msg + lineSeparator); } - /** Prints message to the terminal at {@code verbosity} level, without a newline. */ + /** Prints message to the terminal's standard output at {@code verbosity} level, without a newline. */ public final void print(Verbosity verbosity, String msg) { + print(verbosity, msg, false); + } + + /** Prints message to the terminal at {@code verbosity} level, without a newline. */ + private void print(Verbosity verbosity, String msg, boolean isError) { if (isPrintable(verbosity)) { - getWriter().print(msg); - getWriter().flush(); + PrintWriter writer = isError ? getErrorWriter() : getWriter(); + writer.print(msg); + writer.flush(); } } + /** Prints a line to the terminal's standard error at {@link Verbosity#NORMAL} verbosity level, without a newline. */ + public final void errorPrint(Verbosity verbosity, String msg) { + print(verbosity, msg, true); + } + + /** Prints a line to the terminal's standard error at {@link Verbosity#NORMAL} verbosity level. */ + public final void errorPrintln(String msg) { + errorPrintln(Verbosity.NORMAL, msg); + } + + /** Prints a line to the terminal's standard error at {@code verbosity} level. */ + public final void errorPrintln(Verbosity verbosity, String msg) { + errorPrint(verbosity, msg + lineSeparator); + } + /** Checks if is enough {@code verbosity} level to be printed */ public final boolean isPrintable(Verbosity verbosity) { return this.verbosity.ordinal() >= verbosity.ordinal(); @@ -110,7 +144,7 @@ public abstract class Terminal { answer = answer.toLowerCase(Locale.ROOT); boolean answerYes = answer.equals("y"); if (answerYes == false && answer.equals("n") == false) { - println("Did not understand answer '" + answer + "'"); + errorPrintln("Did not understand answer '" + answer + "'"); continue; } return answerYes; @@ -165,7 +199,7 @@ public abstract class Terminal { @Override public String readText(String text) { - getWriter().print(text); + getErrorWriter().print(text); // prompts should go to standard error to avoid mixing with list output BufferedReader reader = new BufferedReader(new InputStreamReader(System.in, Charset.defaultCharset())); try { final String line = reader.readLine(); diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilElasticsearchCliTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilElasticsearchCliTests.java index e670a4364fe..9e1d6e5710e 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilElasticsearchCliTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilElasticsearchCliTests.java @@ -40,7 +40,7 @@ public class EvilElasticsearchCliTests extends ESElasticsearchCliTestCase { runTest( ExitCodes.OK, true, - output -> {}, + (output, error) -> {}, (foreground, pidFile, quiet, esSettings) -> { Settings settings = esSettings.settings(); assertThat(settings.keySet(), hasSize(2)); @@ -55,7 +55,7 @@ public class EvilElasticsearchCliTests extends ESElasticsearchCliTestCase { runTest( ExitCodes.OK, true, - output -> {}, + (output, error) -> {}, (foreground, pidFile, quiet, esSettings) -> { Settings settings = esSettings.settings(); assertThat(settings.keySet(), hasSize(2)); diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/cli/EvilCommandTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/cli/EvilCommandTests.java index 2990101134f..824dd90ec22 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/cli/EvilCommandTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/cli/EvilCommandTests.java @@ -55,7 +55,7 @@ public class EvilCommandTests extends ESTestCase { command.getShutdownHookThread().run(); command.getShutdownHookThread().join(); assertTrue(closed.get()); - final String output = terminal.getOutput(); + final String output = terminal.getErrorOutput(); if (shouldThrow) { // ensure that we dump the exception assertThat(output, containsString("java.io.IOException: fail")); diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java index d06efb37a3d..0e76d4f3019 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java @@ -377,7 +377,7 @@ public class ArchiveTests extends PackagingTestCase { // Ensure that the exit code from the java command is passed back up through the shell script result = sh.runIgnoreExitCode(bin.elasticsearchCertutil + " invalid-command"); assertThat(result.exitCode, is(not(0))); - assertThat(result.stdout, containsString("Unknown command [invalid-command]")); + assertThat(result.stderr, containsString("Unknown command [invalid-command]")); }; Platforms.onLinux(action); Platforms.onWindows(action); diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginSecurity.java b/server/src/main/java/org/elasticsearch/plugins/PluginSecurity.java index d2246259ab7..c845ff4d3a5 100644 --- a/server/src/main/java/org/elasticsearch/plugins/PluginSecurity.java +++ b/server/src/main/java/org/elasticsearch/plugins/PluginSecurity.java @@ -55,15 +55,15 @@ class PluginSecurity { // sort permissions in a reasonable order Collections.sort(requested); - terminal.println(Verbosity.NORMAL, "@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@"); - terminal.println(Verbosity.NORMAL, "@ WARNING: plugin requires additional permissions @"); - terminal.println(Verbosity.NORMAL, "@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@"); + terminal.errorPrintln(Verbosity.NORMAL, "@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@"); + terminal.errorPrintln(Verbosity.NORMAL, "@ WARNING: plugin requires additional permissions @"); + terminal.errorPrintln(Verbosity.NORMAL, "@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@"); // print all permissions: for (String permission : requested) { - terminal.println(Verbosity.NORMAL, "* " + permission); + terminal.errorPrintln(Verbosity.NORMAL, "* " + permission); } - terminal.println(Verbosity.NORMAL, "See http://docs.oracle.com/javase/8/docs/technotes/guides/security/permissions.html"); - terminal.println(Verbosity.NORMAL, "for descriptions of what these permissions allow and the associated risks."); + terminal.errorPrintln(Verbosity.NORMAL, "See http://docs.oracle.com/javase/8/docs/technotes/guides/security/permissions.html"); + terminal.errorPrintln(Verbosity.NORMAL, "for descriptions of what these permissions allow and the associated risks."); prompt(terminal, batch); } } diff --git a/server/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java b/server/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java index 1f0a953a707..fb6925ba2eb 100644 --- a/server/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java +++ b/server/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java @@ -26,7 +26,7 @@ import org.elasticsearch.monitor.jvm.JvmInfo; import java.nio.file.Path; import java.util.Locale; -import java.util.function.Consumer; +import java.util.function.BiConsumer; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; @@ -53,15 +53,15 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { private void runTestThatVersionIsMutuallyExclusiveToOtherOptions(String... args) throws Exception { runTestVersion( ExitCodes.USAGE, - output -> assertThat( - output, + (output, error) -> assertThat( + error, allOf(containsString("ERROR:"), containsString("are unavailable given other options on the command line"))), args); } private void runTestThatVersionIsReturned(String... args) throws Exception { - runTestVersion(ExitCodes.OK, output -> { + runTestVersion(ExitCodes.OK, (output, error) -> { assertThat(output, containsString("Version: " + Build.CURRENT.getQualifiedVersion())); final String expectedBuildOutput = String.format( Locale.ROOT, @@ -75,7 +75,7 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { }, args); } - private void runTestVersion(int expectedStatus, Consumer outputConsumer, String... args) throws Exception { + private void runTestVersion(int expectedStatus, BiConsumer outputConsumer, String... args) throws Exception { runTest(expectedStatus, false, outputConsumer, (foreground, pidFile, quiet, esSettings) -> {}, args); } @@ -83,19 +83,19 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { runTest( ExitCodes.USAGE, false, - output -> assertThat(output, containsString("Positional arguments not allowed, found [foo]")), + (output, error) -> assertThat(error, containsString("Positional arguments not allowed, found [foo]")), (foreground, pidFile, quiet, esSettings) -> {}, "foo"); runTest( ExitCodes.USAGE, false, - output -> assertThat(output, containsString("Positional arguments not allowed, found [foo, bar]")), + (output, error) -> assertThat(error, containsString("Positional arguments not allowed, found [foo, bar]")), (foreground, pidFile, quiet, esSettings) -> {}, "foo", "bar"); runTest( ExitCodes.USAGE, false, - output -> assertThat(output, containsString("Positional arguments not allowed, found [foo]")), + (output, error) -> assertThat(error, containsString("Positional arguments not allowed, found [foo]")), (foreground, pidFile, quiet, esSettings) -> {}, "-E", "foo=bar", "foo", "-E", "baz=qux"); } @@ -104,12 +104,12 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { Path tmpDir = createTempDir(); Path pidFile = tmpDir.resolve("pid"); runPidFileTest(ExitCodes.USAGE, false, - output -> assertThat(output, containsString("Option p/pidfile requires an argument")), pidFile, "-p"); - runPidFileTest(ExitCodes.OK, true, output -> {}, pidFile, "-p", pidFile.toString()); - runPidFileTest(ExitCodes.OK, true, output -> {}, pidFile, "--pidfile", tmpDir.toString() + "/pid"); + (output, error) -> assertThat(error, containsString("Option p/pidfile requires an argument")), pidFile, "-p"); + runPidFileTest(ExitCodes.OK, true, (output, error) -> {}, pidFile, "-p", pidFile.toString()); + runPidFileTest(ExitCodes.OK, true, (output, error) -> {}, pidFile, "--pidfile", tmpDir.toString() + "/pid"); } - private void runPidFileTest(final int expectedStatus, final boolean expectedInit, Consumer outputConsumer, + private void runPidFileTest(final int expectedStatus, final boolean expectedInit, BiConsumer outputConsumer, Path expectedPidFile, final String... args) throws Exception { runTest( @@ -130,7 +130,7 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { runTest( ExitCodes.OK, true, - output -> {}, + (output, error) -> {}, (foreground, pidFile, quiet, esSettings) -> assertThat(foreground, equalTo(!expectedDaemonize)), args); } @@ -145,7 +145,7 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { runTest( ExitCodes.OK, true, - output -> {}, + (output, error) -> {}, (foreground, pidFile, quiet, esSettings) -> assertThat(quiet, equalTo(expectedQuiet)), args); } @@ -154,7 +154,7 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { runTest( ExitCodes.OK, true, - output -> {}, + (output, error) -> {}, (foreground, pidFile, quiet, env) -> { Settings settings = env.settings(); assertEquals("bar", settings.get("foo")); @@ -167,7 +167,7 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { runTest( ExitCodes.USAGE, false, - output -> assertThat(output, containsString("setting [foo] must not be empty")), + (output, error) -> assertThat(error, containsString("setting [foo] must not be empty")), (foreground, pidFile, quiet, esSettings) -> {}, "-E", "foo="); } @@ -176,7 +176,7 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { runTest( ExitCodes.USAGE, false, - output -> assertThat(output, containsString("setting [foo] already set, saw [bar] and [baz]")), + (output, error) -> assertThat(error, containsString("setting [foo] already set, saw [bar] and [baz]")), (foreground, pidFile, quiet, initialEnv) -> {}, "-E", "foo=bar", "-E", "foo=baz"); } @@ -185,7 +185,7 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase { runTest( ExitCodes.USAGE, false, - output -> assertThat(output, containsString("network.host is not a recognized option")), + (output, error) -> assertThat(error, containsString("network.host is not a recognized option")), (foreground, pidFile, quiet, esSettings) -> {}, "--network.host"); } diff --git a/server/src/test/java/org/elasticsearch/cli/CommandTests.java b/server/src/test/java/org/elasticsearch/cli/CommandTests.java index 2b2437eea65..092e5dfd480 100644 --- a/server/src/test/java/org/elasticsearch/cli/CommandTests.java +++ b/server/src/test/java/org/elasticsearch/cli/CommandTests.java @@ -110,6 +110,31 @@ public class CommandTests extends ESTestCase { assertFalse(command.executed); } + public void testUnknownOptions() throws Exception { + NoopCommand command = new NoopCommand(); + MockTerminal terminal = new MockTerminal(); + String[] args = {"-Z"}; + int status = command.main(args, terminal); + String output = terminal.getOutput(); + String error = terminal.getErrorOutput(); + assertEquals(output, ExitCodes.USAGE, status); + assertTrue(error, error.contains("Does nothing")); + assertFalse(output, output.contains("Some extra help")); // extra help not printed for usage errors + assertTrue(error, error.contains("ERROR: Z is not a recognized option")); + assertFalse(command.executed); + + command = new NoopCommand(); + String[] args2 = {"--foobar"}; + status = command.main(args2, terminal); + output = terminal.getOutput(); + error = terminal.getErrorOutput(); + assertEquals(output, ExitCodes.USAGE, status); + assertTrue(error, error.contains("Does nothing")); + assertFalse(output, output.contains("Some extra help")); // extra help not printed for usage errors + assertTrue(error, error.contains("ERROR: Z is not a recognized option")); + assertFalse(command.executed); + } + public void testVerbositySilentAndVerbose() throws Exception { MockTerminal terminal = new MockTerminal(); NoopCommand command = new NoopCommand(); @@ -155,8 +180,9 @@ public class CommandTests extends ESTestCase { String[] args = {}; int status = command.main(args, terminal); String output = terminal.getOutput(); + String error = terminal.getErrorOutput(); assertEquals(output, ExitCodes.DATA_ERROR, status); - assertTrue(output, output.contains("ERROR: Bad input")); + assertTrue(error, error.contains("ERROR: Bad input")); } public void testUsageError() throws Exception { @@ -165,9 +191,10 @@ public class CommandTests extends ESTestCase { String[] args = {}; int status = command.main(args, terminal); String output = terminal.getOutput(); + String error = terminal.getErrorOutput(); assertEquals(output, ExitCodes.USAGE, status); - assertTrue(output, output.contains("Throws a usage error")); - assertTrue(output, output.contains("ERROR: something was no good")); + assertTrue(error, error.contains("Throws a usage error")); + assertTrue(error, error.contains("ERROR: something was no good")); } } diff --git a/server/src/test/java/org/elasticsearch/cli/TerminalTests.java b/server/src/test/java/org/elasticsearch/cli/TerminalTests.java index 3b409c2add6..99bbe9d6184 100644 --- a/server/src/test/java/org/elasticsearch/cli/TerminalTests.java +++ b/server/src/test/java/org/elasticsearch/cli/TerminalTests.java @@ -41,6 +41,26 @@ public class TerminalTests extends ESTestCase { assertPrinted(terminal, Terminal.Verbosity.VERBOSE, "text"); } + public void testErrorVerbosity() throws Exception { + MockTerminal terminal = new MockTerminal(); + terminal.setVerbosity(Terminal.Verbosity.SILENT); + assertErrorPrinted(terminal, Terminal.Verbosity.SILENT, "text"); + assertErrorNotPrinted(terminal, Terminal.Verbosity.NORMAL, "text"); + assertErrorNotPrinted(terminal, Terminal.Verbosity.VERBOSE, "text"); + + terminal = new MockTerminal(); + assertErrorPrinted(terminal, Terminal.Verbosity.SILENT, "text"); + assertErrorPrinted(terminal, Terminal.Verbosity.NORMAL, "text"); + assertErrorNotPrinted(terminal, Terminal.Verbosity.VERBOSE, "text"); + + terminal = new MockTerminal(); + terminal.setVerbosity(Terminal.Verbosity.VERBOSE); + assertErrorPrinted(terminal, Terminal.Verbosity.SILENT, "text"); + assertErrorPrinted(terminal, Terminal.Verbosity.NORMAL, "text"); + assertErrorPrinted(terminal, Terminal.Verbosity.VERBOSE, "text"); + } + + public void testEscaping() throws Exception { MockTerminal terminal = new MockTerminal(); assertPrinted(terminal, Terminal.Verbosity.NORMAL, "This message contains percent like %20n"); @@ -87,4 +107,18 @@ public class TerminalTests extends ESTestCase { String output = logTerminal.getOutput(); assertTrue(output, output.isEmpty()); } + + private void assertErrorPrinted(MockTerminal logTerminal, Terminal.Verbosity verbosity, String text) throws Exception { + logTerminal.errorPrintln(verbosity, text); + String output = logTerminal.getErrorOutput(); + assertTrue(output, output.contains(text)); + logTerminal.reset(); + } + + private void assertErrorNotPrinted(MockTerminal logTerminal, Terminal.Verbosity verbosity, String text) throws Exception { + logTerminal.errorPrintln(verbosity, text); + String output = logTerminal.getErrorOutput(); + assertTrue(output, output.isEmpty()); + } + } diff --git a/test/framework/src/main/java/org/elasticsearch/bootstrap/ESElasticsearchCliTestCase.java b/test/framework/src/main/java/org/elasticsearch/bootstrap/ESElasticsearchCliTestCase.java index 62b2b422f78..d1a2cbb9cc4 100644 --- a/test/framework/src/main/java/org/elasticsearch/bootstrap/ESElasticsearchCliTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/bootstrap/ESElasticsearchCliTestCase.java @@ -28,7 +28,7 @@ import org.elasticsearch.test.ESTestCase; import java.nio.file.Path; import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Consumer; +import java.util.function.BiConsumer; import static org.hamcrest.CoreMatchers.equalTo; @@ -41,7 +41,7 @@ abstract class ESElasticsearchCliTestCase extends ESTestCase { void runTest( final int expectedStatus, final boolean expectedInit, - final Consumer outputConsumer, + final BiConsumer outputConsumer, final InitConsumer initConsumer, final String... args) throws Exception { final MockTerminal terminal = new MockTerminal(); @@ -69,11 +69,12 @@ abstract class ESElasticsearchCliTestCase extends ESTestCase { }, terminal); assertThat(status, equalTo(expectedStatus)); assertThat(init.get(), equalTo(expectedInit)); - outputConsumer.accept(terminal.getOutput()); + outputConsumer.accept(terminal.getOutput(), terminal.getErrorOutput()); } catch (Exception e) { // if an unexpected exception is thrown, we log // terminal output to aid debugging - logger.info(terminal.getOutput()); + logger.info("Stdout:\n" + terminal.getOutput()); + logger.info("Stderr:\n" + terminal.getErrorOutput()); // rethrow so the test fails throw e; } diff --git a/test/framework/src/main/java/org/elasticsearch/cli/MockTerminal.java b/test/framework/src/main/java/org/elasticsearch/cli/MockTerminal.java index 44c968cf507..cff5c1b49fb 100644 --- a/test/framework/src/main/java/org/elasticsearch/cli/MockTerminal.java +++ b/test/framework/src/main/java/org/elasticsearch/cli/MockTerminal.java @@ -33,8 +33,10 @@ import java.util.List; */ public class MockTerminal extends Terminal { - private final ByteArrayOutputStream buffer = new ByteArrayOutputStream(); - private final PrintWriter writer = new PrintWriter(new OutputStreamWriter(buffer, StandardCharsets.UTF_8)); + private final ByteArrayOutputStream stdoutBuffer = new ByteArrayOutputStream(); + private final ByteArrayOutputStream stderrBuffer = new ByteArrayOutputStream(); + private final PrintWriter writer = new PrintWriter(new OutputStreamWriter(stdoutBuffer, StandardCharsets.UTF_8)); + private final PrintWriter errorWriter = new PrintWriter(new OutputStreamWriter(stderrBuffer, StandardCharsets.UTF_8)); // A deque would be a perfect data structure for the FIFO queue of input values needed here. However, // to support the valid return value of readText being null (defined by Console), we need to be able @@ -73,6 +75,11 @@ public class MockTerminal extends Terminal { return writer; } + @Override + public PrintWriter getErrorWriter() { + return errorWriter; + } + /** Adds an an input that will be return from {@link #readText(String)}. Values are read in FIFO order. */ public void addTextInput(String input) { textInput.add(input); @@ -85,12 +92,18 @@ public class MockTerminal extends Terminal { /** Returns all output written to this terminal. */ public String getOutput() throws UnsupportedEncodingException { - return buffer.toString("UTF-8"); + return stdoutBuffer.toString("UTF-8"); + } + + /** Returns all output written to this terminal. */ + public String getErrorOutput() throws UnsupportedEncodingException { + return stderrBuffer.toString("UTF-8"); } /** Wipes the input and output. */ public void reset() { - buffer.reset(); + stdoutBuffer.reset(); + stderrBuffer.reset(); textIndex = 0; textInput.clear(); secretIndex = 0; diff --git a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateGenerateTool.java b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateGenerateTool.java index 4b30224dcd4..a100afe33aa 100644 --- a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateGenerateTool.java +++ b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateGenerateTool.java @@ -285,12 +285,12 @@ public class CertificateGenerateTool extends EnvironmentAwareCommand { final List errors = certInfo.validate(); if (errors.size() > 0) { hasError = true; - terminal.println(Terminal.Verbosity.SILENT, "Configuration for instance " + certInfo.name.originalName + terminal.errorPrintln(Terminal.Verbosity.SILENT, "Configuration for instance " + certInfo.name.originalName + " has invalid details"); for (String message : errors) { - terminal.println(Terminal.Verbosity.SILENT, " * " + message); + terminal.errorPrintln(Terminal.Verbosity.SILENT, " * " + message); } - terminal.println(""); + terminal.errorPrintln(""); } } if (hasError) { diff --git a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java index 435305b8a69..53e3fadf168 100644 --- a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java +++ b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java @@ -417,7 +417,7 @@ public class CertificateTool extends LoggingAwareMultiCommand { if (validationErrors.isEmpty()) { return Collections.singleton(information); } else { - validationErrors.forEach(terminal::println); + validationErrors.forEach(terminal::errorPrintln); return Collections.emptyList(); } } @@ -477,7 +477,7 @@ public class CertificateTool extends LoggingAwareMultiCommand { if (Name.isValidFilename(filename)) { return filename; } else { - terminal.println(Terminal.Verbosity.SILENT, "'" + filename + "' is not a valid filename"); + terminal.errorPrintln(Terminal.Verbosity.SILENT, "'" + filename + "' is not a valid filename"); continue; } } @@ -891,11 +891,12 @@ public class CertificateTool extends LoggingAwareMultiCommand { final List errors = certInfo.validate(); if (errors.size() > 0) { hasError = true; - terminal.println(Verbosity.SILENT, "Configuration for instance " + certInfo.name.originalName + " has invalid details"); + terminal.errorPrintln(Verbosity.SILENT, "Configuration for instance " + certInfo.name.originalName + + " has invalid details"); for (String message : errors) { - terminal.println(Verbosity.SILENT, " * " + message); + terminal.errorPrintln(Verbosity.SILENT, " * " + message); } - terminal.println(""); + terminal.errorPrintln(""); } } if (hasError) { @@ -961,7 +962,7 @@ public class CertificateTool extends LoggingAwareMultiCommand { return; } if (Files.exists(parent)) { - terminal.println(Terminal.Verbosity.SILENT, "Path " + parent + " exists, but is not a directory. Cannot write to " + path); + terminal.errorPrintln(Terminal.Verbosity.SILENT, "Path " + parent + " exists, but is not a directory. Cannot write to " + path); throw new UserException(ExitCodes.CANT_CREATE, "Cannot write to " + path); } if (terminal.promptYesNo("Directory " + parent + " does not exist. Do you want to create it?", true)) { diff --git a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java index 9e970ea559a..6845edbdc6b 100644 --- a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java +++ b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java @@ -242,8 +242,8 @@ public class CertificateToolTests extends ESTestCase { () -> CertificateTool.parseAndValidateFile(terminal, instanceFile)); assertThat(exception.getMessage(), containsString("invalid configuration")); assertThat(exception.getMessage(), containsString(instanceFile.toString())); - assertThat(terminal.getOutput(), containsString("THIS=not a,valid DN")); - assertThat(terminal.getOutput(), containsString("could not be converted to a valid DN")); + assertThat(terminal.getErrorOutput(), containsString("THIS=not a,valid DN")); + assertThat(terminal.getErrorOutput(), containsString("could not be converted to a valid DN")); } public void testGeneratingCsr() throws Exception { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordTool.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordTool.java index 866f3722e6e..5ac81a06480 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordTool.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordTool.java @@ -195,15 +195,15 @@ public class SetupPasswordTool extends LoggingAwareMultiCommand { SecureString password1 = new SecureString(terminal.readSecret("Enter password for [" + user + "]: ")); Validation.Error err = Validation.Users.validatePassword(password1); if (err != null) { - terminal.println(err.toString()); - terminal.println("Try again."); + terminal.errorPrintln(err.toString()); + terminal.errorPrintln("Try again."); password1.close(); continue; } try (SecureString password2 = new SecureString(terminal.readSecret("Reenter password for [" + user + "]: "))) { if (password1.equals(password2) == false) { - terminal.println("Passwords do not match."); - terminal.println("Try again."); + terminal.errorPrintln("Passwords do not match."); + terminal.errorPrintln("Try again."); password1.close(); continue; } @@ -302,53 +302,55 @@ public class SetupPasswordTool extends LoggingAwareMultiCommand { // keystore password is not valid if (httpCode == HttpURLConnection.HTTP_UNAUTHORIZED) { - terminal.println(""); - terminal.println("Failed to authenticate user '" + elasticUser + "' against " + route.toString()); - terminal.println("Possible causes include:"); - terminal.println(" * The password for the '" + elasticUser + "' user has already been changed on this cluster"); - terminal.println(" * Your elasticsearch node is running against a different keystore"); - terminal.println(" This tool used the keystore at " + KeyStoreWrapper.keystorePath(env.configFile())); - terminal.println(""); + terminal.errorPrintln(""); + terminal.errorPrintln("Failed to authenticate user '" + elasticUser + "' against " + route.toString()); + terminal.errorPrintln("Possible causes include:"); + terminal.errorPrintln(" * The password for the '" + elasticUser + "' user has already been changed on this cluster"); + terminal.errorPrintln(" * Your elasticsearch node is running against a different keystore"); + terminal.errorPrintln(" This tool used the keystore at " + KeyStoreWrapper.keystorePath(env.configFile())); + terminal.errorPrintln(""); throw new UserException(ExitCodes.CONFIG, "Failed to verify bootstrap password"); } else if (httpCode != HttpURLConnection.HTTP_OK) { - terminal.println(""); - terminal.println("Unexpected response code [" + httpCode + "] from calling GET " + route.toString()); + terminal.errorPrintln(""); + terminal.errorPrintln("Unexpected response code [" + httpCode + "] from calling GET " + route.toString()); XPackSecurityFeatureConfig xPackSecurityFeatureConfig = getXPackSecurityConfig(terminal); if (xPackSecurityFeatureConfig.isAvailable == false) { - terminal.println("It doesn't look like the X-Pack security feature is available on this Elasticsearch node."); - terminal.println("Please check if you have installed a license that allows access to X-Pack Security feature."); - terminal.println(""); + terminal.errorPrintln("It doesn't look like the X-Pack security feature is available on this Elasticsearch node."); + terminal.errorPrintln("Please check if you have installed a license that allows access to " + + "X-Pack Security feature."); + terminal.errorPrintln(""); throw new UserException(ExitCodes.CONFIG, "X-Pack Security is not available."); } if (xPackSecurityFeatureConfig.isEnabled == false) { - terminal.println("It doesn't look like the X-Pack security feature is enabled on this Elasticsearch node."); - terminal.println("Please check if you have enabled X-Pack security in your elasticsearch.yml configuration file."); - terminal.println(""); + terminal.errorPrintln("It doesn't look like the X-Pack security feature is enabled on this Elasticsearch node."); + terminal.errorPrintln("Please check if you have enabled X-Pack security in your elasticsearch.yml " + + "configuration file."); + terminal.errorPrintln(""); throw new UserException(ExitCodes.CONFIG, "X-Pack Security is disabled by configuration."); } - terminal.println("X-Pack security feature is available and enabled on this Elasticsearch node."); - terminal.println("Possible causes include:"); - terminal.println(" * The relative path of the URL is incorrect. Is there a proxy in-between?"); - terminal.println(" * The protocol (http/https) does not match the port."); - terminal.println(" * Is this really an Elasticsearch server?"); - terminal.println(""); + terminal.errorPrintln("X-Pack security feature is available and enabled on this Elasticsearch node."); + terminal.errorPrintln("Possible causes include:"); + terminal.errorPrintln(" * The relative path of the URL is incorrect. Is there a proxy in-between?"); + terminal.errorPrintln(" * The protocol (http/https) does not match the port."); + terminal.errorPrintln(" * Is this really an Elasticsearch server?"); + terminal.errorPrintln(""); throw new UserException(ExitCodes.CONFIG, "Unknown error"); } } catch (SSLException e) { - terminal.println(""); - terminal.println("SSL connection to " + route.toString() + " failed: " + e.getMessage()); - terminal.println("Please check the elasticsearch SSL settings under " + XPackSettings.HTTP_SSL_PREFIX); - terminal.println(Verbosity.VERBOSE, ""); - terminal.println(Verbosity.VERBOSE, ExceptionsHelper.stackTrace(e)); - terminal.println(""); + terminal.errorPrintln(""); + terminal.errorPrintln("SSL connection to " + route.toString() + " failed: " + e.getMessage()); + terminal.errorPrintln("Please check the elasticsearch SSL settings under " + XPackSettings.HTTP_SSL_PREFIX); + terminal.errorPrintln(Verbosity.VERBOSE, ""); + terminal.errorPrintln(Verbosity.VERBOSE, ExceptionsHelper.stackTrace(e)); + terminal.errorPrintln(""); throw new UserException(ExitCodes.CONFIG, "Failed to establish SSL connection to elasticsearch at " + route.toString() + ". ", e); } catch (IOException e) { - terminal.println(""); - terminal.println("Connection failure to: " + route.toString() + " failed: " + e.getMessage()); - terminal.println(Verbosity.VERBOSE, ""); - terminal.println(Verbosity.VERBOSE, ExceptionsHelper.stackTrace(e)); - terminal.println(""); + terminal.errorPrintln(""); + terminal.errorPrintln("Connection failure to: " + route.toString() + " failed: " + e.getMessage()); + terminal.errorPrintln(Verbosity.VERBOSE, ""); + terminal.errorPrintln(Verbosity.VERBOSE, ExceptionsHelper.stackTrace(e)); + terminal.errorPrintln(""); throw new UserException(ExitCodes.CONFIG, "Failed to connect to elasticsearch at " + route.toString() + ". Is the URL correct and elasticsearch running?", e); } @@ -361,19 +363,20 @@ public class SetupPasswordTool extends LoggingAwareMultiCommand { final HttpResponse httpResponse = client.execute("GET", route, elasticUser, elasticUserPassword, () -> null, is -> responseBuilder(is, terminal)); if (httpResponse.getHttpStatus() != HttpURLConnection.HTTP_OK) { - terminal.println(""); - terminal.println("Unexpected response code [" + httpResponse.getHttpStatus() + "] from calling GET " + route.toString()); + terminal.errorPrintln(""); + terminal.errorPrintln("Unexpected response code [" + httpResponse.getHttpStatus() + "] from calling GET " + + route.toString()); if (httpResponse.getHttpStatus() == HttpURLConnection.HTTP_BAD_REQUEST) { - terminal.println("It doesn't look like the X-Pack is available on this Elasticsearch node."); - terminal.println("Please check that you have followed all installation instructions and that this tool"); - terminal.println(" is pointing to the correct Elasticsearch server."); - terminal.println(""); + terminal.errorPrintln("It doesn't look like the X-Pack is available on this Elasticsearch node."); + terminal.errorPrintln("Please check that you have followed all installation instructions and that this tool"); + terminal.errorPrintln(" is pointing to the correct Elasticsearch server."); + terminal.errorPrintln(""); throw new UserException(ExitCodes.CONFIG, "X-Pack is not available on this Elasticsearch node."); } else { - terminal.println("* Try running this tool again."); - terminal.println("* Verify that the tool is pointing to the correct Elasticsearch server."); - terminal.println("* Check the elasticsearch logs for additional error details."); - terminal.println(""); + terminal.errorPrintln("* Try running this tool again."); + terminal.errorPrintln("* Verify that the tool is pointing to the correct Elasticsearch server."); + terminal.errorPrintln("* Check the elasticsearch logs for additional error details."); + terminal.errorPrintln(""); throw new UserException(ExitCodes.TEMP_FAILURE, "Failed to determine x-pack security feature configuration."); } } @@ -406,33 +409,34 @@ public class SetupPasswordTool extends LoggingAwareMultiCommand { final HttpResponse httpResponse = client.execute("GET", route, elasticUser, elasticUserPassword, () -> null, is -> responseBuilder(is, terminal)); if (httpResponse.getHttpStatus() != HttpURLConnection.HTTP_OK) { - terminal.println(""); - terminal.println("Failed to determine the health of the cluster running at " + url); - terminal.println("Unexpected response code [" + httpResponse.getHttpStatus() + "] from calling GET " + route.toString()); + terminal.errorPrintln(""); + terminal.errorPrintln("Failed to determine the health of the cluster running at " + url); + terminal.errorPrintln("Unexpected response code [" + httpResponse.getHttpStatus() + "] from calling GET " + + route.toString()); final String cause = getErrorCause(httpResponse); if (cause != null) { - terminal.println("Cause: " + cause); + terminal.errorPrintln("Cause: " + cause); } } else { final String clusterStatus = Objects.toString(httpResponse.getResponseBody().get("status"), ""); if (clusterStatus.isEmpty()) { - terminal.println(""); - terminal.println("Failed to determine the health of the cluster running at " + url); - terminal.println("Could not find a 'status' value at " + route.toString()); + terminal.errorPrintln(""); + terminal.errorPrintln("Failed to determine the health of the cluster running at " + url); + terminal.errorPrintln("Could not find a 'status' value at " + route.toString()); } else if ("red".equalsIgnoreCase(clusterStatus)) { - terminal.println(""); - terminal.println("Your cluster health is currently RED."); - terminal.println("This means that some cluster data is unavailable and your cluster is not fully functional."); + terminal.errorPrintln(""); + terminal.errorPrintln("Your cluster health is currently RED."); + terminal.errorPrintln("This means that some cluster data is unavailable and your cluster is not fully functional."); } else { // Cluster is yellow/green -> all OK return; } } - terminal.println(""); - terminal.println( + terminal.errorPrintln(""); + terminal.errorPrintln( "It is recommended that you resolve the issues with your cluster before running elasticsearch-setup-passwords."); - terminal.println("It is very likely that the password changes will fail when run against an unhealthy cluster."); - terminal.println(""); + terminal.errorPrintln("It is very likely that the password changes will fail when run against an unhealthy cluster."); + terminal.errorPrintln(""); if (shouldPrompt) { final boolean keepGoing = terminal.promptYesNo("Do you want to continue with the password setup process", false); if (keepGoing == false) { @@ -465,28 +469,28 @@ public class SetupPasswordTool extends LoggingAwareMultiCommand { } }, is -> responseBuilder(is, terminal)); if (httpResponse.getHttpStatus() != HttpURLConnection.HTTP_OK) { - terminal.println(""); - terminal.println( + terminal.errorPrintln(""); + terminal.errorPrintln( "Unexpected response code [" + httpResponse.getHttpStatus() + "] from calling PUT " + route.toString()); String cause = getErrorCause(httpResponse); if (cause != null) { - terminal.println("Cause: " + cause); - terminal.println(""); + terminal.errorPrintln("Cause: " + cause); + terminal.errorPrintln(""); } - terminal.println("Possible next steps:"); - terminal.println("* Try running this tool again."); - terminal.println("* Try running with the --verbose parameter for additional messages."); - terminal.println("* Check the elasticsearch logs for additional error details."); - terminal.println("* Use the change password API manually. "); - terminal.println(""); + terminal.errorPrintln("Possible next steps:"); + terminal.errorPrintln("* Try running this tool again."); + terminal.errorPrintln("* Try running with the --verbose parameter for additional messages."); + terminal.errorPrintln("* Check the elasticsearch logs for additional error details."); + terminal.errorPrintln("* Use the change password API manually. "); + terminal.errorPrintln(""); throw new UserException(ExitCodes.TEMP_FAILURE, "Failed to set password for user [" + user + "]."); } } catch (IOException e) { - terminal.println(""); - terminal.println("Connection failure to: " + route.toString() + " failed: " + e.getMessage()); - terminal.println(Verbosity.VERBOSE, ""); - terminal.println(Verbosity.VERBOSE, ExceptionsHelper.stackTrace(e)); - terminal.println(""); + terminal.errorPrintln(""); + terminal.errorPrintln("Connection failure to: " + route.toString() + " failed: " + e.getMessage()); + terminal.errorPrintln(Verbosity.VERBOSE, ""); + terminal.errorPrintln(Verbosity.VERBOSE, ExceptionsHelper.stackTrace(e)); + terminal.errorPrintln(""); throw new UserException(ExitCodes.TEMP_FAILURE, "Failed to set password for user [" + user + "].", e); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/tool/UsersTool.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/tool/UsersTool.java index 6d51fc5df93..03bed43499d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/tool/UsersTool.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/tool/UsersTool.java @@ -475,11 +475,11 @@ public class UsersTool extends LoggingAwareMultiCommand { Set knownRoles = Sets.union(FileRolesStore.parseFileForRoleNames(rolesFile, null), ReservedRolesStore.names()); Set unknownRoles = Sets.difference(Sets.newHashSet(roles), knownRoles); if (!unknownRoles.isEmpty()) { - terminal.println(String.format(Locale.ROOT, "Warning: The following roles [%s] are not in the [%s] file. Make sure the names " + - "are correct. If the names are correct and the roles were created using the API please disregard this message. " + - "Nonetheless the user will still be associated with all specified roles", + terminal.errorPrintln(String.format(Locale.ROOT, "Warning: The following roles [%s] are not in the [%s] file. " + + "Make sure the names are correct. If the names are correct and the roles were created using the API please " + + "disregard this message. Nonetheless the user will still be associated with all specified roles", Strings.collectionToCommaDelimitedString(unknownRoles), rolesFile.toAbsolutePath())); - terminal.println("Known roles: " + knownRoles.toString()); + terminal.errorPrintln("Known roles: " + knownRoles.toString()); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommand.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommand.java index a60b2204095..68be01a2e3f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommand.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommand.java @@ -224,7 +224,7 @@ public class SamlMetadataCommand extends EnvironmentAwareCommand { if (ContactInfo.TYPES.containsKey(type)) { break; } else { - terminal.println("Type '" + type + "' is not valid. Valid values are " + terminal.errorPrintln("Type '" + type + "' is not valid. Valid values are " + Strings.collectionToCommaDelimitedString(ContactInfo.TYPES.keySet())); } } @@ -263,8 +263,8 @@ public class SamlMetadataCommand extends EnvironmentAwareCommand { } else { errorMessage = "Error building signing credentials from provided keyPair"; } - terminal.println(Terminal.Verbosity.SILENT, errorMessage); - terminal.println("The following errors were found:"); + terminal.errorPrintln(Terminal.Verbosity.SILENT, errorMessage); + terminal.errorPrintln("The following errors were found:"); printExceptions(terminal, e); throw new UserException(ExitCodes.CANT_CREATE, "Unable to create metadata document"); } @@ -351,15 +351,16 @@ public class SamlMetadataCommand extends EnvironmentAwareCommand { SamlUtils.validate(xmlInput, METADATA_SCHEMA); terminal.println(Terminal.Verbosity.VERBOSE, "The generated metadata file conforms to the SAML metadata schema"); } catch (SAXException e) { - terminal.println(Terminal.Verbosity.SILENT, "Error - The generated metadata file does not conform to the SAML metadata schema"); - terminal.println("While validating " + xml.toString() + " the follow errors were found:"); + terminal.errorPrintln(Terminal.Verbosity.SILENT, "Error - The generated metadata file does not conform to the " + + "SAML metadata schema"); + terminal.errorPrintln("While validating " + xml.toString() + " the follow errors were found:"); printExceptions(terminal, e); throw new UserException(ExitCodes.CODE_ERROR, "Generated metadata is not valid"); } } private void printExceptions(Terminal terminal, Throwable throwable) { - terminal.println(" - " + throwable.getMessage()); + terminal.errorPrintln(" - " + throwable.getMessage()); for (Throwable sup : throwable.getSuppressed()) { printExceptions(terminal, sup); } @@ -453,10 +454,10 @@ public class SamlMetadataCommand extends EnvironmentAwareCommand { throw new UserException(ExitCodes.CONFIG, "There is no SAML realm configured in " + env.configFile()); } if (saml.size() > 1) { - terminal.println("Using configuration in " + env.configFile()); - terminal.println("Found multiple SAML realms: " + terminal.errorPrintln("Using configuration in " + env.configFile()); + terminal.errorPrintln("Found multiple SAML realms: " + saml.stream().map(Map.Entry::getKey).map(Object::toString).collect(Collectors.joining(", "))); - terminal.println("Use the -" + optionName(realmSpec) + " option to specify an explicit realm"); + terminal.errorPrintln("Use the -" + optionName(realmSpec) + " option to specify an explicit realm"); throw new UserException(ExitCodes.CONFIG, "Found multiple SAML realms, please specify one with '-" + optionName(realmSpec) + "'"); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/FileAttributesChecker.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/FileAttributesChecker.java index 6ef8461db82..5dd7c2b9cfb 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/FileAttributesChecker.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/FileAttributesChecker.java @@ -51,19 +51,19 @@ public class FileAttributesChecker { PosixFileAttributes newAttributes = view.readAttributes(); PosixFileAttributes oldAttributes = attributes[i]; if (oldAttributes.permissions().equals(newAttributes.permissions()) == false) { - terminal.println(Terminal.Verbosity.SILENT, "WARNING: The file permissions of [" + paths[i] + "] have changed " + terminal.errorPrintln(Terminal.Verbosity.SILENT, "WARNING: The file permissions of [" + paths[i] + "] have changed " + "from [" + PosixFilePermissions.toString(oldAttributes.permissions()) + "] " + "to [" + PosixFilePermissions.toString(newAttributes.permissions()) + "]"); - terminal.println(Terminal.Verbosity.SILENT, + terminal.errorPrintln(Terminal.Verbosity.SILENT, "Please ensure that the user account running Elasticsearch has read access to this file!"); } if (oldAttributes.owner().getName().equals(newAttributes.owner().getName()) == false) { - terminal.println(Terminal.Verbosity.SILENT, "WARNING: Owner of file [" + paths[i] + "] " + terminal.errorPrintln(Terminal.Verbosity.SILENT, "WARNING: Owner of file [" + paths[i] + "] " + "used to be [" + oldAttributes.owner().getName() + "], " + "but now is [" + newAttributes.owner().getName() + "]"); } if (oldAttributes.group().getName().equals(newAttributes.group().getName()) == false) { - terminal.println(Terminal.Verbosity.SILENT, "WARNING: Group of file [" + paths[i] + "] " + terminal.errorPrintln(Terminal.Verbosity.SILENT, "WARNING: Group of file [" + paths[i] + "] " + "used to be [" + oldAttributes.group().getName() + "], " + "but now is [" + newAttributes.group().getName() + "]"); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordToolTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordToolTests.java index e93950739a1..6e821720069 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordToolTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordToolTests.java @@ -350,7 +350,7 @@ public class SetupPasswordToolTests extends CommandTestCase { fail("Should have thrown exception"); } catch (UserException e) { assertEquals(ExitCodes.OK, e.exitCode); - assertThat(terminal.getOutput(), Matchers.containsString("Your cluster health is currently RED.")); + assertThat(terminal.getErrorOutput(), Matchers.containsString("Your cluster health is currently RED.")); } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommandTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommandTests.java index 367921ad763..734ea0be0d4 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommandTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommandTests.java @@ -165,9 +165,9 @@ public class SamlMetadataCommandTests extends SamlTestCase { final UserException userException = expectThrows(UserException.class, () -> command.buildEntityDescriptor(terminal, options, env)); assertThat(userException.getMessage(), containsString("multiple SAML realms")); - assertThat(terminal.getOutput(), containsString("saml_a")); - assertThat(terminal.getOutput(), containsString("saml_b")); - assertThat(terminal.getOutput(), containsString("Use the -realm option")); + assertThat(terminal.getErrorOutput(), containsString("saml_a")); + assertThat(terminal.getErrorOutput(), containsString("saml_b")); + assertThat(terminal.getErrorOutput(), containsString("Use the -realm option")); } public void testSpecifyRealmNameAsParameter() throws Exception { @@ -423,7 +423,7 @@ public class SamlMetadataCommandTests extends SamlTestCase { final UserException userException = expectThrows(UserException.class, () -> command.possiblySignDescriptor(terminal, options, descriptor, env)); assertThat(userException.getMessage(), containsString("Unable to create metadata document")); - assertThat(terminal.getOutput(), containsString("Error parsing Private Key from")); + assertThat(terminal.getErrorOutput(), containsString("Error parsing Private Key from")); } public void testSigningMetadataWithPem() throws Exception { diff --git a/x-pack/qa/security-tools-tests/src/test/java/org/elasticsearch/xpack/security/authc/file/tool/UsersToolTests.java b/x-pack/qa/security-tools-tests/src/test/java/org/elasticsearch/xpack/security/authc/file/tool/UsersToolTests.java index 09ef53052b1..47b0f4697dc 100644 --- a/x-pack/qa/security-tools-tests/src/test/java/org/elasticsearch/xpack/security/authc/file/tool/UsersToolTests.java +++ b/x-pack/qa/security-tools-tests/src/test/java/org/elasticsearch/xpack/security/authc/file/tool/UsersToolTests.java @@ -299,7 +299,7 @@ public class UsersToolTests extends CommandTestCase { public void testParseUnknownRole() throws Exception { UsersTool.parseRoles(terminal, TestEnvironment.newEnvironment(settings), "test_r1,r2,r3"); - String output = terminal.getOutput(); + String output = terminal.getErrorOutput(); assertTrue(output, output.contains("The following roles [r2,r3] are not in the [")); } diff --git a/x-pack/qa/security-tools-tests/src/test/java/org/elasticsearch/xpack/security/support/FileAttributesCheckerTests.java b/x-pack/qa/security-tools-tests/src/test/java/org/elasticsearch/xpack/security/support/FileAttributesCheckerTests.java index 12ae440e8f7..af2959410fd 100644 --- a/x-pack/qa/security-tools-tests/src/test/java/org/elasticsearch/xpack/security/support/FileAttributesCheckerTests.java +++ b/x-pack/qa/security-tools-tests/src/test/java/org/elasticsearch/xpack/security/support/FileAttributesCheckerTests.java @@ -28,6 +28,7 @@ public class FileAttributesCheckerTests extends ESTestCase { MockTerminal terminal = new MockTerminal(); checker.check(terminal); assertTrue(terminal.getOutput(), terminal.getOutput().isEmpty()); + assertTrue(terminal.getErrorOutput(), terminal.getErrorOutput().isEmpty()); } public void testNoPosix() throws Exception { @@ -38,6 +39,7 @@ public class FileAttributesCheckerTests extends ESTestCase { MockTerminal terminal = new MockTerminal(); checker.check(terminal); assertTrue(terminal.getOutput(), terminal.getOutput().isEmpty()); + assertTrue(terminal.getErrorOutput(), terminal.getErrorOutput().isEmpty()); } } @@ -51,6 +53,7 @@ public class FileAttributesCheckerTests extends ESTestCase { MockTerminal terminal = new MockTerminal(); checker.check(terminal); assertTrue(terminal.getOutput(), terminal.getOutput().isEmpty()); + assertTrue(terminal.getErrorOutput(), terminal.getErrorOutput().isEmpty()); } } @@ -71,7 +74,7 @@ public class FileAttributesCheckerTests extends ESTestCase { MockTerminal terminal = new MockTerminal(); checker.check(terminal); - String output = terminal.getOutput(); + String output = terminal.getErrorOutput(); assertTrue(output, output.contains("permissions of [" + path + "] have changed")); } } @@ -89,7 +92,7 @@ public class FileAttributesCheckerTests extends ESTestCase { MockTerminal terminal = new MockTerminal(); checker.check(terminal); - String output = terminal.getOutput(); + String output = terminal.getErrorOutput(); assertTrue(output, output.contains("Owner of file [" + path + "] used to be")); } } @@ -107,7 +110,7 @@ public class FileAttributesCheckerTests extends ESTestCase { MockTerminal terminal = new MockTerminal(); checker.check(terminal); - String output = terminal.getOutput(); + String output = terminal.getErrorOutput(); assertTrue(output, output.contains("Group of file [" + path + "] used to be")); } } diff --git a/x-pack/snapshot-tool/src/test/java/org/elasticsearch/snapshots/S3CleanupTests.java b/x-pack/snapshot-tool/src/test/java/org/elasticsearch/snapshots/S3CleanupTests.java index 3358eedf4c8..5a80103f0b5 100644 --- a/x-pack/snapshot-tool/src/test/java/org/elasticsearch/snapshots/S3CleanupTests.java +++ b/x-pack/snapshot-tool/src/test/java/org/elasticsearch/snapshots/S3CleanupTests.java @@ -155,6 +155,7 @@ public class S3CleanupTests extends ESSingleNodeTestCase { } } finally { logger.info("Cleanup command output:\n" + terminal.getOutput()); + logger.info("Cleanup command standard error:\n" + terminal.getErrorOutput()); } return terminal;