[Beats Management] add more tests, update types, break out ES into it's own adapter

[mattapperson]

Sorry, but this PR is sort of a catch-all for a few cleanup tasks.

1. This adds a few additional tests
2. It adds a lot more types, particularly around Kibana's ES plugin
3. Broke out all ES related functions into its own DatabaseAdapter
4. Broke out types related to adapters into their own TS files inside their pater folders
5. Began a convention of prefixing types related to an adapter with the name of the adapter type, thus making it easier to know what types resided where when using a type too
6. Removed incorrect happy types (correct types do not exist)
7. Re-typed internal request system so we pass around JUST the user vs the whole request object

Once the convention change is accepted I can PR a change to the docs to include said changes

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[mattapperson]

CI failure is not related to the PR

[weltenwort]

I like the factored-out database adapter 👍 Left a few inline comments about the specific api.

There is a typo in the directory name server/lib/adapters/famework that caught my eye. 😇

The testing setup is a bit problematic since it breaks the normal unit/integration boundary. As mentioned in the comment below I would recommend to work with the Kibana platform team to solve that.

I'm also seeing a few errors from tsc that are related to expect.js and jest's expect. Not sure if they originate from this PR, but they don't occur for me on master.

x-pack/node_modules/@types/expect.js/index.d.ts(6,18): error TS2451: Cannot redeclare block-scoped variable 'expect'.
x-pack/node_modules/@types/jest/index.d.ts(32,15): error TS2451: Cannot redeclare block-scoped variable 'expect'.
x-pack/plugins/beats/server/utils/error_wrappers/wrap_es_error.test.ts(33,21): error TS2339: Property 'statusCode' does not exist on type 'Error'.
x-pack/plugins/beats/server/utils/wrap_request.ts(7,10): error TS2305: Module '"/home/laeroth/Projects/elastic/kibana/x-pack/plugins/beats/server/lib/lib"' has no exported member 'FrameworkRequest'.
x-pack/plugins/beats/server/utils/wrap_request.ts(7,28): error TS2305: Module '"/home/laeroth/Projects/elastic/kibana/x-pack/plugins/beats/server/lib/lib"' has no exported member 'WrappableRequest'.
x-pack/plugins/beats/server/utils/wrap_request.ts(14,11): error TS2459: Type 'InternalRequest' has no property 'params' and no string index signature.
x-pack/plugins/beats/server/utils/wrap_request.ts(14,19): error TS2459: Type 'InternalRequest' has no property 'payload' and no string index signature.
x-pack/plugins/beats/server/utils/wrap_request.ts(14,28): error TS2459: Type 'InternalRequest' has no property 'query' and no string index signature.
x-pack/plugins/beats/server/utils/wrap_request.ts(14,35): error TS2459: Type 'InternalRequest' has no property 'headers' and no string index signature.
x-pack/plugins/beats/server/utils/wrap_request.ts(14,44): error TS2459: Type 'InternalRequest' has no property 'info' and no string index signature.
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/get_clips.test.ts(20,26): error TS2339: Property 'toBe' does not exist on type 'Root'.
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/get_clips.test.ts(22,24): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(70,32): error TS2339: Property 'toBe' does not exist on type 'Root'.
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(71,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(74,16): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(89,32): error TS2339: Property 'toBe' does not exist on type 'Root'.
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(90,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(93,16): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(109,32): error TS2339: Property 'toBe' does not exist on type 'Root'.
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(110,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(111,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(114,16): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(130,32): error TS2339: Property 'toBe' does not exist on type 'Root'.
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(131,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(132,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(135,16): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(154,32): error TS2339: Property 'toBe' does not exist on type 'Root'.
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(155,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(156,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(157,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(158,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(161,16): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(180,32): error TS2339: Property 'toBe' does not exist on type 'Root'.
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(181,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(182,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(183,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(184,31): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?
x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.test.ts(187,16): error TS2551: Property 'toEqual' does not exist on type 'Root'. Did you mean 'equal'?

[weltenwort]

If we added overloaded and parameterized typings for the callWithRequest and callWithInternalUser functions too we could enforce typing all the way through the call chain, instead of downgrading the return value to any here. 🤔

kibana/x-pack/plugins/infra/server/lib/infra_types.ts

Lines 87 to 106 in a4851bc

	callWithRequest<Hit = {}, Aggregation = undefined>(
	req: InfraFrameworkRequest,
	method: 'search',
	options?: object
	): Promise<InfraDatabaseSearchResponse<Hit, Aggregation>>;
	callWithRequest<Hit = {}, Aggregation = undefined>(
	req: InfraFrameworkRequest,
	method: 'msearch',
	options?: object
	): Promise<InfraDatabaseMultiResponse<Hit, Aggregation>>;
	callWithRequest(
	req: InfraFrameworkRequest,
	method: 'fieldCaps',
	options?: object
	): Promise<InfraDatabaseFieldCapsResponse>;
	callWithRequest(
	req: InfraFrameworkRequest,
	method: string,
	options?: object
	): Promise<InfraDatabaseSearchResponse>;

[mattapperson]

I did this because the typing on data returned from callWithRequest is assumed, and in fact at runtime could be very incorrect. This framework adapter is only used by other domain based adapters such as a beats or tags adapter. Those other adapters primary action is to conform the returned data and return it neatly. Thus expecting any from this (or unknown in future versions of TS) then properly formatting at runtime and returning an accurate type is IMHO a more "honest" approach to typing this solution.

[weltenwort]

Yes, good point. I wonder if elasticsearch-js performs any validation. 🤔

[mattapperson]

Not so far as I can tell

[weltenwort]

This is on the one hand a clever way to perform the user/admin switch, but on the other hand it has a few shortcomings IMHO:

• Only the case where null is passed explicitly for req at the call site communicates the intent of the developer. In other cases there is no easy way to see whether the call is intended to be performed with kibana or user privileges.
• In addition to and following from the communication problem above, this feels like it could open the door to security problems later. If the user somehow managed to get a "falsey" value passed as the request, she could impersonate the Kibana user for the request.

I can imagine two ways to redesign the API to work around that:

1. Use a unique sentinel instead of null to signal the intent to use Kibana privileges, e.g.

   // in this file
   const InternalRequest: unique symbol = Symbol('InternalRequest');
   
   export class KibanaDatabaseAdapter implements DatabaseAdapter {
       public readonly InternalRequest = InternalRequest;
   
       /* ... */
   
       private getType(req: FrameworkRequest | typeof InternalRequest, params: {}) {
         if (req === this.InternalRequest) {
           return this.es.callWithInternalUser;
         } else {
           return this.es.callWithRequest.bind(null, req);
         }
       }
   }
   
   // in a consumer of the adapter
   database.putTemplate(database.InternalRequest, { /* ... */ }); // kibana privileges
   database.putTemplate(upstreamReq, { /* ... */ }); // user privileges

2. Introduce a chaining api that binds the methods to the appropriate request explicitly:

   interface CallDatabase {
     <Hit = {}, Aggregations = undefined>(
       method: 'search',
       options?: SearchOptions
     ): Promise<DatabaseSearchResponse<Hit, Aggregations>>;
     <Source>(
       method: 'get',
       options?: GetOptions
     ): Promise<DatabaseGetResponse<Source>>;
     /* many more... */
     (method: string, options?: object): Promise<DatabaseResponse>;
   }
   
   
   class BoundDatabaseApi {
     private readonly callDatabase: CallDatabase;
   
     constructor(callDatabase: CallDatabase) {
       this.callDatabase = callDatabase;
     }
   
     public async get<Source>(options?: GetOptions) {
       return await this.callDatabase('get', options);
     }
   
     /* many more... */
   }
   
   export class KibanaDatabaseAdapter implements DatabaseAdapter {
     private es: DatabaseKbnESCluster;
   
     constructor(kbnElasticSearch: DatabaseKbnESPlugin) {
       this.es = kbnElasticSearch.getCluster('admin');
     }
   
     public withInternalUser() {
       return new BoundDatabaseApi(this.es.callWithInternalUser);
     }
   
     public withRequest(request: FrameworkRequest) {
       return new BoundDatabaseApi(((...args) => this.es.callWithRequest(request, ...args)) as CallDatabase);
     }
   }
   
   // in a consumer
   database.withInternalUser().get(/* ... */);
   database.withRequest(request).get(/* ... */);

The first option would be a quick adjustment to the current solution while the second would be more effort.

[mattapperson]

@weltenwort I considered that. The request object is very far from easy to return null, as it is the very essence of hapi. If that were to be hacked all bets are off anyway.

That said I think an argument can be made perhaps for readability to use version 1 of your suggestion?

[weltenwort]

It's not just null, but any value that is coerced to false. Hapi does not need to be hacked, a simple bug is enough.

I would just recommend to be a bit more defensive here since it effectively is "the permission check" for all Elasticsearch requests performed via this adapter.

Yes, variant 1 proposed above should already protect from that.

[mattapperson]

OK, I will make that change. Even if just for readability I think its worth it

[mattapperson]

fixed

[weltenwort]

Why not just support adapterSetup() (or getAdapter())? In the case where it's a singleton it could just be implemented as () => myStaticDatabaseAdapter.

[mattapperson]

Yea, I probably could... was trying to be overly cleaver here

[mattapperson]

fixed

[weltenwort]

It could be problematic to use this api here, because it might not honor the user's selection of the source to obtain Elasticsearch from (snapshot, source, etc) as our test runner usually do. That will probably lead to problems on our CI systems as well. I would recommend to talk to the @elastic/kibana-platform team to integrate it properly into the test runner.

[mattapperson]

Per kibana-platform, this should be fine as-is as it uses process.env.TEST_ES_FROM internally.

[weltenwort]

interesting, thanks for clarifying that

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request