[7.x] [Security Solution] [Detections] Replace 'partial failure' with…

… 'warning' for rule statuses (elastic#91167) (elastic#91598) * removes usage of 'partial failure' status and replaces with a 'warning' status, also adds some logic to be backwards compatible with 'partial failure' statuses * update integration tests from 'partial failure' to 'warning' * fix integration test to warn and not error when no index patterns match concrete indices * fix integration test * removes outdated comments from the create_rules e2e test # Conflicts: # x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts
andrew-goldstein · Feb 17, 2021 · 06580cd · 06580cd
1 parent 61d0ae3
commit 06580cd
Show file tree

Hide file tree

Showing 16 changed files with 71 additions and 45 deletions.
diff --git a/x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts b/x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts
@@ -325,7 +325,7 @@ export const job_status = t.keyof({
   succeeded: null,
   failed: null,
   'going to run': null,
-  'partial failure': null,
+  warning: null,
 });
 export type JobStatus = t.TypeOf<typeof job_status>;
 

diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/rule_status/helpers.ts b/x-pack/plugins/security_solution/public/detections/components/rules/rule_status/helpers.ts
@@ -14,6 +14,9 @@ export const getStatusColor = (status: RuleStatusType | string | null) =>
     ? 'success'
     : status === 'failed'
     ? 'danger'
-    : status === 'executing' || status === 'going to run' || status === 'partial failure'
+    : status === 'executing' ||
+      status === 'going to run' ||
+      status === 'partial failure' ||
+      status === 'warning'
     ? 'warning'
     : 'subdued';
diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/rule_status/index.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/rule_status/index.tsx
@@ -16,7 +16,11 @@ import {
 import React, { memo, useCallback, useEffect, useState } from 'react';
 import deepEqual from 'fast-deep-equal';
 
-import { useRuleStatus, RuleInfoStatus } from '../../../containers/detection_engine/rules';
+import {
+  useRuleStatus,
+  RuleInfoStatus,
+  RuleStatusType,
+} from '../../../containers/detection_engine/rules';
 import { FormattedDate } from '../../../../common/components/formatted_date';
 import { getEmptyTagValue } from '../../../../common/components/empty_value';
 import { getStatusColor } from './helpers';
@@ -55,6 +59,19 @@ const RuleStatusComponent: React.FC<RuleStatusProps> = ({ ruleId, ruleEnabled })
     }
   }, [fetchRuleStatus, ruleId]);
 
+  const getStatus = useCallback((status: RuleStatusType | null | undefined) => {
+    if (status == null) {
+      return getEmptyTagValue();
+    } else if (status != null && status === 'partial failure') {
+      // Temporary fix if on upgrade a rule has a status of 'partial failure' we want to display that text as 'warning'
+      // On the next subsequent rule run, that 'partial failure' status will be re-written as a 'warning' status
+      // and this code will no longer be necessary
+      // TODO: remove this code in 8.0.0
+      return 'warning';
+    }
+    return status;
+  }, []);
+
   return (
     <EuiFlexGroup gutterSize="xs" alignItems="center" justifyContent="flexStart">
       <EuiFlexItem grow={false}>
@@ -71,7 +88,7 @@ const RuleStatusComponent: React.FC<RuleStatusProps> = ({ ruleId, ruleEnabled })
           <EuiFlexItem grow={false}>
             <EuiHealth color={getStatusColor(currentStatus?.status ?? null)}>
               <EuiText data-test-subj="ruleStatus" size="xs">
-                {currentStatus?.status ?? getEmptyTagValue()}
+                {getStatus(currentStatus?.status)}
               </EuiText>
             </EuiHealth>
           </EuiFlexItem>

diff --git a/...ck/plugins/security_solution/public/detections/containers/detection_engine/rules/types.ts b/...ck/plugins/security_solution/public/detections/containers/detection_engine/rules/types.ts
@@ -78,6 +78,7 @@ const StatusTypes = t.union([
   t.literal('failed'),
   t.literal('going to run'),
   t.literal('partial failure'),
+  t.literal('warning'),
 ]);
 
 // TODO: make a ticket
@@ -254,7 +255,13 @@ export interface RuleStatus {
   failures: RuleInfoStatus[];
 }
 
-export type RuleStatusType = 'executing' | 'failed' | 'going to run' | 'succeeded';
+export type RuleStatusType =
+  | 'executing'
+  | 'failed'
+  | 'going to run'
+  | 'succeeded'
+  | 'partial failure'
+  | 'warning';
 export interface RuleInfoStatus {
   alert_id: string;
   status_date: string;

diff --git a/.../plugins/security_solution/public/detections/pages/detection_engine/rules/all/columns.tsx b/.../plugins/security_solution/public/detections/pages/detection_engine/rules/all/columns.tsx
@@ -313,7 +313,11 @@ export const getMonitoringColumns = (
             }}
             href={formatUrl(getRuleDetailsUrl(item.id))}
           >
-            {value}
+            {/* Temporary fix if on upgrade a rule has a status of 'partial failure' we want to display that text as 'warning' */}
+            {/* On the next subsequent rule run, that 'partial failure' status will be re-written as a 'warning' status */}
+            {/* and this code will no longer be necessary */}
+            {/* TODO: remove this code in 8.0.0 */}
+            {value === 'partial failure' ? 'warning' : value}
           </LinkAnchor>
         );
       },

