[SPARK-6305][BUILD] Migrate from log4j1 to log4j2 #34895

viirya · 2021-12-14T09:27:28Z

What changes were proposed in this pull request?

This patch proposes to migrate from log4j1 to log4j2 in Spark.

Why are the changes needed?

Apache Spark uses log4j 1.x, so hence is not susceptible to the recent log4j v2 vulnerability. However, log4j 1.x has reached end of life and is no longer supported. Vulnerabilities reported after August 2015 against log4j 1.x were not checked and will not be fixed. As a result, we are recommended to upgrade Spark to Log4j 2 as soon as possible.

Does this PR introduce any user-facing change?

Yes. Users may need to rewrite their log4j properties file for log4j2. As of version 2.4, log4j now supports configuration via properties files. Note that the property syntax is not the same as the syntax used in log4j 1, but during the migration I found the syntax is pretty close so the migration should be straightforward.

How was this patch tested?

Passed all existing tests.

SparkQA · 2021-12-14T10:22:05Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50650/

SparkQA · 2021-12-14T10:25:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50651/

SparkQA · 2021-12-14T10:32:33Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50650/

laychengtan1401

log4j 2.12 still expose to vulnerability unless change to 2.15

SparkQA · 2021-12-14T11:18:32Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50651/

enigmatic00 · 2021-12-14T15:43:35Z

pom.xml

    <sbt.project.name>spark</sbt.project.name>
    <slf4j.version>1.7.30</slf4j.version>
-    <log4j.version>1.2.17</log4j.version>
+    <log4j.version>2.12.1</log4j.version>


bump to 2.16.0 please

SparkQA · 2021-12-14T18:21:23Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50665/

SparkQA · 2021-12-14T18:38:00Z

Test build #146178 has finished for PR 34895 at commit 0d7bd5d.

This patch fails from timeout after a configured wait of 500m.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-12-14T19:10:25Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50667/

SparkQA · 2021-12-14T19:29:43Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50665/

SparkQA · 2021-12-14T20:04:23Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50667/

SparkQA · 2021-12-18T03:18:08Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50820/

SparkQA · 2021-12-18T03:37:29Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50821/

SparkQA · 2021-12-18T04:08:39Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50820/

SparkQA · 2021-12-18T04:36:21Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50821/

dongjoon-hyun · 2021-12-18T04:36:43Z

catalyst & hive-thriftserer GitHub Action job passed!

SparkQA · 2021-12-18T05:32:22Z

Test build #146346 has finished for PR 34895 at commit 2aaed30.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

dongjoon-hyun

+1, LGTM. Merged to master for Apache Spark 3.3.

dongjoon-hyun · 2021-12-18T05:43:56Z

Thank you, @viirya , @dbtsai , @srowen , @laychengtan1401 , @ChethanGB , @FranciscoBorges , @enigmatic00 and all.

viirya · 2021-12-18T06:01:37Z

Thank you @dongjoon-hyun and all. The log4j1 properties files are useless now. I will open another PR to remove them.

SparkQA · 2021-12-18T06:21:39Z

