Skip to content

Commit

Permalink
Address code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
banderror committed Nov 1, 2021
1 parent a4027e2 commit dc16bb8
Show file tree
Hide file tree
Showing 11 changed files with 64 additions and 39 deletions.
2 changes: 1 addition & 1 deletion x-pack/plugins/security_solution/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ export const DETECTION_ENGINE_RULES_PREVIEW_INDEX_URL =
*/
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_statuses` as const;
`${INTERNAL_DETECTION_ENGINE_URL}/rules/_find_status` 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 @@ -16,3 +16,13 @@ export const findRulesStatusesSchema = t.exact(
export type FindRulesStatusesSchema = t.TypeOf<typeof findRulesStatusesSchema>;

export type FindRulesStatusesSchemaDecoded = FindRulesStatusesSchema;

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

export type FindRuleStatusSchema = t.TypeOf<typeof findRuleStatusSchema>;

export type FindRuleStatusSchemaDecoded = FindRuleStatusSchema;
Original file line number Diff line number Diff line change
Expand Up @@ -669,8 +669,8 @@ describe('Detections Rules API', () => {

test('check parameter url, query', async () => {
await getRuleStatusById({ id: 'mySuperRuleId', signal: abortCtrl.signal });
expect(fetchMock).toHaveBeenCalledWith('/internal/detection_engine/rules/_find_statuses', {
body: '{"ids":["mySuperRuleId"]}',
expect(fetchMock).toHaveBeenCalledWith('/internal/detection_engine/rules/_find_status', {
body: '{"ruleId":"mySuperRuleId"}',
method: 'POST',
signal: abortCtrl.signal,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ export const getRuleStatusById = async ({
}): Promise<RuleStatusResponse> =>
KibanaServices.get().http.fetch<RuleStatusResponse>(INTERNAL_DETECTION_ENGINE_RULE_STATUS_URL, {
method: 'POST',
body: JSON.stringify({ ids: [id] }),
body: JSON.stringify({ ruleId: id }),
signal,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ export const internalRuleStatusRequest = () =>
requestMock.create({
method: 'post',
path: INTERNAL_DETECTION_ENGINE_RULE_STATUS_URL,
body: { ids: ['04128c15-0d1b-4716-a4c5-46997ac7f3bd'] },
body: { ruleId: '04128c15-0d1b-4716-a4c5-46997ac7f3bd' },
});

export const getImportRulesRequest = (hapiStream?: HapiReadableStream) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
getRuleExecutionStatusFailed,
} from '../__mocks__/request_responses';
import { serverMock, requestContextMock, requestMock } from '../__mocks__';
import { internalFindRuleStatusRoute } from './internal_find_rule_status_route';
import { findRuleStatusInternalRoute } from './find_rule_status_internal_route';
import { RuleStatusResponse } from '../../rules/types';
import { AlertExecutionStatusErrorReasons } from '../../../../../../alerting/common';
import { getQueryRuleParams } from '../../schemas/rule_schemas.mock';
Expand All @@ -39,7 +39,7 @@ describe.each([
getAlertMock(isRuleRegistryEnabled, getQueryRuleParams())
);

internalFindRuleStatusRoute(server.router);
findRuleStatusInternalRoute(server.router);
});

describe('status codes with actionClient and alertClient', () => {
Expand Down Expand Up @@ -86,7 +86,7 @@ describe.each([
});

const request = internalRuleStatusRequest();
const ruleId = request.body.ids[0];
const { ruleId } = request.body;

const response = await server.inject(request, context);
const responseBody: RuleStatusResponse = response.body;
Expand All @@ -107,7 +107,9 @@ describe.each([
});
const result = server.validate(request);

expect(result.badRequest).toHaveBeenCalledWith('Invalid value "undefined" supplied to "ids"');
expect(result.badRequest).toHaveBeenCalledWith(
'Invalid value "undefined" supplied to "ruleId"'
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,42 +11,45 @@ import type { SecuritySolutionPluginRouter } from '../../../../types';
import { INTERNAL_DETECTION_ENGINE_RULE_STATUS_URL } from '../../../../../common/constants';
import { buildSiemResponse, mergeStatuses, getFailingRules } from '../utils';
import {
findRulesStatusesSchema,
FindRulesStatusesSchemaDecoded,
findRuleStatusSchema,
FindRuleStatusSchemaDecoded,
} from '../../../../../common/detection_engine/schemas/request/find_rule_statuses_schema';
import { mergeAlertWithSidecarStatus } from '../../schemas/rule_converters';

/**
* Given a list of rule ids, return the current status and
* last five errors for each associated rule.
* Returns the current execution status and metrics + last five failed statuses of a given rule.
* Accepts a rule id.
*
* NOTE: This endpoint is a raw implementation of an endpoint for reading rule execution
* status and logs for a given rule (e.g. for use on the Rule Details page). It will be reworked.
* See the plan in https://github.com/elastic/kibana/pull/115574
*
* @param router
* @returns RuleStatusResponse
* @returns RuleStatusResponse containing data only for the given rule (normally it contains data for N rules).
*/
export const internalFindRuleStatusRoute = (router: SecuritySolutionPluginRouter) => {
export const findRuleStatusInternalRoute = (router: SecuritySolutionPluginRouter) => {
router.post(
{
path: INTERNAL_DETECTION_ENGINE_RULE_STATUS_URL,
validate: {
body: buildRouteValidation<typeof findRulesStatusesSchema, FindRulesStatusesSchemaDecoded>(
findRulesStatusesSchema
body: buildRouteValidation<typeof findRuleStatusSchema, FindRuleStatusSchemaDecoded>(
findRuleStatusSchema
),
},
options: {
tags: ['access:securitySolution'],
},
},
async (context, request, response) => {
const { body } = request;
const { ruleId } = request.body;

const siemResponse = buildSiemResponse(response);
const rulesClient = context.alerting?.getRulesClient();

if (!rulesClient) {
return siemResponse.error({ statusCode: 404 });
}

const ruleId = body.ids[0];

try {
const ruleStatusClient = context.securitySolution.getExecutionLogClient();
const spaceId = context.securitySolution.getSpaceId();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,16 @@ import {
import { mergeAlertWithSidecarStatus } from '../../schemas/rule_converters';

/**
* Given a list of rule ids, return the current status and
* last five errors for each associated rule.
* Returns the current execution status and metrics for N rules.
* Accepts an array of rule ids.
*
* NOTE: This endpoint is used on the Rule Management page and will be reworked.
* See the plan in https://github.com/elastic/kibana/pull/115574
*
* @param router
* @returns RuleStatusResponse
* @returns RuleStatusResponse containing data for N requested rules.
* RuleStatusResponse[ruleId].failures is always an empty array, because
* we don't need failure history of every rule when we render tables with rules.
*/
export const findRulesStatusesRoute = (router: SecuritySolutionPluginRouter) => {
router.post(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
SAVED_OBJECT_REL_PRIMARY,
} from '../../../../../../event_log/server';
import { RuleExecutionStatus } from '../../../../../common/detection_engine/schemas/common/schemas';
import { invariant } from '../../../../../common/utils/invariant';
import { IRuleStatusSOAttributes } from '../../rules/types';
import { LogStatusChangeArgs } from '../types';
import {
Expand All @@ -23,6 +24,8 @@ import {

const spaceIdToNamespace = SavedObjectsUtils.namespaceStringToId;

const now = () => new Date().toISOString();

const statusSeverityDict: Record<RuleExecutionStatus, number> = {
[RuleExecutionStatus.succeeded]: 0,
[RuleExecutionStatus['going to run']]: 10,
Expand Down Expand Up @@ -104,19 +107,23 @@ export class EventLogClient implements IExecLogEventLogClient {
});

return findResult.data.map((event) => {
const statusDate = event?.['@timestamp'] ?? new Date().toISOString();
const status = event?.kibana?.alert?.rule?.execution?.status as
invariant(event, 'Event not found');
invariant(event['@timestamp'], 'Required "@timestamp" field is not found');

const statusDate = event['@timestamp'];
const status = event.kibana?.alert?.rule?.execution?.status as
| RuleExecutionStatus
| undefined;
const message = event?.message ?? '';
const isStatusFailed = status === RuleExecutionStatus.failed;
const message = event.message ?? '';

return {
statusDate,
status,
lastFailureAt: status === RuleExecutionStatus.failed ? statusDate : undefined,
lastFailureMessage: status === RuleExecutionStatus.failed ? message : undefined,
lastSuccessAt: status !== RuleExecutionStatus.failed ? statusDate : undefined,
lastSuccessMessage: status !== RuleExecutionStatus.failed ? message : undefined,
lastFailureAt: isStatusFailed ? statusDate : undefined,
lastFailureMessage: isStatusFailed ? message : undefined,
lastSuccessAt: !isStatusFailed ? statusDate : undefined,
lastSuccessMessage: !isStatusFailed ? message : undefined,
lastLookBackDate: undefined,
gap: undefined,
bulkCreateTimeDurations: undefined,
Expand All @@ -133,6 +140,7 @@ export class EventLogClient implements IExecLogEventLogClient {
spaceId,
}: LogExecutionMetricsArgs) {
this.eventLogger.logEvent({
'@timestamp': now(),
rule: {
id: ruleId,
name: ruleName,
Expand Down Expand Up @@ -177,6 +185,8 @@ export class EventLogClient implements IExecLogEventLogClient {
spaceId,
}: LogStatusChangeArgs) {
this.eventLogger.logEvent({
'@timestamp': now(),
message,
rule: {
id: ruleId,
name: ruleName,
Expand All @@ -187,7 +197,6 @@ export class EventLogClient implements IExecLogEventLogClient {
action: RuleExecutionLogAction['status-change'],
sequence: this.sequence++,
},
message,
kibana: {
alert: {
rule: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,19 +84,15 @@ export class SavedObjectsAdapter implements IRuleExecutionLogClient {
): Promise<IRuleStatusSOAttributes | undefined> {
const result = await this.findRuleStatusSavedObjects(args.ruleId, 1);
const currentStatusSavedObject = result[0];
return currentStatusSavedObject ? currentStatusSavedObject.attributes : undefined;
return currentStatusSavedObject?.attributes;
}

public async getCurrentStatusBulk(
args: GetCurrentStatusBulkArgs
): Promise<GetCurrentStatusBulkResult> {
const { ruleIds } = args;
const result = await this.ruleStatusClient.findBulk(ruleIds, 1);

return mapValues(result, (value) => {
const arrayOfAttributes = value ?? [];
return arrayOfAttributes[0];
});
return mapValues(result, (attributes = []) => attributes[0]);
}

public async deleteCurrentStatus(ruleId: string): Promise<void> {
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/security_solution/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { performBulkActionRoute } from '../lib/detection_engine/routes/rules/per
import { importRulesRoute } from '../lib/detection_engine/routes/rules/import_rules_route';
import { exportRulesRoute } from '../lib/detection_engine/routes/rules/export_rules_route';
import { findRulesStatusesRoute } from '../lib/detection_engine/routes/rules/find_rules_status_route';
import { internalFindRuleStatusRoute } from '../lib/detection_engine/routes/rules/internal_find_rule_status_route';
import { findRuleStatusInternalRoute } from '../lib/detection_engine/routes/rules/find_rule_status_internal_route';
import { getPrepackagedRulesStatusRoute } from '../lib/detection_engine/routes/rules/get_prepackaged_rules_status_route';
import {
createTimelinesRoute,
Expand Down Expand Up @@ -123,7 +123,7 @@ export const initRoutes = (
persistPinnedEventRoute(router, config, security);

findRulesStatusesRoute(router);
internalFindRuleStatusRoute(router);
findRuleStatusInternalRoute(router);

// Detection Engine Signals routes that have the REST endpoints of /api/detection_engine/signals
// POST /api/detection_engine/signals/status
Expand Down

0 comments on commit dc16bb8

Please sign in to comment.