From 896317fe36d8dbc236a837fb07704fb8e26dcd4d Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Wed, 4 Jul 2018 10:25:00 +0100 Subject: [PATCH] [ML] Limit ML filter items to 10K (#31731) Add hard limit to the number of items a filter may have. This serves to protect from excessive overhead due to the filters taking too much memory or lookups becoming too expensive. --- .../xpack/core/ml/job/config/MlFilter.java | 15 ++++++++++- .../xpack/core/ml/job/messages/Messages.java | 1 + .../core/ml/job/config/MlFilterTests.java | 27 +++++++++++++++++-- 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java index c55ba401a2f..48051fa4733 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java @@ -29,6 +29,15 @@ import java.util.TreeSet; public class MlFilter implements ToXContentObject, Writeable { + /** + * The max number of items allowed per filter. + * Limiting the number of items protects users + * from running into excessive overhead due to + * filters using too much memory and lookups + * becoming too expensive. + */ + private static final int MAX_ITEMS = 10000; + public static final String DOCUMENT_ID_PREFIX = "filter_"; public static final String FILTER_TYPE = "filter"; @@ -62,7 +71,7 @@ public class MlFilter implements ToXContentObject, Writeable { private MlFilter(String id, String description, SortedSet items) { this.id = Objects.requireNonNull(id); this.description = description; - this.items = Objects.requireNonNull(items, ITEMS.getPreferredName() + " must not be null"); + this.items = Objects.requireNonNull(items); } public MlFilter(StreamInput in) throws IOException { @@ -182,9 +191,13 @@ public class MlFilter implements ToXContentObject, Writeable { public MlFilter build() { ExceptionsHelper.requireNonNull(id, MlFilter.ID.getPreferredName()); + ExceptionsHelper.requireNonNull(items, MlFilter.ITEMS.getPreferredName()); if (!MlStrings.isValidId(id)) { throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.INVALID_ID, ID.getPreferredName(), id)); } + if (items.size() > MAX_ITEMS) { + throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.FILTER_CONTAINS_TOO_MANY_ITEMS, id, MAX_ITEMS)); + } return new MlFilter(id, description, items); } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java index f0329051fed..259d2d06a9c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java @@ -43,6 +43,7 @@ public final class Messages { "Datafeed frequency [{0}] must be a multiple of the aggregation interval [{1}]"; public static final String FILTER_NOT_FOUND = "No filter with id [{0}] exists"; + public static final String FILTER_CONTAINS_TOO_MANY_ITEMS = "Filter [{0}] contains too many items; up to [{1}] items are allowed"; public static final String INCONSISTENT_ID = "Inconsistent {0}; ''{1}'' specified in the body differs from ''{2}'' specified as a URL argument"; diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java index a89250330f0..45ba47281a2 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java @@ -13,6 +13,8 @@ import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.AbstractSerializingTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import java.util.SortedSet; import java.util.TreeSet; @@ -71,9 +73,9 @@ public class MlFilterTests extends AbstractSerializingTestCase { } public void testNullItems() { - NullPointerException ex = expectThrows(NullPointerException.class, + Exception ex = expectThrows(IllegalArgumentException.class, () -> MlFilter.builder(randomValidFilterId()).setItems((SortedSet) null).build()); - assertEquals(MlFilter.ITEMS.getPreferredName() + " must not be null", ex.getMessage()); + assertEquals("[items] must not be null.", ex.getMessage()); } public void testDocumentId() { @@ -102,6 +104,27 @@ public class MlFilterTests extends AbstractSerializingTestCase { assertThat(e.getMessage(), startsWith("Invalid filter_id; 'Invalid id' can contain lowercase")); } + public void testTooManyItems() { + List items = new ArrayList<>(10001); + for (int i = 0; i < 10001; ++i) { + items.add("item_" + i); + } + ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, + () -> MlFilter.builder("huge").setItems(items).build()); + assertThat(e.getMessage(), startsWith("Filter [huge] contains too many items")); + } + + public void testGivenItemsAreMaxAllowed() { + List items = new ArrayList<>(10000); + for (int i = 0; i < 10000; ++i) { + items.add("item_" + i); + } + + MlFilter hugeFilter = MlFilter.builder("huge").setItems(items).build(); + + assertThat(hugeFilter.getItems().size(), equalTo(items.size())); + } + public void testItemsAreSorted() { MlFilter filter = MlFilter.builder("foo").setItems("c", "b", "a").build(); assertThat(filter.getItems(), contains("a", "b", "c"));