Introduce support for the server-side new platform plugins. #25473

azasypkin · 2018-11-09T17:50:50Z

In this PR I want to introduce the initial support for the server-side new platform plugins including, but not limited to:

Topological ordering of the plugins before startup (based on the implementation in the old new-platform branch)
Automatic disabling of the plugins that have missing or disabled plugins within their direct or transitive dependencies
Handling of optional plugin dependencies
Starting and stopping of the plugins based on the topological order
Support for mutual coexistence of new and legacy plugins (e.g. handling of config keys used only in the core, use scanDirs/path for legacy Kibana only etc.).

Here is example of plugin pseudo definition:

src/plugins/testbed/index.ts

import { PluginInitializerContext, PluginName, PluginStartContext } from '../../../';

interface Plugin {
  constructor(initializerContext: PluginInitializerContext);
  start: (startContext: PluginStartContext, deps: Record<PluginName, unknown>) 
      => Promise<TStartContract>;
  stop?: () => Promise<void>;
}

export const plugin = (initializerContext: PluginInitializerContext) => new Plugin(initializerContext);

Part of #18840

elasticmachine · 2018-11-09T17:50:53Z

Pinging @elastic/kibana-platform

azasypkin · 2018-11-09T19:40:41Z

packages/kbn-config-schema/src/types/type.ts

@@ -38,7 +38,7 @@ export abstract class Type<V> {
   */


note: not related change, please ignore, I'll create a dedicated PR to cleanup TS a bit in this package.

azasypkin · 2018-11-09T19:41:22Z

src/cli/serve/serve.js

@@ -165,7 +165,8 @@ export default function (program) {
      pluginDirCollector,
      [
        fromRoot('plugins'),
-        fromRoot('src/core_plugins')
+        fromRoot('src/core_plugins'),
+        fromRoot('src/plugins')


note: this change will go in a dedicated PR probably with testbed-like plugin for the new platform.

azasypkin · 2018-11-09T19:43:21Z

src/core/server/config/config_service.ts

    const config = await this.config$.pipe(first()).toPromise();
    const handledPaths = this.handledPaths.map(pathToString);

    return config.getFlattenedPaths().filter(path => !isPathHandled(path, handledPaths));
  }

+  public async getUsedPaths() {


note: need this for the legacy platform so that it doesn't complain about keys that core knows about (new keys maybe, plugin keys as well).

We'll still need to figure out what to do with the new platform plugin config keys that are not used immediately during startup (like in the testbed plugin in this PR). Maybe we should forbid that and supply config as part of the PluginCore when we start plugin so that these keys are always marked as used....

azasypkin · 2018-11-09T19:47:16Z

src/core/server/index.ts

-    this.plugins = new PluginsModule(configService, logger, env);
-    this.legacy = new LegacyCompatModule(configService, logger, env);
+
+    const core = { env, configService, logger };


note: I need this "bundle" (+ http in the future) in many places so I called them KibanaCore :)

I don't usually care that much about naming, but I think this one is a little too ambiguous. The notion of "core" is generic enough to be confusing in its own right, and we make that worse by referring to multiple different things core.

Let's have "kibana core" refer to the src/core module. If you're talking in the context of the server, "kibana core" might alternatively refer to src/core/server, which is the only module that is relevant to you. If you're talking in the context of the browser, then src/core/public.

Perhaps something like "base services" would be more appropriate here?

Yeah, it's confusing, I didn't use services because of env that is not a service like configService, but maybe we can call it a "service" in a broader sense, so base services works for me - will rename.

azasypkin · 2018-11-09T19:49:09Z

src/core/server/legacy_compat/logging/legacy_logging_server.ts

+ * Converts core log level to a one that's known to the legacy platform.
+ * @param level Log level from the core.
+ */
+function getLegacyLogLevel(level: LogLevel) {


note: noticed that during testing, good doesn't color warn and trace, so here I pick alternatives supported by good.

azasypkin · 2018-11-09T19:53:59Z

src/core/server/plugins/discovery/plugin_manifest_parser.ts

@@ -126,6 +88,21 @@ export async function parseManifest(pluginPath: string, packageInfo: PackageInfo
    );
  }

+  if (


note: I was testing various ideas with configPath and it ended up in the manifest itself, so that I can quickly check whether plugin enabled or not without loading a "definition" file. I think it makes sense to keep it here, since it's only used internally by the PluginCore and I can't think of any reason to keep it inside of the plugin definition in JS/TS... But please tell me if you know any reason to not keep it inside of manifest.

Also maybe we want to forbid using some "reserved" config paths to protect from reading arbitrary config values by plugins. But since it's in manifest and easily auditable it may not be a big deal.

src/core/server/plugins/discovery/plugin_manifest_parser.ts

+      )
+    );
+  }
+
  const expectedKibanaVersion =


azasypkin · 2018-11-09T19:56:26Z

src/core/server/plugins/discovery/plugin_manifest_parser.ts

+ * plugin path is handled by the core plugin system or not.
+ * @param pluginPath Path to the plugin.
+ */
+export async function hasPluginManifest(pluginPath: string) {


note: was the easiest way I could think of...

src/core/server/plugins/discovery/plugins_discovery.ts

azasypkin · 2018-11-09T21:22:23Z

src/plugin_discovery/plugin_pack/package_json_at_path.js

+  // If plugin directory contains manifest file, we should skip it since it
+  // should have been handled by the core plugin system already.
+  Rx.defer(() => hasPluginManifest(path)).pipe(
+    mergeMap(hasPluginManifest => hasPluginManifest ? [] : createPackageJsonAtPath(path)),


note: I'd rather inject this "isnewplatformplugin" function through kbnServer.core, but it's too much hassle to pass it down to here, quite many hops :/

azasypkin · 2018-11-09T21:29:41Z

@elastic/kibana-platform would be great if you can give any preliminary feedback on this PR - I've left a couple of questions in places where I'm particularly interested in what you think. Thanks!

…ixed plugin search paths, use classes for test plugins, separate init and start core plugin values and more.

azasypkin · 2018-11-16T08:22:16Z

src/core/server/config/__snapshots__/env.test.ts.snap

+  "pluginSearchPaths": Array [
+    "/test/cwd/src/plugins",
+    "/test/cwd/plugins",
+    "/test/cwd/../kibana-extra",


note: don't be confused, .. is preserved because I mock path.resolve in tests.

azasypkin · 2018-11-16T08:24:26Z

src/core/server/config/env.ts

  name: 'development' | 'production';
  dev: boolean;
  prod: boolean;
 }

+/** @internal */


note: I'm adding a bunch of @internal here and there as I'm exposing core types and noticing types that shouldn't be exposed.

azasypkin · 2018-11-16T08:31:28Z

src/core/server/legacy_compat/legacy_service.ts

            }
          : { autoListen: false },
+      handledConfigPaths: await this.core.configService.getUsedPaths(),


note: I decided to keep this structure for now (kbnServer.core --> { serverOptions, handledConfigPaths, plugins }) to group everything that core provides to the legacy world, but we can later split into core and plugins as we were discussing.

azasypkin · 2018-11-16T08:34:48Z

src/core/server/legacy_compat/legacy_service.ts

@@ -157,7 +163,7 @@ export class LegacyService implements CoreService {

  private setupProxyListener(server: HapiServer) {
    const legacyProxy = new LegacyPlatformProxy(
-      this.logger.get('legacy', 'proxy'),
+      this.core.logger.get('legacy-proxy'),


note: while working on this I had to see lots of logs, and noticed that legacy logger alphabetically sorts tags that represent new logger context and it's not something we want.

azasypkin · 2018-11-16T09:56:08Z

src/core/server/plugins/plugin.ts

+    this.optionalDependencies = manifest.optionalPlugins;
+  }
+
+  public init(core: PluginInitializerCore) {


note: it was a part of start initially, but I've moved it into separate function since we need another arguments for the initializer, we still don't support init lifecycle event for plugins though.

Can you pass PluginInitializerCore to the constructor like you do with KibanaCore elsewhere? If so, the logic can go back to being kicked off in start (maybe through a private initializePlugin function), and we avoid the ambiguity of adding an init function to this lightweight wrapper around plugins, especially since we might some day add a real init lifecycle handler.

Yes, I can, the only thing is that we'll have to create PluginInitializerCore even for disabled plugins, but we likely aren't going to have hundreds of disabled plugins so I maybe don't have to worry about that ...

Btw, since KibanaCore is BaseServices now, do you prefer replacing Core with BaseServices in PluginInitializerCore and friends as well for consistency @epixa ?

Okay, now it's CoreContext, PluginInitializerContext and PluginStartContext, I'm passing PluginInitializerContext into constructor as you suggest. Let's see how it goes,

azasypkin · 2018-11-16T10:26:37Z

src/core/server/plugins/plugins_config.ts

-  paths: schema.arrayOf(schema.string(), {
-    defaultValue: [],
-  }),
+  scanDirs: schema.maybe(schema.arrayOf(schema.string())),


note: we don't use scanDirs and paths in new platform as we agreed, but we still use initialize and if we don't define scanDirs and paths in schema validation will fail (unknown keys under plugins). There are several solutions to this problem:

Ideally I'd add another new platform only config option with a similar meaning and decoupled from plugins.* used by the legacy platform - requires "patching" of several places where we pass plugins.initalize=false already

add support for ignoreUnknownKeys option for schema.object, a bit too relaxed and dangerous for the config validation

define scandDirs and paths in the schema - the easiest option I chose here

Actually there is one more way, we can just pick initialize in legacy config adapter like we do for other config keys or even transform it into something else that we want to see in new platform.

Is there a downside to that last approach you mentioned? What's your preference at this point?

Is there a downside to that last approach you mentioned?

I can't think of any downside, we do this for server config and it works pretty well.

What's your preference at this point?

The last one, I forgot about it when I was working on this PR and remembered again while fixing regression with SSL config deprecations :)

Cool. The last one sounds good to me as well.

azasypkin · 2018-11-16T10:28:11Z

src/core/server/plugins/plugins_service.ts

+    await this.handleDiscoveryErrors(error$);
+    await this.handleDiscoveredPlugins(plugin$);
+
+    if (!config.initialize || this.core.env.isDevClusterMaster) {


note: hopefully we'll move cluster manager out of the main source code to a separate independent script/package and remove this this.core.env.isDevClusterMaster

epixa

Configurations seems to be handled inconsistently, and I'm not sure whether it's the result of this PR or not.

If I add core.testbed.secret: "woah" to kibana.yml, then node scripts/kibana starts up properly and reflects the non-default config value.

If I add core.testbed.secret: "woah" to kibana.dev.yml, then node scripts/kibana starts up properly but does not reflect the non-default config value.

If I add core.testbed.secret: "woah" to either kibana.yml or kibana.dev.yml, then node scripts/kibana --dev fails startup with the exception Error: "core.testbed.secret" setting was not applied. Check for spelling errors and ensure that expected plugins are installed.

azasypkin · 2018-11-29T21:26:23Z

If I add core.testbed.secret: "woah" to either kibana.yml or kibana.dev.yml, then node scripts/kibana --dev fails startup with the exception Error: "core.testbed.secret" setting was not applied. Check for spelling errors and ensure that expected plugins are installed.

First two behaviors seem to be correct, regarding third: I guess, it's optimizer process that fails? If yes, it's a known issue I mentioned in #25473 (comment). Let me know if you disagree with my plan and I'll try to fix it in this PR somehow.

src/core/server/plugins/discovery/plugin_manifest_parser.test.ts

epixa · 2018-11-29T21:19:56Z

src/core/server/plugins/plugins_system.ts

+import { createPluginInitializerCore, createPluginStartCore } from './plugins_core';
+
+/** @internal */
+export class PluginsSystem {


I'm OK with this naming choice for the sake of an initial merge, but I think in the end we should change this name. In isolation I think PluginsSystem is fine, but in the spirit of having the stuff in core be an example for future development within core and plugins alike, I think a more clear structure is in order for the the plugin module.

Let's look to core itself for inspiration here.

There's a top level service definition for core, we happen to call it Server, but effectively it is just the same thing as any other service definition. It's a class that takes in some constructor arguments from its initializer, it contains start/stop lifecycle events to kick off and tear down the relevant domain logic, etc.

Simply looking at the top-most directory structure of the core module you get a pretty good idea of its features, responsibilities, etc.

core config http legacy logging plugins index.js // the interface for the core module

The directory structure can't describe how these features and responsibilities work together or how they get exposed, but it at least gives a good indication of what the core module is all about.

There are currently also some oddities in core that I didn't list above like root and dev, but I think of those things as technical debt that we chose to take on for the sake of practicality and that we can resolve later. And I left out some name and path details for the sake of explanation.

We should strive to have that same level of clarity from each of the services within core as well.

When I crack open plugin, I'd expect to see a file structure that was equally as expressive as that of core itself. Perhaps something like:

core plugins config // config schema validation discovery // static manifest discovery loader // importing plugin interface and instantiation lifecycle // invoke start/stop/init(?) with relevant contracts index.js // the interface for the core/plugins module

I might not have the right break down there, but hopefully it illustrates my point.

This feedback is really not unique to plugins, so if you'd prefer not to heed it in this PR, I'm fine with that. But let's at least get on the same page about it and then we can apply it to future service design and come back and refactor some of the existing core service designs at a later date.

On further reflection, this PR is already pretty massive, so I don't think overhauling the existing shape of the plugin module in it makes sense. I'd love your feedback so we can get aligned on it going forward.

Hehe, I was struggling with finding the right name and just ended up with what we had in new-platform branch (same as I did for Server and Root) :) I don't like our Server, Root, PluginSystem and a couple more things, these are confusing and meaningless names/notions IMO.

Up until this point SomethingService was a central thing for core/something and SomethingModule served as a grouping mechanism, mainly because of simplicity of Something, but I think ModuleSomething (e.g. core/something/index.ts in your example) should become a central thing and be an "interface" for Something that other SomethingElse from the core can interact with. Details to be discussed though.

When I crack open plugin, I'd expect to see a file structure that was equally as expressive as that of core itself.
On further reflection, this PR is already pretty massive, so I don't think overhauling the existing shape of the plugin module in it makes sense. I'd love your feedback so we can get aligned on it going forward.

I haven't dug that deep yet, but I think that's an interesting point that totally makes sense to me. And I agree that we can handle this in follow-ups more effectively.

epixa · 2018-11-29T21:29:07Z

src/core/server/plugins/plugins_system.ts

+      return exposedValues;
+    }
+
+    const sortedPlugins = this.getTopologicallySortedPluginNames();


I think topological sorting should happen during discovery instead of as an implementation detail when attempting to kick off the plugin lifecycle. There will be at least one other time when core needs plugins listed in the appropriate topological order: for client-side plugins, we will need to pass a properly sorted array of plugin names to the browser so that the client core can asynchronously load the plugin bundles in effectively the same order that they were loaded on the server, at least as far as dependencies are concerned.

I can't think of a situation where we'd want the results of plugins discovery and not have them already sorted.

What do you think?

@epixa have we considered the case when our graph includes plugins without UI (ui: false in manifest)? So if plugin A (ui: true) depends on plugin B (ui: false) then on the server we should run them in order, but on the client side order doesn't matter it seems and hence graph will look differently.

That's a really good point. Let's leave this where it is for now.

epixa · 2018-11-29T21:38:37Z

src/core/server/plugins/plugins_config.ts

-  paths: schema.arrayOf(schema.string(), {
-    defaultValue: [],
-  }),
+  scanDirs: schema.maybe(schema.arrayOf(schema.string())),


Is there a downside to that last approach you mentioned? What's your preference at this point?

epixa · 2018-11-29T21:45:51Z

src/core/server/plugins/plugin.ts

+    this.optionalDependencies = manifest.optionalPlugins;
+  }
+
+  public init(core: PluginInitializerCore) {


Can you pass PluginInitializerCore to the constructor like you do with KibanaCore elsewhere? If so, the logic can go back to being kicked off in start (maybe through a private initializePlugin function), and we avoid the ambiguity of adding an init function to this lightweight wrapper around plugins, especially since we might some day add a real init lifecycle handler.

src/core/server/plugins/discovery/plugin_manifest_parser.ts

+      )
+    );
+  }
+
  const expectedKibanaVersion =


epixa · 2018-11-29T21:52:26Z

src/core/server/index.ts

-    this.plugins = new PluginsModule(configService, logger, env);
-    this.legacy = new LegacyCompatModule(configService, logger, env);
+
+    const core = { env, configService, logger };


I don't usually care that much about naming, but I think this one is a little too ambiguous. The notion of "core" is generic enough to be confusing in its own right, and we make that worse by referring to multiple different things core.

Let's have "kibana core" refer to the src/core module. If you're talking in the context of the server, "kibana core" might alternatively refer to src/core/server, which is the only module that is relevant to you. If you're talking in the context of the browser, then src/core/public.

Perhaps something like "base services" would be more appropriate here?

epixa · 2018-11-29T22:03:11Z

If I add core.testbed.secret: "woah" to either kibana.yml or kibana.dev.yml, then node scripts/kibana --dev fails startup with the exception Error: "core.testbed.secret" setting was not applied. Check for spelling errors and ensure that expected plugins are installed.

First two behaviors seem to be correct, regarding third: I guess, it's optimizer process that fails? If yes, it's a known issue I mentioned in #25473 (comment). Let me know if you disagree with my plan and I'll try to fix it in this PR somehow.

Ah yes, that makes sense. I'm fine moving forward without this change, but we'll need to address it before folks start to do anything with plugins.

