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

Kibana should crash immediately when misconfigured #49378

Closed
joshdover opened this issue Oct 25, 2019 · 5 comments · Fixed by #51880
Closed

Kibana should crash immediately when misconfigured #49378

joshdover opened this issue Oct 25, 2019 · 5 comments · Fixed by #51880
Assignees
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@joshdover
Copy link
Member

If a plugin's config is invalid, it will not crash Kibana until after Kibana connects to Elasticsearch. This doesn't necessarily cause any problems, but it is poor UX for sysadmins.

Currently, Kibana starts up in this order:

  1. Plugin discovery runs
  2. Kibana migrations are executed (which waits to establish a connection with Elasticsearch)
  3. Plugin configs are validated & plugins are initialized

This causes the issue above where a config may be invalid, but Kibana will not crash with an error until after Elasticsearch has connected.

We can move plugin config validation to execute during initial plugin discovery here: https://github.com/elastic/kibana/blob/master/src/core/server/server.ts#L79

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Oct 25, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@epixa
Copy link
Contributor

epixa commented Oct 25, 2019

To be clear, this applies to all configuration, not just plugin configuration. Kibana should not be attempting to use any config even from within core without config validation running. One important part of config validation is preventing startup when unknown configurations exist, which helps to prevent situations where a typo in config can result in unexpected things happening in Kibana. So, for example, if a person puts leasticsearch.username into kibana.yml, it's important that Kibana startup fails with an error before anything attempts to use the elasticsearch configuration.

@joshdover joshdover added this to Prioritized Backlog in kibana-core [DEPRECATED] Nov 12, 2019
@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 13, 2019

Isn't this already fixed by #35453 ? We seems to currently be validating configuration upfront.

private async handleDiscoveredPlugins(plugin$: Observable<PluginWrapper>) {
const pluginEnableStatuses = new Map<
PluginName,
{ plugin: PluginWrapper; isEnabled: boolean }
>();
await plugin$
.pipe(
mergeMap(async plugin => {
const schema = plugin.getConfigSchema();
if (schema) {
await this.coreContext.configService.setSchema(plugin.configPath, schema);
}

public async setSchema(path: ConfigPath, schema: Type<unknown>) {
const namespace = pathToString(path);
if (this.schemas.has(namespace)) {
throw new Error(`Validation schema for ${path} was already registered.`);
}
this.schemas.set(namespace, schema);
await this.validateConfig(path)
.pipe(first())
.toPromise();
}

(and same thing for core services)

@pgayvallet
Copy link
Contributor

Nevermind, missread the when unknown configurations exist part. leasticsearch.username will indeed validate ATM.

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 13, 2019

So currently

  • Invalid configuration in the NP services or plugins causes the application to crash upfront.
  • Invalid configuration in the LP plugins will crash, but after NP services setup
  • Same goes for the invalid configuration that doesn't belongs to any of the previous points (leasticsearch.username) will not cause kibana startup to fail.

To do all configuration checks upfront, we need to move the code that is currently in

export default async function (kbnServer, server, config) {
server.decorate('server', 'config', function () {
return kbnServer.config;
});
const unusedKeys = await getUnusedConfigKeys(
kbnServer.newPlatform.params.handledConfigPaths,
kbnServer.plugins,
kbnServer.disabledPluginSpecs,
kbnServer.settings,
config.get()
);

We need to have access, before the actual plugins setup, to the following:

  • The NP service namespaces
    public async setupConfigSchemas() {
    const schemas: Array<[ConfigPath, Type<unknown>]> = [
    [elasticsearchConfig.path, elasticsearchConfig.schema],
    [loggingConfig.path, loggingConfig.schema],
    [httpConfig.path, httpConfig.schema],
    [pluginsConfig.path, pluginsConfig.schema],
    [devConfig.path, devConfig.schema],
    [kibanaConfig.path, kibanaConfig.schema],
    [savedObjectsConfig.path, savedObjectsConfig.schema],
    [uiSettingsConfig.path, uiSettingsConfig.schema],
    ];
    for (const [path, schema] of schemas) {
    await this.configService.setSchema(path, schema);
    }
    }
  • The NP plugins namespaces
    private async handleDiscoveredPlugins(plugin$: Observable<PluginWrapper>) {
    const pluginEnableStatuses = new Map<
    PluginName,
    { plugin: PluginWrapper; isEnabled: boolean }
    >();
    await plugin$
    .pipe(
    mergeMap(async plugin => {
    const schema = plugin.getConfigSchema();
    if (schema) {
    await this.coreContext.configService.setSchema(plugin.configPath, schema);
    }
    const isEnabled = await this.coreContext.configService.isEnabledAtPath(plugin.configPath);
  • The LP plugins namespace
    const {
    pluginSpecs,
    pluginExtendedConfig,
    disabledPluginSpecs,
    uiExports,
    } = await findLegacyPluginSpecs(this.settings, this.coreContext.logger);

1/ One of the issue is that the LP plugins discovery occurs at the same time as their setup after the actual NP plugins setup

There is an open PR that solves this, by splitting LP plugins discovery and setup in #48882

2/ Another issue is that the NP services config registration is done in src/core/server/root/index.ts and not in src/core/server/server.ts because logging is initialized in index.ts and requires the logging config to be initialized. We will probably needs to move this inside the server to perform all schema registration at the same place.

@pgayvallet pgayvallet self-assigned this Nov 19, 2019
@pgayvallet pgayvallet moved this from Prioritized Backlog to In progress in kibana-core [DEPRECATED] Nov 28, 2019
kibana-core [DEPRECATED] automation moved this from In progress to Done (7.6) Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants