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] Doesn't validate config on startup #34812

Closed
zfy0701 opened this issue Apr 9, 2019 · 8 comments · Fixed by #35453
Closed

[new-platform] Doesn't validate config on startup #34812

zfy0701 opened this issue Apr 9, 2019 · 8 comments · Fixed by #35453
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:New Platform low

Comments

@zfy0701
Copy link
Contributor

zfy0701 commented Apr 9, 2019

I'm trying to create a migrate PR here #34766 to see what's it looks like, here is a few issues I found

@epixa
Copy link
Contributor

epixa commented Apr 9, 2019

Plugin discovery should support x-pack in the next couple days. It was intentionally omitted so far, but it's up next.

I believe that log line you linked would only fire if something actually subscribed to the exposed data$ observable.

I'm not sure why the core.testbed.secret config validation isn't working. That's definitely a bug, so we'll sort it out.

@kindsun kindsun added Feature:New Platform bug Fixes for quality problems that affect the customer experience labels Apr 9, 2019
@rudolf
Copy link
Contributor

rudolf commented Apr 10, 2019

#34725 Fixes the signatures.

@rudolf rudolf changed the title New platform migration issue [new-platform] Doesn't validate config on startup Apr 10, 2019
@mw-ding
Copy link
Contributor

mw-ding commented Apr 11, 2019

To add some additional comments to @zfy0701's comment, the config validation failed because of the error in here. The config is undefined while the upgrade() does not handle this case.

image

Also, the console throws out the right error when you add some print statements for debugging. However, without these print statements, the error won't be triggered, which is also weird.

Configuring logger failed: TypeError: Cannot read property 'appenders' of undefined
    at LoggingService.appenders [as upgrade] (/Users/mengwei/kibana/src/core/server/logging/logging_service.ts:71:56)
    at MapSubscriber.upgrade (/Users/mengwei/kibana/src/core/server/root/index.ts:96:29)
    at MapSubscriber._next (/Users/mengwei/kibana/node_modules/rxjs/src/internal/operators/map.ts:81:29)
    at MapSubscriber.Subscriber.next (/Users/mengwei/kibana/node_modules/rxjs/src/internal/Subscriber.ts:102:12)
    at SwitchMapSubscriber.notifyNext (/Users/mengwei/kibana/node_modules/rxjs/src/internal/operators/switchMap.ts:141:24)
    at InnerSubscriber._next (/Users/mengwei/kibana/node_modules/rxjs/src/internal/InnerSubscriber.ts:17:17)
    at InnerSubscriber.Subscriber.next (/Users/mengwei/kibana/node_modules/rxjs/src/internal/Subscriber.ts:102:12)
    at MapSubscriber._next (/Users/mengwei/kibana/node_modules/rxjs/src/internal/operators/map.ts:86:22)
    at MapSubscriber.Subscriber.next (/Users/mengwei/kibana/node_modules/rxjs/src/internal/Subscriber.ts:102:12)
    at DistinctUntilChangedSubscriber._next (/Users/mengwei/kibana/node_modules/rxjs/src/internal/operators/distinctUntilChanged.ts:121:24)

 FATAL  TypeError: Cannot read property 'appenders' of undefined

@mshustov mshustov self-assigned this Apr 11, 2019
@mshustov
Copy link
Contributor

mshustov commented Apr 11, 2019

problem 1. Cannot run Kibana server in dev mode when New platform plugin specifies config in kibana.yml

steps to reproduce:

  • set core.testbed.secret: secret-value in kibana.yaml
  • run yarn start
  • result: Error: "core.testbed.secret" setting was not applied. Check for spelling errors and ensure that expected plugins are installed.

This error happens on Optimizer server, where plugins.initialize:false, so New platform plugin don't run and don't read from config

@mshustov
Copy link
Contributor

I believe that log line you linked would only fire if something actually subscribed to the exposed data$ observable.

true. to check it works

  public setup(setupContext: PluginSetupContext, deps: Record<PluginName, unknown>) {
    this.log.debug(
      `Setting up TestBed with core contract [${Object.keys(setupContext)}] and deps [${Object.keys(
        deps
      )}]`
    );
    const data$ = this.initializerContext.config.create(TestBedConfig).pipe(
      map(config => {
        this.log.debug(`I've got value from my config: ${config.secret}`);
        return `Some exposed data derived from config: ${config.secret}`;
      })
    );

    data$.subscribe(console.log);
    return {
      data$,
    ...

@mshustov
Copy link
Contributor

mshustov commented Apr 11, 2019

@mw-ding how can I reproduce the error on your screenshot? I tried to add an invalid property to logging config and start immediately failed

regarding #34812 (comment)
New platform plugins are discovered in

  • plugins
  • src/plugins
  • kibana-extra (btw it's breaking change that legacy platform plugins aren't handled here)

Legacy platform plugins are discovered in

  • plugins (when not New plugin)
  • src/legacy/core_plugins
  • x-pack

So Legacy platform doesn't know about New platform plugins status (initialized, disabled, run, etc.). I'd suggest excluding all config keys for New Platform plugins from this check https://github.com/elastic/kibana/blob/master/src/legacy/server/config/complete.js#L27
Because config keys are validated when New platform creates plugin config. If a plugin is disabled or not initialized it can be reported, but shouldn't prevent the server from running.

@mshustov
Copy link
Contributor

mshustov commented Apr 12, 2019

I talked to @azasypkin and he mentioned that we already have related issue to validate config upfront #20303
After discussion we decided to consider next refactoring:

  • define schema field on plugin & core service class
  • run plugin discovery before server starts and collect all plugin manifests and definitions
  • run config validation for plugin & core service definitions
  • if config successfully validated, create Root and start server
  • (optional) core service or plugin receive Observable<validated schema> when created, instead of manually subscribing to config service

@mw-ding
Copy link
Contributor

mw-ding commented Apr 15, 2019

@mw-ding how can I reproduce the error on your screenshot? I tried to add an invalid property to logging config and start immediately failed

Cannot reproduce the TypeError now. And yes, it immediately failed after providing an invalid property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:New Platform low
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants