Repository navigation
guava fixes - #683
guava fixes#683AbhishekKumar9984 wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Guava dependency version across multiple POM files to use the ${guava.version} property. The review feedback points out that specifying the version in the submodules is redundant due to parent dependency management. Additionally, the selected Guava version (32.0.0-jre) is vulnerable to CVE-2023-2976, and it is recommended to upgrade to a secure version like 32.1.3-jre and ensure consistency across all profiles.
fixes3 fixes34 fixes343
2a7fcbb to
f10f18c
Compare
deepika-lalwani123
left a comment
deepika-lalwani123
left a comment
There was a problem hiding this comment.
This depends on cdapio/cdap#16279. The currently published 6.12.0-SNAPSHOT still has Guava 13, so this PR cannot be validated or merged until that PR merges and CDAP (and Twill) are published. Please call out the dependency in the PR description.
| try { | ||
| HydratorTestBase.initialize(); | ||
| initialized = true; | ||
| } catch (LinkageError e) { |
There was a problem hiding this comment.
Catching LinkageError and calling Assume.assumeNoException silently skips every DB integration test, and it does so on exactly the failure this upgrade can cause. CI will be green without running anything. Please remove it and let the tests fail until CDAP with Guava 32 is published.
| <properties> | ||
| <testSourceLocation>src/e2e-test/java</testSourceLocation> | ||
| <TEST_RUNNER>TestRunner.java</TEST_RUNNER> | ||
| <guava.version>32.1.2-jre</guava.version> |
There was a problem hiding this comment.
The e2e profile is being downgraded from 32.1.2-jre to 32.0.0-jre. Please don't go backwards. Please use Guava >= 32.1.3-jre instead of 32.0.0-jre. 32.0.0 has a known regression (temp dir handling), fixed in 32.0.1. Ideally pin it once in dependencyManagement and add a maven-enforcer requireUpperBoundDeps rule rather than per-dependency <exclusion> blocks.
| <cdap.version>6.12.0-SNAPSHOT</cdap.version> | ||
| <cdap.plugin.version>2.13.1</cdap.plugin.version> | ||
| <guava.version>13.0.1</guava.version> | ||
| <guava.version>32.0.0-jre</guava.version> |
There was a problem hiding this comment.
Please use Guava >= 32.1.3-jre instead of 32.0.0-jre. 32.0.0 has a known regression (temp dir handling), fixed in 32.0.1. Ideally pin it once in dependencyManagement and add a maven-enforcer requireUpperBoundDeps rule rather than per-dependency <exclusion> blocks.
| <plugins> | ||
| <plugin> | ||
| <groupId>org.apache.felix</groupId> | ||
| <artifactId>maven-bundle-plugin</artifactId> |
There was a problem hiding this comment.
Removing this maven-bundle-plugin block may change the exported packages for this plugin (same in cloudsql-postgresql). Please compare the bundle manifest before and after.
Issue :
Resolved security vulnerabilities (CVE-2023-2976 / CVE-2020-8908) associated with older versions of
com.google.guava:guava(13.0.1/31.x) pulled in directly and transitively across database plugins.Root Cause :
Root and submodule POMs referenced older Guava versions (
13.0.1,31.0.1-jre,31.1-jre), and CDAP, Hadoop, and CloudSQL socket-factory dependencies transitively brought in conflicting older versions of Guava causing compilation and test issues.Fix :
<guava.version>to32.0.0-jrein rootpom.xml,cloudsql-mysql-plugin/pom.xml, andcloudsql-postgresql-plugin/pom.xml.com.google.guava:guavafromcdap-api,cdap-etl-proto,cdap-etl-api,cdap-data-pipeline3_2.12,hydrator-test,hydrator-common,cdap-test,cdap-unit-test,hadoop-common,cdap-e2e-framework,mysql-socket-factory-connector-j-8, andpostgres-socket-factory.<cdap.version>from6.11.0to6.12.0-SNAPSHOT.Testing evidence: https://docs.google.com/spreadsheets/d/1Cw6Z_k3wlks7sAC4-Kg6FTIMRL7-zxLlsJIXvVJLrb8/edit?gid=0#gid=0