Enable Product check from @elastic/elasticsearch-js #107663

afharo · 2021-08-04T16:28:10Z

Summary

In #107642 we disabled the Product Check while we made sure Kibana was ready for it (and solved some bugs we noticed in the Product-Check implementation). This PR enables the feature back and adds tests to confirm that the new Product check from @elastic/elasticsearch-js does not break Kibana's behaviour.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

afharo · 2021-08-17T09:31:21Z

Blocked by #107536

elasticmachine · 2021-08-19T15:27:26Z

Pinging @elastic/kibana-core (Team:Core)

elasticmachine · 2021-08-19T15:28:04Z

Pinging @elastic/kibana-operations (Team:Operations)

mshustov · 2021-08-20T09:46:54Z

src/core/server/elasticsearch/integration_tests/client.test.ts

@@ -55,3 +63,67 @@ describe('elasticsearch clients', () => {
    expect(resp.headers!.warning).toMatch('system indices');
  });
 });
+
+function createFakeElasticsearchServer() {


As I can see we rely on a product check to be performed during Elasticsearch version compatibility check step. Do you think it might make sense to have an explicit product check step during setup?

AFAIK, you cannot force a product check. The only thing that you can do is to make a random request (other than GET /), and the client will run it internally.

That said, I agree that we could run GET /_nodes like we do in the version-check polling, as the first thing during startup, and even crash Kibana if we agree to do so in the discussion #107663 (comment)

Implemented the method isValidConnection.

mshustov · 2021-08-20T09:49:46Z

src/core/server/elasticsearch/integration_tests/client.test.ts

+    await kibanaServer.preboot();
+    await kibanaServer.setup();
+    kibanaServer.start();
+    await fakeServer.firstRequestReceived$.pipe(first()).toPromise(); // Waiting for the first ES request instead of the start to complete, because start can't never complete due to waiting for ES to be ready


So Kibana doesn't stop, but running indefinitely until the problem is resolved? Is it okay from the product point of view? IMO it's okay since a normal ES server might start later, just want to confirm it aligns with the product vision @thesmallestduck

To be fair, AFAIK, once the client marks the connection as Product-invalid, it internally never checks again. It means that even when the ES server goes back to normal.

It might make sense to crash Kibana because of that. Although, we might need a retry logic to handle any possible errors other than the product check.

As an additional thought: we don't stop when Kibana's credentials or the ES host config are wrong.

I wonder if we should discuss this "Crash during startup on specific ES config errors" on a follow-up issue 🤔

I just noticed about this sentence in the original issue:

Kibana should handle the incompatibility check exception thrown by @elastic/elasticsearch to log an error and stop Kibana gracefully.

I'll implement the shutdown logic 👍

@afharo I added this requirement, but I didn't discuss it with the product management. Let's wait for @thesmallestduck feedback

Agree with @mshustov , a shutdown with a log message seems preferable to me. It is important to give an indication of why Kibana isn't functioning. An indefinite hang without a log message when connecting to an incorrect ES version seems unpleasant.

Deal! I'll trigger Kibana's shutdown when we see the ProductCheckError. FYI, we already log the error. But I'll make sure that it's more explicit.

mshustov · 2021-08-20T10:19:40Z

src/core/server/elasticsearch/integration_tests/client.test.ts

+    // eslint-disable-next-line
+    const resp = await supertest(kibanaServer['server']['http']['prebootServer']['server']!.listener) // Mind that we are using the prebootServer at this point because the migration gets hanging, while waiting for ES to be correct
+      .get('/api/status')
+      .expect(503);


It's turned out you cannot test it locally in the dev mode since basePathProxy blocks all the calls to the http preboot server 😞

kibana/packages/kbn-cli-dev-mode/src/dev_server.ts

Line 198 in 909d39b

if (msg === 'SERVER_LISTENING') {

Hum, this case is supposed to be handled by

kibana/src/core/server/bootstrap.ts

Lines 88 to 91 in 237256a

const isSetupOnHold = preboot.isSetupOnHold();

if (process.send && isSetupOnHold) {

process.send(['SERVER_LISTENING']);

}

Hum, this case is supposed to be handled by

@pgayvallet Right now, this condition is always false because no plugins call holdSetupUntilResolved.

src/core/server/elasticsearch/integration_tests/client.test.ts

mshustov · 2021-08-20T10:37:44Z

packages/kbn-es/src/integration_tests/__fixtures__/es_bin.js

+        return send(
+          200,
+          {
+            name: 'es-bin',


why didn't this test contain this object before?

es_bin was not covering the GET / requests. It'd just returned 404 because of the default use case. This means, that the client would never pass the Product Check and all the kbn-es tests would fail.

We can either disable the product check for these tests, or handle the specific request to pass it and unblock the client from that state.

…t-product-check-behaviour

…not halt

afharo · 2021-08-24T16:45:47Z

Fixed them with 31e6fdc (#107663):
I had to add an optional config to skip the validation of the connection so the tests wouldn't keep hanging due to ES not being started in those. It already skipped such ES connection hold previously performed by SOs by not initializing the plugins, so adding a new config property made sense to me.

…p ES clusters

docs/development/core/server/kibana-plugin-core-server.elasticsearchconfig.md

…docker

mshustov · 2021-08-26T07:35:23Z

src/core/server/saved_objects/routes/integration_tests/migrate.test.ts

+    root = kbnTestServer.createRoot({
+      migrations: { skip: true },
+      plugins: { initialize: false },
+      elasticsearch: { skipStartupConnectionCheck: true },


I don't fancy adding another config value just for testing but mock the service might over-complicate the testing and diverge from the production setup even more.
@afharo Would you mind tagging config values used for testing purposes? It might help us to get rid of them in the future. Maybe we should allow using them in the dev mode only? (if they aren't used in functional tests which are run in production mode)

Done! I had to do it only when running from source (dist === false) because integration tests run with dev: false.

@jbudz FYI: I removed the config from kibana-docker because it is now only allowed when running from source.

…t-product-check-behaviour

mshustov · 2021-08-26T11:03:19Z

src/core/server/elasticsearch/elasticsearch_config.ts

+    // and this config is solely introduced to allow some of the integration tests to run without an ES server.
+    schema.contextRef('dist'),
+    true,
+    schema.boolean({


nit: you can use schena.never() instead

kibana/src/plugins/maps_ems/config.ts

Lines 70 to 75 in 011955f

emsUrl: schema.conditional(

schema.siblingRef('proxyElasticMapsServiceInMaps'),

true,

schema.never(),

schema.string({ defaultValue: '' })

),

makes sense! but then the types will be skipStartupConnectionCheck?: boolean, meaning that we'll have undefined | false | true, it doesn't change much of the behaviour/logic, but do we want those 3 states?

no, we don't. We can normalize them in ElasticsearchConfig:

this.skipStartupConnectionCheck = Boolean(rawConfig.skipStartupConnectionCheck)

I like it! The error message is not as self-explanatory, though:

"[skipStartupConnectionCheck]: a value wasn't expected to be present"

as opposed to the current

"[skipStartupConnectionCheck]: \"skipStartupConnectionCheck\" can only be set to true when running from source to allow integration tests to run without an ES server"

If you think it's fine, I'm happy to change to schema.never() :)

If you think it's fine, I'm happy to change to

Up to you

I'll leave it as is for now then, for consistency with other keys in the same Elasticsearch config (i.e.: ignoreVersionMismatch).

I've created #110239 to follow up and change them all if we think schema.never() is a better approach.

kibanamachine · 2021-08-26T13:09:17Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #148658 succeeded af9bc7d
💚 Build #148484 succeeded 9a4a210
💔 Build #148380 failed 42da9cc
💔 Build #148306 failed a2c28fe
💔 Build #148254 failed 31e6fdc

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-08-26T14:39:51Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Alejandro Fernández Haro <[email protected]>

afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v7.14.1 labels Aug 4, 2021

afharo mentioned this pull request Aug 4, 2021

Gracefully handle elasticsearch client version check #105557

Closed

afharo force-pushed the test-es-client-product-check-behaviour branch from 1b6bb04 to fa3d76f Compare August 6, 2021 08:48

afharo force-pushed the test-es-client-product-check-behaviour branch from fa3d76f to 0e21c2c Compare August 13, 2021 14:37

afharo force-pushed the test-es-client-product-check-behaviour branch from 0e21c2c to 02a9a4b Compare August 18, 2021 15:48

mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021

afharo added 3 commits August 19, 2021 14:17

Test Product check in @elastic/elasticsearch-js

83d0a5c

Remove skipProductCheck from @kbn/es. It shouldn't be needed anymore

75e047b

@kbn/es's es_bin must cover the Product check request now

c9ccff1

afharo force-pushed the test-es-client-product-check-behaviour branch from 02a9a4b to c9ccff1 Compare August 19, 2021 13:05

afharo changed the title ~~Test Product check in @elastic/elasticsearch-js~~ Enable Product check from @elastic/elasticsearch-js Aug 19, 2021

afharo removed the v7.14.1 label Aug 19, 2021

Remove unused logger mock

909d39b

afharo marked this pull request as ready for review August 19, 2021 15:27

afharo requested review from a team as code owners August 19, 2021 15:27

afharo added the Team:Operations Team label for Operations Team label Aug 19, 2021

afharo added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 19, 2021

mshustov reviewed Aug 20, 2021

View reviewed changes

afharo added 3 commits August 20, 2021 15:07

Merge branch 'master' of github.com:elastic/kibana into test-es-clien…

c4cd22b

…t-product-check-behaviour

PR feedback

d737e9e

Fix types

410f11f

Add skipStartupConnectionCheck config so http integration tests do …

31e6fdc

…not halt

afharo added 2 commits August 24, 2021 21:05

API doc changes

c34f8aa

Update snapshots after config change

a2c28fe

afharo requested a review from a team as a code owner August 24, 2021 19:09

afharo requested review from a team August 24, 2021 19:09

afharo added 2 commits August 24, 2021 23:26

Skip ES validation on legacy/logging tests

42da9cc

Skip ES connection check in more integration tests that do not spin u…

9a4a210

…p ES clusters

afgomez self-requested a review August 25, 2021 12:56

afgomez approved these changes Aug 25, 2021

View reviewed changes

jbudz reviewed Aug 25, 2021

View reviewed changes

docs/development/core/server/kibana-plugin-core-server.elasticsearchconfig.md Outdated Show resolved Hide resolved

afharo added 2 commits August 25, 2021 17:17

Add new elasticsearch.skipStartupConnectionCheck setting to kibana-…

d4bb015

…docker

Better API docs

af9bc7d

jbudz approved these changes Aug 25, 2021

View reviewed changes

mshustov reviewed Aug 26, 2021

View reviewed changes

afharo added 2 commits August 26, 2021 10:35

Merge branch 'master' of github.com:elastic/kibana into test-es-clien…

36dc8d4

…t-product-check-behaviour

Allow skipStartupConnectionCheck only when running from source

8f1d6a3

mshustov reviewed Aug 26, 2021

View reviewed changes

afharo mentioned this pull request Aug 26, 2021

Use schema.never() for dev/!dist config parameters #110239

Closed

afharo merged commit bb8ee0c into elastic:master Aug 26, 2021

afharo deleted the test-es-client-product-check-behaviour branch August 26, 2021 14:36

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 26, 2021

Enable Product check from @elastic/elasticsearch-js (elastic#107663)

5f5728c

Co-authored-by: Kibana Machine <[email protected]>

kibanamachine mentioned this pull request Aug 26, 2021

[7.x] Enable Product check from @elastic/elasticsearch-js (#107663) #110250

Merged

afharo mentioned this pull request Aug 26, 2021

eventLog delaying Kibana from shutting down #109804

Closed

kibanamachine added a commit that referenced this pull request Aug 26, 2021

Enable Product check from @elastic/elasticsearch-js (#107663) (#110250)

c6fdc33

Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Alejandro Fernández Haro <[email protected]>

afharo mentioned this pull request Aug 31, 2021

[Elasticsearch] Handle Product Check in scoped clients #110675

Closed

TinaHeiligers mentioned this pull request Sep 23, 2021

Adds new SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107301

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Product check from @elastic/elasticsearch-js #107663

Enable Product check from @elastic/elasticsearch-js #107663

afharo commented Aug 4, 2021 •

edited

Loading

afharo commented Aug 17, 2021

elasticmachine commented Aug 19, 2021

elasticmachine commented Aug 19, 2021

mshustov Aug 20, 2021

afharo Aug 20, 2021

afharo Aug 23, 2021

mshustov Aug 20, 2021 •

edited

Loading

afharo Aug 20, 2021 •

edited

Loading

afharo Aug 20, 2021

mshustov Aug 20, 2021

thesmallestduck Aug 20, 2021

afharo Aug 23, 2021

mshustov Aug 20, 2021

pgayvallet Aug 20, 2021

mshustov Aug 23, 2021

mshustov Aug 20, 2021

afharo Aug 20, 2021

afharo commented Aug 24, 2021

mshustov Aug 26, 2021

afharo Aug 26, 2021

mshustov Aug 26, 2021 •

edited

Loading

afharo Aug 26, 2021

mshustov Aug 26, 2021

afharo Aug 26, 2021

mshustov Aug 26, 2021

afharo Aug 26, 2021

kibanamachine commented Aug 26, 2021

kibanamachine commented Aug 26, 2021

	const isSetupOnHold = preboot.isSetupOnHold();
	if (process.send && isSetupOnHold) {
	process.send(['SERVER_LISTENING']);
	}

	emsUrl: schema.conditional(
	schema.siblingRef('proxyElasticMapsServiceInMaps'),
	true,
	schema.never(),
	schema.string({ defaultValue: '' })
	),

Enable Product check from @elastic/elasticsearch-js #107663

Enable Product check from @elastic/elasticsearch-js #107663

Conversation

afharo commented Aug 4, 2021 • edited Loading

Summary

Checklist

For maintainers

afharo commented Aug 17, 2021

elasticmachine commented Aug 19, 2021

elasticmachine commented Aug 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

afharo Aug 20, 2021 • edited Loading

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

afharo commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Aug 26, 2021 • edited Loading

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

kibanamachine commented Aug 26, 2021

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Aug 26, 2021

💚 Backport successful

afharo commented Aug 4, 2021 •

edited

Loading

mshustov Aug 20, 2021 •

edited

Loading

afharo Aug 20, 2021 •

edited

Loading

mshustov Aug 26, 2021 •

edited

Loading