Ensure logging is configured for CLI commands

Any CLI commands that depend on core Elasticsearch might touch classes
(directly or indirectly) that depends on logging. If they do this and
logging is not configured, Log4j will dump status error messages to the
console. As such, we need to ensure that any such CLI command configures
logging (with a trivial configuration that dumps log messages to the
console). Previously we did this in the base CLI command but with the
refactoring of this class out of core Elasticsearch, we no longer
configure logging there (since we did not want this class to depend on
settings and logging). However, this meant for some CLI commands (like
the plugin CLI) we were no longer configuring logging. This commit adds
base classes between the low-level command and multi-command classes
that ensure that logging is configured. Any CLI command that depends on
core Elasticsearch should use this infrastructure to ensure logging is
configured. There is one exception to this: Elasticsearch itself because
it takes reponsibility into its own hands for configuring logging from
Elasticsearch settings and log4j2.properties. We preserve this special
status.

Relates elastic#27523

Author: jasontedor

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 be 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 {