[BEAM-11055] Update log4j to version 2.14.1 #13073

iemejia · 2020-10-12T07:31:02Z

aromanenko-dev

Please, take a look on failed tests. Some of them (at least Java PreCommit) were failed because of this change.

iemejia · 2020-10-12T09:06:50Z

I missed some places so it became a bit unstable, let's see if it is ok now.

aromanenko-dev · 2020-10-12T09:52:15Z

Do we really need to have so many places where the same version is defined? Couldn't be better to extract this into BeamModulePlugin.groovy since it's shared across different modules?

iemejia · 2020-10-12T13:09:31Z

Do we really need to have so many places where the same version is defined? Couldn't be better to extract this into BeamModulePlugin.groovy since it's shared across different modules?

I hesitated to do this because the references defined in BeamModulePlugin.groovy tend to be used as a reference of libraries Beam devs 'can' use and I did not want to promote the use of log4j which is not Beam's default logging library but mostly a provided lifetime one. If you prefer I can put it there.

aromanenko-dev · 2020-10-13T10:06:34Z

@iemejia Yes, this is not our default logging library but since it's a library that is used in different modules (and seems the same version everywhere), then I'd prefer to extract it as we do for other libraries as well. In any case, the version can be overridden in case if module will need another version.

iemejia · 2020-10-13T19:07:40Z

buildSrc/src/main/groovy/org/apache/beam/gradle/GrpcVendoring_1_26_0.groovy

@@ -23,53 +23,54 @@ package org.apache.beam.gradle
 */
 class GrpcVendoring_1_26_0 {

-  static def guava_version = "26.0-jre"


This one and the next one are only sorting stuff to make it more easy to find the refs.

iemejia · 2020-10-13T19:08:52Z

buildSrc/src/main/groovy/org/apache/beam/gradle/GrpcVendoring_1_26_0.groovy

+  static def jboss_marshalling_version = "1.4.11.Final"
+  static def jboss_modules_version = "1.1.0.Beta1"
+  static def jzlib_version = "1.1.3"
+  static def log4j_version = "2.13.3"


Notice that vendored grpc dependencies can have different versions so they don't depend on Beam main module file, so versions are defined independently.

iemejia · 2020-10-13T19:16:18Z

Done suggested changes can you PTAL again @aromanenko-dev

aromanenko-dev · 2020-10-14T10:44:40Z

@iemejia Thanks! Could you take a look on failed tests? Looks like that some of them are caused by this change.

iemejia · 2020-10-14T13:09:28Z

Arrghh I executed by mistake the hcatalog tests locally instead of hadoop-format ones. Let's hope it is ok this time 🤞

aromanenko-dev · 2020-10-14T17:53:39Z

retest this please

aromanenko-dev · 2020-10-15T08:10:28Z

Unfortunately, there are still failing Java tests, not sure that Python one is related.

iemejia · 2020-11-04T17:18:21Z

This PR is breaking on the HCatalog module with a NoClassDefFoundError and that does not make any sense to me.
In the PR I am not even touching that module and as far as I see none of its dependencies, so I am wondering if this is some sort of gradle craziness.

I tried to run Google's Linkage Checker to find the issue but it seems also that the tool configuration is broken on Beam current master (double checked by Alexey) when used with the hcatalog module. @suztomo sorry to bother you, but I am absolutely puzzled about the issue here and I was wondering if you could have a hint on what is going on.

Also I suppose you could help us check what is going on with the linkage check since you mentioned some months ago it was working now on the HCatalog module. Notice that Beam recently upgraded to gradle 6.6 so maybe something was broken as part of that.

suztomo · 2020-11-04T19:11:27Z

Interesting. Let me see.

org.apache.logging.log4j.util.ReflectionUtil is missing when running the test.

java.lang.NoClassDefFoundError: org/apache/logging/log4j/util/ReflectionUtil
	at org.apache.logging.slf4j.Log4jLoggerFactory.getContext(Log4jLoggerFactory.java:42)
	at org.apache.logging.log4j.spi.AbstractLoggerAdapter.getLogger(AbstractLoggerAdapter.java:48)
	at org.apache.logging.slf4j.Log4jLoggerFactory.getLogger(Log4jLoggerFactory.java:29)
	at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:363)
	at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:388)
	at org.apache.hive.hcatalog.common.HCatUtil.<clinit>(HCatUtil.java:83)
	at org.apache.beam.sdk.io.hcatalog.test.EmbeddedMetastoreService.<init>(EmbeddedMetastoreService.java:50)
	at org.apache.beam.sdk.io.hcatalog.HCatalogBeamSchemaTest.setupEmbeddedMetastoreService(HCatalogBeamSchemaTest.java:52)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/14470/testReport/junit/org.apache.beam.sdk.io.hcatalog/HCatalogBeamSchemaTest/classMethod/

