From f4062331620350d0a33099184122dc9a28ffa671 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 30 Jan 2020 10:49:13 -0500 Subject: [PATCH 1/7] Fix ILM policy creation --- .../event_log/server/es/documents.test.ts | 6 ++--- .../plugins/event_log/server/es/documents.ts | 7 +---- x-pack/plugins/event_log/server/es/init.ts | 26 +++++-------------- x-pack/plugins/event_log/server/types.ts | 2 +- 4 files changed, 12 insertions(+), 29 deletions(-) diff --git a/x-pack/plugins/event_log/server/es/documents.test.ts b/x-pack/plugins/event_log/server/es/documents.test.ts index 2dec23c61de2f8..725365c0bca7a6 100644 --- a/x-pack/plugins/event_log/server/es/documents.test.ts +++ b/x-pack/plugins/event_log/server/es/documents.test.ts @@ -21,7 +21,7 @@ describe('getIndexTemplate()', () => { const esNames = getEsNames('XYZ'); test('returns the correct details of the index template', () => { - const indexTemplate = getIndexTemplate(esNames, true); + const indexTemplate = getIndexTemplate(esNames); expect(indexTemplate.index_patterns).toEqual([esNames.indexPattern]); expect(indexTemplate.aliases[esNames.alias]).toEqual({}); expect(indexTemplate.settings.number_of_shards).toBeGreaterThanOrEqual(0); @@ -30,13 +30,13 @@ describe('getIndexTemplate()', () => { }); test('returns correct index template bits for ilm when ilm is supported', () => { - const indexTemplate = getIndexTemplate(esNames, true); + const indexTemplate = getIndexTemplate(esNames); expect(indexTemplate.settings['index.lifecycle.name']).toBe(esNames.ilmPolicy); expect(indexTemplate.settings['index.lifecycle.rollover_alias']).toBe(esNames.alias); }); test('returns correct index template bits for ilm when ilm is not supported', () => { - const indexTemplate = getIndexTemplate(esNames, false); + const indexTemplate = getIndexTemplate(esNames); expect(indexTemplate.settings['index.lifecycle.name']).toBeUndefined(); expect(indexTemplate.settings['index.lifecycle.rollover_alias']).toBeUndefined(); }); diff --git a/x-pack/plugins/event_log/server/es/documents.ts b/x-pack/plugins/event_log/server/es/documents.ts index dfc544f8a41cbf..09dd7383c4c5e3 100644 --- a/x-pack/plugins/event_log/server/es/documents.ts +++ b/x-pack/plugins/event_log/server/es/documents.ts @@ -8,7 +8,7 @@ import { EsNames } from './names'; import mappings from '../../generated/mappings.json'; // returns the body of an index template used in an ES indices.putTemplate call -export function getIndexTemplate(esNames: EsNames, ilmExists: boolean) { +export function getIndexTemplate(esNames: EsNames) { const indexTemplateBody: any = { index_patterns: [esNames.indexPattern], aliases: { @@ -23,11 +23,6 @@ export function getIndexTemplate(esNames: EsNames, ilmExists: boolean) { mappings, }; - if (!ilmExists) { - delete indexTemplateBody.settings['index.lifecycle.name']; - delete indexTemplateBody.settings['index.lifecycle.rollover_alias']; - } - return indexTemplateBody; } diff --git a/x-pack/plugins/event_log/server/es/init.ts b/x-pack/plugins/event_log/server/es/init.ts index d87f5bce034757..f5ebb07abb3f89 100644 --- a/x-pack/plugins/event_log/server/es/init.ts +++ b/x-pack/plugins/event_log/server/es/init.ts @@ -23,16 +23,14 @@ export async function initializeEs(esContext: EsContext): Promise { async function initializeEsResources(esContext: EsContext) { const steps = new EsInitializationSteps(esContext); - let ilmExists: boolean; - // create the ilm policy, if required - ilmExists = await steps.doesIlmPolicyExist(); - if (!ilmExists) { - ilmExists = await steps.createIlmPolicy(); + // create the ilm policy, if it doesn't exist + if (!(await steps.doesIlmPolicyExist())) { + await steps.createIlmPolicy(); } if (!(await steps.doesIndexTemplateExist())) { - await steps.createIndexTemplate({ ilmExists }); + await steps.createIndexTemplate(); } if (!(await steps.doesInitialIndexExist())) { @@ -40,10 +38,6 @@ async function initializeEsResources(esContext: EsContext) { } } -interface AddTemplateOpts { - ilmExists: boolean; -} - class EsInitializationSteps { constructor(private readonly esContext: EsContext) { this.esContext = esContext; @@ -58,15 +52,12 @@ class EsInitializationSteps { await this.esContext.callEs('transport.request', request); } catch (err) { if (err.statusCode === 404) return false; - // TODO: remove following once kibana user can access ilm - if (err.statusCode === 403) return false; - throw new Error(`error checking existance of ilm policy: ${err.message}`); } return true; } - async createIlmPolicy(): Promise { + async createIlmPolicy(): Promise { const request = { method: 'PUT', path: `_ilm/policy/${this.esContext.esNames.ilmPolicy}`, @@ -75,11 +66,8 @@ class EsInitializationSteps { try { await this.esContext.callEs('transport.request', request); } catch (err) { - // TODO: remove following once kibana user can access ilm - if (err.statusCode === 403) return false; throw new Error(`error creating ilm policy: ${err.message}`); } - return true; } async doesIndexTemplateExist(): Promise { @@ -93,8 +81,8 @@ class EsInitializationSteps { return result as boolean; } - async createIndexTemplate(opts: AddTemplateOpts): Promise { - const templateBody = getIndexTemplate(this.esContext.esNames, opts.ilmExists); + async createIndexTemplate(): Promise { + const templateBody = getIndexTemplate(this.esContext.esNames); const addTemplateParams = { create: true, name: this.esContext.esNames.indexTemplate, diff --git a/x-pack/plugins/event_log/server/types.ts b/x-pack/plugins/event_log/server/types.ts index 4cace59752f366..f606bb2be6c6c1 100644 --- a/x-pack/plugins/event_log/server/types.ts +++ b/x-pack/plugins/event_log/server/types.ts @@ -13,7 +13,7 @@ import { IEvent } from '../generated/schemas'; export const ConfigSchema = schema.object({ enabled: schema.boolean({ defaultValue: true }), logEntries: schema.boolean({ defaultValue: false }), - indexEntries: schema.boolean({ defaultValue: false }), + indexEntries: schema.boolean({ defaultValue: true }), }); export type IEventLogConfig = TypeOf; From 23707bbaca10431b97c20028ef19dc888c72639c Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Fri, 7 Feb 2020 09:57:42 -0500 Subject: [PATCH 2/7] Handle errors thrown in scenario multiple Kibana instances are started at the same time --- x-pack/plugins/event_log/server/es/init.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/event_log/server/es/init.ts b/x-pack/plugins/event_log/server/es/init.ts index f5ebb07abb3f89..ce99f125bbafaf 100644 --- a/x-pack/plugins/event_log/server/es/init.ts +++ b/x-pack/plugins/event_log/server/es/init.ts @@ -91,7 +91,14 @@ class EsInitializationSteps { try { await this.esContext.callEs('indices.putTemplate', addTemplateParams); } catch (err) { - throw new Error(`error creating index template: ${err.message}`); + // The error message doesn't have a type attribute we can look to guarantee it's due + // to the template already existing (only long message) so we'll check ourselves to see + // if the template now exists. This scenario would happen if you startup multiple Kibana + // instances at the same time. + const existsNow = await this.doesIndexTemplateExist(); + if (!existsNow) { + throw new Error(`error creating index template: ${err.message}`); + } } } @@ -111,7 +118,9 @@ class EsInitializationSteps { try { await this.esContext.callEs('indices.create', { index }); } catch (err) { - throw new Error(`error creating initial index: ${err.message}`); + if (err.body?.error?.type !== 'resource_already_exists_exception') { + throw new Error(`error creating initial index: ${err.message}`); + } } } From 28b8d12d436e27258f20a002b2b4ea1c45e73a8d Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 10 Feb 2020 09:25:31 -0500 Subject: [PATCH 3/7] Fix tests and cleanup --- x-pack/plugins/event_log/server/es/documents.test.ts | 12 +----------- x-pack/plugins/event_log/server/types.ts | 2 +- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/event_log/server/es/documents.test.ts b/x-pack/plugins/event_log/server/es/documents.test.ts index 725365c0bca7a6..7edca4b3943a65 100644 --- a/x-pack/plugins/event_log/server/es/documents.test.ts +++ b/x-pack/plugins/event_log/server/es/documents.test.ts @@ -26,18 +26,8 @@ describe('getIndexTemplate()', () => { expect(indexTemplate.aliases[esNames.alias]).toEqual({}); expect(indexTemplate.settings.number_of_shards).toBeGreaterThanOrEqual(0); expect(indexTemplate.settings.number_of_replicas).toBeGreaterThanOrEqual(0); - expect(indexTemplate.mappings).toMatchObject({}); - }); - - test('returns correct index template bits for ilm when ilm is supported', () => { - const indexTemplate = getIndexTemplate(esNames); expect(indexTemplate.settings['index.lifecycle.name']).toBe(esNames.ilmPolicy); expect(indexTemplate.settings['index.lifecycle.rollover_alias']).toBe(esNames.alias); - }); - - test('returns correct index template bits for ilm when ilm is not supported', () => { - const indexTemplate = getIndexTemplate(esNames); - expect(indexTemplate.settings['index.lifecycle.name']).toBeUndefined(); - expect(indexTemplate.settings['index.lifecycle.rollover_alias']).toBeUndefined(); + expect(indexTemplate.mappings).toMatchObject({}); }); }); diff --git a/x-pack/plugins/event_log/server/types.ts b/x-pack/plugins/event_log/server/types.ts index f606bb2be6c6c1..4cace59752f366 100644 --- a/x-pack/plugins/event_log/server/types.ts +++ b/x-pack/plugins/event_log/server/types.ts @@ -13,7 +13,7 @@ import { IEvent } from '../generated/schemas'; export const ConfigSchema = schema.object({ enabled: schema.boolean({ defaultValue: true }), logEntries: schema.boolean({ defaultValue: false }), - indexEntries: schema.boolean({ defaultValue: true }), + indexEntries: schema.boolean({ defaultValue: false }), }); export type IEventLogConfig = TypeOf; From 2f24268272525fbef4d7081d3226094352ce1eaa Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 10 Feb 2020 10:56:01 -0500 Subject: [PATCH 4/7] Start adding tests --- .../event_log/server/es/context.mock.ts | 56 ++----- .../plugins/event_log/server/es/init.test.ts | 158 ++++++++++++++++++ .../plugins/event_log/server/es/names.mock.ts | 23 +++ 3 files changed, 198 insertions(+), 39 deletions(-) create mode 100644 x-pack/plugins/event_log/server/es/init.test.ts create mode 100644 x-pack/plugins/event_log/server/es/names.mock.ts diff --git a/x-pack/plugins/event_log/server/es/context.mock.ts b/x-pack/plugins/event_log/server/es/context.mock.ts index fb894ce6e77875..41d6a7a0962b8e 100644 --- a/x-pack/plugins/event_log/server/es/context.mock.ts +++ b/x-pack/plugins/event_log/server/es/context.mock.ts @@ -4,43 +4,21 @@ * you may not use this file except in compliance with the Elastic License. */ -import { Logger, ClusterClient } from '../../../../../src/core/server'; import { EsContext } from './context'; - -import { EsNames } from './names'; - -export type EsClusterClient = Pick; - -export interface EsError { - readonly statusCode: number; - readonly message: string; -} - -interface CreateMockEsContextParams { - logger: Logger; - esNames: EsNames; -} - -export function createMockEsContext(params: CreateMockEsContextParams): EsContext { - return new EsContextMock(params); -} - -class EsContextMock implements EsContext { - public logger: Logger; - public esNames: EsNames; - - constructor(params: CreateMockEsContextParams) { - this.logger = params.logger; - this.esNames = params.esNames; - } - - initialize() {} - - async waitTillReady(): Promise { - return true; - } - - async callEs(operation: string, body?: any): Promise { - return {}; - } -} +import { namesMock } from './names.mock'; +import { loggingServiceMock } from '../../../../../src/core/server/mocks'; + +const createContextMock = () => { + const mock: jest.Mocked = { + logger: loggingServiceMock.createLogger(), + esNames: namesMock.create(), + initialize: jest.fn(), + waitTillReady: jest.fn(), + callEs: jest.fn(), + }; + return mock; +}; + +export const contextMock = { + create: createContextMock, +}; diff --git a/x-pack/plugins/event_log/server/es/init.test.ts b/x-pack/plugins/event_log/server/es/init.test.ts new file mode 100644 index 00000000000000..87ebc94ecda7e0 --- /dev/null +++ b/x-pack/plugins/event_log/server/es/init.test.ts @@ -0,0 +1,158 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { contextMock } from './context.mock'; +import { initializeEs } from './init'; + +describe('initializeEs', () => { + const esContext = contextMock.create(); + const createIndexBody = { + index: '.kibana-event-log-000001', + }; + const createIndexTemplateBody = { + create: true, + name: '.kibana-event-log-template', + body: expect.anything(), + }; + const createIlmRequestBody = { + method: 'PUT', + path: '_ilm/policy/.kibana-event-log-policy', + body: { + policy: { + phases: { + hot: { + actions: { + rollover: { + max_age: '30d', + max_size: '5GB', + }, + }, + }, + }, + }, + }, + }; + + beforeEach(() => jest.resetAllMocks()); + + test('should initialize elasticsearch', async () => { + esContext.callEs.mockImplementation( + getCallEsImplementation({ + ilmPolicyExists: false, + indexTemplateExists: false, + initialIndexExists: false, + }) + ); + + await initializeEs(esContext); + expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmRequestBody); + expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateBody); + expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexBody); + }); + + test('should skip initializing ILM policy when it already exists', async () => { + esContext.callEs.mockImplementation( + getCallEsImplementation({ + ilmPolicyExists: true, + indexTemplateExists: false, + initialIndexExists: false, + }) + ); + + await initializeEs(esContext); + expect(esContext.callEs).not.toHaveBeenCalledWith('transport.request', createIlmRequestBody); + expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateBody); + expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexBody); + }); + + test('should skip creating index template when it already exists', async () => { + esContext.callEs.mockImplementation( + getCallEsImplementation({ + ilmPolicyExists: false, + indexTemplateExists: true, + initialIndexExists: false, + }) + ); + + await initializeEs(esContext); + expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmRequestBody); + expect(esContext.callEs).not.toHaveBeenCalledWith( + 'indices.putTemplate', + createIndexTemplateBody + ); + expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexBody); + }); + + test('should skip creating initial index when it already exists', async () => { + esContext.callEs.mockImplementation( + getCallEsImplementation({ + ilmPolicyExists: false, + indexTemplateExists: false, + initialIndexExists: true, + }) + ); + + await initializeEs(esContext); + expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmRequestBody); + expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateBody); + expect(esContext.callEs).not.toHaveBeenCalledWith('indices.create', createIndexBody); + }); + + test('should double check if index template exists when error is thrown during creation request', async () => { + esContext.callEs.mockImplementation( + getCallEsImplementation({ + ilmPolicyExists: false, + indexTemplateExists: false, + initialIndexExists: true, + }) + ); + + await initializeEs(esContext); + expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmRequestBody); + expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateBody); + expect(esContext.callEs).not.toHaveBeenCalledWith('indices.create', createIndexBody); + }); +}); + +function throwNotFoundError() { + const err = new Error('Not found') as any; + err.statusCode = 404; + throw err; +} + +function getCallEsImplementation({ + ilmPolicyExists, + indexTemplateExists, + initialIndexExists, +}: { + ilmPolicyExists: boolean; + indexTemplateExists: boolean; + initialIndexExists: boolean; +}) { + return async (operation: string, body?: any) => { + // Make ILM policy exist check return false + if ( + operation === 'transport.request' && + body?.method === 'GET' && + body?.path === '_ilm/policy/.kibana-event-log-policy' + ) { + if (!ilmPolicyExists) { + throwNotFoundError(); + } + return {}; + } + + // Make index template exists check return false + if (operation === 'indices.existsTemplate' && body?.name === '.kibana-event-log-template') { + return indexTemplateExists; + } + + // Make alias exists check return false + if (operation === 'indices.existsAlias' && body?.name === '.kibana-event-log') { + return initialIndexExists; + } + }; +} diff --git a/x-pack/plugins/event_log/server/es/names.mock.ts b/x-pack/plugins/event_log/server/es/names.mock.ts new file mode 100644 index 00000000000000..7b013a0d263da8 --- /dev/null +++ b/x-pack/plugins/event_log/server/es/names.mock.ts @@ -0,0 +1,23 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { EsNames } from './names'; + +const createNamesMock = () => { + const mock: jest.Mocked = { + base: '.kibana', + alias: '.kibana-event-log', + ilmPolicy: '.kibana-event-log-policy', + indexPattern: '.kibana-event-log-*', + initialIndex: '.kibana-event-log-000001', + indexTemplate: '.kibana-event-log-template', + }; + return mock; +}; + +export const namesMock = { + create: createNamesMock, +}; From a818cfc7a6ec682e7259f9f1067e9038f957cefc Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Wed, 12 Feb 2020 09:31:46 -0500 Subject: [PATCH 5/7] Refactor tests, add index template failure test --- .../plugins/event_log/server/es/init.test.ts | 69 +++++++++++++------ 1 file changed, 49 insertions(+), 20 deletions(-) diff --git a/x-pack/plugins/event_log/server/es/init.test.ts b/x-pack/plugins/event_log/server/es/init.test.ts index 87ebc94ecda7e0..ace20846165d0d 100644 --- a/x-pack/plugins/event_log/server/es/init.test.ts +++ b/x-pack/plugins/event_log/server/es/init.test.ts @@ -9,15 +9,22 @@ import { initializeEs } from './init'; describe('initializeEs', () => { const esContext = contextMock.create(); - const createIndexBody = { + const createIndexParams = { index: '.kibana-event-log-000001', }; - const createIndexTemplateBody = { + const getIlmPolicyParams = { + method: 'GET', + path: '_ilm/policy/.kibana-event-log-policy', + }; + const indexTemplateExistsParams = { + name: '.kibana-event-log-template', + }; + const createIndexTemplateParams = { create: true, name: '.kibana-event-log-template', body: expect.anything(), }; - const createIlmRequestBody = { + const createIlmParams = { method: 'PUT', path: '_ilm/policy/.kibana-event-log-policy', body: { @@ -48,9 +55,9 @@ describe('initializeEs', () => { ); await initializeEs(esContext); - expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmRequestBody); - expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateBody); - expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexBody); + expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmParams); + expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateParams); + expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexParams); }); test('should skip initializing ILM policy when it already exists', async () => { @@ -63,9 +70,9 @@ describe('initializeEs', () => { ); await initializeEs(esContext); - expect(esContext.callEs).not.toHaveBeenCalledWith('transport.request', createIlmRequestBody); - expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateBody); - expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexBody); + expect(esContext.callEs).not.toHaveBeenCalledWith('transport.request', createIlmParams); + expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateParams); + expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexParams); }); test('should skip creating index template when it already exists', async () => { @@ -78,12 +85,12 @@ describe('initializeEs', () => { ); await initializeEs(esContext); - expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmRequestBody); + expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmParams); expect(esContext.callEs).not.toHaveBeenCalledWith( 'indices.putTemplate', - createIndexTemplateBody + createIndexTemplateParams ); - expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexBody); + expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexParams); }); test('should skip creating initial index when it already exists', async () => { @@ -96,24 +103,40 @@ describe('initializeEs', () => { ); await initializeEs(esContext); - expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmRequestBody); - expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateBody); - expect(esContext.callEs).not.toHaveBeenCalledWith('indices.create', createIndexBody); + expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmParams); + expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateParams); + expect(esContext.callEs).not.toHaveBeenCalledWith('indices.create', createIndexParams); }); - test('should double check if index template exists when error is thrown during creation request', async () => { + test('should double check if index template exists before logging error', async () => { esContext.callEs.mockImplementation( getCallEsImplementation({ ilmPolicyExists: false, indexTemplateExists: false, - initialIndexExists: true, + initialIndexExists: false, + throwErrorWhenCreatingIndexTemplate: true, }) ); await initializeEs(esContext); - expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmRequestBody); - expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateBody); - expect(esContext.callEs).not.toHaveBeenCalledWith('indices.create', createIndexBody); + expect(esContext.callEs).toHaveBeenCalledTimes(5); + expect(esContext.callEs).toHaveBeenNthCalledWith(1, 'transport.request', getIlmPolicyParams); + expect(esContext.callEs).toHaveBeenNthCalledWith(2, 'transport.request', createIlmParams); + expect(esContext.callEs).toHaveBeenNthCalledWith( + 3, + 'indices.existsTemplate', + indexTemplateExistsParams + ); + expect(esContext.callEs).toHaveBeenNthCalledWith( + 4, + 'indices.putTemplate', + createIndexTemplateParams + ); + expect(esContext.callEs).toHaveBeenNthCalledWith( + 5, + 'indices.existsTemplate', + indexTemplateExistsParams + ); }); }); @@ -127,10 +150,12 @@ function getCallEsImplementation({ ilmPolicyExists, indexTemplateExists, initialIndexExists, + throwErrorWhenCreatingIndexTemplate = false, }: { ilmPolicyExists: boolean; indexTemplateExists: boolean; initialIndexExists: boolean; + throwErrorWhenCreatingIndexTemplate?: boolean; }) { return async (operation: string, body?: any) => { // Make ILM policy exist check return false @@ -154,5 +179,9 @@ function getCallEsImplementation({ if (operation === 'indices.existsAlias' && body?.name === '.kibana-event-log') { return initialIndexExists; } + + if (operation === 'indices.putTemplate' && throwErrorWhenCreatingIndexTemplate) { + throw new Error('Fail'); + } }; } From ee4393c684953310a7681ae6408a279a4b190702 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Wed, 12 Feb 2020 11:32:17 -0500 Subject: [PATCH 6/7] Create cluster client adapter to facilitate testing and isolation --- .../server/es/cluster_client_adapter.mock.ts | 23 +++ .../server/es/cluster_client_adapter.test.ts | 180 +++++++++++++++++ .../server/es/cluster_client_adapter.ts | 122 ++++++++++++ .../event_log/server/es/context.mock.ts | 8 +- x-pack/plugins/event_log/server/es/context.ts | 30 +-- .../plugins/event_log/server/es/init.test.ts | 185 +++--------------- x-pack/plugins/event_log/server/es/init.ts | 118 +++-------- 7 files changed, 395 insertions(+), 271 deletions(-) create mode 100644 x-pack/plugins/event_log/server/es/cluster_client_adapter.mock.ts create mode 100644 x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts create mode 100644 x-pack/plugins/event_log/server/es/cluster_client_adapter.ts diff --git a/x-pack/plugins/event_log/server/es/cluster_client_adapter.mock.ts b/x-pack/plugins/event_log/server/es/cluster_client_adapter.mock.ts new file mode 100644 index 00000000000000..2b210f67f4ea5f --- /dev/null +++ b/x-pack/plugins/event_log/server/es/cluster_client_adapter.mock.ts @@ -0,0 +1,23 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { IClusterClientAdapter } from './cluster_client_adapter'; + +const createClusterClientMock = () => { + const mock: jest.Mocked = { + doesIlmPolicyExist: jest.fn(), + createIlmPolicy: jest.fn(), + doesIndexTemplateExist: jest.fn(), + createIndexTemplate: jest.fn(), + doesAliasExist: jest.fn(), + createIndex: jest.fn(), + }; + return mock; +}; + +export const clusterClientAdapterMock = { + create: createClusterClientMock, +}; diff --git a/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts b/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts new file mode 100644 index 00000000000000..c4ef62d81d3a32 --- /dev/null +++ b/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts @@ -0,0 +1,180 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { ClusterClient, Logger } from '../../../../../src/core/server'; +import { elasticsearchServiceMock, loggingServiceMock } from '../../../../../src/core/server/mocks'; +import { ClusterClientAdapter, IClusterClientAdapter } from './cluster_client_adapter'; + +type EsClusterClient = Pick, 'callAsInternalUser' | 'asScoped'>; + +let logger: Logger; +let clusterClient: EsClusterClient; +let clusterClientAdapter: IClusterClientAdapter; + +beforeEach(() => { + logger = loggingServiceMock.createLogger(); + clusterClient = elasticsearchServiceMock.createClusterClient(); + clusterClientAdapter = new ClusterClientAdapter({ + logger, + clusterClient, + }); +}); + +describe('doesIlmPolicyExist', () => { + const notFoundError = new Error('Not found') as any; + notFoundError.statusCode = 404; + + test('should call cluster with proper arguments', async () => { + await clusterClientAdapter.doesIlmPolicyExist('foo'); + expect(clusterClient.callAsInternalUser).toHaveBeenCalledWith('transport.request', { + method: 'GET', + path: '_ilm/policy/foo', + }); + }); + + test('should return false when 404 error is returned by Elasticsearch', async () => { + clusterClient.callAsInternalUser.mockRejectedValue(notFoundError); + await expect(clusterClientAdapter.doesIlmPolicyExist('foo')).resolves.toEqual(false); + }); + + test('should throw error when error is not 404', async () => { + clusterClient.callAsInternalUser.mockRejectedValue(new Error('Fail')); + await expect( + clusterClientAdapter.doesIlmPolicyExist('foo') + ).rejects.toThrowErrorMatchingInlineSnapshot(`"error checking existance of ilm policy: Fail"`); + }); + + test('should return true when no error is thrown', async () => { + await expect(clusterClientAdapter.doesIlmPolicyExist('foo')).resolves.toEqual(true); + }); +}); + +describe('createIlmPolicy', () => { + test('should call cluster client with given policy', async () => { + clusterClient.callAsInternalUser.mockResolvedValue({ success: true }); + await clusterClientAdapter.createIlmPolicy('foo', { args: true }); + expect(clusterClient.callAsInternalUser).toHaveBeenCalledWith('transport.request', { + method: 'PUT', + path: '_ilm/policy/foo', + body: { args: true }, + }); + }); + + test('should throw error when call cluster client throws', async () => { + clusterClient.callAsInternalUser.mockRejectedValue(new Error('Fail')); + await expect( + clusterClientAdapter.createIlmPolicy('foo', { args: true }) + ).rejects.toThrowErrorMatchingInlineSnapshot(`"error creating ilm policy: Fail"`); + }); +}); + +describe('doesIndexTemplateExist', () => { + test('should call cluster with proper arguments', async () => { + await clusterClientAdapter.doesIndexTemplateExist('foo'); + expect(clusterClient.callAsInternalUser).toHaveBeenCalledWith('indices.existsTemplate', { + name: 'foo', + }); + }); + + test('should return true when call cluster returns true', async () => { + clusterClient.callAsInternalUser.mockResolvedValue(true); + await expect(clusterClientAdapter.doesIndexTemplateExist('foo')).resolves.toEqual(true); + }); + + test('should return false when call cluster returns false', async () => { + clusterClient.callAsInternalUser.mockResolvedValue(false); + await expect(clusterClientAdapter.doesIndexTemplateExist('foo')).resolves.toEqual(false); + }); + + test('should throw error when call cluster throws an error', async () => { + clusterClient.callAsInternalUser.mockRejectedValue(new Error('Fail')); + await expect( + clusterClientAdapter.doesIndexTemplateExist('foo') + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"error checking existance of index template: Fail"` + ); + }); +}); + +describe('createIndexTemplate', () => { + test('should call cluster with given template', async () => { + await clusterClientAdapter.createIndexTemplate('foo', { args: true }); + expect(clusterClient.callAsInternalUser).toHaveBeenCalledWith('indices.putTemplate', { + name: 'foo', + create: true, + body: { args: true }, + }); + }); + + test(`should throw error if index template still doesn't exist after error is thrown`, async () => { + clusterClient.callAsInternalUser.mockRejectedValueOnce(new Error('Fail')); + clusterClient.callAsInternalUser.mockResolvedValueOnce(false); + await expect( + clusterClientAdapter.createIndexTemplate('foo', { args: true }) + ).rejects.toThrowErrorMatchingInlineSnapshot(`"error creating index template: Fail"`); + }); + + test('should not throw error if index template exists after error is thrown', async () => { + clusterClient.callAsInternalUser.mockRejectedValueOnce(new Error('Fail')); + clusterClient.callAsInternalUser.mockResolvedValueOnce(true); + await clusterClientAdapter.createIndexTemplate('foo', { args: true }); + }); +}); + +describe('doesAliasExist', () => { + test('should call cluster with proper arguments', async () => { + await clusterClientAdapter.doesAliasExist('foo'); + expect(clusterClient.callAsInternalUser).toHaveBeenCalledWith('indices.existsAlias', { + name: 'foo', + }); + }); + + test('should return true when call cluster returns true', async () => { + clusterClient.callAsInternalUser.mockResolvedValueOnce(true); + await expect(clusterClientAdapter.doesAliasExist('foo')).resolves.toEqual(true); + }); + + test('should return false when call cluster returns false', async () => { + clusterClient.callAsInternalUser.mockResolvedValueOnce(false); + await expect(clusterClientAdapter.doesAliasExist('foo')).resolves.toEqual(false); + }); + + test('should throw error when call cluster throws an error', async () => { + clusterClient.callAsInternalUser.mockRejectedValue(new Error('Fail')); + await expect( + clusterClientAdapter.doesAliasExist('foo') + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"error checking existance of initial index: Fail"` + ); + }); +}); + +describe('createIndex', () => { + test('should call cluster with proper arguments', async () => { + await clusterClientAdapter.createIndex('foo'); + expect(clusterClient.callAsInternalUser).toHaveBeenCalledWith('indices.create', { + index: 'foo', + }); + }); + + test('should throw error when not getting an error of type resource_already_exists_exception', async () => { + clusterClient.callAsInternalUser.mockRejectedValue(new Error('Fail')); + await expect( + clusterClientAdapter.createIndex('foo') + ).rejects.toThrowErrorMatchingInlineSnapshot(`"error creating initial index: Fail"`); + }); + + test(`shouldn't throw when an error of type resource_already_exists_exception is thrown`, async () => { + const err = new Error('Already exists') as any; + err.body = { + error: { + type: 'resource_already_exists_exception', + }, + }; + clusterClient.callAsInternalUser.mockRejectedValue(err); + await clusterClientAdapter.createIndex('foo'); + }); +}); diff --git a/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts b/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts new file mode 100644 index 00000000000000..2231894a6f8ac6 --- /dev/null +++ b/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts @@ -0,0 +1,122 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { Logger, ClusterClient } from '../../../../../src/core/server'; + +export type EsClusterClient = Pick; +export type IClusterClientAdapter = PublicMethodsOf; + +export interface ConstructorOpts { + logger: Logger; + clusterClient: EsClusterClient; +} + +export class ClusterClientAdapter { + private readonly logger: Logger; + private readonly clusterClient: EsClusterClient; + + constructor(opts: ConstructorOpts) { + this.logger = opts.logger; + this.clusterClient = opts.clusterClient; + } + + public async doesIlmPolicyExist(policyName: string): Promise { + const request = { + method: 'GET', + path: `_ilm/policy/${policyName}`, + }; + try { + await this.callEs('transport.request', request); + } catch (err) { + if (err.statusCode === 404) return false; + throw new Error(`error checking existance of ilm policy: ${err.message}`); + } + return true; + } + + public async createIlmPolicy(policyName: string, policy: any): Promise { + const request = { + method: 'PUT', + path: `_ilm/policy/${policyName}`, + body: policy, + }; + try { + await this.callEs('transport.request', request); + } catch (err) { + throw new Error(`error creating ilm policy: ${err.message}`); + } + } + + public async doesIndexTemplateExist(name: string): Promise { + let result; + try { + result = await this.callEs('indices.existsTemplate', { name }); + } catch (err) { + throw new Error(`error checking existance of index template: ${err.message}`); + } + return result as boolean; + } + + public async createIndexTemplate(name: string, template: any): Promise { + const addTemplateParams = { + name, + create: true, + body: template, + }; + try { + await this.callEs('indices.putTemplate', addTemplateParams); + } catch (err) { + // The error message doesn't have a type attribute we can look to guarantee it's due + // to the template already existing (only long message) so we'll check ourselves to see + // if the template now exists. This scenario would happen if you startup multiple Kibana + // instances at the same time. + const existsNow = await this.doesIndexTemplateExist(name); + if (!existsNow) { + throw new Error(`error creating index template: ${err.message}`); + } + } + } + + public async doesAliasExist(name: string): Promise { + let result; + try { + result = await this.callEs('indices.existsAlias', { name }); + } catch (err) { + throw new Error(`error checking existance of initial index: ${err.message}`); + } + return result as boolean; + } + + public async createIndex(name: string): Promise { + try { + await this.callEs('indices.create', { index: name }); + } catch (err) { + if (err.body?.error?.type !== 'resource_already_exists_exception') { + throw new Error(`error creating initial index: ${err.message}`); + } + } + } + + private async callEs(operation: string, body?: any): Promise { + try { + this.debug(`callEs(${operation}) calls:`, body); + const result = await this.clusterClient.callAsInternalUser(operation, body); + this.debug(`callEs(${operation}) result:`, result); + return result; + } catch (err) { + this.debug(`callEs(${operation}) error:`, { + message: err.message, + statusCode: err.statusCode, + }); + throw err; + } + } + + private debug(message: string, object?: any) { + const objectString = object == null ? '' : JSON.stringify(object); + this.logger.debug(`esContext: ${message} ${objectString}`); + } +} diff --git a/x-pack/plugins/event_log/server/es/context.mock.ts b/x-pack/plugins/event_log/server/es/context.mock.ts index 41d6a7a0962b8e..6581cd689e43d9 100644 --- a/x-pack/plugins/event_log/server/es/context.mock.ts +++ b/x-pack/plugins/event_log/server/es/context.mock.ts @@ -6,15 +6,19 @@ import { EsContext } from './context'; import { namesMock } from './names.mock'; +import { IClusterClientAdapter } from './cluster_client_adapter'; import { loggingServiceMock } from '../../../../../src/core/server/mocks'; +import { clusterClientAdapterMock } from './cluster_client_adapter.mock'; const createContextMock = () => { - const mock: jest.Mocked = { + const mock: jest.Mocked & { + esAdapter: jest.Mocked; + } = { logger: loggingServiceMock.createLogger(), esNames: namesMock.create(), initialize: jest.fn(), waitTillReady: jest.fn(), - callEs: jest.fn(), + esAdapter: clusterClientAdapterMock.create(), }; return mock; }; diff --git a/x-pack/plugins/event_log/server/es/context.ts b/x-pack/plugins/event_log/server/es/context.ts index b93c1892d02064..144f44ac8e5ea9 100644 --- a/x-pack/plugins/event_log/server/es/context.ts +++ b/x-pack/plugins/event_log/server/es/context.ts @@ -8,6 +8,7 @@ import { Logger, ClusterClient } from 'src/core/server'; import { EsNames, getEsNames } from './names'; import { initializeEs } from './init'; +import { ClusterClientAdapter, IClusterClientAdapter } from './cluster_client_adapter'; import { createReadySignal, ReadySignal } from '../lib/ready_signal'; export type EsClusterClient = Pick; @@ -15,9 +16,9 @@ export type EsClusterClient = Pick; - callEs(operation: string, body?: any): Promise; } export interface EsError { @@ -38,16 +39,19 @@ export interface EsContextCtorParams { class EsContextImpl implements EsContext { public readonly logger: Logger; public readonly esNames: EsNames; - private readonly clusterClient: EsClusterClient; + public esAdapter: IClusterClientAdapter; private readonly readySignal: ReadySignal; private initialized: boolean; constructor(params: EsContextCtorParams) { this.logger = params.logger; this.esNames = getEsNames(params.indexNameRoot); - this.clusterClient = params.clusterClient; this.readySignal = createReadySignal(); this.initialized = false; + this.esAdapter = new ClusterClientAdapter({ + logger: params.logger, + clusterClient: params.clusterClient, + }); } initialize() { @@ -73,27 +77,7 @@ class EsContextImpl implements EsContext { return await this.readySignal.wait(); } - async callEs(operation: string, body?: any): Promise { - try { - this.debug(`callEs(${operation}) calls:`, body); - const result = await this.clusterClient.callAsInternalUser(operation, body); - this.debug(`callEs(${operation}) result:`, result); - return result; - } catch (err) { - this.debug(`callEs(${operation}) error:`, { - message: err.message, - statusCode: err.statusCode, - }); - throw err; - } - } - private async _initialize() { await initializeEs(this); } - - private debug(message: string, object?: any) { - const objectString = object == null ? '' : JSON.stringify(object); - this.logger.debug(`esContext: ${message} ${objectString}`); - } } diff --git a/x-pack/plugins/event_log/server/es/init.test.ts b/x-pack/plugins/event_log/server/es/init.test.ts index ace20846165d0d..ad237e522c0a53 100644 --- a/x-pack/plugins/event_log/server/es/init.test.ts +++ b/x-pack/plugins/event_log/server/es/init.test.ts @@ -8,180 +8,57 @@ import { contextMock } from './context.mock'; import { initializeEs } from './init'; describe('initializeEs', () => { - const esContext = contextMock.create(); - const createIndexParams = { - index: '.kibana-event-log-000001', - }; - const getIlmPolicyParams = { - method: 'GET', - path: '_ilm/policy/.kibana-event-log-policy', - }; - const indexTemplateExistsParams = { - name: '.kibana-event-log-template', - }; - const createIndexTemplateParams = { - create: true, - name: '.kibana-event-log-template', - body: expect.anything(), - }; - const createIlmParams = { - method: 'PUT', - path: '_ilm/policy/.kibana-event-log-policy', - body: { - policy: { - phases: { - hot: { - actions: { - rollover: { - max_age: '30d', - max_size: '5GB', - }, - }, - }, - }, - }, - }, - }; + let esContext = contextMock.create(); - beforeEach(() => jest.resetAllMocks()); + beforeEach(() => { + esContext = contextMock.create(); + }); - test('should initialize elasticsearch', async () => { - esContext.callEs.mockImplementation( - getCallEsImplementation({ - ilmPolicyExists: false, - indexTemplateExists: false, - initialIndexExists: false, - }) - ); + test(`should create ILM policy if it doesn't exist`, async () => { + esContext.esAdapter.doesIlmPolicyExist.mockResolvedValue(false); await initializeEs(esContext); - expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmParams); - expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateParams); - expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexParams); + expect(esContext.esAdapter.doesIlmPolicyExist).toHaveBeenCalled(); + expect(esContext.esAdapter.createIlmPolicy).toHaveBeenCalled(); }); - test('should skip initializing ILM policy when it already exists', async () => { - esContext.callEs.mockImplementation( - getCallEsImplementation({ - ilmPolicyExists: true, - indexTemplateExists: false, - initialIndexExists: false, - }) - ); + test(`shouldn't create ILM policy if it exists`, async () => { + esContext.esAdapter.doesIlmPolicyExist.mockResolvedValue(true); await initializeEs(esContext); - expect(esContext.callEs).not.toHaveBeenCalledWith('transport.request', createIlmParams); - expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateParams); - expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexParams); + expect(esContext.esAdapter.doesIlmPolicyExist).toHaveBeenCalled(); + expect(esContext.esAdapter.createIlmPolicy).not.toHaveBeenCalled(); }); - test('should skip creating index template when it already exists', async () => { - esContext.callEs.mockImplementation( - getCallEsImplementation({ - ilmPolicyExists: false, - indexTemplateExists: true, - initialIndexExists: false, - }) - ); + test(`should create index template if it doesn't exist`, async () => { + esContext.esAdapter.doesIndexTemplateExist.mockResolvedValue(false); await initializeEs(esContext); - expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmParams); - expect(esContext.callEs).not.toHaveBeenCalledWith( - 'indices.putTemplate', - createIndexTemplateParams - ); - expect(esContext.callEs).toHaveBeenCalledWith('indices.create', createIndexParams); + expect(esContext.esAdapter.doesIndexTemplateExist).toHaveBeenCalled(); + expect(esContext.esAdapter.createIndexTemplate).toHaveBeenCalled(); }); - test('should skip creating initial index when it already exists', async () => { - esContext.callEs.mockImplementation( - getCallEsImplementation({ - ilmPolicyExists: false, - indexTemplateExists: false, - initialIndexExists: true, - }) - ); + test(`shouldn't create index template if it already exists`, async () => { + esContext.esAdapter.doesIndexTemplateExist.mockResolvedValue(true); await initializeEs(esContext); - expect(esContext.callEs).toHaveBeenCalledWith('transport.request', createIlmParams); - expect(esContext.callEs).toHaveBeenCalledWith('indices.putTemplate', createIndexTemplateParams); - expect(esContext.callEs).not.toHaveBeenCalledWith('indices.create', createIndexParams); + expect(esContext.esAdapter.doesIndexTemplateExist).toHaveBeenCalled(); + expect(esContext.esAdapter.createIndexTemplate).not.toHaveBeenCalled(); }); - test('should double check if index template exists before logging error', async () => { - esContext.callEs.mockImplementation( - getCallEsImplementation({ - ilmPolicyExists: false, - indexTemplateExists: false, - initialIndexExists: false, - throwErrorWhenCreatingIndexTemplate: true, - }) - ); + test(`should create initial index if it doesn't exist`, async () => { + esContext.esAdapter.doesAliasExist.mockResolvedValue(false); await initializeEs(esContext); - expect(esContext.callEs).toHaveBeenCalledTimes(5); - expect(esContext.callEs).toHaveBeenNthCalledWith(1, 'transport.request', getIlmPolicyParams); - expect(esContext.callEs).toHaveBeenNthCalledWith(2, 'transport.request', createIlmParams); - expect(esContext.callEs).toHaveBeenNthCalledWith( - 3, - 'indices.existsTemplate', - indexTemplateExistsParams - ); - expect(esContext.callEs).toHaveBeenNthCalledWith( - 4, - 'indices.putTemplate', - createIndexTemplateParams - ); - expect(esContext.callEs).toHaveBeenNthCalledWith( - 5, - 'indices.existsTemplate', - indexTemplateExistsParams - ); + expect(esContext.esAdapter.doesAliasExist).toHaveBeenCalled(); + expect(esContext.esAdapter.createIndex).toHaveBeenCalled(); }); -}); - -function throwNotFoundError() { - const err = new Error('Not found') as any; - err.statusCode = 404; - throw err; -} - -function getCallEsImplementation({ - ilmPolicyExists, - indexTemplateExists, - initialIndexExists, - throwErrorWhenCreatingIndexTemplate = false, -}: { - ilmPolicyExists: boolean; - indexTemplateExists: boolean; - initialIndexExists: boolean; - throwErrorWhenCreatingIndexTemplate?: boolean; -}) { - return async (operation: string, body?: any) => { - // Make ILM policy exist check return false - if ( - operation === 'transport.request' && - body?.method === 'GET' && - body?.path === '_ilm/policy/.kibana-event-log-policy' - ) { - if (!ilmPolicyExists) { - throwNotFoundError(); - } - return {}; - } - // Make index template exists check return false - if (operation === 'indices.existsTemplate' && body?.name === '.kibana-event-log-template') { - return indexTemplateExists; - } + test(`shouldn't create initial index if it already exists`, async () => { + esContext.esAdapter.doesAliasExist.mockResolvedValue(true); - // Make alias exists check return false - if (operation === 'indices.existsAlias' && body?.name === '.kibana-event-log') { - return initialIndexExists; - } - - if (operation === 'indices.putTemplate' && throwErrorWhenCreatingIndexTemplate) { - throw new Error('Fail'); - } - }; -} + await initializeEs(esContext); + expect(esContext.esAdapter.doesAliasExist).toHaveBeenCalled(); + expect(esContext.esAdapter.createIndex).not.toHaveBeenCalled(); + }); +}); diff --git a/x-pack/plugins/event_log/server/es/init.ts b/x-pack/plugins/event_log/server/es/init.ts index ce99f125bbafaf..7094277f7aa9fd 100644 --- a/x-pack/plugins/event_log/server/es/init.ts +++ b/x-pack/plugins/event_log/server/es/init.ts @@ -24,18 +24,9 @@ export async function initializeEs(esContext: EsContext): Promise { async function initializeEsResources(esContext: EsContext) { const steps = new EsInitializationSteps(esContext); - // create the ilm policy, if it doesn't exist - if (!(await steps.doesIlmPolicyExist())) { - await steps.createIlmPolicy(); - } - - if (!(await steps.doesIndexTemplateExist())) { - await steps.createIndexTemplate(); - } - - if (!(await steps.doesInitialIndexExist())) { - await steps.createInitialIndex(); - } + await steps.createIlmPolicyIfNotExists(); + await steps.createIndexTemplateIfNotExists(); + await steps.createInitialIndexIfNotExists(); } class EsInitializationSteps { @@ -43,92 +34,35 @@ class EsInitializationSteps { this.esContext = esContext; } - async doesIlmPolicyExist(): Promise { - const request = { - method: 'GET', - path: `_ilm/policy/${this.esContext.esNames.ilmPolicy}`, - }; - try { - await this.esContext.callEs('transport.request', request); - } catch (err) { - if (err.statusCode === 404) return false; - throw new Error(`error checking existance of ilm policy: ${err.message}`); - } - return true; - } - - async createIlmPolicy(): Promise { - const request = { - method: 'PUT', - path: `_ilm/policy/${this.esContext.esNames.ilmPolicy}`, - body: getIlmPolicy(), - }; - try { - await this.esContext.callEs('transport.request', request); - } catch (err) { - throw new Error(`error creating ilm policy: ${err.message}`); - } - } - - async doesIndexTemplateExist(): Promise { - const name = this.esContext.esNames.indexTemplate; - let result; - try { - result = await this.esContext.callEs('indices.existsTemplate', { name }); - } catch (err) { - throw new Error(`error checking existance of index template: ${err.message}`); - } - return result as boolean; - } - - async createIndexTemplate(): Promise { - const templateBody = getIndexTemplate(this.esContext.esNames); - const addTemplateParams = { - create: true, - name: this.esContext.esNames.indexTemplate, - body: templateBody, - }; - try { - await this.esContext.callEs('indices.putTemplate', addTemplateParams); - } catch (err) { - // The error message doesn't have a type attribute we can look to guarantee it's due - // to the template already existing (only long message) so we'll check ourselves to see - // if the template now exists. This scenario would happen if you startup multiple Kibana - // instances at the same time. - const existsNow = await this.doesIndexTemplateExist(); - if (!existsNow) { - throw new Error(`error creating index template: ${err.message}`); - } + async createIlmPolicyIfNotExists(): Promise { + const exists = await this.esContext.esAdapter.doesIlmPolicyExist( + this.esContext.esNames.ilmPolicy + ); + if (!exists) { + await this.esContext.esAdapter.createIlmPolicy( + this.esContext.esNames.ilmPolicy, + getIlmPolicy() + ); } } - async doesInitialIndexExist(): Promise { - const name = this.esContext.esNames.alias; - let result; - try { - result = await this.esContext.callEs('indices.existsAlias', { name }); - } catch (err) { - throw new Error(`error checking existance of initial index: ${err.message}`); + async createIndexTemplateIfNotExists(): Promise { + const exists = await this.esContext.esAdapter.doesIndexTemplateExist( + this.esContext.esNames.indexTemplate + ); + if (!exists) { + const templateBody = getIndexTemplate(this.esContext.esNames); + await this.esContext.esAdapter.createIndexTemplate( + this.esContext.esNames.indexTemplate, + templateBody + ); } - return result as boolean; } - async createInitialIndex(): Promise { - const index = this.esContext.esNames.initialIndex; - try { - await this.esContext.callEs('indices.create', { index }); - } catch (err) { - if (err.body?.error?.type !== 'resource_already_exists_exception') { - throw new Error(`error creating initial index: ${err.message}`); - } + async createInitialIndexIfNotExists(): Promise { + const exists = await this.esContext.esAdapter.doesAliasExist(this.esContext.esNames.alias); + if (!exists) { + await this.esContext.esAdapter.createIndex(this.esContext.esNames.initialIndex); } } - - debug(message: string) { - this.esContext.logger.debug(message); - } - - warn(message: string) { - this.esContext.logger.warn(message); - } } From 88b525c95919879d7809f46849ce21edfef87502 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Wed, 12 Feb 2020 11:58:09 -0500 Subject: [PATCH 7/7] Fix places calling callEs still --- .../server/es/cluster_client_adapter.mock.ts | 1 + .../server/es/cluster_client_adapter.test.ts | 16 ++++++++++++++++ .../server/es/cluster_client_adapter.ts | 4 ++++ .../event_log/server/event_log_service.test.ts | 8 ++------ .../event_log/server/event_logger.test.ts | 5 ++--- x-pack/plugins/event_log/server/event_logger.ts | 2 +- 6 files changed, 26 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/event_log/server/es/cluster_client_adapter.mock.ts b/x-pack/plugins/event_log/server/es/cluster_client_adapter.mock.ts index 2b210f67f4ea5f..87e8fb0f521a9e 100644 --- a/x-pack/plugins/event_log/server/es/cluster_client_adapter.mock.ts +++ b/x-pack/plugins/event_log/server/es/cluster_client_adapter.mock.ts @@ -8,6 +8,7 @@ import { IClusterClientAdapter } from './cluster_client_adapter'; const createClusterClientMock = () => { const mock: jest.Mocked = { + indexDocument: jest.fn(), doesIlmPolicyExist: jest.fn(), createIlmPolicy: jest.fn(), doesIndexTemplateExist: jest.fn(), diff --git a/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts b/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts index c4ef62d81d3a32..ecefd4bfa271ef 100644 --- a/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts +++ b/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts @@ -23,6 +23,22 @@ beforeEach(() => { }); }); +describe('indexDocument', () => { + test('should call cluster client with given doc', async () => { + await clusterClientAdapter.indexDocument({ args: true }); + expect(clusterClient.callAsInternalUser).toHaveBeenCalledWith('index', { + args: true, + }); + }); + + test('should throw error when cluster client throws an error', async () => { + clusterClient.callAsInternalUser.mockRejectedValue(new Error('Fail')); + await expect( + clusterClientAdapter.indexDocument({ args: true }) + ).rejects.toThrowErrorMatchingInlineSnapshot(`"Fail"`); + }); +}); + describe('doesIlmPolicyExist', () => { const notFoundError = new Error('Not found') as any; notFoundError.statusCode = 404; diff --git a/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts b/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts index 2231894a6f8ac6..c74eeacc9bb19c 100644 --- a/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts +++ b/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts @@ -23,6 +23,10 @@ export class ClusterClientAdapter { this.clusterClient = opts.clusterClient; } + public async indexDocument(doc: any): Promise { + await this.callEs('index', doc); + } + public async doesIlmPolicyExist(policyName: string): Promise { const request = { method: 'GET', diff --git a/x-pack/plugins/event_log/server/event_log_service.test.ts b/x-pack/plugins/event_log/server/event_log_service.test.ts index c7e752d1a652bc..3b250b74620097 100644 --- a/x-pack/plugins/event_log/server/event_log_service.test.ts +++ b/x-pack/plugins/event_log/server/event_log_service.test.ts @@ -6,18 +6,14 @@ import { IEventLogConfig } from './types'; import { EventLogService } from './event_log_service'; -import { getEsNames } from './es/names'; -import { createMockEsContext } from './es/context.mock'; +import { contextMock } from './es/context.mock'; import { loggingServiceMock } from '../../../../src/core/server/logging/logging_service.mock'; const loggingService = loggingServiceMock.create(); const systemLogger = loggingService.get(); describe('EventLogService', () => { - const esContext = createMockEsContext({ - esNames: getEsNames('ABC'), - logger: systemLogger, - }); + const esContext = contextMock.create(); function getService(config: IEventLogConfig) { const { enabled, logEntries, indexEntries } = config; diff --git a/x-pack/plugins/event_log/server/event_logger.test.ts b/x-pack/plugins/event_log/server/event_logger.test.ts index c2de8d4dfd12bd..e71e7ef801f779 100644 --- a/x-pack/plugins/event_log/server/event_logger.test.ts +++ b/x-pack/plugins/event_log/server/event_logger.test.ts @@ -7,9 +7,8 @@ import { IEvent, IEventLogger, IEventLogService } from './index'; import { ECS_VERSION } from './types'; import { EventLogService } from './event_log_service'; -import { getEsNames } from './es/names'; import { EsContext } from './es/context'; -import { createMockEsContext } from './es/context.mock'; +import { contextMock } from './es/context.mock'; import { loggerMock, MockedLogger } from '../../../../src/core/server/logging/logger.mock'; import { delay } from './lib/delay'; import { EVENT_LOGGED_PREFIX } from './event_logger'; @@ -24,7 +23,7 @@ describe('EventLogger', () => { beforeEach(() => { systemLogger = loggerMock.create(); - esContext = createMockEsContext({ esNames: getEsNames('ABC'), logger: systemLogger }); + esContext = contextMock.create(); service = new EventLogService({ esContext, systemLogger, diff --git a/x-pack/plugins/event_log/server/event_logger.ts b/x-pack/plugins/event_log/server/event_logger.ts index 891abda947fc8a..f5149da069953c 100644 --- a/x-pack/plugins/event_log/server/event_logger.ts +++ b/x-pack/plugins/event_log/server/event_logger.ts @@ -171,7 +171,7 @@ function indexEventDoc(esContext: EsContext, doc: Doc): void { async function indexLogEventDoc(esContext: EsContext, doc: any) { esContext.logger.debug(`writing to event log: ${JSON.stringify(doc)}`); await esContext.waitTillReady(); - await esContext.callEs('index', doc); + await esContext.esAdapter.indexDocument(doc); esContext.logger.debug(`writing to event log complete`); }