Skip to content

Commit

Permalink
[Security Solution][Detections] Rule execution logging overhaul (#121644
Browse files Browse the repository at this point in the history
)

**Epic:** #118324
**Tickets:** #119603, #119597, #91265, #118511

## Summary

The legacy rule execution logging implementation is replaced by a new one that introduces a new model for execution-related data, a new saved object and a new, cleaner interface and implementation.

- [x] The legacy data model is deleted (`IRuleStatusResponseAttributes`, `IRuleStatusSOAttributes`)
- [x] The legacy `siem-detection-engine-rule-status` saved object type is deleted and marked as deleted in `src/core`
- [x] A new data model is introduced (`x-pack/plugins/security_solution/common/detection_engine/schemas/common/rule_monitoring.ts`). This data model doesn't contain a mixture of successful and failed statuses, which should simplify client-side code (e.g. the code of Rule Management and Monitoring tables, as well as Rule Details page).
- [x] A new `siem-detection-engine-rule-execution-info` saved object is introduced (`x-pack/plugins/security_solution/server/lib/detection_engine/rule_execution_log/rule_execution_info/saved_object.ts`).
  - [x] This SO has 1:1 association with the rule SO, so every rule can have 0 or 1 execution info associated with it. This SO is used in order to 1) update the last execution status and metrics and 2) fetch execution data for N rules more efficiently comparing to the legacy SO.
  - [x] The logic of creating or updating this SOs is based on the "upsert" approach (planned in #118511). It does not fetch the SO by rule id before updating it anymore.
- [x] Rule execution logging logic is rewritten (see `x-pack/plugins/security_solution/server/lib/detection_engine/rule_execution_log`). The previous rule execution log client is split into two objects: `IRuleExecutionLogClient` for using it from route handlers, and `IRuleExecutionLogger` for writing logs from rule executors.
  - [x] `IRuleExecutionLogger` instance is scoped to the currently executing rule and space id. There's no need to pass rule id, name, type etc to `.logStatusChange()` every time.
- [x] Rule executors and related functions are updated.
- [x] API routes are updated, including the rule preview route which uses a special "spy" implementation of `IRuleExecutionLogger`. A rule returned from an API endpoint now has optional `execution_summary` field of type `RuleExecutionSummary`.
- [x] UI is updated to use the new data model of `RuleExecutionSummary`:
  - [x] Rule Management and Monitoring tables
  - [x] Rule Details page
- [x] A new API route is introduced for fetching rule execution events: `/internal/detection_engine/rules/{ruleId}/execution/events`. It is used for rendering the Failure History tab (last 5 failures) and is intended to be used in the coming UI of Rule Execution Log on the Details page.
- [x] Rule Details page and Failure History tab are updated to use the new data models and API routes.
- [x] I used `react-query` for fetching execution events
  - [x] See `x-pack/plugins/security_solution/public/detections/containers/detection_engine/rules/use_rule_execution_events.tsx`
  - [x] The lib is updated to the latest version
- [x] Tests and fixed and updated according to all the changes
- [x] Components related to rule execution statuses are all moved to `x-pack/plugins/security_solution/public/detections/components/rules/rule_execution_status`.
- [x] I left a lot of `// TODO: https://github.com/elastic/kibana/pull/121644` comments in the code which I'm planning to address and remove in a follow-up PR. Lots of clean up work is needed, but I'd like to unblock the work on Rule Execution Log UI.

## In the next episodes

- Address and remove `// TODO: https://github.com/elastic/kibana/pull/121644` comments in the code
- Make sure that SO id generation for `siem-detection-engine-rule-execution-info` is safe and future-proof. Sync with the Core team. If there are risks, we will need to choose between risks and performance (reading the SO before updating it). It would be easy to submit a fix if needed.
- Add APM integration. Use `withSecuritySpan` in methods of `rule_execution_log` citizens.
- Add comments to the code and README.
- Add test coverage.
- Etc...

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
- [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
banderror authored Jan 20, 2022
1 parent 7740a5d commit ba0833f
Show file tree
Hide file tree
Showing 155 changed files with 2,386 additions and 3,562 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@
"react-moment-proptypes": "^1.7.0",
"react-monaco-editor": "^0.41.2",
"react-popper-tooltip": "^2.10.1",
"react-query": "^3.28.0",
"react-query": "^3.34.0",
"react-redux": "^7.2.0",
"react-resizable": "^1.7.5",
"react-resize-detector": "^4.2.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ const getRulesSchemaMock = (anchorDate: string = ANCHOR_DATE) => ({
type: 'query',
threat: [],
version: 1,
status: 'succeeded',
status_date: '2020-02-22T16:47:50.047Z',
last_success_at: '2020-02-22T16:47:50.047Z',
last_success_message: 'succeeded',
output_index: '.siem-signals-default',
max_signals: 100,
risk_score: 55,
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions src/core/server/saved_objects/migrations/core/unused_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ export const REMOVED_TYPES: string[] = [
'tsvb-validation-telemetry',
// replaced by osquery-manager-usage-metric
'osquery-usage-metric',
// Was removed in 8.1 https://github.com/elastic/kibana/issues/91265
'siem-detection-engine-rule-status',
// Was removed in 7.16
'timelion-sheet',
].sort();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ const previouslyRegisteredTypes = [
'security-solution-signals-migration',
'server',
'siem-detection-engine-rule-actions',
'siem-detection-engine-rule-execution-info',
'siem-detection-engine-rule-status',
'siem-ui-timeline',
'siem-ui-timeline-note',
Expand Down
2 changes: 1 addition & 1 deletion x-pack/examples/reporting_example/public/application.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import React from 'react';
import ReactDOM from 'react-dom';
import { Router, Route, Switch } from 'react-router-dom';
import { AppMountParameters, CoreStart } from '../../../../src/core/public';
import { KibanaThemeProvider } from '../../../../../kibana/src/plugins/kibana_react/public';
import { KibanaThemeProvider } from '../../../../src/plugins/kibana_react/public';
import { CaptureTest } from './containers/capture_test';
import { Main } from './containers/main';
import { ApplicationContextProvider } from './application_context';
Expand Down
6 changes: 4 additions & 2 deletions x-pack/plugins/security_solution/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,10 @@ export const DETECTION_ENGINE_RULES_PREVIEW = `${DETECTION_ENGINE_RULES_URL}/pre
* Internal detection engine routes
*/
export const INTERNAL_DETECTION_ENGINE_URL = '/internal/detection_engine' as const;
export const INTERNAL_DETECTION_ENGINE_RULE_STATUS_URL =
`${INTERNAL_DETECTION_ENGINE_URL}/rules/_find_status` as const;
export const DETECTION_ENGINE_RULE_EXECUTION_EVENTS_URL =
`${INTERNAL_DETECTION_ENGINE_URL}/rules/{ruleId}/execution/events` as const;
export const detectionEngineRuleExecutionEventsUrl = (ruleId: string) =>
`${INTERNAL_DETECTION_ENGINE_URL}/rules/${ruleId}/execution/events` as const;

export const TIMELINE_RESOLVE_URL = '/api/timeline/resolve' as const;
export const TIMELINE_URL = '/api/timeline' as const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,5 @@
* 2.0.
*/

import { getStatusColor } from './helpers';

describe('rule_status helpers', () => {
it('getStatusColor returns subdued if null was provided', () => {
expect(getStatusColor(null)).toBe('subdued');
});
});
export * from './rule_monitoring';
export * from './schemas';
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import * as t from 'io-ts';
import { enumeration, IsoDateString, PositiveInteger } from '@kbn/securitysolution-io-ts-types';

// -------------------------------------------------------------------------------------------------
// Rule execution status

/**
* Custom execution status of Security rules that is different from the status
* used in the Alerting Framework. We merge our custom status with the
* Framework's status to determine the resulting status of a rule.
*/
export enum RuleExecutionStatus {
'succeeded' = 'succeeded',
'failed' = 'failed',
'going to run' = 'going to run',
'partial failure' = 'partial failure',
/**
* @deprecated 'partial failure' status should be used instead
*/
'warning' = 'warning',
}

export const ruleExecutionStatus = enumeration('RuleExecutionStatus', RuleExecutionStatus);

export const ruleExecutionStatusOrder = PositiveInteger;
export type RuleExecutionStatusOrder = t.TypeOf<typeof ruleExecutionStatusOrder>;

export const ruleExecutionStatusOrderByStatus: Record<
RuleExecutionStatus,
RuleExecutionStatusOrder
> = {
[RuleExecutionStatus.succeeded]: 0,
[RuleExecutionStatus['going to run']]: 10,
[RuleExecutionStatus.warning]: 20,
[RuleExecutionStatus['partial failure']]: 20,
[RuleExecutionStatus.failed]: 30,
};

// -------------------------------------------------------------------------------------------------
// Rule execution metrics

export const durationMetric = PositiveInteger;
export type DurationMetric = t.TypeOf<typeof durationMetric>;

export const ruleExecutionMetrics = t.partial({
total_search_duration_ms: durationMetric,
total_indexing_duration_ms: durationMetric,
execution_gap_duration_s: durationMetric,
});

export type RuleExecutionMetrics = t.TypeOf<typeof ruleExecutionMetrics>;

// -------------------------------------------------------------------------------------------------
// Rule execution summary

export const ruleExecutionSummary = t.type({
last_execution: t.type({
date: IsoDateString,
status: ruleExecutionStatus,
status_order: ruleExecutionStatusOrder,
message: t.string,
metrics: ruleExecutionMetrics,
}),
});

export type RuleExecutionSummary = t.TypeOf<typeof ruleExecutionSummary>;

// -------------------------------------------------------------------------------------------------
// Rule execution events

export const ruleExecutionEvent = t.type({
date: IsoDateString,
status: ruleExecutionStatus,
message: t.string,
});

export type RuleExecutionEvent = t.TypeOf<typeof ruleExecutionEvent>;
Original file line number Diff line number Diff line change
Expand Up @@ -193,19 +193,6 @@ export const status = t.keyof({
});
export type Status = t.TypeOf<typeof status>;

export enum RuleExecutionStatus {
'succeeded' = 'succeeded',
'failed' = 'failed',
'going to run' = 'going to run',
'partial failure' = 'partial failure',
/**
* @deprecated 'partial failure' status should be used instead
*/
'warning' = 'warning',
}

export const ruleExecutionStatus = enumeration('RuleExecutionStatus', RuleExecutionStatus);

export const conflicts = t.keyof({ abort: null, proceed: null });
export type Conflicts = t.TypeOf<typeof conflicts>;

Expand Down Expand Up @@ -332,30 +319,6 @@ export type UpdatedByOrNull = t.TypeOf<typeof updatedByOrNull>;
export const createdByOrNull = t.union([created_by, t.null]);
export type CreatedByOrNull = t.TypeOf<typeof createdByOrNull>;

export const last_success_at = IsoDateString;
export type LastSuccessAt = t.TypeOf<typeof IsoDateString>;

export const last_success_message = t.string;
export type LastSuccessMessage = t.TypeOf<typeof last_success_message>;

export const last_failure_at = IsoDateString;
export type LastFailureAt = t.TypeOf<typeof last_failure_at>;

export const last_failure_message = t.string;
export type LastFailureMessage = t.TypeOf<typeof last_failure_message>;

export const last_gap = t.string;
export type LastGap = t.TypeOf<typeof last_gap>;

export const bulk_create_time_durations = t.array(t.string);
export type BulkCreateTimeDurations = t.TypeOf<typeof bulk_create_time_durations>;

export const search_after_time_durations = t.array(t.string);
export type SearchAfterTimeDurations = t.TypeOf<typeof search_after_time_durations>;

export const status_date = IsoDateString;
export type StatusDate = t.TypeOf<typeof status_date>;

export const rules_installed = PositiveInteger;
export const rules_updated = PositiveInteger;
export const status_code = PositiveInteger;
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import * as t from 'io-ts';

export const GetRuleExecutionEventsRequestParams = t.exact(
t.type({
ruleId: t.string,
})
);

export type GetRuleExecutionEventsRequestParams = t.TypeOf<
typeof GetRuleExecutionEventsRequestParams
>;
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,9 @@ import {
updated_by,
created_at,
created_by,
ruleExecutionStatus,
status_date,
last_success_at,
last_success_message,
last_failure_at,
last_failure_message,
namespace,
last_gap,
bulk_create_time_durations,
search_after_time_durations,
} from '../common/schemas';
ruleExecutionSummary,
} from '../common';

export const createSchema = <
Required extends t.Props,
Expand Down Expand Up @@ -419,16 +411,9 @@ const responseRequiredFields = {
created_at,
created_by,
};

const responseOptionalFields = {
status: ruleExecutionStatus,
status_date,
last_success_at,
last_success_message,
last_failure_at,
last_failure_message,
last_gap,
bulk_create_time_durations,
search_after_time_durations,
execution_summary: ruleExecutionSummary,
};

export const fullResponseSchema = t.intersection([
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import * as t from 'io-ts';
import { ruleExecutionEvent } from '../common';

export const GetRuleExecutionEventsResponse = t.exact(
t.type({
events: t.array(ruleExecutionEvent),
})
);

export type GetRuleExecutionEventsResponse = t.TypeOf<typeof GetRuleExecutionEventsResponse>;
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

export * from './error_schema';
export * from './get_rule_execution_events_response';
export * from './import_rules_schema';
export * from './prepackaged_rules_schema';
export * from './prepackaged_rules_status_schema';
Expand Down
Loading

0 comments on commit ba0833f

Please sign in to comment.