[Platform] Run SavedObject Migrations after PluginService#setup

[rudolf]

In NP Plugins should be able to register SavedObject types, schemas and migrations during the setup lifecycle. This means we can only run SavedObject migrations at the end of Core's setup lifecycle after PluginService#setup has completed.

Currently in Core, migrations run before PluginService#setup which means all migrations have completed before we expose createScopedRepository, and createInternalRepository. Although these repositories aren't meant to be used directly, we need to ensure that if they were used to make SavedObject requests to Elasticsearch, that these requests would only fire once all migrations have completed.

[elasticmachine]

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

[pgayvallet]

Currently in Core, migrations run before PluginService#setup which means all migrations have completed before we expose createScopedRepository, and createInternalRepository. Although these repositories aren't meant to be used directly, we need to ensure that if they were used to make SavedObject requests to Elasticsearch, that these requests would only fire once all migrations have completed.

If I understand correctly, we will run the migration after plugin's setup, but we will 'delay' any SO request performed during plugin's setup until the migration is complete. With plugin lifecycles that still can be async, can't this leads to deadlocks if a plugin's setup method actually performs and wait for a call to SO in their setup?

[pgayvallet]

Currently in Core, migrations run before PluginService#setup

Hum, if I'm not misreading the code, the migration is actually performed during SavedObjectsService start method

kibana/src/core/server/saved_objects/saved_objects_service.ts

Line 290 in 56041f0

await this.migrator!.runMigrations(skipMigrations);

[joshdover]

Right it looks like we don't run the migrations until start, however we may need to do some refactoring to allow migrations to be added during setup. Right now, it looks like the SO service depends on all the migrations being known before SavedObjectsService#setup is ran.

It also appears that if getScopedClient, createInternalRepository, or createScopedRepository are used before start is ran, then the KibanaMigrator#migrateDocument function will be executed before all the migrations are registered.

Options:

• Do we have to expose these during setup? This might be a larger change and would also involve needing to move Elasticsearch APIs to start but that would make setup on the server-side more consistent with the client-side where setup is for registering things and start is for doing things.
• If not, can we delay resolving the methods that use migrations until after migrations have been completed?

[pgayvallet]

Do we have to expose these during setup?

I think that from what I understand, the answer is yes. @rudolf can you confirm?

[rudolf]

Yes, I confused discovering and registering migrations with running them. The issue is exactly what Josh describes here, we need to refactor saved objects to allow for adding migrations during setup.

getScopedClient is internal only, but the other methods are used by security and spaces.

kibana/src/core/server/saved_objects/saved_objects_service.ts

Lines 81 to 127 in 56041f0

	export interface SavedObjectsServiceSetup {
	/**
	* Set a default factory for creating Saved Objects clients. Only one client
	* factory can be set, subsequent calls to this method will fail.
	*/
	setClientFactory: (customClientFactory: SavedObjectsClientFactory<KibanaRequest>) => void;
	/**
	* Add a client wrapper with the given priority.
	*/
	addClientWrapper: (
	priority: number,
	id: string,
	factory: SavedObjectsClientWrapperFactory<KibanaRequest>
	) => void;
	/**
	* Creates a {@link ISavedObjectsRepository | Saved Objects repository} that
	* uses the credentials from the passed in request to authenticate with
	* Elasticsearch.
	*
	* @remarks
	* The repository should only be used for creating and registering a client
	* factory or client wrapper. Using the repository directly for interacting
	* with Saved Objects is an anti-pattern. Use the Saved Objects client from
	* the
	* {@link SavedObjectsServiceStart | SavedObjectsServiceStart#getScopedClient }
	* method or the {@link RequestHandlerContext | route handler context}
	* instead.
	*/
	createScopedRepository: (req: KibanaRequest, extraTypes?: string[]) => ISavedObjectsRepository;
	/**
	* Creates a {@link ISavedObjectsRepository | Saved Objects repository} that
	* uses the internal Kibana user for authenticating with Elasticsearch.
	*
	* @remarks
	* The repository should only be used for creating and registering a client
	* factory or client wrapper. Using the repository directly for interacting
	* with Saved Objects is an anti-pattern. Use the Saved Objects client from
	* the
	* {@link SavedObjectsServiceStart | SavedObjectsServiceStart#getScopedClient }
	* method or the {@link RequestHandlerContext | route handler context}
	* instead.
	*/
	createInternalRepository: (extraTypes?: string[]) => ISavedObjectsRepository;
	}

To really model the behaviour we would need three lifecycle stages to separate the following API calls:

1. Register migrations
2. Register security and spaces wrappers
3. Consume client API's

My initial idea was to put (1) & (2) into #setup and (3) in #start and then potentially doing something like blocking all Saved Object client API calls that might be run from #setup until a internal hasSetupCompleted promise resolves.

If (2) & (3) are placed in #start I don't think we would have a way to know that all Saved Object wrappers have successfully registered and that we can now start running client requests.

[pgayvallet]

So after discussing with @rudolf and @joshdover, it seems that the main (and only) reason why we are exposing the createInternalRepository and createScopedRepository methods to setup is because they are actually needed by plugins that registers wrappers or clientFactories

i.E security:

kibana/x-pack/plugins/security/server/saved_objects/index.ts

Lines 27 to 34 in e582277

	savedObjects.setClientFactory(({ request }) => {
	const kibanaRequest = getKibanaRequest(request);
	return new SavedObjectsClient(
	authz.mode.useRbacForRequest(kibanaRequest)
	? savedObjects.createInternalRepository()
	: savedObjects.createScopedRepository(kibanaRequest)
	);
	});

WDYT about changing the signatures of setClientFactory and addClientWrapper to provides the repository access

The previous snippet would become:

  savedObjects.setClientFactory(repoFactory => ({ request }) => {
    const kibanaRequest = getKibanaRequest(request);
    return new SavedObjectsClient(
      authz.mode.useRbacForRequest(kibanaRequest)
        ? repoFactory.createInternalRepository()
        : repoFactory.createScopedRepository(kibanaRequest)
    );
  });

That would allow to remove the createInternalRepository and createScopedRepository methods from SO service's setup, and only allow to perform calls from plugin's start method.

There would be no more pain point in performing SO migration between plugin's setup and start

I'm really not an expert in SO apis, so is there any blocker in this approach?

[pgayvallet]

Another option would be to have the SO repositories being aware of the current lifecycle state and either logs a warning or throws an error when calls are performed before the end of core's setup phase (aka during plugin's setup)

[pgayvallet]

Looking at

kibana/src/legacy/core_plugins/telemetry/server/collectors/management/telemetry_management_collector.ts

Lines 33 to 50 in ebd2c21

	export function createCollectorFetch(server: Server) {
	return async function fetchUsageStats(): Promise<UsageStats> {
	const internalRepo = server.newPlatform.setup.core.savedObjects.createInternalRepository();
	const uiSettingsClient = server.newPlatform.start.core.uiSettings.asScopedToClient(
	new SavedObjectsClient(internalRepo)
	);
	const user = await uiSettingsClient.getUserProvided();
	const modifiedEntries = Object.keys(user)
	.filter((key: string) => key !== 'buildNum')
	.reduce((obj: any, key: string) => {
	obj[key] = user[key].userValue;
	return obj;
	}, {});
	return modifiedEntries;
	};
	}

there is another need of accessing a SO repository during setup, to register usage collectors actually using SOs.

(this snippet will not be able to be migrated to NP atm, as is currently uses apis from core's both setup and start at the same time, but that's another issue, created #54886):

[rudolf]

That would allow to remove the createInternalRepository and createScopedRepository methods from SO service's setup, and only allow to perform calls from plugin's start method.
There would be no more pain point in performing SO migration between plugin's setup and start

I think this is a really elegant way to solve the problem.

We would also have to expose an internalRepository from the start lifecycle for other telemetry use cases like using the incrementCounter method:

kibana/src/plugins/usage_collection/server/report/store_report.ts

Line 41 in 3658220

return await internalRepository.incrementCounter('ui-metric', savedObjectId, 'count');

[pgayvallet]

We would also have to expose an internalRepository from the start lifecycle for other telemetry use cases like using the incrementCounter method

We will need to expose a repository from start and a solution for #54886 yes.

@rudolf Do you see any other usages for the repositories during setup that I could have missed ?

[pgayvallet]

It also appears that if getScopedClient, createInternalRepository, or createScopedRepository are used before start is ran, then the KibanaMigrator#migrateDocument function will be executed before all the migrations are registered.

That's not exactly true. the migrateDocument will only be executed if calls are actually performed from the clients/repositories created from these methods. Creating a repository on itself will not migrate anything.

There is another bigger issue though: the migrator is currently created during setup, needs 'final' version of the schema/mapping and migration to be instanciated, and is required to creates a repository. Meaning that if the remove it from setup, we can't technically instanciate any repository or client during setup.

There is a big chicken-egg condition here. Even changing the SavedObjectsRepository to have some lazy ref to the migrator is going to be hard, as there are some access done directly in the constructor...

[pgayvallet]

So, after looking up the code for a while, it seems that if we want migration to be performed after core's setup, we have no choice but to forbid any call to the SO clients or repository before the end of the core setup

I currently see three options for this:

• Totally removes any client/repository access from SavedObjectsService#setup API

This seems the cleanest solution, however I'm not really sure of the feasibility without introducing breaking changes to the actual usages of the API we are going to remove.

#52071 (comment) address the solution regarding setClientFactory and addClientWrapper, but there are lot of other part of the code that actually access a repository during setup. If #54886 is adopted it should resolve most of them, but I'm still not totally sure it will cover 100% of the usages.

• Let the plugin access/create clients and repositories, but block any call before the migration is actually complete

We could just add our own wrapper around the repositories and clients that would delay all calls before the migration is done (Which means before the end of core setup).It may appear pragmatic, however I'm really scared of the risk of introducing deadlocks by doing that, at least as long as plugin's setup API can be asynchronous.

Simplest example coming to mind would be a plugin awaiting a SO call during setup before returning. This would just deadlock the initialization.

As we would not be doing the migration before after the plugin's setup phase however, these deadlocks should be deterministic: a lock condition caused by a plugin awaiting such a call would always occurs, and could/should be detected in development. It we decide to go in this direction, we should therefor probably throw an error if a call is performed during setup, to let the developer fix his code.

• Register the migrations before the setup phase

I don't like the idea, but a simple solution which would likely less impact the code would be to allow plugins to either defines their migrations in a static way, as they currently do for their config schema, or to introduce a new 'lifecycle' method that would expose a restricted SO api to be able to register migrations, schema and mapping before the setup phase is run.

Overall, I'm leaning toward trying 1., as it's probably the best approach and is consistent with what the setup and start phases represents, but I'm not sure about the feasability.

[rudolf]

@rudolf Do you see any other usages for the repositories during setup that I could have missed ?

I'm not aware of any other use cases. Using the repository (or the client) in setup is an anti-pattern, so if we come across new use cases we should ideally find a better alternative. So I'm also leaning towards (1).

defines their migrations in a static way

The downside to static migrations is it limits us to static saved object types. This is a theoretical use case that we might never have to support but I can imagine something like the alerting plugin creating an alert type for every solution that integrates with it e.g. apm-alerts. If the alerting plugin ever wants to create a migration, it would have to add a migration for every solution specific type (.e.g. a apm alerts migration and a dashboard alerts migration), but it couldn't know statically which types would get registered.

We might never have to support this use case, so it doesn't justify adding a huge amount of complexity, but my gut feeling is that the additional complexity from dynamic migrations are small enough to justify getting the added flexibility.

[pgayvallet]

I'm currently working on the first proposal, moving the migration to an isolated method on the service to be called at the end of core'ssetup, and to move everything repository-related to the service start contract. But that's a lot of changes... will need some days even to get a (ci-failing) POC to see if we think this is the good direction

[pgayvallet]

POC of the 'moving the repository access from setup to start' (first option) here: #55012

After #55012 (comment), I think I will then try rudolf suggestion which is actually an equivalent to option 2 (Let the plugin access/create clients and repositories, but block any call before the migration is actually complete)