Run SO migration after plugins setup phase. by pgayvallet · Pull Request #55012 · elastic/kibana

pgayvallet · 2020-01-16T09:36:52Z

Summary

Fix #52071

PR does the following:

moves createScopedRepository and createInternalRepository from setup contract to start contract, and removes getScopedClient from the setup contract. There is no longer a possibility to be performing any call to SO before core's start is done
creates the migrator, the repository accessors and run the SO migration at the start of SavedObjectsService.start(), meaning that any SO call performed is assured to be done after the migration
change the setClientFactory API to setClientFactoryProvider to provides a SO repository provider when registering the client factory
adapt the existing plugin calls to the removed APIs from setup to be now using getStartServices to access them on the core start contract

Depends on #55156 (merged)

This is also a requirement for SO issues regarding registration of SO mappings / migrations / schemas / validations from NP (#50313, #50311, #50309)

Dev Docs

moves createScopedRepository and createInternalRepository from setup contract to start contract, and removes getScopedClient from the setup contract. There is no longer a possibility to be performing any call to SO before core's start is done
creates the migrator, the repository accessors and run the SO migration at the start of SavedObjectsService.start(), meaning that any SO call performed is assured to be done after the migration
change the setClientFactory API to setClientFactoryProvider to provides a SO repository provider when registering the client factory
adapt the existing plugin calls to the removed APIs from setup to be now using getStartServices to access them on the core start contract

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Devs Docs

core savedObjects APIs are no longer exposing methods that allows to perform SO calls from it's setup contract. createScopedRepository, createInternalRepository and getScopedClient are now only accessible from the service start contract.

When registering asynchronous handlers during a plugin setup phase that relies on these APIs, getStartServices should be used:

before

class MyPlugin implements Plugin {
  setup(core: CoreSetup, plugins: PluginDeps) {
    plugins.usageCollection.registerCollector({
      type: 'MY_TYPE',
      fetch: async () => {
        const internalRepo = core.savedObjects.createInternalRepository();
        // ...
      },
    });
  }
  start() {}
}

after

class MyPlugin implements Plugin {
  setup(core: CoreSetup, plugins: PluginDeps) {
    plugins.usageCollection.registerCollector({
      type: 'MY_TYPE',
      fetch: async () => {
        const [{ savedObjects }] = await core.getStartServices();
        const internalRepo = savedObjects.createInternalRepository();
        // ...
      },
    });
  }
  start() {}
}

pgayvallet

My thoughts on this first draft.

Finishing the PR implies heavy changes on plugins code, and also now got #54886 as a requirement. I would like to be sure that this is the direction we want to take before continuing this work.

I also got the feeling that is we exposes everything SO-client related in start, we should probably do the same for elasticsearch and uisettings

src/core/server/saved_objects/saved_objects_service.ts

pgayvallet · 2020-01-16T14:17:59Z

src/core/server/saved_objects/saved_objects_service.ts

+    const clientProvider = new SavedObjectsClientProvider<KibanaRequest>({
+      defaultClientFactory({ request }) {
+        const repository = repositoryFactory.createScopedRepository(request);
+        return new SavedObjectsClient(repository);
+      },
+    });
+    if (this.clientFactoryProvider) {
+      const clientFactory = this.clientFactoryProvider(repositoryFactory);
+      clientProvider.setClientFactory(clientFactory);
+    }
+    this.clientFactoryWrappers.forEach(({ id, factory, priority }) => {
+      clientProvider.addClientWrapperFactory(priority, id, factory);
+    });


Now that the clientProvider is created after everything has been registered to the service, we should probably use a simple constructor instead,

const clientProvider = new SavedObjectsClientProvider<KibanaRequest>({ clientFactory, clientWrappers, })

I tried to do it, but TIL is that the clientProvider is exposed in the internal API to be directly used from the legacy SO service in src/legacy/server/saved_objects/saved_objects_mixin.js, so changing the class is not trivial. This should wait until legacy has been removed.

src/core/server/server.ts

elasticmachine · 2020-01-16T14:23:23Z

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

pgayvallet · 2020-01-16T14:23:57Z

@elastic/kibana-platform I would like you opinion on this because going further into the development.

rudolf · 2020-01-16T15:55:01Z

This is a pain point. As of PR state, server#registerCoreContext now requires API from both core setup and start. Not sure what is the best way to address that.

I'm wondering if we could resolve this if we keep creating the repository in setup and keep exposing getScopedClient as part of InternalSavedObjectsSetup, but make mappings, migrations and schemas observables? So we construct the repository (and KibanaMigrator) in setup, but they keep accepting new changes until the end of the setup lifecycle.

There's quite a bit of complexity in how all the SO parts get stitched together, so we would probably have to create a new POC to test this out and make sure it doesn't lead to some sort of dead end.

pgayvallet · 2020-01-16T16:25:38Z

I'm wondering if we could resolve this if we keep creating the repository in setup and keep exposing getScopedClient as part of InternalSavedObjectsSetup, but make mappings, migrations and schemas observables? So we construct the repository (and KibanaMigrator) in setup, but they keep accepting new changes until the end of the setup lifecycle.

I wanted to try that, however:

that's some big changes on a lot of underneath SO classes, as it was really not conceived with this idea in mind.
even with observables, we would still be instantiating a repository with a not-yet-configured migrator, so there will always be technically the risk that at some point a call could be performed to SO before the migrator have received proper/final configuration

If the only thing pushing in that direction is this single own core usage for registerCoreContext, my feeling is that may not be worth it, and that a - way less elegant but yet working - trick with a closure to use a reference to the start contract that would be stored to the server instance during start is probably more pragmatic.

But the discussion is still widely open. I really don't have a clear vision of what the best thing to do is...

joshdover

This looks like the right direction to me.

I agree that uiSettings and Elasticsearch maybe shouldn't be exposed before migrations are ran. uiSettings I'm very sure shouldn't be. However, Elasticsearch might be safe. Many plugins set up or query other non-SO indices. However, I'm not sure there's any advantage to allowing these during setup rather than start.

The strict separation between setup and start has been very helpful on the client. Keeping registration logic and setup and "running" logic in start seems like the way to go, though I do fear the number of plugins that this will affect.

src/core/server/server.ts

joshdover · 2020-01-16T19:56:24Z

src/core/server/saved_objects/service/lib/scoped_client_provider.ts

+ *
+ * @public
+ */
+export type SavedObjectsClientFactoryProvider<Request = unknown> = (


oh no, a factory provider ☕️

seen on a real previous project: RPCClientFactoryBridgedProviderCachedRegistry. We still got some margin.

Haha, in legacy we have Scoped- too so we could create an impressive ScopedClientFactoryWrapperProvider

kibana/src/legacy/server/saved_objects/saved_objects_mixin.js

Lines 139 to 140 in ca55402

setScopedSavedObjectsClientFactory: (...args) => provider.setClientFactory(...args),

addScopedSavedObjectsClientWrapperFactory: (...args) =>

AFAIK all usages of setScopedSavedObjectsClientFactory has been migrated to be using NP API from legacy instead (which is not the case of setScopedSavedObjectsClientFactory ). I think I might removes it in this PR. will do after a first green CI though

src/core/server/server.ts

pgayvallet · 2020-01-16T21:22:05Z

The strict separation between setup and start has been very helpful on the client. Keeping registration logic and setup and "running" logic in start seems like the way to go, though I do fear the number of plugins that this will affect.

That's my feeling too. The getStartServices should probably mitigate this a lot however, as these API are already async, it should be trivial to add another async call to core.getStartService() to retrieve the moved API. I just 'hope' plugin teams did not add massive coverage over their migrated code, as tests would probably the hardest to adapt.

I guess we need to finish this attempt on SO to see if it becomes clear that it should be done also on UI (and maybe ES)

rudolf · 2020-01-17T10:12:15Z

If the only thing pushing in that direction is this single own core usage for registerCoreContext, my feeling is that may not be worth it

Looking at this again I agree. Registering mappings, schemas and migrations aren't fundamentally things we would ever want to be reactive. Modelling these as observables will probably reduce the clarity of the code.

pgayvallet · 2020-01-17T16:48:57Z

core changes are mostly done. Tests has been updated and added.

Now waiting for #55156 to be integrated to be able to properly updates plugin's calls of the moved APIs

…ation-after-setup

pgayvallet · 2020-01-20T10:37:58Z

#55156 has been merged on master and integrated in the PR branch. Should now be able to continue with the PR.

src/core/server/server.ts

mshustov · 2020-01-21T12:11:38Z

However, Elasticsearch might be safe. Many plugins set up or query other non-SO indices. However, I'm not sure there's any advantage to allowing these during setup rather than start.

👍 to restrict access to elasticsearch API or provide customCluster only (can be done by request)

pgayvallet · 2020-01-21T14:19:50Z

Created #55396 for UiSettings and #55397 to discuss about ES API

kobelb

Security changes LGTM

…quest instead

…ation-after-setup

pgayvallet · 2020-01-22T08:48:38Z

x-pack/legacy/plugins/alerting/server/plugin.ts

+      getServices(rawRequest: Hapi.Request): Services {
+        const request = KibanaRequest.from(rawRequest);
        return {
-          callCluster: (...args) =>
-            adminClient!.asScoped(KibanaRequest.from(request)).callAsCurrentUser(...args),
-          savedObjectsClient: core.savedObjects.getScopedSavedObjectsClient(request),
+          callCluster: (...args) => adminClient!.asScoped(request).callAsCurrentUser(...args),
+          // rawRequest is actually a fake request, converting it to KibanaRequest causes issue in SO access
+          savedObjectsClient: core.savedObjects.getScopedSavedObjectsClient(rawRequest as any),


I did properly adapt this to use core.savedObjects.getScopedSavedObjectsClient(request). However I got test failures, and I isolated it to this change.

Calling core.savedObjects.getScopedSavedObjectsClient with KibanaRequest.from(request) instead of the raw HAPI request makes the test fail, with
Saved object [alert/dbbfe2a1-e3ab-4ebe-84f2-9ed495b9d81b] not found

Forcecasting the raw request fix the failure:

savedObjectsClient: core.savedObjects.getScopedSavedObjectsClient(rawRequest as any)

AFAIK it's because the rawRequest, even if typed as Hapi.Request, is actually a fake request:

kibana/x-pack/legacy/plugins/alerting/server/task_runner/task_runner.ts

Lines 67 to 82 in ea9a7b8

const fakeRequest = {

headers: requestHeaders,

getBasePath: () => this.context.getBasePath(spaceId),

path: '/',

route: { settings: {} },

url: {

href: '/',

},

raw: {

req: {

url: '/',

},

},

};

return this.context.getServices(fakeRequest);

and the GetServices signature differs from between the type and the actual implementation, allowing to call it without a proper hapi request...

kibana/x-pack/legacy/plugins/alerting/server/types.ts

Line 15 in 5853360

export type GetServicesFunction = (request: any) => Services;

Not sure what the underlying cause is though. I though only the headers from the request were actually used when using the request to create the scoped repository. But the fake request does miss some properties used when converting from request to kibanaRequest such as method.

Is this comment acceptable/enough for now, or should I investigate in this PR?

This is related to #39430 btw

Is this comment acceptable/enough for now, or should I investigate in this PR?

As for me, it's not a blocker for the current PR. But we should prioritize #39430 to be able to cut off legacy server-side plugins in 7.7

mshustov · 2020-01-22T14:45:28Z

src/plugins/home/server/services/sample_data/usage/usage.ts

    addInstall: async (dataSet: string) => {
      try {
-        await internalRepository.incrementCounter(SAVED_OBJECT_ID, dataSet, `installCount`);
+        await (await internalRepository).incrementCounter(SAVED_OBJECT_ID, dataSet, `installCount`);


optional: I'd expect something similar to https://github.com/elastic/kibana/pull/55012/files#diff-b7a00ca0a0cbdb5c3792d6a7dbfd6811R67-R74

Missed this one when cleanup up the calls, thanks

joshdover · 2020-01-22T17:59:43Z

src/core/server/saved_objects/saved_objects_service.ts

   * factory can be set, subsequent calls to this method will fail.
   */
-  setClientFactory: (customClientFactory: SavedObjectsClientFactory<KibanaRequest>) => void;
+  setClientFactoryProvider: (clientFactoryProvider: SavedObjectsClientFactoryProvider) => void;


nit: can we update the comment above to make it clear that this sets a default factory provider. Also it'd be nice if we used {@link SavedObjectsClientFactoryProvider | factory provider}

…ation-after-setup

* change setClientFactory api to setClientFactoryProvider * cleanup and add test for service * change the signatures of SO start/setup * fix registerCoreContext by accessing stored start contract reference * move migration inside `start` * adapt and add service tests * add doc and export new types * adapt plugins code * update generated doc * better core access * address some review comments * remove parametrized type from SavedObjectsClientFactory, use KibanaRequest instead * add logs when starting and ending so migration * fix KibanaRequest imports * NITs and review comments * fix alerting FTR test * review comments

kibanamachine · 2020-04-10T16:58:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: f7a4051

History

💚 Build #22057 succeeded f7a4051
💚 Build #21814 succeeded 8c213a0
💔 Build #21708 failed 51c0ec0
💔 Build #21655 failed e20a6de
💚 Build #21488 succeeded fed6d58

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

pgayvallet added 3 commits January 16, 2020 09:46

change setClientFactory api to setClientFactoryProvider

5419124

cleanup and add test for service

3a45728

change the signatures of SO start/setup

3b6a092

pgayvallet commented Jan 16, 2020

View reviewed changes

pgayvallet changed the title ~~Run SO migration after plugins setup phase.~~ POC: run SO migration after plugins setup phase. Jan 16, 2020

pgayvallet added Feature:New Platform Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels Jan 16, 2020

pgayvallet mentioned this pull request Jan 16, 2020

[Platform] Run SavedObject Migrations after PluginService#setup #52071

Closed

joshdover reviewed Jan 16, 2020

View reviewed changes

pgayvallet added 4 commits January 17, 2020 14:03

fix registerCoreContext by accessing stored start contract reference

a5e0b12

move migration inside start

8e2394c

adapt and add service tests

2ebe9dd

add doc and export new types

14d36c6

pgayvallet changed the title ~~POC: run SO migration after plugins setup phase.~~ Run SO migration after plugins setup phase. Jan 17, 2020

pgayvallet added the v7.7.0 label Jan 17, 2020

Merge remote-tracking branch 'upstream/master' into kbn-52071-so-migr…

7860fc5

…ation-after-setup

pgayvallet added 2 commits January 20, 2020 14:52

adapt plugins code

098b522

update generated doc

a8070c9

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 and removed v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 21, 2020

mshustov reviewed Jan 21, 2020

View reviewed changes

src/core/server/server.ts Show resolved Hide resolved

This was referenced Jan 21, 2020

Remove UISettings accessors from core setup contract #55396

Closed

Move elasticsearch client creation/access API from core setup to start contract #55397

Closed

address some review comments

798deae

kobelb approved these changes Jan 21, 2020

View reviewed changes

pgayvallet added 7 commits January 21, 2020 18:41

remove parametrized type from SavedObjectsClientFactory, use KibanaRe…

511c4fa

…quest instead

add logs when starting and ending so migration

ceec182

Merge remote-tracking branch 'upstream/master' into kbn-52071-so-migr…

e20a6de

…ation-after-setup

fix KibanaRequest imports

51c0ec0

Merge remote-tracking branch 'upstream/master' into kbn-52071-so-migr…

d4b16db

…ation-after-setup

NITs and review comments

961fec1

fix alerting FTR test

8c213a0

pgayvallet commented Jan 22, 2020

View reviewed changes

pgayvallet requested review from joshdover and mshustov January 22, 2020 14:02

mshustov reviewed Jan 22, 2020

View reviewed changes

mshustov approved these changes Jan 22, 2020

View reviewed changes

joshdover approved these changes Jan 22, 2020

View reviewed changes

pgayvallet added 2 commits January 23, 2020 08:47

review comments

ba749ba

Merge remote-tracking branch 'upstream/master' into kbn-52071-so-migr…

f7a4051

…ation-after-setup

pgayvallet mentioned this pull request Jan 23, 2020

[Platform] SavedObject Mappings #50309

Closed

pgayvallet merged commit a75436d into elastic:master Jan 23, 2020

pgayvallet mentioned this pull request Jan 23, 2020

[7.x] Run SO migration after plugins setup phase. (#55012) #55664

Merged

tylersmalley mentioned this pull request Jan 31, 2020

Perform successful Elasticsearch version check before migrations #51311

Merged

7 tasks

joshdover mentioned this pull request Feb 19, 2020

[skip-ci] Fix broken links to saved objects APIs in MIGRATION.md #58033

Merged

	setScopedSavedObjectsClientFactory: (...args) => provider.setClientFactory(...args),
	addScopedSavedObjectsClientWrapperFactory: (...args) =>

	const fakeRequest = {
	headers: requestHeaders,
	getBasePath: () => this.context.getBasePath(spaceId),
	path: '/',
	route: { settings: {} },
	url: {
	href: '/',
	},
	raw: {
	req: {
	url: '/',
	},
	},
	};

	return this.context.getServices(fakeRequest);

Conversation

pgayvallet commented Jan 16, 2020 • edited by jinmu03 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dev Docs

Checklist

For maintainers

Devs Docs

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticmachine commented Jan 16, 2020

Uh oh!

pgayvallet commented Jan 16, 2020

Uh oh!

rudolf commented Jan 16, 2020

Uh oh!

pgayvallet commented Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgayvallet commented Jan 16, 2020

Uh oh!

rudolf commented Jan 17, 2020

Uh oh!

pgayvallet commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgayvallet commented Jan 20, 2020

Uh oh!

Uh oh!

mshustov commented Jan 21, 2020

Uh oh!

pgayvallet commented Jan 21, 2020

Uh oh!

kobelb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 10, 2020

💔 Build Failed

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

pgayvallet commented Jan 16, 2020 •

edited by jinmu03

Loading

pgayvallet commented Jan 16, 2020 •

edited

Loading

pgayvallet commented Jan 17, 2020 •

edited

Loading