Remove circular dependencies between CLI & core

[mshustov]

CLI relies on core types & utils, core uses cli in runtime.
discussion #46773 (comment)
related work: #76003

The circular dependency exists due to CliDevMode references from the Kibana platform's legacy service. CliDevMode is used for (from #78710):

• start Kibana server in a child process
• watch for changes to the source code for the server and restart the Kibana server process on any changes
• start @kbn/optimizer in the same process, which launches webpack workers in child processes
• observe the status of the optimizer and kibana server, proxy HTTP/HTTPS requests to the Kibana server as long as the server is running and the optimizer isn't currently building assets.

That's no Kibana platform responsibility. Due to that, the platform code contains a lot of code checking whether it's run in dev cluster mode isDevClusterMaster to disable some functionality: SO migration, plugin discovery, plugin initialization, etc. (see #79037). Which makes the platform code error-prone and hard to maintain.

kibana/src/core/server/legacy/legacy_service.ts

Lines 183 to 189 in 4584a8b

	// eslint-disable-next-line @typescript-eslint/no-var-requires
	const { CliDevMode } = require('./cli_dev_mode');
	CliDevMode.fromCoreServices(
	this.coreContext.env.cliArgs,
	config,
	await basePathProxy$.toPromise()
	);

kibana/src/core/server/legacy/cli_dev_mode.js

Line 9 in 4584a8b

export { CliDevMode } from '../../../dev/cli_dev_mode';

[elasticmachine]

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

[elasticmachine]

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

[pgayvallet]

I took an initial look at the exact dependencies coming from DevCliMode to core and how we could address this cyclic dependency issue.

What needs to be done

As explained in the issue's description, in dev mode, we are booting a pseudo Kibana server with most features disabled just to be able to boot the legacy service which will instantiate the cliDevMode with a subset of core configuration and services. CliDevMode will then kickstart the optimizer, the basePath server and the 'real' Kibana server (in another process).

This is just bad design, driven by the fact that CliDevMode is not able to manually read the configuration and have to rely on core to do it. That adds a lot of if !isDevCliParent checks in core code, and it just hard to maintain. In addition, this makes the clustering mode core even complicated (#94057), and is going to be a blocker for Bazel because of the cyclic dependency.

What would need to be done instead is to remove the core->devCliMode dependency. The dev mode should not need an instance of a Kibana server to kickstart itself.

The basic idea is that, when DEV_MODE_SUPPORTED is true, the serve script (src/cli/serve/serve.js) would bootstrap DevCliMode directly (via its own bootstrap script or similar), instead of starting core.

Now, this is easier said than done.

State of the mess

1. The basePathProxy

Currently the basePath proxy server is created from core, and passed down to the CliDevMode instance. Note that BasePathProxyServer is exclusively used in development, and should be moved out of core to land in the src/dev/cli_dev_mode module (or at least in its own module under src/dev).

Now if we take a look at the dependencies from core this class is currently using:

kibana/src/core/server/legacy/legacy_service.ts

Lines 173 to 181 in 4584a8b

	const basePathProxy$ = this.coreContext.env.cliArgs.basePath
	? combineLatest([this.devConfig$, this.httpConfig$]).pipe(
	first(),
	map(
	([dev, http]) =>
	new BasePathProxyServer(this.coreContext.logger.get('server'), http, dev)
	)
	)
	: EMPTY;

There are three things here:

• The dev config
• The http config
• An instance of core's Logger

Solving the logger dependency should be quite easy. It doesn't make any sense to have the dev proxy server use the Kibana instance's logging configuration anyway, so we should adapt the proxy server to use the console logger implementation used by other parts of the cli dev mode when we'll extract it out of core:

kibana/src/dev/cli_dev_mode/log.ts

Line 20 in 4584a8b

export class CliLog implements Log {

As an improvement, we could also have the cli logger implements the Logger interface, now that this is exposed from the @kbn/logging package. Not sure this is really necessary for a logger only meant to be used in development and to only output to the console though.

Regarding the dependencies to core's configuration schemas, we'll see that in the next section

2. The CliDevMode instance

kibana/src/core/server/legacy/legacy_service.ts

Lines 184 to 189 in 4584a8b

	const { CliDevMode } = require('./cli_dev_mode');
	CliDevMode.fromCoreServices(
	this.coreContext.env.cliArgs,
	config,
	await basePathProxy$.toPromise()
	);

The three parameters are:

• The cliArgs from core's Env

These cliArgs are currently passed directly from the serve script to core's bootstrap, so having the serve script pass them down to the yet-to-be CliDevMode bootstrap script directly should not be a problem.

• The legacy configuration

The legacy configuration is only used to access two properties: plugins.path and plugins.scanDirs

kibana/src/dev/cli_dev_mode/cli_dev_mode.ts

Lines 83 to 84 in 4584a8b

	pluginPaths: config.get<string[]>('plugins.paths'),
	pluginScanDirs: config.get<string[]>('plugins.scanDirs'),

The properties are, in new platform terms, respectively PluginsConfig.additionalPluginPaths and PluginsConfig.pluginSearchPaths. so the real dependency is against core's plugins configuration.

• The basePath proxy

See previous section, currently passed from core to cliDevMode, will be instantiated directly by CliDevMode once we extract it.

Our options

So, from last section, we can see that the only thing currently really blocking us from just extracting the instantiation of DevCliMode from core's legacy service to the serve.js script is the required access to core's configuration.

We will need to find a way to have CliDevMode be able to read and parse the configuration itself, instead of relying on core to do it and then pass the parsed configuration as an argument.

It would allow us to break the cyclic dependency by cutting the Core -> CliDevMode dep.

Before

After

Now, how do we do that?

We recently extracted most of core's config service code into @kbn/config (#76874). However, only the service's interface and implementation got extracted. Core's config schemas, deprecations and associated types/classes are still in core.

kibana/src/core/server/server.ts

Lines 318 to 329 in 25e586a

	statusConfig,
	pidConfig,
	i18nConfig,
	];
	this.configService.addDeprecationProvider(rootConfigPath, coreDeprecationProvider);
	for (const descriptor of configDescriptors) {
	if (descriptor.deprecations) {
	this.configService.addDeprecationProvider(descriptor.path, descriptor.deprecations);
	}
	this.configService.setSchema(descriptor.path, descriptor.schema);
	}

At the time, we chose not to move core's config schemas to @kbn/config because the associated types are way less isolated than the config service implementation. Moving the schemas and the associated types and classes (e.g the HttpConfig class) represents some significant refactoring, and would also split core services code between the core module and some Kibana packages, which is not really desirable and would ideally be avoided. Note that another issue with extracting more core classes from the core module is that api-extractor does not generate documentation for external types, meaning that we will be loosing generated documentation for all public types we move to the package (may be a deprecated problem though, now that we're planning on @kbn/docs-utils as a replacement)

I think I only see two options here:

1. Finish core's config extraction, and move the schemas and associated types to a package

We could move that to @kbn/config or create a new @kbn/core-config-schemas package. Not sure which one is preferable. I did not yet look at all we would need to move if we go that way, but I'm expecting it to be quite significant. That way, DevCliMode could register the necessary schemas and then load and access the configuration without having to rely on core.

2. Have the DevCliMode import the schemas and types from core

With the new dependency graph, preserving the CliDevMode -> Core dependency is not an issue, as we removed the inverse dependency and as the cli module would depends on core anyway. So we could just have the src/dev/cli_dev_mode import the schemas and types from core without moving them to a package.

This option probably represents less work, but seems to be a worse design. Note that, with Bazel, we will no longer be able to use deep imports from dependencies (not 100% sure, can someone confirm that), so we will have to publicly expose some of core's config schema and internal types from the server entrypoint.

With our ideal end of 7.13 target to address this cyclic dependency, I'm mixed between both options to be honest. 1. seems like the proper design, but is a significant amount of work for a refactoring only required for dev/infra purposes. 2. is probably faster to implement, but feels more like a workaround or a temporary solution than a definitive implementation.

@rest-ohwait @mshustov @joshdover wdyt?

[joshdover]

I think the option we choose likely depends on the long-term plans of what we're going to do with the BasePathProxy. Personally, I'd like to get away from using this JS proxy and would really prefer we use something realistic to what our users are using (like nginx or haproxy). In either of those cases, I think it's like we use Bazel to start and wire up that frontend with the Kibana backend in development.

Given that, is it necessary that we configure these components exactly the same way? Could we get away with not having to pass the HttpConfig into CliDevMode?

We may also be able to remove the dependency on the plugin path configs. Currently, these are only needed for two things 1) knowing with directories to watch for restarting the server; and 2) running the optimizer. I think 1) could be removed by only supporting this feature for src, x-pack, examples and plugins. I think 2) will be removed once the web pack builds are built by Bazel.

I ask because I think (?) option (1) has little benefit other than this specific case and I'd really like to avoid doing that high-effort, low-value work if possible.

[pgayvallet]

Could we get away with not having to pass the HttpConfig into CliDevMode?

I doubt so. This is the config with the most internal logic, e.g for ssl

kibana/src/core/server/http/http_config.ts

Line 211 in 4584a8b

this.ssl = new SslConfig(rawHttpConfig.ssl || {});

and the BasePathProxyServer is relying on that

kibana/src/core/server/http/base_path_proxy_server.ts

Lines 76 to 85 in 4584a8b

	if (this.httpConfig.ssl.enabled) {
	const tlsOptions = serverOptions.tls as TlsOptions;
	this.httpsAgent = new HttpsAgent({
	ca: tlsOptions.ca,
	cert: tlsOptions.cert,
	key: tlsOptions.key,
	passphrase: tlsOptions.passphrase,
	rejectUnauthorized: false,
	});
	}

[spalger]

The basic idea is that, when DEV_MODE_SUPPORTED is true, the serve script (src/cli/serve/serve.js) would bootstrap DevCliMode directly (via its own bootstrap script or similar), instead of starting core.

@pgayvallet I think I would prefer that instead of calling src/cli/serve in order to start the dev CLI we would instead call the dev CLI directly from the script, and it would in turn start the src/cli in the child process as the mechanism for starting src/core this way we don't need to have any dev-specific logic in the production CLI and can just pass through all flags from the dev CLI to src/cli/serve. Doing this will also allow us to log deprecation messages and things in the child process rather than the parent process addressing...

1. Finish core's config extraction, and move the schemas and associated types to a package
   ...
2. Have the DevCliMode import the schemas and types from core

@pgayvallet I'd prefer a third option where the dev CLI parses the config file and CLI args independently, only extracting the little information that it needs, and then relies on the child Kibana process to handle full config validation, deprecation warnings, etc. In the parent process we should need a very small subset of the standard config and it seems fairly trivial to maintain an isolated implementation of the config parsing logic that can be moved to a package with the rest of the cli_dev_mode code.

I'd like to get away from using this JS proxy and would really prefer we use something realistic to what our users are using (like nginx or haproxy).

@joshdover I understand the idea here, but the BasePathProxy has taken on additional roles since it was created and named and is now responsible for ensuring that browser refreshes get the latest assets/don't fail because of server restarts by integrating with the child processes and pausing requests when the optimizer is running or the server is restarting. Removing this functionality by switching to a more production-like proxy feels like it will only hurt dev experience and I don't think that is a good idea.

[pgayvallet]

@spalger

I think I would prefer that instead of calling src/cli/serve in order to start the dev CLI we would instead call the dev CLI directly from the script

By script, you mean src/cli/dev.js, right?

That would require some refactoring in src/cli/serve/serve.js to extract applyConfigOverrides, as I think we also need the dev cli config parsing to be aware of overrides. But I agree that it seems cleaner, as we would be able to get rid of DEV_MODE_SUPPORTED in src/cli/serve/serve.js, which will have the exact same behavior in dev and production mode.

