[Security Solutions] Fixes the newer notification system throttle resets and enabling immediate execution on first detection of a signal

x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts

@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import { elasticsearchServiceMock } from 'src/core/server/mocks';
+import { elasticsearchServiceMock, loggingSystemMock } from 'src/core/server/mocks';
 import { alertsMock } from '../../../../../alerting/server/mocks';
 import { scheduleThrottledNotificationActions } from './schedule_throttle_notification_actions';
 import {
@@ -19,8 +19,10 @@ jest.mock('./schedule_notification_actions', () => ({
 
 describe('schedule_throttle_notification_actions', () => {
   let notificationRuleParams: NotificationRuleTypeParams;
+  let logger: ReturnType<typeof loggingSystemMock.createLogger>;
 
   beforeEach(() => {
+    logger = loggingSystemMock.createLogger();
     (scheduleNotificationActions as jest.Mock).mockReset();
     notificationRuleParams = {
       author: ['123'],
@@ -82,6 +84,38 @@ describe('schedule_throttle_notification_actions', () => {
       ),
       alertInstance: alertsMock.createAlertInstanceFactory(),
       notificationRuleParams,
+      logger,
+      signals: [],
+    });
+
+    expect(scheduleNotificationActions as jest.Mock).toHaveBeenCalled();
+  });
+
+  it('should call "scheduleNotificationActions" if the signals length is 1 or greater', async () => {
+    await scheduleThrottledNotificationActions({
+      throttle: '1d',
+      startedAt: new Date('2021-08-24T19:19:22.094Z'),
+      id: '123',
+      kibanaSiemAppUrl: 'http://www.example.com',
+      outputIndex: 'output-123',
+      ruleId: 'rule-123',
+      esClient: elasticsearchServiceMock.createElasticsearchClient(
+        elasticsearchServiceMock.createSuccessTransportRequestPromise({
+          hits: {
+            hits: [],
+            total: 0,
+          },
+        })
+      ),
+      alertInstance: alertsMock.createAlertInstanceFactory(),
+      notificationRuleParams,
+      logger,
+      signals: [
+        {
+          _id: '123',
+          index: '123',
+        },
+      ],
     });
 
     expect(scheduleNotificationActions as jest.Mock).toHaveBeenCalled();
@@ -105,6 +139,8 @@ describe('schedule_throttle_notification_actions', () => {
       ),
       alertInstance: alertsMock.createAlertInstanceFactory(),
       notificationRuleParams,
+      logger,
+      signals: [],
     });
 
     expect(scheduleNotificationActions as jest.Mock).not.toHaveBeenCalled();
@@ -132,6 +168,8 @@ describe('schedule_throttle_notification_actions', () => {
       ),
       alertInstance: alertsMock.createAlertInstanceFactory(),
       notificationRuleParams,
+      logger,
+      signals: [],
     });
 
     expect(scheduleNotificationActions as jest.Mock).not.toHaveBeenCalled();
@@ -161,6 +199,8 @@ describe('schedule_throttle_notification_actions', () => {
       ),
       alertInstance: alertsMock.createAlertInstanceFactory(),
       notificationRuleParams,
+      logger,
+      signals: [],
     });
 
     expect((scheduleNotificationActions as jest.Mock).mock.calls[0][0].resultsLink).toMatch(
@@ -174,4 +214,71 @@ describe('schedule_throttle_notification_actions', () => {
       })
     );
   });
+
+  it('should log debug information when passing through in expected format and no error messages', async () => {
+    await scheduleThrottledNotificationActions({
+      throttle: '1d',
+      startedAt: new Date('2021-08-24T19:19:22.094Z'),
+      id: '123',
+      kibanaSiemAppUrl: 'http://www.example.com',
+      outputIndex: 'output-123',
+      ruleId: 'rule-123',
+      esClient: elasticsearchServiceMock.createElasticsearchClient(
+        elasticsearchServiceMock.createSuccessTransportRequestPromise({
+          hits: {
+            hits: [
+              {
+                _source: {},
+              },
+            ],
+            total: 1,
+          },
+        })
+      ),
+      alertInstance: alertsMock.createAlertInstanceFactory(),
+      notificationRuleParams,
+      logger,
+      signals: [],
+    });
+    // We only test the first part since it has date math using math
+    expect(logger.debug.mock.calls[0][0]).toMatch(
+      /The notification throttle resultsLink created is/
+    );
+    expect(logger.debug.mock.calls[1][0]).toEqual(
+      'The notification throttle query result size before deconflicting duplicates is: 1. The notification throttle passed in signals size before deconflicting duplicates is: 0. The deconflicted size and size of the signals sent into throttle notification is: 1.'
+    );
+    // error should not have been called in this case.
+    expect(logger.error).not.toHaveBeenCalled();
+  });
+
+  it('should log error information if "throttle" is an invalid string', async () => {
+    await scheduleThrottledNotificationActions({
+      throttle: 'invalid',
+      startedAt: new Date('2021-08-24T19:19:22.094Z'),
+      id: '123',
+      kibanaSiemAppUrl: 'http://www.example.com',
+      outputIndex: 'output-123',
+      ruleId: 'rule-123',
+      esClient: elasticsearchServiceMock.createElasticsearchClient(
+        elasticsearchServiceMock.createSuccessTransportRequestPromise({
+          hits: {
+            hits: [
+              {
+                _source: {},
+              },
+            ],
+            total: 1,
+          },
+        })
+      ),
+      alertInstance: alertsMock.createAlertInstanceFactory(),
+      notificationRuleParams,
+      logger,
+      signals: [],
+    });
+
+    expect(logger.error).toHaveBeenCalledWith(
+      'The notification throttle "from" and/or "to" range values could be constructed as valid. Tried to construct the values of "from": now-invalid "to": 2021-08-24T19:19:22.094Z. This will cause a reset of the notification throttle. Expect either missing alert notifications or alert notifications happening earlier than expected.'
+    );
+  });
 });

x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts

@@ -5,11 +5,11 @@
  * 2.0.
  */
 
-import { ElasticsearchClient, SavedObject } from 'src/core/server';
+import { ElasticsearchClient, SavedObject, Logger } from 'src/core/server';
 import { parseScheduleDates } from '@kbn/securitysolution-io-ts-utils';
 import { AlertInstance } from '../../../../../alerting/server';
 import { RuleParams } from '../schemas/rule_schemas';
-import { getNotificationResultsLink } from '../notifications/utils';
+import { deconflictSignalsAndResults, getNotificationResultsLink } from '../notifications/utils';
 import { DEFAULT_RULE_NOTIFICATION_QUERY_SIZE } from '../../../../common/constants';
 import { getSignals } from '../notifications/get_signals';
 import {
@@ -18,8 +18,25 @@ import {
 } from './schedule_notification_actions';
 import { AlertAttributes } from '../signals/types';
 
+interface ScheduleThrottledNotificationActionsOptions {
+  id: SavedObject['id'];
+  startedAt: Date;
+  throttle: AlertAttributes['throttle'];
+  kibanaSiemAppUrl: string | undefined;
+  outputIndex: RuleParams['outputIndex'];
+  ruleId: RuleParams['ruleId'];
+  esClient: ElasticsearchClient;
+  alertInstance: AlertInstance;
+  notificationRuleParams: NotificationRuleTypeParams;
+  signals: unknown[];
+  logger: Logger;
+}
+
 /**
  * Schedules a throttled notification action for executor rules.
+ * NOTE: It's important that since this is throttled that you call this in _ALL_ cases including error conditions or results being empty or not a success.
+ * If you do not call this within your rule executor then this will cause a "reset" and will stop "throttling" and the next call will cause an immediate action
+ * to be sent through the system.
  * @param throttle The throttle which is the alerting saved object throttle
  * @param startedAt When the executor started at
  * @param id The id the alert which caused the notifications
@@ -40,17 +57,9 @@ export const scheduleThrottledNotificationActions = async ({
   esClient,
   alertInstance,
   notificationRuleParams,
-}: {
-  id: SavedObject['id'];
-  startedAt: Date;
-  throttle: AlertAttributes['throttle'];
-  kibanaSiemAppUrl: string | undefined;
-  outputIndex: RuleParams['outputIndex'];
-  ruleId: RuleParams['ruleId'];
-  esClient: ElasticsearchClient;
-  alertInstance: AlertInstance;
-  notificationRuleParams: NotificationRuleTypeParams;
-}): Promise<void> => {
+  signals,
+  logger,
+}: ScheduleThrottledNotificationActionsOptions): Promise<void> => {
   const fromInMs = parseScheduleDates(`now-${throttle}`);
   const toInMs = parseScheduleDates(startedAt.toISOString());
 
@@ -62,6 +71,22 @@ export const scheduleThrottledNotificationActions = async ({
       kibanaSiemAppUrl,
     });
 
+    logger.debug(
+      [
+        `The notification throttle resultsLink created is: ${resultsLink}.`,
+        ' Notification throttle is querying the results using',
+        ` "from:" ${fromInMs.valueOf()}`,
+        ' "to":',
+        ` ${toInMs.valueOf()}`,
+        ' "size":',
+        ` ${DEFAULT_RULE_NOTIFICATION_QUERY_SIZE}`,
+        ' "index":',
+        ` ${outputIndex}`,
+        ' "ruleId":',
+        ` ${ruleId}`,
+      ].join('')
+    );
+
     const results = await getSignals({
       from: `${fromInMs.valueOf()}`,
       to: `${toInMs.valueOf()}`,
@@ -70,19 +95,43 @@ export const scheduleThrottledNotificationActions = async ({
       ruleId,
       esClient,
     });
+    const resultsFlattened = results.hits.hits.map((hit) => {
+      return {
+        _id: hit._id,
+        _index: hit._index,
+        ...hit._source,
+      };
+    });
+    const deconflicted = deconflictSignalsAndResults({
+      logger,
+      signals,
+      querySignals: resultsFlattened,
+    });
+    logger.debug(
+      [
+        `The notification throttle query result size before deconflicting duplicates is: ${results.hits.hits.length}.`,
+        ` The notification throttle passed in signals size before deconflicting duplicates is: ${signals.length}.`,
+        ` The deconflicted size and size of the signals sent into throttle notification is: ${deconflicted.length}.`,
+      ].join('')
+    );
 
-    const signalsCount =
-      typeof results.hits.total === 'number' ? results.hits.total : results.hits.total.value;
-
-    const signals = results.hits.hits.map((hit) => hit._source);
-    if (results.hits.hits.length !== 0) {
+    if (deconflicted.length !== 0) {
       scheduleNotificationActions({
         alertInstance,
-        signalsCount,
-        signals,
+        signalsCount: deconflicted.length,
+        signals: deconflicted,
         resultsLink,
         ruleParams: notificationRuleParams,
       });
     }
+  } else {
+    logger.error(
+      [
+        'The notification throttle "from" and/or "to" range values could be constructed as valid. Tried to construct the values of',
+        ` "from": now-${throttle}`,
+        ` "to": ${startedAt.toISOString()}.`,
+        ' This will cause a reset of the notification throttle. Expect either missing alert notifications or alert notifications happening earlier than expected.',
+      ].join('')
+    );
   }
 };