Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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][Alerts] Replace schemas derived from FieldMaps with versioned alert schema #127218
[Security Solution][Alerts] Replace schemas derived from FieldMaps with versioned alert schema #127218
Changes from all commits
7dff107
b8c3c82
694180d
2ce2c37
2f60cf6
ecd7f49
3f29171
b1787a6
96a1a71
4753672
179c195
a52cf69
3371fb8
dac9ded
ed48a31
52ec700
be7965f
63d0545
d8bae7e
8ab1020
63223fe
70f24ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
andz
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 anumber
but available asSearchTypes
inDetectionAlert800
I think it's going to get worse when we'll need to start adding more versions of
DetectionAlert
to the final union type:I think what we need here instead of using the union type is constructing an interface manually that would:
x | undefined
)Still, combining multiple versions into a single
DetectionAlert
interface is a little bit unclear to me.There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forALERT_RULE_PARAMETERS
though, the fieldalert[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 beEqlBuildingBlockAlert800 | 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 likealert[ALERT_RULE_PARAMETERS].type
as a discriminant.Alternatively, developers can fetch
ALERT_GROUP_INDEX
, accept that its type isSearchTypes
, 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 fromDetectionAlert
- 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 anyDetectionAlert
will always bestring
and extra validation isn't needed.