From 8e78f899a387c89d1c1118b3d34e8b52f079dbe6 Mon Sep 17 00:00:00 2001 From: Patrick Mueller Date: Thu, 19 Mar 2020 17:47:24 -0400 Subject: [PATCH] [Alerting]: harden APIs of built-in alert index-threshold resolves https://github.com/elastic/kibana/issues/59889 The index threshold APIs - used by both the index threshold UI and the alert executor - were returning errors (500's from http endpoints) when getting errors from ES. These have been changed so that the error is logged as a warning, and the relevant API returns an "empty" result. Another 500 response was found while experimenting with this. Apparently the date_range agg requires a date format to be passed in if the date format in ES is not an ISO date. The repro on this was to select the `.security` alias (or it's index) within the index threshold alert UI, and then select one of it's date fields. --- .../index_threshold/lib/time_series_query.ts | 5 ++-- .../index_threshold/routes/fields.ts | 7 +++-- .../index_threshold/routes/indices.ts | 11 +++++--- .../routes/time_series_query.ts | 22 ++++++---------- .../common/lib/es_test_index_tool.ts | 4 +++ .../index_threshold/alert.ts | 9 ++++--- .../index_threshold/create_test_data.ts | 1 + .../index_threshold/fields_endpoint.ts | 6 +++++ .../index_threshold/indices_endpoint.ts | 6 +++++ .../time_series_query_endpoint.ts | 26 +++++++++++++++++++ 10 files changed, 72 insertions(+), 25 deletions(-) diff --git a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/lib/time_series_query.ts b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/lib/time_series_query.ts index 0382792dafb35..1d9cc1c98bc01 100644 --- a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/lib/time_series_query.ts +++ b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/lib/time_series_query.ts @@ -94,6 +94,7 @@ export async function timeSeriesQuery( dateAgg: { date_range: { field: timeField, + format: 'strict_date_time', ranges: dateRangeInfo.dateRanges, }, }, @@ -134,8 +135,8 @@ export async function timeSeriesQuery( esResult = await callCluster('search', esQuery); } catch (err) { // console.log('time_series_query.ts error\n', JSON.stringify(err, null, 4)); - logger.warn(`${logPrefix} error: ${JSON.stringify(err.message)}`); - throw new Error('error running search'); + logger.warn(`${logPrefix} error: ${err.message}`); + return { results: [] }; } // console.log('time_series_query.ts response\n', JSON.stringify(esResult, null, 4)); diff --git a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/fields.ts b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/fields.ts index c862d96828eb4..32d6409d9c9fb 100644 --- a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/fields.ts +++ b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/fields.ts @@ -53,8 +53,11 @@ export function createFieldsRoute(service: Service, router: IRouter, baseRoute: try { rawFields = await getRawFields(ctx.core.elasticsearch.dataClient, req.body.indexPatterns); } catch (err) { - service.logger.debug(`route ${path} error: ${err.message}`); - return res.internalError({ body: 'error getting field data' }); + const indexPatterns = req.body.indexPatterns.join(','); + service.logger.warn( + `route ${path} error getting fields from pattern "${indexPatterns}": ${err.message}` + ); + return res.ok({ body: { fields: [] } }); } const result = { fields: getFieldsFromRawFields(rawFields) }; diff --git a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/indices.ts b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/indices.ts index 760ed21078de2..c08450448b44c 100644 --- a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/indices.ts +++ b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/indices.ts @@ -54,15 +54,18 @@ export function createIndicesRoute(service: Service, router: IRouter, baseRoute: try { aliases = await getAliasesFromPattern(ctx.core.elasticsearch.dataClient, pattern); } catch (err) { - service.logger.debug(`route ${path} error: ${err.message}`); - return res.internalError({ body: 'error getting alias data' }); + service.logger.warn( + `route ${path} error getting aliases from pattern "${pattern}": ${err.message}` + ); } + let indices: string[] = []; try { indices = await getIndicesFromPattern(ctx.core.elasticsearch.dataClient, pattern); } catch (err) { - service.logger.debug(`route ${path} error: ${err.message}`); - return res.internalError({ body: 'error getting index data' }); + service.logger.warn( + `route ${path} error getting indices from pattern "${pattern}": ${err.message}` + ); } const result = { indices: uniqueCombined(aliases, indices, MAX_INDICES) }; diff --git a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/time_series_query.ts b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/time_series_query.ts index 16864d250a747..c8129c2428ee4 100644 --- a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/time_series_query.ts +++ b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/time_series_query.ts @@ -13,7 +13,7 @@ import { } from 'kibana/server'; import { Service } from '../../../types'; -import { TimeSeriesQuery, TimeSeriesQuerySchema, TimeSeriesResult } from '../lib/time_series_types'; +import { TimeSeriesQuery, TimeSeriesQuerySchema } from '../lib/time_series_types'; export { TimeSeriesQuery, TimeSeriesResult } from '../lib/time_series_types'; export function createTimeSeriesQueryRoute(service: Service, router: IRouter, baseRoute: string) { @@ -33,21 +33,15 @@ export function createTimeSeriesQueryRoute(service: Service, router: IRouter, ba req: KibanaRequest, res: KibanaResponseFactory ): Promise { - service.logger.debug(`route query_data request: ${JSON.stringify(req.body)}`); + service.logger.debug(`route ${path} request: ${JSON.stringify(req.body)}`); - let result: TimeSeriesResult; - try { - result = await service.indexThreshold.timeSeriesQuery({ - logger: service.logger, - callCluster: ctx.core.elasticsearch.dataClient.callAsCurrentUser, - query: req.body, - }); - } catch (err) { - service.logger.debug(`route query_data error: ${err.message}`); - return res.internalError({ body: 'error running time series query' }); - } + const result = await service.indexThreshold.timeSeriesQuery({ + logger: service.logger, + callCluster: ctx.core.elasticsearch.dataClient.callAsCurrentUser, + query: req.body, + }); - service.logger.debug(`route query_data response: ${JSON.stringify(result)}`); + service.logger.debug(`route ${path} response: ${JSON.stringify(result)}`); return res.ok({ body: result }); } } diff --git a/x-pack/test/alerting_api_integration/common/lib/es_test_index_tool.ts b/x-pack/test/alerting_api_integration/common/lib/es_test_index_tool.ts index 999a8686e0ee7..14a91325d1cc1 100644 --- a/x-pack/test/alerting_api_integration/common/lib/es_test_index_tool.ts +++ b/x-pack/test/alerting_api_integration/common/lib/es_test_index_tool.ts @@ -41,6 +41,10 @@ export class ESTestIndexTool { type: 'date', format: 'strict_date_time', }, + date_epoch_millis: { + type: 'date', + format: 'epoch_millis', + }, testedValue: { type: 'long', }, diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/alert.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/alert.ts index 13f3a4971183c..87acbcf99d383 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/alert.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/alert.ts @@ -172,12 +172,14 @@ export default function alertTests({ getService }: FtrProviderContext) { // create some more documents in the first group createEsDocumentsInGroups(1); + // this never fires because of bad fields error await createAlert({ name: 'never fire', + timeField: 'source', // bad field for time aggType: 'avg', - aggField: 'testedValue', + aggField: 'source', // bad field for agg groupBy: 'all', - thresholdComparator: '<', + thresholdComparator: '>', threshold: [0], }); @@ -303,6 +305,7 @@ export default function alertTests({ getService }: FtrProviderContext) { name: string; aggType: string; aggField?: string; + timeField?: string; groupBy: 'all' | 'top'; termField?: string; termSize?: number; @@ -347,7 +350,7 @@ export default function alertTests({ getService }: FtrProviderContext) { actions: [action], params: { index: ES_TEST_INDEX_NAME, - timeField: 'date', + timeField: params.timeField || 'date', aggType: params.aggType, aggField: params.aggField, groupBy: params.groupBy, diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/create_test_data.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/create_test_data.ts index 21f73ac9b9833..1a83f34f0fdfb 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/create_test_data.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/create_test_data.ts @@ -53,6 +53,7 @@ async function createEsDocument(es: any, epochMillis: number, testedValue: numbe source: DOCUMENT_SOURCE, reference: DOCUMENT_REFERENCE, date: new Date(epochMillis).toISOString(), + date_epoch_millis: epochMillis, testedValue, group, }; diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/fields_endpoint.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/fields_endpoint.ts index fa7aed2c035b9..c6f8f6d1b80b1 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/fields_endpoint.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/fields_endpoint.ts @@ -139,6 +139,12 @@ export default function fieldsEndpointTests({ getService }: FtrProviderContext) expect(field.name).to.eql('updated_at'); expect(field.type).to.eql('date'); }); + + // TODO: the pattern '*a:b,c:d*' throws an exception in dev, but not ci! + it('should handle no_such_remote_cluster', async () => { + const result = await runQueryExpect({ indexPatterns: ['*a:b,c:d*'] }, 200); + expect(result.fields.length).to.be(0); + }); }); function getFieldNamed(fields: any[], fieldName: string): any | undefined { diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/indices_endpoint.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/indices_endpoint.ts index 6908398deb57b..72484fa70f9cc 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/indices_endpoint.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/indices_endpoint.ts @@ -105,6 +105,12 @@ export default function indicesEndpointTests({ getService }: FtrProviderContext) expect(result.indices).to.be.an('array'); expect(result.indices.includes('.kibana')).to.be(true); }); + + // TODO: the pattern '*a:b,c:d*' throws an exception in dev, but not ci! + it('should handle no_such_remote_cluster', async () => { + const result = await runQueryExpect({ pattern: '*a:b,c:d*' }, 200); + expect(result.indices.length).to.be(0); + }); }); async function runQueryExpect(requestBody: any, status: number): Promise { diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/time_series_query_endpoint.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/time_series_query_endpoint.ts index c9b488da5dec5..932ffe3a7ce14 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/time_series_query_endpoint.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/time_series_query_endpoint.ts @@ -273,6 +273,32 @@ export default function timeSeriesQueryEndpointTests({ getService }: FtrProvider }; expect(await runQueryExpect(query, 400)).eql(expected); }); + + it('should handle epoch_millis time field', async () => { + const query = getQueryBody({ + dateStart: START_DATE, + dateEnd: START_DATE, + timeField: 'date_epoch_millis', + }); + const expected = { + results: [{ group: 'all documents', metrics: [[START_DATE, 6]] }], + }; + expect(await runQueryExpect(query, 200)).eql(expected); + }); + + it('should handle ES errors', async () => { + const query = getQueryBody({ + dateStart: START_DATE, + dateEnd: START_DATE, + timeField: 'source', // bad field for time + aggType: 'avg', + aggField: 'source', // bad field for agg + }); + const expected = { + results: [], + }; + expect(await runQueryExpect(query, 200)).eql(expected); + }); }); async function runQueryExpect(requestBody: TimeSeriesQuery, status: number): Promise {