Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository #48882

rudolf · 2019-10-22T10:51:06Z

Summary

Split legacy plugin discovery out of LegacyService setup
Expose the following SavedObjects API's:

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.
   */
  scopedRepository: (req: KibanaRequest, extraTypes?: string[]) => ISavedObjectsRepository;

  /**
   * Creates a {@link ISavedObjectsRepository | Saved Objects repository} that
   * uses the internal Kibana user for authenticating with Elasticsearch.
   *
   * For a client that uses the credentials and associated privileges of the
   * incoming request see the Saved Objects client exposed from the
   * {@link RequestHandlerContext}.
   */
  internalRepository: (extraTypes?: string[]) => ISavedObjectsRepository;
}

export interface SavedObjectsServiceStart {
  /**
   * Creates a {@link SavedObjectsClientContract | Saved Objects client} that
   * uses the credentials from the passed in request to authenticate with
   * Elasticsearch. Any middleware
   *
   * A client that is already scoped to the incoming request is also exposed
   * from the route handler context see {@link RequestHandlerContext}.
   */
  scopedClient: (
    req: KibanaRequest,
    options?: SavedObjectsClientProviderOptions
  ) => SavedObjectsClientContract;
}

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

elasticmachine · 2019-10-22T10:51:08Z

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

elasticmachine · 2019-10-22T11:03:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5026bce83967f16e0797b9aaf552a5f55f8195ed

elasticmachine · 2019-10-22T13:27:25Z

💚 Build Succeeded

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

elasticmachine · 2019-10-24T12:59:12Z

💚 Build Succeeded

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

joshdover

This internalClient does not expose a way to use "fake credentials" to get a scoped client. I know we need this for a few background workers that do work on behalf of a user, using cached credentials (eg. Reporting, Upgrade Assistant). Are you planning on doing this as a separate change?

elasticmachine · 2019-10-30T20:09:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 38d05b3

rudolf · 2019-11-01T14:06:48Z

After speaking to security, uptime and APM it seems like the existing usage in legacy was incorrect and really should be using scoped saved objects clients. So it doesn't appear like there's any valid dependencies on a SavedObjectsSetup.internalClient. I'll keep this PR and use it to expose a scoped client, scoped repository and internal repository.

joshdover · 2019-11-11T19:15:48Z

Is this still ready for review? It seems it still exposes the internalClient?

elasticmachine · 2019-11-13T12:14:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 671daf7

elasticmachine · 2019-11-20T22:43:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9be6cc3

elasticmachine · 2019-11-22T15:11:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 01403db

pgayvallet · 2019-11-22T15:08:38Z

src/core/server/plugins/plugins_service.ts

-export interface PluginsServiceStartDeps {} // eslint-disable-line @typescript-eslint/no-empty-interface
+export type PluginsServiceStartDeps = InternalCoreStart;


I'm not totally convinced we want exact type equivalence here ?

In https://github.com/elastic/kibana/pull/51438/files#diff-725c498a267472754f01cff2baa3901b I explicitly defined the properties

- export interface PluginsServiceStartDeps {} // eslint-disable-line @typescript-eslint/no-empty-interface + export interface PluginsServiceStartDeps { + capabilities: InternalCoreStart['capabilities']; + }

But I'm not sure which one is preferable? I'm guessing we may not necessarily want alls internal core in the service deps? wdyt?

I don't think it needs to be exactly equivalent, but I don't see the advantage to a more minimal typing, especially since the plugin service will probably require almost every API except the legacy ones which we'll remove later. The more strictly we type it, the more boilerplate it takes to introduce a new API. Although it makes dependencies explicit, I don't think having several layers of slightly different types makes the code more readable, gives us better guarantees or reduced bugs.

Reduced boilerplate is a very valid argument

pgayvallet · 2019-11-22T15:17:17Z

src/core/server/saved_objects/saved_objects_service.ts

+ * When plugins access the Saved Objects client, a new client is created using
+ * the factory provided to `setClientFactory` and wrapped by all wrappers
+ * registered through `addClientWrapper`. To create a factory or wrapper,
+ * plugins will have to construct a Saved Objects client. First create a
+ * repository by calling `scopedRepository` or `internalRepository` and then
+ * use this repository as the argument to the {@link SavedObjectsClient}


Honest question, is the setClientFactory isolated per-plugin, or is this the responsibility or a single plugin to define it for everyone? What is the actual expected usage of this setter?

These extension points to Saved Objects are there to support the x-pack security plugins: security, encrypted saved objects and spaces and they apply globally, to all plugins that use the saved objects client. This makes the documentation hard to write, cause it's there to solve a very specific problem.

There's broad consensus that this factory and the client wrappers aren't the most elegant architecture, but the way it worked in legacy did the job so we won't try to improve this until after 8.0.0. The security team might have some upcoming requirements that require something more like middleware than just composing clients over each other, but there's no concrete plans yet.

Not sure if we're there yet, but if we are sure that this pattern will be replaced, we should mark the setClientFactory and addClientWrapper methods @deprecated.

A public API that allows only to be called once sure seems like fragile :) But this will do the job until proper revision.

elasticmachine · 2019-11-22T16:15:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9eca319

src/core/server/saved_objects/saved_objects_service.ts

joshdover · 2019-11-25T18:34:43Z

src/core/server/saved_objects/saved_objects_service.ts

+ * When plugins access the Saved Objects client, a new client is created using
+ * the factory provided to `setClientFactory` and wrapped by all wrappers
+ * registered through `addClientWrapper`. To create a factory or wrapper,
+ * plugins will have to construct a Saved Objects client. First create a
+ * repository by calling `scopedRepository` or `internalRepository` and then
+ * use this repository as the argument to the {@link SavedObjectsClient}


Not sure if we're there yet, but if we are sure that this pattern will be replaced, we should mark the setClientFactory and addClientWrapper methods @deprecated.

elasticmachine · 2019-11-27T13:29:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 145c292

