Ensure logging is initialized in CLI tools

Today when CLI tools are executed, logging statements can intentionally
or unintentionally be executed when logging is not configured. This
leads to log messages that the status logger is not configured. This
commit reworks logging configuration for CLI tools so that logging is
always configured.

Relates #20575

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

@@ -246,7 +246,7 @@ final class Bootstrap {
 
         Environment environment = initialEnvironment(foreground, pidFile, esSettings);
         try {
-            LogConfigurator.configure(environment, true);
+            LogConfigurator.configure(environment);
         } catch (IOException e) {
             throw new BootstrapException(e);
         }

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

@@ -23,7 +23,10 @@ import joptsimple.OptionException;
 import joptsimple.OptionParser;
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
+import org.apache.logging.log4j.Level;
 import org.elasticsearch.common.SuppressForbidden;
+import org.elasticsearch.common.logging.LogConfigurator;
+import org.elasticsearch.common.settings.Settings;
 
 import java.io.IOException;
 import java.util.Arrays;
@@ -50,6 +53,11 @@ public abstract class Command {
 
     /** Parses options for this command from args and executes it. */
     public final int main(String[] args, Terminal terminal) throws Exception {
+        // 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);
+
         try {
             mainWithoutErrorHandling(args, terminal);
         } catch (OptionException e) {

core/src/main/java/org/elasticsearch/common/logging/LogConfigurator.java

@@ -48,45 +48,81 @@ import java.util.ArrayList;
 import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 
 public class LogConfigurator {
 
-    public static void configure(final Environment environment, final boolean resolveConfig) throws IOException, UserException {
+    /**
-        final Settings settings = environment.settings();
+     * Configure logging without reading a log4j2.properties file, effectively configuring the
-
+     * status logger and all loggers to the console.
-        setLogConfigurationSystemProperty(environment, settings);
+     *
-
+     * @param settings for configuring logger.level and individual loggers
+     */
+    public static void configureWithoutConfig(final Settings settings) {
+        Objects.requireNonNull(settings);
         // we initialize the status logger immediately otherwise Log4j will complain when we try to get the context
-        final ConfigurationBuilder<BuiltConfiguration> builder = ConfigurationBuilderFactory.newConfigurationBuilder();
+        configureStatusLogger();
-        builder.setStatusLevel(Level.ERROR);
+        configureLoggerLevels(settings);
-        Configurator.initialize(builder.build());
+    }
+
+    /**
+     * Configure logging reading from any log4j2.properties found in the config directory and its
+     * subdirectories from the specified environment. Will also configure logging to point the logs
+     * directory from the specified environment.
+     *
+     * @param environment the environment for reading configs and the logs path
+     * @throws IOException   if there is an issue readings any log4j2.properties in the config
+     *                       directory
+     * @throws UserException if there are no log4j2.properties in the specified configs path
+     */
+    public static void configure(final Environment environment) throws IOException, UserException {
+        Objects.requireNonNull(environment);
+        configure(environment.settings(), environment.configFile(), environment.logsFile());
+    }
+
+    private static void configure(final Settings settings, final Path configsPath, final Path logsPath) throws IOException, UserException {
+        Objects.requireNonNull(settings);
+        Objects.requireNonNull(configsPath);
+        Objects.requireNonNull(logsPath);
+
+        setLogConfigurationSystemProperty(logsPath, settings);
+        // we initialize the status logger immediately otherwise Log4j will complain when we try to get the context
+        configureStatusLogger();
 
         final LoggerContext context = (LoggerContext) LogManager.getContext(false);
 
-        if (resolveConfig) {
+        final List<AbstractConfiguration> configurations = new ArrayList<>();
-            final List<AbstractConfiguration> configurations = new ArrayList<>();
+        final PropertiesConfigurationFactory factory = new PropertiesConfigurationFactory();
-            final PropertiesConfigurationFactory factory = new PropertiesConfigurationFactory();
+        final Set<FileVisitOption> options = EnumSet.of(FileVisitOption.FOLLOW_LINKS);
-            final Set<FileVisitOption> options = EnumSet.of(FileVisitOption.FOLLOW_LINKS);
+        Files.walkFileTree(configsPath, options, Integer.MAX_VALUE, new SimpleFileVisitor<Path>() {
-            Files.walkFileTree(environment.configFile(), options, Integer.MAX_VALUE, new SimpleFileVisitor<Path>() {
+            @Override
-                @Override
+            public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
-                public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
+                if (file.getFileName().toString().equals("log4j2.properties")) {
-                    if (file.getFileName().toString().equals("log4j2.properties")) {
+                    configurations.add((PropertiesConfiguration) factory.getConfiguration(file.toString(), file.toUri()));
-                        configurations.add((PropertiesConfiguration) factory.getConfiguration(file.toString(), file.toUri()));
-                    }
-                    return FileVisitResult.CONTINUE;
                 }
-            });
+                return FileVisitResult.CONTINUE;
-
-            if (configurations.isEmpty()) {
-                throw new UserException(
-                    ExitCodes.CONFIG,
-                    "no log4j2.properties found; tried [" + environment.configFile() + "] and its subdirectories");
             }
+        });
 
-            context.start(new CompositeConfiguration(configurations));
+        if (configurations.isEmpty()) {
+            throw new UserException(
+                    ExitCodes.CONFIG,
+                    "no log4j2.properties found; tried [" + configsPath + "] and its subdirectories");
         }
 
+        context.start(new CompositeConfiguration(configurations));
+
+        configureLoggerLevels(settings);
+    }
+
+    private static void configureStatusLogger() {
+        final ConfigurationBuilder<BuiltConfiguration> builder = ConfigurationBuilderFactory.newConfigurationBuilder();
+        builder.setStatusLevel(Level.ERROR);
+        Configurator.initialize(builder.build());
+    }
+
+    private static void configureLoggerLevels(Settings settings) {
         if (ESLoggerFactory.LOG_DEFAULT_LEVEL_SETTING.exists(settings)) {
             final Level level = ESLoggerFactory.LOG_DEFAULT_LEVEL_SETTING.get(settings);
             Loggers.setLevel(ESLoggerFactory.getRootLogger(), level);
@@ -99,9 +135,10 @@ public class LogConfigurator {
         }
     }
 
+
     @SuppressForbidden(reason = "sets system property for logging configuration")
-    private static void setLogConfigurationSystemProperty(final Environment environment, final Settings settings) {
+    private static void setLogConfigurationSystemProperty(final Path logsPath, final Settings settings) {
-        System.setProperty("es.logs", environment.logsFile().resolve(ClusterName.CLUSTER_NAME_SETTING.get(settings).value()).toString());
+        System.setProperty("es.logs", logsPath.resolve(ClusterName.CLUSTER_NAME_SETTING.get(settings).value()).toString());
     }
 
 }

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

@@ -37,17 +37,7 @@ public class TranslogToolCli extends MultiCommand {
     }
 
     public static void main(String[] args) throws Exception {
-        // initialize default for es.logger.level because we will not read the log4j2.properties
-        String loggerLevel = System.getProperty("es.logger.level", "INFO");
-        String pathHome = System.getProperty("es.path.home");
-        // Set the appender for all potential log files to terminal so that other components that use the logger print out the
-        // same terminal.
-        Environment loggingEnvironment = InternalSettingsPreparer.prepareEnvironment(Settings.builder()
-                .put("path.home", pathHome)
-                .put("logger.level", loggerLevel)
-                .build(), Terminal.DEFAULT);
-        LogConfigurator.configure(loggingEnvironment, false);
-
         exit(new TranslogToolCli().main(args, Terminal.DEFAULT));
     }
+
 }

core/src/main/java/org/elasticsearch/plugins/PluginCli.java

@@ -39,21 +39,6 @@ public class PluginCli extends MultiCommand {
     }
 
     public static void main(String[] args) throws Exception {
-        // initialize default for es.logger.level because we will not read the log4j2.properties
-        String loggerLevel = System.getProperty("es.logger.level", "INFO");
-        String pathHome = System.getProperty("es.path.home");
-        // Set the appender for all potential log files to terminal so that other components that use the logger print out the
-        // same terminal.
-        // The reason for this is that the plugin cli cannot be configured with a file appender because when the plugin command is
-        // executed there is no way of knowing where the logfiles should be placed. For example, if elasticsearch
-        // is run as service then the logs should be at /var/log/elasticsearch but when started from the tar they should be at es.home/logs.
-        // Therefore we print to Terminal.
-        Environment loggingEnvironment = InternalSettingsPreparer.prepareEnvironment(Settings.builder()
-                .put("path.home", pathHome)
-                .put("logger.level", loggerLevel)
-                .build(), Terminal.DEFAULT);
-        LogConfigurator.configure(loggingEnvironment, false);
-
         exit(new PluginCli().main(args, Terminal.DEFAULT));
     }
 

qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerConfigurationTests.java

@@ -58,7 +58,7 @@ public class EvilLoggerConfigurationTests extends ESTestCase {
                 .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString())
                 .build();
             final Environment environment = new Environment(settings);
-            LogConfigurator.configure(environment, true);
+            LogConfigurator.configure(environment);
 
             {
                 final LoggerContext ctx = (LoggerContext) LogManager.getContext(false);
@@ -97,7 +97,7 @@ public class EvilLoggerConfigurationTests extends ESTestCase {
             .put("logger.level", level)
             .build();
         final Environment environment = new Environment(settings);
-        LogConfigurator.configure(environment, true);
+        LogConfigurator.configure(environment);
 
         final String loggerName = "test";
         final Logger logger = ESLoggerFactory.getLogger(loggerName);
@@ -113,7 +113,7 @@ public class EvilLoggerConfigurationTests extends ESTestCase {
             .put("logger.test_resolve_order", "TRACE")
             .build();
         final Environment environment = new Environment(settings);
-        LogConfigurator.configure(environment, true);
+        LogConfigurator.configure(environment);
 
         // args should overwrite whatever is in the config
         final String loggerName = "test_resolve_order";
@@ -128,7 +128,7 @@ public class EvilLoggerConfigurationTests extends ESTestCase {
                 .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString())
                 .build();
         final Environment environment = new Environment(settings);
-        LogConfigurator.configure(environment, true);
+        LogConfigurator.configure(environment);
 
         assertThat(ESLoggerFactory.getLogger("x").getLevel(), equalTo(Level.TRACE));
         assertThat(ESLoggerFactory.getLogger("x.y").getLevel(), equalTo(Level.DEBUG));
@@ -147,7 +147,7 @@ public class EvilLoggerConfigurationTests extends ESTestCase {
             .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString())
             .build();
         final Environment environment = new Environment(settings);
-        UserException e = expectThrows(UserException.class, () -> LogConfigurator.configure(environment, true));
+        UserException e = expectThrows(UserException.class, () -> LogConfigurator.configure(environment));
         assertThat(e, hasToString(containsString("no log4j2.properties found; tried")));
     }
 

qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java

@@ -188,7 +188,7 @@ public class EvilLoggerTests extends ESTestCase {
                 .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString())
                 .build();
         final Environment environment = new Environment(settings);
-        LogConfigurator.configure(environment, true);
+        LogConfigurator.configure(environment);
     }
 
     private void assertLogLine(final String logLine, final Level level, final String location, final String message) {