Add mechansim for legacy plugins to depend on New Platform plugins

[joshdover]

Currently, legacy plugins can define their dependencies on other plugins via the require parameter passed to the kibana.Plugin constructor. If any of these dependencies are missing or disabled, Kibana refuses to start with the error:

Error: Unmet requirement "apm_oss" for plugin "apm"

With the migration effort, legacy plugins should be able to use New Platform plugins as dependencies (but not the other way around). This allows plugin developers to gradually move over functionality or extract common code into NP plugins before moving over the entire legacy plugin.

This creates a problem because these dependencies are not known and if the NP plugin that is being depended on is disabled, the legacy plugin that depends on it will produce errors at runtime rather than when Kibana starts.

Proposal

Add a new requireNewPlatform parameter to the kibana.Plugin constructor that behaves similarly to the require parameter and will refuse to start Kibana if some New Platform plugin is not present.

[elasticmachine]

Pinging @elastic/kibana-platform

[mshustov]

New platform plugins have a notion of lifecycle contracts. What type of contracts should be exposed to the Legacy plugins? setup/start/both? on both client and server sides?

[joshdover]

On the client side, we're currently exposing both the setup and start contracts. There could be issues with this since no legacy code runs on the client side until after the start lifecycle.

Alternatively, we could remove this bridge completely and require that legacy plugins depend on a shimmed version of the NP plugin.

[joshdover]

One possible solution to this is to create an API that allows any arbitrary code to get a version of the Core APIs defined for a "dynamic" plugin that does not "exist" in the New Platform proper, but is added later.

Example:

interface CoreSetup {
  runPlugin(plugin: {
    /** Support parts of the kibana.json manifest for the dynamically added plugin */
    manifest: Pick<PluginManifest, 'id' | 'configPath' | 'requiredPlugins' | 'optionalPlugins'>;
    
    /** Config schema for the plugin's configuration */
    schema: Schema;

    /** Function used to create a new Plugin instance */
    init(initializerContext: PluginInitializerContext): Plugin;

    /**
     * Extra fields to be merged with the default core & plugin services.
     * Used to shim services that have not yet been migrated to the New Platform.
     */
    extras?: DeepPartial<{
      setup: { core: object; plugins: object };
      start: { core: object; plugins: object };
    }>;
  }): boolean;
}

The plugin function should return an object that conforms to the existing Plugin interface. Example:

core.runPlugin({
  manifest: {
    id: 'my-legacy-plugin',
    requiredPlugins: ['data']
  },
  schema: schema.object({ myConfigField: schema.string() }),
  plugin: (initializerContext) => new MyLegacyPlugin(initializerContext),
  extras: {
    setup: {
      plugins: { legacyPluginA }
    }
  }
});

Behavior:

• This API would create the contracts for PluginInitializerContext, CoreSetup, CoreStart, and their plugin equivalents, and call the setup and start functions.
• If any of the plugins listed in requiredPlugins are not present or enabled, this function would return false, otherwise this function would return true. When false is returned, the init function will not be ran.
• The values in extras can be used to merge services into the arguments supplied to the Plugin's setup and start methods. This can be used to "shim" any services that are not yet migrated to the New Platform.

Some caveats:

• Legacy plugins don't have the same lifecycle events as New Platform plugins. This means they wouldn't be able to call some APIs in setup that cannot be called after start begins. For example, core.application.register cannot be called in the start phase, but no legacy code runs until after setup and start in the New Platform have been executed. For these instances, we could provide an alternate implementation as outlined in Add a legacy compatibility layer for ApplicationService #46481.
• New Platform plugins would not be able to depend on plugins registered in this way. This is by design.

[joshdover]

We discussed this offline this week and have decided that the work required to add this runPlugin API is probably not worth the effort. We have other patterns we can leverage in the meantime.

I will do some investigation on what the behavior should be when a LP plugin has a dependency on an NP plugin and how it should handle the situation where that NP plugin is disabled.

[lukeelmers]

I will do some investigation on what the behavior should be when a LP plugin has a dependency on an NP plugin and how it should handle the situation where that NP plugin is disabled.

@joshdover Was there an outcome on this yet, or any recommendations for how folks should be addressing it?

[mshustov]

Can we just do this by checking that requiredPlugins are enabled in NP or LP? Legacy plugins have access to all available NP plugins anyway.

[joshdover]

Apologies, I forgot to update this issue. Specifying dependencies is really two things:

• Giving legacy plugins a way to access NP plugins (which is now solved, including for context dependencies)
• Giving legacy plugins a way to crash Kibana if their dependencies are not available (not solved globally)

During their init function, legacy plugins should be able to check server.newPlatform.setup.plugins.* for their requirements, and if they are not enabled, throw an exception to crash the server.

It would be pretty trivial to add a property to the legacy plugin definition that could list the NP dependencies to do this crashing automatically. If we think this is a common case, then feel free to open this issue.

[lukeelmers]

It would be pretty trivial to add a property to the legacy plugin definition that could list the NP dependencies to do this crashing automatically. If we think this is a common case, then feel free to open this issue.

I don't think it's a huge deal to crash your plugin manually in init, mostly I'm interested in this question because I'm not sure it is something people are currently thinking about while they are shimming.

It may be worth documenting/communicating this as a best practice to reduce the likelihood of bugs coming up in 7.x.