cloud plugin disabled by default

[afharo]

Currently, the cloud plugin is always enabled, even for non-ESS distributions.

It was disable-able until 8.0 #113495, but it was still enabled by default.

Should we allow this plugin to be disable-able?
Also, should it be disabled by default?

[elasticmachine]

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

[pgayvallet]

IIRC, the reasoning to not add the cloud plugin to the list of disable-able plugins was that is wasn't necessary:

• the plugin is aware of whether we're running on cloud or not, and therefor can decide which features it needs to enable / logic it needs to execute accordingly.
• consumers can rely on the cloudSetup.isCloudEnabled property to follow the same logic.

So, to reverse the question:

Should we allow this plugin to be disable-able

Which upsides do you see doing so?

[afharo]

Which upsides do you see doing so?

• IMO, smaller bundle size.
• consumers already use cloudSetup?.isCloudEnabled across Kibana.
• if a plugin should only run on cloud, it can set it as a required dependency and it won't load (smaller bundle size++).
• simplification of the code in the cloud plugin (it's polluted with tons of if (this.isCloudEnabled)).
• cloud also exposes other APIs like cloudSetup.instanceSizeMb. Consumers shouldn't need to const instanceSizeMb = cloudSetup.isCloudEnabled ? cloudSetup.instanceSizeMb : undefined`. If the plugin is there it should just be enough.

IMO, using isCloudEnabled to register routes & features inside the cloud plugin seems like a workaround to our plugin management system.

[pgayvallet]

IMO, smaller bundle size.

Fair one.

if a plugin should only run on cloud, it can set it as a required dependency and it won't load

FWIW, that's not how our plugin dependency system works. In such scenario, Kibana would just refuse to start.

simplification of the code in the cloud plugin (it's polluted with tons of if (this.isCloudEnabled)).

Not gonna disagree on this one, but these checks are only here because of bad architectural / API design ihmo. It could be worked around by wrapping the cloud API altogether and just not returning it at all from the plugin's contracts when not in cloud, as some bridge plugins already do:

kibana/src/plugins/saved_objects_tagging_oss/public/types.ts

Lines 24 to 35 in 4584a8b

	export interface SavedObjectTaggingOssPluginStart {
	/**
	* Returns true if the tagging feature is available (if a provider registered the API)
	*/
	isTaggingAvailable(): boolean;
	/**
	* Returns the tagging API, if registered.
	* This will always returns a value if `isTaggingAvailable` returns true, and undefined otherwise.
	*/
	getTaggingApi(): SavedObjectsTaggingApi | undefined;
	}

CloudPlugin.setup could just looks like (and same for start):

setup() {
  if(notOnCloud) {
     return { isCloudEnabled: false, cloudAPI: undefined }
  }
  // normal contract return

}

Which is not perfect, but the recommended design for such 'soft disable-able' plugins.

cloud also exposes other APIs like cloudSetup.instanceSizeMb. Consumers shouldn't need to const instanceSizeMb = cloudSetup.isCloudEnabled ? cloudSetup.instanceSizeMb : undefined`. If the plugin is there it should just be enough.

Yea, it would be slightly better. But same as previous point, this is how this is done for the SOT optional dependency everywhere:

const taggingApi: SavedObjectsTaggingApi | undefined = SOT.isTaggingAvailable() ? SOT.getTaggingApi() : undefined.
// later
if (taggingApi) {
  // tagging feature available, do something.
}

Overall, the decision of limiting the number of plugins we can disable was mainly driven by the fact that it reduces the number of permutations we have to run the FTR against (as in theory we should be running against every entry in the disabled-plugin matrix permutations).

But I'd love to see the other teammates's thoughts on that question. We can totally reopen the discussion if necessary.

[lukeelmers]

Overall, the decision of limiting the number of plugins we can disable was mainly driven by the fact that it reduces the number of permutations we have to run the FTR against (as in theory we should be running against every entry in the disabled-plugin matrix permutations).

++ This was the primary reasoning for not making plugins disable-able by default. The thinking was that unless there is a valid use case for for using it which could be tested separately (like an experimental new plugin), it only adds more complexity to the system.

IMO the two most compelling arguments for making the cloud plugin disable-able would be:

• bundle size
• avoiding loading any cloud-related code for on prem users

But as pointed out above, we had decided for 8.0 that:

• the benefits of a slightly smaller bundle size (cloud bundle is currently just ~11k anyway) were outweighed by the benefits of having a simpler system overall. plus we have visibility into those bundle sizes on CI so we can avoid regressions
• loading the cloud plugin itself on prem wasn't a problem as long as it immediately performed checks to ensure we were running in cloud, and returned early if we weren't (plus, "cloud first", etc)

I'm also fine with reopening that discussion, but I'd just want to make sure we have a really compelling list of reasons before reverting to the older behavior.

[rudolf]

I agree with the summary and theoretical benefits so I believe this is something we might want to re-evaluate in the future. But if the end-user benefit is just 11k then I'm not sure it justifies spending time on it right now.

[afharo]

if a plugin should only run on cloud, it can set it as a required dependency and it won't load

FWIW, that's not how our plugin dependency system works. In such scenario, Kibana would just refuse to start.

I may have a wrong understanding of this, but, AFAIK, if plugin C depends on plugin B (it lists "pluginB" in the requiredPlugins list in the kibana.json manifest) and plugin B is disabled, Kibana will load everything except plugins B & C. AFAIK, it doesn't crash.

Overall, the decision of limiting the number of plugins we can disable was mainly driven by the fact that it reduces the number of permutations we have to run the FTR against (as in theory we should be running against every entry in the disabled-plugin matrix permutations).

I understand this (and I am a huge ++) for plugins like maps that, after removing their disable-ability, we don't have the disabled permutation anymore.

However, IMO, cloud, as it is right now, introduces those complexities via its soft-disabled approach. In our FTRs, we load the cloud plugin, sure, but we don't run 90% of the code because it sits behind isCloudEnabled and cloud.featureX.enabled flags. So, even tests running on Cloud might not cover its internal cloud.featureX.enabled-driven features.

All that said, I'm OK with waiting until we have a stronger requirement for this. We have so many things on our plate so far.

[pgayvallet]

AFAIK, if plugin C depends on plugin B (it lists "pluginB" in the requiredPlugins list in the kibana.json manifest) and plugin B is disabled, Kibana will load everything except plugins B & C. AFAIK, it doesn't crash

Nope, it does crash.

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

Lines 326 to 330 in 9b75efd

	if (pluginsDependenciesGraph.size > 0) {
	const edgesLeft = JSON.stringify([...pluginsDependenciesGraph.keys()]);
	throw new Error(
	`Topological ordering of plugins did not complete, these plugins have cyclic or missing dependencies: ${edgesLeft}`
	);

kibana/src/core/server/plugins/plugins_system.test.ts

Lines 157 to 163 in a969d42

	test('`setupPlugins` throws plugin has missing required dependency', async () => {
	pluginsSystem.addPlugin(createPlugin('some-id', { required: ['missing-dep'] }));
	await expect(pluginsSystem.setupPlugins(setupDeps)).rejects.toMatchInlineSnapshot(
	`[Error: Topological ordering of plugins did not complete, these plugins have cyclic or missing dependencies: ["some-id"]]`
	);
	});

[afharo]

Thanks for confirming... somehow, I recalled that being the case in some scenarios (telemetry.enabled: false) was allowed up until a few months ago, and telemetryManagementSection was behaving as I described... I guess it's a UI-only plugin...

[pgayvallet]

Strange, because atm the PluginSystem don't make the distinction between browser and server-side plugins during the topological sorting (even if it should probably do).

The filtering is done later in the initialization chain, here for server plugins:

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

Lines 93 to 95 in 9b75efd

	[...this.getTopologicallySortedPluginNames()]
	.map((pluginName) => [pluginName, this.plugins.get(pluginName)!] as [string, PluginWrapper])
	.filter(([pluginName, plugin]) => plugin.includesServerPlugin)

and here for ui plugins

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

Lines 247 to 249 in 9b75efd

	const uiPluginNames = [...this.getTopologicallySortedPluginNames().keys()].filter(
	(pluginName) => this.plugins.get(pluginName)!.includesUiPlugin
	);

So it should have been crashing.

But anyway,

If for any reason at some point we want to change the behavior to exclude plugins with missing deps instead of terminating the process, I'm fine-ish with it, as long as we think carefully about the consequences (and as long I'm not the one changing the algorithm to be able to dissociate plugins with missing deps from plugins with cyclic dependencies, as Kahn's doesn't make that distinction )

[afharo]

Closing this issue... still no need to disable it :)