Validate global V2 templates don't set index.hidden by andreidan · Pull Request #55010 · elastic/elasticsearch

andreidan · 2020-04-09T12:56:28Z

No description provided.

elasticmachine · 2020-04-09T12:56:31Z

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

.../main/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2Action.java

...java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateV2RequestTests.java

probakowski · 2020-04-09T14:21:52Z

Thanks @andreidan for working on this, I've added 2 request around null handling and 1 small nit that can be skipped

dakrone · 2020-04-09T16:07:02Z

We could probably also add validation to MetadataIndexTemplateService.validateTemplate that can check the current composed_of settings for this also, what do you think @andreidan?

andreidan · 2020-04-09T16:40:37Z

@dakrone sure, I'll add that validation in this PR

probakowski

LGTM, thanks @andreidan !

andreidan · 2020-04-15T11:13:13Z

@elasticmachine update branch

This also prevents updating the component template to add the index.hidden setting if that ct is referenced by a global index template.

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

@dakrone made the changes to prevent the index.hidden setting from being part of a global template (note that we're preventing it being part of the global template full stop, regardless of the true/false value it might be set to).

I didn't use the validateTemplate method as you initially suggested for this validation as that's more generic, looking at the settings and mappings only, irrespective of what kind of construct it's providing them (index templates v1, v2 or component template). It's also run after the cluster update request was issued but for the purpose of this global indices validation we can do it before we issue the cluster state update (similar to the validate(PutRequest) method used by the V1 templates).

andreidan · 2020-04-20T09:03:48Z

@dakrone @probakowski this probably slipped off your radar. do you have any more thoughts/suggestions on this?

dakrone

LGTM, thanks Andrei!

probakowski

LGTM, thanks @andreidan! I left few comments but they are purely subjective and can be skipped

probakowski · 2020-04-20T12:35:01Z

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

+        // 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)) {


Ifs can be merged

I don't think that'd be necessarily more readable (especially in the context of maybe us wanting to add more checks, this index.hidden one being just one condition we identified now). If it's ok, I'll leave it like this now

You definitely can leave it as it is, as I said this totally my personal preference only

probakowski · 2020-04-20T17:29:32Z

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

+                List<String> globalTemplatesThatUseThisComponent = new ArrayList<>();
+                for (Map.Entry<String, IndexTemplateV2> entry : existingTemplates.entrySet()) {
+                    IndexTemplateV2 templateV2 = entry.getValue();
+                    if (templateV2.composedOf().contains(name) && 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());
+                    }
+                }


This can be replaced with stream version, which for me is more readable:

List<String> globalTemplatesThatUseThisComponent = currentState.metadata().templatesV2().entrySet().stream() .filter(e -> e.getValue().composedOf().contains(name)) .filter(e -> e.getValue().indexPatterns().stream().anyMatch(Regex::isMatchAllPattern)) .map(Map.Entry::getKey) .collect(Collectors.toList());

I'm not sure about this, to me they're equally readable, but your option is a bit more concise.
In the interest of moving this rather stale PR forward I'm merging it as it is and we can refactor it later if that's ok?

Validate adding global V2 templates don't configure the index.hidden setting. This also prevents updating the component template to add the index.hidden setting if that component template is referenced by a global index template. (cherry picked from commit 2e76898) Signed-off-by: Andrei Dan <andrei.dan@elastic.co>

…55519) Validate adding global V2 templates don't configure the index.hidden setting. This also prevents updating the component template to add the index.hidden setting if that component template is referenced by a global index template. (cherry picked from commit 2e76898) Signed-off-by: Andrei Dan <andrei.dan@elastic.co>

Validate global V2 templates don't set index.hidden

12df473

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 13:39

probakowski suggested changes Apr 9, 2020

View reviewed changes

Handle null template properly

e21a594

andreidan requested a review from probakowski April 9, 2020 15:32

andreidan mentioned this pull request Apr 9, 2020

Handle v2 template with index.hidden setting #55015

Merged

probakowski approved these changes Apr 9, 2020

View reviewed changes

elasticmachine and others added 2 commits April 15, 2020 07:13

Merge branch 'master' into global-v2template-dont-set-indexhidden

a2fa7bb

Check the index template resolved settings for index.hidden

d38ad90

This also prevents updating the component template to add the index.hidden setting if that ct is referenced by a global index template.

andreidan and others added 2 commits April 16, 2020 14:28

Add fail statements in test

09342ee

Merge branch 'master' into global-v2template-dont-set-indexhidden

f0ac92c

andreidan requested a review from probakowski April 17, 2020 09:54

dakrone approved these changes Apr 20, 2020

View reviewed changes

probakowski approved these changes Apr 20, 2020

View reviewed changes

andreidan merged commit 2e76898 into elastic:master Apr 21, 2020

andreidan mentioned this pull request Apr 21, 2020

[7.x] Validate global V2 templates don't set index.hidden (#55010) #55519

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

Conversation

andreidan commented Apr 9, 2020

Uh oh!

elasticmachine commented Apr 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

probakowski commented Apr 9, 2020

Uh oh!

dakrone commented Apr 9, 2020

Uh oh!

andreidan commented Apr 9, 2020

Uh oh!

probakowski left a comment

Choose a reason for hiding this comment

Uh oh!

andreidan commented Apr 15, 2020

Uh oh!

andreidan commented Apr 16, 2020

Uh oh!

andreidan commented Apr 20, 2020

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

probakowski left a comment

Choose a reason for hiding this comment

Uh oh!

probakowski Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

andreidan Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

probakowski Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

probakowski Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

andreidan Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants