From b393e30af04a06fa08eff0ad21f43dca603af2e6 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Tue, 2 Jun 2020 17:00:42 -0600 Subject: [PATCH 1/5] Disallow merging existing mapping field definitions in templates This commit changes the merge strategy introduced in #55607 and #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: #57393). I have added tests that check the new disallowing behavior in the meantime. --- .../15_composition.yml | 10 +- .../metadata/MetadataCreateIndexService.java | 23 ++- .../MetadataIndexTemplateService.java | 122 +++++++++++- .../metadata/ComponentTemplateTests.java | 2 +- .../ComposableIndexTemplateTests.java | 2 +- .../MetadataCreateIndexServiceTests.java | 54 +++--- .../MetadataIndexTemplateServiceTests.java | 175 +++++++++++++++++- 7 files changed, 339 insertions(+), 49 deletions(-) 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 index 0099ec96b2e9d..9bad6437dcf6a 100644 --- 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 index 3592a386d3e7e..c5e18a047cc60 100644 --- 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 parseV2Mappings(String mappingsJson, List parseV2Mappings(String mappingsJson, List dedupDynamicTemplates(Map 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 mergeIgnoringDots(Map first, Map second) { + static Map mergeFailingOnReplacement(Map first, Map 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 results = new HashMap<>(first); Set prefixes = second.keySet().stream().map(MetadataCreateIndexService::prefix).collect(Collectors.toSet()); - results.keySet().removeIf(k -> prefixes.contains(prefix(k))); + List 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 index caa15eb560830..448dc96fca1a3 100644 --- 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 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 existingTemplates = currentState.metadata().templatesV2(); List globalTemplatesThatUseThisComponent = new ArrayList<>(); - for (Map.Entry entry : existingTemplates.entrySet()) { + for (Map.Entry 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 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 resolveMappings(final ClusterState state, return List.of(); } final Map componentTemplates = state.metadata().componentTemplates(); - // TODO: more fine-grained merging of component template mappings, ie, merge fields as distint entities List mappings = template.composedOf().stream() .map(componentTemplates::get) .filter(Objects::nonNull) @@ -910,6 +951,77 @@ public static List> 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 mappings = resolveMappings(stateWithIndex, templateName); + Map 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 index 2709c59863c77..844dcb221113f 100644 --- 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 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 index dfb89ab7c361c..caa9182739644 100644 --- 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 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 index 49573366c0ecb..d8faacf778ca7 100644 --- 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 first = new HashMap<>(); - first.put("foo", Collections.singletonMap("type", "long")); - Map second = new HashMap<>(); - second.put("foo.bar", Collections.singletonMap("type", "long")); - Map results = MetadataCreateIndexService.mergeIgnoringDots(first, second); - assertThat(results, equalTo(second)); - - results = MetadataCreateIndexService.mergeIgnoringDots(second, first); - assertThat(results, equalTo(first)); - - second.clear(); - Map 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 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 index 763094eb0e25d..79e6ecfaedfef 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -71,6 +71,7 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.matchesRegex; public class MetadataIndexTemplateServiceTests extends ESSingleNodeTestCase { public void testIndexTemplateInvalidNumberOfShards() { @@ -670,7 +671,8 @@ public void testFindV2InvalidGlobalTemplate() { } } - public void testResolveMappings() throws Exception { + @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/pull/57393") + public void testResolveConflictingMappings() throws Exception { final MetadataIndexTemplateService service = getMetadataIndexTemplateService(); ClusterState state = ClusterState.EMPTY_STATE; @@ -733,6 +735,64 @@ public void testResolveMappings() throws Exception { equalTo(Map.of("_doc", Map.of("properties", Map.of("field", Map.of("type", "keyword")))))); } + public void testResolveMappings() throws Exception { + final MetadataIndexTemplateService service = getMetadataIndexTemplateService(); + ClusterState state = ClusterState.EMPTY_STATE; + + ComponentTemplate ct1 = new ComponentTemplate(new Template(null, + new CompressedXContent("{\n" + + " \"properties\": {\n" + + " \"field1\": {\n" + + " \"type\": \"keyword\"\n" + + " }\n" + + " }\n" + + " }"), null), null, null); + ComponentTemplate ct2 = new ComponentTemplate(new Template(null, + new CompressedXContent("{\n" + + " \"properties\": {\n" + + " \"field2\": {\n" + + " \"type\": \"text\"\n" + + " }\n" + + " }\n" + + " }"), null), null, null); + state = service.addComponentTemplate(state, true, "ct_high", ct1); + state = service.addComponentTemplate(state, true, "ct_low", ct2); + ComposableIndexTemplate it = new ComposableIndexTemplate(List.of("i*"), + new Template(null, + new CompressedXContent("{\n" + + " \"properties\": {\n" + + " \"field3\": {\n" + + " \"type\": \"integer\"\n" + + " }\n" + + " }\n" + + " }"), null), + List.of("ct_low", "ct_high"), 0L, 1L, null, null); + state = service.addIndexTemplateV2(state, true, "my-template", it); + + List mappings = MetadataIndexTemplateService.resolveMappings(state, "my-template"); + + assertNotNull(mappings); + assertThat(mappings.size(), equalTo(3)); + List> parsedMappings = mappings.stream() + .map(m -> { + try { + return MapperService.parseMapping(new NamedXContentRegistry(List.of()), m.string()); + } catch (Exception e) { + logger.error(e); + fail("failed to parse mappings: " + m.string()); + return null; + } + }) + .collect(Collectors.toList()); + + assertThat(parsedMappings.get(0), + equalTo(Map.of("_doc", Map.of("properties", Map.of("field2", Map.of("type", "text")))))); + assertThat(parsedMappings.get(1), + equalTo(Map.of("_doc", Map.of("properties", Map.of("field1", Map.of("type", "keyword")))))); + assertThat(parsedMappings.get(2), + equalTo(Map.of("_doc", Map.of("properties", Map.of("field3", Map.of("type", "integer")))))); + } + public void testResolveSettings() throws Exception { final MetadataIndexTemplateService service = getMetadataIndexTemplateService(); ClusterState state = ClusterState.EMPTY_STATE; @@ -859,6 +919,119 @@ public void testRemoveComponentTemplateInUse() throws Exception { containsString("component templates [ct] cannot be removed as they are still in use by index templates [template]")); } + /** + * Tests that we check that settings/mappings/etc are valid even after template composition, + * when adding/updating a composable index template + */ + public void testInvalidComposableCompositeTemplate() throws Exception { + final MetadataIndexTemplateService service = getMetadataIndexTemplateService(); + ClusterState state = ClusterState.EMPTY_STATE; + + ComponentTemplate ct1 = new ComponentTemplate(new Template(null, + new CompressedXContent("{\n" + + " \"properties\": {\n" + + " \"field2\": {\n" + + " \"type\": \"object\",\n" + + " \"properties\": {\n" + + " \"foo\": {\n" + + " \"type\": \"integer\"\n" + + " }\n" + + " }\n" + + " }\n" + + " }\n" + + " }"), null), null, null); + ComponentTemplate ct2 = new ComponentTemplate(new Template(null, + new CompressedXContent("{\n" + + " \"properties\": {\n" + + " \"field1\": {\n" + + " \"type\": \"text\"\n" + + " }\n" + + " }\n" + + " }"), null), null, null); + state = service.addComponentTemplate(state, true, "c1", ct1); + state = service.addComponentTemplate(state, true, "c2", ct2); + ComposableIndexTemplate it = new ComposableIndexTemplate(List.of("i*"), + new Template(null, new CompressedXContent("{\n" + + " \"properties\": {\n" + + " \"field2\": {\n" + + " \"type\": \"text\"\n" + + " }\n" + + " }\n" + + " }"), null), + randomBoolean() ? Arrays.asList("c1", "c2") : Arrays.asList("c2", "c1"), 0L, 1L, null, null); + + final ClusterState finalState = state; + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> service.addIndexTemplateV2(finalState, randomBoolean(), "my-template", it)); + + assertThat(e.getMessage(), + matchesRegex("composable template \\[my-template\\] template after composition with component templates .+ is invalid")); + + assertNotNull(e.getCause()); + assertThat(e.getCause().getMessage(), + containsString("mapping fields [field2] cannot be replaced during template composition")); + } + + /** + * Tests that we check that settings/mappings/etc are valid even after template composition, + * when updating a component template + */ + public void testInvalidComponentCompositeTemplate() throws Exception { + final MetadataIndexTemplateService service = getMetadataIndexTemplateService(); + ClusterState state = ClusterState.EMPTY_STATE; + + ComponentTemplate ct1 = new ComponentTemplate(new Template(null, + new CompressedXContent("{\n" + + " \"properties\": {\n" + + " \"field2\": {\n" + + " \"type\": \"object\",\n" + + " \"properties\": {\n" + + " \"foo\": {\n" + + " \"type\": \"integer\"\n" + + " }\n" + + " }\n" + + " }\n" + + " }\n" + + " }"), null), null, null); + ComponentTemplate ct2 = new ComponentTemplate(new Template(null, + new CompressedXContent("{\n" + + " \"properties\": {\n" + + " \"field1\": {\n" + + " \"type\": \"text\"\n" + + " }\n" + + " }\n" + + " }"), null), null, null); + state = service.addComponentTemplate(state, true, "c1", ct1); + state = service.addComponentTemplate(state, true, "c2", ct2); + ComposableIndexTemplate it = new ComposableIndexTemplate(List.of("i*"), + new Template(null, null, null), + randomBoolean() ? Arrays.asList("c1", "c2") : Arrays.asList("c2", "c1"), 0L, 1L, null, null); + + // Great, the templates aren't invalid + state = service.addIndexTemplateV2(state, randomBoolean(), "my-template", it); + + ComponentTemplate changedCt2 = new ComponentTemplate(new Template(null, + new CompressedXContent("{\n" + + " \"properties\": {\n" + + " \"field2\": {\n" + + " \"type\": \"text\"\n" + + " }\n" + + " }\n" + + " }"), null), null, null); + + final ClusterState finalState = state; + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> service.addComponentTemplate(finalState, false, "c2", changedCt2)); + + assertThat(e.getMessage(), + containsString("updating component template [c2] results in invalid " + + "composable template [my-template] after templates are merged")); + + assertNotNull(e.getCause()); + assertThat(e.getCause().getMessage(), + containsString("mapping fields [field2] cannot be replaced during template composition")); + } + private static List putTemplate(NamedXContentRegistry xContentRegistry, PutRequest request) { MetadataCreateIndexService createIndexService = new MetadataCreateIndexService( Settings.EMPTY, From 2646d9c5d2a54cca25cecf4edae4c04004cebf86 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Sun, 7 Jun 2020 21:32:07 -0600 Subject: [PATCH 2/5] Use functional instead of imperative prefix collection --- .../cluster/metadata/MetadataCreateIndexService.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java index c5e18a047cc60..be7bc3e20a88e 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java @@ -695,12 +695,7 @@ static Map mergeFailingOnReplacement(Map first, Objects.requireNonNull(second, "merging requires two non-null maps but the second map was null"); Map results = new HashMap<>(first); Set prefixes = second.keySet().stream().map(MetadataCreateIndexService::prefix).collect(Collectors.toSet()); - List matchedPrefixes = new ArrayList<>(); - results.keySet().forEach(k -> { - if (prefixes.contains(prefix(k))) { - matchedPrefixes.add(k); - } - }); + List 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"); } From 53b8b9cc9317eb354a50b3401e02b0420d4d3f0a Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Sun, 7 Jun 2020 21:41:22 -0600 Subject: [PATCH 3/5] Use IndexService.withTempIndexService --- .../MetadataIndexTemplateService.java | 105 +++++++++--------- 1 file changed, 52 insertions(+), 53 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java index 448dc96fca1a3..cb0ad250b4325 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java @@ -965,61 +965,60 @@ private static void validateCompositeTemplate(final ClusterState 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 mappings = resolveMappings(stateWithIndex, templateName); - Map 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); - } + 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(); - } finally { - if (createdIndex != null) { - indicesService.removeIndex(createdIndex, NO_LONGER_ASSIGNED, "created for validating template addition"); - } - } + // 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 mappings = resolveMappings(stateWithIndex, templateName); + final Map finalMappings; + try { + finalMappings = MetadataCreateIndexService.parseV2Mappings("{}", mappings, xContentRegistry); + + MapperService dummyMapperService = tempIndexService.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); + } + } catch (Exception e) { + throw new IllegalArgumentException("invalid composite mappings for [" + templateName + "]", e); + } + return null; + }); } private static void validateTemplate(Settings validateSettings, String mappings, From 9b3033202886f59d4b1396e2e8a3719c1b327b51 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Sun, 7 Jun 2020 21:48:34 -0600 Subject: [PATCH 4/5] Rename tests --- .../cluster/metadata/MetadataIndexTemplateServiceTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java index 79e6ecfaedfef..1e7cb5f3876d9 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -923,7 +923,7 @@ public void testRemoveComponentTemplateInUse() throws Exception { * Tests that we check that settings/mappings/etc are valid even after template composition, * when adding/updating a composable index template */ - public void testInvalidComposableCompositeTemplate() throws Exception { + public void testIndexTemplateFailsToOverrideComponentTemplateMappingField() throws Exception { final MetadataIndexTemplateService service = getMetadataIndexTemplateService(); ClusterState state = ClusterState.EMPTY_STATE; @@ -976,7 +976,7 @@ public void testInvalidComposableCompositeTemplate() throws Exception { * Tests that we check that settings/mappings/etc are valid even after template composition, * when updating a component template */ - public void testInvalidComponentCompositeTemplate() throws Exception { + public void testUpdateComponentTemplateFailsIfResolvedIndexTemplatesWouldBeInvalid() throws Exception { final MetadataIndexTemplateService service = getMetadataIndexTemplateService(); ClusterState state = ClusterState.EMPTY_STATE; From 95aa23f41c4ca2e6ed2980cf44fac29960f0e76a Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Sun, 7 Jun 2020 21:52:38 -0600 Subject: [PATCH 5/5] Fix tests --- .../metadata/MetadataIndexTemplateServiceTests.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java index 1e7cb5f3876d9..8955852ca156d 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -969,6 +969,10 @@ public void testIndexTemplateFailsToOverrideComponentTemplateMappingField() thr assertNotNull(e.getCause()); assertThat(e.getCause().getMessage(), + containsString("invalid composite mappings for [my-template]")); + + assertNotNull(e.getCause().getCause()); + assertThat(e.getCause().getCause().getMessage(), containsString("mapping fields [field2] cannot be replaced during template composition")); } @@ -1029,6 +1033,10 @@ public void testUpdateComponentTemplateFailsIfResolvedIndexTemplatesWouldBeInval assertNotNull(e.getCause()); assertThat(e.getCause().getMessage(), + containsString("invalid composite mappings for [my-template]")); + + assertNotNull(e.getCause().getCause()); + assertThat(e.getCause().getCause().getMessage(), containsString("mapping fields [field2] cannot be replaced during template composition")); }