Remove deprecated code to add node name into log pattern of log4j pro…

…perty file (opensearch-project#4568) Signed-off-by: Tianli Feng <ftianli@amazon.com> Signed-off-by: Vishal Sarda <vsarda@amazon.com>
Vishalks · Sep 28, 2022 · 5ec4292 · 5ec4292
1 parent ab129f4
commit 5ec4292
Show file tree

Hide file tree

Showing 3 changed files with 2 additions and 84 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -48,6 +48,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 ### Deprecated
 
 ### Removed
+- Remove deprecated code to add node name into log pattern of log4j property file ([#4568](https://github.com/opensearch-project/OpenSearch/pull/4568))
 
 ### Fixed
 - `opensearch-service.bat start` and `opensearch-service.bat manager` failing to run ([#4289](https://github.com/opensearch-project/OpenSearch/pull/4289))

diff --git a/qa/evil-tests/src/test/java/org/opensearch/common/logging/EvilLoggerTests.java b/qa/evil-tests/src/test/java/org/opensearch/common/logging/EvilLoggerTests.java
@@ -285,29 +285,6 @@ public void testProperties() throws IOException, UserException {
         assertThat(System.getProperty("opensearch.logs.node_name"), equalTo(Node.NODE_NAME_SETTING.get(settings)));
     }
 
-    public void testNoNodeNameInPatternWarning() throws IOException, UserException {
-        String nodeName = randomAlphaOfLength(16);
-        LogConfigurator.setNodeName(nodeName);
-        setupLogging("no_node_name");
-        final String path =
-            System.getProperty("opensearch.logs.base_path") +
-                System.getProperty("file.separator") +
-                System.getProperty("opensearch.logs.cluster_name") + ".log";
-        final List<String> events = Files.readAllLines(PathUtils.get(path));
-        assertThat(events.size(), equalTo(2));
-        final String location = "org.opensearch.common.logging.LogConfigurator";
-        // the first message is a warning for unsupported configuration files
-        assertLogLine(events.get(0), Level.WARN, location, "\\[" + nodeName + "\\] Some logging configurations have "
-                + "%marker but don't have %node_name. We will automatically add %node_name to the pattern to ease the "
-                + "migration for users who customize log4j2.properties but will stop this behavior in 7.0. You should "
-                + "manually replace `%node_name` with `\\[%node_name\\]%marker ` in these locations:");
-        if (Constants.WINDOWS) {
-            assertThat(events.get(1), endsWith("no_node_name\\log4j2.properties"));
-        } else {
-            assertThat(events.get(1), endsWith("no_node_name/log4j2.properties"));
-        }
-    }
-
     private void setupLogging(final String config) throws IOException, UserException {
         setupLogging(config, Settings.EMPTY);
     }

diff --git a/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java b/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java
@@ -36,16 +36,13 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.core.LoggerContext;
 import org.apache.logging.log4j.core.config.AbstractConfiguration;
-import org.apache.logging.log4j.core.config.ConfigurationException;
-import org.apache.logging.log4j.core.config.ConfigurationSource;
 import org.apache.logging.log4j.core.config.Configurator;
 import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder;
 import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory;
 import org.apache.logging.log4j.core.config.builder.impl.BuiltConfiguration;
 import org.apache.logging.log4j.core.config.composite.CompositeConfiguration;
 import org.apache.logging.log4j.core.config.plugins.util.PluginManager;
 import org.apache.logging.log4j.core.config.properties.PropertiesConfiguration;
-import org.apache.logging.log4j.core.config.properties.PropertiesConfigurationBuilder;
 import org.apache.logging.log4j.core.config.properties.PropertiesConfigurationFactory;
 import org.apache.logging.log4j.status.StatusConsoleListener;
 import org.apache.logging.log4j.status.StatusData;
@@ -60,7 +57,6 @@
 import org.opensearch.node.Node;
 
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.PrintStream;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.FileVisitOption;
@@ -75,7 +71,6 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
-import java.util.Properties;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.stream.StreamSupport;
@@ -189,50 +184,7 @@ private static void configure(final Settings settings, final Path configsPath, f
 
         final Set<String> locationsWithDeprecatedPatterns = Collections.synchronizedSet(new HashSet<>());
         final List<AbstractConfiguration> configurations = new ArrayList<>();
-        /*
-         * Subclass the properties configurator to hack the new pattern in
-         * place so users don't have to change log4j2.properties in
-         * a minor release. In 7.0 we'll remove this and force users to
-         * change log4j2.properties. If they don't customize log4j2.properties
-         * then they won't have to do anything anyway.
-         *
-         * Everything in this subclass that isn't marked as a hack is copied
-         * from log4j2's source.
-         */
-        final PropertiesConfigurationFactory factory = new PropertiesConfigurationFactory() {
-            @Override
-            public PropertiesConfiguration getConfiguration(final LoggerContext loggerContext, final ConfigurationSource source) {
-                final Properties properties = new Properties();
-                try (InputStream configStream = source.getInputStream()) {
-                    properties.load(configStream);
-                } catch (final IOException ioe) {
-                    throw new ConfigurationException("Unable to load " + source.toString(), ioe);
-                }
-                // Hack the new pattern into place
-                for (String name : properties.stringPropertyNames()) {
-                    if (false == name.endsWith(".pattern")) continue;
-                    // Null is weird here but we can't do anything with it so ignore it
-                    String value = properties.getProperty(name);
-                    if (value == null) continue;
-                    // Tests don't need to be changed
-                    if (value.contains("%test_thread_info")) continue;
-                    /*
-                     * Patterns without a marker are sufficiently customized
-                     * that we don't have an opinion about them.
-                     */
-                    if (false == value.contains("%marker")) continue;
-                    if (false == value.contains("%node_name")) {
-                        locationsWithDeprecatedPatterns.add(source.getLocation());
-                        properties.setProperty(name, value.replace("%marker", "[%node_name]%marker "));
-                    }
-                }
-                // end hack
-                return new PropertiesConfigurationBuilder().setConfigurationSource(source)
-                    .setRootProperties(properties)
-                    .setLoggerContext(loggerContext)
-                    .build();
-            }
-        };
+        final PropertiesConfigurationFactory factory = new PropertiesConfigurationFactory();
         final Set<FileVisitOption> options = EnumSet.of(FileVisitOption.FOLLOW_LINKS);
         Files.walkFileTree(configsPath, options, Integer.MAX_VALUE, new SimpleFileVisitor<Path>() {
             @Override
@@ -252,18 +204,6 @@ public FileVisitResult visitFile(final Path file, final BasicFileAttributes attr
 
         configureLoggerLevels(settings);
 
-        final String deprecatedLocationsString = String.join("\n  ", locationsWithDeprecatedPatterns);
-        if (deprecatedLocationsString.length() > 0) {
-            LogManager.getLogger(LogConfigurator.class)
-                .warn(
-                    "Some logging configurations have %marker but don't have %node_name. "
-                        + "We will automatically add %node_name to the pattern to ease the migration for users who customize "
-                        + "log4j2.properties but will stop this behavior in 7.0. You should manually replace `%node_name` with "
-                        + "`[%node_name]%marker ` in these locations:\n  {}",
-                    deprecatedLocationsString
-                );
-        }
-
         // Redirect stdout/stderr to log4j. While we ensure Elasticsearch code does not write to those streams,
         // third party libraries may do that
         System.setOut(