Add preflight check on Elasticsearch before connecting #1026

andsel · 2021-07-14T13:32:03Z

Update client behavior to match functionality of 7.14+

Tasks

change error message from LogStash::ConfigurationError: Not a valid Elasticsearch to could not connect to a compatible version of Elasticsearch

andsel · 2021-08-17T13:21:42Z

The red CI on case:

INTEGRATION=true 
ELASTIC_STACK_VERSION=8.x 
SNAPSHOT=true

Is due to security "on" by default in 8.0.0. It doesn't depend on this PR, but could be solved disabling the security.
Add to .ci/Dockerfile.elasticsearch the line:

RUN echo "xpack.security.enabled: false" >> $es_yml

jsvd

I believe we can simplify the healthcheck process by reducing the number of calls to ES, and always performing the product check, which shouldn't have a performance impact.

lib/logstash/outputs/elasticsearch/http_client/pool.rb

jsvd · 2021-09-22T11:43:23Z

lib/logstash/outputs/elasticsearch/http_client/pool.rb

      # Try to keep locking granularity low such that we don't affect IO...
      @state_mutex.synchronize { @url_info.select {|url,meta| meta[:state] != :alive } }.each do |url,meta|
        begin
          health_check_request(url)
+
+          # when called from resurrectionist skip the product check done during register phase
+          if register_phase


This makes me wonder if we could just make the elasticsearch? check everytime instead of HEAD'ing the /?

Right after we do HEAD then we request '/' in es_version = get_es_version(url).

With this change we'll be doing 4 requests to ES:

HEAD request

GET '/' for elasticsearch? on register_phase

GET '/' to get elasticsearch version

GET '/_license'

We can just do instead:

