Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Plugins to access elasticsearch from CoreStart #59915

Open
wants to merge 26 commits into
base: master
from

Conversation

@rudolf
Copy link
Contributor

rudolf commented Mar 11, 2020

Summary

Refactors some plugins to use the elasticsearch client from start instead of setup after it was deprecated it #59886. I'm not familiar with the functionality of the plugins and haven't tested any of the changes, please test your plugin before giving a 馃憤

The first commit is from #59886 and should be ignored. I don't want the Core API changes to block on the refactoring work.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@rudolf rudolf requested review from elastic/kibana-alerting-services as code owners Mar 11, 2020
@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Mar 11, 2020

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

@rudolf rudolf added this to Pending review in kibana-platform via automation Mar 11, 2020
@rudolf

This comment has been minimized.

Copy link
Contributor Author

rudolf commented Mar 11, 2020

Having Elasticsearch only available in setup causes a lot ugly ways to access Elasticsearch 馃槥 some of these could be improved by refactoring more code "downstream", but there are some plugins which I don't think can be improved like actions. Actions would have to expose it's client through an async getter which would make the dev experience worse for all plugins depending on the actions client.

Copy link
Contributor

lizozom left a comment

Code owner changes are in a single file.

Side note:
Love the new way to access elasticsearch, though not crazy about the fact we use [0] to access core on the start services (core.getStartServices())[0].elasticsearch.client).

@rudolf rudolf force-pushed the rudolf:refactor-elasticsearch-from-start branch from 2260086 to f0bff3e Mar 12, 2020
http.registerRouteHandlerContext('watcher', (ctx, request) => {
return {
client: watcherESClient.asScoped(request),
client: {

This comment has been minimized.

Copy link
@sebelga

sebelga Mar 17, 2020

Contributor

This looks more like a hack than a solution to me. Snapshot and Restore app has a very similar logic than watcher to create the ES client. Should we move all the logic of the client creation + registering routes directly in the start() method?

This comment has been minimized.

Copy link
@rudolf

rudolf Mar 17, 2020

Author Contributor

Yeah I'm not very happy with how this looks 馃槥

We want all routes to be registered before we start the HTTP server and that requires that it's done during setup(). The alternative would be to turn client into a promise returning function. This makes it slightly less convenient to use, but since route handlers are already async functions adding another await might not make much difference.

What do you think of (I only refactored one route to illustrate):

diff --git a/x-pack/plugins/watcher/server/plugin.ts b/x-pack/plugins/watcher/server/plugin.ts
index 88cecf5335..75f727b738 100644
--- a/x-pack/plugins/watcher/server/plugin.ts
+++ b/x-pack/plugins/watcher/server/plugin.ts
@@ -17,6 +17,7 @@ import {
   Plugin,
   PluginInitializerContext,
 } from 'kibana/server';
+import { once } from 'lodash';
 import { PLUGIN } from '../common/constants';
 import { Dependencies, LicenseStatus, RouteDependencies } from './types';
 import { LICENSE_CHECK_STATE } from '../../licensing/server';
@@ -31,7 +32,7 @@ import { registerLoadHistoryRoute } from './routes/api/register_load_history_rou
 import { elasticsearchJsPlugin } from './lib/elasticsearch_js_plugin';
 
 export interface WatcherContext {
-  client: IScopedClusterClient;
+  getClient: () => Promise<IScopedClusterClient>;
 }
 
 export class WatcherServerPlugin implements Plugin<void, void, any, any> {
@@ -52,19 +53,15 @@ export class WatcherServerPlugin implements Plugin<void, void, any, any> {
       getLicenseStatus: () => this.licenseStatus,
     };
 
-    const getWatcherEsClient = async () => {
+    const getWatcherEsClient = once(async () => {
       const [coreStart] = await getStartServices();
       const config = { plugins: [elasticsearchJsPlugin] };
       return coreStart.elasticsearch.legacy.createClient('watcher', config);
-    };
+    });
+
     http.registerRouteHandlerContext('watcher', (ctx, request) => {
       return {
-        client: {
-          callAsCurrentUser: async (...args) =>
-            (await getWatcherEsClient()).asScoped(request).callAsCurrentUser(...args),
-          callAsInternalUser: async (...args) =>
-            (await getWatcherEsClient()).asScoped(request).callAsInternalUser(...args),
-        },
+        getClient: async () => (await getWatcherEsClient()).asScoped(request),
       };
     });
 
diff --git a/x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts b/x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts
index d72e5ad2f8..89a9daf56a 100644
--- a/x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts
+++ b/x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts
@@ -40,7 +40,7 @@ export function registerListFieldsRoute(deps: RouteDependencies) {
       const { indexes } = request.body;
 
       try {
-        const fieldsResponse = await fetchFields(ctx.watcher!.client, indexes);
+        const fieldsResponse = await fetchFields(await ctx.watcher!.getClient(), indexes);
         const json = fieldsResponse.status === 404 ? { fields: [] } : fieldsResponse;
         const fields = Fields.fromUpstreamJson(json);
         return response.ok({ body: fields.downstreamJson });

This comment has been minimized.

Copy link
@sebelga

sebelga Mar 18, 2020

Contributor

Yes this is much better 馃槉 Thanks for making this change!

Depending on the route handler, we might need to externalize the client fetching once at the top whenever it is needed in multiple places.

This comment has been minimized.

Copy link
@sebelga

sebelga Mar 18, 2020

Contributor

Why is it "legacy" es? (coreStart.elasticsearch.legacy.createClient)

This comment has been minimized.

Copy link
@rudolf

rudolf Mar 18, 2020

Author Contributor

We're going to introduce the new Elasticsearch-js client soon (Kibana is currently using a deprecated version). So while we're making these breaking changes we thought it's a good idea to move these into a legacy namespace so that introducing the new client won't be another breaking change.

This comment has been minimized.

Copy link
@rudolf

rudolf Mar 18, 2020

Author Contributor

Since creating a custom ES client and then using the request handler context to share it with routes is such a common pattern I did a quick POC of another approach. Core can expose a createScopedClient method from the request handler context that allows you to create a custom client scoped to the incoming request.

@elastic/kibana-platform what do you think of something like:

diff --git a/src/core/server/elasticsearch/elasticsearch_service.ts b/src/core/server/elasticsearch/elasticsearch_service.ts
index 9785bd6700..135767d931 100644
--- a/src/core/server/elasticsearch/elasticsearch_service.ts
+++ b/src/core/server/elasticsearch/elasticsearch_service.ts
@@ -70,6 +70,7 @@ export class ElasticsearchService
     clientConfig?: Partial<ElasticsearchClientConfig>
   ) => ICustomClusterClient;
   private adminClient?: IClusterClient;
+  private customClients?: WeakMap<object, ICustomClusterClient>;
 
   constructor(private readonly coreContext: CoreContext) {
     this.kibanaVersion = coreContext.env.packageInfo.version;
@@ -182,9 +183,15 @@ export class ElasticsearchService
       kibanaVersion: this.kibanaVersion,
     }).pipe(takeUntil(this.stop$), shareReplay({ refCount: true, bufferSize: 1 }));
 
+    this.customClients = new WeakMap();
     this.createClient = (type: string, clientConfig: Partial<ElasticsearchClientConfig> = {}) => {
+      if (this.customClients!.has({ type, clientConfig })) {
+        return this.customClients!.get({ type, clientConfig })!;
+      }
       const finalConfig = merge({}, config, clientConfig);
-      return this.createClusterClient(type, finalConfig, deps.http.getAuthHeaders);
+      const customClient = this.createClusterClient(type, finalConfig, deps.http.getAuthHeaders);
+      this.customClients!.set({ type, clientConfig }, customClient);
+      return customClient;
     };
 
     return {
diff --git a/src/core/server/index.ts b/src/core/server/index.ts
index 4a1ac8988e..92755d9d48 100644
--- a/src/core/server/index.ts
+++ b/src/core/server/index.ts
@@ -322,6 +322,12 @@ export interface RequestHandlerContext {
     elasticsearch: {
       dataClient: IScopedClusterClient;
       adminClient: IScopedClusterClient;
+      legacy: {
+        createScopedClient: (
+          type: string,
+          clientConfig?: Partial<ElasticsearchClientConfig>
+        ) => IScopedClusterClient;
+      };
     };
     uiSettings: {
       client: IUiSettingsClient;
diff --git a/src/core/server/server.ts b/src/core/server/server.ts
index 86f1b5153d..25b82ce64e 100644
--- a/src/core/server/server.ts
+++ b/src/core/server/server.ts
@@ -244,8 +244,15 @@ export class Server {
             typeRegistry: this.coreStart!.savedObjects.getTypeRegistry(),
           },
           elasticsearch: {
-            adminClient: coreSetup.elasticsearch.adminClient.asScoped(req),
-            dataClient: coreSetup.elasticsearch.dataClient.asScoped(req),
+            adminClient: this.coreStart!.elasticsearch.legacy.client.asScoped(req),
+            dataClient: this.coreStart!.elasticsearch.legacy.client.asScoped(req),
+            legacy: {
+              createScopedClient: (
+                type: string,
+                clientConfig: Partial<ElasticsearchClientConfig>
+              ) =>
+                this.coreStart!.elasticsearch.legacy.createClient(type, clientConfig).asScoped(req),
+            },
           },
           uiSettings: {
             client: uiSettingsClient,
diff --git a/x-pack/plugins/watcher/server/plugin.ts b/x-pack/plugins/watcher/server/plugin.ts
index dcbc7e6526..a807866115 100644
--- a/x-pack/plugins/watcher/server/plugin.ts
+++ b/x-pack/plugins/watcher/server/plugin.ts
@@ -10,7 +10,6 @@ declare module 'kibana/server' {
   }
 }
 
-import { once } from 'lodash';
 import {
   CoreSetup,
   IScopedClusterClient,
@@ -53,21 +52,11 @@ export class WatcherServerPlugin implements Plugin<void, void, any, any> {
       getLicenseStatus: () => this.licenseStatus,
     };
 
-    const getWatcherEsClient = once(async () => {
-      const [coreStart] = await getStartServices();
-      const config = { plugins: [elasticsearchJsPlugin] };
-      return coreStart.elasticsearch.legacy.createClient('watcher', config);
-    });
-    http.registerRouteHandlerContext('watcher', (ctx, request) => {
-      return {
-        client: {
-          callAsCurrentUser: async (...args) =>
-            (await getWatcherEsClient()).asScoped(request).callAsCurrentUser(...args),
-          callAsInternalUser: async (...args) =>
-            (await getWatcherEsClient()).asScoped(request).callAsInternalUser(...args),
-        },
-      };
-    });
+    const config = { plugins: [elasticsearchJsPlugin] };
+
+    http.registerRouteHandlerContext('watcher', (ctx, request) => ({
+      client: ctx.core.elasticsearch.legacy.createScopedClient('watcher', config),
+    }));
 
     registerListFieldsRoute(routeDependencies);
     registerLoadHistoryRoute(routeDependencies);

This comment has been minimized.

Copy link
@joshdover

joshdover Mar 18, 2020

Member

Makes total sense to me. Especially useful for plugins that need to install plugins to the client to support endpoints that do not currently exist in the legacy client (ML comes to mind).

This comment has been minimized.

Copy link
@restrry

restrry Mar 18, 2020

Member

Ok. It makes sense to me. But with this WeakMap every client becomes a singleton, which introduces a lot of side effects (how would we handle es config updates, for example?). I'd avoid adding stateful logic as long as possible and would rather keep once in the plugin.

This comment has been minimized.

Copy link
@pgayvallet

pgayvallet Mar 18, 2020

Contributor

But with this WeakMap every client becomes a singleton, which introduces a lot of side effects

+1. We would need something like a deepEqual test on the clientConfig to get the proper behavior instead of this.customClients!.has({ type, clientConfig }) I think?

This comment has been minimized.

Copy link
@rudolf

rudolf Mar 19, 2020

Author Contributor

I agree that keeping this state in Core introduces some complexity. But if we keep this state in the plugin (with e.g. a once) then Core cannot automatically scope the client to the incoming request which is how most context API's work. I think whether there is a singleton in Core or in the Plugin the problem of how we update Elasticsearch config remains the same.

You're right, WeakMap.has uses reference equality for objects, so the suggestion won't work as is.

I'll revert the watcher changes for now and then open a new PR to discuss and flesh out how this API might look and work.

http.registerRouteHandlerContext('watcher', (ctx, request) => {
return {
client: watcherESClient.asScoped(request),
client: {

This comment has been minimized.

Copy link
@sebelga

sebelga Mar 17, 2020

Contributor

This looks more like a hack than a solution to me. Snapshot and Restore app has a very similar logic than watcher to create the ES client. Should we move all the logic of the client creation + registering routes directly in the start() method?

Copy link
Contributor

mikecote left a comment

Alerting changes LGTM

const router = http.createRouter();
const routeDependencies: RouteDependencies = {
router,
getLicenseStatus: () => this.licenseStatus,
};

const config = { plugins: [elasticsearchJsPlugin] };
const watcherESClient = elasticsearchService.createClient('watcher', config);
const getWatcherEsClient = once(async () => {

This comment has been minimized.

Copy link
@restrry

restrry Mar 19, 2020

Member

are we still okay to provide registerRouteHandlerContext during the start phase? that would simplify this code

This comment has been minimized.

Copy link
@rudolf

rudolf Mar 19, 2020

Author Contributor

registerRouteHandlerContext from start could potentially introduce a race condition where HTTP route handlers are being called before the call to registerRouteHandlerContext finished registering the context.

Copy link
Contributor

pgayvallet left a comment

I see you just reverted some changes (x-pack/plugins/watcher/server/plugin.ts mostly). Is the PR in it's final stage?

@rudolf rudolf requested a review from elastic/apm-ui as a code owner Mar 20, 2020
@rudolf

This comment has been minimized.

Copy link
Contributor Author

rudolf commented Mar 20, 2020

I see you just reverted some changes (x-pack/plugins/watcher/server/plugin.ts mostly). Is the PR in it's final stage?

I just pushed the final easy refactors. What's left will either require significant refactoring in the plugins, or helpers from Core (like the suggestion for a custom scoped client in context).

@rudolf rudolf requested a review from mikecote Mar 20, 2020
Copy link
Contributor

mikecote left a comment

Alerting changes LGTM

@rudolf

This comment has been minimized.

Copy link
Contributor Author

rudolf commented Mar 23, 2020

@elasticmachine merge upstream

const currentConfig = (this.currentConfig = await mergedConfig$
.pipe(take(1))
.toPromise());
Comment on lines 74 to 76

This comment has been minimized.

Copy link
@sqren

sqren Mar 23, 2020

Member

For simplicity let's stick to a single assignment

Suggested change
const currentConfig = (this.currentConfig = await mergedConfig$
.pipe(take(1))
.toPromise());
this.currentConfig = await mergedConfig$
.pipe(take(1))
.toPromise();

This comment has been minimized.

Copy link
@rudolf

rudolf Mar 23, 2020

Author Contributor

If using only the class variable, typescript is unable to infer if the class variable is defined in async context requiring a ! to override the possibly undefined values. So I preferred the type safety of another local variable. I've addressed your suggestions in 4898cfc let me know what you think?

@@ -49,30 +57,23 @@ export class APMPlugin implements Plugin<APMPluginContract> {
usageCollection?: UsageCollectionSetup;
}
) {
const logger = this.initContext.logger.get('apm');
const logger = (this.logger = this.initContext.logger.get('apm'));

This comment has been minimized.

Copy link
@sqren

sqren Mar 23, 2020

Member

Like below, let's stick to a single assignment:

Suggested change
const logger = (this.logger = this.initContext.logger.get('apm'));
this.logger = this.initContext.logger.get('apm');
@sqren
sqren approved these changes Mar 23, 2020
Copy link
Member

sqren left a comment

Just a couple of nits regarding assignment

@kibanamachine

This comment has been minimized.

Copy link

kibanamachine commented Mar 25, 2020

馃挌 Build Succeeded

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
kibana-platform
  
Pending Review
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can鈥檛 perform that action at this time.