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.

Sign up for GitHub

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
Draft

ILM injection vs template_api setting #1115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6038383
use resolved `template_api` when injecting ILM
yaauie Feb 1, 2023
1bb2161
make `template` required when `template_api` is provided
yaauie Feb 1, 2023
37ab2af
deprecate combining ILM injection with user-provided template
yaauie Feb 1, 2023
0db4015
raise meaningful exception when ILM injection fails
yaauie Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions docs/index.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,25 +1077,25 @@ the *$JDK_HOME/conf/security/java.security* configuration file. That is, `TLSv1.

* Value type is <<path,path>>
* There is no default value for this setting.
* When a `template` is provided, you may also configure <<plugins-{type}s-{plugin}-template_api>> as a hint about the format of the provided template.

You can set the path to your own template here, if you so desire.
If not set, the included template will be used.

WARNING: Injecting ILM configuration into a custom `template` is deprecated.
If you are providing your own template, your ILM configuration should be a part of the template you provide.

[id="plugins-{type}s-{plugin}-template_api"]
===== `template_api`

* Value can be any of: `auto`, `legacy`, `composable`
* Default value is `auto`

The default setting of `auto` will use
{ref}/index-templates.html[index template API] to create index template, if the
Elasticsearch cluster is running Elasticsearch version `8.0.0` or higher,
and use {ref}/indices-templates-v1.html[legacy template API] otherwise.

Setting this flag to `legacy` will use legacy template API to create index template.
Setting this flag to `composable` will use index template API to create index template.
* 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.

** `composable`: maps to composable {ref}/index-templates.html[index template API]
** `legacy`: maps to the {ref}/indices-templates-v1.html[legacy template API]
* The default value is determined based on which version of Elasticsearch we are connected to:
** for {es} 7.x or older, the default is `legacy`
** for {es} 8.0+, the default is `composable`

NOTE: The format of template provided to <<plugins-{type}s-{plugin}-template>> needs to match the template API being used.
This is a hint for the format of the provided <<plugins-{type}s-{plugin}-template>>.

[id="plugins-{type}s-{plugin}-template_name"]
===== `template_name`
Expand Down
2 changes: 2 additions & 0 deletions lib/logstash/outputs/elasticsearch.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

end

def register
Expand Down
39 changes: 24 additions & 15 deletions lib/logstash/outputs/elasticsearch/template_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ class TemplateManager
def self.install_template(plugin)
return unless plugin.manage_template

if plugin.maximum_seen_major_version < 8 && plugin.template_api == 'auto'
plugin.logger.warn("`template_api => auto` resolved to `legacy` since we are connected to " + "Elasticsearch #{plugin.maximum_seen_major_version}, " +
"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.")
end

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

plugin.logger.warn("`template_api => auto` resolved to `legacy` since we are connected to " + "Elasticsearch #{plugin.maximum_seen_major_version}, " +
"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.

end

plugin.logger.info("Using mapping template from", :path => plugin.template)
template = read_template_file(plugin.template)
else
Expand All @@ -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.

end
# Overwrite any index patterns, and use the rollover alias. Use 'index_patterns' rather than 'template' for pattern
# definition - remove any existing definition of 'template'
template.delete('template') if template.include?('template') if plugin.maximum_seen_major_version < 8
template.delete('template') if template_endpoint(plugin) == LEGACY_TEMPLATE_ENDPOINT
template['index_patterns'] = "#{plugin.ilm_rollover_alias}-*"
settings = template_settings(plugin, template)
if settings && (settings['index.lifecycle.name'] || settings['index.lifecycle.rollover_alias'])
plugin.logger.info("Overwriting index lifecycle name and rollover alias as ILM is enabled")
end
settings.update({ 'index.lifecycle.name' => plugin.ilm_policy, 'index.lifecycle.rollover_alias' => plugin.ilm_rollover_alias})
rescue Exception => e
fail("Failed to apply ILM settings to template: #{e.message}")
end

def self.template_settings(plugin, template)
plugin.maximum_seen_major_version < 8 ? template['settings']: template['template']['settings']
if template_endpoint(plugin) == LEGACY_TEMPLATE_ENDPOINT
return template['settings'] ||= {}
end

template['template'] ||= {}
template['template']['settings'] ||= {}
end

# Template name - if template_name set, use it
Expand All @@ -77,12 +87,11 @@ def self.read_template_file(template_path)
end

def self.template_endpoint(plugin)
if plugin.template_api == 'auto'
plugin.maximum_seen_major_version < 8 ? LEGACY_TEMPLATE_ENDPOINT : INDEX_TEMPLATE_ENDPOINT
elsif plugin.template_api.to_s == 'legacy'
LEGACY_TEMPLATE_ENDPOINT
case plugin.template_api.to_s
when 'composable' then INDEX_TEMPLATE_ENDPOINT
when 'legacy' then LEGACY_TEMPLATE_ENDPOINT
else
INDEX_TEMPLATE_ENDPOINT
plugin.maximum_seen_major_version < 8 ? LEGACY_TEMPLATE_ENDPOINT : INDEX_TEMPLATE_ENDPOINT
end
end

Expand Down