[BEAM-12422] Vendored gRPC: Removing unnecessary log4j-api dependency by suztomo · Pull Request #15098 · apache/beam

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

    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

Conversation

suztomo commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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)

Uh oh!

suztomo Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

suztomo commented Jun 29, 2021

Uh oh!

suztomo commented Jun 29, 2021

Uh oh!

suztomo commented Jun 30, 2021

Uh oh!

suztomo commented Jun 30, 2021

Uh oh!

codecov bot commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

suztomo commented Jun 30, 2021

Uh oh!

suztomo commented Jun 30, 2021

Uh oh!

iemejia left a comment

Choose a reason for hiding this comment

Uh oh!

iemejia commented Jun 30, 2021

Uh oh!

lukecwik Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

lukecwik Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

lukecwik Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

suztomo commented Jun 29, 2021 •

edited

Loading

`ValidatesRunner` compliance status (on master branch)

codecov bot commented Jun 30, 2021 •

edited

Loading