-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[New Platform] Validate config upfront #35453
Conversation
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.
Pinging @elastic/kibana-platform |
💚 Build Succeeded |
💚 Build Succeeded |
c309b81
to
16530f9
Compare
16530f9
to
001efc6
Compare
💚 Build Succeeded |
const namespace = pathToString(path); | ||
const schema = this.schemas.get(namespace); | ||
if (!schema) { | ||
throw new Error(`No config validator defined for ${namespace}`); |
There was a problem hiding this comment.
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}
There was a problem hiding this 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.
Set validation schemes in ConfigService.preSetup stage.
142ca78
to
a09520b
Compare
|
💚 Build Succeeded |
probably. also I don't see a problem to fallback to plugin id even if |
💚 Build Succeeded |
plugins system is not setup when kibana is run in optimizer mode, so config keys aren't marked as applied.
34d01fe
to
1345904
Compare
@azasypkin addressed |
@azasypkin what do you think about moving
|
💚 Build Succeeded |
There was a problem hiding this 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:
- 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
} catch (e) { | ||
expect(e).toMatchSnapshot(); | ||
} | ||
expect(configs.pipe(first()).toPromise()).rejects.toMatchInlineSnapshot( |
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -98,10 +94,11 @@ export class Root { | |||
} | |||
|
|||
private async setupLogging() { | |||
const { configService } = this.server; |
There was a problem hiding this comment.
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 :)
works as on master
|
💚 Build Succeeded |
* 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
* 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
* 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
Summary
Issue #20303
Should close #34812
To validate configuration on the start we need:
To discuss
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.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 useConfigClass
it can do wrapping manually. The current solution requires definingConfigClass
that may look like over-complication for such simple cases https://github.com/elastic/kibana/pull/35453/files#diff-8217a395349429081ef42e1c34f6d54ecreated 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
strikethroughsto 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[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers