From de1a53773f0e8859df2ce632afb3b7a554b931a2 Mon Sep 17 00:00:00 2001 From: restrry Date: Tue, 14 Apr 2020 15:23:04 +0200 Subject: [PATCH] address Pierres comments --- x-pack/plugins/licensing/server/index.ts | 5 +-- ... => wrap_route_with_license_check.test.ts} | 28 +++++++++---- ...er.ts => wrap_route_with_license_check.ts} | 2 +- .../lib/check_license/check_license.test.ts | 3 +- x-pack/plugins/logstash/server/plugin.ts | 14 +++++-- .../logstash/server/routes/cluster/load.ts | 39 +++++++++---------- .../plugins/logstash/server/routes/index.ts | 22 +++++------ .../logstash/server/routes/pipeline/delete.ts | 10 ++--- .../logstash/server/routes/pipeline/load.ts | 10 ++--- .../logstash/server/routes/pipeline/save.ts | 14 +++---- .../server/routes/pipelines/delete.ts | 10 ++--- .../logstash/server/routes/pipelines/list.ts | 10 ++--- .../logstash/server/routes/upgrade/execute.ts | 10 ++--- x-pack/plugins/logstash/server/types.ts | 11 +++++- 14 files changed, 102 insertions(+), 86 deletions(-) rename x-pack/plugins/licensing/server/{licensing_checker_route_handler_wrapper.test.ts => wrap_route_with_license_check.test.ts} (66%) rename x-pack/plugins/licensing/server/{license_checker_route_handler_wrapper.ts => wrap_route_with_license_check.ts} (94%) diff --git a/x-pack/plugins/licensing/server/index.ts b/x-pack/plugins/licensing/server/index.ts index 1f9545951381..76e65afc595c 100644 --- a/x-pack/plugins/licensing/server/index.ts +++ b/x-pack/plugins/licensing/server/index.ts @@ -12,7 +12,4 @@ export const plugin = (context: PluginInitializerContext) => new LicensingPlugin export * from '../common/types'; export * from './types'; export { config } from './licensing_config'; -export { - CheckLicense, - licenseCheckerRouteHandlerWrapper, -} from './license_checker_route_handler_wrapper'; +export { CheckLicense, wrapRouteWithLicenseCheck } from './wrap_route_with_license_check'; diff --git a/x-pack/plugins/licensing/server/licensing_checker_route_handler_wrapper.test.ts b/x-pack/plugins/licensing/server/wrap_route_with_license_check.test.ts similarity index 66% rename from x-pack/plugins/licensing/server/licensing_checker_route_handler_wrapper.test.ts rename to x-pack/plugins/licensing/server/wrap_route_with_license_check.test.ts index 7bd1cb4e2c88..7abdd3f6190c 100644 --- a/x-pack/plugins/licensing/server/licensing_checker_route_handler_wrapper.test.ts +++ b/x-pack/plugins/licensing/server/wrap_route_with_license_check.test.ts @@ -6,10 +6,7 @@ import { httpServerMock } from 'src/core/server/mocks'; -import { - licenseCheckerRouteHandlerWrapper, - CheckLicense, -} from './license_checker_route_handler_wrapper'; +import { wrapRouteWithLicenseCheck, CheckLicense } from './wrap_route_with_license_check'; const context = { licensing: { @@ -18,11 +15,11 @@ const context = { } as any; const request = httpServerMock.createKibanaRequest(); -describe('licenseCheckerRouteHandlerWrapper', () => { +describe('wrapRouteWithLicenseCheck', () => { it('calls route handler if checkLicense returns "valid": true', async () => { const checkLicense: CheckLicense = () => ({ valid: true, message: null }); const routeHandler = jest.fn(); - const wrapper = licenseCheckerRouteHandlerWrapper(checkLicense, routeHandler); + const wrapper = wrapRouteWithLicenseCheck(checkLicense, routeHandler); const response = httpServerMock.createResponseFactory(); await wrapper(context, request, response); @@ -34,7 +31,7 @@ describe('licenseCheckerRouteHandlerWrapper', () => { it('does not call route handler if checkLicense returns "valid": false', async () => { const checkLicense: CheckLicense = () => ({ valid: false, message: 'reason' }); const routeHandler = jest.fn(); - const wrapper = licenseCheckerRouteHandlerWrapper(checkLicense, routeHandler); + const wrapper = wrapRouteWithLicenseCheck(checkLicense, routeHandler); const response = httpServerMock.createResponseFactory(); await wrapper(context, request, response); @@ -49,11 +46,26 @@ describe('licenseCheckerRouteHandlerWrapper', () => { const routeHandler = () => { throw new Error('reason'); }; - const wrapper = licenseCheckerRouteHandlerWrapper(checkLicense, routeHandler); + const wrapper = wrapRouteWithLicenseCheck(checkLicense, routeHandler); const response = httpServerMock.createResponseFactory(); await expect(wrapper(context, request, response)).rejects.toThrowErrorMatchingInlineSnapshot( `"reason"` ); }); + + it('allows an exception to bubble up if "checkLicense" throws', async () => { + const checkLicense: CheckLicense = () => { + throw new Error('reason'); + }; + const routeHandler = jest.fn(); + const wrapper = wrapRouteWithLicenseCheck(checkLicense, routeHandler); + const response = httpServerMock.createResponseFactory(); + + await expect(wrapper(context, request, response)).rejects.toThrowErrorMatchingInlineSnapshot( + `"reason"` + ); + + expect(routeHandler).toHaveBeenCalledTimes(0); + }); }); diff --git a/x-pack/plugins/licensing/server/license_checker_route_handler_wrapper.ts b/x-pack/plugins/licensing/server/wrap_route_with_license_check.ts similarity index 94% rename from x-pack/plugins/licensing/server/license_checker_route_handler_wrapper.ts rename to x-pack/plugins/licensing/server/wrap_route_with_license_check.ts index 7f07a054c41b..e0cac8d9db20 100644 --- a/x-pack/plugins/licensing/server/license_checker_route_handler_wrapper.ts +++ b/x-pack/plugins/licensing/server/wrap_route_with_license_check.ts @@ -18,7 +18,7 @@ export type CheckLicense = ( license: ILicense ) => { valid: false; message: string } | { valid: true; message: null }; -export function licenseCheckerRouteHandlerWrapper( +export function wrapRouteWithLicenseCheck( checkLicense: CheckLicense, handler: RequestHandler ): RequestHandler { diff --git a/x-pack/plugins/logstash/server/lib/check_license/check_license.test.ts b/x-pack/plugins/logstash/server/lib/check_license/check_license.test.ts index 6e88b485c471..324e0a22ff37 100755 --- a/x-pack/plugins/logstash/server/lib/check_license/check_license.test.ts +++ b/x-pack/plugins/logstash/server/lib/check_license/check_license.test.ts @@ -7,7 +7,7 @@ import { licensingMock } from '../../../../licensing/server/mocks'; import { checkLicense } from './check_license'; describe('check_license', function() { - describe('returns "valid": false & message', () => { + describe('returns "valid": false & message when', () => { it('license information is not available', () => { const license = licensingMock.createLicenseMock(); license.isAvailable = false; @@ -48,6 +48,7 @@ describe('check_license', function() { expect(message).toStrictEqual(expect.any(String)); }); }); + it('returns "valid": true without message otherwise', () => { const license = licensingMock.createLicenseMock(); diff --git a/x-pack/plugins/logstash/server/plugin.ts b/x-pack/plugins/logstash/server/plugin.ts index be039427505e..c048dd13bfb0 100644 --- a/x-pack/plugins/logstash/server/plugin.ts +++ b/x-pack/plugins/logstash/server/plugin.ts @@ -5,6 +5,7 @@ */ import { CoreSetup, + CoreStart, ICustomClusterClient, Logger, Plugin, @@ -23,18 +24,25 @@ interface SetupDeps { export class LogstashPlugin implements Plugin { private readonly logger: Logger; private esClient?: ICustomClusterClient; + private coreSetup?: CoreSetup; constructor(context: PluginInitializerContext) { this.logger = context.logger.get(); } setup(core: CoreSetup, deps: SetupDeps) { this.logger.debug('Setting up Logstash plugin'); - const esClient = core.elasticsearch.createClient('logstash'); - registerRoutes(core.http.createRouter(), esClient, deps.security); + this.coreSetup = core; + registerRoutes(core.http.createRouter(), deps.security); } - start() {} + start(core: CoreStart) { + const esClient = core.elasticsearch.legacy.createClient('logstash'); + + this.coreSetup!.http.registerRouteHandlerContext('logstash', async (context, request) => { + return { esClient: esClient.asScoped(request) }; + }); + } stop() { if (this.esClient) { this.esClient.close(); diff --git a/x-pack/plugins/logstash/server/routes/cluster/load.ts b/x-pack/plugins/logstash/server/routes/cluster/load.ts index 19f207bb5ea6..18fe21f3da67 100644 --- a/x-pack/plugins/logstash/server/routes/cluster/load.ts +++ b/x-pack/plugins/logstash/server/routes/cluster/load.ts @@ -3,35 +3,32 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import { ICustomClusterClient, IRouter } from 'src/core/server'; -import { licenseCheckerRouteHandlerWrapper } from '../../../../licensing/server'; +import { IRouter } from 'src/core/server'; +import { wrapRouteWithLicenseCheck } from '../../../../licensing/server'; import { Cluster } from '../../models/cluster'; import { checkLicense } from '../../lib/check_license'; -export function registerClusterLoadRoute(router: IRouter, esClient: ICustomClusterClient) { +export function registerClusterLoadRoute(router: IRouter) { router.get( { path: '/api/logstash/cluster', validate: false, }, - licenseCheckerRouteHandlerWrapper( - checkLicense, - router.handleLegacyErrors(async (context, request, response) => { - try { - const client = esClient.asScoped(request); - const info = await client.callAsCurrentUser('info'); - return response.ok({ - body: { - cluster: Cluster.fromUpstreamJSON(info).downstreamJSON, - }, - }); - } catch (err) { - if (err.status === 403) { - return response.ok(); - } - return response.internalError(); + wrapRouteWithLicenseCheck(checkLicense, async (context, request, response) => { + try { + const client = context.logstash!.esClient; + const info = await client.callAsCurrentUser('info'); + return response.ok({ + body: { + cluster: Cluster.fromUpstreamJSON(info).downstreamJSON, + }, + }); + } catch (err) { + if (err.status === 403) { + return response.ok(); } - }) - ) + return response.internalError(); + } + }) ); } diff --git a/x-pack/plugins/logstash/server/routes/index.ts b/x-pack/plugins/logstash/server/routes/index.ts index 771ef83e49b2..0c7183b40905 100644 --- a/x-pack/plugins/logstash/server/routes/index.ts +++ b/x-pack/plugins/logstash/server/routes/index.ts @@ -3,7 +3,7 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import { IRouter, ICustomClusterClient } from 'src/core/server'; +import { IRouter } from 'src/core/server'; import { SecurityPluginSetup } from '../../../security/server'; import { registerClusterLoadRoute } from './cluster'; import { @@ -14,19 +14,15 @@ import { import { registerPipelinesListRoute, registerPipelinesDeleteRoute } from './pipelines'; import { registerUpgradeRoute } from './upgrade'; -export function registerRoutes( - router: IRouter, - esClient: ICustomClusterClient, - security?: SecurityPluginSetup -) { - registerClusterLoadRoute(router, esClient); +export function registerRoutes(router: IRouter, security?: SecurityPluginSetup) { + registerClusterLoadRoute(router); - registerPipelineDeleteRoute(router, esClient); - registerPipelineLoadRoute(router, esClient); - registerPipelineSaveRoute(router, esClient, security); + registerPipelineDeleteRoute(router); + registerPipelineLoadRoute(router); + registerPipelineSaveRoute(router, security); - registerPipelinesListRoute(router, esClient); - registerPipelinesDeleteRoute(router, esClient); + registerPipelinesListRoute(router); + registerPipelinesDeleteRoute(router); - registerUpgradeRoute(router, esClient); + registerUpgradeRoute(router); } diff --git a/x-pack/plugins/logstash/server/routes/pipeline/delete.ts b/x-pack/plugins/logstash/server/routes/pipeline/delete.ts index 73c1029dd327..4aeae3e0ae2d 100644 --- a/x-pack/plugins/logstash/server/routes/pipeline/delete.ts +++ b/x-pack/plugins/logstash/server/routes/pipeline/delete.ts @@ -4,13 +4,13 @@ * you may not use this file except in compliance with the Elastic License. */ import { schema } from '@kbn/config-schema'; -import { ICustomClusterClient, IRouter } from 'src/core/server'; +import { IRouter } from 'src/core/server'; import { INDEX_NAMES } from '../../../common/constants'; -import { licenseCheckerRouteHandlerWrapper } from '../../../../licensing/server'; +import { wrapRouteWithLicenseCheck } from '../../../../licensing/server'; import { checkLicense } from '../../lib/check_license'; -export function registerPipelineDeleteRoute(router: IRouter, esClient: ICustomClusterClient) { +export function registerPipelineDeleteRoute(router: IRouter) { router.delete( { path: '/api/logstash/pipeline/{id}', @@ -20,10 +20,10 @@ export function registerPipelineDeleteRoute(router: IRouter, esClient: ICustomCl }), }, }, - licenseCheckerRouteHandlerWrapper( + wrapRouteWithLicenseCheck( checkLicense, router.handleLegacyErrors(async (context, request, response) => { - const client = esClient.asScoped(request); + const client = context.logstash!.esClient; await client.callAsCurrentUser('delete', { index: INDEX_NAMES.PIPELINES, diff --git a/x-pack/plugins/logstash/server/routes/pipeline/load.ts b/x-pack/plugins/logstash/server/routes/pipeline/load.ts index cfc1a9286421..fec9097114d2 100644 --- a/x-pack/plugins/logstash/server/routes/pipeline/load.ts +++ b/x-pack/plugins/logstash/server/routes/pipeline/load.ts @@ -4,14 +4,14 @@ * you may not use this file except in compliance with the Elastic License. */ import { schema } from '@kbn/config-schema'; -import { ICustomClusterClient, IRouter } from 'src/core/server'; +import { IRouter } from 'src/core/server'; import { INDEX_NAMES } from '../../../common/constants'; import { Pipeline } from '../../models/pipeline'; -import { licenseCheckerRouteHandlerWrapper } from '../../../../licensing/server'; +import { wrapRouteWithLicenseCheck } from '../../../../licensing/server'; import { checkLicense } from '../../lib/check_license'; -export function registerPipelineLoadRoute(router: IRouter, esClient: ICustomClusterClient) { +export function registerPipelineLoadRoute(router: IRouter) { router.get( { path: '/api/logstash/pipeline/{id}', @@ -21,10 +21,10 @@ export function registerPipelineLoadRoute(router: IRouter, esClient: ICustomClus }), }, }, - licenseCheckerRouteHandlerWrapper( + wrapRouteWithLicenseCheck( checkLicense, router.handleLegacyErrors(async (context, request, response) => { - const client = esClient.asScoped(request); + const client = context.logstash!.esClient; const result = await client.callAsCurrentUser('get', { index: INDEX_NAMES.PIPELINES, diff --git a/x-pack/plugins/logstash/server/routes/pipeline/save.ts b/x-pack/plugins/logstash/server/routes/pipeline/save.ts index f5852d9489c8..556c281944a8 100644 --- a/x-pack/plugins/logstash/server/routes/pipeline/save.ts +++ b/x-pack/plugins/logstash/server/routes/pipeline/save.ts @@ -5,19 +5,15 @@ */ import { schema } from '@kbn/config-schema'; import { i18n } from '@kbn/i18n'; -import { ICustomClusterClient, IRouter } from 'src/core/server'; +import { IRouter } from 'src/core/server'; import { INDEX_NAMES } from '../../../common/constants'; import { Pipeline } from '../../models/pipeline'; -import { licenseCheckerRouteHandlerWrapper } from '../../../../licensing/server'; +import { wrapRouteWithLicenseCheck } from '../../../../licensing/server'; import { SecurityPluginSetup } from '../../../../security/server'; import { checkLicense } from '../../lib/check_license'; -export function registerPipelineSaveRoute( - router: IRouter, - esClient: ICustomClusterClient, - security?: SecurityPluginSetup -) { +export function registerPipelineSaveRoute(router: IRouter, security?: SecurityPluginSetup) { router.put( { path: '/api/logstash/pipeline/{id}', @@ -34,7 +30,7 @@ export function registerPipelineSaveRoute( }), }, }, - licenseCheckerRouteHandlerWrapper( + wrapRouteWithLicenseCheck( checkLicense, router.handleLegacyErrors(async (context, request, response) => { try { @@ -44,7 +40,7 @@ export function registerPipelineSaveRoute( username = user?.username; } - const client = esClient.asScoped(request); + const client = context.logstash!.esClient; const pipeline = Pipeline.fromDownstreamJSON(request.body, request.params.id, username); await client.callAsCurrentUser('index', { diff --git a/x-pack/plugins/logstash/server/routes/pipelines/delete.ts b/x-pack/plugins/logstash/server/routes/pipelines/delete.ts index 5e2e6ef3d77c..ac3097ac0424 100644 --- a/x-pack/plugins/logstash/server/routes/pipelines/delete.ts +++ b/x-pack/plugins/logstash/server/routes/pipelines/delete.ts @@ -4,8 +4,8 @@ * you may not use this file except in compliance with the Elastic License. */ import { schema } from '@kbn/config-schema'; -import { APICaller, ICustomClusterClient, IRouter } from 'src/core/server'; -import { licenseCheckerRouteHandlerWrapper } from '../../../../licensing/server'; +import { APICaller, IRouter } from 'src/core/server'; +import { wrapRouteWithLicenseCheck } from '../../../../licensing/server'; import { INDEX_NAMES } from '../../../common/constants'; import { checkLicense } from '../../lib/check_license'; @@ -31,7 +31,7 @@ async function deletePipelines(callWithRequest: APICaller, pipelineIds: string[] }; } -export function registerPipelinesDeleteRoute(router: IRouter, esClient: ICustomClusterClient) { +export function registerPipelinesDeleteRoute(router: IRouter) { router.post( { path: '/api/logstash/pipelines/delete', @@ -41,10 +41,10 @@ export function registerPipelinesDeleteRoute(router: IRouter, esClient: ICustomC }), }, }, - licenseCheckerRouteHandlerWrapper( + wrapRouteWithLicenseCheck( checkLicense, router.handleLegacyErrors(async (context, request, response) => { - const client = esClient.asScoped(request); + const client = context.logstash!.esClient; const results = await deletePipelines(client.callAsCurrentUser, request.body.pipelineIds); return response.ok({ body: { results } }); diff --git a/x-pack/plugins/logstash/server/routes/pipelines/list.ts b/x-pack/plugins/logstash/server/routes/pipelines/list.ts index eaa4a49b93ae..bc477a25a798 100644 --- a/x-pack/plugins/logstash/server/routes/pipelines/list.ts +++ b/x-pack/plugins/logstash/server/routes/pipelines/list.ts @@ -5,8 +5,8 @@ */ import { i18n } from '@kbn/i18n'; import { SearchResponse } from 'elasticsearch'; -import { APICaller, ICustomClusterClient, IRouter } from 'src/core/server'; -import { licenseCheckerRouteHandlerWrapper } from '../../../../licensing/server'; +import { APICaller, IRouter } from 'src/core/server'; +import { wrapRouteWithLicenseCheck } from '../../../../licensing/server'; import { INDEX_NAMES, ES_SCROLL_SETTINGS } from '../../../common/constants'; import { PipelineListItem } from '../../models/pipeline_list_item'; @@ -27,17 +27,17 @@ async function fetchPipelines(callWithRequest: APICaller) { return fetchAllFromScroll(response, callWithRequest); } -export function registerPipelinesListRoute(router: IRouter, esClient: ICustomClusterClient) { +export function registerPipelinesListRoute(router: IRouter) { router.get( { path: '/api/logstash/pipelines', validate: false, }, - licenseCheckerRouteHandlerWrapper( + wrapRouteWithLicenseCheck( checkLicense, router.handleLegacyErrors(async (context, request, response) => { try { - const client = esClient.asScoped(request); + const client = context.logstash!.esClient; const pipelinesHits = await fetchPipelines(client.callAsCurrentUser); const pipelines = pipelinesHits.map(pipeline => { diff --git a/x-pack/plugins/logstash/server/routes/upgrade/execute.ts b/x-pack/plugins/logstash/server/routes/upgrade/execute.ts index e844830379f8..2bd2c0f89e19 100644 --- a/x-pack/plugins/logstash/server/routes/upgrade/execute.ts +++ b/x-pack/plugins/logstash/server/routes/upgrade/execute.ts @@ -3,22 +3,22 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import { ICustomClusterClient, IRouter } from 'src/core/server'; -import { licenseCheckerRouteHandlerWrapper } from '../../../../licensing/server'; +import { IRouter } from 'src/core/server'; +import { wrapRouteWithLicenseCheck } from '../../../../licensing/server'; import { INDEX_NAMES } from '../../../common/constants'; import { checkLicense } from '../../lib/check_license'; -export function registerUpgradeRoute(router: IRouter, esClient: ICustomClusterClient) { +export function registerUpgradeRoute(router: IRouter) { router.post( { path: '/api/logstash/upgrade', validate: false, }, - licenseCheckerRouteHandlerWrapper( + wrapRouteWithLicenseCheck( checkLicense, router.handleLegacyErrors(async (context, request, response) => { - const client = esClient.asScoped(request); + const client = context.logstash!.esClient; const doesIndexExist = await client.callAsCurrentUser('indices.exists', { index: INDEX_NAMES.PIPELINES, diff --git a/x-pack/plugins/logstash/server/types.ts b/x-pack/plugins/logstash/server/types.ts index 63647a07826d..2b266b2f2770 100644 --- a/x-pack/plugins/logstash/server/types.ts +++ b/x-pack/plugins/logstash/server/types.ts @@ -4,8 +4,9 @@ * you may not use this file except in compliance with the Elastic License. */ import { SearchResponse } from 'elasticsearch'; +import { IScopedClusterClient } from 'src/core/server'; -type UnwrapArray = T extends Array ? U : T; +type UnwrapArray = T extends Array ? U : never; export type Hits = SearchResponse['hits']['hits']; export type Hit = UnwrapArray; @@ -16,3 +17,11 @@ export interface PipelineListItemOptions { last_modified: string; username: string; } + +declare module 'src/core/server' { + interface RequestHandlerContext { + logstash?: { + esClient: IScopedClusterClient; + }; + } +}