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][Alerts] Replace schemas derived from FieldMaps with versioned alert schema #127218

Merged
merged 22 commits into from
Mar 30, 2022
Merged

[Security Solution][Alerts] Replace schemas derived from FieldMaps with versioned alert schema #127218

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7dff107
Replace schemas derived from FieldMaps with versioned alert schema
marshallmain Jan 31, 2022
b8c3c82
Merge branch 'main' into versioned-alert-schemas
marshallmain Mar 8, 2022
694180d
Import fixes and comment
marshallmain Mar 8, 2022
2ce2c37
Another import fix
marshallmain Mar 8, 2022
2f60cf6
Separate read and write schemas
marshallmain Mar 9, 2022
ecd7f49
Separate read and write schemas for common alert fields
marshallmain Mar 9, 2022
3f29171
fix import
marshallmain Mar 9, 2022
b1787a6
Update ALERT_RULE_PARAMETERS type
marshallmain Mar 10, 2022
96a1a71
Fix getField type
marshallmain Mar 10, 2022
4753672
Fix more types
marshallmain Mar 10, 2022
179c195
Merge branch 'main' into versioned-alert-schemas
kibanamachine Mar 14, 2022
a52cf69
Remove unneeded index signature from PersistenceAlertServiceResult
marshallmain Mar 15, 2022
3371fb8
Merge branch 'main' into versioned-alert-schemas
kibanamachine Mar 15, 2022
dac9ded
Merge branch 'main' into versioned-alert-schemas
marshallmain Mar 22, 2022
ed48a31
Merge branch 'main' into versioned-alert-schemas
marshallmain Mar 28, 2022
52ec700
Merge branch 'main' into versioned-alert-schemas
marshallmain Mar 29, 2022
be7965f
Fix types and tests
marshallmain Mar 29, 2022
63d0545
Update comment describing new schema process
marshallmain Mar 29, 2022
d8bae7e
Update Ancestor800 type
marshallmain Mar 29, 2022
8ab1020
Add modified PR description as initial README
marshallmain Mar 30, 2022
63223fe
Remove duplication in CommonAlertFields definition
marshallmain Mar 30, 2022
70f24ad
Add explicit undefined value for rule in mock
marshallmain Mar 30, 2022
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
3 changes: 3 additions & 0 deletions packages/kbn-rule-data-utils/src/technical_field_names.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const ALERT_RULE_INTERVAL = `${ALERT_RULE_NAMESPACE}.interval` as const;
const ALERT_RULE_LICENSE = `${ALERT_RULE_NAMESPACE}.license` as const;
const ALERT_RULE_CATEGORY = `${ALERT_RULE_NAMESPACE}.category` as const;
const ALERT_RULE_NAME = `${ALERT_RULE_NAMESPACE}.name` as const;
const ALERT_RULE_NAMESPACE_FIELD = `${ALERT_RULE_NAMESPACE}.namespace` as const;
const ALERT_RULE_NOTE = `${ALERT_RULE_NAMESPACE}.note` as const;
const ALERT_RULE_PARAMETERS = `${ALERT_RULE_NAMESPACE}.parameters` as const;
const ALERT_RULE_REFERENCES = `${ALERT_RULE_NAMESPACE}.references` as const;
Expand Down Expand Up @@ -109,6 +110,7 @@ const fields = {
ALERT_RULE_INTERVAL,
ALERT_RULE_LICENSE,
ALERT_RULE_NAME,
ALERT_RULE_NAMESPACE_FIELD,
ALERT_RULE_NOTE,
ALERT_RULE_PARAMETERS,
ALERT_RULE_REFERENCES,
Expand Down Expand Up @@ -163,6 +165,7 @@ export {
ALERT_RULE_INTERVAL,
ALERT_RULE_LICENSE,
ALERT_RULE_NAME,
ALERT_RULE_NAMESPACE_FIELD,
ALERT_RULE_NOTE,
ALERT_RULE_PARAMETERS,
ALERT_RULE_REFERENCES,
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/rule_registry/common/assets/field_maps/experimental_rule_field_map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import * as Fields from '../../../common/technical_rule_data_field_names';
import * as Fields from '../../technical_rule_data_field_names';

export const experimentalRuleFieldMap = {
[Fields.ALERT_INSTANCE_ID]: { type: 'keyword', required: true },
Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugins/rule_registry/common/assets/field_maps/technical_rule_field_map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { pickWithPatterns } from '../../../common/pick_with_patterns';
import * as Fields from '../../../common/technical_rule_data_field_names';

import { pickWithPatterns } from '../../pick_with_patterns';
import * as Fields from '../../technical_rule_data_field_names';
import { ecsFieldMap } from './ecs_field_map';

export const technicalRuleFieldMap = {
Expand Down
64 changes: 64 additions & 0 deletions x-pack/plugins/rule_registry/common/schemas/8.0.0/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* 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 { Values } from '@kbn/utility-types';
import {
ALERT_INSTANCE_ID,
ALERT_UUID,
ALERT_RULE_CATEGORY,
ALERT_RULE_CONSUMER,
ALERT_RULE_EXECUTION_UUID,
ALERT_RULE_NAME,
ALERT_RULE_PRODUCER,
ALERT_RULE_TYPE_ID,
ALERT_RULE_UUID,
SPACE_IDS,
ALERT_RULE_TAGS,
TIMESTAMP,
} from '@kbn/rule-data-utils';

/* DO NOT MODIFY THIS SCHEMA TO ADD NEW FIELDS. These types represent the alerts that shipped in 8.0.0.
Any changes to these types should be bug fixes so the types more accurately represent the alerts from 8.0.0.

If you are adding new fields for a new release of Kibana, create a new sibling folder to this one
for the version to be released and add the field(s) to the schema in that folder.

Then, update `../index.ts` to import from the new folder that has the latest schemas, add the
new schemas to the union of all alert schemas, and re-export the new schemas as the `*Latest` schemas.
*/

const commonAlertFieldNames = [
ALERT_RULE_CATEGORY,
ALERT_RULE_CONSUMER,
ALERT_RULE_EXECUTION_UUID,
ALERT_RULE_NAME,
ALERT_RULE_PRODUCER,
ALERT_RULE_TYPE_ID,
ALERT_RULE_UUID,
SPACE_IDS,
ALERT_RULE_TAGS,
TIMESTAMP,
];
export type CommonAlertFieldName800 = Values<typeof commonAlertFieldNames>;
Copy link
Contributor

Choose a reason for hiding this comment

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

curious to hear your thoughts about the naming convention here with CommonAlertFieldName800. If we plan to associate the version numbers with the release numbers I can see it getting a little awkward with no separators for the 800 part. If we are going to bump it by one with each change then we probably don't need to start with 800. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that ending with 800 is a bit awkward. I initially wanted to use something more like CommonAlertFieldName_8_0_0 at first, but the naming convention linter complains about using underscores if the type isn't all caps. Alerting uses a similar convention in the SO migrations logic which seems to have worked so far so I adopted that instead of trying to disable the linter rule for these type names.

If we are going to bump it by one with each change then we probably don't need to start with 800

With this proposal we wouldn't bump the number by 1, instead we'd increment it to the version of Kibana that's shipping the change, e.g. the next one might be CommonAlertFieldName830 for 8.3.0 if we make changes soon. But we haven't made changes in 8.1 or 8.2 so there would never be a CommonAlertFieldName810 or 820.

In the past we used integer version numbers, starting at 1 and incrementing on each Kibana version that shipped changes, for the legacy signals mappings and it was pretty confusing IMO. Most of the time when looking at version numbers what I wanted to now was what version of Kibana a particular version number shipped with so it would have saved time to just version the signals mappings with the Kibana version. Instead with the integer it has to be cross-referenced with Git history to figure out when it shipped.

The other possible option I considered was dropping the 800 from the name entirely and relying on the 8.0.0 in the folder path to differentiate this CommonAlertFieldName type from possible future CommonAlertFieldName types. I worried that that would make it too easy to accidentally import the wrong type though.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty nitpicky, but I'd be in favor of dropping the suffix entirely. This naming schema will become ambiguous if there's ever a minor or patch version > 10, or once we hit 10.x. Sure, it makes it easier to import the wrong version, but import paths matter and devs should be able to navigate that. If nothing else, code reviewers should be able to mitigate this possibility.


const commonAlertIdFieldNames = [ALERT_INSTANCE_ID, ALERT_UUID];
export type CommonAlertIdFieldName800 = Values<typeof commonAlertIdFieldNames>;
Comment on lines +34 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these two id fields are not included in CommonAlertFields800 and have their own union type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are used by Observability for the lifecycle rule executor logic. I'm not sure why they're separated out, but I kept them the same way they were in get_common_alert_fields.ts


export interface CommonAlertFields800 {
[ALERT_RULE_CATEGORY]: string;
[ALERT_RULE_CONSUMER]: string;
[ALERT_RULE_EXECUTION_UUID]: string;
[ALERT_RULE_NAME]: string;
[ALERT_RULE_PRODUCER]: string;
[ALERT_RULE_TYPE_ID]: string;
[ALERT_RULE_UUID]: string;
[SPACE_IDS]: string[];
[ALERT_RULE_TAGS]: string[];
[TIMESTAMP]: string;
}

export type AlertWithCommonFields800<T> = T & CommonAlertFields800;
20 changes: 20 additions & 0 deletions x-pack/plugins/rule_registry/common/schemas/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* 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 type {
CommonAlertFieldName800,
CommonAlertIdFieldName800,
CommonAlertFields800,
AlertWithCommonFields800,
} from './8.0.0';

export type {
CommonAlertFieldName800 as CommonAlertFieldNameLatest,
CommonAlertIdFieldName800 as CommonAlertIdFieldNameLatest,
CommonAlertFields800 as CommonAlertFieldsLatest,
AlertWithCommonFields800 as AlertWithCommonFieldsLatest,
};
9 changes: 3 additions & 6 deletions x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,13 @@ import {
TIMESTAMP,
VERSION,
} from '../../common/technical_rule_data_field_names';
import { CommonAlertFieldNameLatest, CommonAlertIdFieldNameLatest } from '../../common/schemas';
import { IRuleDataClient } from '../rule_data_client';
import { AlertExecutorOptionsWithExtraServices } from '../types';
import { fetchExistingAlerts } from './fetch_existing_alerts';
import {
CommonAlertFieldName,
CommonAlertIdFieldName,
getCommonAlertFields,
} from './get_common_alert_fields';
import { getCommonAlertFields } from './get_common_alert_fields';

type ImplicitTechnicalFieldName = CommonAlertFieldName | CommonAlertIdFieldName;
type ImplicitTechnicalFieldName = CommonAlertFieldNameLatest | CommonAlertIdFieldNameLatest;

type ExplicitTechnicalAlertFields = Partial<
Omit<ParsedTechnicalFields, ImplicitTechnicalFieldName>
Expand Down
26 changes: 2 additions & 24 deletions x-pack/plugins/rule_registry/server/utils/get_common_alert_fields.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@
* 2.0.
*/

import { Values } from '@kbn/utility-types';
import {
ALERT_INSTANCE_ID,
ALERT_UUID,
ALERT_RULE_CATEGORY,
ALERT_RULE_CONSUMER,
ALERT_RULE_EXECUTION_UUID,
Expand All @@ -22,30 +19,11 @@ import {
} from '@kbn/rule-data-utils';

import { AlertExecutorOptions } from '../../../alerting/server';
import { ParsedTechnicalFields } from '../../common/parse_technical_fields';

const commonAlertFieldNames = [
ALERT_RULE_CATEGORY,
ALERT_RULE_CONSUMER,
ALERT_RULE_EXECUTION_UUID,
ALERT_RULE_NAME,
ALERT_RULE_PRODUCER,
ALERT_RULE_TYPE_ID,
ALERT_RULE_UUID,
SPACE_IDS,
ALERT_RULE_TAGS,
TIMESTAMP,
];
export type CommonAlertFieldName = Values<typeof commonAlertFieldNames>;

const commonAlertIdFieldNames = [ALERT_INSTANCE_ID, ALERT_UUID];
export type CommonAlertIdFieldName = Values<typeof commonAlertIdFieldNames>;

export type CommonAlertFields = Pick<ParsedTechnicalFields, CommonAlertFieldName>;
import { CommonAlertFieldsLatest } from '../../common/schemas';

export const getCommonAlertFields = (
options: AlertExecutorOptions<any, any, any, any, any>
): CommonAlertFields => {
): CommonAlertFieldsLatest => {
return {
[ALERT_RULE_CATEGORY]: options.rule.ruleTypeName,
[ALERT_RULE_CONSUMER]: options.rule.consumer,
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/rule_registry/server/utils/persistence_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
import { WithoutReservedActionGroups } from '../../../alerting/common';
import { IRuleDataClient } from '../rule_data_client';
import { BulkResponseErrorAggregation } from './utils';
import { AlertWithCommonFieldsLatest } from '../../common/schemas';

export type PersistenceAlertService = <T>(
alerts: Array<{
Expand All @@ -27,7 +28,7 @@ export type PersistenceAlertService = <T>(
) => Promise<PersistenceAlertServiceResult<T>>;

export interface PersistenceAlertServiceResult<T> {
createdAlerts: Array<T & { _id: string; _index: string }>;
createdAlerts: Array<AlertWithCommonFieldsLatest<T> & { _id: string; _index: string }>;
errors: BulkResponseErrorAggregation;
}

Expand Down
178 changes: 178 additions & 0 deletions x-pack/plugins/security_solution/common/detection_engine/schemas/alerts/8.0.0/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
/*
* 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 {
ALERT_BUILDING_BLOCK_TYPE,
ALERT_REASON,
ALERT_RISK_SCORE,
ALERT_RULE_AUTHOR,
ALERT_RULE_CONSUMER,
ALERT_RULE_CREATED_AT,
ALERT_RULE_CREATED_BY,
ALERT_RULE_DESCRIPTION,
ALERT_RULE_ENABLED,
ALERT_RULE_FROM,
ALERT_RULE_INTERVAL,
ALERT_RULE_LICENSE,
ALERT_RULE_NAME,
ALERT_RULE_NAMESPACE_FIELD,
ALERT_RULE_NOTE,
ALERT_RULE_PARAMETERS,
ALERT_RULE_REFERENCES,
ALERT_RULE_RULE_ID,
ALERT_RULE_RULE_NAME_OVERRIDE,
ALERT_RULE_TAGS,
ALERT_RULE_TO,
ALERT_RULE_TYPE,
ALERT_RULE_UPDATED_AT,
ALERT_RULE_UPDATED_BY,
ALERT_RULE_UUID,
ALERT_RULE_VERSION,
ALERT_SEVERITY,
ALERT_STATUS,
ALERT_UUID,
ALERT_WORKFLOW_STATUS,
EVENT_KIND,
SPACE_IDS,
TIMESTAMP,
} from '@kbn/rule-data-utils';
// TODO: Create and import 8.0.0 versioned ListArray schema
import { ListArray } from '@kbn/securitysolution-io-ts-list-types';
// TODO: Create and import 8.0.0 versioned alerting-types schemas
import {
RiskScoreMapping,
SeverityMapping,
Threats,
Type,
} from '@kbn/securitysolution-io-ts-alerting-types';
import {
ALERT_ANCESTORS,
ALERT_DEPTH,
ALERT_ORIGINAL_TIME,
ALERT_RULE_ACTIONS,
ALERT_RULE_EXCEPTIONS_LIST,
ALERT_RULE_FALSE_POSITIVES,
ALERT_GROUP_ID,
ALERT_GROUP_INDEX,
ALERT_RULE_IMMUTABLE,
ALERT_RULE_MAX_SIGNALS,
ALERT_RULE_RISK_SCORE_MAPPING,
ALERT_RULE_SEVERITY_MAPPING,
ALERT_RULE_THREAT,
ALERT_RULE_THROTTLE,
ALERT_RULE_TIMELINE_ID,
ALERT_RULE_TIMELINE_TITLE,
ALERT_RULE_TIMESTAMP_OVERRIDE,
} from '../../../../field_maps/field_names';
// TODO: Create and import 8.0.0 versioned RuleAlertAction type
import { RuleAlertAction, SearchTypes } from '../../../types';
import { AlertWithCommonFields800 } from '../../../../../../rule_registry/common/schemas/8.0.0';

/* DO NOT MODIFY THIS SCHEMA TO ADD NEW FIELDS. These types represent the alerts that shipped in 8.0.0.
Any changes to these types should be bug fixes so the types more accurately represent the alerts from 8.0.0.

If you are adding new fields for a new release of Kibana, create a new sibling folder to this one
for the version to be released and add the field(s) to the schema in that folder.

Then, update `../index.ts` to import from the new folder that has the latest schemas, add the
new schemas to the union of all alert schemas, and re-export the new schemas as the `*Latest` schemas.
*/

export interface Ancestor800 {
rule?: string;
id: string;
type: string;
index: string;
depth: number;
}

export interface BaseFields800 {
[TIMESTAMP]: string;
[SPACE_IDS]: string[];
[EVENT_KIND]: 'signal';
[ALERT_ORIGINAL_TIME]: string | undefined;
// When we address https://github.com/elastic/kibana/issues/102395 and change ID generation logic, consider moving
// ALERT_UUID creation into buildAlert and keep ALERT_UUID with the rest of BaseFields fields. As of 8.2 though,
// ID generation logic is fragmented and it would be more confusing to put any of it in buildAlert
// [ALERT_UUID]: string;
[ALERT_RULE_CONSUMER]: string;
[ALERT_ANCESTORS]: Ancestor800[];
[ALERT_STATUS]: string;
[ALERT_WORKFLOW_STATUS]: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Off-topic, but it would be great to eventually have a jsdoc comment for every field in the alert schema containing a description (what it means), examples of values, and any other important information.

[ALERT_DEPTH]: number;
[ALERT_REASON]: string;
[ALERT_BUILDING_BLOCK_TYPE]: string | undefined;
[ALERT_SEVERITY]: string;
[ALERT_RISK_SCORE]: number;
// TODO: version rule schemas and pull in 8.0.0 versioned rule schema to define alert rule parameters type
[ALERT_RULE_PARAMETERS]: { [key: string]: SearchTypes };
[ALERT_RULE_ACTIONS]: RuleAlertAction[];
[ALERT_RULE_AUTHOR]: string[];
[ALERT_RULE_CREATED_AT]: string;
[ALERT_RULE_CREATED_BY]: string;
[ALERT_RULE_DESCRIPTION]: string;
[ALERT_RULE_ENABLED]: boolean;
[ALERT_RULE_EXCEPTIONS_LIST]: ListArray;
[ALERT_RULE_FALSE_POSITIVES]: string[];
[ALERT_RULE_FROM]: string;
[ALERT_RULE_IMMUTABLE]: boolean;
[ALERT_RULE_INTERVAL]: string;
[ALERT_RULE_LICENSE]: string | undefined;
[ALERT_RULE_MAX_SIGNALS]: number;
[ALERT_RULE_NAME]: string;
[ALERT_RULE_NAMESPACE_FIELD]: string | undefined;
[ALERT_RULE_NOTE]: string | undefined;
[ALERT_RULE_REFERENCES]: string[];
[ALERT_RULE_RISK_SCORE_MAPPING]: RiskScoreMapping;
[ALERT_RULE_RULE_ID]: string;
[ALERT_RULE_RULE_NAME_OVERRIDE]: string | undefined;
[ALERT_RULE_SEVERITY_MAPPING]: SeverityMapping;
[ALERT_RULE_TAGS]: string[];
[ALERT_RULE_THREAT]: Threats;
[ALERT_RULE_THROTTLE]: string | undefined;
[ALERT_RULE_TIMELINE_ID]: string | undefined;
[ALERT_RULE_TIMELINE_TITLE]: string | undefined;
[ALERT_RULE_TIMESTAMP_OVERRIDE]: string | undefined;
[ALERT_RULE_TO]: string;
[ALERT_RULE_TYPE]: Type;
[ALERT_RULE_UPDATED_AT]: string;
[ALERT_RULE_UPDATED_BY]: string;
[ALERT_RULE_UUID]: string;
[ALERT_RULE_VERSION]: number;
'kibana.alert.rule.risk_score': number;
'kibana.alert.rule.severity': string;
'kibana.alert.rule.building_block_type': string | undefined;
[key: string]: SearchTypes;
}

// This type is used after the alert UUID is generated and stored in the _id and ALERT_UUID fields
export interface WrappedFields800<T extends BaseFields800> {
_id: string;
_index: string;
_source: T & { [ALERT_UUID]: string };
}

export interface EqlBuildingBlockFields800 extends BaseFields800 {
[ALERT_GROUP_ID]: string;
[ALERT_GROUP_INDEX]: number;
[ALERT_BUILDING_BLOCK_TYPE]: 'default';
}

export interface EqlShellFields800 extends BaseFields800 {
[ALERT_GROUP_ID]: string;
[ALERT_UUID]: string;
}

export type EqlBuildingBlockAlert800 = AlertWithCommonFields800<EqlBuildingBlockFields800>;

export type EqlShellAlert800 = AlertWithCommonFields800<EqlShellFields800>;

export type GenericAlert800 = AlertWithCommonFields800<BaseFields800>;

// This is the type of the final generated alert including base fields, common fields
Copy link
Contributor

Choose a reason for hiding this comment

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

This is all really great, thank you!

// added by the alertWithPersistence function, and arbitrary fields copied from source documents
export type DetectionAlert800 = GenericAlert800 | EqlShellAlert800 | EqlBuildingBlockAlert800;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how union type is gonna work here, maybe missing something.

Please check this example where properties x, y and z are not accessible without an additional type casting:

I'd expect similar issues with access to fields in the real DetectionAlert800: ALERT_GROUP_INDEX is defined as a number but available as SearchTypes in DetectionAlert800

export interface EqlBuildingBlockFields800 extends BaseFields800 {
  [ALERT_GROUP_ID]: string;
  [ALERT_GROUP_INDEX]: number;
  [ALERT_BUILDING_BLOCK_TYPE]: 'default';
}

I think it's going to get worse when we'll need to start adding more versions of DetectionAlert to the final union type:

// When new Alert schemas are created for new Kibana versions, add the DetectionAlert type from the new version
// here, e.g. `export type DetectionAlert = DetectionAlert800 | DetectionAlert820` if a new schema is created in 8.2.0
export type DetectionAlert = DetectionAlert800;

I think what we need here instead of using the union type is constructing an interface manually that would:

  • Make all the common (base) fields required
  • Make all the alert type-specific fields optional (x | undefined)
export type DetectionAlert800 = 
  CommonAlertFields800 & 
  BaseFields800 & 
  Partial<EqlShellFields800> & 
  Partial<EqlBuildingBlockFields800>;

Still, combining multiple versions into a single DetectionAlert interface is a little bit unclear to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, TS has a pretty nasty bug in the implementation of deeply nested type intersection (microsoft/TypeScript#47935) which can become a source of bugs in the code (unless all the fields in the alert schema will be flat).

Copy link
Contributor Author

@marshallmain marshallmain Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd expect similar issues with access to fields in the real DetectionAlert800: ALERT_GROUP_INDEX is defined as a number but available as SearchTypes in DetectionAlert800

This is working as intended IMO - when an alert is first retrieved, without additional checks at runtime the type of ALERT_GROUP_INDEX really could be anything. Once we add the full types for ALERT_RULE_PARAMETERS though, the field alert[ALERT_RULE_PARAMETERS].type can be used as a discriminant at runtime to narrow the type down to EQL alerts only. At that point the type should be EqlBuildingBlockAlert800 | EqlShellAlert800, so we'd still need some other discriminant between those 2 types. But in general for alerts from different types of rules, we can use a known field like alert[ALERT_RULE_PARAMETERS].type as a discriminant.

Alternatively, developers can fetch ALERT_GROUP_INDEX, accept that its type is SearchTypes, and then do extra runtime validation on the retrieved value to ensure that it's not undefined or an object or some other unexpected value.

In both cases though I think it's a feature that ALERT_GROUP_INDEX has a very general type if it's retrieved from DetectionAlert - we're warning developers that the value could be anything and they need to do more validation there. For fields that are common and required across all alert types, e.g. ALERT_RULE_DESCRIPTION, the type system can convey that the value received from any DetectionAlert will always be string and extra validation isn't needed.

Loading