Verify connection to Elasticsearch #1487

delvedor · 2021-07-12T09:00:25Z

Follows this logic for verifying a connection to Elasticsearch:

Before the first API request:

Make an API request to /, inspect the response:
If call to / fails with 401 or 403 pass the check and show a warning (message will be linked later). This happens if the monitor permission missing for user. The subsequent checks must be ignored.
If there's no version field or if the version field value is <6.0.0 raise an error.
If there is a version field and it's >= 6.0.0 and <7.0.0:
- If there is no tagline field or if the tagline field value isn't You know, for Search raise an error.
If there is a version field and it's >=7.0.0 and <7.14.0:
- If there is no tagline field or if the tagline field value isn't You know, for Search raise an error.
- If there is no build_flavor field or if the build_flavor field value isn't default raise an error.
If there is a version field and it's >=7.14.0:
- If there is no X-Elastic-Product HTTP header in the response or if the X-Elastic-Product HTTP header value isn't Elasticsearch raise an error.

mshustov · 2021-07-14T08:26:41Z

Kibana implements a periodical version check as well https://github.com/elastic/kibana/blob/ef6b84dfa544a4677e1a33e911ef484979b5af17/src/core/server/elasticsearch/version_check/ensure_es_version.ts#L142 but both checks complement each other.

Before the first API request:

I guess we need to handle the incompatibility error in a special way to stop the Kibana instance. It might require additional work on the Kibana side to trigger @elastic/elasticsearch version check. Are you going to ship the check in v8.0 or v7.x?

lib/Transport.js

delvedor · 2021-07-14T08:45:41Z

@mshustov the check will be shipped in v7.

mshustov · 2021-07-14T08:49:41Z

@delvedor in which version? I will fill out an ER for the Kibana.

joshdover · 2021-07-16T11:48:34Z

index.js

+    // sync product check
+    const tSymbols = Object.getOwnPropertySymbols(this.transport)
+    client.transport[tSymbols[0]] = this.transport[tSymbols[0]]


Seems a bit fragile IMO to rely on the order of when the symbols were assigned outside the Transport class. Would it make sense to add a function like this.transport.sync(client.transport) so that the order of the symbols does not need to be relied on?

Good call out, I'll see what I can do.

joshdover · 2021-07-16T11:52:35Z

lib/Transport.js

+      if (err) {
+        if (err.statusCode === 401 || err.statusCode === 403) {
+          this[kProductCheck] = 2
+          process.emitWarning('The client is unable to verify that the server is Elasticsearch due to security privileges on the server side. Some functionality may not be compatible if the server is running an unsupported product.')


nit: I think this wording would be more clear

Suggested change

process.emitWarning('The client is unable to verify that the server is Elasticsearch due to security privileges on the server side. Some functionality may not be compatible if the server is running an unsupported product.')

process.emitWarning('The client is unable to verify that the server is Elasticsearch due to an authentication error with the server. Some functionality may not be compatible if the server is running an unsupported product.')

I like your suggestion, but this message cannot be changed as it has already been approved by product :)

joshdover · 2021-07-16T11:53:28Z

lib/Transport.js

@@ -494,6 +536,51 @@ class Transport {
      callback(null, hosts)
    })
  }
