Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New Platform] Validate config upfront #35453

Merged
merged 24 commits into from
May 10, 2019

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Apr 23, 2019

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'
};

Possible improvements

Checklist

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

For maintainers

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.
@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v7.2.0 labels Apr 23, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov mshustov marked this pull request as ready for review April 23, 2019 11:12
@mshustov mshustov requested a review from a team as a code owner April 23, 2019 11:12
@mshustov mshustov requested a review from azasypkin April 23, 2019 11:12
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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

💚 Build Succeeded

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mshustov mshustov requested a review from rudolf May 5, 2019 10:18
Set validation schemes in ConfigService.preSetup stage.
@mshustov mshustov force-pushed the issue-20303-validate-config-upfront branch from 142ca78 to a09520b Compare May 5, 2019 10:39
@mshustov
Copy link
Contributor Author

mshustov commented May 5, 2019

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
Copy link
Contributor

💚 Build Succeeded

@mshustov
Copy link
Contributor Author

mshustov commented May 5, 2019

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
Copy link
Contributor

💚 Build Succeeded

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

mshustov commented May 9, 2019

@azasypkin addressed

@mshustov mshustov requested a review from azasypkin May 9, 2019 13:01
@mshustov
Copy link
Contributor Author

mshustov commented May 9, 2019

@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
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/core/server/config/config_service.ts Outdated Show resolved Hide resolved
src/core/server/config/config_service.test.ts Outdated Show resolved Hide resolved
src/core/server/config/config_service.test.ts Show resolved Hide resolved
src/core/server/config/config_service.test.ts Outdated Show resolved Hide resolved
} catch (e) {
expect(e).toMatchSnapshot();
}
expect(configs.pipe(first()).toPromise()).rejects.toMatchInlineSnapshot(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, too bad

@@ -130,6 +130,10 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
await plugin$
.pipe(
mergeMap(async plugin => {
const schema = plugin.getConfigSchema();
if (schema) {
await this.coreContext.configService.setSchema(plugin.configPath, schema);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/core/server/server.ts Outdated Show resolved Hide resolved
@@ -98,10 +94,11 @@ export class Root {
}

private async setupLogging() {
const { configService } = this.server;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mshustov
Copy link
Contributor Author

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
Copy link
Contributor

💚 Build Succeeded

@mshustov mshustov merged commit 6b5c01c into elastic:master May 10, 2019
@mshustov mshustov deleted the issue-20303-validate-config-upfront branch May 10, 2019 12:47
sorenlouv pushed a commit to sorenlouv/kibana that referenced this pull request May 10, 2019
* 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 a commit to mshustov/kibana that referenced this pull request May 10, 2019
* 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 a commit that referenced this pull request May 10, 2019
* 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 mshustov added the release_note:skip Skip the PR/issue when compiling release notes label May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[new-platform] Doesn't validate config on startup
4 participants