Skip to content

Commit

Permalink
Disallow merging existing mapping field definitions in templates (ela…
Browse files Browse the repository at this point in the history
…stic#57701)

* Disallow merging existing mapping field definitions in templates

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.

* Use functional instead of imperative prefix collection

* Use IndexService.withTempIndexService

* Rename tests

* Fix tests

Co-authored-by: Elastic Machine <[email protected]>
  • Loading branch information
dakrone and elasticmachine committed Jun 8, 2020
1 parent 047e787 commit f39754a
Showing 7 changed files with 341 additions and 49 deletions.
Original file line number Diff line number Diff line change
@@ -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: {}}
Original file line number Diff line number Diff line change
@@ -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;
}
Original file line number Diff line number Diff line change
@@ -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,76 @@ 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);
final Map<String, Object> 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,
IndicesService indicesService, NamedXContentRegistry xContentRegistry) throws Exception {
validateTemplate(validateSettings, Collections.singletonMap(MapperService.SINGLE_MAPPING_NAME, mappings),
Original file line number Diff line number Diff line change
@@ -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())
Original file line number Diff line number Diff line change
@@ -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())
Original file line number Diff line number Diff line change
@@ -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, 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")
Loading

0 comments on commit f39754a

Please sign in to comment.