From 177af69c7ff85786f62497a17d0b0ee554dadb0a Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Mon, 4 May 2020 11:51:29 -0600 Subject: [PATCH] Validate non-negative priorities for V2 index templates This also fixes an issue where a `null` priority was treated as below a 0 priority. `null` is now treated as 0 priority when it comes to comparing V2 templates. Relates to #53101 --- .../indices/index-templates.asciidoc | 3 +- .../put/PutIndexTemplateV2Action.java | 3 ++ .../cluster/metadata/IndexTemplateV2.java | 7 ++++ .../MetadataIndexTemplateService.java | 13 +++---- .../rest/action/cat/RestTemplatesAction.java | 2 +- .../put/PutIndexTemplateV2RequestTests.java | 12 ++++++ .../MetadataIndexTemplateServiceTests.java | 38 +++++++++++++------ 7 files changed, 57 insertions(+), 21 deletions(-) diff --git a/docs/reference/indices/index-templates.asciidoc b/docs/reference/indices/index-templates.asciidoc index fc45c24e47ad8..59d37804a9888 100644 --- a/docs/reference/indices/index-templates.asciidoc +++ b/docs/reference/indices/index-templates.asciidoc @@ -212,7 +212,8 @@ specified, meaning that the last component template specified has the highest pr `priority`:: (Optional, integer) Priority to determine index template precedence when a new index is created. The index template with -the highest priority is chosen. +the highest priority is chosen. If no priority is specified the template is treated as though it is +of priority 0 (lowest priority). This number is not automatically generated by {es}. `version`:: diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2Action.java b/server/src/main/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2Action.java index 3cceeebac7f58..53e0ee7898f0c 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2Action.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2Action.java @@ -103,6 +103,9 @@ public ActionRequestValidationException validateIndexTemplate(@Nullable ActionRe ); } } + if (indexTemplate.priority() != null && indexTemplate.priority() < 0) { + validationException = addValidationError("index template priority must be >= 0", validationException); + } } return validationException; } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexTemplateV2.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexTemplateV2.java index 0b5a68c14ad3a..70bd69e62ca82 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexTemplateV2.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexTemplateV2.java @@ -130,6 +130,13 @@ public Long priority() { return priority; } + public long priorityOrZero() { + if (priority == null) { + return 0L; + } + return priority; + } + public Long version() { return version; } 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 5a336d093732b..280e546f983a7 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java @@ -339,7 +339,7 @@ public ClusterState addIndexTemplateV2(final ClusterState currentState, final bo } Map> overlaps = findConflictingV2Templates(currentState, name, template.indexPatterns(), true, - template.priority()); + template.priorityOrZero()); overlaps.remove(name); if (overlaps.size() > 0) { String error = String.format(Locale.ROOT, "index template [%s] has index patterns %s matching patterns from " + @@ -351,7 +351,7 @@ public ClusterState addIndexTemplateV2(final ClusterState currentState, final bo overlaps.entrySet().stream() .map(e -> e.getKey() + " => " + e.getValue()) .collect(Collectors.joining(",")), - template.priority()); + template.priorityOrZero()); throw new IllegalArgumentException(error); } @@ -435,7 +435,7 @@ public static Map> findConflictingV1Templates(final Cluster */ public static Map> findConflictingV2Templates(final ClusterState state, final String candidateName, final List indexPatterns) { - return findConflictingV2Templates(state, candidateName, indexPatterns, false, null); + return findConflictingV2Templates(state, candidateName, indexPatterns, false, 0L); } /** @@ -449,7 +449,7 @@ public static Map> findConflictingV2Templates(final Cluster * index templates with the same priority). */ static Map> findConflictingV2Templates(final ClusterState state, final String candidateName, - final List indexPatterns, boolean checkPriority, Long priority) { + final List indexPatterns, boolean checkPriority, long priority) { Automaton v1automaton = Regex.simpleMatchToAutomaton(indexPatterns.toArray(Strings.EMPTY_ARRAY)); Map> overlappingTemplates = new HashMap<>(); for (Map.Entry entry : state.metadata().templatesV2().entrySet()) { @@ -457,7 +457,7 @@ static Map> findConflictingV2Templates(final ClusterState s IndexTemplateV2 template = entry.getValue(); Automaton v2automaton = Regex.simpleMatchToAutomaton(template.indexPatterns().toArray(Strings.EMPTY_ARRAY)); if (Operations.isEmpty(Operations.intersection(v1automaton, v2automaton)) == false) { - if (checkPriority == false || Objects.equals(priority, template.priority())) { + if (checkPriority == false || priority == template.priorityOrZero()) { logger.debug("old template {} and index template {} would overlap: {} <=> {}", candidateName, name, indexPatterns, template.indexPatterns()); overlappingTemplates.put(name, template.indexPatterns()); @@ -733,8 +733,7 @@ public static String findV2Template(Metadata metadata, String indexName, boolean } final List candidates = new ArrayList<>(matchedTemplates.keySet()); - CollectionUtil.timSort(candidates, Comparator.comparing(IndexTemplateV2::priority, - Comparator.nullsLast(Comparator.reverseOrder()))); + CollectionUtil.timSort(candidates, Comparator.comparing(IndexTemplateV2::priorityOrZero, Comparator.reverseOrder())); assert candidates.size() > 0 : "we should have returned early with no candidates"; IndexTemplateV2 winner = candidates.get(0); diff --git a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTemplatesAction.java b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTemplatesAction.java index fcb9910143432..fd002364e10c4 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTemplatesAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTemplatesAction.java @@ -108,7 +108,7 @@ private Table buildTable(RestRequest request, ClusterStateResponse clusterStateR table.startRow(); table.addCell(name); table.addCell("[" + String.join(", ", template.indexPatterns()) + "]"); - table.addCell(template.priority()); + table.addCell(template.priorityOrZero()); table.addCell(template.version()); table.addCell("[" + String.join(", ", template.composedOf()) + "]"); table.endRow(); diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2RequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2RequestTests.java index 125ed69984cad..8697be97e2c4f 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2RequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2RequestTests.java @@ -29,6 +29,7 @@ import org.elasticsearch.test.AbstractWireSerializingTestCase; import java.io.IOException; +import java.util.Arrays; import java.util.List; import static org.hamcrest.Matchers.is; @@ -79,4 +80,15 @@ public void testPutIndexTemplateV2RequestMustContainTemplate() { String error = validationErrors.get(0); assertThat(error, is("an index template is required")); } + + public void testValidationOfPriority() { + PutIndexTemplateV2Action.Request req = new PutIndexTemplateV2Action.Request("test"); + req.indexTemplate(new IndexTemplateV2(Arrays.asList("foo", "bar"), null, null, -5L, null, null)); + ActionRequestValidationException validationException = req.validate(); + assertThat(validationException, is(notNullValue())); + List validationErrors = validationException.validationErrors(); + assertThat(validationErrors.size(), is(1)); + String error = validationErrors.get(0); + assertThat(error, is("index template priority must be >= 0")); + } } 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 f6039cb63a363..09eeb4da3094f 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -413,7 +413,7 @@ public void testPuttingV2TemplateGeneratesWarning() throws Exception { "take precedence during new index creation"); assertNotNull(state.metadata().templatesV2().get("v2-template")); - assertThat(state.metadata().templatesV2().get("v2-template"), equalTo(v2Template)); + assertTemplatesEqual(state.metadata().templatesV2().get("v2-template"), v2Template); } public void testPutGlobalV2TemplateWhichResolvesIndexHiddenSetting() throws Exception { @@ -527,7 +527,7 @@ public void testUpdatingV1NonStarTemplateWithUnchangedPatternsGeneratesWarning() "take precedence during new index creation"); assertNotNull(state.metadata().templatesV2().get("v2-template")); - assertThat(state.metadata().templatesV2().get("v2-template"), equalTo(v2Template)); + assertTemplatesEqual(state.metadata().templatesV2().get("v2-template"), v2Template); // Now try to update the existing v1-template @@ -567,7 +567,7 @@ public void testUpdatingV1NonStarWithChangedPatternsTemplateGeneratesError() thr "take precedence during new index creation"); assertNotNull(state.metadata().templatesV2().get("v2-template")); - assertThat(state.metadata().templatesV2().get("v2-template"), equalTo(v2Template)); + assertTemplatesEqual(state.metadata().templatesV2().get("v2-template"), v2Template); // Now try to update the existing v1-template @@ -583,15 +583,29 @@ public void testUpdatingV1NonStarWithChangedPatternsTemplateGeneratesError() thr } public void testPuttingOverlappingV2Template() throws Exception { - IndexTemplateV2 template = new IndexTemplateV2(Arrays.asList("egg*", "baz"), null, null, 1L, null, null); - MetadataIndexTemplateService metadataIndexTemplateService = getMetadataIndexTemplateService(); - ClusterState state = metadataIndexTemplateService.addIndexTemplateV2(ClusterState.EMPTY_STATE, false, "foo", template); - IndexTemplateV2 newTemplate = new IndexTemplateV2(Arrays.asList("abc", "baz*"), null, null, 1L, null, null); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> metadataIndexTemplateService.addIndexTemplateV2(state, false, "foo2", newTemplate)); - assertThat(e.getMessage(), equalTo("index template [foo2] has index patterns [abc, baz*] matching patterns from existing " + - "templates [foo] with patterns (foo => [egg*, baz]) that have the same priority [1], multiple index templates may not " + - "match during index creation, please use a different priority")); + { + IndexTemplateV2 template = new IndexTemplateV2(Arrays.asList("egg*", "baz"), null, null, 1L, null, null); + MetadataIndexTemplateService metadataIndexTemplateService = getMetadataIndexTemplateService(); + ClusterState state = metadataIndexTemplateService.addIndexTemplateV2(ClusterState.EMPTY_STATE, false, "foo", template); + IndexTemplateV2 newTemplate = new IndexTemplateV2(Arrays.asList("abc", "baz*"), null, null, 1L, null, null); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> metadataIndexTemplateService.addIndexTemplateV2(state, false, "foo2", newTemplate)); + assertThat(e.getMessage(), equalTo("index template [foo2] has index patterns [abc, baz*] matching patterns from existing " + + "templates [foo] with patterns (foo => [egg*, baz]) that have the same priority [1], multiple index templates may not " + + "match during index creation, please use a different priority")); + } + + { + IndexTemplateV2 template = new IndexTemplateV2(Arrays.asList("egg*", "baz"), null, null, null, null, null); + MetadataIndexTemplateService metadataIndexTemplateService = getMetadataIndexTemplateService(); + ClusterState state = metadataIndexTemplateService.addIndexTemplateV2(ClusterState.EMPTY_STATE, false, "foo", template); + IndexTemplateV2 newTemplate = new IndexTemplateV2(Arrays.asList("abc", "baz*"), null, null, 0L, null, null); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> metadataIndexTemplateService.addIndexTemplateV2(state, false, "foo2", newTemplate)); + assertThat(e.getMessage(), equalTo("index template [foo2] has index patterns [abc, baz*] matching patterns from existing " + + "templates [foo] with patterns (foo => [egg*, baz]) that have the same priority [0], multiple index templates may not " + + "match during index creation, please use a different priority")); + } } public void testFindV2Templates() throws Exception {