diff --git a/...lugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx b/...lugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx
@@ -314,7 +314,7 @@ const RuleDetailsPageComponent = () => {
         />
       );
     } else if (
-      rule?.status === 'partial failure' &&
+      (rule?.status === 'warning' || rule?.status === 'partial failure') &&
       ruleDetailTab === RuleDetailTabs.alerts &&
       rule?.last_success_at != null
     ) {

diff --git a/.../security_solution/public/detections/pages/detection_engine/rules/details/translations.ts b/.../security_solution/public/detections/pages/detection_engine/rules/details/translations.ts
@@ -52,7 +52,7 @@ export const ERROR_CALLOUT_TITLE = i18n.translate(
 export const PARTIAL_FAILURE_CALLOUT_TITLE = i18n.translate(
   'xpack.securitySolution.detectionEngine.ruleDetails.partialErrorCalloutTitle',
   {
-    defaultMessage: 'Partial rule failure at',
+    defaultMessage: 'Warning at',
   }
 );
 

diff --git a/...plugins/security_solution/server/lib/detection_engine/signals/rule_status_service.mock.ts b/...plugins/security_solution/server/lib/detection_engine/signals/rule_status_service.mock.ts
@@ -23,7 +23,7 @@ export const ruleStatusServiceFactoryMock = async ({
 
     success: jest.fn(),
 
-    partialFailure: jest.fn(),
+    warning: jest.fn(),
 
     error: jest.fn(),
   };

diff --git a/...plugins/security_solution/server/lib/detection_engine/signals/rule_status_service.test.ts b/...plugins/security_solution/server/lib/detection_engine/signals/rule_status_service.test.ts
@@ -54,13 +54,13 @@ describe('buildRuleStatusAttributes', () => {
     expect(result.statusDate).toEqual(result.lastSuccessAt);
   });
 
-  it('returns partial failure fields if "partial failure"', () => {
+  it('returns warning fields if "warning"', () => {
     const result = buildRuleStatusAttributes(
-      'partial failure',
+      'warning',
       'some indices missing timestamp override field'
     );
     expect(result).toEqual({
-      status: 'partial failure',
+      status: 'warning',
       statusDate: expectIsoDateString,
       lastSuccessAt: expectIsoDateString,
       lastSuccessMessage: 'some indices missing timestamp override field',

diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/rule_status_service.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/rule_status_service.ts
@@ -24,7 +24,7 @@ interface Attributes {
 export interface RuleStatusService {
   goingToRun: () => Promise<void>;
   success: (message: string, attributes?: Attributes) => Promise<void>;
-  partialFailure: (message: string, attributes?: Attributes) => Promise<void>;
+  warning: (message: string, attributes?: Attributes) => Promise<void>;
   error: (message: string, attributes?: Attributes) => Promise<void>;
 }
 
@@ -48,7 +48,7 @@ export const buildRuleStatusAttributes: (
         lastSuccessMessage: message,
       };
     }
-    case 'partial failure': {
+    case 'warning': {
       return {
         ...baseAttributes,
         lastSuccessAt: now,
@@ -102,15 +102,15 @@ export const ruleStatusServiceFactory = async ({
       });
     },
 
-    partialFailure: async (message, attributes) => {
+    warning: async (message, attributes) => {
       const [currentStatus] = await getOrCreateRuleStatuses({
         alertId,
         ruleStatusClient,
       });
 
       await ruleStatusClient.update(currentStatus.id, {
         ...currentStatus.attributes,
-        ...buildRuleStatusAttributes('partial failure', message, attributes),
+        ...buildRuleStatusAttributes('warning', message, attributes),
       });
     },
 

diff --git a/...gins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts b/...gins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts
@@ -110,7 +110,7 @@ describe('rules_notification_alert_type', () => {
       find: jest.fn(),
       goingToRun: jest.fn(),
       error: jest.fn(),
-      partialFailure: jest.fn(),
+      warning: jest.fn(),
     };
     (ruleStatusServiceFactory as jest.Mock).mockReturnValue(ruleStatusService);
     (getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(0));
@@ -207,7 +207,7 @@ describe('rules_notification_alert_type', () => {
       });
     });
 
-    it('should set a partial failure for when rules cannot read ALL provided indices', async () => {
+    it('should set a warning for when rules cannot read ALL provided indices', async () => {
       (checkPrivileges as jest.Mock).mockResolvedValueOnce({
         username: 'elastic',
         has_all_requested: false,
@@ -227,8 +227,8 @@ describe('rules_notification_alert_type', () => {
       });
       payload.params.index = ['some*', 'myfa*', 'anotherindex*'];
       await alert.executor(payload);
-      expect(ruleStatusService.partialFailure).toHaveBeenCalled();
-      expect(ruleStatusService.partialFailure.mock.calls[0][0]).toContain(
+      expect(ruleStatusService.warning).toHaveBeenCalled();
+      expect(ruleStatusService.warning.mock.calls[0][0]).toContain(
         'Missing required read privileges on the following indices: ["some*"]'
       );
     });
@@ -250,8 +250,8 @@ describe('rules_notification_alert_type', () => {
       });
       payload.params.index = ['some*', 'myfa*'];
       await alert.executor(payload);
-      expect(ruleStatusService.partialFailure).toHaveBeenCalled();
-      expect(ruleStatusService.partialFailure.mock.calls[0][0]).toContain(
+      expect(ruleStatusService.warning).toHaveBeenCalled();
+      expect(ruleStatusService.warning.mock.calls[0][0]).toContain(
         'This rule may not have the required read privileges to the following indices: ["myfa*","some*"]'
       );
     });

diff --git a/...k/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts b/...k/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts
@@ -181,7 +181,7 @@ export const signalRulesAlertType = ({
 
       logger.debug(buildRuleMessage('[+] Starting Signal Rule execution'));
       logger.debug(buildRuleMessage(`interval: ${interval}`));
-      let wrotePartialFailureStatus = false;
+      let wroteWarningStatus = false;
       await ruleStatusService.goingToRun();
 
       // check if rule has permissions to access given index pattern
@@ -202,7 +202,7 @@ export const signalRulesAlertType = ({
             }),
           ]);
 
-          wrotePartialFailureStatus = await flow(
+          wroteWarningStatus = await flow(
             () =>
               tryCatch(
                 () =>
@@ -659,7 +659,7 @@ export const signalRulesAlertType = ({
               `[+] Finished indexing ${result.createdSignalsCount} signals into ${outputIndex}`
             )
           );
-          if (!hasError && !wrotePartialFailureStatus) {
+          if (!hasError && !wroteWarningStatus) {
             await ruleStatusService.success('succeeded', {
               bulkCreateTimeDurations: result.bulkCreateTimes,
               searchAfterTimeDurations: result.searchAfterTimes,

diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.test.ts
@@ -69,7 +69,7 @@ const ruleStatusServiceMock = {
   find: jest.fn(),
   goingToRun: jest.fn(),
   error: jest.fn(),
-  partialFailure: jest.fn(),
+  warning: jest.fn(),
 };
 
 describe('utils', () => {

diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts
@@ -79,12 +79,12 @@ export const hasReadIndexPrivileges = async (
 
   if (indexesWithReadPrivileges.length > 0 && indexesWithNoReadPrivileges.length > 0) {
     // some indices have read privileges others do not.
-    // set a partial failure status
+    // set a warning status
     const errorString = `Missing required read privileges on the following indices: ${JSON.stringify(
       indexesWithNoReadPrivileges
     )}`;
     logger.error(buildRuleMessage(errorString));
-    await ruleStatusService.partialFailure(errorString);
+    await ruleStatusService.warning(errorString);
     return true;
   } else if (
     indexesWithReadPrivileges.length === 0 &&
@@ -96,7 +96,7 @@ export const hasReadIndexPrivileges = async (
       indexesWithNoReadPrivileges
     )}`;
     logger.error(buildRuleMessage(errorString));
-    await ruleStatusService.partialFailure(errorString);
+    await ruleStatusService.warning(errorString);
     return true;
   }
   return false;
@@ -119,7 +119,7 @@ export const hasTimestampFields = async (
       inputIndices
     )}`;
     logger.error(buildRuleMessage(errorString));
-    await ruleStatusService.error(errorString);
+    await ruleStatusService.warning(errorString);
     return true;
   } else if (
     !wroteStatus &&
@@ -128,7 +128,7 @@ export const hasTimestampFields = async (
       timestampFieldCapsResponse.body.fields[timestampField]?.unmapped?.indices != null)
   ) {
     // if there is a timestamp override and the unmapped array for the timestamp override key is not empty,
-    // partial failure
+    // warning
     const errorString = `The following indices are missing the ${
       timestampField === '@timestamp'
         ? 'timestamp field "@timestamp"'
@@ -139,7 +139,7 @@ export const hasTimestampFields = async (
         : timestampFieldCapsResponse.body.fields[timestampField].unmapped.indices
     )}`;
     logger.error(buildRuleMessage(errorString));
-    await ruleStatusService.partialFailure(errorString);
+    await ruleStatusService.warning(errorString);
     return true;
   }
   return wroteStatus;

diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts
@@ -118,24 +118,24 @@ export default ({ getService }: FtrProviderContext) => {
           expect(statusBody[body.id].current_status.status).to.eql('succeeded');
         });
 
-        it('should create a single rule with a rule_id and an index pattern that does not match anything available and fail the rule', async () => {
+        it('should create a single rule with a rule_id and an index pattern that does not match anything available and warning for the rule', async () => {
           const simpleRule = getRuleForSignalTesting(['does-not-exist-*']);
           const { body } = await supertest
             .post(DETECTION_ENGINE_RULES_URL)
             .set('kbn-xsrf', 'true')
             .send(simpleRule)
             .expect(200);
 
-          await waitForRuleSuccessOrStatus(supertest, body.id, 'failed');
+          await waitForRuleSuccessOrStatus(supertest, body.id, 'warning');
 
           const { body: statusBody } = await supertest
             .post(DETECTION_ENGINE_RULES_STATUS_URL)
             .set('kbn-xsrf', 'true')
             .send({ ids: [body.id] })
             .expect(200);
 
-          expect(statusBody[body.id].current_status.status).to.eql('failed');
-          expect(statusBody[body.id].current_status.last_failure_message).to.eql(
+          expect(statusBody[body.id].current_status.status).to.eql('warning');
+          expect(statusBody[body.id].current_status.last_success_message).to.eql(
             'The following index patterns did not match any indices: ["does-not-exist-*"]'
           );
         });
@@ -287,10 +287,7 @@ export default ({ getService }: FtrProviderContext) => {
         await deleteAllAlerts(supertest);
         await esArchiver.unload('security_solution/timestamp_override');
       });
-      it.skip('should create a single rule which has a timestamp override and generates two signals with a failing status', async () => {
-        // should be a failing status because one of the indices in the index pattern is missing
-        // the timestamp override field.
-
+      it('should create a single rule which has a timestamp override and generates two signals with a "warning" status', async () => {
         // defaults to event.ingested timestamp override.
         // event.ingested is one of the timestamp fields set on the es archive data
         // inside of x-pack/test/functional/es_archives/security_solution/timestamp_override/data.json.gz
@@ -302,7 +299,7 @@ export default ({ getService }: FtrProviderContext) => {
           .expect(200);
         const bodyId = body.id;
 
-        await waitForRuleSuccessOrStatus(supertest, bodyId, 'partial failure');
+        await waitForRuleSuccessOrStatus(supertest, bodyId, 'warning');
         await waitForSignalsToBePresent(supertest, 2, [bodyId]);
 
         const { body: statusBody } = await supertest
@@ -311,9 +308,7 @@ export default ({ getService }: FtrProviderContext) => {
           .send({ ids: [bodyId] })
           .expect(200);
 
-        // set to "failed" for now. Will update this with a partial failure
-        // once I figure out the logic
-        expect(statusBody[bodyId].current_status.status).to.eql('partial failure');
+        expect(statusBody[bodyId].current_status.status).to.eql('warning');
       });
     });
   });

diff --git a/x-pack/test/detection_engine_api_integration/utils.ts b/x-pack/test/detection_engine_api_integration/utils.ts
@@ -972,7 +972,7 @@ export const getRule = async (
 export const waitForRuleSuccessOrStatus = async (
   supertest: SuperTest<supertestAsPromised.Test>,
   id: string,
-  status: 'succeeded' | 'failed' | 'partial failure' = 'succeeded'
+  status: 'succeeded' | 'failed' | 'partial failure' | 'warning' = 'succeeded'
 ): Promise<void> => {
   await waitFor(async () => {
     const { body } = await supertest