Skip to content

Commit

Permalink
[Security Solution] [Detections] Replace 'partial failure' with 'warn…
Browse files Browse the repository at this point in the history
…ing' for rule statuses (#91167)

* 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
  • Loading branch information
dhurley14 authored Feb 17, 2021
1 parent 5b97d52 commit 540b1d3
Show file tree
Hide file tree
Showing 16 changed files with 71 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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>;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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}>
Expand All @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const ruleStatusServiceFactoryMock = async ({

success: jest.fn(),

partialFailure: jest.fn(),
warning: jest.fn(),

error: jest.fn(),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
}

Expand All @@ -48,7 +48,7 @@ export const buildRuleStatusAttributes: (
lastSuccessMessage: message,
};
}
case 'partial failure': {
case 'warning': {
return {
...baseAttributes,
lastSuccessAt: now,
Expand Down Expand Up @@ -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),
});
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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,
Expand All @@ -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*"]'
);
});
Expand All @@ -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*"]'
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -202,7 +202,7 @@ export const signalRulesAlertType = ({
}),
]);

wrotePartialFailureStatus = await flow(
wroteWarningStatus = await flow(
() =>
tryCatch(
() =>
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ const ruleStatusServiceMock = {
find: jest.fn(),
goingToRun: jest.fn(),
error: jest.fn(),
partialFailure: jest.fn(),
warning: jest.fn(),
};

describe('utils', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand All @@ -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;
Expand All @@ -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 &&
Expand All @@ -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"'
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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-*"]'
);
});
Expand Down Expand Up @@ -287,10 +287,7 @@ export default ({ getService }: FtrProviderContext) => {
await deleteAllAlerts(supertest);
await esArchiver.unload('security_solution/timestamp_override');
});
it('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
Expand All @@ -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
Expand All @@ -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');
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion x-pack/test/detection_engine_api_integration/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 540b1d3

Please sign in to comment.