From 9492be65d4b27b1b45a69c6f2e7190fe1c7fcce7 Mon Sep 17 00:00:00 2001 From: Britta Weber Date: Tue, 6 Oct 2015 14:13:24 +0200 Subject: [PATCH] plugin cli tool should not create empty log files Plugin cli tools configures logging with whatever is in the logging.yml. If a file appender is configured for any of the logs this will cause creation of an empty log file. If a plugin was for example installed as root it will create empty logs at es.home/logs. This is problematic when for example plugins are installed as root and es is run as service. Logs will then be created in /usr/share/elasticsearch/logs and can later not be removed by for example dpkg -r or -purge. To avoid this, configure the logger to use an appender that writes to the same output that plugin cli tool does. This allows other components that are called from Plugin cli tool to write to the same terminal that plugin cli tool writes to by using the logging mechanism already in place. The logging conf is not read at all pb plugin cli tool. As a side effect, the loging level for components that are called from the plugin command such as the jar hell check can now be configured with -Des.logger.level which makes it easier to debug the jar hell check. --- .../elasticsearch/bootstrap/Bootstrap.java | 2 +- .../common/logging/log4j/LogConfigurator.java | 14 +++++- .../logging/log4j/TerminalAppender.java | 44 +++++++++++++++++++ .../internal/InternalSettingsPreparer.java | 2 +- .../plugins/PluginManagerCliParser.java | 19 ++++++-- .../logging/log4j/Log4jESLoggerTests.java | 2 +- .../log4j/LoggingConfigurationTests.java | 37 +++++++++++++--- .../packaging/scripts/plugin_test_cases.bash | 23 ++++++++++ .../resources/packaging/scripts/plugins.bash | 2 + 9 files changed, 131 insertions(+), 14 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/common/logging/log4j/TerminalAppender.java diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index 5b8c14aa3fb..542444b4097 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -195,7 +195,7 @@ final class Bootstrap { 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) { diff --git a/core/src/main/java/org/elasticsearch/common/logging/log4j/LogConfigurator.java b/core/src/main/java/org/elasticsearch/common/logging/log4j/LogConfigurator.java index 8f92864ea0f..a715d3434e3 100644 --- a/core/src/main/java/org/elasticsearch/common/logging/log4j/LogConfigurator.java +++ b/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()); diff --git a/core/src/main/java/org/elasticsearch/common/logging/log4j/TerminalAppender.java b/core/src/main/java/org/elasticsearch/common/logging/log4j/TerminalAppender.java new file mode 100644 index 00000000000..3c60c44d3e3 --- /dev/null +++ b/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; + } +} diff --git a/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java b/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java index bd15a6ae1b9..7aacde5283f 100644 --- a/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java +++ b/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java @@ -73,7 +73,7 @@ public class InternalSettingsPreparer { * and then replacing all property placeholders. If a {@link Terminal} is provided and configuration settings are loaded, * settings with a value of ${prompt.text} or ${prompt.secret} 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} */ diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginManagerCliParser.java b/core/src/main/java/org/elasticsearch/plugins/PluginManagerCliParser.java index db6afd17647..5d83fa2656f 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginManagerCliParser.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginManagerCliParser.java @@ -24,7 +24,6 @@ import org.elasticsearch.common.Strings; 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 java.util.Locale; 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); } diff --git a/core/src/test/java/org/elasticsearch/common/logging/log4j/Log4jESLoggerTests.java b/core/src/test/java/org/elasticsearch/common/logging/log4j/Log4jESLoggerTests.java index c1bdf00bd3c..d0cd3879dbc 100644 --- a/core/src/test/java/org/elasticsearch/common/logging/log4j/Log4jESLoggerTests.java +++ b/core/src/test/java/org/elasticsearch/common/logging/log4j/Log4jESLoggerTests.java @@ -57,7 +57,7 @@ public class Log4jESLoggerTests extends ESTestCase { .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(); diff --git a/core/src/test/java/org/elasticsearch/common/logging/log4j/LoggingConfigurationTests.java b/core/src/test/java/org/elasticsearch/common/logging/log4j/LoggingConfigurationTests.java index 199f94c3151..2b84bec93e9 100644 --- a/core/src/test/java/org/elasticsearch/common/logging/log4j/LoggingConfigurationTests.java +++ b/core/src/test/java/org/elasticsearch/common/logging/log4j/LoggingConfigurationTests.java @@ -32,8 +32,10 @@ import org.junit.Test; 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 class LoggingConfigurationTests extends ESTestCase { .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 class LoggingConfigurationTests extends ESTestCase { 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 class LoggingConfigurationTests extends ESTestCase { 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; } diff --git a/qa/vagrant/src/test/resources/packaging/scripts/plugin_test_cases.bash b/qa/vagrant/src/test/resources/packaging/scripts/plugin_test_cases.bash index ec2f177d3ca..a199cb989ab 100644 --- a/qa/vagrant/src/test/resources/packaging/scripts/plugin_test_cases.bash +++ b/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 +} + diff --git a/qa/vagrant/src/test/resources/packaging/scripts/plugins.bash b/qa/vagrant/src/test/resources/packaging/scripts/plugins.bash index 2af60c8a5bd..31e302c5e95 100644 --- a/qa/vagrant/src/test/resources/packaging/scripts/plugins.bash +++ b/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