…eServices`.

elasticmachine · 2018-12-03T19:26:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-12-04T16:28:50Z

@epixa PR should be ready for another review pass, thanks!

Next I'd like to tackle this one #20303 and solve the "known issue" with optimizer process mentioned in this PR.

epixa

LGTM

I left one comment that might warrant a change, but if you don't agree with it, I think we're good to go as-is.

I didn't test these changes quite as thoroughly as I did in the first round of feedback, but the changes themselves look good and I started up kibana with various dev configurations (base path, no base path, --dev, no --dev). Everything looked appropriate in verbose logs as well.

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

epixa

Changes LGTM

elasticmachine · 2018-12-04T19:36:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2018-12-04T20:12:33Z

🎊 🎉 🎊 🎉

azasypkin · 2018-12-04T22:03:50Z

6.x/6.6: 09cd080

azasypkin added feedback_needed WIP Work in progress Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Nov 9, 2018

epixa mentioned this pull request Nov 9, 2018

Core plugins support (server side) #18840

Closed

6 tasks

azasypkin force-pushed the issue-xxx-core-plugin-graph branch from 4d4d008 to 96661d6 Compare November 9, 2018 19:39

azasypkin commented Nov 9, 2018

View reviewed changes

Introduce support for the server-side new platform plugins.

c381bad

azasypkin force-pushed the issue-xxx-core-plugin-graph branch from 96661d6 to c381bad Compare November 9, 2018 22:46

azasypkin added 3 commits November 14, 2018 13:41

Merge branch 'master' into issue-xxx-core-plugin-graph

b362c87

Review №1: rename hasPluginManifest to isNewPlatformPlugin, use f…

02b2802

…ixed plugin search paths, use classes for test plugins, separate init and start core plugin values and more.

Merge branch 'master' into issue-xxx-core-plugin-graph

d7cb1cc

azasypkin changed the title ~~[skip-ci][wip] Introduce support for the server-side new platform plugins.~~ Introduce support for the server-side new platform plugins. Nov 16, 2018

azasypkin requested a review from epixa November 16, 2018 08:17

azasypkin added review and removed WIP Work in progress feedback_needed labels Nov 16, 2018

Review №1: remove unrelated changes.

2138d18

azasypkin commented Nov 16, 2018

View reviewed changes

Fix config.complete tests.

90208b6

azasypkin commented Nov 16, 2018

View reviewed changes

Log only fatal log line in config integration tests.

4fb7a41

azasypkin commented Nov 16, 2018

View reviewed changes

elastic deleted a comment from elasticmachine Nov 16, 2018

epixa reviewed Nov 29, 2018

View reviewed changes

azasypkin added 7 commits November 30, 2018 13:41

Merge branch 'master' into issue-xxx-core-plugin-graph

9a6a33f

Rename KibanaCore to BaseServices.

d585e86

Rename PluginInitializer{Start}Core to `PluginInitializer{Start}Bas…

