Whitelist 'enabled' config for Platform plugins

[rudolf]

If a plugin doesn't explicitly define an enabled config schema, then disabling the plugin in kibana.yml will throw a validation error.

//kibana.yml
apm_oss.enabled: false

// throws
log   [12:21:46.564] [fatal][root] { Error: [config validation of [apm_oss].enabled]: definition for this key is missing

[elasticmachine]

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

[mshustov]

to note: In LP we require a plugin to define it explicitly, fallback to the deault schema otherwise

kibana/src/legacy/plugin_discovery/plugin_config/schema.js

Lines 28 to 32 in 8e9a8a8

	const DEFAULT_CONFIG_SCHEMA = Joi.object()
	.keys({
	enabled: Joi.boolean().default(true),
	})
	.default();

[joshdover]

On second thought, should enabled be allowed for every plugin? I think we may have some plugins that we don't ever want to be disabled, for example, the data plugin.

@elastic/kibana-app-arch do you have any thoughts on this? Is there any valid reason we would want to disable some of the more "core" plugins? The only thing I can think of is for testing a slimmer Kibana for some reason.

[lukeelmers]

Kibana would be pretty useless without the data plugin enabled, although data does have a few required dependencies that must remain enabled as well:

• data
  • uiActions
  • expressions
    • bfetch
    • inspector

The only thing I can think of is for testing a slimmer Kibana for some reason.

This is the only valid reason I can think of as well, and I believe it is an approach APM uses in testing. But I would argue that it might actually make life easier for folks in this scenario, because it would prevent them from accidentally disabling a plugin that was somewhere in data's dependency tree. Most likely they would need all of these enabled for their tests anyway.

My vote would be to stick with the legacy platform approach of requiring all plugins to have an enabled option (or just set it behind the scenes for them), and possibly make an exception for items in the list above. It feels like it'd be asking for trouble if any plugin had the ability to make themselves "mandatory".

[joshdover]

My vote would be to stick with the legacy platform approach of requiring all plugins to have an enabled option (or just set it behind the scenes for them), and possibly make an exception for items in the list above. It feels like it'd be asking for trouble if any plugin had the ability to make themselves "mandatory".

This is where I'm leaning as well, though I'd like to make it a bit less confusing than legacy and not provide a default schema at all if none is specified.

The only downside I can think of to this approach is documenting the enabled config option and making that discoverable.

If we do go with this approach, there's really no work to do here in Core itself. It is up to plugins to make sure their config schema includes enabled.

---

Alternatively, we could add the enabled key to a plugin's config schema only if it is not already specified which would still allow plugins like data to specify that enabled must be true:

export const config = {
  schema: schema.object({
    enabled: schema.oneOf([true], { defaultValue: true })
  })
}

The code in Core would look something like (some of these APIs don't currently exist):

if (!plugin.config.schema.includes('enabled')) {
  plugin.config.schema.extend({
    enabled: schema.boolean({ defaultValue: true })
  });
}

[pgayvallet]

My vote would be to stick with the legacy platform approach of requiring all plugins to have an enabled option

+1 for that.

[joshdover]

Let's do this: go with what we have now (require plugins to specify this option explicitly) and later work on improving the DX here in the future (by implementing a bit more magic as proposed above).

[joshdover]

I want to note that the NP actually does mirror the legacy behavior here: if no config schema is specified at all, plugins still have an implicit <configPrefix>.enabled config that is allowed and respected by the plugin discovery process.

However this value is not exposed to plugins (not particularly useful anyways).

[lukeelmers]

the NP actually does mirror the legacy behavior here: if no config schema is specified at all, plugins still have an implicit <configPrefix>.enabled config that is allowed and respected by the plugin discovery process.

Just wanted to note for posterity that this no longer appears to be the case. Currently, setting myPlugin.enabled to any value will cause config validation to fail if the plugin does not explicitly have enabled in their schema.

If enabled is not defined, we will still implicitly consider the plugin to be enabled during discovery as mentioned above:

kibana/packages/kbn-config/src/config_service.ts

Lines 177 to 183 in 237256a

	// if plugin hasn't got a config schema, we try to read "enabled" directly
	const isEnabled = validatedConfig?.enabled ?? config.get(enabledPath);
	// not declared. consider that plugin is enabled by default
	if (isEnabled === undefined) {
	return true;
	}

However, the <configPrefix>.enabled config will not be allowed.

So at this point, the only reason for a plugin to specify enabled in their config is if they want their plugin to be "disable-able".

EDIT: Josh has helpfully clarified in #89584 (comment) that the implicit config is in fact added, but only when there isn't a config schema to begin with (key words I missed above are if no config schema is specified at all, which I translated in my mind to "if enabled doesn't exist in the config schema". 🙂 However, plugins with a config schema but no enabled setting will not have one implicitly added.