Skip to content

Comments

Dependency Version Fixes: Woodstox 6.4.0, Scala-lang 2.13.9, Jackson-Databind 2.14.0, Kafka-clients 3.0.2#2269

Merged
DarshitChanpura merged 7 commits intoopensearch-project:1.xfrom
stephen-crawford:1.x
Nov 29, 2022
Merged

Dependency Version Fixes: Woodstox 6.4.0, Scala-lang 2.13.9, Jackson-Databind 2.14.0, Kafka-clients 3.0.2#2269
DarshitChanpura merged 7 commits intoopensearch-project:1.xfrom
stephen-crawford:1.x

Conversation

@stephen-crawford
Copy link
Contributor

@stephen-crawford stephen-crawford commented Nov 21, 2022

Signed-off-by: Stephen Crawford steecraw@amazon.com

Description

Fixes a remaining dependency to Woodstox 6.2.6 library as a follow-up to #2197. I incorrectly looked at only the direct dependencies previously not accounting for the transient dependencies introduced by other libraries. This force should make the Woodstox version match the desired 6.4.0.

I also corrected the versions for jackson_databind, kafka, and snakeyaml.

Issues Resolved

Further resolves the Woodstox dependency problem.

Testing

After the change, runnning ./gradlew dependencies shows all references to Woodstox-core being bumped to 6.4.0 and all the other WhiteSource issues being fixed.

Check List

  • New functionality includes testing
  • [] New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>
@stephen-crawford stephen-crawford requested a review from a team November 21, 2022 14:42
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
@stephen-crawford stephen-crawford changed the title Force woodstox to 6.4.0 Dependency Version Fixes Nov 21, 2022
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
@codecov-commenter
Copy link

Codecov Report

Merging #2269 (9a72120) into 1.x (154cf69) will increase coverage by 0.02%.
The diff coverage is 50.00%.

❗ Current head 9a72120 differs from pull request most recent head ed5634f. Consider uploading reports for the commit ed5634f to get more accurate results

@@             Coverage Diff              @@
##                1.x    #2269      +/-   ##
============================================
+ Coverage     64.60%   64.62%   +0.02%     
- Complexity     3214     3218       +4     
============================================
  Files           247      247              
  Lines         17356    17358       +2     
  Branches       3083     3085       +2     
============================================
+ Hits          11212    11218       +6     
+ Misses         4594     4590       -4     
  Partials       1550     1550              
