[7.8] Disallow merging existing mapping field definitions in templates (#57701) (#57823)

dakrone · elasticmachine · web-flow · commit eda4da189bc2 · 2020-06-08T10:47:05.000-06:00
Co-authored-by: Elastic Machine &lt;elasticmachine@users.noreply.github.com&gt;
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml
@@ -14,7 +14,7 @@
               number_of_replicas: 1
             mappings:
               properties:
-                field2:
+                field1:
                   type: text
             aliases:
               aliasname:
@@ -47,9 +47,8 @@
               index.number_of_shards: 2
             mappings:
               properties:
-                field:
-                  type: keyword
-                  ignore_above: 255
+                field3:
+                  type: integer
             aliases:
               my_alias: {}
               aliasname:
@@ -78,8 +77,9 @@
   - match: {bar-baz.settings.index.number_of_shards: "2"}
   - match: {bar-baz.settings.index.number_of_replicas: "0"}
   - match: {bar-baz.settings.index.priority: "17"}
-  - match: {bar-baz.mappings.properties.field: {type: keyword, ignore_above: 255}}
+  - match: {bar-baz.mappings.properties.field1: {type: text}}
   - match: {bar-baz.mappings.properties.field2: {type: keyword}}
+  - match: {bar-baz.mappings.properties.field3: {type: integer}}
   - match: {bar-baz.mappings.properties.foo: {type: keyword}}
   - match: {bar-baz.aliases.aliasname: {filter: {match_all: {}}}}
   - match: {bar-baz.aliases.my_alias: {}}
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
@@ -590,7 +590,7 @@ static Map<String, Map<String, Object>> parseV2Mappings(String mappingsJson, Lis
                 nonProperties = innerTemplateNonProperties;
 
                 if (maybeProperties != null) {
-                    properties = mergeIgnoringDots(properties, maybeProperties);
+                    properties = mergeFailingOnReplacement(properties, maybeProperties);
                 }
             }
         }
@@ -605,7 +605,7 @@ static Map<String, Map<String, Object>> parseV2Mappings(String mappingsJson, Lis
             nonProperties = innerRequestNonProperties;
 
             if (maybeRequestProperties != null) {
-                properties = mergeIgnoringDots(properties, maybeRequestProperties);
+                properties = mergeFailingOnReplacement(properties, maybeRequestProperties);
             }
         }
 
@@ -705,18 +705,18 @@ private static Map<String, Object> dedupDynamicTemplates(Map<String, Object> map
     }
 
     /**
-     * Add the objects in the second map to the first, where the keys in the {@code second} map have
-     * higher predecence and overwrite the keys in the {@code first} map. In the event of a key with
-     * a dot in it (ie, "foo.bar"), the keys are treated as only the prefix counting towards
-     * equality. If the {@code second} map has a key such as "foo", all keys starting from "foo." in
-     * the {@code first} map are discarded.
+     * Add the objects in the second map to the first, A duplicated field is treated as illegal and
+     * an exception is thrown.
      */
