Skip to content

Commit

Permalink
jackson stream read constraints: code-based defaults (#16854) (#16883)
Browse files Browse the repository at this point in the history 
* Revert "Apply Jackson stream read constraints defaults at runtime (#16832)"

This reverts commit cc608eb.

* jackson stream read constraints: code-based defaults

refactors stream read constraints to couple default values with their
associated overrides, which allows us to have more descriptive logging
that includes provenance of the value that has been applied.

(cherry picked from commit 1639290)

Co-authored-by: Ry Biesemeyer <[email protected]>
  • Loading branch information
@github-actions @yaauie
github-actions[bot] and yaauie authored Jan 9, 2025
1 parent 871d62f commit 6b45a21
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 71 deletions.
4 changes: 2 additions & 2 deletions config/jvm.options
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,11 @@
# text values with sizes less than or equal to this limit will be treated as invalid.
# This value should be higher than `logstash.jackson.stream-read-constraints.max-number-length`.
# The jackson library defaults to 20000000 or 20MB, whereas Logstash defaults to 200MB or 200000000 characters.
-Dlogstash.jackson.stream-read-constraints.max-string-length=200000000
#-Dlogstash.jackson.stream-read-constraints.max-string-length=200000000
#
# Sets the maximum number length (in chars or bytes, depending on input context).
# The jackson library defaults to 1000, whereas Logstash defaults to 10000.
-Dlogstash.jackson.stream-read-constraints.max-number-length=10000
#-Dlogstash.jackson.stream-read-constraints.max-number-length=10000
#
# Sets the maximum nesting depth. The depth is a count of objects and arrays that have not
# been closed, `{` and `[` respectively.
Expand Down
83 changes: 52 additions & 31 deletions logstash-core/src/main/java/org/logstash/jackson/StreamReadConstraintsUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import java.util.*;
import javax.annotation.Nullable;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Properties;
import java.util.Set;
import java.util.function.BiConsumer;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Collectors;

public class StreamReadConstraintsUtil {
Expand All @@ -18,38 +24,51 @@ 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
);

enum Override {
MAX_STRING_LENGTH(StreamReadConstraints.Builder::maxStringLength, StreamReadConstraints::getMaxStringLength),
MAX_NUMBER_LENGTH(StreamReadConstraints.Builder::maxNumberLength, StreamReadConstraints::getMaxNumberLength),
MAX_NESTING_DEPTH(StreamReadConstraints.Builder::maxNestingDepth, StreamReadConstraints::getMaxNestingDepth),
MAX_STRING_LENGTH(200_000_000, StreamReadConstraints.Builder::maxStringLength, StreamReadConstraints::getMaxStringLength),
MAX_NUMBER_LENGTH(10_000, StreamReadConstraints.Builder::maxNumberLength, StreamReadConstraints::getMaxNumberLength),
MAX_NESTING_DEPTH(1_000, StreamReadConstraints.Builder::maxNestingDepth, StreamReadConstraints::getMaxNestingDepth),
;

static final String PROP_PREFIX = "logstash.jackson.stream-read-constraints.";

final String propertyName;
final int defaultValue;
private final IntValueApplicator applicator;
private final IntValueObserver observer;

Override(final IntValueApplicator applicator,
Override(final int defaultValue,
final IntValueApplicator applicator,
final IntValueObserver observer) {
this.propertyName = PROP_PREFIX + this.name().toLowerCase().replace('_', '-');
this.defaultValue = defaultValue;
this.applicator = applicator;
this.observer = observer;
}

int resolve(final Integer specifiedValue) {
return Objects.requireNonNullElse(specifiedValue, defaultValue);
}

void apply(final StreamReadConstraints.Builder constraintsBuilder,
@Nullable final Integer specifiedValue) {
this.applicator.applyIntValue(constraintsBuilder, resolve(specifiedValue));
}

int observe(final StreamReadConstraints constraints) {
return this.observer.observeIntValue(constraints);
}

@FunctionalInterface
interface IntValueObserver extends Function<StreamReadConstraints, Integer> {}
interface IntValueObserver {
int observeIntValue(final StreamReadConstraints constraints);
}

@FunctionalInterface
interface IntValueApplicator extends BiFunction<StreamReadConstraints.Builder, Integer, StreamReadConstraints.Builder> {}
interface IntValueApplicator {
@SuppressWarnings("UnusedReturnValue")
StreamReadConstraints.Builder applyIntValue(final StreamReadConstraints.Builder constraintsBuilder, final int value);
}
}

/**
Expand Down Expand Up @@ -86,9 +105,7 @@ 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));
eachOverride((override, value) -> override.applicator.apply(builder, value));
eachOverride((override, specifiedValue) -> override.apply(builder, specifiedValue));

this.configuredStreamReadConstraints = builder.build();
}
Expand All @@ -106,11 +123,14 @@ public void validateIsGlobalDefault() {
private void validate(final StreamReadConstraints streamReadConstraints) {
final List<String> fatalIssues = new ArrayList<>();
eachOverride((override, specifiedValue) -> {
final Integer effectiveValue = override.observer.apply(streamReadConstraints);
if (Objects.equals(specifiedValue, effectiveValue)) {
logger.info("Jackson default value override `{}` configured to `{}`", override.propertyName, specifiedValue);
final int effectiveValue = override.observe(streamReadConstraints);
final int expectedValue = override.resolve(specifiedValue);

if (!Objects.equals(effectiveValue, expectedValue)) {
fatalIssues.add(String.format("`%s` (expected: `%s`, actual: `%s`)", override.propertyName, expectedValue, effectiveValue));
} else {
fatalIssues.add(String.format("`%s` (expected: `%s`, actual: `%s`)", override.propertyName, specifiedValue, effectiveValue));
final String reason = Objects.nonNull(specifiedValue) ? "system properties" : "logstash default";
logger.info("Jackson default value override `{}` configured to `{}` ({})", override.propertyName, effectiveValue, reason);
}
});
for (String unsupportedProperty : getUnsupportedProperties()) {
Expand All @@ -124,13 +144,14 @@ private void validate(final StreamReadConstraints streamReadConstraints) {
void eachOverride(BiConsumer<Override,Integer> overrideIntegerBiConsumer) {
for (Override override : Override.values()) {
final String propValue = this.propertyOverrides.get(override.propertyName);
if (propValue != null) {
try {
int intValue = Integer.parseInt(propValue);
overrideIntegerBiConsumer.accept(override, intValue);
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException(String.format("System property `%s` must be positive integer value. Received: `%s`", override.propertyName, propValue), e);
}

try {
final Integer explicitValue = Optional.ofNullable(propValue)
.map(Integer::parseInt)
.orElse(null);
overrideIntegerBiConsumer.accept(override, explicitValue);
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException(String.format("System property `%s` must be positive integer value. Received: `%s`", override.propertyName, propValue), e);
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions logstash-core/src/test/java/org/logstash/ObjectMappersTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,9 @@ public void testLog4jOMRegisterRubyBasicObjectSerializersFirst() {
assertNotNull(found);
assertTrue("RubyBasicObjectSerializer must be registered before others non-default serializers", found instanceof RubyBasicObjectSerializer);
}

@Test
public void testStreamReadConstraintsApplication() {
ObjectMappers.CONFIGURED_STREAM_READ_CONSTRAINTS.validateIsGlobalDefault();
}
}
87 changes: 49 additions & 38 deletions logstash-core/src/test/java/org/logstash/jackson/StreamReadConstraintsUtilTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@
import org.apache.logging.log4j.test.appender.ListAppender;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;

import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.Properties;
import java.util.function.BiConsumer;

import org.apache.logging.log4j.Level;
import org.logstash.ObjectMappers;

import static org.assertj.core.api.Assertions.*;
import static org.logstash.jackson.StreamReadConstraintsUtil.Override.*;
Expand All @@ -23,13 +27,12 @@ public class StreamReadConstraintsUtilTest {
@ClassRule
public static final LoggerContextRule LOGGER_CONTEXT_RULE = new LoggerContextRule("log4j2-log-stream-read-constraints.xml");

@Rule
public final DefaultsRestorer defaultsRestorer = new DefaultsRestorer();

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 @@ -39,6 +42,7 @@ public void setUpLoggingListAppender() {

@Test
public void configuresDefaultsByDefault() {
Objects.requireNonNull(ObjectMappers.CBOR_MAPPER); // force static init
StreamReadConstraintsUtil.fromSystemProperties().validateIsGlobalDefault();
}

Expand All @@ -55,8 +59,8 @@ public void configuresMaxStringLength() {
assertThat(configuredConstraints).as("inherited defaults")
.returns(defaults.getMaxDocumentLength(), from(StreamReadConstraints::getMaxDocumentLength))
.returns(defaults.getMaxNameLength(), from(StreamReadConstraints::getMaxNameLength))
.returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth))
.returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength));
.returns(MAX_NESTING_DEPTH.defaultValue, from(StreamReadConstraints::getMaxNestingDepth))
.returns(MAX_NUMBER_LENGTH.defaultValue, from(StreamReadConstraints::getMaxNumberLength));

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

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

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

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

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

Expand Down Expand Up @@ -187,8 +191,6 @@ public void validatesApplication() {
configuredUtil.validateIsGlobalDefault();
});

System.out.format("OK%n");

assertLogObserved(Level.INFO, "override `" + MAX_NESTING_DEPTH.propertyName + "` configured to `1010`");
assertLogObserved(Level.INFO, "override `" + MAX_STRING_LENGTH.propertyName + "` configured to `1011`");
assertLogObserved(Level.INFO, "override `" + MAX_NUMBER_LENGTH.propertyName + "` configured to `1110`");
Expand All @@ -198,28 +200,17 @@ public void validatesApplication() {
}

@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");
public void validatesApplicationWithDefaults() {
final Properties properties = new Properties(); // empty -- no overrides

StreamReadConstraintsUtil util = new StreamReadConstraintsUtil(props, this.observedLogger);
StreamReadConstraints constraints = util.get();
fromProperties(properties, (configuredUtil, defaults) -> {
configuredUtil.applyAsGlobalDefault();
configuredUtil.validateIsGlobalDefault();

assertThat(constraints)
.returns(100, from(StreamReadConstraints::getMaxStringLength))
.returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength))
.returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth));
assertLogObserved(Level.INFO, "override `" + MAX_NESTING_DEPTH.propertyName + "` configured to `" + MAX_NESTING_DEPTH.defaultValue + "` (logstash default)");
assertLogObserved(Level.INFO, "override `" + MAX_STRING_LENGTH.propertyName + "` configured to `" + MAX_STRING_LENGTH.defaultValue + "` (logstash default)");
assertLogObserved(Level.INFO, "override `" + MAX_NUMBER_LENGTH.propertyName + "` configured to `" + MAX_NUMBER_LENGTH.defaultValue + "` (logstash default)");
});
}

private void assertLogObserved(final Level level, final String... messageFragments) {
Expand All @@ -230,13 +221,33 @@ private void assertLogObserved(final Level level, final String... messageFragmen
.anySatisfy(logEvent -> assertThat(logEvent.getMessage().getFormattedMessage()).contains(messageFragments));
}

private synchronized void fromProperties(final Properties properties, BiConsumer<StreamReadConstraintsUtil, StreamReadConstraints> defaultsConsumer) {
private synchronized void fromProperties(final Properties properties,
final BiConsumer<StreamReadConstraintsUtil, StreamReadConstraints> defaultsConsumer) {
final StreamReadConstraints defaults = StreamReadConstraints.defaults();
try {
final StreamReadConstraintsUtil util = new StreamReadConstraintsUtil(properties, this.observedLogger);
defaultsConsumer.accept(util, defaults);
} finally {
StreamReadConstraints.overrideDefaultStreamReadConstraints(defaults);
final StreamReadConstraintsUtil util = new StreamReadConstraintsUtil(properties, this.observedLogger);

defaultsConsumer.accept(util, defaults);
}

/**
* A TestRule to snapshot the current defaults from jackson prior to test execution, and to
* ensure that snapshot is restored after test execution has completed.
*/
public static class DefaultsRestorer extends org.junit.rules.ExternalResource {
private StreamReadConstraints snapshot;

public StreamReadConstraints getSnapshot() {
return this.snapshot;
}

@Override
protected void before() throws Throwable {
this.snapshot = StreamReadConstraints.defaults();
}

@Override
protected void after() {
StreamReadConstraints.overrideDefaultStreamReadConstraints(this.snapshot);
}
}
}

0 comments on commit 6b45a21

Please sign in to comment.