Apply Jackson stream read constraints defaults at runtime #16832

donoghuc · 2024-12-24T01:02:53Z

Release notes

Ensure that the Jackson read constraints defaults (Maximum Number value length, Maximum String value length, and Maximum Nesting depth) are applied at runtime if they are absent from jvm.options.

What does this PR do?

When Logstash 8.12.0 added increased Jackson stream read constraints to jvm.options, assumptions about the existence of that file's contents were invalidated. This led to issues like #16683.

This change ensures Logstash applies defaults from config at runtime:

MAX_STRING_LENGTH: 200_000_000
MAX_NUMBER_LENGTH: 10_000
MAX_NESTING_DEPTH: 1_000

These match the jvm.options defaults and are applied even when config is missing. Config values still override these defaults when present.

Why is it important/What is the impact to the user?

Users who have custom jvm.options files or have files that are not being updated with defaults in latest Logstash releases will still get the defaults we intend configure for the Jackson parsing lib.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files (and/or docker env variables)~~
I have added tests that prove my fix is effective or that my feature works

Related issues

Closes #16773

donoghuc · 2024-12-24T01:04:20Z

logstash-core/src/main/java/org/logstash/jackson/StreamReadConstraintsUtil.java

+    public static final Map<Override, Integer> JACKSON_DEFAULTS = Map.of(
+        Override.MAX_STRING_LENGTH, 200_000_000,
+        Override.MAX_NUMBER_LENGTH, 10_000,
+        Override.MAX_NESTING_DEPTH, 1_000


CODEREVIEW: I just noticed this is actually not set explicitly

logstash/config/jvm.options

Line 90 in 0eb03ad

#-Dlogstash.jackson.stream-read-constraints.max-nesting-depth=1000

(it was commented out). Should i remove this setting?

@edmocosta Is it intended to comment out? I think it is supposed to be uncommented

Yes, it's intended to be commented out, at time we added those properties, 1000 was the default value already, and we haven't had any problem with that specific limit that would justify overriding it.

It seems the Jackson's team is still tuning it, and the new default value was reduced to 500 (FasterXML/jackson-core#1234). They've a good reasoning on why it was reduced (FasterXML/jackson-core#1233), but I think we can be more conservative and sticky to the well tested 1000 (at least for now). With this new code, it seems uncommenting the property wouldn't make much difference, so I'm fine with whatever you folks think is better.

thanks, keeping the comment is good to me.

Wow it sounds we lost consistency here as well.
Do we know till which jackson version (and LS version) we had nesting-depth with 1000 and which version decreased to 500 in which LS version? Do we have issues reported for such situation? If so, addressing separately would be good idea considering user stories.

Wow it sounds we lost consistency here as well.

The new default value will be released in 3.0, so we should be fine now and after upgrading (thanks to this PR).

Sorry, my comment was very ambiguous after reading it following a break 😅

My question is given we have this commented out should I remove the default from being applied. I was not suggesting we remove the example from the options file.

Given we have this commented out should I remove the default from being applied

IMO, I'd keep it, it's a no-op for now, but knowing that Jackson will reduce this limit in the near future, setting it here would ensure that all defaults values are aligned with Logstash's requirements.

kaisecheng

LGTM. Let's wait for Edmo's response regarding the nested depth config

logstash-core/src/main/java/org/logstash/jackson/StreamReadConstraintsUtil.java

mashhurs · 2024-12-24T19:03:15Z

logstash-core/src/main/java/org/logstash/jackson/StreamReadConstraintsUtil.java

+    public static final Map<Override, Integer> JACKSON_DEFAULTS = Map.of(
+        Override.MAX_STRING_LENGTH, 200_000_000,
+        Override.MAX_NUMBER_LENGTH, 10_000,
+        Override.MAX_NESTING_DEPTH, 1_000


Wow it sounds we lost consistency here as well.
Do we know till which jackson version (and LS version) we had nesting-depth with 1000 and which version decreased to 500 in which LS version? Do we have issues reported for such situation? If so, addressing separately would be good idea considering user stories.

mashhurs · 2024-12-24T19:30:46Z

logstash-core/src/main/java/org/logstash/jackson/StreamReadConstraintsUtil.java

@@ -78,6 +86,8 @@ StreamReadConstraints get() {
        if (configuredStreamReadConstraints == null) {
            final StreamReadConstraints.Builder builder = StreamReadConstraints.defaults().rebuild();

+            // Apply the Jackson defaults first, then the overrides from config
+            JACKSON_DEFAULTS.forEach((override, value) -> override.applicator.apply(builder, value));


My biggest worry with these kind of changes is how to educate users to keep settings up to date.

Users upgraded LS from the versions which jvm.options doesn't have Jackson default params to current change, we need to make sure to inform them jvm.options is not up to date. The situations,

users who jumped from 8.12.0-, the jvm.options doesn't have

users upgraded LS multiple times (8.12.0- to 8.15.0 then 8.17.0), the jvm.options may not have jackson default params, depending on how (through package managers, or download tarball and replace) they upgrade.

and with 1) we also need to inform LS is applying default values or user defined values are active. I think we are covering this

I was thinking this approach removed the need for user's to worry about this at all. In all the cases you mentioned they never cared about this setting and now they can continue to not care about it. They only have to think about it if they are in a situation where they explicitly need to tune it. But maybe i'm not quite understanding your concern?

When Logstash 8.12.0 added increased Jackson stream read constraints to jvm.options, assumptions about the existence of that file's contents were invalidated. This led to issues like elastic#16683. This change ensures Logstash applies defaults from config at runtime: - MAX_STRING_LENGTH: 200_000_000 - MAX_NUMBER_LENGTH: 10_000 - MAX_NESTING_DEPTH: 1_000 These match the jvm.options defaults and are applied even when config is missing. Config values still override these defaults when present.

elastic-sonarqube · 2024-12-30T18:26:10Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

elasticmachine · 2024-12-30T18:40:01Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 7859a63

Failed CI Steps

History

💚 Build #1987 succeeded 42a15f2

donoghuc commented Dec 24, 2024

View reviewed changes

donoghuc added the bug label Dec 24, 2024

kaisecheng approved these changes Dec 24, 2024

View reviewed changes

mashhurs reviewed Dec 24, 2024

View reviewed changes

donoghuc force-pushed the GH-16773-jackson-defaults branch from 42a15f2 to 7859a63 Compare December 30, 2024 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply Jackson stream read constraints defaults at runtime #16832

Apply Jackson stream read constraints defaults at runtime #16832

donoghuc commented Dec 24, 2024 •

edited

Loading

donoghuc Dec 24, 2024

kaisecheng Dec 24, 2024

edmocosta Dec 24, 2024

kaisecheng Dec 24, 2024

mashhurs Dec 24, 2024

edmocosta Dec 30, 2024 •

edited

Loading

donoghuc Dec 30, 2024

edmocosta Dec 31, 2024

kaisecheng left a comment

mashhurs Dec 24, 2024

mashhurs Dec 24, 2024

donoghuc Dec 30, 2024

elastic-sonarqube bot commented Dec 30, 2024

elasticmachine commented Dec 30, 2024

Apply Jackson stream read constraints defaults at runtime #16832

Are you sure you want to change the base?

Apply Jackson stream read constraints defaults at runtime #16832

Conversation

donoghuc commented Dec 24, 2024 • edited Loading

Release notes

What does this PR do?

Why is it important/What is the impact to the user?

Checklist

Related issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmocosta Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaisecheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Dec 30, 2024

Quality Gate passed

elasticmachine commented Dec 30, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

History

donoghuc commented Dec 24, 2024 •

edited

Loading

edmocosta Dec 30, 2024 •

edited

Loading