GET '/' for healthcheck (and always confirm it's ES)

GET '/_license'

I agree with you, I think that point 2 and 3 could be merged or at least do one one HTTP GET call to /.
About the HTTP HEAD, that call could use a customized path from configuration (healthcheck_path) that some users could have customized. If we drop the HEAD call is like ignoring that setting and we should remove it.
I'm ok with removing the HEAD call, because actually seems to be used just to ping the server for reachability and then it immediately send a GET.
We could split this work in 2 steps:

first collapse the GET call for point 2 and 3

create a follow-up PR to drop the HEAD call and deprecate the healthcheck_path param

create a follow-up PR to drop the HEAD call and deprecate the healthcheck_path param

It's fine if there's only an issue instead to track the goal of reducing the number of network calls for a healthcheck.

Opened #1043 to track this feature

lib/logstash/outputs/elasticsearch/http_client/pool.rb

spec/unit/outputs/elasticsearch/http_client/pool_spec.rb

jsvd

other than a few cosmetic refactors the change LGTM but we need a better error message, just like in the ES input and filter.

…authenticy test, switched to direct stubbing

Co-authored-by: João Duarte <[email protected]>

jsvd

LGTM only minor nitpick

lib/logstash/outputs/elasticsearch/http_client/pool.rb

jsvd · 2021-10-11T16:38:46Z

lib/logstash/outputs/elasticsearch/http_client/pool.rb

      # Try to keep locking granularity low such that we don't affect IO...
      @state_mutex.synchronize { @url_info.select {|url,meta| meta[:state] != :alive } }.each do |url,meta|
        begin
          health_check_request(url)
+
+          # when called from resurrectionist skip the product check done during register phase
+          if register_phase


create a follow-up PR to drop the HEAD call and deprecate the healthcheck_path param

It's fine if there's only an issue instead to track the goal of reducing the number of network calls for a healthcheck.

Co-authored-by: João Duarte <[email protected]>

andsel changed the title ~~Feature/add elasticsearch check before connecting~~ Add preflight check on Elasticsearch before connecting Jul 14, 2021

roaksoax added the status:needs-review label Jul 14, 2021

andsel marked this pull request as draft July 15, 2021 07:25

roaksoax added status:work-in-progress and removed status:needs-review labels Jul 15, 2021

andsel force-pushed the feature/add_elasticsearch_check_before_connecting branch from 2c6d745 to ff08c78 Compare July 15, 2021 15:28

andsel marked this pull request as ready for review September 3, 2021 15:13

andsel requested a review from yaauie September 3, 2021 15:42

roaksoax added status:needs-review and removed status:work-in-progress labels Sep 3, 2021

andsel force-pushed the feature/add_elasticsearch_check_before_connecting branch from 6ca5c67 to 46d6835 Compare September 7, 2021 16:15

roaksoax requested a review from jsvd September 21, 2021 14:42

jsvd reviewed Sep 22, 2021

View reviewed changes

jsvd reviewed Sep 27, 2021

View reviewed changes

lib/logstash/outputs/elasticsearch/http_client/pool.rb Outdated Show resolved Hide resolved

lib/logstash/outputs/elasticsearch/http_client/pool.rb Outdated Show resolved Hide resolved

spec/unit/outputs/elasticsearch/http_client/pool_spec.rb Show resolved Hide resolved

jsvd reviewed Sep 27, 2021

View reviewed changes

andsel and others added 14 commits September 29, 2021 17:40

Draft the idea of the checks done at the same time of the license check

ec36553

Fixed failing test and store last version in case of bad Elasticsearch

9306b11

Added unit test to validate the authenticity rules

496a70b

Remove WebMock stubbing to avoid interference with other tests

4a60bd1

Removed WebMock used to mock HTTP request response for Elasticsearch …

2e9cdee

…authenticy test, switched to direct stubbing

Fixed bad spelled property

d483a4a

Updated version

6ff1eed

Switched #healthcheck! Elasticsearch check to raise a ConfigurationError

7255f02

Fixed failing tests

61de6e2

Apply product check only during registration phase

e2d82e6

Reused root URI string and avoided creation of Gem::Verson objects

35902e0

Simplify check

710577f

Co-authored-by: João Duarte <[email protected]>

Simplify if condition

8b4126e

Co-authored-by: João Duarte <[email protected]>

Changed error message to something more useful

1c2f467

Changed the version to 11.2.0

e590e04

andsel force-pushed the feature/add_elasticsearch_check_before_connecting branch from d46c39b to e590e04 Compare September 29, 2021 15:45

Removed disable of xpack.security

83d644d

andsel force-pushed the feature/add_elasticsearch_check_before_connecting branch from ee6fb44 to 83d644d Compare October 1, 2021 07:22

andsel requested a review from jsvd October 4, 2021 09:05

jsvd approved these changes Oct 11, 2021

View reviewed changes

Fixed error message

e8d3d7d

Co-authored-by: João Duarte <[email protected]>

roaksoax added status:approved and removed status:needs-review labels Oct 11, 2021

andsel merged commit 3a170cf into logstash-plugins:master Oct 12, 2021

roaksoax removed the status:approved label Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preflight check on Elasticsearch before connecting #1026

Add preflight check on Elasticsearch before connecting #1026

andsel commented Jul 14, 2021 •

edited

Loading

andsel commented Aug 17, 2021

jsvd left a comment

jsvd Sep 22, 2021

andsel Sep 23, 2021

jsvd Oct 11, 2021

andsel Oct 12, 2021

jsvd left a comment

jsvd left a comment

jsvd Oct 11, 2021

Add preflight check on Elasticsearch before connecting #1026

Add preflight check on Elasticsearch before connecting #1026

Conversation

andsel commented Jul 14, 2021 • edited Loading

Tasks

andsel commented Aug 17, 2021

jsvd left a comment

Choose a reason for hiding this comment

jsvd Sep 22, 2021

Choose a reason for hiding this comment

andsel Sep 23, 2021

Choose a reason for hiding this comment

jsvd Oct 11, 2021

Choose a reason for hiding this comment

andsel Oct 12, 2021

Choose a reason for hiding this comment

jsvd left a comment

Choose a reason for hiding this comment

jsvd left a comment

Choose a reason for hiding this comment

jsvd Oct 11, 2021

Choose a reason for hiding this comment

andsel commented Jul 14, 2021 •

edited

Loading