Ensure logging is configured for CLI commands

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

@@ -38,6 +38,8 @@ public abstract class Command implements Closeable {
     /** A description of the command, used in the help output. */
     protected final String description;
 
+    private final Runnable beforeMain;
+
     /** The option parser for this command. */
     protected final OptionParser parser = new OptionParser();
 
@@ -46,8 +48,15 @@ public abstract class Command implements Closeable {
     private final OptionSpec<Void> verboseOption =
         parser.acceptsAll(Arrays.asList("v", "verbose"), "show verbose output").availableUnless(silentOption);
 
-    public Command(String description) {
+    /**
+     * Construct the command with the specified command description and runnable to execute before main is invoked.
+     *
+     * @param description the command description
+     * @param beforeMain the before-main runnable
+     */
+    public Command(final String description, final Runnable beforeMain) {
         this.description = description;
+        this.beforeMain = beforeMain;
     }
 
     private Thread shutdownHookThread;
@@ -75,7 +84,7 @@ public final int main(String[] args, Terminal terminal) throws Exception {
             Runtime.getRuntime().addShutdownHook(shutdownHookThread);
         }
 
-        beforeExecute();
+        beforeMain.run();
 
         try {
             mainWithoutErrorHandling(args, terminal);
@@ -93,12 +102,6 @@ public final int main(String[] args, Terminal terminal) throws Exception {
         return ExitCodes.OK;
     }
 
-    /**
-     * Setup method to be executed before parsing or execution of the command being run. Any exceptions thrown by the
-     * method will not be cleanly caught by the parser.
-     */
-    protected void beforeExecute() {}
-
     /**
      * Executes the command, but all errors are thrown.
      */

core/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java

@@ -35,8 +35,14 @@ public class MultiCommand extends Command {
 
     private final NonOptionArgumentSpec<String> arguments = parser.nonOptions("command");
 
-    public MultiCommand(String description) {
-        super(description);
+    /**
+     * Construct the multi-command with the specified command description and runnable to execute before main is invoked.
+     *
+     * @param description the multi-command description
+     * @param beforeMain the before-main runnable
+     */
+    public MultiCommand(final String description, final Runnable beforeMain) {
+        super(description, beforeMain);
         parser.posixlyCorrect(true);
     }
 

core/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java

@@ -51,7 +51,7 @@ class Elasticsearch extends EnvironmentAwareCommand {
 
     // visible for testing
     Elasticsearch() {
-        super("starts elasticsearch");
+        super("starts elasticsearch", () -> {}); // we configure logging later so we override the base class from configuring logging
         versionOption = parser.acceptsAll(Arrays.asList("V", "version"),
             "Prints elasticsearch version information and exits");
         daemonizeOption = parser.acceptsAll(Arrays.asList("d", "daemonize"),
@@ -92,15 +92,6 @@ static int main(final String[] args, final Elasticsearch elasticsearch, final Te
         return elasticsearch.main(args, terminal);
     }
 
-    @Override
-    protected boolean shouldConfigureLoggingWithoutConfig() {
-        /*
-         * If we allow logging to be configured without a config before we are ready to read the log4j2.properties file, then we will fail
-         * to detect uses of logging before it is properly configured.
-         */
-        return false;
-    }
-
     @Override
     protected void execute(Terminal terminal, OptionSet options, Environment env) throws UserException {
         if (options.nonOptionArguments().isEmpty() == false) {

core/src/main/java/org/elasticsearch/cli/CommandLoggingConfigurator.java

@@ -0,0 +1,43 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.cli;
+
+import org.apache.logging.log4j.Level;
+import org.elasticsearch.common.logging.LogConfigurator;
+import org.elasticsearch.common.settings.Settings;
+
+/**
+ * Holder class for method to configure logging without Elasticsearch configuration files for use in CLI tools that will not read such
+ * files.
+ */
+final class CommandLoggingConfigurator {
+
+    /**
+     * Configures logging without Elasticsearch configuration files based on the system property "es.logger.level" only. As such, any
+     * logging will written to the console.
+     */
+    static void configureLoggingWithoutConfig() {
+        // initialize default for es.logger.level because we will not read the log4j2.properties
+        final String loggerLevel = System.getProperty("es.logger.level", Level.INFO.name());
+        final Settings settings = Settings.builder().put("logger.level", loggerLevel).build();
+        LogConfigurator.configureWithoutConfig(settings);
+    }
+
+}

core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java

@@ -22,9 +22,7 @@
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
 import joptsimple.util.KeyValuePair;
-import org.apache.logging.log4j.Level;
 import org.elasticsearch.common.SuppressForbidden;
-import org.elasticsearch.common.logging.LogConfigurator;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.node.InternalSettingsPreparer;
@@ -40,8 +38,25 @@ public abstract class EnvironmentAwareCommand extends Command {
 
     private final OptionSpec<KeyValuePair> settingOption;
 
-    public EnvironmentAwareCommand(String description) {
-        super(description);
+    /**
+     * Construct the command with the specified command description. This command will have logging configured without reading Elasticsearch
+     * configuration files.
+     *
+     * @param description the command description
+     */
+    public EnvironmentAwareCommand(final String description) {
+        this(description, CommandLoggingConfigurator::configureLoggingWithoutConfig);
+    }
+
+    /**
+     * Construct the command with the specified command description and runnable to execute before main is invoked. Commands constructed
+     * with this constructor must take ownership of configuring logging.
+     *
+     * @param description the command description
+     * @param beforeMain the before-main runnable
+     */
+    public EnvironmentAwareCommand(final String description, final Runnable beforeMain) {
+        super(description, beforeMain);
         this.settingOption = parser.accepts("E", "Configure a setting").withRequiredArg().ofType(KeyValuePair.class);
     }
 
@@ -104,26 +119,6 @@ private static void putSystemPropertyIfSettingIsMissing(final Map<String, String
         }
     }
 
-    @Override
-    protected final void beforeExecute() {
-        if (shouldConfigureLoggingWithoutConfig()) {
-            // initialize default for es.logger.level because we will not read the log4j2.properties
-            final String loggerLevel = System.getProperty("es.logger.level", Level.INFO.name());
-            final Settings settings = Settings.builder().put("logger.level", loggerLevel).build();
-            LogConfigurator.configureWithoutConfig(settings);
-        }
-    }
-
-    /**
-     * Indicate whether or not logging should be configured without reading a log4j2.properties. Most commands should do this because we do
-     * not configure logging for CLI tools. Only commands that configure logging on their own should not do this.
-     *
-     * @return true if logging should be configured without reading a log4j2.properties file
-     */
-    protected boolean shouldConfigureLoggingWithoutConfig() {
-        return true;
-    }
-
     /** Execute the command with the initialized {@link Environment}. */
     protected abstract void execute(Terminal terminal, OptionSet options, Environment env) throws Exception;
 

core/src/main/java/org/elasticsearch/cli/LoggingAwareCommand.java

@@ -0,0 +1,38 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.cli;
+
+/**
+ * A command that is aware of logging. This class should be preferred over the base {@link Command} class for any CLI tools that depend on
+ * core Elasticsearch as they could directly or indirectly touch classes that touch logging and as such logging needs to be configured.
+ */
+public abstract class LoggingAwareCommand extends Command {
+
+    /**
+     * Construct the command with the specified command description. This command will have logging configured without reading Elasticsearch
+     * configuration files.
+     *
+     * @param description the command description
+     */
+    public LoggingAwareCommand(final String description) {
+        super(description, CommandLoggingConfigurator::configureLoggingWithoutConfig);
+    }
+
+}

core/src/main/java/org/elasticsearch/cli/LoggingAwareMultiCommand.java

@@ -0,0 +1,39 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.cli;
+
+/**
+ * A multi-command that is aware of logging. This class should be preferred over the base {@link MultiCommand} class for any CLI tools that
+ * depend on core Elasticsearch as they could directly or indirectly touch classes that touch logging and as such logging needs to be
+ * configured.
+ */
+public abstract class LoggingAwareMultiCommand extends MultiCommand {
+
+    /**
+     * Construct the command with the specified command description. This command will have logging configured without reading Elasticsearch
+     * configuration files.
+     *
+     * @param description the command description
+     */
+    public LoggingAwareMultiCommand(final String description) {
+        super(description, CommandLoggingConfigurator::configureLoggingWithoutConfig);
+    }
+
+}

core/src/main/java/org/elasticsearch/common/settings/KeyStoreCli.java

@@ -19,13 +19,14 @@
 
 package org.elasticsearch.common.settings;
 
+import org.elasticsearch.cli.LoggingAwareMultiCommand;
 import org.elasticsearch.cli.MultiCommand;
 import org.elasticsearch.cli.Terminal;
 
 /**
  * A cli tool for managing secrets in the elasticsearch keystore.
  */
-public class KeyStoreCli extends MultiCommand {
+public class KeyStoreCli extends LoggingAwareMultiCommand {
 
     private KeyStoreCli() {
         super("A tool for managing settings stored in the elasticsearch keystore");
@@ -39,4 +40,5 @@ private KeyStoreCli() {
     public static void main(String[] args) throws Exception {
         exit(new KeyStoreCli().main(args, Terminal.DEFAULT));
     }
+
 }

core/src/main/java/org/elasticsearch/index/translog/TranslogToolCli.java

@@ -19,13 +19,14 @@
 
 package org.elasticsearch.index.translog;
 
+import org.elasticsearch.cli.LoggingAwareMultiCommand;
 import org.elasticsearch.cli.MultiCommand;
 import org.elasticsearch.cli.Terminal;
 
 /**
  * Class encapsulating and dispatching commands from the {@code elasticsearch-translog} command line tool
  */
-public class TranslogToolCli extends MultiCommand {
+public class TranslogToolCli extends LoggingAwareMultiCommand {
 
     private TranslogToolCli() {
         super("A CLI tool for various Elasticsearch translog actions");

core/src/test/java/org/elasticsearch/cli/CommandTests.java

@@ -28,7 +28,7 @@ public class CommandTests extends ESTestCase {
     static class UserErrorCommand extends Command {
 
         UserErrorCommand() {
-            super("Throws a user error");
+            super("Throws a user error", () -> {});
         }
 
         @Override
@@ -46,7 +46,7 @@ protected boolean addShutdownHook() {
     static class UsageErrorCommand extends Command {
 
         UsageErrorCommand() {
-            super("Throws a usage error");
+            super("Throws a usage error", () -> {});
         }
 
         @Override
@@ -66,7 +66,7 @@ static class NoopCommand extends Command {
         boolean executed = false;
 
         NoopCommand() {
-            super("Does nothing");
+            super("Does nothing", () -> {});
         }
 
         @Override

core/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

@@ -26,13 +26,13 @@ public class MultiCommandTests extends CommandTestCase {
 
     static class DummyMultiCommand extends MultiCommand {
         DummyMultiCommand() {
-            super("A dummy multi command");
+            super("A dummy multi command", () -> {});
         }
     }
 
     static class DummySubCommand extends Command {
         DummySubCommand() {
-            super("A dummy subcommand");
+            super("A dummy subcommand", () -> {});
         }
         @Override
         protected void execute(Terminal terminal, OptionSet options) throws Exception {

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

@@ -21,6 +21,7 @@
 
 import org.apache.lucene.util.IOUtils;
 import org.elasticsearch.cli.Command;
+import org.elasticsearch.cli.LoggingAwareMultiCommand;
 import org.elasticsearch.cli.MultiCommand;
 import org.elasticsearch.cli.Terminal;
 
@@ -31,7 +32,7 @@
 /**
  * A cli tool for adding, removing and listing plugins for elasticsearch.
  */
-public class PluginCli extends MultiCommand {
+public class PluginCli extends LoggingAwareMultiCommand {
 
     private final Collection<Command> commands;
 

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

@@ -33,7 +33,7 @@ public class EvilCommandTests extends ESTestCase {
     public void testCommandShutdownHook() throws Exception {
         final AtomicBoolean closed = new AtomicBoolean();
         final boolean shouldThrow = randomBoolean();
-        final Command command = new Command("test-command-shutdown-hook") {
+        final Command command = new Command("test-command-shutdown-hook", () -> {}) {
             @Override
             protected void execute(Terminal terminal, OptionSet options) throws Exception {