Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System indices ignore all user templates #87260

Merged

Conversation

williamrandolph
Copy link
Contributor

@williamrandolph williamrandolph commented May 31, 2022

System indices should ignore all user templates, except when explicitly stated (allowTemplates flag on SystemIndexDescriptor). This was the intended behavior for 8.0.

If merged, this will fix the longstanding issue #42508, as well as the stepping-stone issue #74271.

  • Verify that Kibana is not using templates for its settings and mappings

When creating an index with a system index descriptor, ignore all
templates. Update tests that check template behavior.
@williamrandolph williamrandolph added :Core/Infra/Core Core issues without another label >bug v8.4.0 labels May 31, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @williamrandolph, I've created a changelog YAML for you.

@@ -632,6 +639,52 @@ private ClusterState applyCreateIndexRequestWithV2Template(
);
}

private ClusterState applyCreateIndexRequestForSystemIndex(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I used applyCreateIndexRequestWithV1Templates and a list of empty templates, but that seemed very unsatisfying.

@grcevski
Copy link
Contributor

Kibana does use templates for at least one system index .kibana_security_session_1

https://github.com/elastic/kibana/blob/3730dd0779ed8efd74aee88f57422781ec1ac122/x-pack/plugins/security/server/session_management/session_index.ts#L61-L92

Alternatively we can ignore user templates for all MANAGED system indices.

@grcevski
Copy link
Contributor

Here's the related Kibana issue to migrate the security_session indices to supply the index options at creation time: elastic/kibana#134897

@grcevski grcevski marked this pull request as ready for review June 23, 2022 20:29
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jun 23, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@grcevski
Copy link
Contributor

To mitigate the fact that Kibana is using templates we introduced the allowsTemplates flag on SystemIndexDescriptor (#87933). We now use this flag in MetadataCreateIndexService to only allow templates to impact index creation on index descriptors that have this flag.

Tests have been adjusted accordingly to reflect this behaviour. We ensure that templates have impact only on index descriptors that have the flag explicitly on.

Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a bunch of comments but they're all really minor, this looks really good!

summary: System indices ignore all user templates
area: Infra/Core
type: bug
issues: []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to add the relevant issue links here.

2
);

assertAcked(client().admin().indices().prepareDeleteTemplate("*").get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line isn't necessary, it's done in the @After (and this should be handled by the test infrastructure anyway, I thought).

public void testAutoCreateSystemAliasViaV1Template() throws Exception {
var nonPrimaryIndex = autoCreateSystemAliasViaV1Template(INDEX_NAME);

assertAliasesHidden(nonPrimaryIndex, Set.of(INDEX_NAME), 1);

assertAcked(client().admin().indices().prepareDeleteTemplate("*").get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it looks like this line was copied - can you remove it from here as well? It's equally unnecessary.

2
);

assertAcked(client().admin().indices().prepareDeleteTemplate("*").get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this is done in the @After.


assertHasAliases(Set.of(".test-index", ".test-index-legacy-alias"));
assertHasAliases(Set.of(INDEX_NAME), INDEX_NAME, PRIMARY_INDEX_NAME, 1);

assertAcked(client().admin().indices().prepareDeleteTemplate("*").get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another one.


assertAliasesHidden(nonPrimaryIndex, Set.of(INDEX_NAME), 1);

assertAcked(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this is unnecessary as well despite not being explicitly in the @After, I ran it locally without this x10 and every thing, passed, and :server:check passed as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to leave it, it does clean up the template explicitly and at the same time it tests we can clean up composable templates.

@grcevski
Copy link
Contributor

@elasticmachine update branch

@grcevski
Copy link
Contributor

Hey @gwbrown, I think I addressed all of your feedback. Do you mind taking another look?

@legrego
Copy link
Member

legrego commented Jun 30, 2022

Here's the related Kibana issue to migrate the security_session indices to supply the index options at creation time: elastic/kibana#134897

The fix for this has been merged to Kibana's 'main' branch.

Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@grcevski grcevski merged commit d3f74ca into elastic:master Jun 30, 2022
@grcevski grcevski deleted the si2/system-indices-ignore-templates branch June 30, 2022 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants