[Detection Engine][Rule Suppression] Add Suppression to EQL Non-sequence based queries #176422
Conversation
…hema (#176391) ## Summary - Address schema changes - Add Tests in rule_converters to validate converting from/to snake case to/from camel case
WafaaNasr
added
Team: SecuritySolution
|
/ci |
|
/ci |
|
/ci |
Can't reproduce it anymore. Thanks for fixing it.
I agree on point 1, it's temporary, so we might just leave it there as it is. I'll leave it with you to decide how do you want to proceed here. |
There was a problem hiding this comment.
Rule Management codeowners changes LGTM 👍
Reviewed the corresponding diffs, but didn't test the PR -- relying on the owners of the feature here.
Really appreciate all the details in the PR description -- it was helpful to get some context before reviewing the change. Thank you to whoever spent their time on this!
@rylnd Just curious what's the strategy for releasing this feature. Since there's a dedicated feature flag, does that mean that after merging this PR it's not going to go live in Serverless next week, and acceptance/exploratory testing will be done + writing the docs, all after the FF?
.../security_solution/common/api/detection_engine/model/rule_schema/rule_request_schema.mock.ts
Outdated
Show resolved
Hide resolved
| @@ -46,6 +46,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) { | |||
| '--xpack.ruleRegistry.unsafe.legacyMultiTenancy.enabled=true', | |||
| `--xpack.securitySolution.enableExperimental=${JSON.stringify([ | |||
| 'alertSuppressionForNewTermsRuleEnabled', | |||
| 'alertSuppressionForNonSequenceEqlRuleEnabled', | |||
There was a problem hiding this comment.
As the FF has been activated here, all the tests are going to be affected by it, not only the ones where the FF has specifically added at the top of the spec file.
There was a problem hiding this comment.
As far as I'm aware, this is still the only way to run a cypress test locally with a feature flag enabled (https://github.com/elastic/security-team/issues/7423). Please correct me if that's changed.
The global scope is unfortunate, but we don't really have an alternative. If the flag is not enabled globally, then a developer attempting to run a FF test would see it pass on CI and then fail locally; how would they know to set the global config in order to see the test pass?
There was a problem hiding this comment.
You are completely right.
If I'm not mistaken that behaviour should be documented so everybody should be aware about it. I understand your point, if you prefer to have it globally then you can remove the FF from the spec files.
My comment was more a reminder about what the behaviour is going to be :)
There was a problem hiding this comment.
if you prefer to have it globally
I would certainly prefer not to set the global configuration, as it's both onerous and broad. But until https://github.com/elastic/security-team/issues/7423 is addressed, it's the only way to develop cypress tests requiring a feature flag.
| describe( | ||
| 'Detection Rule Creation - EQL Rules - With Alert Suppression', | ||
| { | ||
| tags: ['@ess', '@serverless'], |
There was a problem hiding this comment.
This test should be marked as @serverless. Currently, there is no mechanism to enable through configuration FF on MKI environments, so this test is going to start failing. cc @yctercero
There was a problem hiding this comment.
You mean @skipInServerless, right?
There was a problem hiding this comment.
If we can also leave a note or issue reference when we use the skip tags to denote if there's a bug or issue that needs follow up or if it's an intentional skip that does not require any follow up.
There was a problem hiding this comment.
Yes, our squashing strategy has unfortunately severely reduced the utility of commit messages; the only way to pass along context is through declarative code or code comments :(
...on_response/detection_engine/rule_creation/event_correlation_rule_suppression_sequence.cy.ts
Outdated
Show resolved
Hide resolved
| describe( | ||
| 'Detection Rule Creation - EQL Rules - With Alert Suppression - Serverless Essentials License', | ||
| { | ||
| tags: ['@serverless'], |
There was a problem hiding this comment.
| @@ -35,6 +35,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) { | |||
| { product_line: 'cloud', product_tier: 'complete' }, | |||
| ])}`, | |||
| `--xpack.securitySolution.enableExperimental=${JSON.stringify([ | |||
| 'alertSuppressionForNonSequenceEqlRuleEnabled', | |||
There was a problem hiding this comment.
|
I did the desk testing. Functionality seems to be working great. Although, I had one question. In alert table, we can see one icon for suppressed alerts. But I cannot see that icon in timeline. Is this expected? Please see below video for the demo. eql_supressed_alerts_timeline_icon.mov |
There was a problem hiding this comment.
approving for the Threat Hunting Investigations team because @logeekal did the review and his question was answered!
The name/description indicate that this should contain an index pattern, but it did not.
@banderror that's mostly correct. We plan merge with the FF disabled, and use the next week or so for testing. Docs are already in review so we'll just need to make minor amendments depending on the outcome of testing/release. At a high level, this PR is relatively low risk as it mainly reuses existing suppression code, and applies it to candidate EQL alerts in a manner nearly identical to several other rule types. |
Without a mechanism to enable this feature flag in those environments, these tests will fail and block operations.
|
@MadameSheema I added the |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @rylnd @WafaaNasr |
I'm open to add more tags and I personally believe that is a pretty good idea, but there are some objections regarding that. @yctercero @maximpn thoughts around @rylnd proposal? |
@MadameSheema AFAIK the majority of feature flags are transient though it's matter of months or years until a feature flag is permanently enabled and removed. If we decide to add Additionally I'd rather leave it to the test to properly set up the environment to enable proper feature flags than adding a new tag. In a case when a test must be skipped it's should be enough to have @yctercero WDYT? |
|
@maximpn thanks for your feedback!
This is true, and I personally see this behavior as a feature. Consider the alternative: if a test is skipped with We can do that with a comment, or a tag; I don't really care how we do it. But seeing
No disagreement here, the tag makes it very clear that the test is skipped. But it tells us nothing about why it was skipped, which is my iissue.
I agree, that's the correct solution. What we're trying to address are symptoms caused by inadequate tooling. |
|
Hi @rylnd, I see your point. Do you mind creating a discussion ticket? I think we could assemble all the information there and have more opinions in a centralised way. |
Summary
Checklist
Internal documentation request (Elastic employees)template. The following information is included: feature flags used, target ESS version, planned timing for ESS and Serverless releases.Related Issues
afteresArchive path [Detection Engine][Rule Suppression] Disable EQL sequence suppression #178531Screenshots/recordings
Non-Sequence Suppression
EQL-NonseqRulecreation.mov
eqlsuppressionarray.mov
Eql-investigateInTimeline.mov
Disabled Sequence Suppression
DisableSequenceEqlsuppression.mov