…ra/kibana into IS-46410_remove-@kbn/ui-framework * 'IS-46410_remove-@kbn/ui-framework' of github.com:mbondyra/kibana: (49 commits) [ML] Re-activate after method in transform test (elastic#51815) [SIEM] [Detection Engine] Add edit on rule creation (elastic#51670) De-angularize visLegend (elastic#50613) [SIEM][Detection Engine] Change security model to use SIEM permissions [Monitoring] Sass cleanup (elastic#51100) Move errors and validate index pattern ⇒ NP (elastic#51805) fixes pagination tests (elastic#51822) Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository (elastic#48882) [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) [Lens] Remove client-side reference to server source code (elastic#51763) Fix infinite redirect loop when multiple cookies are sent (elastic#50452) fixes drag and drop in tests (elastic#51806) [Console] Proxy fallback (elastic#50185) Query String(Bar) Input - cleanup (elastic#51598) shim visualizations plugin (elastic#50624) Expressions service fixes: better error and loading states handling (elastic#51183) fixes url state tests (elastic#51746) fixes browser field tests (elastic#51738) [ML] Fix anomaly detection test suite (elastic#51712) [SIEM] Fix Timeline drag and drop behavior (elastic#51558) ...

…license-management * 'master' of github.com:elastic/kibana: (48 commits) Enable alerting and actions plugin by default (elastic#51254) Fix error returned when creating an alert with ES security disabled (elastic#51639) [Discover] Improve Percy functional tests (elastic#51699) fixes timeline data providers tests (elastic#51862) [Dependencies]: upgrade react to latest v16.12.0 (elastic#51145) Allow routes to define some payload config values (elastic#50783) Move saved queries service + language switcher ⇒ NP (elastic#51812) [ML] Re-activate after method in transform test (elastic#51815) [SIEM] [Detection Engine] Add edit on rule creation (elastic#51670) De-angularize visLegend (elastic#50613) [SIEM][Detection Engine] Change security model to use SIEM permissions [Monitoring] Sass cleanup (elastic#51100) Move errors and validate index pattern ⇒ NP (elastic#51805) fixes pagination tests (elastic#51822) Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository (elastic#48882) [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) [Lens] Remove client-side reference to server source code (elastic#51763) Fix infinite redirect loop when multiple cookies are sent (elastic#50452) fixes drag and drop in tests (elastic#51806) [Console] Proxy fallback (elastic#50185) ...

…pers, repository (elastic#48882) * Split legacy plugin discovery, expose internal SavedObjectsClient * Expose internal SavedObjectsClient to plugins * Add more documentation * Expose client wrappers, repository, scoped client from SavedObjects * Remove unused onBeforeWrite * Refactor Service / Repository for testability * Bind exposed clientProvider methods * Fix eArchiver's KibanaMigrator * Cleanup * Use APICaller type * Expose SavedObjectsServiceStart to plugins * API documentation * Rename API methods to be verbs

…pers, repository (#48882) (#51978) * Split legacy plugin discovery, expose internal SavedObjectsClient * Expose internal SavedObjectsClient to plugins * Add more documentation * Expose client wrappers, repository, scoped client from SavedObjects * Remove unused onBeforeWrite * Refactor Service / Repository for testability * Bind exposed clientProvider methods * Fix eArchiver's KibanaMigrator * Cleanup * Use APICaller type * Expose SavedObjectsServiceStart to plugins * API documentation * Rename API methods to be verbs

rudolf added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v7.6.0 labels Oct 22, 2019

rudolf added 2 commits October 22, 2019 14:02

Split legacy plugin discovery, expose internal SavedObjectsClient

0a0082c

Expose internal SavedObjectsClient to plugins

df75b17

rudolf force-pushed the saved-object-setup-client branch from 5026bce to df75b17 Compare October 22, 2019 12:09

rudolf marked this pull request as ready for review October 22, 2019 13:43

rudolf requested a review from a team as a code owner October 22, 2019 13:43

rudolf added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 labels Oct 22, 2019

rudolf added 2 commits October 24, 2019 13:43

Add more documentation

0d101e0

Merge branch 'master' into saved-object-setup-client

c6ddbfe

joshdover reviewed Oct 25, 2019

View reviewed changes

Merge branch 'master' into saved-object-setup-client

38d05b3

Merge branch 'master' into saved-object-setup-client

ca74aa1

Merge branch 'master' into saved-object-setup-client

671daf7

pgayvallet mentioned this pull request Nov 13, 2019

Kibana should crash immediately when misconfigured #49378

Closed

Expose client wrappers, repository, scoped client from SavedObjects

17935e7

dgieselaar mentioned this pull request Nov 20, 2019

[APM] Migrate server routes to NP #49455

Merged

rudolf added 2 commits November 20, 2019 22:15

Use APICaller type

561e857

Merge branch 'master' into saved-object-setup-client

9be6cc3

rudolf requested a review from joshdover November 20, 2019 21:24

dgieselaar mentioned this pull request Nov 21, 2019

[APM] Plugin & request context and query setup #51257

Closed

jasonrhodes mentioned this pull request Nov 21, 2019

[APM] New Kibana platform migration plan #32894

Closed

rudolf added 3 commits November 22, 2019 14:21

Expose SavedObjectsServiceStart to plugins

de35589

Merge branch 'master' into saved-object-setup-client

01403db

API documentation

9eca319

pgayvallet reviewed Nov 22, 2019

View reviewed changes

rudolf requested a review from pgayvallet November 25, 2019 14:36

joshdover approved these changes Nov 25, 2019

View reviewed changes

pgayvallet approved these changes Nov 26, 2019

View reviewed changes

rudolf added 2 commits November 27, 2019 12:17

Merge branch 'master' into saved-object-setup-client

290679b

Rename API methods to be verbs

145c292

rudolf merged commit f29af12 into elastic:master Nov 27, 2019

rudolf deleted the saved-object-setup-client branch November 27, 2019 15:47

rudolf mentioned this pull request Dec 2, 2019

[7.x] Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository (#48882) #51978

Merged

rudolf mentioned this pull request Dec 3, 2019

static createRepository factory function #52056

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository #48882

Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository #48882

rudolf commented Oct 22, 2019 •

edited

Loading

elasticmachine commented Oct 22, 2019

elasticmachine commented Oct 22, 2019

elasticmachine commented Oct 22, 2019

elasticmachine commented Oct 24, 2019

joshdover left a comment

elasticmachine commented Oct 30, 2019

rudolf commented Nov 1, 2019

joshdover commented Nov 11, 2019

elasticmachine commented Nov 13, 2019

elasticmachine commented Nov 20, 2019

elasticmachine commented Nov 22, 2019

pgayvallet Nov 22, 2019

rudolf Nov 25, 2019

pgayvallet Nov 26, 2019

pgayvallet Nov 22, 2019

rudolf Nov 25, 2019

joshdover Nov 25, 2019

pgayvallet Nov 26, 2019

elasticmachine commented Nov 22, 2019

joshdover Nov 25, 2019

elasticmachine commented Nov 27, 2019

		export interface PluginsServiceStartDeps {} // eslint-disable-line @typescript-eslint/no-empty-interface
		export type PluginsServiceStartDeps = InternalCoreStart;

Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository #48882

Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository #48882

Conversation

rudolf commented Oct 22, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Oct 22, 2019

elasticmachine commented Oct 22, 2019

💔 Build Failed

elasticmachine commented Oct 22, 2019

💚 Build Succeeded

elasticmachine commented Oct 24, 2019

💚 Build Succeeded

joshdover left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 30, 2019

💚 Build Succeeded

rudolf commented Nov 1, 2019

joshdover commented Nov 11, 2019

elasticmachine commented Nov 13, 2019

💔 Build Failed

elasticmachine commented Nov 20, 2019

💚 Build Succeeded

elasticmachine commented Nov 22, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 22, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented Nov 27, 2019

💚 Build Succeeded

rudolf commented Oct 22, 2019 •

edited

Loading