Log deprecation warnings when plugins are disabled but will no longer be disable-able in 8.0

[lukeelmers]

As a part of the effort in #89584 to remove the ability for plugins to be disable-able by default, we need to log deprecation warnings when the myplugin.enabled config option is used.

This change affects all plugins except those outlined in #89584 (comment).

Since there are so many affected plugins, and some of them don't define a config schema at all, we'll need to think of the most efficient way to log these deprecations (i.e. we may want to consider handling this inside core rather than attempting to log a warning from every single plugin).

Once this task is complete, we can move forward with addressing #89584 and implementing the actual breaking change.

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[lukeelmers]

Cross-posting @mshustov's comment from #89584 (comment):

An implementation-wise question: as we want to enforce this rule for the internal plugins-only, can we add a dedicated flag to the manifest file to differentiate between 1st party and 3rd party plugins? It will help us in the future to add more strict / relax rules for the Elastic plugins (like this one #61087)

These changes will still affect 3rd party plugin devs in that plugins without config schemas will no longer be disable-able. So ideally we are logging deprecation warnings for:

1. Plugins which currently define a config schema that has an enabled property. These will just be 1st party / internal since we won't have control over 3rd party plugins.
   • For these we can either manually add the deprecations to each plugin, or try to handle them in core
2. Plugins which do not define a config schema, and therefore implicitly have an enabled setting added. These will be both 1st and 3rd party plugins.
   • For these, we should be ideally be adding a config schema for the plugins which must keep an enabled setting, and then logging deprecation warnings for all other plugins in this category

So as for adding a flag to differentiate between 1st and 3rd party plugins, I think that would only be necessary if we took the "handle deprecation logs in core" approach for plugins falling into category (1). But I do agree it would be useful to have this distinction, so might be worth doing as a part of this work if it isn't a huge effort.

[pgayvallet]

This change affects all plugins except those outlined in #89584 (comment).

Are we going to keep an hard-coded list inside core to handle these exceptions?

These changes will still affect 3rd party plugin devs in that plugins without config schemas will no longer be disable-able

It would be doable to identify third party plugins and preserve the old behavior for them. We did that at some point to log deprecation warnings to legacy 3rd party plugins. At the time, we simply used the plugin's path to identify them (src/plugin and x-pack/plugins vs anywhere else). This is probably better than adding an internal flag to the plugin's descriptor, as nothing would block a 3rd party dev to just add the flag to its manifest.

Plugins which currently define a config schema that has an enabled property. For these we can either manually add the deprecations to each plugin, or try to handle them in core

Doing that from core seems way more robust, so it should imho be the preferred approach, if that's technically doable. Also, if we were to add deprecation to each plugin, we wouldn't be able to do it for 3rd parties.

Technically, there is currently no API to check if an ObjectType has a defined key, however this could easily be added.

kibana/src/core/server/plugins/types.ts

Line 28 in 52ee65b

export type PluginConfigSchema<T> = Type<T>;

The PluginConfigDescriptor.schema type is currently a plain Type, and not an ObjectType, but I think that in practice, all plugins schemas are ObjectType, so maybe we should even enforce that from core.

myPlugin.schema = schema.string()

doesn't really make a lot of sense.

[mshustov]

At the time, we simply used the plugin's path to identify them (src/plugin and x-pack/plugins vs anywhere else).

We can do it, for sure.

This is probably better than adding an internal flag to the plugin's descriptor, as nothing would block a 3rd party dev to just add the flag to its manifest.

Who said that we cannot prevent that by validating the manifest file? Also, there is always an option to use Symbol to emulate a private property https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol#symbol_wrapper_objects_as_property_keys

The PluginConfigDescriptor.schema type is currently a plain Type, and not an ObjectType, but I think that in practice, all plugins schemas are ObjectType, so maybe we should even enforce that from core.

+1

These changes will still affect 3rd party plugin devs in that plugins without config schemas will no longer be disable-able

Yeah, it's fine as long as they are able to fix that by changing the config declaration.

[pgayvallet]

Who said that we cannot prevent that by validating the manifest file?

Sure, but such validation will require to differentiate 1st vs 3rd party plugins anyway to see which are allowed to set the flag to true.

Are we expecting all our plugins to be internal? If so, given that we'll have to identify the plugin source to perform the validation, do we see any value in adding a flag?

[mshustov]

If so, given that we'll have to identify the plugin source to perform the validation, do we see any value in adding a flag?

Nor really, as long as this validation logic is kept in the manifest validator file.
Btw, it's turned out @jportner already added the source field to PluginWrapper

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

Line 77 in 1bf765d

this.source = getPluginSource(params.path);

So we have this information at a later stage.

[lukeelmers]

Yeah, it's fine as long as they are able to fix that by changing the config declaration.

This is the approach I am leaning towards -- 3rd party plugins which have *.enabled added by default can still preserve this behavior, it just requires them to create a config schema with the property.

So the approach I'm starting with now is:

• We log a deprecation warning from core for any plugin with an .enabled config that's implicitly added.
• We manually add a deprecation warning outside of core for any plugin with an .enabled config that's explicitly added in their config schema.
• Any plugin can get the old behavior back by creating a config schema with an .enabled property

[pgayvallet]

We manually add a deprecation warning outside of core for any plugin with an .enabled config that's explicitly added in their config schema

What 'manually' means exactly here? By adding deprecations in each plugin's descriptor?

[lukeelmers]

What 'manually' means exactly here? By adding deprecations in each plugin's descriptor?

Yes, as we will need to remove those enabled settings from each individual plugin anyway, I feel like we might as well add explicit deprecations to each plugin too.

I have a draft PR up if you want to take a look and add feedback: #111890