-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Detection Engine] Adds Alert Suppression to ML Rules #181926
Merged
+2,503
−222
Merged
Changes from 1 commit
Commits
Show all changes
111 commits
Select commit
Hold shift + click to select a range
ad461bb
Add outline of integration test scenarios
rylnd 8c0f6c1
Fleshing out more of our suppression execution tests
rylnd 01bcf8e
Declare alert suppression fields as optional for ML rules
rylnd c42b339
Generated new types from new schema
rylnd b78c531
First legitimately failing test
rylnd cad4183
Extract executor params to interface
rylnd 5377c6d
Merge branch 'main' into ml_rule_alert_suppression
rylnd 6dbb88f
Adding more ML suppression functionality as typescript and tests dictate
rylnd e29c3d7
Declare ML rule to be suppressible
rylnd 12ad5f5
Add ML rule to general suppression schema tests
rylnd c8b7c6a
Add placeholder for ML executor functionality
rylnd 7f317cf
Declare our new executor parameters needed for rule suppression
rylnd 13408a3
Call bulkCreateSuppressedAlertsInMemory to ML rules
rylnd 703084f
Enable feature flag in FTR tests
rylnd b9de69e
Handle ML suppression params in rule converters
rylnd 7e63b4d
First passing integration test
rylnd 0caa342
Implementing most of our FTR test functionality
rylnd 99aaffe
Flesh out remaining API integration tests
rylnd ee86fb1
Update test description in response to feedback
rylnd b5e809d
Add non-destructive form filling task for ML rules
rylnd 10a9a42
Remove unused helper
rylnd f010c1c
Add cypress tests around creating/editing ML rules with suppression
rylnd 5358bc4
Add TODO for later
rylnd 4804eef
Add missing frontend logic to enable ML alert suppression
rylnd 03833d1
Fix ML executor unit tests
rylnd 184375d
Fix type error
rylnd 982649f
Add alert suppression fields to alerting integration snapshot
rylnd 241deaf
Allow cypress task to fill combobox with varying values
rylnd 651806f
Get most of the rule editing UI tests green
rylnd c738d51
Merge branch 'main' into ml_rule_alert_suppression
rylnd c9a371e
Fix type errors in prebuilt rule cypress test
rylnd a39c6ba
Merge branch 'main' into ml_rule_alert_suppression
rylnd fe2635f
Merge branch 'main' into ml_rule_alert_suppression
rylnd fc40364
Fix nondeterministic ordering of alerts
rylnd afa7db6
Merge branch 'main' into ml_rule_alert_suppression
rylnd 4a172ee
Fix missed merge conflicts
rylnd 1a92d58
Remove unused dependency from test helper
rylnd 60149d1
Merge branch 'main' into ml_rule_alert_suppression
rylnd eacb0b0
Remove unnecessary linting exception
rylnd 845a217
Fix type error in helper fn
rylnd 3c68939
Use empty array as input index for wrapSuppressedHits
rylnd 3914d74
Test that some timing information is used in the ML executor
rylnd 3f5d9c2
Remove TODO that we won't touch
rylnd 3d8a11e
Remove placeholder comment
rylnd 7901b5c
Remove unused variable
rylnd 3b25273
Merge branch 'main' into ml_rule_alert_suppression
rylnd f8145a2
Update test following upstream behavioral change
rylnd 7aa764a
Merge branch 'main' into ml_rule_alert_suppression
rylnd c54e3e9
Merge branch 'main' into ml_rule_alert_suppression
rylnd 9c79daf
Merge branch 'main' into ml_rule_alert_suppression
rylnd 922d8b0
Merge branch 'main' into ml_rule_alert_suppression
rylnd 56de5f6
Add copy for new ML rule warnings
rylnd 661a195
Add UX messaging for ML suppression edge cases
rylnd dfeb857
style: sort StepDefineRule arguments
rylnd 5273d1f
Provide ML fields on Define step of rule creation
rylnd 4955831
WIP: adding cypress tests for ML warning
rylnd eefa1c0
Install test ML jobs with a known/compatible group
rylnd cf5ceae
Use correct job ID in cypress tests
rylnd 87c079d
Replace duplicated, magic-stringed function for existing one
rylnd cf71e6d
ML Suppression Cypress tests are green
rylnd 65e16e8
More direct copy for user action
rylnd e6d85e2
Update ML warning copy per docs' suggestions
rylnd 9c2d143
Replace display names with actual Job IDs
rylnd 1aa170a
Extract ML rule validation logic into hook
rylnd fedb755
Relax requirement on number of rules
rylnd 61d24d4
Clean up existing ML cypress test
rylnd 99deac3
Bring back job display names for assertions
rylnd b0970bf
Re-enable ML rule edit tests
rylnd 1a99fd1
Robustify ML cypress tasks
rylnd 742503b
Ensure test has clean setup
rylnd f5cbaa5
Remove exclusivity from FTR tests, add TODO for re-skipping
rylnd 8e9d4c5
Fix suppression fields for non-ML cases
rylnd e6aae21
Merge pull request #9 from rylnd/ml_rule_suppression_warnings
rylnd 0cf3a51
Merge branch 'main' into ml_rule_alert_suppression
rylnd 792373e
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine a3117a1
Revert change to form field types
rylnd 9e77939
Update cypress tests to reflect latest copy
rylnd a42b966
Better assertion
rylnd df8e8a2
Fix mappings/aliases for v3 ML job archive
rylnd d71d5b0
Fix test assertion by loosening constraints
rylnd 5ab6506
Stop ALL potential jobs at the start of our ML tests
rylnd 5f1e4fa
Fix type error in test helper
rylnd 87c3331
Reset combobox state after every option "fill"
rylnd a789766
Merge branch 'main' into ml_rule_alert_suppression
rylnd 8f18a8e
Stop both datafeeds and jobs between ML tests
rylnd f451762
Don't fail if no jobs found during setup
rylnd 32a55fb
Disable ML suppression fields until they're loaded and available
rylnd 072e2f7
Add ML feature flag for serverless cypress
rylnd e78404c
Add read access to ML indices for security roles
rylnd 240e998
Skip FF-dependent cypress tests in MKI builds
rylnd 61ea144
Merge branch 'main' into ml_rule_alert_suppression
rylnd 550bc8e
Fix type error in integration test
rylnd 798fcb0
Add missing ML privileges from other role definitions
rylnd 8462043
Re-skip flaky ML API tests
rylnd 33ad209
Merge branch 'main' into ml_rule_alert_suppression
rylnd 3168eac
Extract hairy inline logic to named variables
rylnd 307e964
Update copy to be more direct
rylnd 6ce66d1
More precise instruction
rylnd 98ac972
Inline function-specific parameter
rylnd d152289
Add ML rules to Basic/Essentials workflow tests
rylnd ce395da
Add additional non-suppressible alert to better demonstrate suppression
rylnd f56840d
Merge branch 'main' into ml_rule_alert_suppression
rylnd d666eb0
Remove unused helper
rylnd 112dafe
Add comment indicating why tests are skipped in MKI
rylnd 006c88c
DRY up helper predicates
rylnd dd897d6
Send suppressed ML alerts to timeline as normal alerts
rylnd 1924596
Merge branch 'main' into ml_rule_alert_suppression
rylnd 96141dd
Test prebuilt rule workflows with ML Alert Suppression
rylnd 3aa105d
Abstract ML-related logic into helper hook
rylnd a9b410d
Remove unused parameter
rylnd 8e07763
Skip FTR suppression tests in MKI
rylnd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Add UX messaging for ML suppression edge cases
* Disables suppression fields if no relevant ML jobs are running (as we cannot retrieve field info) * Adds a warning message if not all relevant ML jobs are running (as we may be missing some field info) Next step is testing this; we don't currently have a way to run ML rules in cypress, but I'm going to attempt to copy the logic in our FTR to accomplish this.
commit 661a195e362f08db89bdfe7697b164d48e236178
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,7 @@ import type { DataViewBase } from '@kbn/es-query'; | |
import { FormattedMessage } from '@kbn/i18n-react'; | ||
import { useSetFieldValueWithCallback } from '../../../../common/utils/use_set_field_value_cb'; | ||
import { useRuleFromTimeline } from '../../../../detections/containers/detection_engine/rules/use_rule_from_timeline'; | ||
import { isMlRule } from '../../../../../common/machine_learning/helpers'; | ||
import { isJobStarted, isMlRule } from '../../../../../common/machine_learning/helpers'; | ||
import { hasMlAdminPermissions } from '../../../../../common/machine_learning/has_ml_admin_permissions'; | ||
import { hasMlLicense } from '../../../../../common/machine_learning/has_ml_license'; | ||
import { useMlCapabilities } from '../../../../common/components/ml/hooks/use_ml_capabilities'; | ||
|
@@ -93,6 +93,7 @@ import { NewTermsFields } from '../new_terms_fields'; | |
import { ScheduleItem } from '../../../rule_creation/components/schedule_item_form'; | ||
import { RequiredFields } from '../../../rule_creation/components/required_fields'; | ||
import { DocLink } from '../../../../common/components/links_to_docs/doc_link'; | ||
import { useInstalledSecurityJobs } from '../../../../common/components/ml/hooks/use_installed_security_jobs'; | ||
import { defaultCustomQuery } from '../../../../detections/pages/detection_engine/rules/utils'; | ||
import { MultiSelectFieldsAutocomplete } from '../multi_select_fields'; | ||
import { useLicense } from '../../../../common/hooks/use_license'; | ||
|
@@ -196,12 +197,24 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({ | |
const queryClient = useQueryClient(); | ||
|
||
const { isSuppressionEnabled: isAlertSuppressionEnabled } = useAlertSuppression(ruleType); | ||
const mlCapabilities = useMlCapabilities(); | ||
const [openTimelineSearch, setOpenTimelineSearch] = useState(false); | ||
const [indexModified, setIndexModified] = useState(false); | ||
const [threatIndexModified, setThreatIndexModified] = useState(false); | ||
const license = useLicense(); | ||
|
||
const mlCapabilities = useMlCapabilities(); | ||
const installedMlJobs = useInstalledSecurityJobs(); | ||
const [{ machineLearningJobId }] = useFormData<DefineStepRule>({ | ||
form, | ||
watch: ['machineLearningJobId'], | ||
}); | ||
const ruleMlJobs = installedMlJobs.jobs.filter((job) => machineLearningJobId.includes(job.id)); | ||
const numberOfRuleMlJobsStarted = ruleMlJobs.filter((job) => | ||
isJobStarted(job.jobState, job.datafeedState) | ||
).length; | ||
const noMlJobsStarted = numberOfRuleMlJobsStarted === 0; | ||
const someMlJobsStarted = !noMlJobsStarted && numberOfRuleMlJobsStarted !== ruleMlJobs.length; | ||
|
||
const esqlQueryRef = useRef<DefineStepRule['queryBar'] | undefined>(undefined); | ||
|
||
const isAlertSuppressionLicenseValid = license.isAtLeast(MINIMUM_LICENSE_FOR_SUPPRESSION); | ||
|
@@ -1068,22 +1081,32 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({ | |
</EuiText> | ||
} | ||
> | ||
<UseField | ||
path="groupByFields" | ||
component={MultiSelectFieldsAutocomplete} | ||
componentProps={{ | ||
browserFields: isEsqlRule(ruleType) | ||
? esqlSuppressionFields | ||
: termsAggregationFields, | ||
isDisabled: | ||
!isAlertSuppressionLicenseValid || | ||
areSuppressionFieldsDisabledBySequence || | ||
isEsqlSuppressionLoading, | ||
disabledText: areSuppressionFieldsDisabledBySequence | ||
? i18n.EQL_SEQUENCE_SUPPRESSION_DISABLE_TOOLTIP | ||
: alertSuppressionUpsellingMessage, | ||
}} | ||
/> | ||
<> | ||
<UseField | ||
path="groupByFields" | ||
component={MultiSelectFieldsAutocomplete} | ||
componentProps={{ | ||
browserFields: isEsqlRule(ruleType) | ||
? esqlSuppressionFields | ||
: termsAggregationFields, | ||
isDisabled: | ||
!isAlertSuppressionLicenseValid || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: here is lot logic in this condition, can we move it to the variable? |
||
areSuppressionFieldsDisabledBySequence || | ||
isEsqlSuppressionLoading || | ||
noMlJobsStarted, | ||
disabledText: areSuppressionFieldsDisabledBySequence | ||
? i18n.EQL_SEQUENCE_SUPPRESSION_DISABLE_TOOLTIP | ||
: noMlJobsStarted | ||
? i18n.MACHINE_LEARNING_SUPPRESSION_DISABLED_LABEL | ||
: alertSuppressionUpsellingMessage, | ||
}} | ||
/> | ||
{someMlJobsStarted && ( | ||
<EuiText size="xs" color="warning"> | ||
{i18n.MACHINE_LEARNING_SUPPRESSION_INCOMPLETE_LABEL} | ||
</EuiText> | ||
)} | ||
</> | ||
</RuleTypeEuiFormRow> | ||
|
||
<IntendedRuleTypeEuiFormRow | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
nit: personally I found ternary operators with more than 1 level hard to read, maybe moving this logic can simplify a little bit
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.
agree on this one: https://github.com/elastic/kibana/blob/main/STYLEGUIDE.mdx#only-use-ternary-operators-for-small-simple-code
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 applied 3168eac, but for the reasons stated there it's not going to be easy to move this logic into e.g. a helper function (that isn't defined within this component context, at least). LMK what you think.
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 only solution to nested ternary operators in that case I see as either extracting it into helper(which does not look to helpful) or put it is
useMemo
(or Immediately Invoked Function Expression)so,
would become
I would prefer a second approach. It is more readable to me + you would get a benefit of memoization(although not critical at all here)