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

ILM injection vs template_api setting #1115

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yaauie
Copy link
Contributor

@yaauie yaauie commented Feb 1, 2023

This is a spike that attempts to convey meaningully what I suggested in an issue comment of #1102 to solve #1108.

There are two issues at play, both with the newly-introduced template_api hint about the format of the provided template (which allows the user to provide a template whose format doesn't match the favored format for the version of ES we are connected to):

  1. template_api does not make sense in the absence of a user-provided template (it does not change how a default template is resolved or generated, and making it do so was never part of its scope).
  2. Even when a template_api hint is available, it is not used by the ILM injection code when it is guessing how to apply the ILM settings. Instead, it continues to switch based solely on which version of Elasticsearch it is connected to, and when it guesses incorrectly it crashes opaquely.

Therefore:

  • Providing the new template_api setting without also providing a template should be a configuration error (e.g., a `template` is required when providing a `template_api` hint about how to handle a user-provided `template`; If you wish to use the #{template_api} template API, you need to provide your own #{template_api}-formatted template).
  • ILM should use the template_api hint when deciding how to apply the ILM settings.

Additionally, we know that half-managed templates with ILM are unnecessarily risky for our users, and the complexity is only going to continue to grow; therefore:

  • we should deprecate (but continue to support) providing ilm_* settings along-side an explicit template, and should guide users to resolve the deprecation by adding their ILM settings to the template that they are providing (e.g., "Injecting ILM configuration into a custom template is deprecated and support for doing so will be removed in a future release; when managing your own template, please include your ILM configuration directly in the template you provide instead of using this plugin's `ilm_*` settings")
  • ILM injection should be audited to ensure we are as resilient as we can possibly be (e.g., tolerate missing intermediate nestings by creating them)
  • When ILM injection does crash (e.g., if we need to set a key on a key/value map, but the value at that address is a scalar), we should provide clear messaging and helpful guidance (e.g., "failed to apply ILM settings to the provided template: #{e.message}; when managing your own template, we advise including your ILM configuration directly in the template you provide and avoiding this plugin's `ilm_*` settings")

@yaauie yaauie force-pushed the ilm-injection-vs-template_api branch from d06ce4f to 0db4015 Compare February 1, 2023 19:05
Setting this flag to `composable` will use index template API to create index template.

NOTE: The format of template provided to <<plugins-{type}s-{plugin}-template>> needs to match the template API being used.
* Value can be any of:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

review note: I intentionally don't list auto here, because it is not particularly meaningful since it is already the default. Instead, I reveal that the default behavior is the same as either composable or legacy depending on which version of Elasticsearch we are connected to.

@@ -279,6 +279,8 @@ class LogStash::Outputs::ElasticSearch < LogStash::Outputs::Base
def initialize(*params)
super
setup_ecs_compatibility_related_defaults

raise LogStash::ConfigurationError("`template_api` is not allowed unless a `template` is also provided") if original_params.include?('template_api') && !original_params.include?('template')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very very small breaking risk. The intent of template_api was always as a hint of the template that was being provided, and never successfully controlled which API was used for the default template. It is possible that a user explicitly added a meaningless template_api hint without a template that happened to align with the default behaviour that did work, but even these cases we should reject the config helpfully.

if plugin.template
if plugin.maximum_seen_major_version < 8 && plugin.template_api == 'auto'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

review note: since template_api is a hint about template, only check-and-emit this warning if the user has provided a template

@@ -42,19 +42,29 @@ def self.install(client, template_endpoint, template_name, template, template_ov
end

def self.add_ilm_settings_to_template(plugin, template)
if plugin.template
plugin.deprecation_logger.deprecated("Injecting Index Lifecycle Management configuration into a provided `template` is deprecated, and support will be removed in a future version. Please add the configuration directly to your template.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree with most of changes but this message brings one concern to me. We are asking user to add the configuration directly to their template. However, we are overwriting their template index.lifecycle.name and index.lifecycle.rollover_alias ILM settings (template_manager.rb L53~L56) with plugin ILM settings, that means user custom template is updated even they add configurations to their template. Both index.lifecycle.name and index.lifecycle.rollover_alias have default values which may differ from user custom template ILM settings.

I was thinking following two options, they may be applied together but choosing one would simplify:

  1. Clear message which should highlight that we will opt out automatic setting but for now, to set same ILM configs int both plugin and template. I would not consider this option as it is manual effort and still under the risk that plugin still automatically applies index.lifecycle.name and index.lifecycle.rollover_alias to the custom template.
Suggested change
plugin.deprecation_logger.deprecated("Injecting Index Lifecycle Management configuration into a provided `template` is deprecated, and support will be removed in a future version. Please add the configuration directly to your template.")
plugin.deprecation_logger.deprecated("Injecting Index Lifecycle Management configuration into a provided `template` is deprecated, and support will be removed in a future version. Please add `index.lifecycle.name` and `index.lifecycle.rollover_alias` ILM configurations directly to your template as well as `ilm_policy` and `ilm_rollover_alias` in plugin configs.")
  1. We automatically set the ILM index.lifecycle.name and index.lifecycle.rollover_alias only IFF user custom template doesn't contain these settings. We also _info_rm user that we are using custom template ILM configs as it is and ignoring plugin level config. With this approach, we achieve backward compatibility. I do believe that the users who touch these settings in the custom template, mostly are knowledgable about ILM policy, rollover, etc..

In my opinion, option-2 looks better approach.
Let me please know if I am missing (or misunderstanding) anything.

"but will resolve to `composable` the first time it connects to Elasticsearch 8+. " +
"We recommend either setting `template_api => legacy` to continue providing legacy-style templates, " +
"or migrating your template to the composable style and setting `template_api => composable`. " +
"The legacy template API is slated for removal in Elasticsearch 9.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that Elasticsearch 9 is not newly added with change but I would prefer to change it to upcoming Elasticsearch versions(?!) as I couldn't find Elasticsearch 9 relative context in the public docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants