Skip to content

Apply Jackson stream read constraints defaults at runtime #16832

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 2, 2025
Merged

Apply Jackson stream read constraints defaults at runtime #16832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions logstash-core/src/main/java/org/logstash/jackson/StreamReadConstraintsUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ public class StreamReadConstraintsUtil {

private StreamReadConstraints configuredStreamReadConstraints;

// Provide default values for Jackson constraints in the case they are
// not specified in configuration file.
private 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
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, keeping the comment is good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@edmocosta edmocosta Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @edmocosta here - let's keep it. While I wouldn't expect the upcoming changes in Jackson defaults to 500 to impact us, this will retain consistency with what we know is already working.

);

enum Override {
MAX_STRING_LENGTH(StreamReadConstraints.Builder::maxStringLength, StreamReadConstraints::getMaxStringLength),
MAX_NUMBER_LENGTH(StreamReadConstraints.Builder::maxNumberLength, StreamReadConstraints::getMaxNumberLength),
Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. 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.
  1. 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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

eachOverride((override, value) -> override.applicator.apply(builder, value));

this.configuredStreamReadConstraints = builder.build();
Expand Down
41 changes: 35 additions & 6 deletions logstash-core/src/test/java/org/logstash/jackson/StreamReadConstraintsUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ public class StreamReadConstraintsUtilTest {
private ListAppender listAppender;
private Logger observedLogger;

private static final int DEFAULT_MAX_STRING_LENGTH = 200_000_000;
private static final int DEFAULT_MAX_NUMBER_LENGTH = 10_000;
private static final int DEFAULT_MAX_NESTING_DEPTH = 1_000;

@Before
public void setUpLoggingListAppender() {
int i = 1+16;
Expand All @@ -51,8 +55,8 @@ public void configuresMaxStringLength() {
assertThat(configuredConstraints).as("inherited defaults")
.returns(defaults.getMaxDocumentLength(), from(StreamReadConstraints::getMaxDocumentLength))
.returns(defaults.getMaxNameLength(), from(StreamReadConstraints::getMaxNameLength))
.returns(defaults.getMaxNestingDepth(), from(StreamReadConstraints::getMaxNestingDepth))
.returns(defaults.getMaxNumberLength(), from(StreamReadConstraints::getMaxNumberLength));
.returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth))
.returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength));

assertThatThrownBy(configuredUtil::validateIsGlobalDefault).isInstanceOf(IllegalStateException.class).hasMessageContaining(MAX_STRING_LENGTH.propertyName);

Expand Down Expand Up @@ -94,8 +98,8 @@ public void configuresMaxNumberLength() {
assertThat(configuredConstraints).as("inherited defaults")
.returns(defaults.getMaxDocumentLength(), from(StreamReadConstraints::getMaxDocumentLength))
.returns(defaults.getMaxNameLength(), from(StreamReadConstraints::getMaxNameLength))
.returns(defaults.getMaxNestingDepth(), from(StreamReadConstraints::getMaxNestingDepth))
.returns(defaults.getMaxStringLength(), from(StreamReadConstraints::getMaxStringLength));
.returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth))
.returns(DEFAULT_MAX_STRING_LENGTH, from(StreamReadConstraints::getMaxStringLength));

assertThatThrownBy(configuredUtil::validateIsGlobalDefault).isInstanceOf(IllegalStateException.class).hasMessageContaining(MAX_NUMBER_LENGTH.propertyName);

Expand Down Expand Up @@ -137,8 +141,8 @@ public void configuresMaxNestingDepth() {
assertThat(configuredConstraints).as("inherited defaults")
.returns(defaults.getMaxDocumentLength(), from(StreamReadConstraints::getMaxDocumentLength))
.returns(defaults.getMaxNameLength(), from(StreamReadConstraints::getMaxNameLength))
.returns(defaults.getMaxStringLength(), from(StreamReadConstraints::getMaxStringLength))
.returns(defaults.getMaxNumberLength(), from(StreamReadConstraints::getMaxNumberLength));
.returns(DEFAULT_MAX_STRING_LENGTH, from(StreamReadConstraints::getMaxStringLength))
.returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength));

assertThatThrownBy(configuredUtil::validateIsGlobalDefault).isInstanceOf(IllegalStateException.class).hasMessageContaining(MAX_NESTING_DEPTH.propertyName);

Expand Down Expand Up @@ -193,6 +197,31 @@ public void validatesApplication() {
assertLogObserved(Level.WARN, "override `" + PROP_PREFIX + "unsupported-option1` is unknown and has been ignored");
}

@Test
public void usesJacksonDefaultsWhenNoConfig() {
StreamReadConstraintsUtil util = new StreamReadConstraintsUtil(new Properties(), this.observedLogger);
StreamReadConstraints constraints = util.get();

assertThat(constraints)
.returns(DEFAULT_MAX_STRING_LENGTH, from(StreamReadConstraints::getMaxStringLength))
.returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength))
.returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth));
}

@Test
public void configOverridesDefault() {
Properties props = new Properties();
props.setProperty("logstash.jackson.stream-read-constraints.max-string-length", "100");

StreamReadConstraintsUtil util = new StreamReadConstraintsUtil(props, this.observedLogger);
StreamReadConstraints constraints = util.get();

assertThat(constraints)
.returns(100, from(StreamReadConstraints::getMaxStringLength))
.returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength))
.returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth));
}

private void assertLogObserved(final Level level, final String... messageFragments) {
List<LogEvent> logEvents = listAppender.getEvents();
assertThat(logEvents)
Expand Down
Loading