Detect requiredBundles circular references on Kibana startup

[afharo]

We noticed that some plugins are getting around the current circular references runtime validation by using requiredBundles dependencies instead of requiredPlugins/optionalPlugins.

Circular dependencies in requiredBundles can also lead to unexpected behaviours like #123720.

We could add a new CI script that validates and warns about these circular dependencies #124860. However, we should discuss if it makes sense to add it to the existing runtime validation in Kibana:

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

Lines 281 to 332 in 237256a

	private getTopologicallySortedPluginNames() {
	// We clone plugins so we can remove handled nodes while we perform the
	// topological ordering. If the cloned graph is _not_ empty at the end, we
	// know we were not able to topologically order the graph. We exclude optional
	// dependencies that are not present in the plugins graph.
	const pluginsDependenciesGraph = new Map(
	[...this.plugins.entries()].map(([pluginName, plugin]) => {
	return [
	pluginName,
	new Set([
	...plugin.requiredPlugins,
	...plugin.optionalPlugins.filter((dependency) => this.plugins.has(dependency)),
	]),
	] as [PluginName, Set<PluginName>];
	})
	);
	// First, find a list of "start nodes" which have no outgoing edges. At least
	// one such node must exist in a non-empty acyclic graph.
	const pluginsWithAllDependenciesSorted = [...pluginsDependenciesGraph.keys()].filter(
	(pluginName) => pluginsDependenciesGraph.get(pluginName)!.size === 0
	);
	const sortedPluginNames = new Set<PluginName>();
	while (pluginsWithAllDependenciesSorted.length > 0) {
	const sortedPluginName = pluginsWithAllDependenciesSorted.pop()!;
	// We know this plugin has all its dependencies sorted, so we can remove it
	// and include into the final result.
	pluginsDependenciesGraph.delete(sortedPluginName);
	sortedPluginNames.add(sortedPluginName);
	// Go through the rest of the plugins and remove `sortedPluginName` from their
	// unsorted dependencies.
	for (const [pluginName, dependencies] of pluginsDependenciesGraph) {
	// If we managed delete `sortedPluginName` from dependencies let's check
	// whether it was the last one and we can mark plugin as sorted.
	if (dependencies.delete(sortedPluginName) && dependencies.size === 0) {
	pluginsWithAllDependenciesSorted.push(pluginName);
	}
	}
	}
	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}`
	);
	}
	return sortedPluginNames;
	}

IMO, if we want to avoid them, the sooner we error in the dev process, the better.

What do you think?

[elasticmachine]

Pinging @elastic/kibana-operations (Team:Operations)

[elasticmachine]

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

[mshustov]

Circular dependencies in requiredBundles can also lead to unexpected behaviours like

I believe it makes sense considering that Core already processes these data.

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

Line 264 in 7b9901e

requiredBundles: plugin.manifest.requiredBundles,

However, we need to improve the error message to show a cyclic graph, Topological ordering of plugins did not complete, these plugins have cyclic or missing dependencies: pluginA, pluginB, pluginC message is not developer-friendly.

[afharo]

However, we need to improve the error message to show a cyclic graph

I agree! The graphs should look more like: #124860

IMO, we'll need to create a separate function for this validation because getTopologicallySortedPluginNames is used to identify the order we need to initialize the plugins, and requiredBundles should not affect this.

[afharo]

Ideally, we should implement this issue so Kibana refuses to start. However, we depend on #126578 to be able to do so. I'm speaking with the @elastic/fleet team to get it sorted. If they don't have the bandwidth, we may want to print a warning for now.