-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][Exceptions] Implement exceptions for ML rules #84006
Conversation
@elasticmachine merge upstream |
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.
Added some initial comments, mostly around typings. Generally this looks great, but I'd like to pair with you on desk-testing this if possible; I wanna see what you've been using to generate anomaly data!
@@ -197,8 +197,10 @@ export const useFetchIndex = ( | |||
); | |||
|
|||
useEffect(() => { | |||
if (!isEmpty(indexNames) && !isEqual(previousIndexesName.current, indexNames)) { | |||
if (!isEqual(previousIndexesName.current, indexNames)) { |
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 change might be redundant with mine
@@ -15,6 +14,7 @@ import { RuleTypeParams, RefreshTypes } from '../types'; | |||
import { singleBulkCreate, SingleBulkCreateResponse } from './single_bulk_create'; | |||
import { AnomalyResults, Anomaly } from '../../machine_learning'; | |||
import { BuildRuleMessage } from './rule_messages'; | |||
import { SearchResponse } from '../../types'; |
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 see that we're continuing to diverge from the official typings, but I couldn't find an explanation as to why (I was able to revert these without type errors). If we have an unsupported use case we should probably file an issue upstream.
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 think I was playing around with the types as well and forgot to switch this one back...got curious about the duplicates
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.
Ah I got an error when changing it back and tracked it down. The search response for KQL query rules is typed based on the custom SearchResponse
type because it is usinghits.total
which looks to be typed (incorrectly) as number
in the official SearchResponse
type. To share the "filter against lists" code with the KQL rules then the ML bulk create logic has to accept our custom SearchResponse
.
Need to investigate further why the upstream SearchResponse.hits.total
type doesn't match the expected type based on https://www.elastic.co/guide/en/elasticsearch/reference/current/search-search.html
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.
Looks like it was a number
before 7.0: https://www.elastic.co/guide/en/elasticsearch/reference/current/breaking-changes-7.0.html#hits-total-now-object-search-response
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.
The types package is out of date by over a year.
There's an issue to include up-to-date types with the client, but nothing has been merged. We may be better off preferring to use our own SearchResponse
type in general over the unmaintained SearchResponse
from ES 5 - until there are centrally maintained response types.
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.
Yeah, this has been an issue for me in the past too. I think we're on our own until those are updated...
// TS does not allow .every to be called on val as-is, even though every type in the union | ||
// is an array. https://github.com/microsoft/TypeScript/issues/36390 | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return (val as any[]).every((subVal: string | number | boolean | object) => |
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.
If we get rid of the as any[]
and leave subVal
implicitly typed, could we change the comment to a @ts-expect-error
instead?
@@ -71,13 +80,19 @@ export const createSetToFilterAgainst = async ({ | |||
return matchedListItemsSet; | |||
}; | |||
|
|||
export const filterEventsAgainstList = async ({ | |||
export const filterEventsAgainstList = async <T>({ |
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.
😎
if (isStringableType(valueField)) { | ||
acc.add(valueField.toString()); | ||
} else if (isStringableArray(valueField)) { | ||
valueField.forEach((subVal: string | number | boolean) => acc.add(subVal.toString())); |
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.
Annotation of subVal
here shouldn't be necessary, but maybe I'm missing something...
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.
You're right, I think I added this before changing the return type of isStringableArray
from val is string[] | number[] | boolean[]
to Array<string | number | boolean
and forgot to change it
@@ -273,15 +283,16 @@ export const signalRulesAlertType = ({ | |||
}); | |||
// The legacy ES client does not define failures when it can be present on the structure, hence why I have the & { failures: [] } |
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.
It looks like our local, artisanal SearchResponse
type now makes this comment/code obsolete, so that answers part of my earlier question. I'd still like to be able to extend the upstream type rather than diverge entirely, though, if at all possible.
@@ -49,6 +52,17 @@ export const getAnomalies = async ( | |||
}, | |||
}, | |||
], | |||
must_not: buildExceptionFilter({ |
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 see in other places we're conditionally adding this filter based on the length of exceptionItems
; does specifying must_not: undefined
work in the general case here?
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.
Yeah, the logic around those conditions is to prevent an empty object from showing up in the queries, like
{
bool: {}
}
Here if the exception filter comes back undefined
then must_not
will be omitted from the final query that gets sent to ES, but the filter
inside bool
will always exist so we won't end up with any empty objects.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Looks like the exception modal is falling back to using the Alert Actions: Lines 77 to 87 in 87f817e
Rule Details Action: Line 557 in 87f817e
I think we can go ahead and fix this for future rules by setting the That'll fix it for future rules, but existing ones will still have this issue. As opposed to modifying the above touch points, perhaps we query for the ML index within initial load of the |
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.
Checked out and desk tested a bunch of different scenarios (individual exception items, value-lists, against array fields, etc) and exceptions seem to be working great! 🙌 Only issue I encountered was in adding exceptions via the modal as we don't send the ml index to the modal so field validation won't pass (more details in my other comment). Going to request changes for fixing this, but once fixed this LGTM! 👍
I mentioned a spot or two for how we could fix the issue, but let me know if you have any other ideas here, and happy to pair on this one as well since it's a bit more UI-related, just let me know! 🙂
@spong I'm leaning towards fetching the ML index on initial load. That'll work for existing rules and new rules and also avoid the possibility of the index stored in the rule getting out of sync with the ML job if the ML team adds the ability to change the anomaly index of a job in the future. I'll try to implement that tomorrow. |
} | ||
}, [jobs, ruleIndices]); | ||
|
||
const [isIndexPatternLoading, { indexPatterns }] = useFetchIndex(memoRuleIndices); |
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.
You'll want to add this logic to the Edit Modal component as well edit_exception_moddal/index.tsx
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
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.
Checked out the latest changes and verified all touch points of the exceptions modal now populate with the fields/values from the corresponding ml index. Thanks for all the goodness here @marshallmain! LGTM! 🙂 🎉
Note: Stumbled across a similar issue on rule create/edit with the override suggestions -- I've opened #84836 for that.
…astic#84006) * Implement exceptions for ML rules * Remove unused import * Better implicit types * Retrieve ML rule index pattern for exception field suggestions and autocomplete * Add ML job logic to edit exception modal * Remove unnecessary logic change Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…4006) (#84841) * Implement exceptions for ML rules * Remove unused import * Better implicit types * Retrieve ML rule index pattern for exception field suggestions and autocomplete * Add ML job logic to edit exception modal * Remove unnecessary logic change Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* master: (40 commits) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992) [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006) ...
* master: (236 commits) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992) [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006) ...
Summary
Adds exceptions for ML rules through the UI and implements them in the backend. Also implements the ability to use value list exceptions on arrays of values like
user.name: ["root", "some_user"]
which is important for ML rules. Documents created by the ML plugin use arrays for most fields, even when they only contain a single value.Checklist
Delete any items that are not applicable to this PR.
For maintainers