-    static Map<String, Object> mergeIgnoringDots(Map<String, Object> first, Map<String, Object> second) {
+    static Map<String, Object> mergeFailingOnReplacement(Map<String, Object> first, Map<String, Object> second) {
         Objects.requireNonNull(first, "merging requires two non-null maps but the first map was null");
         Objects.requireNonNull(second, "merging requires two non-null maps but the second map was null");
         Map<String, Object> results = new HashMap<>(first);
         Set<String> prefixes = second.keySet().stream().map(MetadataCreateIndexService::prefix).collect(Collectors.toSet());
-        results.keySet().removeIf(k -> prefixes.contains(prefix(k)));
+        List<String> matchedPrefixes = results.keySet().stream().filter(k -> prefixes.contains(prefix(k))).collect(Collectors.toList());
+        if (matchedPrefixes.size() > 0) {
+            throw new IllegalArgumentException("mapping fields " + matchedPrefixes + " cannot be replaced during template composition");
+        }
         results.putAll(second);
         return results;
     }
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
@@ -196,15 +196,21 @@ ClusterState addComponentTemplate(final ClusterState currentState, final boolean
 
         validateTemplate(finalSettings, stringMappings, indicesService, xContentRegistry);
 
+        // Collect all the composable (index) templates that use this component template, we'll use
+        // this for validating that they're still going to be valid after this component template
+        // has been updated
+        final Map<String, ComposableIndexTemplate> templatesUsingComponent = currentState.metadata().templatesV2().entrySet().stream()
+            .filter(e -> e.getValue().composedOf().contains(name))
+            .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
         // if we're updating a component template, let's check if it's part of any V2 template that will yield the CT update invalid
         if (create == false && finalSettings != null) {
             // if the CT is specifying the `index.hidden` setting it cannot be part of any global template
             if (IndexMetadata.INDEX_HIDDEN_SETTING.exists(finalSettings)) {
-                Map<String, ComposableIndexTemplate> existingTemplates = currentState.metadata().templatesV2();
                 List<String> globalTemplatesThatUseThisComponent = new ArrayList<>();
-                for (Map.Entry<String, ComposableIndexTemplate> entry : existingTemplates.entrySet()) {
+                for (Map.Entry<String, ComposableIndexTemplate> entry : templatesUsingComponent.entrySet()) {
                     ComposableIndexTemplate templateV2 = entry.getValue();
-                    if (templateV2.composedOf().contains(name) && templateV2.indexPatterns().stream().anyMatch(Regex::isMatchAllPattern)) {
+                    if (templateV2.indexPatterns().stream().anyMatch(Regex::isMatchAllPattern)) {
                         // global templates don't support configuring the `index.hidden` setting so we don't need to resolve the settings as
                         // no other component template can remove this setting from the resolved settings, so just invalidate this update
                         globalTemplatesThatUseThisComponent.add(entry.getKey());
@@ -234,6 +240,32 @@ ClusterState addComponentTemplate(final ClusterState currentState, final boolean
             stringMappings == null ? null : new CompressedXContent(stringMappings), template.template().aliases());
         final ComponentTemplate finalComponentTemplate = new ComponentTemplate(finalTemplate, template.version(), template.metadata());
         validate(name, finalComponentTemplate);
+
+        if (templatesUsingComponent.size() > 0) {
+            ClusterState tempStateWithComponentTemplateAdded = ClusterState.builder(currentState)
+                .metadata(Metadata.builder(currentState.metadata()).put(name, finalComponentTemplate))
+                .build();
+            Exception validationFailure = null;
+            for (Map.Entry<String, ComposableIndexTemplate> entry : templatesUsingComponent.entrySet()) {
+                final String composableTemplateName = entry.getKey();
+                final ComposableIndexTemplate composableTemplate = entry.getValue();
+                try {
+                    validateCompositeTemplate(tempStateWithComponentTemplateAdded, composableTemplateName,
+                        composableTemplate, indicesService, xContentRegistry);
+                } catch (Exception e) {
+                    if (validationFailure == null) {
+                        validationFailure = new IllegalArgumentException("updating component template [" + name +
+                            "] results in invalid composable template [" + composableTemplateName + "] after templates are merged", e);
+                    } else {
+                        validationFailure.addSuppressed(e);
+                    }
+                }
+            }
+            if (validationFailure != null) {
+                throw validationFailure;
+            }
+        }
+
         logger.info("adding component template [{}]", name);
         return ClusterState.builder(currentState)
             .metadata(Metadata.builder(currentState.metadata()).put(name, finalComponentTemplate))
@@ -385,7 +417,6 @@ public ClusterState addIndexTemplateV2(final ClusterState currentState, final bo
             // adjusted (to add _doc) and it should be validated
             CompressedXContent mappings = innerTemplate.mappings();
             String stringMappings = mappings == null ? null : mappings.string();
-            validateTemplate(finalSettings, stringMappings, indicesService, xContentRegistry);
 
             // Mappings in index templates don't include _doc, so update the mappings to include this single type
             if (stringMappings != null) {
@@ -404,6 +435,17 @@ public ClusterState addIndexTemplateV2(final ClusterState currentState, final bo
         }
 
         validate(name, finalIndexTemplate);
+
+        // Finally, right before adding the template, we need to ensure that the composite settings,
+        // mappings, and aliases are valid after it's been composed with the component templates
+        try {
+            validateCompositeTemplate(currentState, name, finalIndexTemplate, indicesService, xContentRegistry);
+        } catch (Exception e) {
+            throw new IllegalArgumentException("composable template [" + name +
+                "] template after composition " +
+                (finalIndexTemplate.composedOf().size() > 0 ? "with component templates " + finalIndexTemplate.composedOf() + " " : "") +
+                "is invalid", e);
+        }
         logger.info("adding index template [{}]", name);
         return ClusterState.builder(currentState)
             .metadata(Metadata.builder(currentState.metadata()).put(name, finalIndexTemplate))
@@ -748,7 +790,6 @@ public static List<CompressedXContent> resolveMappings(final ClusterState state,
             return Collections.emptyList();
         }
         final Map<String, ComponentTemplate> componentTemplates = state.metadata().componentTemplates();
-        // TODO: more fine-grained merging of component template mappings, ie, merge fields as distint entities
         List<CompressedXContent> mappings = template.composedOf().stream()
             .map(componentTemplates::get)
             .filter(Objects::nonNull)
@@ -855,6 +896,72 @@ public static List<Map<String, AliasMetadata>> resolveAliases(final Metadata met
         return Collections.unmodifiableList(aliases);
     }
 
+    /**
+     * Given a state and a composable template, validate that the final composite template
+     * generated by the composable template and all of its component templates contains valid
+     * settings, mappings, and aliases.
+     */
+    private static void validateCompositeTemplate(final ClusterState state,
+                                                  final String templateName,
+                                                  final ComposableIndexTemplate template,
+                                                  final IndicesService indicesService,
+                                                  final NamedXContentRegistry xContentRegistry) throws Exception {
+        final ClusterState stateWithTemplate = ClusterState.builder(state)
+            .metadata(Metadata.builder(state.metadata()).put(templateName, template))
+            .build();
+
+        final String temporaryIndexName = "validate-template-" + UUIDs.randomBase64UUID().toLowerCase(Locale.ROOT);
+        Settings resolvedSettings = resolveSettings(stateWithTemplate.metadata(), templateName);
+
+        // use the provided values, otherwise just pick valid dummy values
+        int dummyPartitionSize = IndexMetadata.INDEX_ROUTING_PARTITION_SIZE_SETTING.get(resolvedSettings);
+        int dummyShards = resolvedSettings.getAsInt(IndexMetadata.SETTING_NUMBER_OF_SHARDS,
+            dummyPartitionSize == 1 ? 1 : dummyPartitionSize + 1);
+        int shardReplicas = resolvedSettings.getAsInt(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0);
+
+
+        // Create the final aggregate settings, which will be used to create the temporary index metadata to validate everything
+        Settings finalResolvedSettings = Settings.builder()
+            .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT)
+            .put(resolvedSettings)
+            .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, dummyShards)
+            .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, shardReplicas)
+            .put(IndexMetadata.SETTING_INDEX_UUID, UUIDs.randomBase64UUID())
+            .build();
+
+        // Validate index metadata (settings)
+        final ClusterState stateWithIndex = ClusterState.builder(stateWithTemplate)
+            .metadata(Metadata.builder(stateWithTemplate.metadata())
+                .put(IndexMetadata.builder(temporaryIndexName).settings(finalResolvedSettings))
+                .build())
+            .build();
+        final IndexMetadata tmpIndexMetadata = stateWithIndex.metadata().index(temporaryIndexName);
+        indicesService.withTempIndexService(tmpIndexMetadata,
+            tempIndexService -> {
+                // Validate aliases
+                MetadataCreateIndexService.resolveAndValidateAliases(temporaryIndexName, Collections.emptySet(),
+                    MetadataIndexTemplateService.resolveAliases(stateWithIndex.metadata(), templateName), stateWithIndex.metadata(),
+                    new AliasValidator(),
+                    // the context is only used for validation so it's fine to pass fake values for the
+                    // shard id and the current timestamp
+                    xContentRegistry, tempIndexService.newQueryShardContext(0, null, () -> 0L, null));
+
+                // Parse mappings to ensure they are valid after being composed
+                List<CompressedXContent> mappings = resolveMappings(stateWithIndex, templateName);
+                try {
+                    MapperService dummyMapperService = tempIndexService.mapperService();
+                    for (CompressedXContent mapping : mappings) {
+                        // TODO: Eventually change this to:
+                        // dummyMapperService.merge(MapperService.SINGLE_MAPPING_NAME, mapping, MergeReason.INDEX_TEMPLATE);
+                        dummyMapperService.merge(MapperService.SINGLE_MAPPING_NAME, mapping, MergeReason.MAPPING_UPDATE);
+                    }
+                } catch (Exception e) {
+                    throw new IllegalArgumentException("invalid composite mappings for [" + templateName + "]", e);
+                }
+                return null;
+            });
+    }
+
     private static void validateTemplate(Settings validateSettings, String mappings,
                                          IndicesService indicesService, NamedXContentRegistry xContentRegistry) throws Exception {
         validateTemplate(validateSettings, Collections.singletonMap(MapperService.SINGLE_MAPPING_NAME, mappings),
diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/ComponentTemplateTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/ComponentTemplateTests.java
@@ -88,7 +88,7 @@ public static ComponentTemplate randomInstance() {
     public static Map<String, AliasMetadata> randomAliases() {
         String aliasName = randomAlphaOfLength(5);
         AliasMetadata aliasMeta = AliasMetadata.builder(aliasName)
-            .filter(Collections.singletonMap(randomAlphaOfLength(2), randomAlphaOfLength(2)))
+            .filter("{\"term\":{\"year\":" + randomIntBetween(1, 3000) + "}}")
             .routing(randomBoolean() ? null : randomAlphaOfLength(3))
             .isHidden(randomBoolean() ? null : randomBoolean())
             .writeIndex(randomBoolean() ? null : randomBoolean())
diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplateTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplateTests.java
@@ -100,7 +100,7 @@ public static ComposableIndexTemplate randomInstance() {
     private static Map<String, AliasMetadata> randomAliases() {
         String aliasName = randomAlphaOfLength(5);
         AliasMetadata aliasMeta = AliasMetadata.builder(aliasName)
-            .filter(Collections.singletonMap(randomAlphaOfLength(2), randomAlphaOfLength(2)))
+            .filter("{\"term\":{\"year\":" + randomIntBetween(1, 3000) + "}}")
             .routing(randomBoolean() ? null : randomAlphaOfLength(3))
             .isHidden(randomBoolean() ? null : randomBoolean())
             .writeIndex(randomBoolean() ? null : randomBoolean())
diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java
@@ -1002,6 +1002,7 @@ public void testValidateTranslogRetentionSettings() {
     }
 
     @SuppressWarnings("unchecked")
+    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/pull/57393")
     public void testMappingsMergingIsSmart() throws Exception {
         Template ctt1 = new Template(null,
             new CompressedXContent("{\"_doc\":{\"_source\":{\"enabled\": false},\"_meta\":{\"ct1\":{\"ver\": \"text\"}}," +
@@ -1066,6 +1067,7 @@ public void testMappingsMergingIsSmart() throws Exception {
     }
 
     @SuppressWarnings("unchecked")
+    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/pull/57393")
     public void testMappingsMergingHandlesDots() throws Exception {
         Template ctt1 = new Template(null,
             new CompressedXContent("{\"_doc\":{\"properties\":{\"foo\":{\"properties\":{\"bar\":{\"type\": \"long\"}}}}}}"), null);
@@ -1100,33 +1102,31 @@ public void testMappingsMergingHandlesDots() throws Exception {
             equalTo(Collections.singletonMap("properties", Collections.singletonMap("bar", Collections.singletonMap("type", "long")))));
     }
 
-    public void testMergeIgnoringDots() throws Exception {
-        Map<String, Object> first = new HashMap<>();
-        first.put("foo", Collections.singletonMap("type", "long"));
-        Map<String, Object> second = new HashMap<>();
-        second.put("foo.bar", Collections.singletonMap("type", "long"));
-        Map<String, Object> results = MetadataCreateIndexService.mergeIgnoringDots(first, second);
-        assertThat(results, equalTo(second));
-
-        results = MetadataCreateIndexService.mergeIgnoringDots(second, first);
-        assertThat(results, equalTo(first));
-
-        second.clear();
-        Map<String, Object> inner = new HashMap<>();
-        inner.put("type", "text");
-        inner.put("analyzer", "english");
-        second.put("foo", inner);
-
-        results = MetadataCreateIndexService.mergeIgnoringDots(first, second);
-        assertThat(results, equalTo(second));
-
-        first.put("baz", 3);
-        second.put("egg", 7);
-
-        results = MetadataCreateIndexService.mergeIgnoringDots(first, second);
-        Map<String, Object> expected = new HashMap<>(second);
-        expected.put("baz", 3);
-        assertThat(results, equalTo(expected));
+    public void testMappingsMergingThrowsOnConflictDots() throws Exception {
+        Template ctt1 = new Template(null,
+            new CompressedXContent("{\"_doc\":{\"properties\":{\"foo\":{\"properties\":{\"bar\":{\"type\": \"long\"}}}}}}"), null);
+        Template ctt2 = new Template(null,
+            new CompressedXContent("{\"_doc\":{\"properties\":{\"foo.bar\":{\"type\": \"text\",\"analyzer\":\"english\"}}}}"), null);
+
+        ComponentTemplate ct1 = new ComponentTemplate(ctt1, null, null);
+        ComponentTemplate ct2 = new ComponentTemplate(ctt2, null, null);
+
+        ComposableIndexTemplate template = new ComposableIndexTemplate(Collections.singletonList("index"),
+            null, Arrays.asList("ct2", "ct1"), null, null, null);
+
+        ClusterState state = ClusterState.builder(ClusterState.EMPTY_STATE)
+            .metadata(Metadata.builder(Metadata.EMPTY_METADATA)
+                .put("ct1", ct1)
+                .put("ct2", ct2)
+                .put("index-template", template)
+                .build())
+            .build();
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+            () -> MetadataCreateIndexService.resolveV2Mappings("{}", state,
+                    "index-template", new NamedXContentRegistry(Collections.emptyList())));
+
+        assertThat(e.getMessage(), containsString("mapping fields [foo.bar] cannot be replaced during template composition"));
     }
 
     @SuppressWarnings("unchecked")
diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java

Original file line number	Diff line number	Diff line change
`@@ -590,7 +590,7 @@ static Map<String, Map<String, Object>> parseV2Mappings(String mappingsJson, Lis`
`590`	`590`	`nonProperties = innerTemplateNonProperties;`
`591`	`591`
`592`	`592`	`if (maybeProperties != null) {`
`593`		`- properties = mergeIgnoringDots(properties, maybeProperties);`
	`593`	`+ properties = mergeFailingOnReplacement(properties, maybeProperties);`
`594`	`594`	`}`
`595`	`595`	`}`
`596`	`596`	`}`
`@@ -605,7 +605,7 @@ static Map<String, Map<String, Object>> parseV2Mappings(String mappingsJson, Lis`
`605`	`605`	`nonProperties = innerRequestNonProperties;`
`606`	`606`
`607`	`607`	`if (maybeRequestProperties != null) {`
`608`		`- properties = mergeIgnoringDots(properties, maybeRequestProperties);`
	`608`	`+ properties = mergeFailingOnReplacement(properties, maybeRequestProperties);`
`609`	`609`	`}`
`610`	`610`	`}`
`611`	`611`
`@@ -705,18 +705,18 @@ private static Map<String, Object> dedupDynamicTemplates(Map<String, Object> map`
`705`	`705`	`}`
`706`	`706`
`707`	`707`	`/**`
`708`		`- * Add the objects in the second map to the first, where the keys in the {@code second} map have`
`709`		`- * higher predecence and overwrite the keys in the {@code first} map. In the event of a key with`
`710`		`- * a dot in it (ie, "foo.bar"), the keys are treated as only the prefix counting towards`
`711`		`- * equality. If the {@code second} map has a key such as "foo", all keys starting from "foo." in`
`712`		`- * the {@code first} map are discarded.`
	`708`	`+ * Add the objects in the second map to the first, A duplicated field is treated as illegal and`
	`709`	`+ * an exception is thrown.`
`713`	`710`	`*/`
`714`		`- static Map<String, Object> mergeIgnoringDots(Map<String, Object> first, Map<String, Object> second) {`
	`711`	`+ static Map<String, Object> mergeFailingOnReplacement(Map<String, Object> first, Map<String, Object> second) {`
`715`	`712`	`Objects.requireNonNull(first, "merging requires two non-null maps but the first map was null");`
`716`	`713`	`Objects.requireNonNull(second, "merging requires two non-null maps but the second map was null");`
`717`	`714`	`Map<String, Object> results = new HashMap<>(first);`
`718`	`715`	`Set<String> prefixes = second.keySet().stream().map(MetadataCreateIndexService::prefix).collect(Collectors.toSet());`
`719`		`- results.keySet().removeIf(k -> prefixes.contains(prefix(k)));`
	`716`	`+ List<String> matchedPrefixes = results.keySet().stream().filter(k -> prefixes.contains(prefix(k))).collect(Collectors.toList());`
	`717`	`+ if (matchedPrefixes.size() > 0) {`
	`718`	`+ throw new IllegalArgumentException("mapping fields " + matchedPrefixes + " cannot be replaced during template composition");`
	`719`	`+ }`
`720`	`720`	`results.putAll(second);`
`721`	`721`	`return results;`
`722`	`722`	`}`