Handle v2 template with index.hidden setting by andreidan · Pull Request #55015 · elastic/elasticsearch

andreidan · 2020-04-09T15:24:49Z

This validates that if the winner v2 template is a global one, it doesn't specify the
index.hidden setting.

This commit handles the case where the user request doesn't explicitly set the index as hidden but a mathcing template has the `index.hidden` setting configured to true. In this case the global templates need to be excluded.

elasticmachine · 2020-04-09T15:24:51Z

Pinging @elastic/es-core-features (:Core/Features/Indices APIs)

andreidan · 2020-04-09T15:36:07Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java

+                    throw new IllegalStateException("A global index V2 template [" + indexTemplateV2ToNameEntry.getValue() +
+                        "] defined the index hidden setting, which is not allowed");


#55010 guards against this when creating a v2 template

dakrone

Thanks for addressing this Andrei, I left a comment I think we need to address here too

dakrone · 2020-04-09T20:32:52Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java

+        // then we need to exclude global templates
+        if (isHidden == null) {
+            final Optional<Map.Entry<IndexTemplateV2, String>> templateWithHiddenSetting = matchedTemplates.entrySet().stream()
+                .filter(entry -> IndexMetadata.INDEX_HIDDEN_SETTING.exists(resolveSettings(metadata, entry.getValue()))).findFirst();


So this brings a question:

Should we throw an error (the error below for validation) on index creation for a template that won't actually "win" for the creation?

If the answer is yes, then I think rather than returning only the first one, it would be good to return all templates (in the error message I mean) that exhibit this behavior.

If the answer is no, then we can move this check to be below the candidate sorting and only check the winner template.

What do you think?

For me the answer is no and we should only be interested in winning template - index can became hidden only by the means of the winner, doesn't matter what is in other templates.
So I'd move the code after winner is selected.
I'd also change sorting to linear min selection to avoid creating new list and sorting candidates that won't win:

IndexTemplateV2 winner = matchedTemplates.keySet().stream().min(Comparator.comparing(IndexTemplateV2::priority, Comparator.nullsLast(Comparator.reverseOrder()))).get();

Great points. Thanks for bringing this up.

I saw this validation as "filter the invalid templates before we find the winner template" (ie. the global templates should not even be considered when finding the winning template).

The other way around, as I understand it, applying this validation on the winner template would be a no-op (since we don't support inheritance anymore, so settings from the global templates cannot be applied) and would look something along the lines of:

the winning template is a global template - no index.hiddne property is allowed on the global templates - so just return it

the winning template is not a global one but it has an index.hidden setting set to true in the resolved settings, but the other templates have a lower priority so just return it

the winning template is not a global one and doesn't have index.hidden set, so just return it

I do agree that having a lower priority (an explicit template) influence and remove a higher priority template (the global one - as illustrated in this test https://github.com/elastic/elasticsearch/pull/55015/files#diff-4e391d69e5a1fe53728ba9530499a6e8R552-R554) from the list of candidates might be (is) counter-intuitive so I think we either go along with this approach or, given the user has the option to override a global template using the priority, we could just issue a warning that a gloabl template is used as the matching one but that it is shadowing explicit templates that try to configure the index as hidden (I do however think this would go unnoticed in the logs, so treating this as expected and dropping this complex use case would also be an option I think)

Ok, we decided to validate only the winner in this particular case. Generally (together with #55010) we will be failing if the index.hidden setting is present in the resolved settings of a global template (regardless of the setting value).

With regards to this particular validation, if the winner is a global template we'll do a sanity check to make sure it doesn't include the index.hidden setting, in which case we'll fail, but otherwise we'll return it as the winner template.

probakowski

Thanks Andrei, I left few comments

probakowski · 2020-04-09T21:50:51Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java

+        // then we need to exclude global templates
+        if (isHidden == null) {
+            final Optional<Map.Entry<IndexTemplateV2, String>> templateWithHiddenSetting = matchedTemplates.entrySet().stream()
+                .filter(entry -> IndexMetadata.INDEX_HIDDEN_SETTING.exists(resolveSettings(metadata, entry.getValue()))).findFirst();


For me the answer is no and we should only be interested in winning template - index can became hidden only by the means of the winner, doesn't matter what is in other templates.
So I'd move the code after winner is selected.
I'd also change sorting to linear min selection to avoid creating new list and sorting candidates that won't win:

IndexTemplateV2 winner = matchedTemplates.keySet().stream().min(Comparator.comparing(IndexTemplateV2::priority, Comparator.nullsLast(Comparator.reverseOrder()))).get();

probakowski · 2020-04-09T22:00:55Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java


+        // this is complex but if the index is not hidden in the create request but is hidden as the result of template application,
+        // then we need to exclude global templates
+        if (isHidden == null) {


This can be simplified to something like this:

if (isHidden == null) { matchedTemplates.entrySet().stream() .filter(entry -> IndexMetadata.INDEX_HIDDEN_SETTING.get(resolveSettings(metadata, entry.getValue()))) // relies on the fact that default for INDEX_HIDDEN_SETTING is false .findAny() .ifPresent(indexTemplateV2ToNameEntry -> { matchedTemplates.keySet().removeIf(current -> current.indexPatterns().stream().anyMatch(Regex::isMatchAllPattern)); if (matchedTemplates.containsKey(indexTemplateV2ToNameEntry.getKey()) == false) { throw new IllegalStateException("A global index V2 template [" + indexTemplateV2ToNameEntry.getValue() + "] defined the index hidden setting, which is not allowed"); } }); }

On the second iteration on this I think there's a problem (fixed by proposal above): we can have 2 templates one with INDEX_HIDDEN_SETTING=false (explicitly set) and one with INDEX_HIDDEN_SETTING=true, findFirst can pick up false one and skip the whole block despite the fact that there's still one template with true

andreidan · 2020-04-16T13:30:04Z

@elasticmachine update branch

andreidan · 2020-04-16T16:06:47Z

@dakrone @probakowski Looking at the new code I believe we can drop the isHidden == null check altogether and just do this sanity check regardless of the isHidden value (as the check is only saying "the matching template is a global one, is it valid?")

What do you think?

https://github.com/elastic/elasticsearch/pull/55015/files#diff-115cd2f0fff9dd97acc95e1d29012a15R705-R711

dakrone · 2020-04-16T16:55:40Z

@andreidan I think it's reasonable to drop the null check, we're doing everything we can to prevent these templates from being settable. The isHidden parameter is actually whether the index.hidden setting is specified in the create index request itself, so it shouldn't affect anything related to the templates.

dakrone

LGTM (with the null check removal), I left one really minor nit

dakrone · 2020-04-16T16:56:00Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java


    /**
-     * Add the given component template to the cluster state. If {@code create} is true, an
+     * Add the given index template to the cluster state. If {@code create} is true, an


server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java

Co-Authored-By: Lee Hinman <dakrone@users.noreply.github.com>

probakowski

LGTM, thanks Andrei!

andreidan · 2020-04-17T12:32:38Z

@elasticmachine update branch

This validates that if the winner v2 template is a global one, it doesn't specify the index.hidden setting. (cherry picked from commit 19a97f7) Signed-off-by: Andrei Dan <andrei.dan@elastic.co>

isHidden was a `Boolean` in order to treat a special case identified with V1 templates where if the create index request didn't specify if the index should be hidden or not (ie. isHidden was `null`) but the index matched a template that specified the `index.hidden` setting we needed to remove the global templates from the templates we'll apply to the new index (note: this is important with V1 templates as inheritance is supported). With V2 templates we match only one template with an index so the equivalent check did not need to exist (we added a sanity check in elastic#55015 where we make sure we don't apply an invalid global template - one that specifes the `index.hidden` setting, but this is a check we make irrespective of the user specifying or not if the index should be hidden) This commit makes `isHidden` when matching V2 templates a boolean primitive, eliminating the need for the `null` state to exist. Note that some methods which use the matching V2 templates still work with a `Boolean` object `isHidden` attribute as they are also matching the V1 templates. These methods will pass in `false` instead of `null` when finding the V2 templates.

isHidden was a `Boolean` in order to treat a special case identified with V1 templates where if the create index request didn't specify if the index should be hidden or not (ie. isHidden was `null`) but the index matched a template that specified the `index.hidden` setting we needed to remove the global templates from the templates we'll apply to the new index (note: this is important with V1 templates as inheritance is supported). With V2 templates we match only one template with an index so the equivalent check did not need to exist (we added a sanity check in #55015 where we make sure we don't apply an invalid global template - one that specifes the `index.hidden` setting, but this is a check we make irrespective of the user specifying or not if the index should be hidden) This commit makes `isHidden` when matching V2 templates a boolean primitive, eliminating the need for the `null` state to exist. Note that some methods which use the matching V2 templates still work with a `Boolean` object `isHidden` attribute as they are also matching the V1 templates. These methods will pass in `false` instead of `null` when finding the V2 templates.

isHidden was a `Boolean` in order to treat a special case identified with V1 templates where if the create index request didn't specify if the index should be hidden or not (ie. isHidden was `null`) but the index matched a template that specified the `index.hidden` setting we needed to remove the global templates from the templates we'll apply to the new index (note: this is important with V1 templates as inheritance is supported). With V2 templates we match only one template with an index so the equivalent check did not need to exist (we added a sanity check in elastic#55015 where we make sure we don't apply an invalid global template - one that specifes the `index.hidden` setting, but this is a check we make irrespective of the user specifying or not if the index should be hidden) This commit makes `isHidden` when matching V2 templates a boolean primitive, eliminating the need for the `null` state to exist. Note that some methods which use the matching V2 templates still work with a `Boolean` object `isHidden` attribute as they are also matching the V1 templates. These methods will pass in `false` instead of `null` when finding the V2 templates. (cherry picked from commit c5b923a) Signed-off-by: Andrei Dan <andrei.dan@elastic.co>

isHidden was a `Boolean` in order to treat a special case identified with V1 templates where if the create index request didn't specify if the index should be hidden or not (ie. isHidden was `null`) but the index matched a template that specified the `index.hidden` setting we needed to remove the global templates from the templates we'll apply to the new index (note: this is important with V1 templates as inheritance is supported). With V2 templates we match only one template with an index so the equivalent check did not need to exist (we added a sanity check in #55015 where we make sure we don't apply an invalid global template - one that specifes the `index.hidden` setting, but this is a check we make irrespective of the user specifying or not if the index should be hidden) This commit makes `isHidden` when matching V2 templates a boolean primitive, eliminating the need for the `null` state to exist. Note that some methods which use the matching V2 templates still work with a `Boolean` object `isHidden` attribute as they are also matching the V1 templates. These methods will pass in `false` instead of `null` when finding the V2 templates. (cherry picked from commit c5b923a) Signed-off-by: Andrei Dan <andrei.dan@elastic.co>

Handle v2 template with index.hidden setting

321f74a

This commit handles the case where the user request doesn't explicitly set the index as hidden but a mathcing template has the `index.hidden` setting configured to true. In this case the global templates need to be excluded.

andreidan added :Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. v8.0.0 v7.8.0 labels Apr 9, 2020

andreidan requested review from dakrone and probakowski April 9, 2020 15:35

andreidan commented Apr 9, 2020

View reviewed changes

dakrone reviewed Apr 9, 2020

View reviewed changes

probakowski suggested changes Apr 9, 2020

View reviewed changes

elasticmachine and others added 3 commits April 16, 2020 09:30

Merge branch 'master' into itv2-null-isHidden-flag

9b54661

Validate the winner template when isHidden is null

d7f2393

Check if winner is a global template

8239490

andreidan requested review from dakrone and probakowski April 16, 2020 16:07

dakrone approved these changes Apr 16, 2020

View reviewed changes

andreidan and others added 2 commits April 16, 2020 18:00

Update exception message

b11c6f2

Co-Authored-By: Lee Hinman <dakrone@users.noreply.github.com>

Remve the isHidden flag null check

5cb83b8

probakowski approved these changes Apr 17, 2020

View reviewed changes

elasticmachine added 2 commits April 17, 2020 05:20

Merge branch 'master' into itv2-null-isHidden-flag

66ba801

Merge branch 'master' into itv2-null-isHidden-flag

819c9cf

Merge branch 'master' into itv2-null-isHidden-flag

5fc3fec

andreidan merged commit 19a97f7 into elastic:master Apr 17, 2020

andreidan added the backport pending label Apr 17, 2020

andreidan mentioned this pull request Apr 17, 2020

[7.x] Handle v2 template with index.hidden setting (#55015) #55400

Merged

andreidan removed the backport pending label Apr 17, 2020

andreidan mentioned this pull request Apr 17, 2020

Make isHidden a primitive when finding v2 templates #55408

Merged

andreidan mentioned this pull request Apr 19, 2020

[7.x] Make isHidden a primitive when finding v2 templates (#55408) #55433

Merged

pugnascotia added the >non-issue label May 6, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		throw new IllegalStateException("A global index V2 template [" + indexTemplateV2ToNameEntry.getValue() +
		"] defined the index hidden setting, which is not allowed");

Conversation

andreidan commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreidan Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

probakowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreidan commented Apr 16, 2020

Uh oh!

andreidan commented Apr 16, 2020

Uh oh!

dakrone commented Apr 16, 2020

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

probakowski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreidan commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andreidan commented Apr 9, 2020 •

edited

Loading

dakrone left a comment •

edited

Loading

andreidan Apr 13, 2020 •

edited

Loading

probakowski left a comment •

edited

Loading