Solr
  1. Solr
  2. SOLR-7825

Forbid usage of log4j and JUL logger in Solr

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 5.2.1
    • Fix Version/s: 5.3, 6.0
    • Component/s: Server
    • Labels:
      None

      Description

      There are a few classes that directly rely on log4j to be on the classpath instead and don't use the slf4j logging facade. This creates problems when trying to switch the logging implementation.

      1. org.apache.solr.core.ZkContainer

      https://github.com/apache/lucene-solr/blob/trunk/solr/core/src/java/org/apache/solr/core/ZkContainer.java#L218

      I don't know the impact of this change, but shouldn't this call `org.apache.solr.logging.MDCLoggingContext.clear()` ?

      2. org.apache.solr.handler.component.RangeFacetProcessor and org.apache.solr.handler.component.RangeFacetRequest

      should use slf4j instead of log4j

      I had a stab at it at

      https://github.com/oschrenk/lucene-solr/commit/025b4802caf0360c63a3554af82e9ed4c94ff5a3#diff-7d822e8ff8ff21d88437652bbc894739R28

      1. slf4j.patch
        4 kB
        Oliver Schrenk
      2. SOLR-7825.patch
        23 kB
        Shalin Shekhar Mangar
      3. SOLR-7825.patch
        23 kB
        Shalin Shekhar Mangar

        Activity

        Hide
        Shawn Heisey added a comment -

        We should certainly fix problems like this.

        All is not lost even without the fixes, though: If you're switching logging implementations, you should be adding the slf4j jar to intercept log4j calls and direct them through slf4j – log4j-over-slf4j-X.Y.Z.jar – and removing the actual log4j jar. Some of Solr's dependencies (zookeeper being a prime example) use log4j directly.

        Show
        Shawn Heisey added a comment - We should certainly fix problems like this. All is not lost even without the fixes, though: If you're switching logging implementations, you should be adding the slf4j jar to intercept log4j calls and direct them through slf4j – log4j-over-slf4j-X.Y.Z.jar – and removing the actual log4j jar. Some of Solr's dependencies (zookeeper being a prime example) use log4j directly.
        Hide
        Oliver Schrenk added a comment -

        The logging implementations of dependencies can't be changed. As you mentioned though, not all is lost and these calls can be intercepted. I felt though that at least core should strive for consistency in its logging usage. There are actually not so many usages throughout the project

        There are some instances in contrib

        contrib/map-reduce/src/java/org/apache/solr/hadoop/MapReduceIndexerTool.java
        contrib/map-reduce/src/java/org/apache/solr/hadoop/SolrRecordWriter.java
        contrib/map-reduce/src/java/org/apache/solr/hadoop/Utils.java
        

        or just used by contrib

        core/src/java/org/apache/solr/util/SolrLogLayout.java
        

        Then there are that are specific to log4j. Im not sure how they are used.

        core/src/java/org/apache/solr/logging/log4j/EventAppender.java
        core/src/java/org/apache/solr/logging/log4j/Log4jInfo.java:22
        core/src/java/org/apache/solr/logging/log4j/Log4jWatcher.java
        

        This is standalone

        core/src/java/org/apache/solr/util/SolrCLI.java
        

        Usages in test

        core/src/test/org/apache/solr/cloud/ConcurrentDeleteAndCreateCollectionTest.java
        core/src/test/org/apache/solr/handler/admin/LoggingHandlerTest.java
        core/src/test/org/apache/solr/handler/RequestLoggingTest.java
        

        I just thought I start because the one in ZkContainer was actually a nuisance to me (because I wasn't aware of log4j-over-slf4

        Show
        Oliver Schrenk added a comment - The logging implementations of dependencies can't be changed. As you mentioned though, not all is lost and these calls can be intercepted. I felt though that at least core should strive for consistency in its logging usage. There are actually not so many usages throughout the project There are some instances in contrib contrib/map-reduce/src/java/org/apache/solr/hadoop/MapReduceIndexerTool.java contrib/map-reduce/src/java/org/apache/solr/hadoop/SolrRecordWriter.java contrib/map-reduce/src/java/org/apache/solr/hadoop/Utils.java or just used by contrib core/src/java/org/apache/solr/util/SolrLogLayout.java Then there are that are specific to log4j. Im not sure how they are used. core/src/java/org/apache/solr/logging/log4j/EventAppender.java core/src/java/org/apache/solr/logging/log4j/Log4jInfo.java:22 core/src/java/org/apache/solr/logging/log4j/Log4jWatcher.java This is standalone core/src/java/org/apache/solr/util/SolrCLI.java Usages in test core/src/test/org/apache/solr/cloud/ConcurrentDeleteAndCreateCollectionTest.java core/src/test/org/apache/solr/handler/admin/LoggingHandlerTest.java core/src/test/org/apache/solr/handler/RequestLoggingTest.java I just thought I start because the one in ZkContainer was actually a nuisance to me (because I wasn't aware of log4j-over-slf4
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Oliver.

        This patch:

        1. Fixes almost all places except the classes which are specific to log4j or java.util.logging.
        2. Forbids usage of all log4j and java.util.logging classes across the entire Solr code base so that such mistakes do not occur again.
        3. The property configurator use inside MapReduceIndexerTool and Utils is refactored to a single method which is excluded from forbidden-apis check
        4. I don't know why ConcurrentDeleteAndCreateCollectionTest was trying to set logging levels directly but it doesn't seem useful and I removed it. Shai Erera - is this okay?
        5. The usage inside SolrCLI was only because it changed log levels for ZK and solrj to reduce noise. On Timothy Potter's suggestion, I set the log levels inside solr/server/scripts/cloud-scripts/log4j.properties instead of code. Also the default log level for org.apache.zookeeper is changed to WARN instead of ERROR. This is because if ZK is not running, the ZK client retries a few times and this activity is logged in WARN. If we set the log level to ERROR then the zkcli script just hangs for a while (more than a minute).
        Show
        Shalin Shekhar Mangar added a comment - Thanks Oliver. This patch: Fixes almost all places except the classes which are specific to log4j or java.util.logging. Forbids usage of all log4j and java.util.logging classes across the entire Solr code base so that such mistakes do not occur again. The property configurator use inside MapReduceIndexerTool and Utils is refactored to a single method which is excluded from forbidden-apis check I don't know why ConcurrentDeleteAndCreateCollectionTest was trying to set logging levels directly but it doesn't seem useful and I removed it. Shai Erera - is this okay? The usage inside SolrCLI was only because it changed log levels for ZK and solrj to reduce noise. On Timothy Potter 's suggestion, I set the log levels inside solr/server/scripts/cloud-scripts/log4j.properties instead of code. Also the default log level for org.apache.zookeeper is changed to WARN instead of ERROR. This is because if ZK is not running, the ZK client retries a few times and this activity is logged in WARN. If we set the log level to ERROR then the zkcli script just hangs for a while (more than a minute).
        Hide
        Shai Erera added a comment -

        Shalin Shekhar Mangar I think I left it there by mistake, probably when I debugged the issue I added this test for, and forgot to remove it . Thanks!

        Show
        Shai Erera added a comment - Shalin Shekhar Mangar I think I left it there by mistake, probably when I debugged the issue I added this test for, and forgot to remove it . Thanks!
        Hide
        Uwe Schindler added a comment - - edited

        Some comments:

        • MapReduceIndexertool still imports the log4j class (leftover), it also imports SuppressForbidden but does not use it -> cleanup
        • In the solr.txt sigatures file i would truncate with ** instead of *. ** is like Ant/Maven's fileset includes and spawn several packages. So org.apache.log4j.** would also match possible sub-packages.
        Show
        Uwe Schindler added a comment - - edited Some comments: MapReduceIndexertool still imports the log4j class (leftover), it also imports SuppressForbidden but does not use it -> cleanup In the solr.txt sigatures file i would truncate with ** instead of * . ** is like Ant/Maven's fileset includes and spawn several packages. So org.apache.log4j.** would also match possible sub-packages.
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks for reviewing, Uwe! This patch incorporates both of your suggestions.

        Show
        Shalin Shekhar Mangar added a comment - Thanks for reviewing, Uwe! This patch incorporates both of your suggestions.
        Hide
        ASF subversion and git services added a comment -

        Commit 1692624 from shalin@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1692624 ]

        SOLR-7825: Forbid all usages of log4j and java.util.logging classes in Solr

        Show
        ASF subversion and git services added a comment - Commit 1692624 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1692624 ] SOLR-7825 : Forbid all usages of log4j and java.util.logging classes in Solr
        Hide
        ASF subversion and git services added a comment -

        Commit 1692634 from shalin@apache.org in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1692634 ]

        SOLR-7825: Forbid all usages of log4j and java.util.logging classes in Solr

        Show
        ASF subversion and git services added a comment - Commit 1692634 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1692634 ] SOLR-7825 : Forbid all usages of log4j and java.util.logging classes in Solr
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Oliver for reporting the issue!

        Show
        Shalin Shekhar Mangar added a comment - Thanks Oliver for reporting the issue!
        Hide
        Shalin Shekhar Mangar added a comment -

        Bulk close for 5.3.0 release

        Show
        Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release

          People

          • Assignee:
            Shalin Shekhar Mangar
            Reporter:
            Oliver Schrenk
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development