[New Platform] Validate config upfront by mshustov · Pull Request #35453 · elastic/kibana

mshustov · 2019-04-23T09:53:23Z

Summary

Issue #20303
Should close #34812

To validate configuration on the start we need:

define config schema and config paths core services
validatie core service configs
run core services
plugins discovery collect all plugin manifests and schemas
plugins service runs config validation for discovered plugins
server crashes if validation fails on any step

To discuss

The way how plugins and core services define their configuration definitions.
The current PR introduces convention to export an object (code below).
Where configPath defines the path in the config object used by the current plugin/core service.
Should we deprecate configPath declaration in plugin manifest? In this case we unify approach for plugins and core services. Open to another suggestions as well.

// ../serviceName/index.ts
export const configDefinition = {
  configPath,  // ConfigPath,
  schema       // schema created with '@kbn/config-schema'
};

To simplify setup I declared schema definitions for core services manually.
https://github.com/elastic/kibana/pull/35453/files#diff-14bbd3d5a20ead019cd2d6d4ff234eb6R34
That allows us doesn't implement another discovery mechanism for core services, which is fine until we have a small limited number of them. Objections?

Possible improvements

we can make access to validated config as Observable<validated schema>. In plugin/core service wants to use ConfigClass it can do wrapping manually. The current solution requires defining ConfigClass that may look like over-complication for such simple cases https://github.com/elastic/kibana/pull/35453/files#diff-8217a395349429081ef42e1c34f6d54e
created an appropriate issue Simplify NP Config service consumption #36099
we can create config path collision detection to make sure one plugin/config does have access only to an isolated part of the config.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

We need to define a way to acquire configuration schema as a part of plugin definition. Having schema we can split steps of config validation and plugin instantiation.

Config validation finished before core services and plugins read from it. That allows us to fail fast and have predictable validation results.

elasticmachine · 2019-04-23T09:53:25Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-04-23T10:47:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-23T11:57:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-03T09:34:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

rudolf · 2019-05-03T09:47:26Z

src/core/server/config/config_service.ts

+    const namespace = pathToString(path);
+    const schema = this.schemas.get(namespace);
+    if (!schema) {
+      throw new Error(`No config validator defined for ${namespace}`);


nit: maybe we should change this error message to: No config schema defined for ${namespace}

rudolf

I would prefer keeping configPath in the manifest file. That way we keep the existing behaviour where we fallback to the plugin id in the manifest if no configPath is specified. I suspect most plugins wouldn't have a need to specify different configPath and would just use the id default?

To simplify setup I declared schema definitions for core services manually

I like this, we get a lot of code re-use without requiring that core services are exactly the same as plugins.

Set validation schemes in ConfigService.preSetup stage.

mshustov · 2019-05-05T11:00:19Z

After a chat with @azasypkin we decided that plugin discovery should a part of Plugin lifecycle, so I introduced additional preSetup hook for plugin and config services. This is not a part of public contract, but additional stage for core services, where we prepare and validate environment.
Updated: not actual anymore
@skaapgif would you mind to review updated version?

elasticmachine · 2019-05-05T11:58:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mshustov · 2019-05-05T12:58:26Z

I would prefer keeping configPath in the manifest file. That way we keep the existing behaviour where we fallback to the plugin id in the manifest if no configPath is specified. I suspect most plugins wouldn't have a need to specify different configPath and would just use the id default?

probably. also I don't see a problem to fallback to plugin id even if configPath is specified outside of the manifest file.

elasticmachine · 2019-05-08T16:47:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

plugins system is not setup when kibana is run in optimizer mode, so config keys aren't marked as applied.

mshustov · 2019-05-09T13:01:08Z

@azasypkin addressed

mshustov · 2019-05-09T13:48:00Z

@azasypkin what do you think about moving configPath from plugin manifest file to the same config object where schema if defined? like's done here for core services. in this case:

consistency with core services
config params are allocated together
config validation is less coupled with manifest file

elasticmachine · 2019-05-09T14:12:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin

Great job! Just a bunch of nits, but nothing important. I tested locally several cases - everything worked as expected. And as we agreed offline:

Can you please check if SIGHUP still functioning as expected with valid/invalid logging config
Please file issue for the bug that exists in master already: https://github.com/restrry/kibana/blob/b8cc18a1dc879e57f3ff98ac7d6dcba93aabfc9d/src/legacy/server/config/complete.js#L71

src/core/server/config/config_service.ts

src/core/server/config/config_service.test.ts

azasypkin · 2019-05-10T06:03:23Z

src/core/server/config/config_service.test.ts

-  } catch (e) {
-    expect(e).toMatchSnapshot();
-  }
+  expect(configs.pipe(first()).toPromise()).rejects.toMatchInlineSnapshot(


issue: I believe this test will pass even if you change rejects to resolves since you don't await on expect 😉 And in test below.

Actually I'm surprised that we don't have any automatic linter-like check for cases like this :/

alright, I expected that jest marks test as async if I call rejects/resolves in assertion

linter rule is not implemented 😞 jest-community/eslint-plugin-jest#54

Eh, too bad

azasypkin · 2019-05-10T06:29:28Z

src/core/server/plugins/plugins_service.ts

        mergeMap(async plugin => {
+          const schema = plugin.getConfigSchema();
+          if (schema) {
+            await this.coreContext.configService.setSchema(plugin.configPath, schema);


note: I have a bit mixed feelings about validating schema for disabled plugins, but it doesn't hurt I guess. Let's see how it goes.

Do you know if we include schema from disabled plugins into main schema that is validated in the legacy world? IIRC we mark config paths as handled even for disabled legacy plugins since recently, but not sure if we validate config for them.

IIRC we mark config paths as handled even for disabled legacy plugins since recently, but not sure if we validate config for them.

seems that we do validate them because we run validation
https://github.com/restrry/kibana/blob/b8cc18a1dc879e57f3ff98ac7d6dcba93aabfc9d/src/legacy/plugin_discovery/find_plugin_specs.js#L153
before read enabled
https://github.com/restrry/kibana/blob/b8cc18a1dc879e57f3ff98ac7d6dcba93aabfc9d/src/legacy/plugin_discovery/find_plugin_specs.js#L166
although it does't look like desired behaviour because we disable validation later
https://github.com/restrry/kibana/blob/b8cc18a1dc879e57f3ff98ac7d6dcba93aabfc9d/src/legacy/plugin_discovery/find_plugin_specs.js#L180

src/core/server/plugins/plugins_service.test.ts

src/core/server/server.ts

azasypkin · 2019-05-10T06:43:17Z

src/core/server/root/index.ts

  }

  private async setupLogging() {
+    const { configService } = this.server;


note: it's becoming clearer and clearer that we may need to merge server with root someday :)

src/core/server/elasticsearch/elasticsearch_config.ts

mshustov · 2019-05-10T08:02:18Z

Can you please check if SIGHUP still functioning as expected with valid/invalid logging config

works as on master

Please file issue for the bug that exists in master already:

#36423

elasticmachine · 2019-05-10T11:25:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Introduce new convention for config definition. We need to define a way to acquire configuration schema as a part of plugin definition. Having schema we can split steps of config validation and plugin instantiation. * Discover plugins, read their schema and validate the config. Config validation finished before core services and plugins read from it. That allows us to fail fast and have predictable validation results. * Instantiate plugins using DiscoveredPluginsDefinitions. * Update tests for new API. * test server is not created if config validation fails * move plugin discovery to plugin service pre-setup stage. Set validation schemes in ConfigService.preSetup stage. * fix eslint problem * generate docs * address Rudolfs comments * separate core services and plugins validation * rename files for consistency * address comments for root.js * address comments #1 * useSchema everywhere for consistency. get rid of validateAll * plugin system runs plugin config validation * rename configDefinition * move plugin schema registration in plugins plugins service plugins system is not setup when kibana is run in optimizer mode, so config keys aren't marked as applied. * cleanup * update docs * address comments

mshustov added 4 commits April 23, 2019 11:51

Introduce new convention for config definition.

c8f3ce6

We need to define a way to acquire configuration schema as a part of plugin definition. Having schema we can split steps of config validation and plugin instantiation.

Discover plugins, read their schema and validate the config.

1d37d47

Config validation finished before core services and plugins read from it. That allows us to fail fast and have predictable validation results.

Instantiate plugins using DiscoveredPluginsDefinitions.

0b9d5a8

Update tests for new API.

532089a

mshustov added chore Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform v7.2.0 labels Apr 23, 2019

test server is not created if config validation fails

4d805ad

mshustov marked this pull request as ready for review April 23, 2019 11:12

mshustov requested a review from a team as a code owner April 23, 2019 11:12

mshustov requested a review from azasypkin April 23, 2019 11:12

mshustov added the review label Apr 23, 2019

mshustov mentioned this pull request Apr 25, 2019

[New platform] Introduce start phase for core services on server #35297

Merged

3 tasks

mshustov force-pushed the issue-20303-validate-config-upfront branch from c309b81 to 16530f9 Compare May 3, 2019 08:31

Merge branch 'master' into issue-20303-validate-config-upfront

001efc6

mshustov force-pushed the issue-20303-validate-config-upfront branch from 16530f9 to 001efc6 Compare May 3, 2019 08:33

rudolf reviewed May 3, 2019

View reviewed changes

mshustov requested a review from rudolf May 5, 2019 10:18

move plugin discovery to plugin service pre-setup stage.

a09520b

Set validation schemes in ConfigService.preSetup stage.

mshustov force-pushed the issue-20303-validate-config-upfront branch from 142ca78 to a09520b Compare May 5, 2019 10:39

fix eslint problem

75d01ee

mshustov added 8 commits May 9, 2019 09:08

rename files for consistency

be4e4d3

address comments for root.js

09cf5f2

address comments #1

19c02c4

useSchema everywhere for consistency. get rid of validateAll

e622cd3

plugin system runs plugin config validation

f90711a

rename configDefinition

12d9947

move plugin schema registration in plugins plugins service

2a01ed7

plugins system is not setup when kibana is run in optimizer mode, so config keys aren't marked as applied.

cleanup

1345904

mshustov force-pushed the issue-20303-validate-config-upfront branch from 34d01fe to 1345904 Compare May 9, 2019 12:43

mshustov added 2 commits May 9, 2019 14:50

update docs

48c99cb

Merge branch 'master' into issue-20303-validate-config-upfront

b8cc18a

mshustov requested a review from azasypkin May 9, 2019 13:01

azasypkin approved these changes May 10, 2019

View reviewed changes

address comments

de341ac

mshustov merged commit 6b5c01c into elastic:master May 10, 2019

mshustov deleted the issue-20303-validate-config-upfront branch May 10, 2019 12:47

sorenlouv mentioned this pull request May 10, 2019

[7.x] [New Platform] Validate config upfront (#35453) #36438

Closed

mshustov mentioned this pull request May 10, 2019

[7.x] [New Platform] Validate config upfront (#35453) #36439

Merged

mshustov mentioned this pull request May 10, 2019

Config validation should occur up front #20303

Closed

mshustov added the release_note:skip Skip the PR/issue when compiling release notes label May 24, 2019

pgayvallet mentioned this pull request Nov 13, 2019

Kibana should crash immediately when misconfigured #49378

Closed

Conversation

mshustov commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

To discuss

Possible improvements

Checklist

For maintainers

Uh oh!

elasticmachine commented Apr 23, 2019

Uh oh!

elasticmachine commented Apr 23, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Apr 23, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented May 3, 2019

💚 Build Succeeded

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudolf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov commented May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented May 5, 2019

💚 Build Succeeded

Uh oh!

mshustov commented May 5, 2019

Uh oh!

elasticmachine commented May 8, 2019

💚 Build Succeeded

Uh oh!

mshustov commented May 9, 2019

Uh oh!

mshustov commented May 9, 2019

Uh oh!

elasticmachine commented May 9, 2019

💚 Build Succeeded

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mshustov commented May 10, 2019

Uh oh!

elasticmachine commented May 10, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mshustov commented Apr 23, 2019 •

edited

Loading

rudolf left a comment •

edited

Loading

mshustov commented May 5, 2019 •

edited

Loading