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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
6 tasks done
pgayvallet opened this issue Jan 21, 2020 · 11 comments · Fixed by #67596
Closed
6 tasks done

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

pgayvallet opened this issue Jan 21, 2020 · 11 comments · Fixed by #67596
Assignees
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@pgayvallet
Copy link
Contributor

pgayvallet commented Jan 21, 2020

In #55012 we decided to remove any API that could allow to access/query savedObjects from core setup contract, and move them to the start contract instead.

#55396 plans to do the same thing regarding UiSettings.

Steps:

@pgayvallet pgayvallet added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Jan 21, 2020
@elasticmachine
Copy link
Contributor

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

@rudolf
Copy link
Contributor

rudolf commented Mar 10, 2020

I've audited all the x-pack plugins which use the Elasticsearch client from CoreSetup:

  • Security constructs an Elasticsearch client with a custom plugin and uses it to register http authentication and authorization and http routes. Once we get a valid licence it also uses the Elalsticsearch client to register privileges with Elasticsearch.
    this.clusterClient = core.elasticsearch.createClient('security', {
    plugins: [elasticsearchClientPlugin],
    });
    const existingPrivileges = await clusterClient.callAsInternalUser('shield.getPrivilege', {
  • Reporting validates the Kibana vs ES max content length settings. According to the comments this should be done "early" presumably to provide an warning before any work is started.
    const elasticClusterSettingsResponse = await callAsInternalUser('cluster.getSettings', {
    includeDefaults: true,
    });
    const { persistent, transient, defaults: defaultSettings } = elasticClusterSettingsResponse;
    const elasticClusterSettings = defaults({}, persistent, transient, defaultSettings);
    const elasticSearchMaxContent = get(elasticClusterSettings, 'http.max_content_length', '100mb');
    const elasticSearchMaxContentBytes = numeral().unformat(elasticSearchMaxContent.toUpperCase());
    const kibanaMaxContentBytes: number = config.get(KIBANA_MAX_SIZE_BYTES_PATH);
    if (kibanaMaxContentBytes > elasticSearchMaxContentBytes) {
    // TODO this should simply throw an error and let the handler conver it to a warning mesasge. See validateServerHost.
    logger.warning(
    `${KIBANA_MAX_SIZE_BYTES_PATH} (${kibanaMaxContentBytes}) is higher than ElasticSearch's ${ES_MAX_SIZE_BYTES_PATH} (${elasticSearchMaxContentBytes}). ` +
    `Please set ${ES_MAX_SIZE_BYTES_PATH} in ElasticSearch to match, or lower your ${KIBANA_MAX_SIZE_BYTES_PATH} in Kibana to avoid this warning.`
    );
    }
  • Telemetry
    registerCollection(telemetryCollectionManager, elasticsearch.dataClient);
  • Monitoring uses dataClient to create callWithRequest for the legacy shim
    esDataClient: core.elasticsearch.dataClient,

    name === 'monitoring' ? cluster : this.legacyShimDependencies.esDataClient;
  • Monitoring to get cluster uuid. can be moved as it's an async operation
    elasticsearch.adminClient.callAsInternalUser('info').then(data => {
  • Rollup creates an Elasticsearch client with a custom plugin and then uses this client from it's registered routes. Rollup would have to use getStartServices to get access to ElasticsearchService
    return elasticsearchService.createClient('rollup', config);
    const getJobsHandler: RequestHandler<any, any, any> = async (ctx, request, response) => {
    const callWithRequest = callWithRequestFactory(deps.elasticsearchService, request);
  • Actions accesses the Elasticsearch adminClient from setup to use it in start
    this.adminClient = core.elasticsearch.adminClient;
  • Alerting accesses the Elasticsearch adminClient from setup to use it in start
    this.adminClient = core.elasticsearch.adminClient;
  • APM uses Elasticsearch from setup to create it's own indices
    // create agent configuration index without blocking setup lifecycle
    createApmAgentConfigurationIndex({
    esClient: core.elasticsearch.dataClient,
    config: currentConfig,
    logger
    });
    // create custom action index without blocking setup lifecycle
    createApmCustomLinkIndex({
    esClient: core.elasticsearch.dataClient,
    config: currentConfig,
    logger
    });
  • EventLog constructs an object during setup which then initialises it's index during start
    this.esContext.initialize();
  • Lens registers a task to collect telemetry during setup
    export function telemetryTaskRunner(
    logger: Logger,
    core: CoreSetup,
    config: Observable<{ kibana: { index: string } }>
    ) {
    return ({ taskInstance }: RunContext) => {
    const { state } = taskInstance;
    const callCluster = core.elasticsearch.adminClient.callAsInternalUser;
  • Licensing constructs a licence poller during setup
    const { refresh, license$ } = this.createLicensePoller(
    dataClient,
    pollingFrequency.asMilliseconds()
    );
  • ML constructs an Elasticsearch client with custom plugin and then exposes it through the request context
    const mlClient = coreSetup.elasticsearch.createClient(PLUGIN_ID, {
    plugins: [elasticsearchJsPlugin],
    });
    coreSetup.http.registerRouteHandlerContext(PLUGIN_ID, (context, request) => {
    return {
    mlClient: mlClient.asScoped(request),
    };
    });
  • OssTelemetry registers a task definition for collecting telemetry during setup and then schedules the tasks to run during start
    taskManager.registerTaskDefinitions({
    [VIS_TELEMETRY_TASK]: {
    title: 'X-Pack telemetry calculator for Visualizations',
    type: VIS_TELEMETRY_TASK,
    createTaskRunner({ taskInstance }: { taskInstance: TaskInstance }) {
    return {
    run: visualizationsTaskRunner(taskInstance, config, elasticsearch),
    };
    },
    },
    });
  • RemoteClusters seems to have an unused dependency on Elasticsearch during setup
    elasticsearch,
    elasticsearchService,
  • SnapshotRestore creates a elasticsearch client with custom plugin during setup and exposes it through the route context
    const esClientConfig = { plugins: [elasticsearchJsPlugin] };
    const snapshotRestoreESClient = elasticsearch.createClient('snapshotRestore', esClientConfig);
    http.registerRouteHandlerContext('snapshotRestore', (ctx, request) => {
    return {
    client: snapshotRestoreESClient.asScoped(request),
    };
    });
  • Spaces client uses Elasticsearch to create a spaces SavedObjectsClient wrapper. It's also used to create a default space saved object document which happens after the licensing plugin emits a valid licence.
    const internalRepository = this.getLegacyAPI().savedObjects.getSavedObjectsRepository(
    elasticsearch.adminClient.callAsInternalUser,
    ['space']
    );
    const callCluster = elasticsearch.adminClient.asScoped(request).callAsCurrentUser;
    const callWithRequestRepository = this.getLegacyAPI().savedObjects.getSavedObjectsRepository(
    callCluster,
    ['space']
    );
    createDefaultSpace: async () => {
    return await createDefaultSpace({
    esClient: core.elasticsearch.adminClient,
    savedObjects: this.getLegacyAPI().savedObjects,
    });
    },
  • TaskManager constructs a task manager using the Elasticsearch client, but only starts it in the start lifecycle (currently after all legacy plugins are loaded).
    createTaskManager(core, {
    logger,
    config,
    elasticsearch,
    savedObjectsRepository,
    savedObjectsSerializer: savedObjects.createSerializer(),
    })
    this.kbnServer.afterPluginsInit(() => {
    taskManagerPlugin.start();
    });
  • Transform creates an elasticsearch client with custom plugin and exposes it through the request handler context
    const transformClient = elasticsearch.createClient('transform', {
    plugins: [elasticsearchJsPlugin],
    });
    http.registerRouteHandlerContext('transform', (context, request) => {
    return {
    dataClient: transformClient.asScoped(request),
    };
    });
  • UpgradeAssistant pull Elasticsearch client during setup and uses it during start
    this.elasticSearchService = elasticsearch;
  • Watcher creates an Elasticsearch client with custom plugin and exposes it through the request handler context
    const config = { plugins: [elasticsearchJsPlugin] };
    const watcherESClient = elasticsearchService.createClient('watcher', config);
    http.registerRouteHandlerContext('watcher', (ctx, request) => {
    return {
    client: watcherESClient.asScoped(request),
    };
    });
  • Expressions implements a bulk fetch route
    const scopedClient = core.elasticsearch.dataClient.asScoped(request);
  • Observability uses dataClient in getScopedAnnotationsClient to get scoped client
    getScopedAnnotationsClient: (request: KibanaRequest) => {
    return createAnnotationsClient({
    index,
    apiCaller: core.elasticsearch.dataClient.asScoped(request).callAsCurrentUser,
    logger,
    });
    },
  • APM reporting
    const esClient = core.elasticsearch.dataClient;
  • File uploader
    setElasticsearchClientServices(core.elasticsearch);

OSS usages:

@rudolf
Copy link
Contributor

rudolf commented Mar 11, 2020

Several plugins construct Elasticsearch clients with custom plugins and then inject these into their routes by registering a route handler context. To refactor these to get their Elasticsearch client from start gets awkward:

--- a/x-pack/plugins/snapshot_restore/server/plugin.ts
+++ b/x-pack/plugins/snapshot_restore/server/plugin.ts
@@ -72,11 +72,18 @@ export class SnapshotRestoreServerPlugin implements Plugin<void, void, any, any>
       }
     );
 
-    const esClientConfig = { plugins: [elasticsearchJsPlugin] };
-    const snapshotRestoreESClient = elasticsearch.createClient('snapshotRestore', esClientConfig);
+    const getClient = async () => {
+      const esClientConfig = { plugins: [elasticsearchJsPlugin] };
+      const coreStart = (await getStartServices())[0];
+      return coreStart.elasticsearch.createClient('snapshotRestore', esClientConfig);
+    };
+
     http.registerRouteHandlerContext('snapshotRestore', (ctx, request) => {
       return {
-        client: snapshotRestoreESClient.asScoped(request),
+        client: {
+          callAsCurrentUser: async (...args) =>
+            (await getClient()).asScoped(request).callAsCurrentUser(...args),
+          callAsInternalUser: async (...args) =>
+            (await getClient()).asScoped(request).callAsInternalUser(...args),
+        },
       };
     });

Although it would require downstream changes, the other alternative is to expose the client as an async function:

--- a/x-pack/plugins/snapshot_restore/server/plugin.ts
+++ b/x-pack/plugins/snapshot_restore/server/plugin.ts
@@ -72,11 +72,15 @@ export class SnapshotRestoreServerPlugin implements Plugin<void, void, any, any>
       }
     );
 
-    const esClientConfig = { plugins: [elasticsearchJsPlugin] };
-    const snapshotRestoreESClient = elasticsearch.createClient('snapshotRestore', esClientConfig);
+    const getClient = async () => {
+      const esClientConfig = { plugins: [elasticsearchJsPlugin] };
+      const coreStart = (await getStartServices())[0];
+      return coreStart.elasticsearch.createClient('snapshotRestore', esClientConfig);
+    };
+
     http.registerRouteHandlerContext('snapshotRestore', (ctx, request) => {
       return {
-        client: snapshotRestoreESClient.asScoped(request),
+        getClient,
       };
     });

@rudolf rudolf changed the title Move ES client creation/access API from core setup to start contract Move elasticsearch client creation/access API from core setup to start contract Mar 11, 2020
@joshdover
Copy link
Contributor

Several plugins construct Elasticsearch clients with custom plugins and then inject these into their routes by registering a route handler context.

I know it breaks the convention of "register in setup, do things in start", but I think it would make sense if we exposed the registerRequestHandlerContext API during start. The objects that are exposed in context are things that should only be available during start anyways. Even core can't register it's request context with the ES client until start.

WDYT?

@mshustov
Copy link
Contributor

Even core can't register it's request context with the ES client until start.

It would work as long as we start listening on a port after plugins register their context providers. The problems will start to appear when we deprecate async lifecycles, as we can no longer guarantee that the context contains all the registered API. Everything that we can do to avoid this problem is to educate how to register the async API...

const depsPromise = doAsync();
core.registerRequestHandlerContext(() => {
   async getFoo() {
     return (await depsPromise).getFoo()
   }
})
      

@rudolf
Copy link
Contributor

rudolf commented Mar 13, 2020

Yeah we could say CoreStart.registerRequestHandlerContext can only be used synchronously from start() so that we know we can start handling requests after all plugins' start return. But most start API's remain available during the entire lifecycle so I think this will become very confusing.

@pgayvallet
Copy link
Contributor Author

pgayvallet commented Mar 16, 2020

I know it breaks the convention of "register in setup, do things in start", but I think it would make sense if we exposed the registerRequestHandlerContext API during start. The objects that are exposed in context are things that should only be available during start anyways. Even core can't register it's request context with the ES client until start.

registerRouteHandlerContext could also have a reference to core's start contract one way or another, to be able to keep calling it from setup while exposing the start APIs to it, as it's mean to be used/resolved during startup anyway.

http.registerRouteHandlerContext('snapshotRestore',
      async (context, req, res)=> {...}

could become something like

http.registerRouteHandlerContext('snapshotRestore', coreStart => {
         const snapshotRestoreESClient = coreStart.elasticsearch.createClient('snapshotRestore', esClientConfig);
         return async (context, req, res) => ({
            client: snapshotRestoreESClient.asScoped(request),
         })

This is more complicated to implement probably, but would keep the 'register during setup' convention.

We could even replace the coreStart parameter with [coreStart, pluginStartDeps] if we want to.

@joshdover
Copy link
Contributor

This is more complicated to implement probably, but would keep the 'register during setup' convention.

One thing I'd like to note is that the CoreStart interface is plugin-specific. So you'll need to keep a reference to this value for each plugin and use it when building the context. It is currently stored in the PluginWrapper in src/core/plugins/{public,server}/plugin.ts, so you'll just need a way to pipe these values into the ContextService.

@pgayvallet
Copy link
Contributor Author

Yea, the implementation would requires some low-level refactoring with the contextService.

However as @rudolf said on slack yesterday

@delvedor said the new client supports the entire ES API, so I don't think any plugins should have to create custom clients.
So if this is going away soon, we wouldn't want it to affect Core's API in a lasting way

So probably this registerRouteHandlerContext changes should now longer be an option we want?

@mshustov
Copy link
Contributor

Reporting

Reporting validates the Kibana vs ES max content length settings. According to the comments this should be done "early" presumably to provide a warning before any work is started.

This validation should be moved away from the setup, as we are deprecating async lifecycles. Added a comment to their migration issue #53898 (comment)

Licensing

#67291 migrates to elasticsearch client to the provided from the start contract.

Security

Creates a custom elasticsearch plugin and provides access to it via public API. Switching to elasticsearch client exposed from start requires public API refactoring.
Created issue #66405

Observability & APM

#67263 migrates to the context-provided elasticsearch client and elasticsearch start.

Monitoring

Provides creates a custom es client and provides it to alerting plugin. Added a comment to the migration issue #33671 (comment)

File upload

#67277 switches to es client provided via request handler context.

Telemetry

#67279 an attempt to migrate to es client provided from start contract.

@mshustov
Copy link
Contributor

mshustov commented Jun 2, 2020

Not all the API provided from the setup contract has been migrated. The list of leftovers #55397 (comment)
It's up to consumers to prioritize their API migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants