Disallow merging existing mapping field definitions in templates

dakrone · dakrone · commit b393e30af04a · 2020-06-04T15:00:51.000-06:00
This commit changes the merge strategy introduced in elastic#55607 and elastic#55982. Instead of overwriting these fields, we now prevent them from being merged with an exception when a user attempts to overwrite a field. As part of this, a more robust validation has been added. The existing validation checked whether templates (composable and component) were valid on their own, this new validation now checks that the composite template (mappings/settings/aliases) is valid. This means that when a composable template is added or updated, we confirm that it is valid with its component pieces. When a component template is updated we ensure that all composable templates that make use of the component template continue to be valid before allowing the component template to be updated. This change also necessitated changes in the tests, however, I have left tests that exercise mapping merging with nested object fields as `@AwaitsFix`, as we intend to change the behavior soon to allow merging in a recursive-with-replacement fashion (see: elastic#57393). I have added tests that check the new disallowing behavior in the meantime.
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
@@ -572,7 +572,7 @@ static Map<String, Object> parseV2Mappings(String mappingsJson, List<CompressedX
                 nonProperties = innerTemplateNonProperties;
 
                 if (maybeProperties != null) {
-                    properties = mergeIgnoringDots(properties, maybeProperties);
+                    properties = mergeFailingOnReplacement(properties, maybeProperties);
                 }
             }
         }
@@ -587,7 +587,7 @@ static Map<String, Object> parseV2Mappings(String mappingsJson, List<CompressedX
             nonProperties = innerRequestNonProperties;
 
             if (maybeRequestProperties != null) {
-                properties = mergeIgnoringDots(properties, maybeRequestProperties);
+                properties = mergeFailingOnReplacement(properties, maybeRequestProperties);
             }
         }
 
@@ -687,18 +687,23 @@ 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 = new ArrayList<>();
+        results.keySet().forEach(k -> {
+            if (prefixes.contains(prefix(k))) {
+                matchedPrefixes.add(k);
+            }
+        });
+        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
@@ -195,15 +195,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());
@@ -233,6 +239,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))
@@ -422,7 +454,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) {
@@ -441,6 +472,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))
@@ -803,7 +845,6 @@ public static List<CompressedXContent> resolveMappings(final ClusterState state,
             return List.of();
         }
         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)
@@ -910,6 +951,77 @@ 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();
+
+        Index createdIndex = null;
+        final String temporaryIndexName = "validate-template-" + UUIDs.randomBase64UUID().toLowerCase(Locale.ROOT);
+        try {
+            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 index service for parsing and validating "mappings"
+            Settings dummySettings = 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(dummySettings))
+                    .build())
+                .build();
+            final IndexMetadata tmpIndexMetadata = stateWithIndex.metadata().index(temporaryIndexName);
+            IndexService dummyIndexService = indicesService.createIndex(tmpIndexMetadata, Collections.emptyList(), false);
+            createdIndex = dummyIndexService.index();
+
+            // 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, dummyIndexService.newQueryShardContext(0, null, () -> 0L, null));
+
+            // Parse mappings to ensure they are valid after being composed
+            List<CompressedXContent> mappings = resolveMappings(stateWithIndex, templateName);
+            Map<String, Object> finalMappings = MetadataCreateIndexService.parseV2Mappings("{}", mappings, xContentRegistry);
+            MapperService dummyMapperService = dummyIndexService.mapperService();
+            if (finalMappings.isEmpty() == false) {
+                assert finalMappings.size() == 1 : finalMappings;
+                // TODO: Eventually change this to:
+                // dummyMapperService.merge(MapperService.SINGLE_MAPPING_NAME, mapping, MergeReason.INDEX_TEMPLATE);
+                dummyMapperService.merge(MapperService.SINGLE_MAPPING_NAME, finalMappings, MergeReason.MAPPING_UPDATE);
+            }
+
+        } finally {
+            if (createdIndex != null) {
+                indicesService.removeIndex(createdIndex, NO_LONGER_ASSIGNED, "created for validating template addition");
+            }
+        }
+    }
+
     private static void validateTemplate(Settings validateSettings, String mappings,
                                          IndicesService indicesService, NamedXContentRegistry xContentRegistry) throws Exception {
         // First check to see if mappings are valid XContent
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
@@ -103,7 +103,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
@@ -946,6 +946,7 @@ public void testDeprecateTranslogRetentionSettings() {
     }
 
     @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\"}}," +
@@ -1010,6 +1011,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);
@@ -1044,33 +1046,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, 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
