From 3d8c53bb73375bc2a9d5f89463f73ced5e7e4465 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Sat, 11 May 2024 12:11:53 +0300 Subject: [PATCH 1/5] Change logger level to debug --- .../server/alerts_service/lib/set_alerts_to_untracked.ts | 2 +- .../alerting/server/rules_client/lib/untrack_rule_alerts.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts b/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts index c28a760853ba62..6d4ff82179b081 100644 --- a/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts +++ b/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts @@ -253,7 +253,7 @@ export async function setAlertsToUntracked( }); return searchResponse.hits.hits.map((hit) => hit._source) as UntrackedAlertsResult; } catch (err) { - logger.error( + logger.debug( `Error marking ${ruleIds ? 'ruleIds' : 'alertUuids'} ${ ruleIds ? ruleIds : alertUuids } as untracked - ${err.message}` diff --git a/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts b/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts index d69d84c7fd8c73..7f497f529bff95 100644 --- a/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts +++ b/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts @@ -86,7 +86,7 @@ export const untrackRuleAlerts = async ( } } catch (error) { // this should not block the rest of the disable process - context.logger.warn( + context.logger.debug( `rulesClient.disable('${id}') - Could not write untrack events - ${error.message}` ); } From 45adbe32b78f5e1ffbddc1b19fe955e19d013193 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Mon, 13 May 2024 11:35:04 +0300 Subject: [PATCH 2/5] Fix unit test --- .../plugins/alerting/server/rules_client/tests/disable.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts b/x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts index 35e24eb9af4401..463fc7cf806b33 100644 --- a/x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts +++ b/x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts @@ -440,7 +440,7 @@ describe('disable()', () => { expect(taskManager.removeIfExists).not.toHaveBeenCalledWith(); expect(eventLogger.logEvent).toHaveBeenCalledTimes(0); - expect(rulesClientParams.logger.warn).toHaveBeenCalledWith( + expect(rulesClientParams.logger.debug).toHaveBeenCalledWith( `rulesClient.disable('1') - Could not write untrack events - Fail` ); }); From e0c65fe376118b414c017596e291d60a1157249b Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Tue, 14 May 2024 16:15:03 +0300 Subject: [PATCH 3/5] Do not throw when there are no alerts to untrack --- .../lib/set_alerts_to_untracked.test.ts | 97 +++++++++++++++++++ .../lib/set_alerts_to_untracked.ts | 14 ++- .../rules_client/lib/untrack_rule_alerts.ts | 4 +- 3 files changed, 112 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.test.ts b/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.test.ts index 691fc8548c098f..49ece72c42f955 100644 --- a/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.test.ts +++ b/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.test.ts @@ -426,6 +426,8 @@ describe('setAlertsToUntracked()', () => { }, }); + clusterClient.updateByQuery.mockResponseOnce({ total: 2, updated: 2 }); + const result = await setAlertsToUntracked({ isUsingQuery: true, query: [ @@ -523,4 +525,99 @@ describe('setAlertsToUntracked()', () => { }, ]); }); + + test('should return an empty array if the search returns zero results', async () => { + getAuthorizedRuleTypesMock.mockResolvedValue([ + { + id: 'test-rule-type', + }, + ]); + getAlertIndicesAliasMock.mockResolvedValue(['test-alert-index']); + + clusterClient.search.mockResponseOnce({ + took: 1, + timed_out: false, + _shards: { + total: 1, + successful: 1, + skipped: 0, + failed: 0, + }, + hits: { + hits: [], + }, + aggregations: { + ruleTypeIds: { + buckets: [ + { + key: 'some rule type', + consumers: { + buckets: [{ key: 'o11y' }], + }, + }, + ], + }, + }, + }); + + clusterClient.updateByQuery.mockResponseOnce({ total: 0, updated: 0 }); + + const result = await setAlertsToUntracked({ + isUsingQuery: true, + query: [ + { + bool: { + must: { + term: { + 'kibana.alert.rule.name': 'test', + }, + }, + }, + }, + ], + featureIds: ['o11y'], + spaceId: 'default', + getAuthorizedRuleTypes: getAuthorizedRuleTypesMock, + getAlertIndicesAlias: getAlertIndicesAliasMock, + ensureAuthorized: ensureAuthorizedMock, + logger, + esClient: clusterClient, + }); + + expect(getAlertIndicesAliasMock).lastCalledWith(['test-rule-type'], 'default'); + + expect(clusterClient.updateByQuery).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.objectContaining({ + query: { + bool: { + must: [ + { + term: { + 'kibana.alert.status': { + value: 'active', // This has to be active + }, + }, + }, + ], + filter: [ + { + bool: { + must: { + term: { + 'kibana.alert.rule.name': 'test', + }, + }, + }, + }, + ], + }, + }, + }), + }) + ); + + expect(clusterClient.search).not.toHaveBeenCalledWith(); + expect(result).toEqual([]); + }); }); diff --git a/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts b/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts index 6d4ff82179b081..ed0c2cb21e06b8 100644 --- a/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts +++ b/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts @@ -208,6 +208,7 @@ export async function setAlertsToUntracked( // Retry this updateByQuery up to 3 times to make sure the number of documents // updated equals the number of documents matched let total = 0; + for (let retryCount = 0; retryCount < 3; retryCount++) { const response = await esClient.updateByQuery({ index: indices, @@ -224,14 +225,18 @@ export async function setAlertsToUntracked( }); if (total === 0 && response.total === 0) { - throw new Error('No active alerts matched the query'); + logger.debug('No active alerts matched the query'); + break; } + if (response.total) { total = response.total; } + if (response.total === response.updated) { break; } + logger.warn( `Attempt ${retryCount + 1}: Failed to untrack ${ (response.total ?? 0) - (response.updated ?? 0) @@ -241,6 +246,10 @@ export async function setAlertsToUntracked( ); } + if (total === 0) { + return []; + } + // Fetch and return updated rule and alert instance UUIDs const searchResponse = await esClient.search({ index: indices, @@ -251,9 +260,10 @@ export async function setAlertsToUntracked( query: getUntrackQuery(params, ALERT_STATUS_UNTRACKED), }, }); + return searchResponse.hits.hits.map((hit) => hit._source) as UntrackedAlertsResult; } catch (err) { - logger.debug( + logger.error( `Error marking ${ruleIds ? 'ruleIds' : 'alertUuids'} ${ ruleIds ? ruleIds : alertUuids } as untracked - ${err.message}` diff --git a/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts b/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts index 7f497f529bff95..76dd34a7de68a3 100644 --- a/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts +++ b/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts @@ -24,11 +24,13 @@ export const untrackRuleAlerts = async ( ) => { return withSpan({ name: 'untrackRuleAlerts', type: 'rules' }, async () => { if (!context.eventLogger || !attributes.scheduledTaskId) return; + try { const taskInstance = taskInstanceToAlertTaskInstance( await context.taskManager.get(attributes.scheduledTaskId), attributes as unknown as SanitizedRule ); + const { state } = taskInstance; const untrackedAlerts = mapValues, Alert>( @@ -86,7 +88,7 @@ export const untrackRuleAlerts = async ( } } catch (error) { // this should not block the rest of the disable process - context.logger.debug( + context.logger.warn( `rulesClient.disable('${id}') - Could not write untrack events - ${error.message}` ); } From 45bef8edfb32a94e443252e09b3193380546c71c Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Wed, 15 May 2024 11:25:21 +0300 Subject: [PATCH 4/5] Skip update task state with no taskIds and log success --- .../bulk_untrack/bulk_untrack_alerts.test.ts | 59 +++++++++++++++++++ .../bulk_untrack/bulk_untrack_alerts.ts | 25 +++++--- 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.test.ts b/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.test.ts index 4e2077f15a899b..befd240286a061 100644 --- a/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.test.ts +++ b/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.test.ts @@ -70,7 +70,9 @@ const rulesClientParams: jest.Mocked = { describe('bulkUntrackAlerts()', () => { let rulesClient: RulesClient; + beforeEach(async () => { + jest.clearAllMocks(); rulesClient = new RulesClient(rulesClientParams); }); @@ -241,4 +243,61 @@ describe('bulkUntrackAlerts()', () => { } `); }); + + it('should not call bulkUpdateState with no taskIds', async () => { + alertsService.setAlertsToUntracked.mockResolvedValueOnce([]); + + await rulesClient.bulkUntrackAlerts({ + isUsingQuery: true, + indices: ['test-index'], + alertUuids: ['my-uuid'], + }); + + expect(alertsService.setAlertsToUntracked).toHaveBeenCalledTimes(1); + expect(taskManager.bulkUpdateState).not.toHaveBeenCalledWith(); + }); + + it('filters out undefined rule uuids', async () => { + alertsService.setAlertsToUntracked.mockResolvedValueOnce([{}, { foo: 'bar' }]); + + await rulesClient.bulkUntrackAlerts({ + isUsingQuery: true, + indices: ['test-index'], + alertUuids: ['my-uuid'], + }); + + expect(alertsService.setAlertsToUntracked).toHaveBeenCalledTimes(1); + expect(taskManager.bulkUpdateState).not.toHaveBeenCalledWith(); + }); + + it('should audit log success with no taskIds', async () => { + alertsService.setAlertsToUntracked.mockResolvedValueOnce([]); + + await rulesClient.bulkUntrackAlerts({ + isUsingQuery: true, + indices: ['test-index'], + alertUuids: ['my-uuid'], + }); + + expect(taskManager.bulkUpdateState).not.toHaveBeenCalledWith(); + expect(auditLogger.log.mock.calls[0][0]).toMatchInlineSnapshot(` + Object { + "error": undefined, + "event": Object { + "action": "rule_alert_untrack", + "category": Array [ + "database", + ], + "outcome": "success", + "type": Array [ + "change", + ], + }, + "kibana": Object { + "saved_object": undefined, + }, + "message": "User has untracked a rule", + } + `); + }); }); diff --git a/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.ts b/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.ts index d3a1badd0ab330..304037782f6d39 100644 --- a/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.ts +++ b/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.ts @@ -9,6 +9,7 @@ import { omitBy } from 'lodash'; import Boom from '@hapi/boom'; import { withSpan } from '@kbn/apm-utils'; import { ALERT_RULE_UUID, ALERT_UUID } from '@kbn/rule-data-utils'; +import { AuditLogger } from '@kbn/core-security-server'; import { bulkUntrackBodySchema } from './schemas'; import type { BulkUntrackBody } from './types'; import { WriteOperations, AlertingAuthorizationEntity } from '../../../../authorization'; @@ -64,7 +65,13 @@ async function bulkUntrackAlertsWithOCC(context: RulesClientContext, params: Bul }); // Clear alert instances from their corresponding tasks so that they can remain untracked - const taskIds = [...new Set(result.map((doc) => doc[ALERT_RULE_UUID]))]; + const taskIds = [...new Set(result.map((doc) => doc[ALERT_RULE_UUID]).filter(Boolean))]; + + if (taskIds.length === 0) { + auditLogSuccess(context.auditLogger); + return; + } + await context.taskManager.bulkUpdateState(taskIds, (state, id) => { try { const uuidsToClear = result @@ -90,12 +97,7 @@ async function bulkUntrackAlertsWithOCC(context: RulesClientContext, params: Bul } }); - context.auditLogger?.log( - ruleAuditEvent({ - action: RuleAuditAction.UNTRACK_ALERT, - outcome: 'success', - }) - ); + auditLogSuccess(context.auditLogger); } catch (error) { context.auditLogger?.log( ruleAuditEvent({ @@ -106,3 +108,12 @@ async function bulkUntrackAlertsWithOCC(context: RulesClientContext, params: Bul throw error; } } + +const auditLogSuccess = (auditLogger?: AuditLogger) => { + auditLogger?.log( + ruleAuditEvent({ + action: RuleAuditAction.UNTRACK_ALERT, + outcome: 'success', + }) + ); +}; From c94b4530eddf1383d5d608d770ac17f84e694d27 Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Wed, 15 May 2024 08:33:03 +0000 Subject: [PATCH 5/5] [CI] Auto-commit changed files from 'node scripts/lint_ts_projects --fix' --- x-pack/plugins/alerting/tsconfig.json | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugins/alerting/tsconfig.json b/x-pack/plugins/alerting/tsconfig.json index f76c6d781ad830..1bc43c34e5aa8e 100644 --- a/x-pack/plugins/alerting/tsconfig.json +++ b/x-pack/plugins/alerting/tsconfig.json @@ -70,6 +70,7 @@ "@kbn/core-execution-context-server-mocks", "@kbn/react-kibana-context-render", "@kbn/search-types", + "@kbn/core-security-server", ], "exclude": [ "target/**/*"