From c2340823329575d881e7fe7eca06f95c1ff7a1ad Mon Sep 17 00:00:00 2001 From: Ying Date: Wed, 18 Sep 2024 13:34:05 -0400 Subject: [PATCH 1/5] Using es partial update at end of rule run --- .../partially_update_rule.test.ts | 116 +++++++++++++++++- .../saved_objects/partially_update_rule.ts | 34 +++++ .../alerting/server/task_runner/fixtures.ts | 89 +++++++------- .../server/task_runner/task_runner.test.ts | 51 +++----- .../server/task_runner/task_runner.ts | 20 ++- .../task_runner_alerts_client.test.ts | 18 +-- .../task_runner/task_runner_cancel.test.ts | 85 +++++++------ 7 files changed, 274 insertions(+), 139 deletions(-) diff --git a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.test.ts b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.test.ts index 5fcf23cbae6fb..f1eabff5d963c 100644 --- a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.test.ts +++ b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.test.ts @@ -10,16 +10,22 @@ import { ISavedObjectsRepository, SavedObjectsErrorHelpers, } from '@kbn/core/server'; - -import { PartiallyUpdateableRuleAttributes, partiallyUpdateRule } from './partially_update_rule'; -import { savedObjectsClientMock } from '@kbn/core/server/mocks'; +import { + PartiallyUpdateableRuleAttributes, + partiallyUpdateRule, + partiallyUpdateRuleWithEs, +} from './partially_update_rule'; +import { elasticsearchServiceMock, savedObjectsClientMock } from '@kbn/core/server/mocks'; import { RULE_SAVED_OBJECT_TYPE } from '.'; +import { ALERTING_CASES_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server'; +import { estypes } from '@elastic/elasticsearch'; const MockSavedObjectsClientContract = savedObjectsClientMock.create(); const MockISavedObjectsRepository = MockSavedObjectsClientContract as unknown as jest.Mocked; +const esClient = elasticsearchServiceMock.createClusterClient().asInternalUser; -describe('partially_update_rule', () => { +describe('partiallyUpdateRule', () => { beforeEach(() => { jest.resetAllMocks(); }); @@ -104,6 +110,98 @@ describe('partially_update_rule', () => { }); }); +describe('partiallyUpdateRuleWithEs', () => { + beforeEach(() => { + jest.resetAllMocks(); + jest.clearAllMocks(); + }); + + test('should work with no options', async () => { + esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); + + await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes); + expect(esClient.update).toHaveBeenCalledTimes(1); + expect(esClient.update).toHaveBeenCalledWith({ + id: `alert:${MockRuleId}`, + index: ALERTING_CASES_SAVED_OBJECT_INDEX, + doc: { + alert: DefaultAttributes, + }, + }); + }); + + test('should work with extraneous attributes ', async () => { + const attributes = ExtraneousAttributes as unknown as PartiallyUpdateableRuleAttributes; + esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); + + await partiallyUpdateRuleWithEs(esClient, MockRuleId, attributes); + expect(esClient.update).toHaveBeenCalledWith({ + id: `alert:${MockRuleId}`, + index: ALERTING_CASES_SAVED_OBJECT_INDEX, + doc: { + alert: ExtraneousAttributes, + }, + }); + }); + + test('should handle ES errors', async () => { + esClient.update.mockRejectedValueOnce(new Error('wops')); + + await expect( + partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes) + ).rejects.toThrowError('wops'); + }); + + test('should handle the version option', async () => { + esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); + + await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes, { + version: 'WzQsMV0=', + }); + expect(esClient.update).toHaveBeenCalledWith({ + id: `alert:${MockRuleId}`, + index: ALERTING_CASES_SAVED_OBJECT_INDEX, + if_primary_term: 1, + if_seq_no: 4, + doc: { + alert: DefaultAttributes, + }, + }); + }); + + test('should handle the ignore404 option', async () => { + esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); + + await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes, { ignore404: true }); + expect(esClient.update).toHaveBeenCalledWith( + { + id: `alert:${MockRuleId}`, + index: ALERTING_CASES_SAVED_OBJECT_INDEX, + doc: { + alert: DefaultAttributes, + }, + }, + { ignore: [404] } + ); + }); + + test('should handle the refresh option', async () => { + esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); + + await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes, { + refresh: 'wait_for', + }); + expect(esClient.update).toHaveBeenCalledWith({ + id: `alert:${MockRuleId}`, + index: ALERTING_CASES_SAVED_OBJECT_INDEX, + doc: { + alert: DefaultAttributes, + }, + refresh: 'wait_for', + }); + }); +}); + function getMockSavedObjectClients(): Record< string, jest.Mocked @@ -137,3 +235,13 @@ const MockUpdateValue = { }, references: [], }; + +const MockEsUpdateResponse = (id: string) => ({ + _index: '.kibana_alerting_cases_9.0.0_001', + _id: `alert:${id}`, + _version: 3, + result: 'updated' as estypes.Result, + _shards: { total: 1, successful: 1, failed: 0 }, + _seq_no: 5, + _primary_term: 1, +}); diff --git a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts index 2665845a1110f..329f01ece3dec 100644 --- a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts +++ b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts @@ -7,10 +7,13 @@ import { omit, pick } from 'lodash'; import { + ElasticsearchClient, SavedObjectsClient, SavedObjectsErrorHelpers, SavedObjectsUpdateOptions, } from '@kbn/core/server'; +import { decodeRequestVersion } from '@kbn/core-saved-objects-base-server-internal'; +import { ALERTING_CASES_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server'; import { RawRule } from '../types'; import { @@ -67,3 +70,34 @@ export async function partiallyUpdateRule( throw err; } } + +// direct, partial update to a rule saved object via ElasticsearchClient +// using namespace set in the client +export async function partiallyUpdateRuleWithEs( + esClient: ElasticsearchClient, + id: string, + attributes: PartiallyUpdateableRuleAttributes, + options: PartiallyUpdateRuleSavedObjectOptions = {} +): Promise { + // ensure we only have the valid attributes that are not encrypted and are excluded from AAD + const attributeUpdates = omit(attributes, [ + ...RuleAttributesToEncrypt, + ...RuleAttributesIncludedInAAD, + ]); + + const updateParams = { + id: `alert:${id}`, + index: ALERTING_CASES_SAVED_OBJECT_INDEX, + ...(options.version ? decodeRequestVersion(options.version) : {}), + doc: { + alert: attributeUpdates, + }, + ...(options.refresh ? { refresh: options.refresh } : {}), + }; + + if (options.ignore404) { + await esClient.update(updateParams, { ignore: [404] }); + } else { + await esClient.update(updateParams); + } +} diff --git a/x-pack/plugins/alerting/server/task_runner/fixtures.ts b/x-pack/plugins/alerting/server/task_runner/fixtures.ts index ae8eccfcb1f86..5174aa9b965ec 100644 --- a/x-pack/plugins/alerting/server/task_runner/fixtures.ts +++ b/x-pack/plugins/alerting/server/task_runner/fixtures.ts @@ -7,6 +7,7 @@ import { TaskStatus } from '@kbn/task-manager-plugin/server'; import { SavedObject } from '@kbn/core/server'; +import { ALERTING_CASES_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server'; import { Rule, RuleTypeParams, @@ -64,7 +65,7 @@ const defaultHistory = [ }, ]; -export const generateSavedObjectParams = ({ +export const generateRuleUpdateParams = ({ error = null, warning = null, status = 'ok', @@ -83,53 +84,59 @@ export const generateSavedObjectParams = ({ history?: RuleMonitoring['run']['history']; alertsCount?: Record; }) => [ - RULE_SAVED_OBJECT_TYPE, - '1', { - monitoring: { - run: { - calculated_metrics: { - success_ratio: successRatio, + id: `alert:1`, + index: ALERTING_CASES_SAVED_OBJECT_INDEX, + doc: { + alert: { + monitoring: { + run: { + calculated_metrics: { + success_ratio: successRatio, + }, + history, + last_run: { + timestamp: '1970-01-01T00:00:00.000Z', + metrics: { + duration: 0, + gap_duration_s: null, + total_alerts_created: null, + total_alerts_detected: null, + total_indexing_duration_ms: null, + total_search_duration_ms: null, + }, + }, + }, }, - history, - last_run: { - timestamp: '1970-01-01T00:00:00.000Z', - metrics: { - duration: 0, - gap_duration_s: null, - total_alerts_created: null, - total_alerts_detected: null, - total_indexing_duration_ms: null, - total_search_duration_ms: null, + executionStatus: { + error, + lastDuration: 0, + lastExecutionDate: '1970-01-01T00:00:00.000Z', + status, + warning, + }, + lastRun: { + outcome, + outcomeOrder: RuleLastRunOutcomeOrderMap[outcome], + outcomeMsg: + (error?.message && [error?.message]) || + (warning?.message && [warning?.message]) || + null, + warning: error?.reason || warning?.reason || null, + alertsCount: { + active: 0, + ignored: 0, + new: 0, + recovered: 0, + ...(alertsCount || {}), }, }, + nextRun, + running: false, }, }, - executionStatus: { - error, - lastDuration: 0, - lastExecutionDate: '1970-01-01T00:00:00.000Z', - status, - warning, - }, - lastRun: { - outcome, - outcomeOrder: RuleLastRunOutcomeOrderMap[outcome], - outcomeMsg: - (error?.message && [error?.message]) || (warning?.message && [warning?.message]) || null, - warning: error?.reason || warning?.reason || null, - alertsCount: { - active: 0, - ignored: 0, - new: 0, - recovered: 0, - ...(alertsCount || {}), - }, - }, - nextRun, - running: false, }, - { refresh: false, namespace: undefined }, + { ignore: [404] }, ]; export const GENERIC_ERROR_MESSAGE = 'GENERIC ERROR MESSAGE'; diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts index 833393504fdeb..5d9b9597c2e3f 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts @@ -59,7 +59,7 @@ import { generateRunnerResult, RULE_ACTIONS, generateEnqueueFunctionInput, - generateSavedObjectParams, + generateRuleUpdateParams, mockTaskInstance, GENERIC_ERROR_MESSAGE, generateAlertInstance, @@ -344,8 +344,8 @@ describe('Task Runner', () => { testAlertingEventLogCalls({ status: 'ok' }); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({}) + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({}) ); expect(taskRunnerFactoryInitializerParams.executionContext.withContext).toBeCalledTimes(1); @@ -2739,8 +2739,8 @@ describe('Task Runner', () => { status: 'ok', }); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({}) + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({}) ); expect(mockUsageCounter.incrementCounter).not.toHaveBeenCalled(); }); @@ -2852,10 +2852,8 @@ describe('Task Runner', () => { }); await taskRunner.run(); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({ - nextRun: '1970-01-01T00:00:10.000Z', - }) + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({ nextRun: '1970-01-01T00:00:10.000Z' }) ); }); @@ -2888,21 +2886,14 @@ describe('Task Runner', () => { ); await taskRunner.run(); ruleType.executor.mockClear(); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({ - error: { - message: GENERIC_ERROR_MESSAGE, - reason: 'execute', - }, + + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({ + error: { message: GENERIC_ERROR_MESSAGE, reason: 'execute' }, outcome: 'failed', status: 'error', successRatio: 0, - history: [ - { - success: false, - timestamp: 0, - }, - ], + history: [{ success: false, timestamp: 0 }], }) ); }); @@ -3010,15 +3001,12 @@ describe('Task Runner', () => { expect(actionsClient.bulkEnqueueExecution).toHaveBeenCalledTimes(1); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({ + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({ status: 'warning', outcome: 'warning', warning, - alertsCount: { - active: 1, - new: 1, - }, + alertsCount: { active: 1, new: 1 }, }) ); @@ -3180,15 +3168,12 @@ describe('Task Runner', () => { expect(actionsClient.bulkEnqueueExecution).toHaveBeenCalledTimes(1); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({ + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({ status: 'warning', outcome: 'warning', warning, - alertsCount: { - active: 2, - new: 2, - }, + alertsCount: { active: 2, new: 2 }, }) ); diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.ts index 936ff2bc64797..eb304e4e8ba8c 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner.ts @@ -42,7 +42,7 @@ import { import { asErr, asOk, isErr, isOk, map, resolveErr, Result } from '../lib/result_type'; import { taskInstanceToAlertTaskInstance } from './alert_task_instance'; import { isAlertSavedObjectNotFoundError, isEsUnavailableError } from '../lib/is_alerting_error'; -import { partiallyUpdateRule, RULE_SAVED_OBJECT_TYPE } from '../saved_objects'; +import { RULE_SAVED_OBJECT_TYPE } from '../saved_objects'; import { AlertInstanceContext, AlertInstanceState, @@ -69,6 +69,7 @@ import { filterMaintenanceWindowsIds, getMaintenanceWindows } from './get_mainte import { RuleTypeRunner } from './rule_type_runner'; import { initializeAlertsClient } from '../alerts_client'; import { createTaskRunnerLogger, withAlertingSpan, processRunResults } from './lib'; +import { partiallyUpdateRuleWithEs } from '../saved_objects/partially_update_rule'; const FALLBACK_RETRY_INTERVAL = '5m'; const CONNECTIVITY_RETRY_INTERVAL = '5m'; @@ -208,7 +209,6 @@ export class TaskRunner< private async updateRuleSavedObjectPostRun( ruleId: string, - namespace: string | undefined, attributes: { executionStatus?: RawRuleExecutionStatus; monitoring?: RawRuleMonitoring; @@ -216,7 +216,7 @@ export class TaskRunner< lastRun?: RawRuleLastRun | null; } ) { - const client = this.internalSavedObjectsRepository; + const client = this.context.elasticsearch.client.asInternalUser; try { // Future engineer -> Here we are just checking if we need to wait for // the update of the attribute `running` in the rule's saved object @@ -227,13 +227,12 @@ export class TaskRunner< // eslint-disable-next-line no-empty } catch {} try { - await partiallyUpdateRule( + await partiallyUpdateRuleWithEs( client, ruleId, { ...attributes, running: false }, { ignore404: true, - namespace, refresh: false, } ); @@ -576,7 +575,7 @@ export class TaskRunner< const { executionStatus: execStatus, executionMetrics: execMetrics } = await this.timer.runWithTimer(TaskRunnerTimerSpan.ProcessRuleRun, async () => { const { - params: { alertId: ruleId, spaceId }, + params: { alertId: ruleId }, startedAt, schedule: taskSchedule, } = this.taskInstance; @@ -588,8 +587,6 @@ export class TaskRunner< nextRun = getNextRun({ startDate: startedAt, interval: taskSchedule.interval }); } - const namespace = this.context.spaceIdToNamespace(spaceId); - const { executionStatus, executionMetrics, lastRun, outcome } = processRunResults({ logger: this.logger, logPrefix: `${this.ruleType.id}:${ruleId}`, @@ -630,7 +627,7 @@ export class TaskRunner< )} - ${JSON.stringify(lastRun)}` ); } - await this.updateRuleSavedObjectPostRun(ruleId, namespace, { + await this.updateRuleSavedObjectPostRun(ruleId, { executionStatus: ruleExecutionStatusToRaw(executionStatus), nextRun, lastRun: lastRunToRaw(lastRun), @@ -786,11 +783,10 @@ export class TaskRunner< // Write event log entry const { - params: { alertId: ruleId, spaceId, consumer }, + params: { alertId: ruleId, consumer }, schedule: taskSchedule, startedAt, } = this.taskInstance; - const namespace = this.context.spaceIdToNamespace(spaceId); if (consumer && !this.ruleConsumer) { this.ruleConsumer = consumer; @@ -831,7 +827,7 @@ export class TaskRunner< `Updating rule task for ${this.ruleType.id} rule with id ${ruleId} - execution error due to timeout` ); const outcome = 'failed'; - await this.updateRuleSavedObjectPostRun(ruleId, namespace, { + await this.updateRuleSavedObjectPostRun(ruleId, { executionStatus: ruleExecutionStatusToRaw(executionStatus), lastRun: { outcome, diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner_alerts_client.test.ts b/x-pack/plugins/alerting/server/task_runner/task_runner_alerts_client.test.ts index cee8a2ea3b345..5c762ce3971a9 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner_alerts_client.test.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner_alerts_client.test.ts @@ -45,7 +45,7 @@ import { RULE_NAME, generateRunnerResult, RULE_ACTIONS, - generateSavedObjectParams, + generateRuleUpdateParams, mockTaskInstance, DATE_1970, DATE_1970_5_MIN, @@ -354,8 +354,8 @@ describe('Task Runner', () => { { tags: ['1', 'test'] } ); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({}) + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({}) ); expect(taskRunnerFactoryInitializerParams.executionContext.withContext).toBeCalledTimes(1); @@ -488,8 +488,8 @@ describe('Task Runner', () => { 'ruleRunMetrics for test:1: {"numSearches":3,"totalSearchDurationMs":23423,"esSearchDurationMs":33,"numberOfTriggeredActions":0,"numberOfGeneratedActions":0,"numberOfActiveAlerts":0,"numberOfRecoveredAlerts":0,"numberOfNewAlerts":0,"numberOfDelayedAlerts":0,"hasReachedAlertLimit":false,"hasReachedQueuedActionsLimit":false,"triggeredActionsStatus":"complete"}', { tags: ['1', 'test'] } ); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({}) + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({}) ); expect(taskRunnerFactoryInitializerParams.executionContext.withContext).toBeCalledTimes(1); expect( @@ -682,8 +682,8 @@ describe('Task Runner', () => { tags: ['1', 'test'], }); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({}) + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({}) ); expect(taskRunnerFactoryInitializerParams.executionContext.withContext).toBeCalledTimes(1); @@ -769,8 +769,8 @@ describe('Task Runner', () => { tags: ['1', 'test'], }); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - ...generateSavedObjectParams({}) + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( + ...generateRuleUpdateParams({}) ); expect(taskRunnerFactoryInitializerParams.executionContext.withContext).toBeCalledTimes(1); diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner_cancel.test.ts b/x-pack/plugins/alerting/server/task_runner/task_runner_cancel.test.ts index b9fb6284c3911..1bc2ad15b0968 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner_cancel.test.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner_cancel.test.ts @@ -64,6 +64,7 @@ import { TaskRunnerContext } from './types'; import { backfillClientMock } from '../backfill_client/backfill_client.mock'; import { UntypedNormalizedRuleType } from '../rule_type_registry'; import { rulesSettingsServiceMock } from '../rules_settings/rules_settings_service.mock'; +import { ALERTING_CASES_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server'; jest.mock('uuid', () => ({ v4: () => '5f6aa57d-3e22-484e-bae8-cbed868f4d28', @@ -226,53 +227,57 @@ describe('Task Runner Cancel', () => { testAlertingEventLogCalls({ status: 'ok' }); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledTimes(1); - expect(internalSavedObjectsRepository.update).toHaveBeenCalledWith( - RULE_SAVED_OBJECT_TYPE, - '1', + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledTimes(1); + expect(elasticsearchService.client.asInternalUser.update).toHaveBeenCalledWith( { - executionStatus: { - error: { - message: `test:1: execution cancelled due to timeout - exceeded rule type timeout of 5m`, - reason: 'timeout', - }, - lastDuration: 0, - lastExecutionDate: '1970-01-01T00:00:00.000Z', - status: 'error', - warning: null, - }, - lastRun: { - alertsCount: {}, - outcome: 'failed', - outcomeMsg: [ - 'test:1: execution cancelled due to timeout - exceeded rule type timeout of 5m', - ], - outcomeOrder: 20, - warning: 'timeout', - }, - monitoring: { - run: { - calculated_metrics: { - success_ratio: 0, + id: `alert:1`, + index: ALERTING_CASES_SAVED_OBJECT_INDEX, + doc: { + alert: { + executionStatus: { + error: { + message: `test:1: execution cancelled due to timeout - exceeded rule type timeout of 5m`, + reason: 'timeout', + }, + lastDuration: 0, + lastExecutionDate: '1970-01-01T00:00:00.000Z', + status: 'error', + warning: null, + }, + lastRun: { + alertsCount: {}, + outcome: 'failed', + outcomeMsg: [ + 'test:1: execution cancelled due to timeout - exceeded rule type timeout of 5m', + ], + outcomeOrder: 20, + warning: 'timeout', }, - history: [], - last_run: { - metrics: { - duration: 0, - gap_duration_s: null, - total_alerts_created: null, - total_alerts_detected: null, - total_indexing_duration_ms: null, - total_search_duration_ms: null, + monitoring: { + run: { + calculated_metrics: { + success_ratio: 0, + }, + history: [], + last_run: { + metrics: { + duration: 0, + gap_duration_s: null, + total_alerts_created: null, + total_alerts_detected: null, + total_indexing_duration_ms: null, + total_search_duration_ms: null, + }, + timestamp: '1970-01-01T00:00:00.000Z', + }, }, - timestamp: '1970-01-01T00:00:00.000Z', }, + nextRun: '1970-01-01T00:00:10.000Z', + running: false, }, }, - nextRun: '1970-01-01T00:00:10.000Z', - running: false, }, - { refresh: false, namespace: undefined } + { ignore: [404] } ); expect(mockUsageCounter.incrementCounter).toHaveBeenCalledTimes(1); expect(mockUsageCounter.incrementCounter).toHaveBeenCalledWith({ From d6c59253df221f2157258926fbd97f4e51b6bb97 Mon Sep 17 00:00:00 2001 From: Ying Date: Wed, 18 Sep 2024 13:37:20 -0400 Subject: [PATCH 2/5] Cleanup --- x-pack/plugins/alerting/server/saved_objects/index.ts | 2 +- .../alerting/server/saved_objects/partially_update_rule.ts | 1 - x-pack/plugins/alerting/server/task_runner/task_runner.ts | 3 +-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/alerting/server/saved_objects/index.ts b/x-pack/plugins/alerting/server/saved_objects/index.ts index eb07a84950d14..a3bb0b4f0afe8 100644 --- a/x-pack/plugins/alerting/server/saved_objects/index.ts +++ b/x-pack/plugins/alerting/server/saved_objects/index.ts @@ -23,7 +23,7 @@ import { RawRule } from '../types'; import { getImportWarnings } from './get_import_warnings'; import { isRuleExportable } from './is_rule_exportable'; import { RuleTypeRegistry } from '../rule_type_registry'; -export { partiallyUpdateRule } from './partially_update_rule'; +export { partiallyUpdateRule, partiallyUpdateRuleWithEs } from './partially_update_rule'; import { RULES_SETTINGS_SAVED_OBJECT_TYPE, MAINTENANCE_WINDOW_SAVED_OBJECT_TYPE, diff --git a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts index 329f01ece3dec..bfece166a4d09 100644 --- a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts +++ b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts @@ -72,7 +72,6 @@ export async function partiallyUpdateRule( } // direct, partial update to a rule saved object via ElasticsearchClient -// using namespace set in the client export async function partiallyUpdateRuleWithEs( esClient: ElasticsearchClient, id: string, diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.ts index eb304e4e8ba8c..c9a31fa997f05 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner.ts @@ -42,7 +42,7 @@ import { import { asErr, asOk, isErr, isOk, map, resolveErr, Result } from '../lib/result_type'; import { taskInstanceToAlertTaskInstance } from './alert_task_instance'; import { isAlertSavedObjectNotFoundError, isEsUnavailableError } from '../lib/is_alerting_error'; -import { RULE_SAVED_OBJECT_TYPE } from '../saved_objects'; +import { partiallyUpdateRuleWithEs, RULE_SAVED_OBJECT_TYPE } from '../saved_objects'; import { AlertInstanceContext, AlertInstanceState, @@ -69,7 +69,6 @@ import { filterMaintenanceWindowsIds, getMaintenanceWindows } from './get_mainte import { RuleTypeRunner } from './rule_type_runner'; import { initializeAlertsClient } from '../alerts_client'; import { createTaskRunnerLogger, withAlertingSpan, processRunResults } from './lib'; -import { partiallyUpdateRuleWithEs } from '../saved_objects/partially_update_rule'; const FALLBACK_RETRY_INTERVAL = '5m'; const CONNECTIVITY_RETRY_INTERVAL = '5m'; From 0e92d580941b2a552661f41216fbbc13fc5a182f Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Wed, 18 Sep 2024 17:55:18 +0000 Subject: [PATCH 3/5] [CI] Auto-commit changed files from 'node scripts/notice' --- x-pack/plugins/alerting/tsconfig.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/alerting/tsconfig.json b/x-pack/plugins/alerting/tsconfig.json index c09816222b010..c0951663a8489 100644 --- a/x-pack/plugins/alerting/tsconfig.json +++ b/x-pack/plugins/alerting/tsconfig.json @@ -72,7 +72,8 @@ "@kbn/alerting-state-types", "@kbn/core-security-server", "@kbn/core-http-server", - "@kbn/zod" + "@kbn/zod", + "@kbn/core-saved-objects-base-server-internal" ], "exclude": [ "target/**/*" From 468307830e7ff7d368adf972162708273bebcc65 Mon Sep 17 00:00:00 2001 From: Ying Date: Fri, 20 Sep 2024 10:50:29 -0400 Subject: [PATCH 4/5] Adding allowlist for attributes we can partially update with ES --- .../partially_update_rule.test.ts | 70 ++++++++++++++++--- .../saved_objects/partially_update_rule.ts | 14 +++- 2 files changed, 72 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.test.ts b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.test.ts index f1eabff5d963c..294bc81481540 100644 --- a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.test.ts +++ b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.test.ts @@ -19,6 +19,7 @@ import { elasticsearchServiceMock, savedObjectsClientMock } from '@kbn/core/serv import { RULE_SAVED_OBJECT_TYPE } from '.'; import { ALERTING_CASES_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server'; import { estypes } from '@elastic/elasticsearch'; +import { RuleExecutionStatuses } from '@kbn/alerting-types'; const MockSavedObjectsClientContract = savedObjectsClientMock.create(); const MockISavedObjectsRepository = @@ -119,19 +120,20 @@ describe('partiallyUpdateRuleWithEs', () => { test('should work with no options', async () => { esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); - await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes); + await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributesForEsUpdate); expect(esClient.update).toHaveBeenCalledTimes(1); expect(esClient.update).toHaveBeenCalledWith({ id: `alert:${MockRuleId}`, index: ALERTING_CASES_SAVED_OBJECT_INDEX, doc: { - alert: DefaultAttributes, + alert: DefaultAttributesForEsUpdate, }, }); }); - test('should work with extraneous attributes ', async () => { - const attributes = ExtraneousAttributes as unknown as PartiallyUpdateableRuleAttributes; + test('should strip unallowed attributes ', async () => { + const attributes = + AttributesForEsUpdateWithUnallowedFields as unknown as PartiallyUpdateableRuleAttributes; esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); await partiallyUpdateRuleWithEs(esClient, MockRuleId, attributes); @@ -139,7 +141,7 @@ describe('partiallyUpdateRuleWithEs', () => { id: `alert:${MockRuleId}`, index: ALERTING_CASES_SAVED_OBJECT_INDEX, doc: { - alert: ExtraneousAttributes, + alert: DefaultAttributesForEsUpdate, }, }); }); @@ -155,7 +157,7 @@ describe('partiallyUpdateRuleWithEs', () => { test('should handle the version option', async () => { esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); - await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes, { + await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributesForEsUpdate, { version: 'WzQsMV0=', }); expect(esClient.update).toHaveBeenCalledWith({ @@ -164,7 +166,7 @@ describe('partiallyUpdateRuleWithEs', () => { if_primary_term: 1, if_seq_no: 4, doc: { - alert: DefaultAttributes, + alert: DefaultAttributesForEsUpdate, }, }); }); @@ -172,13 +174,15 @@ describe('partiallyUpdateRuleWithEs', () => { test('should handle the ignore404 option', async () => { esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); - await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes, { ignore404: true }); + await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributesForEsUpdate, { + ignore404: true, + }); expect(esClient.update).toHaveBeenCalledWith( { id: `alert:${MockRuleId}`, index: ALERTING_CASES_SAVED_OBJECT_INDEX, doc: { - alert: DefaultAttributes, + alert: DefaultAttributesForEsUpdate, }, }, { ignore: [404] } @@ -188,14 +192,14 @@ describe('partiallyUpdateRuleWithEs', () => { test('should handle the refresh option', async () => { esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId)); - await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes, { + await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributesForEsUpdate, { refresh: 'wait_for', }); expect(esClient.update).toHaveBeenCalledWith({ id: `alert:${MockRuleId}`, index: ALERTING_CASES_SAVED_OBJECT_INDEX, doc: { - alert: DefaultAttributes, + alert: DefaultAttributesForEsUpdate, }, refresh: 'wait_for', }); @@ -224,6 +228,50 @@ const DefaultAttributes = { const ExtraneousAttributes = { ...DefaultAttributes, foo: 'bar' }; +const DefaultAttributesForEsUpdate = { + running: false, + executionStatus: { + status: 'active' as RuleExecutionStatuses, + lastExecutionDate: '2023-01-01T08:44:40.000Z', + lastDuration: 12, + error: null, + warning: null, + }, + monitoring: { + run: { + calculated_metrics: { + success_ratio: 20, + }, + history: [ + { + success: true, + timestamp: 1640991880000, + duration: 12, + outcome: 'success', + }, + ], + last_run: { + timestamp: '2023-01-01T08:44:40.000Z', + metrics: { + duration: 12, + gap_duration_s: null, + total_alerts_created: null, + total_alerts_detected: null, + total_indexing_duration_ms: null, + total_search_duration_ms: null, + }, + }, + }, + }, +}; + +const AttributesForEsUpdateWithUnallowedFields = { + ...DefaultAttributesForEsUpdate, + alertTypeId: 'foo', + consumer: 'consumer', + randomField: 'bar', +}; + const MockRuleId = 'rule-id'; const MockUpdateValue = { diff --git a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts index bfece166a4d09..d165b5278d31b 100644 --- a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts +++ b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts @@ -71,6 +71,16 @@ export async function partiallyUpdateRule( } } +// Explicit list of attributes that we allow to be partially updated +// There should be no overlap between this list and RuleAttributesIncludedInAAD or RuleAttributesToEncrypt +const RuleAttributesAllowedForPartialUpdate = [ + 'executionStatus', + 'lastRun', + 'monitoring', + 'nextRun', + 'running', +]; + // direct, partial update to a rule saved object via ElasticsearchClient export async function partiallyUpdateRuleWithEs( esClient: ElasticsearchClient, @@ -83,13 +93,15 @@ export async function partiallyUpdateRuleWithEs( ...RuleAttributesToEncrypt, ...RuleAttributesIncludedInAAD, ]); + // ensure we only have attributes that we explicitly allow to be updated + const attributesAllowedForUpdate = pick(attributeUpdates, RuleAttributesAllowedForPartialUpdate); const updateParams = { id: `alert:${id}`, index: ALERTING_CASES_SAVED_OBJECT_INDEX, ...(options.version ? decodeRequestVersion(options.version) : {}), doc: { - alert: attributeUpdates, + alert: attributesAllowedForUpdate, }, ...(options.refresh ? { refresh: options.refresh } : {}), }; From 409a2faa3cbe53161729292eb967358586ac9682 Mon Sep 17 00:00:00 2001 From: Ying Date: Mon, 30 Sep 2024 08:50:26 -0400 Subject: [PATCH 5/5] Adding comment about bypassing audit logging --- .../alerting/server/saved_objects/partially_update_rule.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts index d165b5278d31b..f9b4da5ed767b 100644 --- a/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts +++ b/x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts @@ -82,6 +82,11 @@ const RuleAttributesAllowedForPartialUpdate = [ ]; // direct, partial update to a rule saved object via ElasticsearchClient + +// we do this direct partial update to avoid the overhead of the SavedObjectsClient for +// only these allow-listed fields which don't impact encryption. in addition, because these +// fields are only updated by the system user at the end of a rule run, they should not +// need to be included in any (user-centric) audit logs. export async function partiallyUpdateRuleWithEs( esClient: ElasticsearchClient, id: string,