Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] [Detections] Replace 'partial failure' with 'warning' for rule statuses #91167

Merged
merged 5 commits into from
Feb 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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';
}
Comment on lines +65 to +71
Copy link
Contributor

@banderror banderror Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer: I'm not familiar with Kibana upgrades and migrations, what's supported and not.

What if a user upgrades Kibana from let's say 7.10 (where a rule can have a partial failure status) to 8.1? I mean data-wise. Such a rule will (for a short time) have a status which will not be supported in the API anymore (do I understand the intention right?). But still we'll fetch this partial failure status, return in the API response, and the client will receive it. And it's ok if this "adapter"-like code will still be here, but if not, we might get something we don't want in the UI.

Wouldn't it be simpler to do either of the following:

  1. render partial failure as "warning" in the UI layer without modifying the contract between the client and server
  2. add a new warning status and convert all partial failures to it on the server; and support it on the client

I'm also not sure about the current users of our API endpoints. While this is not a breaking change in terms of response structure, it's a change in behaviour. If some code in userland relies on some checks for partial failure, that could be a de-facto breaking change.

So in this regards option num 1 looks better to me until 8.0, where we could in fact "rename" partial failure to warning in the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering user-land breaking changes we will have this change reflected in the docs and in the release notes. If a status is 'partial failure' after upgrade it will still be reflected as a 'warning' on the UI so we do take care of option 1.

Option 2 we no longer write 'partial failure' as a status so these statuses will technically be migrated off of 'partial failure' and to 'warning' as the status text on the server side. You bring up a good point though that even with 8.0 allowing breaking changes this code should still be left in there to account for situations like that.

My hope is that by 8.0 we will be completely off of the saved objects used for rule statuses and then this code won't really be relevant anymore.

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 @@ -77,6 +77,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 @@ -252,7 +253,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 @@ -180,7 +180,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 @@ -201,7 +201,7 @@ export const signalRulesAlertType = ({
}),
]);

wrotePartialFailureStatus = await flow(
wroteWarningStatus = await flow(
() =>
tryCatch(
() =>
Expand Down Expand Up @@ -657,7 +657,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