Test build #146349 has finished for PR 34895 at commit 67e5bae.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class RocksDB implements KVStore
public static class TypeAliases
class RocksDBIterator<T> implements KVStoreIterator<T>
class RocksDBTypeInfo
class Index
(defaultdict(<class 'list'>,
class ExecutorRollPlugin extends SparkPlugin
class ExecutorRollDriverPlugin extends DriverPlugin with Logging
case class RegrCount(left: Expression, right: Expression)
case class MapContainsKey(
case class AesEncrypt(
case class AesDecrypt(

sunchao · 2021-12-18T16:41:06Z

Awesome job @viirya !

Looks like there is another CVE in 2.16: https://logging.apache.org/log4j/2.x/security.html. Should we upgrade to 2.17?

viirya · 2021-12-18T17:19:18Z

@sunchao yea, I will upgrade to 2.17.

LuciferYang · 2021-12-20T03:00:02Z

launcher/src/test/java/org/apache/spark/launcher/ChildProcAppHandleSuite.java

+  public static class LogAppender extends AbstractAppender {
+
+    protected LogAppender(String name,
+                          Filter filter,


Indent seems fine here

JoshRosen · 2021-12-20T22:55:26Z

core/src/main/scala/org/apache/spark/internal/Logging.scala

-    // If Log4j 1.2 is being used, but is not initialized, load a default properties file
-    if (Logging.isLog4j12()) {
-      val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+    if (!Logging.isLog4j12()) {


The code guarded by if(!Logging.isLog4j12()) only works with Log4J2 and not other SLF4J bindings (such as Logback). I think it might be a bit clearer and slightly more correct to replace this with a positive if(Logging.isLog4j2()) check which checks that the Log4J2 binding is in use (e.g. that the binder class is org.apache.logging.slf4j.Log4jLoggerFactory).

Similarly, I think log4j12Initialized should be renamed to log4j2Initialized (since we're not using Log4J 1.2 in the branch where that variable is defined).

Agreed. I will fix them.

JoshRosen · 2021-12-20T22:59:05Z

core/src/test/scala/org/apache/spark/internal/LoggingSuite.scala

-import org.apache.spark.SparkFunSuite
-import org.apache.spark.util.Utils
-
-class LoggingSuite extends SparkFunSuite {


Why did we need to delete this suite? I think this suite's removal means that we no longer have unit test coverage of SparkShellLoggingFilter?

ah, I think I removed it at the beginning of the attempt. I will add it back with other fixes regarding your other comments.

JoshRosen · 2021-12-20T23:09:20Z

core/src/main/scala/org/apache/spark/util/logging/DriverLogger.scala

+    val fa = Log4jFileAppender.createAppender(localLogFile, "false", "false",
+      DriverLogger.APPENDER_NAME, "true", "false", "false", "4000", layout, null,
+      "false", null, config);


My IDE says that this createAppender method is deprecated. Since the old 1. code was only setting the filename and layout, could this instead be

val fa = Log4jFileAppender.newBuilder().withFileName(localLogFile).setLayout(layout).build()

or do we need to set some of those other arguments?

I think we can rewrite it using builder.

JoshRosen · 2021-12-20T23:11:41Z

core/src/main/scala/org/apache/spark/util/logging/DriverLogger.scala

+      val logger = LogManager.getRootLogger().asInstanceOf[Logger]
+      val fa = logger.getAppenders.get(DriverLogger.APPENDER_NAME)
+      logger.removeAppender(fa)
+      fa.stop()


Should we omit this fa.stop() since we have a Utils.tryLogNonFatalError(fa.stop()) on the following line?

JoshRosen · 2021-12-20T23:21:55Z

core/src/main/scala/org/apache/spark/util/Utils.scala

+    val rootLogger = org.apache.logging.log4j.LogManager.getRootLogger()
+      .asInstanceOf[org.apache.logging.log4j.core.Logger]
    rootLogger.setLevel(l)
+    rootLogger.get().setLevel(l)


Do we know if this is the best way to programmatically change the root logger's level? I noticed that https://stackoverflow.com/questions/23434252/programmatically-change-log-level-in-log4j2 mentions several techniques which differ from the one used here.

I ask because I'm not very familiar with Log4J2 and I'm unsure if there's subtle pitfalls to avoid (e.g. will this work if we don't call updateLoggers()?).

If we wind up changing this code then we should also update similar code in SparkFunSuite.

Hmm, this is a good question. I think I feel the same regarding log4j2 API. This is the way I finally made it work. I will look if there is more proper way for it.

JoshRosen · 2021-12-20T23:24:43Z

core/src/test/scala/org/apache/spark/SparkFunSuite.scala

+            logger.setLevel(level.get)
+            logger.get().setLevel(level.get)
+          }
+        case _ =>


Since loggers is a Seq[Logger], is this branch ever reachable? Unless I'm mistaken, I think think we'll always hit the case logger: Logger case.

JoshRosen · 2021-12-20T23:26:03Z

core/src/main/scala/org/apache/spark/internal/Logging.scala

-      while (logger.getParent() != null) {
-        if (logger.getLevel != null || logger.getAllAppenders.hasMoreElements) {
-          return Filter.NEUTRAL
+  private class SparkShellLoggingFilter extends Filter {


I think we could potentially simplify this class by having it extend AbstractFilter instead of Filter:

From AbstractFilter's Javadoc:

/** * Users should extend this class to implement filters. Filters can be either context wide or attached to * an appender. A filter may choose to support being called only from the context or only from an appender in * which case it will only implement the required method(s). The rest will default to return {@link org.apache.logging.log4j.core.Filter.Result#NEUTRAL}. * <p> * Garbage-free note: the methods with unrolled varargs by default delegate to the * {@link #filter(Logger, Level, Marker, String, Object...) filter method with vararg parameters}. * Subclasses that want to be garbage-free should override these methods to implement the appropriate filtering * without creating a vararg array. * </p> */ public abstract class AbstractFilter extends AbstractLifeCycle implements Filter {

Okay. I will try to rewrite this with AbstractFilter. I think the functionality should be the same but yea it looks better to use AbstractFilter here.

xkrogen · 2021-12-20T17:53:24Z

pom.xml

+      <!--
      <dependency>
        <groupId>org.slf4j</groupId>
        <artifactId>slf4j-api</artifactId>


We should remove this commented-out block since it is no longer being used?

Yea, they were removed in a followup later.

### What changes were proposed in this pull request? Change log level of tests to WARN ### Why are the changes needed? After #34895, the log level of tests becomes either "INFO" or "DEBUG", this increases the log output file to over 100MB! E.g https://github.com/gengliangwang/spark/runs/4623214509?check_suite_focus=true This makes it difficult to find the CI test failures. Changing the log level of tests to WARN should help. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? It's just conf change. Closes #35011 from gengliangwang/loglevel. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

steveloughran · 2021-12-30T13:44:01Z

Pretty bold timing. I would have expected private builds to simply ship with a patched version of log4j 1.17, as we do https://mvnrepository.com/artifact/log4j/log4j/1.2.17-cloudera1

Given all projects using log4j 2.x have had to do three emergency releases in the space of the week, you are potentially adding more work in the immediate future. in hadoop we are now looking at logback

important have you made sure any/ all transit to dependencies of log4j 1.x have been excluded, including its SLF4J binding library? there may be some transient dependencies waiting to cause problems

### _Why are the changes needed?_  This patch proposes to migrate from log4j1 to log4j2 in Kyuubi. Refer the spark patch: apache/spark#34895 ### Does this PR introduce any user-facing change? Yes. Users may need to rewrite their log4j properties file for log4j2. As of version 2.4, log4j now supports configuration via properties files. Note that the property syntax is not the same as the syntax used in log4j 1, but during the migration I found the syntax is pretty close so the migration should be straightforward. ### _How was this patch tested?_ Passed all existing tests. Closes #1769 from turboFei/log4j2. Closes #1769 8613779 [Fei Wang] remove log4j dependencies from spark-sql-engine b2fe6db [Fei Wang] Use String to present default log level 762e2d0 [Fei Wang] only remove org.apache.logging.log4j:log4j-slf4j-impl 8a91208 [Fei Wang] remove dependencies from spark-sql-engine 7e3a498 [Fei Wang] address commments 051f49f [Fei Wang] address comments 85316a0 [Fei Wang] Keep compatible with log4j12 01d1a84 [Fei Wang] for log4j1 b9e17e1 [Fei Wang] refactor e24391e [Fei Wang] revert log count 3880300 [Fei Wang] add log4j2.properties.template 4f0b22f [Fei Wang] save 7ce8411 [Fei Wang] modify log level 1ea5ca5 [Fei Wang] add log4j to engine c4a86d4 [Fei Wang] use AbstractFilter 27b08b6 [Fei Wang] remove more 8cc15ae [Fei Wang] reformat c13ec29 [Fei Wang] save temporarily 33a38e2 [Fei Wang] exclude log4j12 from spark-sql 9129a64 [Fei Wang] refactor 5362b43 [Fei Wang] make it run at first 7f27f51 [Fei Wang] more 56f4f1f [Fei Wang] fix logging a74b6d3 [Fei Wang] start appender dea964a [Fei Wang] fix build erorr at first e20b750 [Fei Wang] address comments 2ec02b4 [Fei Wang] fix LogDivertAppender dded129 [Fei Wang] more c63e000 [Fei Wang] add log4j2.properties Authored-by: Fei Wang <[email protected]> Signed-off-by: Fei Wang <[email protected]>

This patch proposes to migrate from log4j1 to log4j2 in Spark. Apache Spark uses log4j 1.x, so hence is not susceptible to the recent log4j v2 vulnerability. However, log4j 1.x has reached end of life and is no longer supported. Vulnerabilities reported after August 2015 against log4j 1.x were not checked and will not be fixed. As a result, we are recommended to upgrade Spark to Log4j 2 as soon as possible. Yes. Users may need to rewrite their log4j properties file for log4j2. As of version 2.4, log4j now supports configuration via properties files. Note that the property syntax is not the same as the syntax used in log4j 1, but during the migration I found the syntax is pretty close so the migration should be straightforward. Passed all existing tests. Closes apache#34895 from viirya/log4j2. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

### What changes were proposed in this pull request? This patch proposes to migrate from log4j1 to log4j2 in Spark. ### Why are the changes needed? Apache Spark uses log4j 1.x, so hence is not susceptible to the recent log4j v2 vulnerability. However, log4j 1.x has reached end of life and is no longer supported. Vulnerabilities reported after August 2015 against log4j 1.x were not checked and will not be fixed. As a result, we are recommended to upgrade Spark to Log4j 2 as soon as possible. ### Does this PR introduce _any_ user-facing change? Yes. Users may need to rewrite their log4j properties file for log4j2. As of version 2.4, log4j now supports configuration via properties files. Note that the property syntax is not the same as the syntax used in log4j 1, but during the migration I found the syntax is pretty close so the migration should be straightforward. ### How was this patch tested? Passed all existing tests. Closes apache#34895 from viirya/log4j2. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit ca5cabe)

Kimahriman · 2022-09-11T14:24:57Z

core/src/main/resources/org/apache/spark/log4j2-defaults.properties

+
+appender.console.type = Console
+appender.console.name = STDOUT
+appender.console.target = SYSTEM_OUT


Random thing I just started noticing, the default logging now goes to stdout instead of stderr, was that intentional? The log4j2.properties.template uses stderr too.

spark/core/src/main/resources/org/apache/spark/log4j-defaults.properties

Lines 18 to 23 in 78a5825

# Set everything to be logged to the console

log4j.rootCategory=INFO, console

log4j.appender.console=org.apache.log4j.ConsoleAppender

log4j.appender.console.target=System.err

log4j.appender.console.layout=org.apache.log4j.PatternLayout

log4j.appender.console.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n

spark/core/src/main/resources/org/apache/spark/log4j2-defaults.properties

Lines 19 to 26 in 78d492c

rootLogger.level = info

rootLogger.appenderRef.stdout.ref = STDOUT

appender.console.type = Console

appender.console.name = STDOUT

appender.console.target = SYSTEM_OUT

appender.console.layout.type = PatternLayout

appender.console.layout.pattern = %d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n%ex

look different, WDYT? @viirya

Might be when I copied across properties file and missed to change.

roughly scanned, seems only this place changes from stderr to stdout, #37854 try fix this

viirya added 7 commits December 11, 2021 00:09

Migrate to log4j2 + log4j1 api bridge.

93db02d

More

d21cf16

More

1672ec8

More

e39649b

More

3204b7d

More

18ba1c9

More

80a0cb7

github-actions bot added AVRO BUILD CORE SPARK SHELL SQL labels Dec 14, 2021

This comment has been minimized.

Sign in to view

Fix style

0d7bd5d

viirya marked this pull request as draft December 14, 2021 09:41

laychengtan1401 suggested changes Dec 14, 2021

View reviewed changes

attilapiros mentioned this pull request Dec 14, 2021

[SPARK-37625] update log4j to 2.15 #34877

Closed

enigmatic00 reviewed Dec 14, 2021

View reviewed changes

viirya added 2 commits December 14, 2021 09:25

bump log4j version

5084efe

Complete log4j2-defaults.properties.

7aafeef

Start appender.

2679547

dongjoon-hyun approved these changes Dec 18, 2021

View reviewed changes

dongjoon-hyun closed this in ca5cabe Dec 18, 2021

LuciferYang reviewed Dec 20, 2021

View reviewed changes

JoshRosen reviewed Dec 20, 2021

View reviewed changes

xkrogen reviewed Dec 20, 2021

View reviewed changes

gengliangwang mentioned this pull request Dec 24, 2021

[SPARK-37733][BUILD] Change log level of tests to WARN #35011

Closed

adahn6 mentioned this pull request Jan 12, 2022

Update log4j to 2.17.1 to fix DDOS attacks osmlab/atlas#780

Merged

turboFei mentioned this pull request Jan 16, 2022

[BUILD] Migrate from log4j1 to log4j2 apache/kyuubi#1769

Closed

Kimahriman reviewed Sep 11, 2022

View reviewed changes

viirya deleted the log4j2 branch December 27, 2023 18:25

viirya restored the log4j2 branch December 27, 2023 18:25

	# Set everything to be logged to the console
	log4j.rootCategory=INFO, console
	log4j.appender.console=org.apache.log4j.ConsoleAppender
	log4j.appender.console.target=System.err
	log4j.appender.console.layout=org.apache.log4j.PatternLayout
	log4j.appender.console.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n

	rootLogger.level = info
	rootLogger.appenderRef.stdout.ref = STDOUT

	appender.console.type = Console
	appender.console.name = STDOUT
	appender.console.target = SYSTEM_OUT
	appender.console.layout.type = PatternLayout
	appender.console.layout.pattern = %d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n%ex

[SPARK-6305][BUILD] Migrate from log4j1 to log4j2 #34895

[SPARK-6305][BUILD] Migrate from log4j1 to log4j2 #34895

Uh oh!

Conversation

viirya commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

This comment has been minimized.

SparkQA commented Dec 14, 2021

Uh oh!

SparkQA commented Dec 14, 2021

Uh oh!

SparkQA commented Dec 14, 2021

Uh oh!

laychengtan1401 left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 14, 2021

Uh oh!

SparkQA commented Dec 14, 2021

Uh oh!

SparkQA commented Dec 14, 2021

Uh oh!

SparkQA commented Dec 14, 2021

Uh oh!

SparkQA commented Dec 14, 2021

Uh oh!

SparkQA commented Dec 18, 2021

Uh oh!

SparkQA commented Dec 18, 2021

Uh oh!

SparkQA commented Dec 18, 2021

Uh oh!

SparkQA commented Dec 18, 2021

Uh oh!

dongjoon-hyun commented Dec 18, 2021

Uh oh!

SparkQA commented Dec 18, 2021

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Dec 18, 2021

Uh oh!

viirya commented Dec 18, 2021

Uh oh!

SparkQA commented Dec 18, 2021

Uh oh!

sunchao commented Dec 18, 2021

Uh oh!

viirya commented Dec 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Dec 14, 2021 •

edited

Loading

steveloughran commented Dec 30, 2021 •

edited

Loading

Kimahriman Sep 11, 2022 •

edited

Loading

LuciferYang Sep 12, 2022 •

edited

Loading