I'd prefer a third option where the dev CLI parses the config file and CLI args independently, only extracting the little information that it needs

I'd really like that too tbh, and it's almost possible as the config service itself is in a package. The main thing kinda blocking that option is the fact that the basePath proxy relies on the HttpConfig class, as the class is directly used to invoke getServerOptions and getListenerOptions. The hardest part is the ssl config logic, contained into SslConfig, that is used in getServerOptions:

kibana/src/core/server/http/http_tools.ts

Lines 72 to 80 in 4584a8b

	if (configureTLS && config.ssl.enabled) {
	const ssl = config.ssl;
	// TODO: Hapi types have a typo in `tls` property type definition: `https.RequestOptions` is used instead of
	// `https.ServerOptions`, and `honorCipherOrder` isn't presented in `https.RequestOptions`.
	const tlsOptions: TLSOptions = {
	ca: ssl.certificateAuthorities,
	cert: ssl.certificate,
	ciphers: config.ssl.cipherSuites.join(':'),

Now, I'm not sure if we really have / want to support SSL for the basePath server.

As we're gonna need to extract getServerOptions and getListenerOptions to a package anyway (everything that is currently in src/core/server/http/http_tools.ts AFAIK), we could also extract the ssl parsing logic into that package and import/use it from both SslConfig and the new devMode 'config parser'

it seems fairly trivial to maintain an isolated implementation of the config parsing logic

Not that trivial if we want to preserve the SSL capabilities of the dev mode.

[pgayvallet]

@spalger I opened a PR (#95145), starting by extracting the http stuff that now needs to be shared between core and cli_dev_mode.

I'm now looking at how to bootstrap cli_dev_mode directly from src/cli/dev.js. I was going to extract applyConfigOverrides from src/cli/serve/serve.js, but it will not be sufficient. ATM all the options, including the dev only options, are registered on the serve command.

kibana/src/cli/serve/serve.js

Lines 184 to 197 in 28942df

	if (DEV_MODE_SUPPORTED) {
	command
	.option('--dev', 'Run the server with development mode defaults')
	.option('--ssl', 'Run the dev server using HTTPS')
	.option('--dist', 'Use production assets from kbn/optimizer')
	.option(
	'--no-base-path',
	"Don't put a proxy in front of the dev server, which adds a random basePath"
	)
	.option('--no-watch', 'Prevents automatic restarts of the server in --dev mode')
	.option('--no-optimizer', 'Disable the kbn/optimizer completely')
	.option('--no-cache', 'Disable the kbn/optimizer cache')
	.option('--no-dev-config', 'Prevents loading the kibana.dev.yml file in --dev mode');
	}

Also, the construction of what we call CliArgs is also done in the serve command and depends on the registered options (and of course, convert/rename them). If we want to isolate the 2 scripts, we may need to duplicate or extract quite a lot of things (like 95% of the content of src/cli/serve/serve.js: applyConfigOverrides and most of the option registration and conversion to cliArgs)

I think uncoupling the two is going to require some work. I'm not even sure what the correct approach would be. Should I create a new command for dev mode and use it as default when DEV_MODE_SUPPORTED instead of serve here?

kibana/src/cli/cli.js

Lines 43 to 49 in 4584a8b

	if (!subCommand) {
	if (_.intersection(argv.slice(2), ['-h', '--help']).length) {
	program.defaultHelp();
	} else {
	argv.splice(2, 0, ['serve']);
	}
	}

Or do you have another idea in mind?

[pgayvallet]

During a sync discussion with @spalger yesterday, we decided to keep the src/cli/serve as the single entry point for both dev and production mode for now.