7e95d39

…eServices`.

Pass PluginInitializerBaseServices to Plugin constructor.

da59e08

Merge branch 'master' into issue-xxx-core-plugin-graph

6a58ce0

Use context instead of baseServices.

89c81a4

Strip plugins.scanDirs and plugins.paths from the plugins config.

fc13a1c

epixa removed the review label Dec 3, 2018

Add tests and more JSDoc for plugin.ts.

ee383b7

azasypkin requested a review from epixa December 4, 2018 16:29

epixa approved these changes Dec 4, 2018

View reviewed changes

src/core/server/plugins/plugin.test.ts Outdated Show resolved Hide resolved

azasypkin added 2 commits December 4, 2018 19:40

Merge branch 'master' into issue-xxx-core-plugin-graph

c27e140

Make Plugin.stop fail if plugin is not started.

a99e111

epixa approved these changes Dec 4, 2018

View reviewed changes

azasypkin merged commit 74e21c1 into elastic:master Dec 4, 2018

azasypkin deleted the issue-xxx-core-plugin-graph branch December 4, 2018 19:57

azasypkin added the backport pending label Dec 4, 2018

azasypkin mentioned this pull request Dec 4, 2018

[6.x] Introduce support for the server-side new platform plugins. (#25473) #26655

Merged

azasypkin added backported and removed backport pending labels Dec 4, 2018

epixa mentioned this pull request Dec 11, 2018

New Kibana platform #9675

Closed

                   );
                 }
+                if (

Introduce support for the server-side new platform plugins. #25473

Introduce support for the server-side new platform plugins. #25473

Conversation

azasypkin commented Nov 9, 2018 • edited Loading

elasticmachine commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epixa left a comment

Choose a reason for hiding this comment

azasypkin commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

epixa commented Nov 29, 2018

elasticmachine commented Dec 3, 2018

💚 Build Succeeded

azasypkin commented Dec 4, 2018

epixa left a comment

Choose a reason for hiding this comment

epixa left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 4, 2018

💚 Build Succeeded

spalger commented Dec 4, 2018

azasypkin commented Dec 4, 2018

azasypkin commented Nov 9, 2018 •

edited

Loading

azasypkin Dec 3, 2018 •

edited

Loading