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.

Sign up for GitHub

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] Remove the advanced sorting switch from the rules management page #149840

Merged
merged 1 commit into from
Feb 7, 2023
Merged

[Security Solution] Remove the advanced sorting switch from the rules management page #149840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions x-pack/plugins/alerting/common/rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ export type RuleExecutionStatuses = typeof RuleExecutionStatusValues[number];
export const RuleLastRunOutcomeValues = ['succeeded', 'warning', 'failed'] as const;
export type RuleLastRunOutcomes = typeof RuleLastRunOutcomeValues[number];

export const RuleLastRunOutcomeOrderMap: Record<RuleLastRunOutcomes, number> = {
succeeded: 0,
pmuellr marked this conversation as resolved.
Show resolved Hide resolved
warning: 10,
failed: 20,
};

export enum RuleExecutionStatusErrorReasons {
Read = 'read',
Decrypt = 'decrypt',
Expand Down Expand Up @@ -93,6 +99,7 @@ export interface RuleAggregations {

export interface RuleLastRun {
outcome: RuleLastRunOutcomes;
outcomeOrder?: number;
warning?: RuleExecutionStatusErrorReasons | RuleExecutionStatusWarningReasons | null;
outcomeMsg?: string[] | null;
alertsCount: {
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/alerting/public/lib/common_transformations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ describe('common_transformations', () => {
},
last_run: {
outcome: RuleLastRunOutcomeValues[2],
outcome_order: 20,
outcome_msg: ['this is just a test'],
warning: RuleExecutionStatusErrorReasons.Unknown,
alerts_count: {
Expand Down Expand Up @@ -137,6 +138,7 @@ describe('common_transformations', () => {
"outcomeMsg": Array [
"this is just a test",
],
"outcomeOrder": 20,
"warning": "unknown",
},
"monitoring": Object {
Expand Down Expand Up @@ -255,6 +257,7 @@ describe('common_transformations', () => {
},
last_run: {
outcome: 'failed',
outcome_order: 20,
outcome_msg: ['this is just a test'],
warning: RuleExecutionStatusErrorReasons.Unknown,
alerts_count: {
Expand Down Expand Up @@ -300,6 +303,7 @@ describe('common_transformations', () => {
"outcomeMsg": Array [
"this is just a test",
],
"outcomeOrder": 20,
"warning": "unknown",
},
"monitoring": Object {
Expand Down
8 changes: 7 additions & 1 deletion x-pack/plugins/alerting/public/lib/common_transformations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,16 @@ function transformMonitoring(input: RuleMonitoring): RuleMonitoring {
}

function transformLastRun(input: AsApiContract<RuleLastRun>): RuleLastRun {
const { outcome_msg: outcomeMsg, alerts_count: alertsCount, ...rest } = input;
const {
outcome_msg: outcomeMsg,
alerts_count: alertsCount,
outcome_order: outcomeOrder,
...rest
} = input;
return {
outcomeMsg,
alertsCount,
outcomeOrder,
...rest,
};
}
Expand Down
8 changes: 6 additions & 2 deletions x-pack/plugins/alerting/server/lib/last_run_status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { RuleTaskStateAndMetrics } from '../task_runner/types';
import { getReasonFromError } from './error_with_reason';
import { getEsErrorMessage } from './errors';
import { ActionsCompletion, RuleLastRunOutcomes } from '../../common';
import { ActionsCompletion, RuleLastRunOutcomeOrderMap, RuleLastRunOutcomes } from '../../common';
import {
RuleLastRunOutcomeValues,
RuleExecutionStatusWarningReasons,
Expand Down Expand Up @@ -65,6 +65,7 @@ export const lastRunFromState = (
return {
lastRun: {
outcome,
outcomeOrder: RuleLastRunOutcomeOrderMap[outcome],
outcomeMsg: outcomeMsg.length > 0 ? outcomeMsg : null,
warning: warning || null,
alertsCount: {
Expand All @@ -80,9 +81,11 @@ export const lastRunFromState = (

export const lastRunFromError = (error: Error): ILastRun => {
const esErrorMessage = getEsErrorMessage(error);
const outcome = RuleLastRunOutcomeValues[2];
return {
lastRun: {
outcome: RuleLastRunOutcomeValues[2],
outcome,
outcomeOrder: RuleLastRunOutcomeOrderMap[outcome],
warning: getReasonFromError(error),
outcomeMsg: esErrorMessage ? [esErrorMessage] : null,
alertsCount: {},
Expand All @@ -104,5 +107,6 @@ export const lastRunToRaw = (lastRun: ILastRun['lastRun']): RawRuleLastRun => {
},
warning: warning ?? null,
outcomeMsg: outcomeMsg && !Array.isArray(outcomeMsg) ? [outcomeMsg] : outcomeMsg,
outcomeOrder: RuleLastRunOutcomeOrderMap[lastRun.outcome],
};
};
3 changes: 2 additions & 1 deletion x-pack/plugins/alerting/server/routes/lib/rewrite_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ import { omit } from 'lodash';
import { RuleTypeParams, SanitizedRule, RuleLastRun } from '../../types';

export const rewriteRuleLastRun = (lastRun: RuleLastRun) => {
const { outcomeMsg, alertsCount, ...rest } = lastRun;
const { outcomeMsg, outcomeOrder, alertsCount, ...rest } = lastRun;
return {
alerts_count: alertsCount,
outcome_msg: outcomeMsg,
outcome_order: outcomeOrder,
...rest,
};
};
Expand Down
24 changes: 22 additions & 2 deletions x-pack/plugins/alerting/server/saved_objects/migrations/8.7/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
isLogThresholdRuleType,
pipeMigrations,
} from '../utils';
import { RawRule } from '../../../types';
import { RawRule, RuleLastRunOutcomeOrderMap } from '../../../types';

function addGroupByToEsQueryRule(
doc: SavedObjectUnsanitizedDoc<RawRule>
Expand Down Expand Up @@ -70,9 +70,29 @@ function addLogViewRefToLogThresholdRule(
return doc;
}

function addOutcomeOrder(
doc: SavedObjectUnsanitizedDoc<RawRule>
): SavedObjectUnsanitizedDoc<RawRule> {
if (!doc.attributes.lastRun) {
return doc;
}

const outcome = doc.attributes.lastRun.outcome;
return {
...doc,
attributes: {
...doc.attributes,
lastRun: {
...doc.attributes.lastRun,
outcomeOrder: RuleLastRunOutcomeOrderMap[outcome],
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall off hand, but are there any old legacy outcome's that a user could have lingering around that wouldn't be included in the RuleLastRunOutcomeOrderMap? If so, is undefined fine here until the next rule execution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The outcome field was added in 8.6 with only three values specified in the RuleLastRunOutcomeOrderMap. So I don't expect any other values there. That said, undefined should also work just fine.

Copy link
Member

Choose a reason for hiding this comment

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

Problem is, the outcomeOrder is defined as a number, but in the case of a outcome: undefined - or some change in the future that would add a new outcome but forget to add the order - the order will end up being undefined. Feels like we should change outcomeOrder to be number | undefined, or write a function to generate the updated value that deals with the potential undefined value, where it's generated in execution.

If we want to keep it typed as a number, then I think we need to supply a number here, but not sure what we'd use. If we go with making outcome_order as number | undefined, then this code is fine as-is , as it would generate an undefined (if outcome is not set or an unexpected value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I miss something, outcome cannot be undefined. See the 8.6 migration, for example:
https://github.com/elastic/kibana/blob/2d675b47a7c3210a1b430e89ff51c9d7485d1fd2/x-pack/plugins/alerting/server/saved_objects/migrations/8.6/index.ts#L22-L35

If the outcome is unknown, the whole lastRun structure is omitted.

Furthermore, adding a new outcome value will cause a type error if the value is not added to the outcome->order map:

Screenshot 2023-02-07 at 10 12 49

So TypeScript protects us from forgetting to add values to the linked structures.

That said, if you feel strongly about changing the type of outcomeOrder to number | undefined, I'm okay with that. It doesn't affect the code of Security Solution in any way, as we don't work with that field directly.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! We've become more sensitive to migrations recently so wanted to understand this one well.

The main thing I'm worrying about is some bug where we would unintentionally have outcome set to some invalid value, as part of normal (presumably buggy!) operation. And then the migration would end up writing out an undefined for the order. But ... that's valid for that field :-)

So, even in that case, I don't think there's anything else we can do to improve the migration.

The rest of the type-checking on outcome looks good, thanks for explaining.

},
},
};
}

export const getMigrations870 = (encryptedSavedObjects: EncryptedSavedObjectsPluginSetup) =>
createEsoMigration(
encryptedSavedObjects,
(doc: SavedObjectUnsanitizedDoc<RawRule>): doc is SavedObjectUnsanitizedDoc<RawRule> => true,
pipeMigrations(addGroupByToEsQueryRule, addLogViewRefToLogThresholdRule)
pipeMigrations(addGroupByToEsQueryRule, addLogViewRefToLogThresholdRule, addOutcomeOrder)
);
22 changes: 22 additions & 0 deletions x-pack/plugins/alerting/server/saved_objects/migrations/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2606,6 +2606,28 @@ describe('successful migrations', () => {
expect(migratedAlert870.references).toEqual([]);
});
});

test('migrates last run outcome order', () => {
const migration870 = getMigrations(encryptedSavedObjectsSetup, {}, isPreconfigured)['8.7.0'];

// Failed rule
const failedRule = getMockData({ lastRun: { outcome: 'failed' } });
const failedRule870 = migration870(failedRule, migrationContext);
expect(failedRule870.attributes.lastRun).toEqual({ outcome: 'failed', outcomeOrder: 20 });

// Rule with warnings
const warningRule = getMockData({ lastRun: { outcome: 'warning' } });
const warningRule870 = migration870(warningRule, migrationContext);
expect(warningRule870.attributes.lastRun).toEqual({ outcome: 'warning', outcomeOrder: 10 });

// Succeeded rule
const succeededRule = getMockData({ lastRun: { outcome: 'succeeded' } });
const succeededRule870 = migration870(succeededRule, migrationContext);
expect(succeededRule870.attributes.lastRun).toEqual({
outcome: 'succeeded',
outcomeOrder: 0,
});
});
});

describe('Metrics Inventory Threshold rule', () => {
Expand Down
12 changes: 10 additions & 2 deletions x-pack/plugins/alerting/server/task_runner/fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,14 @@
*/

import { TaskStatus } from '@kbn/task-manager-plugin/server';
import { Rule, RuleTypeParams, RecoveredActionGroup, RuleMonitoring } from '../../common';
import {
Rule,
RuleTypeParams,
RecoveredActionGroup,
RuleMonitoring,
RuleLastRunOutcomeOrderMap,
RuleLastRunOutcomes,
} from '../../common';
import { getDefaultMonitoring } from '../lib/monitoring';
import { UntypedNormalizedRuleType } from '../rule_type_registry';
import { EVENT_LOG_ACTIONS } from '../plugin';
Expand Down Expand Up @@ -74,7 +81,7 @@ export const generateSavedObjectParams = ({
error?: null | { reason: string; message: string };
warning?: null | { reason: string; message: string };
status?: string;
outcome?: string;
outcome?: RuleLastRunOutcomes;
nextRun?: string | null;
successRatio?: number;
history?: RuleMonitoring['run']['history'];
Expand Down Expand Up @@ -110,6 +117,7 @@ export const generateSavedObjectParams = ({
},
lastRun: {
outcome,
outcomeOrder: RuleLastRunOutcomeOrderMap[outcome],
outcomeMsg:
(error?.message && [error?.message]) || (warning?.message && [warning?.message]) || null,
warning: error?.reason || warning?.reason || null,
Expand Down
14 changes: 7 additions & 7 deletions x-pack/plugins/alerting/server/task_runner/task_runner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ describe('Task Runner', () => {
);
expect(logger.debug).nthCalledWith(
3,
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeMsg":null,"warning":null,"alertsCount":{"active":0,"new":0,"recovered":0,"ignored":0}}'
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeOrder":0,"outcomeMsg":null,"warning":null,"alertsCount":{"active":0,"new":0,"recovered":0,"ignored":0}}'
);
expect(logger.debug).nthCalledWith(
4,
Expand Down Expand Up @@ -360,7 +360,7 @@ describe('Task Runner', () => {
);
expect(logger.debug).nthCalledWith(
4,
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeMsg":null,"warning":null,"alertsCount":{"active":1,"new":1,"recovered":0,"ignored":0}}'
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeOrder":0,"outcomeMsg":null,"warning":null,"alertsCount":{"active":1,"new":1,"recovered":0,"ignored":0}}'
);
expect(logger.debug).nthCalledWith(
5,
Expand Down Expand Up @@ -447,7 +447,7 @@ describe('Task Runner', () => {
);
expect(logger.debug).nthCalledWith(
5,
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeMsg":null,"warning":null,"alertsCount":{"active":1,"new":1,"recovered":0,"ignored":0}}'
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeOrder":0,"outcomeMsg":null,"warning":null,"alertsCount":{"active":1,"new":1,"recovered":0,"ignored":0}}'
);
expect(logger.debug).nthCalledWith(
6,
Expand Down Expand Up @@ -627,7 +627,7 @@ describe('Task Runner', () => {
);
expect(logger.debug).nthCalledWith(
5,
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeMsg":null,"warning":null,"alertsCount":{"active":2,"new":2,"recovered":0,"ignored":0}}'
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeOrder":0,"outcomeMsg":null,"warning":null,"alertsCount":{"active":2,"new":2,"recovered":0,"ignored":0}}'
);
expect(logger.debug).nthCalledWith(
6,
Expand Down Expand Up @@ -1066,7 +1066,7 @@ describe('Task Runner', () => {
);
expect(logger.debug).nthCalledWith(
5,
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeMsg":null,"warning":null,"alertsCount":{"active":1,"new":0,"recovered":1,"ignored":0}}'
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeOrder":0,"outcomeMsg":null,"warning":null,"alertsCount":{"active":1,"new":0,"recovered":1,"ignored":0}}'
);
expect(logger.debug).nthCalledWith(
6,
Expand Down Expand Up @@ -1192,7 +1192,7 @@ describe('Task Runner', () => {
);
expect(logger.debug).nthCalledWith(
5,
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeMsg":null,"warning":null,"alertsCount":{"active":1,"new":0,"recovered":1,"ignored":0}}'
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeOrder":0,"outcomeMsg":null,"warning":null,"alertsCount":{"active":1,"new":0,"recovered":1,"ignored":0}}'
);
expect(logger.debug).nthCalledWith(
6,
Expand Down Expand Up @@ -2330,7 +2330,7 @@ describe('Task Runner', () => {
);
expect(logger.debug).nthCalledWith(
3,
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeMsg":null,"warning":null,"alertsCount":{"active":0,"new":0,"recovered":0,"ignored":0}}'
'ruleRunStatus for test:1: {"outcome":"succeeded","outcomeOrder":0,"outcomeMsg":null,"warning":null,"alertsCount":{"active":0,"new":0,"recovered":0,"ignored":0}}'
);
expect(logger.debug).nthCalledWith(
4,
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/alerting/server/task_runner/task_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import {
RuleTypeState,
parseDuration,
RawAlertInstance,
RuleLastRunOutcomeOrderMap,
} from '../../common';
import { NormalizedRuleType, UntypedNormalizedRuleType } from '../rule_type_registry';
import { getEsErrorMessage } from '../lib/errors';
Expand Down Expand Up @@ -821,10 +822,12 @@ export class TaskRunner<
this.logger.debug(
`Updating rule task for ${this.ruleType.id} rule with id ${ruleId} - execution error due to timeout`
);
const outcome = 'failed';
await this.updateRuleSavedObjectPostRun(ruleId, namespace, {
executionStatus: ruleExecutionStatusToRaw(executionStatus),
lastRun: {
outcome: 'failed',
outcome,
outcomeOrder: RuleLastRunOutcomeOrderMap[outcome],
warning: RuleExecutionStatusErrorReasons.Timeout,
outcomeMsg,
alertsCount: {},
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/task_runner/task_runner_cancel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ describe('Task Runner Cancel', () => {
outcomeMsg: [
'test:1: execution cancelled due to timeout - exceeded rule type timeout of 5m',
],
outcomeOrder: 20,
warning: 'timeout',
},
monitoring: {
Expand Down
10 changes: 5 additions & 5 deletions ...ution/common/detection_engine/rule_management/api/rules/find_rules/request_schema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('Find rules request schema', () => {
const payload: FindRulesRequestQuery = {
per_page: 5,
page: 1,
sort_field: 'some field',
sort_field: 'name',
fields: ['field 1', 'field 2'],
filter: 'some filter',
sort_order: 'asc',
Expand Down Expand Up @@ -80,14 +80,14 @@ describe('Find rules request schema', () => {

test('sort_field validates', () => {
const payload: FindRulesRequestQuery = {
sort_field: 'value',
sort_field: 'name',
};

const decoded = FindRulesRequestQuery.decode(payload);
const checked = exactCheck(payload, decoded);
const message = pipe(checked, foldLeftRight);
expect(getPaths(left(message.errors))).toEqual([]);
expect((message.schema as FindRulesRequestQuery).sort_field).toEqual('value');
expect((message.schema as FindRulesRequestQuery).sort_field).toEqual('name');
});

test('fields validates with a string', () => {
Expand Down Expand Up @@ -173,7 +173,7 @@ describe('Find rules request schema', () => {
test('sort_order validates with desc and sort_field', () => {
const payload: FindRulesRequestQuery = {
sort_order: 'desc',
sort_field: 'some field',
sort_field: 'name',
};

const decoded = FindRulesRequestQuery.decode(payload);
Expand All @@ -187,7 +187,7 @@ describe('Find rules request schema', () => {
test('sort_order does not validate with a string other than asc and desc', () => {
const payload: Omit<FindRulesRequestQuery, 'sort_order'> & { sort_order: string } = {
sort_order: 'some other string',
sort_field: 'some field',
sort_field: 'name',
};

const decoded = FindRulesRequestQuery.decode(payload);
Expand Down
25 changes: 23 additions & 2 deletions ...y_solution/common/detection_engine/rule_management/api/rules/find_rules/request_schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,28 @@
import * as t from 'io-ts';
import { DefaultPerPage, DefaultPage } from '@kbn/securitysolution-io-ts-alerting-types';
import type { PerPage, Page } from '../../../../schemas/common';
import { queryFilter, fields, SortField, SortOrder } from '../../../../schemas/common';
import { queryFilter, fields, SortOrder } from '../../../../schemas/common';

export type FindRulesSortField = t.TypeOf<typeof FindRulesSortField>;
export const FindRulesSortField = t.union([
t.literal('created_at'),
t.literal('createdAt'), // Legacy notation, keeping for backwards compatibility
t.literal('enabled'),
t.literal('execution_summary.last_execution.date'),
t.literal('execution_summary.last_execution.metrics.execution_gap_duration_s'),
t.literal('execution_summary.last_execution.metrics.total_indexing_duration_ms'),
t.literal('execution_summary.last_execution.metrics.total_search_duration_ms'),
t.literal('execution_summary.last_execution.status'),
t.literal('name'),
t.literal('risk_score'),
t.literal('riskScore'), // Legacy notation, keeping for backwards compatibility
t.literal('severity'),
t.literal('updated_at'),
t.literal('updatedAt'), // Legacy notation, keeping for backwards compatibility
]);

export type FindRulesSortFieldOrUndefined = t.TypeOf<typeof FindRulesSortFieldOrUndefined>;
export const FindRulesSortFieldOrUndefined = t.union([FindRulesSortField, t.undefined]);

/**
* Query string parameters of the API route.
Expand All @@ -18,7 +39,7 @@ export const FindRulesRequestQuery = t.exact(
t.partial({
fields,
filter: queryFilter,
sort_field: SortField,
sort_field: FindRulesSortField,
sort_order: SortOrder,
page: DefaultPage, // defaults to 1
per_page: DefaultPerPage, // defaults to 20
Expand Down
Loading