+
+  productCheck () {


Should we add any debug logging to this check so that we can easily find problems with it if it's not working as intended? Specifically, it'd be helpful to be able to inspect the response from Elasticsearch and know which branch failed the check.

joshdover · 2021-07-16T11:56:04Z

lib/Transport.js

+          productCheckEmitter.emit('product-check', false)
+        }
+      } else {
+        if (result.body.version == null || typeof result.body.version.number !== 'string') {


IMO I'd rather not rely on loose equality between undefined and null and would add an explicit check here. If nothing else, this makes it clear that this case should be tested.

Suggested change

if (result.body.version == null || typeof result.body.version.number !== 'string') {

if (result.body.version === null || result.body.version === undefined || typeof result.body.version.number !== 'string') {

As far as I know, when you are checking for both null and undefined, doing == null does the job pretty well, as it's testing for both.

joshdover · 2021-07-16T12:03:29Z

test/acceptance/product-check.test.js

+          cluster_name: 'docker-cluster',
+          cluster_uuid: 'cQ5pAMvRRTyEzObH4L5mTA',
+          version: {
+            number: '8.0.0-SNAPSHOT',


nit: Should we be using SNAPSHOT in the test data? (applies to all of these tests)

Given that we are only looking for the first two numbers it's fine :)

joshdover · 2021-07-16T12:04:54Z

test/acceptance/product-check.test.js

+            lucene_version: '8.9.0',
+            minimum_wire_compatibility_version: '7.15.0',
+            minimum_index_compatibility_version: '7.0.0'


nit: I don't think these are correct for any of the 7.x/6.x/5.x/4.x tests. They're not being used by the check right now, but it seems worth making sure this tests data is close to being accurate in case we do use these fields in the future.

You are right, I only changed the fields that we need to validate.
Given that from 7.14 we'll only test the response headers there is no need to make this too much future proof. I'll revisit if needed :)

joshdover · 2021-07-16T12:05:38Z

test/acceptance/product-check.test.js

+  const MockConnection = buildMockConnection({
+    onRequest (params) {
+      return {
+        statusCode: 401,


We should add test for 403 case for full coverage

Co-authored-by: Tomas Della Vedova <delvedor@users.noreply.github.com>

This reverts commit 17c744e

marcbachmann · 2021-08-17T10:10:48Z

lib/Transport.js

+            return productCheckEmitter.emit('product-check', false)
+          }
+        } else if (major === 7 && minor < 14) {
+          if (tagline !== 'You Know, for Search' || result.body.version.build_flavor !== 'default') {


FYI this breaks support for the oss docker images <= 7.10.1.
The build_flavor in there s oss.

{ "name" : "elasticsearch-production-2", "cluster_name" : "elasticsearch-bmg-prd", "cluster_uuid" : "41tljEp7T66B-13N2zaE4A", "version" : { "number" : "7.10.1", "build_flavor" : "oss", "build_type" : "docker", "build_hash" : "1c34507e66d7db1211f66f3513706fdf548736aa", "build_date" : "2020-12-05T01:00:33.671820Z", "build_snapshot" : false, "lucene_version" : "8.7.0", "minimum_wire_compatibility_version" : "6.8.0", "minimum_index_compatibility_version" : "6.0.0-beta1" }, "tagline" : "You Know, for Search" }

While an upgrade to a newer image fixes the incompatibility, you might want to adopt the check in here.

Hello! Please see #1519.

Thanks Thomas, that's then on purpose as I've assumed. There's just no mention in this PR, which could have the change a bit more transparent.

delvedor added 2 commits July 12, 2021 10:57

Add product check

6c2724c

Updated test

5435e9d

delvedor added semver-minor backport 7.x labels Jul 12, 2021

delvedor added 6 commits July 12, 2021 11:01

Updated test

99cd236

More test

c2bdc67

Fix check

5c6eec9

Updated test

589e11e

Updated test

6295a9b

Avoid creating a new event emitter per transport

a9b788d

mshustov reviewed Jul 14, 2021

View reviewed changes

lib/Transport.js Outdated Show resolved Hide resolved

delvedor added 2 commits July 14, 2021 11:39

Improve retry logic and introduce custom error class

c9aea83

Updated test

9f57297

mshustov mentioned this pull request Jul 14, 2021

Gracefully handle elasticsearch client version check elastic/kibana#105557

Closed

delvedor added 3 commits July 15, 2021 13:53

Updated warning log

a2dabc4

Updated docs

b1fa745

Merge branch 'master' into product-check

dc9c3d3

joshdover reviewed Jul 16, 2021

View reviewed changes

Address feedback

5ce708d

joshdover approved these changes Jul 16, 2021

View reviewed changes

delvedor merged commit 17c744e into master Jul 19, 2021

delvedor deleted the product-check branch July 19, 2021 14:42

github-actions bot mentioned this pull request Jul 19, 2021

[Backport 7.x] Verify connection to Elasticsearch #1497

Merged

github-actions bot pushed a commit that referenced this pull request Jul 19, 2021

Verify connection to Elasticsearch (#1487)

4d7524c

delvedor added a commit that referenced this pull request Jul 19, 2021

Verify connection to Elasticsearch (#1487) (#1497)

adc5c2b

Co-authored-by: Tomas Della Vedova <delvedor@users.noreply.github.com>

delvedor added a commit that referenced this pull request Jul 19, 2021

Verify connection to Elasticsearch (#1487)

12dfc31

TinaHeiligers mentioned this pull request Jul 23, 2021

Add explicit error type for SavedObjectsRepository for 404 errors that do not originate from Elasticsearch elastic/kibana#102353

Closed

mike-yu-roam pushed a commit to mike-yu-roam/working-mike-search-js that referenced this pull request Aug 12, 2021

Revert "Verify connection to Elasticsearch (elastic#1487)"

6061af5

This reverts commit 17c744e

YanivD mentioned this pull request Aug 12, 2021

fix: Elasticsearch v7.14.0 product check aspecto-io/opentelemetry-ext-js#173

Merged

xfournet mentioned this pull request Aug 17, 2021

Fails since 7.14.0 elastic/elasticsearch-js-mock#18

Closed

marcbachmann reviewed Aug 17, 2021

View reviewed changes

achrinza mentioned this pull request Sep 23, 2021

The client noticed that the server is not a supported distribution of Elasticsearch loopbackio/loopback-connector-elastic-search#140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify connection to Elasticsearch #1487

Verify connection to Elasticsearch #1487

delvedor commented Jul 12, 2021

mshustov commented Jul 14, 2021

delvedor commented Jul 14, 2021

mshustov commented Jul 14, 2021

joshdover Jul 16, 2021

delvedor Jul 16, 2021

joshdover Jul 16, 2021

delvedor Jul 16, 2021

joshdover Jul 16, 2021

joshdover Jul 16, 2021

delvedor Jul 16, 2021

joshdover Jul 16, 2021

delvedor Jul 16, 2021

joshdover Jul 16, 2021

delvedor Jul 16, 2021

joshdover Jul 16, 2021

marcbachmann Aug 17, 2021

delvedor Aug 17, 2021

marcbachmann Aug 17, 2021

	process.emitWarning('The client is unable to verify that the server is Elasticsearch due to security privileges on the server side. Some functionality may not be compatible if the server is running an unsupported product.')
	process.emitWarning('The client is unable to verify that the server is Elasticsearch due to an authentication error with the server. Some functionality may not be compatible if the server is running an unsupported product.')

	if (result.body.version == null \|\| typeof result.body.version.number !== 'string') {
	if (result.body.version === null \|\| result.body.version === undefined \|\| typeof result.body.version.number !== 'string') {

Verify connection to Elasticsearch #1487

Verify connection to Elasticsearch #1487

Conversation

delvedor commented Jul 12, 2021

mshustov commented Jul 14, 2021

delvedor commented Jul 14, 2021

mshustov commented Jul 14, 2021

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

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