From 5e0a8087d8d77097726e6b920d565cfa3aa25f32 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Thu, 27 Feb 2020 11:52:27 +0100 Subject: [PATCH 1/2] Add validation for dynamic templates (#51233) Tries to load a `Mapper` instance for the mapping snippet of a dynamic template. This should catch things like using an analyzer that is undefined or mapping attributes that are unused. This is best effort: * If `{{name}}` placeholder is used in the mapping snippet then validation is skipped. * If `match_mapping_type` is not specified then validation is performed for all mapping types. If parsing succeeds with a single mapping type then this the dynamic mapping is considered valid. If is detected that a dynamic template mapping snippet is invalid at mapping update time then the mapping update is failed for indices created on 8.0.0-alpha1 and later. For indices created on prior version a deprecation warning is omitted instead. In 7.x clusters the mapping update will never fail in case of an invalid dynamic template mapping snippet and a deprecation warning will always be omitted. Closes #17411 Closes #24419 Co-authored-by: Adrien Grand --- .../mapping/dynamic/templates.asciidoc | 14 +- docs/reference/release-notes/7.7.asciidoc | 13 ++ .../index/mapper/DynamicTemplate.java | 12 ++ .../index/mapper/MapperService.java | 1 + .../index/mapper/RootObjectMapper.java | 124 ++++++++++- .../index/mapper/RootObjectMapperTests.java | 196 ++++++++++++++++++ 6 files changed, 355 insertions(+), 5 deletions(-) create mode 100644 docs/reference/release-notes/7.7.asciidoc diff --git a/docs/reference/mapping/dynamic/templates.asciidoc b/docs/reference/mapping/dynamic/templates.asciidoc index 2dec3912d2351..fab2034476b65 100644 --- a/docs/reference/mapping/dynamic/templates.asciidoc +++ b/docs/reference/mapping/dynamic/templates.asciidoc @@ -37,6 +37,19 @@ Dynamic templates are specified as an array of named objects: <2> The match conditions can include any of : `match_mapping_type`, `match`, `match_pattern`, `unmatch`, `path_match`, `path_unmatch`. <3> The mapping that the matched field should use. +If a provided mapping contains an invalid mapping snippet then that results in +a validation error. Validation always occurs when applying the dynamic template +at index time or in most cases when updating the dynamic template. + +Whether updating the dynamic template fails when supplying an invalid mapping snippet depends on the following: +* If no `match_mapping_type` has been specified then if the template is valid with one predefined mapping type then + the mapping snippet is considered valid. However if at index time a field that matches with the template is indexed + as a different type then an validation error will occur at index time instead. For example configuring a dynamic + template with no `match_mapping_type` is considered valid as string type, but at index time a field that matches with + the dynamic template is indexed as a long, then at index time a validation error may still occur. +* If the `{{name}}` placeholder is used in the mapping snippet then the validation is skipped when updating + the dynamic template. This is because the field name is unknown at that time. The validation will then occur + when applying the template at index time. Templates are processed in order -- the first matching template wins. When putting new dynamic templates through the <> API, @@ -409,4 +422,3 @@ PUT my_index <1> Like the default dynamic mapping rules, doubles are mapped as floats, which are usually accurate enough, yet require half the disk space. - diff --git a/docs/reference/release-notes/7.7.asciidoc b/docs/reference/release-notes/7.7.asciidoc new file mode 100644 index 0000000000000..2e4fce5af8bcc --- /dev/null +++ b/docs/reference/release-notes/7.7.asciidoc @@ -0,0 +1,13 @@ +[[release-notes-7.7.0]] +== {es} version 7.7.0 + +coming[7.7.0] + +[[breaking-7.7.0]] +[float] +=== Breaking changes + +Mapping:: +* Dynamic mappings in indices created on 8.0 and later have stricter validation at mapping update time and + results in a deprecation warning for indices created in Elasticsearch 7.7.0 and later. + (e.g. incorrect analyzer settings or unknown field types). {pull}51233[#51233] diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java b/server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java index aafe9f6ba03de..868bc123a88c1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java @@ -354,6 +354,18 @@ private List processList(List list, String name, String dynamicType) { return processedList; } + String getName() { + return name; + } + + XContentFieldType getXContentFieldType() { + return xcontentFieldType; + } + + Map getMapping() { + return mapping; + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 6ed6cec179742..fe5678fa335b8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -886,4 +886,5 @@ public synchronized List reloadSearchAnalyzers(AnalysisRegistry registry } return reloadedAnalyzers; } + } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index fad840c2f7653..c7781988f3084 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -19,9 +19,13 @@ package org.elasticsearch.index.mapper; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.elasticsearch.Version; import org.elasticsearch.common.Explicit; import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.xcontent.ToXContent; @@ -34,6 +38,7 @@ import java.util.Collections; import java.util.Iterator; import java.util.List; +import java.util.Locale; import java.util.Map; import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue; @@ -41,6 +46,9 @@ public class RootObjectMapper extends ObjectMapper { + private static final Logger LOGGER = LogManager.getLogger(RootObjectMapper.class); + private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(LOGGER); + public static class Defaults { public static final DateFormatter[] DYNAMIC_DATE_TIME_FORMATTERS = new DateFormatter[]{ @@ -128,7 +136,7 @@ public Mapper.Builder parse(String name, Map node, ParserContext String fieldName = entry.getKey(); Object fieldNode = entry.getValue(); if (parseObjectOrDocumentTypeProperties(fieldName, fieldNode, parserContext, builder) - || processField(builder, fieldName, fieldNode, parserContext.indexVersionCreated())) { + || processField(builder, fieldName, fieldNode, parserContext)) { iterator.remove(); } } @@ -136,7 +144,7 @@ public Mapper.Builder parse(String name, Map node, ParserContext } protected boolean processField(RootObjectMapper.Builder builder, String fieldName, Object fieldNode, - Version indexVersionCreated) { + ParserContext parserContext) { if (fieldName.equals("date_formats") || fieldName.equals("dynamic_date_formats")) { if (fieldNode instanceof List) { List formatters = new ArrayList<>(); @@ -159,7 +167,7 @@ protected boolean processField(RootObjectMapper.Builder builder, String fieldNam // "template_1" : { // "match" : "*_test", // "match_mapping_type" : "string", - // "mapping" : { "type" : "string", "store" : "yes" } + // "mapping" : { "type" : "keyword", "store" : "yes" } // } // } // ] @@ -176,8 +184,9 @@ protected boolean processField(RootObjectMapper.Builder builder, String fieldNam Map.Entry entry = tmpl.entrySet().iterator().next(); String templateName = entry.getKey(); Map templateParams = (Map) entry.getValue(); - DynamicTemplate template = DynamicTemplate.parse(templateName, templateParams, indexVersionCreated); + DynamicTemplate template = DynamicTemplate.parse(templateName, templateParams, parserContext.indexVersionCreated()); if (template != null) { + validateDynamicTemplate(parserContext, template); templates.add(template); } } @@ -326,4 +335,111 @@ protected void doXContent(XContentBuilder builder, ToXContent.Params params) thr builder.field("numeric_detection", numericDetection.value()); } } + + private static void validateDynamicTemplate(Mapper.TypeParser.ParserContext parserContext, + DynamicTemplate dynamicTemplate) { + + if (containsSnippet(dynamicTemplate.getMapping(), "{name}")) { + // Can't validate template, because field names can't be guessed up front. + return; + } + + final XContentFieldType[] types; + if (dynamicTemplate.getXContentFieldType() != null) { + types = new XContentFieldType[]{dynamicTemplate.getXContentFieldType()}; + } else { + types = XContentFieldType.values(); + } + + Exception lastError = null; + boolean dynamicTemplateInvalid = true; + + for (XContentFieldType contentFieldType : types) { + String defaultDynamicType = contentFieldType.defaultMappingType(); + String mappingType = dynamicTemplate.mappingType(defaultDynamicType); + Mapper.TypeParser typeParser = parserContext.typeParser(mappingType); + if (typeParser == null) { + lastError = new IllegalArgumentException("No mapper found for type [" + mappingType + "]"); + continue; + } + + Map fieldTypeConfig = dynamicTemplate.mappingForName("__dummy__", defaultDynamicType); + fieldTypeConfig.remove("type"); + try { + Mapper.Builder dummyBuilder = typeParser.parse("__dummy__", fieldTypeConfig, parserContext); + if (fieldTypeConfig.isEmpty()) { + Settings indexSettings = parserContext.mapperService().getIndexSettings().getSettings(); + BuilderContext builderContext = new BuilderContext(indexSettings, new ContentPath(1)); + dummyBuilder.build(builderContext); + dynamicTemplateInvalid = false; + break; + } else { + lastError = new IllegalArgumentException("Unused mapping attributes [" + fieldTypeConfig + "]"); + } + } catch (Exception e) { + lastError = e; + } + } + + final boolean shouldEmitDeprecationWarning = parserContext.indexVersionCreated().onOrAfter(Version.V_7_7_0); + if (dynamicTemplateInvalid && shouldEmitDeprecationWarning) { + String message = String.format(Locale.ROOT, "dynamic template [%s] has invalid content [%s]", + dynamicTemplate.getName(), Strings.toString(dynamicTemplate)); + + final String deprecationMessage; + if (lastError != null) { + deprecationMessage = String.format(Locale.ROOT, "%s, caused by [%s]", message, lastError.getMessage()); + } else { + deprecationMessage = message; + } + DEPRECATION_LOGGER.deprecatedAndMaybeLog("invalid_dynamic_template", deprecationMessage); + } + } + + private static boolean containsSnippet(Map map, String snippet) { + for (Map.Entry entry : map.entrySet()) { + String key = entry.getKey().toString(); + if (key.contains(snippet)) { + return true; + } + + Object value = entry.getValue(); + if (value instanceof Map) { + if (containsSnippet((Map) value, snippet)) { + return true; + } + } else if (value instanceof List) { + if (containsSnippet((List) value, snippet)) { + return true; + } + } else if (value instanceof String) { + String valueString = (String) value; + if (valueString.contains(snippet)) { + return true; + } + } + } + + return false; + } + + private static boolean containsSnippet(List list, String snippet) { + for (Object value : list) { + if (value instanceof Map) { + if (containsSnippet((Map) value, snippet)) { + return true; + } + } else if (value instanceof List) { + if (containsSnippet((List) value, snippet)) { + return true; + } + } else if (value instanceof String) { + String valueString = (String) value; + if (valueString.contains(snippet)) { + return true; + } + } + } + return false; + } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java index fbd9de02fefe5..cfa52b9818fda 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -19,14 +19,22 @@ package org.elasticsearch.index.mapper; +import org.elasticsearch.Version; +import org.elasticsearch.cluster.metadata.IndexMetaData; 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.common.xcontent.XContentFactory; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.test.ESSingleNodeTestCase; import java.util.Arrays; +import static org.elasticsearch.test.VersionUtils.randomVersionBetween; +import static org.hamcrest.Matchers.both; +import static org.hamcrest.Matchers.containsString; + public class RootObjectMapperTests extends ESSingleNodeTestCase { public void testNumericDetection() throws Exception { @@ -200,4 +208,192 @@ public void testIllegalDynamicTemplates() throws Exception { () -> parser.parse("type", new CompressedXContent(mapping))); assertEquals("Dynamic template syntax error. An array of named objects is expected.", e.getMessage()); } + + public void testIllegalDynamicTemplateUnknownFieldType() throws Exception { + XContentBuilder mapping = XContentFactory.jsonBuilder(); + mapping.startObject(); + { + mapping.startObject("type"); + mapping.startArray("dynamic_templates"); + { + mapping.startObject(); + mapping.startObject("my_template"); + mapping.field("match_mapping_type", "string"); + mapping.startObject("mapping"); + mapping.field("type", "string"); + mapping.endObject(); + mapping.endObject(); + mapping.endObject(); + } + mapping.endArray(); + mapping.endObject(); + } + mapping.endObject(); + MapperService mapperService = createIndex("test").mapperService(); + DocumentMapper mapper = mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE); + assertThat(mapper.mappingSource().toString(), containsString("\"type\":\"string\"")); + assertWarnings("dynamic template [my_template] has invalid content [{\"match_mapping_type\":\"string\",\"mapping\":{\"type\":" + + "\"string\"}}], caused by [No mapper found for type [string]]"); + } + + public void testIllegalDynamicTemplateUnknownAttribute() throws Exception { + XContentBuilder mapping = XContentFactory.jsonBuilder(); + mapping.startObject(); + { + mapping.startObject("type"); + mapping.startArray("dynamic_templates"); + { + mapping.startObject(); + mapping.startObject("my_template"); + mapping.field("match_mapping_type", "string"); + mapping.startObject("mapping"); + mapping.field("type", "keyword"); + mapping.field("foo", "bar"); + mapping.endObject(); + mapping.endObject(); + mapping.endObject(); + } + mapping.endArray(); + mapping.endObject(); + } + mapping.endObject(); + MapperService mapperService = createIndex("test").mapperService(); + DocumentMapper mapper = mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE); + assertThat(mapper.mappingSource().toString(), containsString("\"foo\":\"bar\"")); + assertWarnings("dynamic template [my_template] has invalid content [{\"match_mapping_type\":\"string\",\"mapping\":{" + + "\"foo\":\"bar\",\"type\":\"keyword\"}}], caused by [Unused mapping attributes [{foo=bar}]]"); + } + + public void testIllegalDynamicTemplateInvalidAttribute() throws Exception { + XContentBuilder mapping = XContentFactory.jsonBuilder(); + mapping.startObject(); + { + mapping.startObject("type"); + mapping.startArray("dynamic_templates"); + { + mapping.startObject(); + mapping.startObject("my_template"); + mapping.field("match_mapping_type", "string"); + mapping.startObject("mapping"); + mapping.field("type", "text"); + mapping.field("analyzer", "foobar"); + mapping.endObject(); + mapping.endObject(); + mapping.endObject(); + } + mapping.endArray(); + mapping.endObject(); + } + mapping.endObject(); + MapperService mapperService = createIndex("test").mapperService(); + DocumentMapper mapper = mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE); + assertThat(mapper.mappingSource().toString(), containsString("\"analyzer\":\"foobar\"")); + assertWarnings("dynamic template [my_template] has invalid content [{\"match_mapping_type\":\"string\",\"mapping\":{" + + "\"analyzer\":\"foobar\",\"type\":\"text\"}}], caused by [analyzer [foobar] not found for field [__dummy__]]"); + } + + public void testIllegalDynamicTemplateNoMappingType() throws Exception { + MapperService mapperService; + + { + XContentBuilder mapping = XContentFactory.jsonBuilder(); + mapping.startObject(); + { + mapping.startObject("type"); + mapping.startArray("dynamic_templates"); + { + mapping.startObject(); + mapping.startObject("my_template"); + if (randomBoolean()) { + mapping.field("match_mapping_type", "*"); + } else { + mapping.field("match", "string_*"); + } + mapping.startObject("mapping"); + mapping.field("type", "{dynamic_type}"); + mapping.field("index_phrases", true); + mapping.endObject(); + mapping.endObject(); + mapping.endObject(); + } + mapping.endArray(); + mapping.endObject(); + } + mapping.endObject(); + mapperService = createIndex("test").mapperService(); + DocumentMapper mapper = + mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE); + assertThat(mapper.mappingSource().toString(), containsString("\"index_phrases\":true")); + } + { + boolean useMatchMappingType = randomBoolean(); + XContentBuilder mapping = XContentFactory.jsonBuilder(); + mapping.startObject(); + { + mapping.startObject("type"); + mapping.startArray("dynamic_templates"); + { + mapping.startObject(); + mapping.startObject("my_template"); + if (useMatchMappingType) { + mapping.field("match_mapping_type", "*"); + } else { + mapping.field("match", "string_*"); + } + mapping.startObject("mapping"); + mapping.field("type", "{dynamic_type}"); + mapping.field("foo", "bar"); + mapping.endObject(); + mapping.endObject(); + mapping.endObject(); + } + mapping.endArray(); + mapping.endObject(); + } + mapping.endObject(); + DocumentMapper mapper = mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE); + assertThat(mapper.mappingSource().toString(), containsString("\"foo\":\"bar\"")); + if (useMatchMappingType) { + assertWarnings("dynamic template [my_template] has invalid content [{\"match_mapping_type\":\"*\",\"mapping\":{" + + "\"foo\":\"bar\",\"type\":\"{dynamic_type}\"}}], caused by [Unused mapping attributes [{foo=bar}]]"); + } else { + assertWarnings("dynamic template [my_template] has invalid content [{\"match\":\"string_*\",\"mapping\":{" + + "\"foo\":\"bar\",\"type\":\"{dynamic_type}\"}}], caused by [Unused mapping attributes [{foo=bar}]]"); + } + } + } + + @Override + protected boolean forbidPrivateIndexSettings() { + return false; + } + + public void testIllegalDynamicTemplatePre7Dot7Index() throws Exception { + XContentBuilder mapping = XContentFactory.jsonBuilder(); + mapping.startObject(); + { + mapping.startObject("type"); + mapping.startArray("dynamic_templates"); + { + mapping.startObject(); + mapping.startObject("my_template"); + mapping.field("match_mapping_type", "string"); + mapping.startObject("mapping"); + mapping.field("type", "string"); + mapping.endObject(); + mapping.endObject(); + mapping.endObject(); + } + mapping.endArray(); + mapping.endObject(); + } + mapping.endObject(); + Version createdVersion = randomVersionBetween(random(), Version.V_7_0_0, Version.V_7_6_0); + Settings indexSettings = Settings.builder() + .put(IndexMetaData.SETTING_INDEX_VERSION_CREATED.getKey(), createdVersion) + .build(); + MapperService mapperService = createIndex("test", indexSettings).mapperService(); + DocumentMapper mapper = mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE); + assertThat(mapper.mappingSource().toString(), containsString("\"type\":\"string\"")); + } } From 407abe1d1ee94b385a8fdf9dcd490c8250c88ef4 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Thu, 27 Feb 2020 14:39:43 +0100 Subject: [PATCH 2/2] fixed style violations --- .../org/elasticsearch/index/mapper/RootObjectMapperTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java index cfa52b9818fda..00fbbf3744440 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -32,7 +32,6 @@ import java.util.Arrays; import static org.elasticsearch.test.VersionUtils.randomVersionBetween; -import static org.hamcrest.Matchers.both; import static org.hamcrest.Matchers.containsString; public class RootObjectMapperTests extends ESSingleNodeTestCase { @@ -351,7 +350,8 @@ public void testIllegalDynamicTemplateNoMappingType() throws Exception { mapping.endObject(); } mapping.endObject(); - DocumentMapper mapper = mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE); + DocumentMapper mapper = + mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE); assertThat(mapper.mappingSource().toString(), containsString("\"foo\":\"bar\"")); if (useMatchMappingType) { assertWarnings("dynamic template [my_template] has invalid content [{\"match_mapping_type\":\"*\",\"mapping\":{" +