Reproduce Problem

The command below reproduces the problem. It's strange that Gradle marks it successful:

./gradlew -p ./sdks/java/io/hcatalog test
...
> Task :sdks:java:io:hcatalog:test

org.apache.beam.sdk.io.hcatalog.HCatalogIOTest > classMethod FAILED
    java.lang.NoClassDefFoundError at HCatalogIOTest.java:88
        Caused by: java.lang.ClassNotFoundException at HCatalogIOTest.java:88

org.apache.beam.sdk.io.hcatalog.HCatalogBeamSchemaTest > classMethod FAILED
    java.lang.NoClassDefFoundError at HCatalogBeamSchemaTest.java:52

org.apache.beam.sdk.io.hcatalog.SchemaUtilsTest > testParameterizedTypesToBeamTypes FAILED
    java.lang.NoClassDefFoundError at SchemaUtilsTest.java:43

3 tests completed, 3 failed
There were failing tests. See the report at: file:///usr/local/google/home/suztomo/beam/sdks/java/io/hcatalog/build/reports/tests/test/index.html

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.6.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 2m 52s
49 actionable tasks: 49 executed
suztomo@suztomo:~/beam$

Found ignoreFailures in the build.gradle:

test {
  // TODO: Get tests to run. Known issues:
  //  * calcite-avatica bundles w/o repackaging Jackson (CALCITE-1110)
  //  * hive-exec bundles w/o repackaging Guava (HIVE-13690)
  ignoreFailures true
}

Turning it off.

TestRuntimeClasspath

https://gist.github.com/suztomo/63d6104d054d80370cd9e34f64e4d13e

As per IntelliJ, org.apache.logging.log4j:log4j-core:2.13.3 holds ~~org.apache.logging.log4j.util.ReflectionUtil~~ (This was wrong! org.apache.logging.log4j.core.util.ReflectionUtil). The artifact appears in the dependency graph:

+--- org.apache.hive.hcatalog:hive-hcatalog-core:2.1.0
|    +--- org.apache.hive:hive-cli:2.1.0
|    |    +--- org.apache.hive:hive-common:2.1.0
|    |    |    +--- org.apache.hive:hive-shims:2.1.0
...
|    |    |    +--- org.apache.logging.log4j:log4j-1.2-api:2.4.1
|    |    |    |    +--- org.apache.logging.log4j:log4j-api:2.4.1 -> 2.13.3
|    |    |    |    \--- org.apache.logging.log4j:log4j-core:2.4.1 -> 2.13.3
|    |    |    |         \--- org.apache.logging.log4j:log4j-api:2.13.3
|    |    |    +--- org.apache.logging.log4j:log4j-web:2.4.1
|    |    |    |    +--- org.apache.logging.log4j:log4j-api:2.4.1 -> 2.13.3
|    |    |    |    \--- org.apache.logging.log4j:log4j-core:2.4.1 -> 2.13.3 (*)

I don't know why Gradle does not supply the missing class from org.apache.logging.log4j:log4j-core:2.13.3 when it runs the test.

iemejia · 2020-11-04T20:19:50Z

Thanks for taking a look @suztomo. I had not seen that ignoreFailures part, weird.
Gradle behavior is so weird, show the dependency but still ignoring it.
I think the person who knows Beam's build best is @lukecwik, maybe he understands what is going on here.

suztomo · 2020-11-05T20:20:07Z

With some debug output, I see the Gradle supplies the ReflectionUtil class when the class runs.

suztomo@8138139#r43916554

I'm now feeling that the EmbeddedMetastoreService runs with a special class path.

Edit: My intelliJ is complaining it too.

Now I know that this case involves two fully-qualified class names:

