-
Notifications
You must be signed in to change notification settings - Fork 3.8k
CASSANDRA-18875: Upgrade jackson-dataformat-yaml to 2.19.2 and snakeyaml to 2.1 #4376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Patch by Raymond Huffman; reviewed by brandonwilliams and smiklosovic for CASSANDRA-18875
|
Hi @griffindvs ,
Since that comment, we have updated OWASP dependency check task significantly so there is a chance that this will work. EDIT: I see that OWASP 12.1.0 uses Jackson 2.18.2 which uses snakeyaml 2.3. I think we could be OK here. https://github.com/dependency-check/DependencyCheck/blob/v12.1.0/pom.xml#L175 Even better, OWASP 12.1.3 uses Jackson 2.19.0 which uses Snake 2.4 https://github.com/dependency-check/DependencyCheck/blob/v12.1.3/pom.xml#L174 I think that if this breaks OWASP, it would be temporary and we might just update it from 12.1.0 to 12.1.3 to resolve it (in a separate ticket). |
smiklosovic
left a comment
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.
suggestoins left in another comment
|
@smiklosovic, going through the feedback in a bit more detail…
action plan:
|
Patch by Griffin Davis; reviewed by Mick Semb Wever and Štefan Miklošovič for CASSANDRA-18875
392402b to
8e65c82
Compare
|
Thank you @smiklosovic and @michaelsembwever for the reviews and guidance!
|
8e65c82 to
8f02725
Compare
|
3 yes, but don't stress it. Committers would ultimately do that if you don't. It is just nice to do from your side so you save committers some time but ultimately if you don't do that, that is in no way any blocker. 8 thanks for checking it works. But we should have 0 CVEs unless these you showed are brand new we have not caught yet? I will check more closely later. EDIT 8) yes I see the same ... interesting. I have not seen them when releasing 5.0.5. We will need to treat this separately. |
|
I still see 2.11.3 dependencies of jackson in build/test/lib/jars, it seems to be non-deterministic. |
|
Hi @smiklosovic , @michaelsembwever helped me run: which led me to find: This is the only reference to an older version of jackson that I see (I don't see 2.11 anywhere in here). This log output led me to look at wiremock 2.35.0 which uses jackson 2.13.4: https://github.com/wiremock/wiremock/blob/2.35.0/build.gradle#L31 I'm not sure if this helps identify where |
|
@smiklosovic I did some more digging, and it looks like cassandra harry is pulling in Jackson 2.11.3: https://github.com/apache/cassandra-harry/blob/0.0.1/pom.xml#L56 |
|
@griffindvs great find! I was looking at this with AI and I could not find the culprit. I am not sure what to do with it though. Would not be helpful if we tried to move declared dependencies "lower" in the pom? I think how it works is that dependencies declared later would "shadow" earlier ones. But at the same time it does not make a lot of sense to me because 2.11.3 is the dependency of harry while we are declaring these dependencies as "top level" and in that case the top level ones should win. Anyway, worth of trying maybe? |
|
@smiklosovic I tried moving our declared jackson dependencies lower in the POM (below wiremock and harry), but the older jars were still present. If we add an exclusion for jackson from harry and wiremock then the jars are removed: <dependency>
<groupId>org.apache.cassandra</groupId>
<artifactId>harry-core</artifactId>
<version>0.0.1</version>
<scope>test</scope>
<exclusions>
<exclusion>
<artifactId>jackson-databind</artifactId>
<groupId>com.fasterxml.jackson.core</groupId>
</exclusion>
<exclusion>
<artifactId>jackson-annotations</artifactId>
<groupId>com.fasterxml.jackson.core</groupId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock-jre8</artifactId>
<version>2.35.0</version>
<scope>test</scope>
<exclusions>
<exclusion>
<artifactId>jackson-annotations</artifactId>
<groupId>com.fasterxml.jackson.core</groupId>
</exclusion>
</exclusions>
</dependency>I'm not sure though if there would be side effects from doing this (eg. if wiremock or harry are incompatible with the newer jackson version). |
|
We can test it. Give me some time please. I will kick off a build and will return back to you. |
|
https://pre-ci.cassandra.apache.org/job/cassandra-5.0/22/pipeline-overview/ I have run tests where wiremock is used locally and they passed. |
patch by Griffin Davis; reviewed by Mick Semb Wever and Štefan Miklošovič for CASSANDRA-20848
8f02725 to
3d509f4
Compare
|
3d509f4 includes the exclusions for jackson in wiremock and harry |
This isn't the correct approach to take. The correct approach is exclusions. patch for cassandra-harry: apache/cassandra-harry@trunk...thelastpickle:cassandra-harry:mck/CASSANDRA-18875 But… why arn't these exclusion problems a problem in trunk ? |
|
@michaelsembwever because Harry was taken into the tree in trunk while in 5.0 it was as a dependency. But there is still 2.13.4 of Jackson annotations in I have build 5.0 patch and works (see above link) so not going to build it once again. Trunk patch is subset of that, just updates annotations of 2.13.4 to 2.19.2. Are we prepared to merge? |
|
@griffindvs thank you! |
CASSANDRA-18875
CASSANDRA-17907
CASSANDRA-20848
This PR back-ports 64ae866 for cassandra 5.0. The original commit was reverted in 8cd0690 and un-reverted in 7204bc4.
Jackson databind, core, and datatype were already updated to a newer 2.19.2 in af0197e.
2.15.3 of jackson-dataformat-yaml uses a matching version 2.1 of snakeyaml. This PR updates to jackson-dataformat-yaml to 2.19.2 to match the other jackson libraries and mirror the changes in trunk for CASSANDRA-20848. Because jackson-dataformat-yaml uses snakeyaml 2.4 which does not yet work with cassandra, we exclude it in the jackson dependency. There were no backwards-incompatible changes in the intervening versions of jackson-dataformat-yaml.
snakeyaml 2.0 included a few backwards-incompatible changes:
snakeyaml 2.1 included one backwards-incompatible change: