Browse Source

Check total field limit at parse time (#73713)

If dynamic mapping updates are enabled, having a crazy number of fields in a document can generate a very large
dynamic mapping update that in turn can make a node go OOM even before the mapping update is validated. This
commit adds a pre-flight check on the total number of fields allowed in a mapping at document parse time, while the
(potentially large) dynamic mapping update is being built.

Relates #73460
Yannick Welsch 4 years ago
parent
commit
d81398a206

+ 45 - 4
server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java

@@ -40,6 +40,7 @@ import java.util.Map;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicReference;
 
+import static org.elasticsearch.index.mapper.MapperService.INDEX_MAPPING_DEPTH_LIMIT_SETTING;
 import static org.elasticsearch.index.mapper.MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchHits;
@@ -131,9 +132,11 @@ public class DynamicMappingIT extends ESIntegTestCase {
     }
 
     public void testPreflightCheckAvoidsMaster() throws InterruptedException {
-        createIndex("index", Settings.builder().put(INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(), 2).build());
+        // can't use INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING as a check here, as that is already checked at parse time,
+        // see testTotalFieldsLimitForDynamicMappingsUpdateCheckedAtDocumentParseTime
+        createIndex("index", Settings.builder().put(INDEX_MAPPING_DEPTH_LIMIT_SETTING.getKey(), 2).build());
         ensureGreen("index");
-        client().prepareIndex("index").setId("1").setSource("field1", "value1").get();
+        client().prepareIndex("index").setId("1").setSource("field1", Map.of("field2", "value1")).get();
 
         final CountDownLatch masterBlockedLatch = new CountDownLatch(1);
         final CountDownLatch indexingCompletedLatch = new CountDownLatch(1);
@@ -154,11 +157,49 @@ public class DynamicMappingIT extends ESIntegTestCase {
         });
 
         masterBlockedLatch.await();
-        final IndexRequestBuilder indexRequestBuilder = client().prepareIndex("index").setId("2").setSource("field2", "value2");
+        final IndexRequestBuilder indexRequestBuilder = client().prepareIndex("index").setId("2").setSource("field1",
+            Map.of("field3", Map.of("field4", "value2")));
         try {
             assertThat(
                 expectThrows(IllegalArgumentException.class, () -> indexRequestBuilder.get(TimeValue.timeValueSeconds(10))).getMessage(),
-                Matchers.containsString("Limit of total fields [2] has been exceeded"));
+                Matchers.containsString("Limit of mapping depth [2] has been exceeded due to object field [field1.field3]"));
+        } finally {
+            indexingCompletedLatch.countDown();
+        }
+    }
+
+    public void testTotalFieldsLimitForDynamicMappingsUpdateCheckedAtDocumentParseTime() throws InterruptedException {
+        createIndex("index", Settings.builder().put(INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(), 2).build());
+        ensureGreen("index");
+        client().prepareIndex("index").setId("1").setSource("field1", "value1").get();
+
+        final CountDownLatch masterBlockedLatch = new CountDownLatch(1);
+        final CountDownLatch indexingCompletedLatch = new CountDownLatch(1);
+
+        internalCluster().getInstance(ClusterService.class, internalCluster().getMasterName()).submitStateUpdateTask("block-state-updates",
+            new ClusterStateUpdateTask() {
+                @Override
+                public ClusterState execute(ClusterState currentState) throws Exception {
+                    masterBlockedLatch.countDown();
+                    indexingCompletedLatch.await();
+                    return currentState;
+                }
+
+                @Override
+                public void onFailure(String source, Exception e) {
+                    throw new AssertionError("unexpected", e);
+                }
+            });
+
+        masterBlockedLatch.await();
+        final IndexRequestBuilder indexRequestBuilder = client().prepareIndex("index").setId("2").setSource("field2", "value2");
+        try {
+            Exception e = expectThrows(MapperParsingException.class, () -> indexRequestBuilder.get(TimeValue.timeValueSeconds(10)));
+            assertThat(e.getMessage(),
+                Matchers.containsString("failed to parse"));
+            assertThat(e.getCause(), instanceOf(IllegalArgumentException.class));
+            assertThat(e.getCause().getMessage(),
+                Matchers.containsString("Limit of total fields [2] has been exceeded while adding new fields [1]"));
         } finally {
             indexingCompletedLatch.countDown();
         }

+ 7 - 2
server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java

@@ -207,8 +207,13 @@ public final class MappingLookup {
     }
 
     private void checkFieldLimit(long limit) {
-        if (fieldMappers.size() + objectMappers.size() - mapping.getSortedMetadataMappers().length > limit) {
-            throw new IllegalArgumentException("Limit of total fields [" + limit + "] has been exceeded");
+        checkFieldLimit(limit, 0);
+    }
+
+    void checkFieldLimit(long limit, int additionalFieldsToAdd) {
+        if (fieldMappers.size() + objectMappers.size() + additionalFieldsToAdd - mapping.getSortedMetadataMappers().length > limit) {
+            throw new IllegalArgumentException("Limit of total fields [" + limit + "] has been exceeded" +
+                (additionalFieldsToAdd > 0 ? " while adding new fields [" + additionalFieldsToAdd + "]" : ""));
         }
     }
 

+ 9 - 0
server/src/main/java/org/elasticsearch/index/mapper/ParseContext.java

@@ -306,6 +306,7 @@ public abstract class ParseContext {
         private final SourceToParse sourceToParse;
         private final long maxAllowedNumNestedDocs;
         private final List<Mapper> dynamicMappers = new ArrayList<>();
+        private final Set<String> newFieldsSeen = new HashSet<>();
         private final Map<String, ObjectMapper> dynamicObjectMappers = new HashMap<>();
         private final List<RuntimeField> dynamicRuntimeFields = new ArrayList<>();
         private final Set<String> ignoredFields = new HashSet<>();
@@ -427,6 +428,14 @@ public abstract class ParseContext {
 
         @Override
         public void addDynamicMapper(Mapper mapper) {
+            // eagerly check field name limit here to avoid OOM errors
+            // only check fields that are not already mapped or tracked in order to avoid hitting field limit too early via double-counting
+            // note that existing fields can also receive dynamic mapping updates (e.g. constant_keyword to fix the value)
+            if (mappingLookup.getMapper(mapper.name()) == null &&
+                mappingLookup.objectMappers().containsKey(mapper.name()) == false &&
+                newFieldsSeen.add(mapper.name())) {
+                mappingLookup.checkFieldLimit(indexSettings.getMappingTotalFieldsLimit(), newFieldsSeen.size());
+            }
             if (mapper instanceof ObjectMapper) {
                 dynamicObjectMappers.put(mapper.name(), (ObjectMapper)mapper);
             }

+ 21 - 0
x-pack/plugin/mapper-constant-keyword/src/internalClusterTest/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapperTests.java

@@ -11,6 +11,7 @@ import org.apache.lucene.search.MatchNoDocsQuery;
 import org.apache.lucene.search.Query;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.compress.CompressedXContent;
+import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
@@ -28,6 +29,7 @@ import java.io.IOException;
 import java.util.Collection;
 import java.util.List;
 
+import static org.elasticsearch.index.mapper.MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING;
 import static org.hamcrest.Matchers.instanceOf;
 
 public class ConstantKeywordFieldMapperTests extends MapperTestCase {
@@ -102,6 +104,25 @@ public class ConstantKeywordFieldMapperTests extends MapperTestCase {
         assertNull(doc.dynamicMappingsUpdate());
     }
 
+    public void testDynamicValueFieldLimit() throws Exception {
+        MapperService mapperService = createMapperService(
+            Settings.builder().put(INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(), 1).build(),
+            fieldMapping(b -> b.field("type", "constant_keyword")));
+
+        ParsedDocument doc = mapperService.documentMapper().parse(source(b -> b.field("field", "foo")));
+        assertNull(doc.rootDoc().getField("field"));
+        assertNotNull(doc.dynamicMappingsUpdate());
+
+        CompressedXContent mappingUpdate = new CompressedXContent(Strings.toString(doc.dynamicMappingsUpdate()));
+        DocumentMapper updatedMapper = mapperService.merge("_doc", mappingUpdate, MergeReason.MAPPING_UPDATE);
+        String expectedMapping = Strings.toString(fieldMapping(b -> b.field("type", "constant_keyword").field("value", "foo")));
+        assertEquals(expectedMapping, updatedMapper.mappingSource().toString());
+
+        doc = updatedMapper.parse(source(b -> b.field("field", "foo")));
+        assertNull(doc.rootDoc().getField("field"));
+        assertNull(doc.dynamicMappingsUpdate());
+    }
+
     public void testBadValues() {
         {
             MapperParsingException e = expectThrows(MapperParsingException.class, () -> createMapperService(fieldMapping(b -> {