org.apache.logging.log4j.util.ReflectionUtil (the missing class)
org.apache.logging.log4j.core.util.ReflectionUtil (available in org.apache.logging.log4j:log4j-core:2.13.3`)

iemejia · 2020-11-06T14:11:45Z

Argh! I was afraid of that too, It clearly looks like a different classpath issue, that will explain why it breaks so weirdly.

I think the simplest solution would be to let that module to provide its own log4j. The part that still does not make any sense to me is how the hcatalog module in Beam ends up resolving the new version of log4j? Can you spot which dependency is overwriting the version ? From a look at the module build.gradle file I don't see any of the Beam dependencies of hcatalog requiring log4j. I have checked in the gradle scan too and I just cannot see who is doing it

suztomo · 2020-11-06T17:23:45Z

@iemejia

Can you spot which dependency is overwriting the version ?

BeamModulePlugin.groovy forces the library version listed in the file.

    def log4j_version = "2.13.3"
...
        log4j_core                                  : "org.apache.logging.log4j:log4j-core:$log4j_version",
...
          config.resolutionStrategy {
...
            def librariesWithVersion = project.library.java.values().findAll { it.split(':').size() > 2 }
            force librariesWithVersion
          }

log4j-api-2.4.1.jar had the missing class, while log4j-api-2.13.3.jar does not have it.

suztomo-macbookpro44% jar tf log4j-api-2.4.1.jar |grep Reflection
org/apache/logging/log4j/util/ReflectionUtil$PrivateSecurityManager.class
org/apache/logging/log4j/util/ReflectionUtil.class
suztomo-macbookpro44% jar tf log4j-api-2.13.3.jar |grep Reflection
suztomo-macbookpro44%

It seems that org.slf4j:slf4j-log4j12:1.7.30 has org.apache.logging.slf4j.Log4jLoggerFactory, the caller of the missing class. This does not seem compatible with org.apache.logging.log4j:log4j-api:2.13.3.

iemejia · 2020-11-10T09:27:40Z

@suztomo thanks for the pointer I was not aware we were forcing dependencies, but does forcing all dependencies make even sense? The exclusion I am doing at the hcatalog module in this PR feels hacky specially because log4j is a dependency none of the Beam modules require it is only required by transitive dependencies.

I saw your work on BEAM-9542 #11168 but it seems that the change to support the GCP BoM undid it, was this intentional?

suztomo · 2020-11-12T16:11:46Z

My attempt #11168 was not merged. (As per my comment there, I had to upgrade google-api-client at that time and didn't get a chance to come back.)

iemejia · 2020-11-14T09:58:27Z

I see, good to know there is awareness on the issue, thanks @suztomo.

I wonder now if my forcing fix makes even sense since this PR has two goals (1) upgrade the dependency and (2) silence automatic security detectors from reporting this dependency as a security issue which clearly won't be the case at least for the hcatalog module if I force the previous version even if this is not at all a Beam problem.

WDYT @aromanenko-dev shall we upgrade as it is to cover (1) for most of the other modules knowing that (2) would still be an issue for HCatalog?

codecov · 2021-04-27T10:03:05Z

Codecov Report

Merging #13073 (1958090) into master (a25261a) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 1958090 differs from pull request most recent head 8a24eb8. Consider uploading reports for the commit 8a24eb8 to get more accurate results

@@            Coverage Diff             @@
##           master   #13073      +/-   ##
==========================================
- Coverage   83.64%   83.64%   -0.01%     
==========================================
  Files         443      886     +443     
  Lines       59032   118046   +59014     
==========================================
+ Hits        49376    98735   +49359     
- Misses       9656    19311    +9655

Impacted Files	Coverage Δ
...g/benchmarks/nexmark/queries/nexmark_query_util.py
...rcs/sdks/python/apache_beam/io/aws/s3filesystem.py
...m/examples/snippets/transforms/elementwise/keys.py
...ache_beam/runners/interactive/recording_manager.py
...am/examples/snippets/transforms/aggregation/top.py
...srcs/sdks/python/apache_beam/typehints/__init__.py
...apache_beam/runners/dataflow/internal/apiclient.py
...hon/apache_beam/examples/wordcount_with_metrics.py
.../srcs/sdks/python/apache_beam/coders/coder_impl.py
...ache_beam/runners/interactive/caching/cacheable.py
... and 1319 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a25261a...8a24eb8. Read the comment docs.

iemejia · 2021-04-27T17:04:42Z

sdks/java/io/hcatalog/build.gradle

@@ -41,6 +41,17 @@ test {
  ignoreFailures true
 }

+configurations.testRuntimeClasspath {
+  resolutionStrategy {
+    def log4j_version = "2.4.1"


Somehow I had to redefine this here to make it work, bad to have this repeated but it is the only workaround I could find.

Should this be 2.14.1 ?

Arghh what a silly mistake, my excuses I opened #14751 to upgrade this dep to the minimum compatible version with Hive dependencies, wondering if we should also cherry-pick it for the release. Thanks for noticing @aaltay !

It is a pity we cannot move to the latest log4j version because of Hive's HCatalog dependency constraints. The bad part is that this will still trigger security analysis tools warnings, but I have no idea of how to workaround this apart of doing a full upgrade of Hive to the latest but that does not seem to be so simple either, for ref BEAM-9351.

Thank you @iemejia. I do not have an opinion about cherry picking to the release or not. If it helps, I am not aware of any user issues or bug reports. I noticed this coincidentally.

aromanenko-dev · 2021-04-28T09:40:25Z

Run Java PostCommit

aromanenko-dev

LGTM, thanks!

TheNeuralBit · 2021-05-10T20:56:13Z

sdks/java/io/elasticsearch-tests/elasticsearch-tests-5/build.gradle

-    force "org.apache.logging.log4j:log4j-api:$log4j_version"
-    force "org.apache.logging.log4j:log4j-core:$log4j_version"
+    force library.java.log4j_api
+    force library.java.log4j_core


I think this is no longer necessary with the change to library in BeamModulePlugin.groovy. applyJavaNature does this for all dependencies in library:

beam/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

Lines 1537 to 1544 in 3c9807b

if (config.getName() != "errorprone" && !inDependencyUpdates) {

config.resolutionStrategy {

// Filtering versionless coordinates that depend on BOM. Beam project needs to set the

// versions for only handful libraries when building the project (BEAM-9542).

def librariesWithVersion = project.library.java.values().findAll { it.split(':').size() > 2 }

force librariesWithVersion

}

}

iemejia requested a review from aromanenko-dev October 12, 2020 07:31

aromanenko-dev requested changes Oct 12, 2020

View reviewed changes

iemejia force-pushed the BEAM-11055-log4j-api branch from ea7eeef to 3f385bd Compare October 12, 2020 09:06

iemejia force-pushed the BEAM-11055-log4j-api branch from 3f385bd to 2416310 Compare October 13, 2020 19:06

iemejia commented Oct 13, 2020

View reviewed changes

iemejia force-pushed the BEAM-11055-log4j-api branch from 2416310 to 88baac3 Compare October 13, 2020 19:12

iemejia force-pushed the BEAM-11055-log4j-api branch from 88baac3 to c46bed5 Compare October 14, 2020 08:17

iemejia force-pushed the BEAM-11055-log4j-api branch from c46bed5 to 342b7e9 Compare October 14, 2020 13:08

iemejia force-pushed the BEAM-11055-log4j-api branch from 342b7e9 to b62612d Compare October 19, 2020 09:08

iemejia force-pushed the BEAM-11055-log4j-api branch from b62612d to d0c620e Compare November 4, 2020 15:48

iemejia force-pushed the BEAM-11055-log4j-api branch from d0c620e to 512c432 Compare November 10, 2020 09:24

iemejia force-pushed the BEAM-11055-log4j-api branch from 512c432 to c65b034 Compare November 12, 2020 14:37

iemejia changed the title ~~[BEAM-11055] Update log4j to version 2.13.3~~ [BEAM-11055] Update log4j to version 2.14.0 Nov 12, 2020

iemejia force-pushed the BEAM-11055-log4j-api branch from c65b034 to 25df3a2 Compare April 27, 2021 09:45

iemejia changed the title ~~[BEAM-11055] Update log4j to version 2.14.0~~ [BEAM-11055] Update log4j to version 2.14.1 Apr 27, 2021

iemejia force-pushed the BEAM-11055-log4j-api branch 2 times, most recently from 1958090 to 5f82918 Compare April 27, 2021 16:57

iemejia commented Apr 27, 2021

View reviewed changes

[BEAM-11055] Update log4j to version 2.14.1

8a24eb8

iemejia force-pushed the BEAM-11055-log4j-api branch from 5f82918 to 8a24eb8 Compare April 28, 2021 07:13

aromanenko-dev approved these changes Apr 28, 2021

View reviewed changes

aromanenko-dev merged commit cbc67c8 into apache:master Apr 28, 2021

iemejia deleted the BEAM-11055-log4j-api branch April 28, 2021 10:10

TheNeuralBit reviewed May 10, 2021

View reviewed changes

TheNeuralBit mentioned this pull request May 11, 2021

[BEAM-11055] Don't add log4j to library #14777

Merged

tvalentyn mentioned this pull request Nov 23, 2021

[BEAM-13073] UseParallelGC on Java container #16045

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-11055] Update log4j to version 2.14.1 #13073

[BEAM-11055] Update log4j to version 2.14.1 #13073

iemejia commented Oct 12, 2020

aromanenko-dev left a comment •

edited

iemejia commented Oct 12, 2020

aromanenko-dev commented Oct 12, 2020

iemejia commented Oct 12, 2020 •

edited

aromanenko-dev commented Oct 13, 2020

iemejia Oct 13, 2020

iemejia Oct 13, 2020

iemejia commented Oct 13, 2020

aromanenko-dev commented Oct 14, 2020 •

edited

iemejia commented Oct 14, 2020

aromanenko-dev commented Oct 14, 2020

aromanenko-dev commented Oct 15, 2020

iemejia commented Nov 4, 2020 •

edited

suztomo commented Nov 4, 2020 •

edited

iemejia commented Nov 4, 2020

suztomo commented Nov 5, 2020 •

edited

iemejia commented Nov 6, 2020

suztomo commented Nov 6, 2020 •

edited

iemejia commented Nov 10, 2020

suztomo commented Nov 12, 2020

iemejia commented Nov 14, 2020

codecov bot commented Apr 27, 2021 •

edited

iemejia Apr 27, 2021

aaltay May 6, 2021

iemejia May 7, 2021

iemejia May 7, 2021

aaltay May 7, 2021

aromanenko-dev commented Apr 28, 2021

aromanenko-dev left a comment

TheNeuralBit May 10, 2021

	if (config.getName() != "errorprone" && !inDependencyUpdates) {
	config.resolutionStrategy {
	// Filtering versionless coordinates that depend on BOM. Beam project needs to set the
	// versions for only handful libraries when building the project (BEAM-9542).
	def librariesWithVersion = project.library.java.values().findAll { it.split(':').size() > 2 }
	force librariesWithVersion
	}
	}

[BEAM-11055] Update log4j to version 2.14.1 #13073

[BEAM-11055] Update log4j to version 2.14.1 #13073

Conversation

iemejia commented Oct 12, 2020

aromanenko-dev left a comment • edited

Choose a reason for hiding this comment

iemejia commented Oct 12, 2020

aromanenko-dev commented Oct 12, 2020

iemejia commented Oct 12, 2020 • edited

aromanenko-dev commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iemejia commented Oct 13, 2020

aromanenko-dev commented Oct 14, 2020 • edited

iemejia commented Oct 14, 2020

aromanenko-dev commented Oct 14, 2020

aromanenko-dev commented Oct 15, 2020

iemejia commented Nov 4, 2020 • edited

suztomo commented Nov 4, 2020 • edited

Reproduce Problem

TestRuntimeClasspath

iemejia commented Nov 4, 2020

suztomo commented Nov 5, 2020 • edited

iemejia commented Nov 6, 2020

suztomo commented Nov 6, 2020 • edited

iemejia commented Nov 10, 2020

suztomo commented Nov 12, 2020

iemejia commented Nov 14, 2020

codecov bot commented Apr 27, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aromanenko-dev commented Apr 28, 2021

aromanenko-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aromanenko-dev left a comment •

edited

iemejia commented Oct 12, 2020 •

edited

aromanenko-dev commented Oct 14, 2020 •

edited

iemejia commented Nov 4, 2020 •

edited

suztomo commented Nov 4, 2020 •

edited

suztomo commented Nov 5, 2020 •

edited

suztomo commented Nov 6, 2020 •

edited

codecov bot commented Apr 27, 2021 •

edited