Merge pull request #13933 from brwe/plugin-cli-logging

plugin cli tool should not create empty log files

Author: brwe

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

@@ -195,7 +195,7 @@ private void setupSecurity(Settings settings, Environment environment) throws Ex
     private static void setupLogging(Settings settings, Environment environment) {
         try {
             Class.forName("org.apache.log4j.Logger");
-            LogConfigurator.configure(settings);
+            LogConfigurator.configure(settings, true);
         } catch (ClassNotFoundException e) {
             // no log4j
         } catch (NoClassDefFoundError e) {

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

@@ -70,6 +70,7 @@ public class LogConfigurator {
             .put("socketHub", "org.apache.log4j.net.SocketHubAppender")
             .put("syslog", "org.apache.log4j.net.SyslogAppender")
             .put("telnet", "org.apache.log4j.net.TelnetAppender")
+            .put("terminal", "org.elasticsearch.common.logging.log4j.TerminalAppender")
                     // policies
             .put("timeBased", "org.apache.log4j.rolling.TimeBasedRollingPolicy")
             .put("sizeBased", "org.apache.log4j.rolling.SizeBasedTriggeringPolicy")
@@ -83,15 +84,24 @@ public class LogConfigurator {
             .put("xml", "org.apache.log4j.XMLLayout")
             .immutableMap();
 
-    public static void configure(Settings settings) {
+    /**
+     * Consolidates settings and converts them into actual log4j settings, then initializes loggers and appenders.
+     *
+     * @param settings      custom settings that should be applied
+     * @param resolveConfig controls whether the logging conf file should be read too or not.
+     */
+    public static void configure(Settings settings, boolean resolveConfig) {
         if (loaded) {
             return;
         }
         loaded = true;
         // TODO: this is partly a copy of InternalSettingsPreparer...we should pass in Environment and not do all this...
         Environment environment = new Environment(settings);
+
         Settings.Builder settingsBuilder = settingsBuilder();
-        resolveConfig(environment, settingsBuilder);
+        if (resolveConfig) {
+            resolveConfig(environment, settingsBuilder);
+        }
         settingsBuilder
                 .putProperties("elasticsearch.", System.getProperties())
                 .putProperties("es.", System.getProperties());

core/src/main/java/org/elasticsearch/common/logging/log4j/TerminalAppender.java

@@ -0,0 +1,44 @@
+/*
+ * 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.common.logging.log4j;
+
+import org.apache.log4j.AppenderSkeleton;
+import org.apache.log4j.spi.LoggingEvent;
+import org.elasticsearch.common.cli.Terminal;
+
+/**
+ * TerminalAppender logs event to Terminal.DEFAULT. It is used for example by the PluginManagerCliParser.
+ * */
+public class TerminalAppender extends AppenderSkeleton {
+    @Override
+    protected void append(LoggingEvent event) {
+        Terminal.DEFAULT.println(event.getRenderedMessage());
+    }
+
+    @Override
+    public void close() {
+    }
+
+    @Override
+    public boolean requiresLayout() {
+        return false;
+    }
+}

core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java

@@ -73,7 +73,7 @@ public static Settings prepareSettings(Settings input) {
      * and then replacing all property placeholders. If a {@link Terminal} is provided and configuration settings are loaded,
      * settings with a value of <code>${prompt.text}</code> or <code>${prompt.secret}</code> will result in a prompt for
      * the setting to the user.
-     * @param input The initial settings to use
+     * @param input The custom settings to use. These are not overwritten by settings in the configuration file.
      * @param terminal the Terminal to use for input/output
      * @return the {@link Settings} and {@link Environment} as a {@link Tuple}
      */

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

@@ -24,7 +24,6 @@
 import org.elasticsearch.common.cli.CliTool;
 import org.elasticsearch.common.cli.CliToolConfig;
 import org.elasticsearch.common.cli.Terminal;
-import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.logging.log4j.LogConfigurator;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
@@ -39,7 +38,6 @@
 
 import static org.elasticsearch.common.cli.CliToolConfig.Builder.cmd;
 import static org.elasticsearch.common.cli.CliToolConfig.Builder.option;
-import static org.elasticsearch.common.settings.Settings.EMPTY;
 
 public class PluginManagerCliParser extends CliTool {
 
@@ -51,8 +49,21 @@ public class PluginManagerCliParser extends CliTool {
             .build();
 
     public static void main(String[] args) {
-        Environment env = InternalSettingsPreparer.prepareEnvironment(EMPTY, Terminal.DEFAULT);
-        LogConfigurator.configure(env.settings());
+        // initialize default for es.logger.level because we will not read the logging.yml
+        String loggerLevel = System.getProperty("es.logger.level", "INFO");
+        // 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 env = InternalSettingsPreparer.prepareEnvironment(Settings.builder()
+                .put("appender.terminal.type", "terminal")
+                .put("rootLogger", "${es.logger.level}, terminal")
+                .put("es.logger.level", loggerLevel)
+                .build(), Terminal.DEFAULT);
+        // configure but do not read the logging conf file
+        LogConfigurator.configure(env.settings(), false);
         int status = new PluginManagerCliParser().execute(args).status();
         System.exit(status);
     }

core/src/test/java/org/elasticsearch/common/logging/log4j/Log4jESLoggerTests.java

@@ -57,7 +57,7 @@ public void setUp() throws Exception {
                 .put("path.conf", configDir.toAbsolutePath())
                 .put("path.home", createTempDir().toString())
                 .build();
-        LogConfigurator.configure(settings);
+        LogConfigurator.configure(settings, true);
 
         esTestLogger = Log4jESLoggerFactory.getLogger("test");
         Logger testLogger = ((Log4jESLogger) esTestLogger).logger();

core/src/test/java/org/elasticsearch/common/logging/log4j/LoggingConfigurationTests.java

@@ -32,8 +32,10 @@
 
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
+import java.nio.file.OpenOption;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
+import java.util.Arrays;
 
 import static org.hamcrest.Matchers.*;
 
@@ -56,7 +58,7 @@ public void testResolveMultipleConfigs() throws Exception {
                     .put("path.conf", configDir.toAbsolutePath())
                     .put("path.home", createTempDir().toString())
                     .build();
-            LogConfigurator.configure(settings);
+            LogConfigurator.configure(settings, true);
 
             ESLogger esLogger = Log4jESLoggerFactory.getLogger("test");
             Logger logger = ((Log4jESLogger) esLogger).logger();
@@ -157,20 +159,20 @@ public void testResolveConfigInvalidFilename() throws Exception {
     public void testResolveOrder() throws Exception {
         Path tmpDir = createTempDir();
         Path loggingConf = tmpDir.resolve(loggingConfiguration("yaml"));
-        Files.write(loggingConf, "logger.test: INFO, file\n".getBytes(StandardCharsets.UTF_8));
+        Files.write(loggingConf, "logger.test_resolve_order: INFO, file\n".getBytes(StandardCharsets.UTF_8));
         Files.write(loggingConf, "appender.file.type: file\n".getBytes(StandardCharsets.UTF_8), StandardOpenOption.APPEND);
         Environment environment = InternalSettingsPreparer.prepareEnvironment(
                 Settings.builder()
                         .put("path.conf", tmpDir.toAbsolutePath())
                         .put("path.home", createTempDir().toString())
-                        .put("logger.test", "TRACE, console")
+                        .put("logger.test_resolve_order", "TRACE, console")
                         .put("appender.console.type", "console")
                         .put("appender.console.layout.type", "consolePattern")
                         .put("appender.console.layout.conversionPattern", "[%d{ISO8601}][%-5p][%-25c] %m%n")
                         .build(), new CliToolTestCase.MockTerminal());
-        LogConfigurator.configure(environment.settings());
+        LogConfigurator.configure(environment.settings(), true);
         // args should overwrite whatever is in the config
-        ESLogger esLogger = Log4jESLoggerFactory.getLogger("test");
+        ESLogger esLogger = Log4jESLoggerFactory.getLogger("test_resolve_order");
         Logger logger = ((Log4jESLogger) esLogger).logger();
         Appender appender = logger.getAppender("console");
         assertThat(appender, notNullValue());
@@ -179,6 +181,31 @@ public void testResolveOrder() throws Exception {
         assertThat(appender, nullValue());
     }
 
+    // tests that config file is not read when we call LogConfigurator.configure(Settings, false)
+    @Test
+    public void testConfigNotRead() throws Exception {
+        Path tmpDir = createTempDir();
+        Path loggingConf = tmpDir.resolve(loggingConfiguration("yaml"));
+        Files.write(loggingConf,
+                Arrays.asList(
+                        "logger.test_config_not_read: INFO, console",
+                        "appender.console.type: console"),
+                StandardCharsets.UTF_8);
+        Environment environment = InternalSettingsPreparer.prepareEnvironment(
+                Settings.builder()
+                        .put("path.conf", tmpDir.toAbsolutePath())
+                        .put("path.home", createTempDir().toString())
+                        .build(), new CliToolTestCase.MockTerminal());
+        LogConfigurator.configure(environment.settings(), false);
+        ESLogger esLogger = Log4jESLoggerFactory.getLogger("test_config_not_read");
+
+        assertNotNull(esLogger);
+        Logger logger = ((Log4jESLogger) esLogger).logger();
+        Appender appender = logger.getAppender("console");
+        // config was not read
+        assertNull(appender);
+    }
+
     private static String loggingConfiguration(String suffix) {
         return "logging." + randomAsciiOfLength(randomIntBetween(0, 10)) + "." + suffix;
     }

qa/vagrant/src/test/resources/packaging/scripts/plugin_test_cases.bash

@@ -352,3 +352,26 @@ fi
 @test "[$GROUP] stop elasticsearch" {
     stop_elasticsearch_service
 }
+
+@test "[$GROUP] install jvm-example with different logging modes and check output" {
+    local relativePath=${1:-$(readlink -m jvm-example-*.zip)}
+    sudo -E -u $ESPLUGIN_COMMAND_USER "$ESHOME/bin/plugin" install "file://$relativePath" > /tmp/plugin-cli-output
+    local loglines=$(cat /tmp/plugin-cli-output | wc -l)
+    [ "$loglines" = "6" ] || {
+        echo "Expected 6 lines but the output was:"
+        cat /tmp/plugin-cli-output
+        false
+    }
+    remove_jvm_example
+
+    local relativePath=${1:-$(readlink -m jvm-example-*.zip)}
+    sudo -E -u $ESPLUGIN_COMMAND_USER "$ESHOME/bin/plugin" install "file://$relativePath" -Des.logger.level=DEBUG > /tmp/plugin-cli-output
+    local loglines=$(cat /tmp/plugin-cli-output | wc -l)
+    [ "$loglines" -gt "6" ] || {
+        echo "Expected more than 6 lines but the output was:"
+        cat /tmp/plugin-cli-output
+        false
+    }
+    remove_jvm_example
+}
+

qa/vagrant/src/test/resources/packaging/scripts/plugins.bash

@@ -36,6 +36,8 @@ install_plugin() {
 
     assert_file_exist "$ESPLUGINS/$name"
     assert_file_exist "$ESPLUGINS/$name/plugin-descriptor.properties"
+    #check we did not accidentially create a log file as root as /usr/share/elasticsearch
+    assert_file_not_exist "/usr/share/elasticsearch/logs"
 
     # At some point installing or removing plugins caused elasticsearch's logs
     # to be owned by root. This is bad so we want to make sure it doesn't