[BEAM-12422] Vendored gRPC: Removing unnecessary log4j-api dependency #15098

suztomo · 2021-06-29T21:51:12Z

Vendored gRPC 1.36.0 was using a log4j version with security issues. https://issues.apache.org/jira/browse/BEAM-12422

gRPC does not declare log4j dependency. It uses java.util.logging for logging. Therefore this PR removes the dependency.

How I tested this

I created #15103 to confirm the vendored gRPC works fine by installing the vendored gRPC into local Maven repository. The checks passed:

Linkage Checker

There's no reference to log4j-api in the result of Linkage Checker:
https://gist.github.com/suztomo/4719cedebd20887b0a0dcd50f0f5a4f2

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Samza	Twister2
Go	---	---	---
Java
Python	---	---	---
XLang		---	---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

Vendored gRPC 1.36.0 was using a log4j version with security issues. gRPC does not declare log4j dependency. It uses java.util.logging for logging. Therefore removing the dependency.

suztomo · 2021-06-29T21:52:18Z

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

-      "org/apache/log4j/**",
-      "org/apache/logging/log4j/**",


I confirmed that the resulting JAR does not contain log4j classes.

suztomo-macbookpro44% jar tf vendor/grpc-1_36_0/build/libs/beam-vendor-grpc-1_36_0-0.2.jar |grep log4j suztomo-macbookpro44%

suztomo · 2021-06-29T23:35:06Z

Run Python Precommit

suztomo · 2021-06-29T23:56:42Z

Run Java_Examples_Dataflow PreCommit

suztomo · 2021-06-30T00:50:11Z

Run Java_Examples_Dataflow PreCommit

suztomo · 2021-06-30T02:07:21Z

Run Python Precommit

codecov · 2021-06-30T03:06:26Z

Codecov Report

Merging #15098 (5690af7) into master (000ac07) will increase coverage by 1.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #15098      +/-   ##
==========================================
+ Coverage   82.55%   83.77%   +1.21%     
==========================================
  Files         455      439      -16     
  Lines       55143    59245    +4102     
==========================================
+ Hits        45526    49630    +4104     
+ Misses       9617     9615       -2

Impacted Files	Coverage Δ
...python/apache_beam/examples/wordcount_dataframe.py	`0.00% <0.00%> (-92.60%)`	⬇️
...s/python/apache_beam/examples/snippets/snippets.py	`76.70% <0.00%> (-12.83%)`	⬇️
sdks/python/apache_beam/io/kafka.py	`79.16% <0.00%> (-6.55%)`	⬇️
sdks/python/apache_beam/utils/interactive_utils.py	`87.80% <0.00%> (-5.06%)`	⬇️
.../python/apache_beam/testing/test_stream_service.py	`88.37% <0.00%> (-4.81%)`	⬇️
...n/apache_beam/runners/direct/test_direct_runner.py	`37.50% <0.00%> (-4.81%)`	⬇️
...pache_beam/runners/interactive/interactive_beam.py	`74.72% <0.00%> (-4.81%)`	⬇️
test_config.py	`66.66% <0.00%> (-4.77%)`	⬇️
sdks/python/apache_beam/runners/test/__init__.py	`66.66% <0.00%> (-4.77%)`	⬇️
sdks/python/apache_beam/io/gcp/__init__.py	`80.00% <0.00%> (-4.62%)`	⬇️
... and 457 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 5fffad6...5690af7. Read the comment docs.

suztomo · 2021-06-30T11:59:09Z

R: @lukecwik

All checks passed in #15103

suztomo · 2021-06-30T13:47:14Z

R: @iemejia

iemejia

LGTM

iemejia · 2021-06-30T14:59:41Z

Thanks @suztomo!

lukecwik · 2021-07-01T17:33:17Z

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

@@ -70,7 +70,6 @@ class GrpcVendoring_1_36_0 {
    return [
      'com.google.errorprone:error_prone_annotations:2.4.0',
      'commons-logging:commons-logging:1.2',
-      'org.apache.logging.log4j:log4j-api:2.6.2',


I don't think this is what we wanted to do.

The idea has always been to have the vendored libraries expose some runtime deps as not everything should be relocated (e.g. logging shouldn't be relocated otherwise we lose logging from the relocated code). We should have just bumped the version to something that doesn't have the security issue and is compatible with the 2.6.2 version.

Ditto on the exclusions, we specifically keep them to prevent relocating logging stuff and other libs that can't be relocated.

Created #15113 to address this.

Turns out netty attempts to load various loggers so this change was always ok. #15113 removes the additional logging libs.

[BEAM-12422] Removing unnecessary log4j-api dependency

5690af7

Vendored gRPC 1.36.0 was using a log4j version with security issues. gRPC does not declare log4j dependency. It uses java.util.logging for logging. Therefore removing the dependency.

suztomo commented Jun 29, 2021

View reviewed changes

suztomo requested a review from lukecwik June 29, 2021 23:57

iemejia approved these changes Jun 30, 2021

View reviewed changes

iemejia merged commit 68c56f8 into apache:master Jun 30, 2021

lukecwik reviewed Jul 1, 2021

View reviewed changes

lukecwik mentioned this pull request Jul 1, 2021

[BEAM-12422] Drop other optional logger libraries since Netty can detect which logger is available automatically #15113

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-12422] Vendored gRPC: Removing unnecessary log4j-api dependency #15098

[BEAM-12422] Vendored gRPC: Removing unnecessary log4j-api dependency #15098

suztomo commented Jun 29, 2021 •

edited

Loading

suztomo Jun 29, 2021

suztomo commented Jun 29, 2021

suztomo commented Jun 29, 2021

suztomo commented Jun 30, 2021

suztomo commented Jun 30, 2021

codecov bot commented Jun 30, 2021 •

edited

Loading

suztomo commented Jun 30, 2021

suztomo commented Jun 30, 2021

iemejia left a comment

iemejia commented Jun 30, 2021

lukecwik Jul 1, 2021

lukecwik Jul 1, 2021

lukecwik Jul 1, 2021

[BEAM-12422] Vendored gRPC: Removing unnecessary log4j-api dependency #15098

[BEAM-12422] Vendored gRPC: Removing unnecessary log4j-api dependency #15098

Conversation

suztomo commented Jun 29, 2021 • edited Loading

How I tested this

Linkage Checker

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

suztomo Jun 29, 2021

Choose a reason for hiding this comment

suztomo commented Jun 29, 2021

suztomo commented Jun 29, 2021

suztomo commented Jun 30, 2021

suztomo commented Jun 30, 2021

codecov bot commented Jun 30, 2021 • edited Loading

Codecov Report

suztomo commented Jun 30, 2021

suztomo commented Jun 30, 2021

iemejia left a comment

Choose a reason for hiding this comment

iemejia commented Jun 30, 2021

lukecwik Jul 1, 2021

Choose a reason for hiding this comment

lukecwik Jul 1, 2021

Choose a reason for hiding this comment

lukecwik Jul 1, 2021

Choose a reason for hiding this comment

suztomo commented Jun 29, 2021 •

edited

Loading

`ValidatesRunner` compliance status (on master branch)

codecov bot commented Jun 30, 2021 •

edited

Loading