Impacted Files Coverage Δ
...earch/security/resolver/IndexResolverReplacer.java 63.97% <0.00%> (-0.19%) ⬇️
...earch/security/privileges/PrivilegesEvaluator.java 71.57% <100.00%> (+0.09%) ⬆️
...search/security/transport/SecurityInterceptor.java 74.78% <0.00%> (+0.84%) ⬆️
...ecurity/configuration/ConfigurationRepository.java 75.27% <0.00%> (+2.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cwperks
Copy link
Member

cwperks commented Nov 21, 2022

@scrawfor99 Can you list the dependencies and versions being upgraded in the PR title?

build.gradle Outdated
force "com.fasterxml.woodstox:woodstox-core:6.4.0"
force "org.scala-lang:scala-library:2.13.9"
force "org.yaml:snakeyaml:1.32"
force 'io.netty:netty-buffer:4.1.78.Final'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to force the resolution of the netty version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a netty cve in the previous push without the netty change. I know that in later versions we have a netty.version variable but this does not seem to exist for 1.x.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this should be fine even without adding it as a force resolution but I don't see any harm in it being added here too.

See https://docs.gradle.org/current/dsl/org.gradle.api.artifacts.ResolutionStrategy.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think I may have seen the CVE for 1.3 on Netty and then because I just copy+pasted the same changes it may have been included. I can remove it if you think I should but I imagine keeping it is not a bad idea either.

Copy link
Member

@cwperks cwperks Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

netty is set to 4.1.79.Final 4.1.84.Final for core, can this version match the version from core?

Note the SNAPSHOT may be outdated, this is the version in core's version.properties in 1.3: https://github.com/opensearch-project/OpenSearch/blob/1.3/buildSrc/version.properties#L22

runtimeClasspath - Runtime classpath of source set 'main'.
+--- jakarta.annotation:jakarta.annotation-api:1.3.5
+--- org.opensearch.plugin:transport-netty4-client:1.3.6-SNAPSHOT
|    +--- io.netty:netty-buffer:4.1.79.Final
|    +--- io.netty:netty-codec:4.1.79.Final
|    +--- io.netty:netty-codec-http:4.1.79.Final
|    +--- io.netty:netty-common:4.1.79.Final
|    +--- io.netty:netty-handler:4.1.79.Final
|    +--- io.netty:netty-resolver:4.1.79.Final
|    +--- io.netty:netty-transport:4.1.79.Final
|    \--- io.netty:netty-transport-native-unix-common:4.1.79.Final

Does dependabot pick up on stale versions in this section?

I think snakeyaml is unnecessary here, its coming from opensearch dependencies that have already been patched. snakeyaml comes to security via org.opensearch:opensearch-x-content:1.3.6-SNAPSHOT and its been upgraded to 1.32 in the latest snapshot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I can remove snakeyaml and then swap netty to be 79

@stephen-crawford stephen-crawford changed the title Dependency Version Fixes Dependency Version Fixes: Woodstox 6.4.0, Scala-lang 2.13.9, Snakeyaml 1.32, Netty 4.1.78 Nov 21, 2022
@stephen-crawford stephen-crawford changed the title Dependency Version Fixes: Woodstox 6.4.0, Scala-lang 2.13.9, Snakeyaml 1.32, Netty 4.1.78 Dependency Version Fixes: Woodstox 6.4.0, Scala-lang 2.13.9, Snakeyaml 1.32, Netty 4.1.78, Jackson-Databind 2.14.0 Nov 21, 2022
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
@cwperks
Copy link
Member

cwperks commented Nov 21, 2022

@scrawfor99 The snakeyaml flag can be ignored, the SNAPSHOT build is outdated. Netty can be added back into the resolutionStrategy section with 4.1.84.Final as the version, but we will need a way to be notified of upstream changes from core to try to match the version. In the main branch we are able to reference versions from versions.properties using ${version.netty}, but its not possible to do that in the 1.3 branch.

@cwperks
Copy link
Member

cwperks commented Nov 21, 2022

You can monitor builds here: https://build.ci.opensearch.org/job/distribution-build-opensearch/. Looks like there are some recent attempts to build 1.3.7, but none have succeeded.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>
@stephen-crawford
Copy link
Contributor Author

@cwperks @DarshitChanpura what do you two think about the current state of things? Are we keeping netty and snakeyaml for now since 1.3 does not have the version properties or did you prefer I remove them? I would like to get this merged ASAP so just following up.

@cwperks cwperks changed the title Dependency Version Fixes: Woodstox 6.4.0, Scala-lang 2.13.9, Snakeyaml 1.32, Netty 4.1.78, Jackson-Databind 2.14.0 Dependency Version Fixes: Woodstox 6.4.0, Scala-lang 2.13.9, Jackson-Databind 2.14.0 Nov 23, 2022
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
@stephen-crawford
Copy link
Contributor Author

Removing it causes WhiteSource Security to fail like before. I am not sure why we wanted to remove it if it causes the check to report a vulnerability.

@DarshitChanpura
Copy link
Member

Removing it causes WhiteSource Security to fail like before. I am not sure why we wanted to remove it if it causes the check to report a vulnerability.

For the sake of this PR let's add netty and snakeyaml back, and change the PR description. We can cleanup later. @cwperks Let's fix the whitesource errors so that CI is unblocked.

@stephen-crawford
Copy link
Contributor Author

stephen-crawford commented Nov 23, 2022

Per further discussion: this PR is going to remained blocked until the artifact changes allow WhiteSource to pass.

@cwperks
Copy link
Member

cwperks commented Nov 23, 2022

I think we should add a step in the CI to checkout the corresponding branch of core, assemble the jar and publish to maven local so that the build is not dependent on updated artifacts from infra team.

The additional steps at the beginning of CI would be:

  1. Checkout corresponding branch of core
  2. Run ./gradlew assemble or the corresponding command needed to generate a jar to publish to maven
  3. Run ./gradlew publishToMavenLocal

@cwperks
Copy link
Member

cwperks commented Nov 23, 2022

It may not be necessary to run ./gradlew assemble. ./gradlew publishToMavenLocal does the compilation, jarring and publishing to the local maven m2 repository.

@DarshitChanpura DarshitChanpura changed the title Dependency Version Fixes: Woodstox 6.4.0, Scala-lang 2.13.9, Jackson-Databind 2.14.0 Dependency Version Fixes: Woodstox 6.4.0, Scala-lang 2.13.9, Jackson-Databind 2.14.0, Kafka-clients 3.0.2 Nov 29, 2022
@DarshitChanpura
Copy link
Member

Merging this as the whitesource failures are due to stale artifacts and should resolve itself once latest artifacts become avaialble.

@DarshitChanpura DarshitChanpura merged commit 4ed546a into opensearch-project:1.x Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants