Updating dependencies (guava and what brought in older guava) to get rid of the guava-related CVE-2018-10237 and CVE-2020-8908 #13716
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,10 @@ | |
| <artifactId>pulsar-io-flume</artifactId> | ||
| <name>Pulsar IO :: Flume</name> | ||
|
|
||
| <properties> | ||
| <avro.version>1.8.2</avro.version> | ||
| </properties> | ||
|
Comment on lines
+34
to
+36
There was a problem hiding this comment. It's better to move the dependency management to the root pom |
||
|
|
||
| <dependencies> | ||
| <dependency> | ||
| <groupId>${project.groupId}</groupId> | ||
|
|
@@ -54,8 +58,8 @@ | |
| <type>pom</type> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>org.apache.avro</groupId> | ||
| <artifactId>avro-ipc</artifactId> | ||
| </exclusion> | ||
| <exclusion> | ||
| <artifactId>avro</artifactId> | ||
|
|
@@ -66,12 +70,27 @@ | |
| <dependency> | ||
| <groupId>org.apache.avro</groupId> | ||
| <artifactId>avro</artifactId> | ||
| <version>${avro.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>commons-lang</groupId> | ||
| <artifactId>commons-lang</artifactId> | ||
| <version>2.6</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.avro</groupId> | ||
| <artifactId>avro-ipc</artifactId> | ||
| <version>${avro.version}</version> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>org.mortbay.jetty</groupId> | ||
| <artifactId>servlet-api</artifactId> | ||
| </exclusion> | ||
| <exclusion> | ||
| <groupId>io.netty</groupId> | ||
| <artifactId>netty</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.curator</groupId> | ||
|
|
@@ -106,7 +125,7 @@ | |
| <dependency> | ||
| <groupId>com.google.guava</groupId> | ||
| <artifactId>guava</artifactId> | ||
| <version>${guava.version}</version> | ||
|
There was a problem hiding this comment. Does the current Flume version support latest Guava version? There are multiple breaking changes between 18.0 and 31.0 . There was a problem hiding this comment. The tests passed, that's as much as I can tell. There was a problem hiding this comment. I believe that there must have been a reason why the version was pinned to 18.0 . I doubt that the tests in pulsar-io/flume validate the full functionality. @tuteng could you shade some light into this since you are the original author? There was a problem hiding this comment. @lhotari the best testing I could do is:
There was a problem hiding this comment. great work in confirming the compatibility @dlg99 |
||
| </dependency> | ||
| </dependencies> | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change! Please create a separate PR for the jetty-bom change and the jetty.version upgrade. This change isn't related to Guava and would be valuable on it's own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lhotari IIRC this came in after I upgraded other dependencies that used older guava (confluent?), to settle on the newer version. I'd prefer to avoid spending time on ripping the PR apart/rebuilding/rescanning, testing what else needs to be downgraded etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can keep the change in this PR. If you submit it separately, the PR could be merged before this PR. If this PR gets later on reverted for some reason, let's say that it breaks something, we can continue to keep the Jetty change. The Jetty change might also be cherry-picked to maintenance branches, but the Guava change might not be picked. There are multiple reasons to do minimal PRs. I know that it's terrible now that our CI is in such bad shape with a lot of flaky tests and long build times. We just have to keep on improving.
@eolivelli What's your opinion about splitting the jetty-bom change to a new PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC importing the BOM of Jetty solves some problems with the version of Guava imported by Jetty.
While I agree with @lhotari's concern about tracking broken patches and reverting, I also understand @dlg99's pain.
Considering that this patch is only about updating dependencies that are related one to each other I am +1 with merging this patch as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is the Jetty upgrade related to Guava upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found the relation to Jetty, np.