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

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/ListPluginsCommand.java

@@ -66,7 +66,7 @@ private void printPlugin(Environment env, Terminal terminal, Path plugin, String
         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");
         }
     }

distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java

@@ -789,7 +789,7 @@ public void testInstallMisspelledOfficialPlugins() throws Exception {
     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 @@ private void assertPolicyConfirmation(Tuple<Path, Environment> env, String plugi
             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<Path> fileStream = Files.list(env.v2().pluginsFile())) {
                 assertThat(fileStream.collect(Collectors.toList()), empty());
             }
@@ -1238,7 +1238,7 @@ private void assertPolicyConfirmation(Tuple<Path, Environment> env, String plugi
             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<Path> fileStream = Files.list(env.v2().pluginsFile())) {
                 assertThat(fileStream.collect(Collectors.toList()), empty());
             }
@@ -1251,7 +1251,7 @@ private void assertPolicyConfirmation(Tuple<Path, Environment> env, String plugi
         }
         installPlugin(pluginZip, env.v1());
         for (String warning : warnings) {
-            assertThat(terminal.getOutput(), containsString("WARNING: " + warning));
+            assertThat(terminal.getErrorOutput(), containsString("WARNING: " + warning));
         }
     }
 

distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java

@@ -247,8 +247,11 @@ public void testExistingIncompatiblePlugin() throws Exception {
         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);

distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java

@@ -237,11 +237,14 @@ protected boolean addShutdownHook() {
                 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());
         }
     }
 

libs/cli/src/main/java/org/elasticsearch/cli/Command.java

@@ -73,7 +73,7 @@ public final int main(String[] args, Terminal terminal) throws Exception {
                         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 final int main(String[] args, Terminal terminal) throws Exception {
         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 @@ void mainWithoutErrorHandling(String[] args, Terminal terminal) throws Exception
         final OptionSet options = parser.parse(args);
 
         if (options.has(helpOption)) {
-            printHelp(terminal);
+            printHelp(terminal, false);
             return;
         }
 
@@ -125,11 +126,17 @@ void mainWithoutErrorHandling(String[] args, Terminal terminal) throws Exception
     }
 
     /** 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 */

libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java

@@ -39,9 +39,17 @@
 */
 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 void setVerbosity(Verbosity verbosity) {
     /** 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 final void println(Verbosity verbosity, String msg) {
         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 final boolean promptYesNo(String prompt, boolean defaultYes) {
             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 PrintWriter getWriter() {
 
         @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();

qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilElasticsearchCliTests.java

@@ -40,7 +40,7 @@ public void testPathHome() throws Exception {
         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 void testPathHome() throws Exception {
         runTest(
                 ExitCodes.OK,
                 true,
-                output -> {},
+                (output, error) -> {},
                 (foreground, pidFile, quiet, esSettings) -> {
                     Settings settings = esSettings.settings();
                     assertThat(settings.keySet(), hasSize(2));

qa/evil-tests/src/test/java/org/elasticsearch/cli/EvilCommandTests.java

@@ -55,7 +55,7 @@ public void close() throws IOException {
         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"));

qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java

@@ -377,7 +377,7 @@ public void test90SecurityCliPackaging() throws Exception {
                 // 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);

server/src/main/java/org/elasticsearch/plugins/PluginSecurity.java

@@ -55,15 +55,15 @@ static void confirmPolicyExceptions(Terminal terminal, Set<String> permissions,
             // 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);
         }
     }