replace direct access of hidden indices with system indices api #12279

kaisecheng · 2020-09-28T17:17:10Z

What does this PR do?

Replace direct access of elasticsearch hidden indices with elasticsearch system indices API

Why is it important/What is the impact to the user?

Elasticsearch has allowed other services to manipulate hidden indices directly for a long time. Recently, ES team introduced the System Indices and Hidden Indices concepts to replace indices that start with a dot, eg .logstash. These dot indices are an implementation detail that users should not interact with. Therefore, ES restricst the access by introducing a new restful API, system indices API. (elastic/elasticsearch#50251)

User who has cluster permission manage_logstash_pipeline can call system indices API

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files (and/or docker env variables)
I have added tests that prove my fix is effective or that my feature works

Author's Checklist

[ ]

How to test this PR locally

Related issues

related to elastic/elasticsearch#50251 elastic/elasticsearch#53350

Use cases

Screenshots

Logs

…ces-api

roaksoax

I've done a quick look over the PR and while it seems sane, there's one thing that wasn't considered (which Joao brought to my attention).

Nothing stops Logstash from using an older version of ES (< 7.10), and if that were to happen, logstash would fail to communicate. So this needs to be adapted to use the system index API starting from stack version 7.10+. For 7.9 and before, we use the old way.

roaksoax

This is not a full review, as I'm not a Ruby expert, but I like the implementation path you took.

That said, I think you should add comments to the code that explain what's going to to make it easier for reviewers or code readers.

x-pack/lib/config_management/elasticsearch_source.rb

x-pack/spec/config_management/elasticsearch_source_spec.rb

…ces-api

kaisecheng · 2020-10-02T20:44:46Z

jenkins test this please

…ces-api

robbavey · 2020-10-05T18:29:22Z

x-pack/lib/config_management/elasticsearch_source.rb

+          raise RemoteConfigError, "Cannot find elasticsearch version, server returned status: `#{response["status"]}`, message: `#{response["error"]}`"
+        end
+
+        logger.debug("Elasticsearch version ", response["version"]["number"])


As it stands, this line won't output the value of `response["version"]["number"] - you can update this by either:
logger.debug("Elasticsearch version {}", response["version"]["number"]),
```logger.debug("Elasticsearch version #{response['version']['number']}")``` or
```logger.debug("Elasticsearch version ", :version => response["version"]["number"])```

You might also want to add some context as to what is happening, such as "Reading configuration from Elasticsearch version..."

robbavey · 2020-10-05T18:30:39Z

x-pack/lib/config_management/elasticsearch_source.rb

@@ -50,6 +49,21 @@ def config_conflict?
        false
      end

+      # decide using system indices api (7.10+) or legacy api (< 7.10) base on elasticsearch server version
+      def pipeline_fetcher_factory


Nit: Maybe a different name for the method, such as get_pipeline_fetcher?

robbavey · 2020-10-05T19:01:10Z

x-pack/lib/config_management/elasticsearch_source.rb

+      end
+    end
+
+    # TODO clean up LegacyHiddenIndicesFetcher when 7.9.* is deprecated


Let's create an issue to remove this, and put a link in here

robbavey · 2020-10-05T19:13:34Z

x-pack/lib/config_management/elasticsearch_source.rb

+        client.get("#{SYSTEM_INDICES_API_PATH}/#{path_ids}")
+      end
+
+      def format_response(response)


Do we need to expose this method, which is always required by the caller? Or could the fetch_config method return the formatted response?

I will put format_response to fetch_config

robbavey · 2020-10-05T19:14:10Z

x-pack/lib/config_management/elasticsearch_source.rb

@@ -63,33 +77,27 @@ def pipeline_configs
          end
        end

-        response = fetch_config(pipeline_ids)
+        fetcher = pipeline_fetcher_factory


Nit: Maybe call this method get_pipeline_fetcher rather than factory?

robbavey · 2020-10-05T20:00:35Z

x-pack/lib/config_management/elasticsearch_source.rb

+        client.get("#{SYSTEM_INDICES_API_PATH}/#{path_ids}")
+      end
+
+      def format_response(response)


Do we need to expose this method, or could it be done as part of fetch_config?

robbavey · 2020-10-06T13:36:42Z

x-pack/lib/config_management/elasticsearch_source.rb

-
-        if response["found"] == false
+      def get_pipeline(pipeline_id, response, fetcher)
+        if response.has_key?(pipeline_id) == false


Nit: Consider using unless instead of if X == false, eg unless response.has_key?(pipeline_id)

robbavey · 2020-10-06T13:43:31Z

x-pack/lib/config_management/elasticsearch_source.rb

@@ -193,5 +186,63 @@ def client
        @client ||= build_client
      end
    end
+
+    class SystemIndicesFetcher


I wonder if this might be simpler if we kept the response object in this class, and had methods like 'config_exists?(pipeline_id)', get_pipeline_config(pipeline_id) and get_pipeline_settings(pipeline_id). This would avoid having to pass around the response and fetcher objects

This question to me is if we want to further refactor the existing code or the goal is to apply the new API in a manageable way. This involved thirty lines of code, mainly moving get_pipeline and the fetcher in a OO way, which is not a big change. At the same time, the readability of the current version is quite similar to the existing one. I am opened to the suggestion. Do you think we should refactor the code?

I'm not sure there is a huge amount of refactoring to the existing code either way, beyond what is already present; you already have the method fetcher.get_single_pipeline_setting(response, pipeline_id)["pipeline"]
which could change to something like fetched_config.get_pipeline_settings(pipeline_id), although I do realize that there is more work to be done in the extra classes that you have added.

I'm comfortable either way, what you have appears to be functionally correct after running this code locally against Elasticsearch 7.9 and 8.0

robbavey

LGTM.

* replace direct hidden indices access with system indices api * fulfill backward compatibility * fix log msg, rename class, simplify response handling * modularise fetcher

kaisecheng added 2 commits September 28, 2020 18:42

replace direct hidden indices access with system indices api

9da547f

Merge branch 'master' of github.com:elastic/logstash into system-indi…

19bb1e1

…ces-api

roaksoax requested a review from robbavey September 29, 2020 12:57

roaksoax suggested changes Sep 29, 2020

View reviewed changes

roaksoax requested review from jsvd and removed request for robbavey September 29, 2020 13:00

fulfill backward compatibility

9dd7c73

kaisecheng mentioned this pull request Sep 30, 2020

[Meta] Logstash use system indices API #12291

Closed

8 tasks

add log

ddc1b70

kaisecheng requested a review from roaksoax October 1, 2020 09:51

roaksoax requested a review from robbavey October 1, 2020 13:35

roaksoax reviewed Oct 1, 2020

View reviewed changes

kaisecheng commented Oct 1, 2020

View reviewed changes

x-pack/lib/config_management/elasticsearch_source.rb Show resolved Hide resolved

kaisecheng commented Oct 1, 2020

View reviewed changes

x-pack/lib/config_management/elasticsearch_source.rb Show resolved Hide resolved

kaisecheng commented Oct 1, 2020

View reviewed changes

x-pack/spec/config_management/elasticsearch_source_spec.rb Show resolved Hide resolved

kaisecheng added 2 commits October 2, 2020 11:38

Merge branch 'master' of github.com:elastic/logstash into system-indi…

ad24c5f

…ces-api

add comment

4b0a134

Merge branch 'master' of github.com:elastic/logstash into system-indi…

96006cb

…ces-api

robbavey reviewed Oct 5, 2020

View reviewed changes

robbavey requested changes Oct 5, 2020

View reviewed changes

fix log msg, rename class, simplify response handling

329e37a

robbavey requested changes Oct 6, 2020

View reviewed changes

modularise fetcher

cba4387

robbavey approved these changes Oct 6, 2020

View reviewed changes

kaisecheng merged commit 999601c into elastic:master Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace direct access of hidden indices with system indices api #12279

replace direct access of hidden indices with system indices api #12279

kaisecheng commented Sep 28, 2020

roaksoax left a comment

roaksoax left a comment

kaisecheng commented Oct 2, 2020

robbavey Oct 5, 2020

robbavey Oct 5, 2020

robbavey Oct 5, 2020

robbavey Oct 5, 2020

kaisecheng Oct 6, 2020

robbavey Oct 5, 2020

robbavey Oct 5, 2020

robbavey Oct 6, 2020

robbavey Oct 6, 2020

kaisecheng Oct 6, 2020

robbavey Oct 6, 2020

robbavey left a comment

replace direct access of hidden indices with system indices api #12279

replace direct access of hidden indices with system indices api #12279

Conversation

kaisecheng commented Sep 28, 2020

What does this PR do?

Why is it important/What is the impact to the user?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

roaksoax left a comment

Choose a reason for hiding this comment

roaksoax left a comment

Choose a reason for hiding this comment

kaisecheng commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbavey left a comment

Choose a reason for hiding this comment