-
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
[Security Solution][Detection Engine] adds alert suppression for New Terms rule type #178294
[Security Solution][Detection Engine] adds alert suppression for New Terms rule type #178294
Conversation
…taliidm/kibana into de_8_14/new_terms_suppression
…taliidm/kibana into de_8_14/new_terms_suppression
Pinging @elastic/security-detection-engine (Team:Detection Engine) |
|
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.
LGTM
...2e/detection_response/detection_engine/rule_creation/common_flows_supression_ess_basic.cy.ts
Outdated
Show resolved
Hide resolved
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.
security-engineering-productivity changes LGTM, even so, please take into consideration the following:
-
As the FF has been enabled in
x-pack/test/security_solution_cypress/config.ts
all the tests executed for both ESS and Serverless on PRs will be affected by that change, not only the ones were are specifically set at the top of the spec file. -
On MKI environments, we don't have the ability to enable FF through the automation, once the changes asked regarding tagging are implemented, all the serverless tests that have the feature flag will fail on the periodic pipeline. cc @yctercero
-
I saw you are doing changes as well in archives living in
x-pack/test/functional/es_archives/security_solution
, now we are able to reuse archives on that location in our Cypress tests as well. Letting you know in case that this new functionality might facilitate your work.
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.
Rule Management Area LGTM 👍
I checked the PR locally in ESS and Serverless for one field alert suppression and haven't found any problems.
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 gave a first pass here and everything looks GREAT. Pretty straightforward overall, with some reasonable refactorings that didn't explode the diff too much.
I had a few nits, but they were mainly around documentation/posterity stuff.
I have NOT reviewed the integration/cypress tests yet. I plan to go through those alongside the EQL suppression branch to compare/contrast coverage and approach. If I have any suggestions for adding coverage, we can address those after this gets merged.
Between the plentiful tests, the green build, and the feature flag, I'm approving this as is. I did check this out locally, tried a few different suppression scenarios, tried to break the rule creation form, etc. Everything worked for me!
.../public/detection_engine/rule_management/components/rule_details/rule_definition_section.tsx
Show resolved
Hide resolved
|
||
describe('useAlertSuppression', () => { | ||
it('should return isSuppressionEnabled false if rule Type exists in SUPPRESSIBLE_ALERT_RULES and Feature Flag is disabled', () => { |
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: the should
is unnecessary here. I'm going to die on this hill 😅 .
it('should return isSuppressionEnabled false if rule Type exists in SUPPRESSIBLE_ALERT_RULES and Feature Flag is disabled', () => { | |
it('returns isSuppressionEnabled: false when rule Type exists in SUPPRESSIBLE_ALERT_RULES and Feature Flag is disabled', () => { |
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.
a lot of other tests have should
, which is used widely across app.
What the criteria to use/not use should
in your opinion?
Maybe based on it, we can draw some guideline on its usage?
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's not a big deal, but I have never encountered a situation where the addition of "should" added any clarity to a test's description.
.../server/lib/detection_engine/rule_types/new_terms/bulk_create_suppressed_alerts_in_memory.ts
Outdated
Show resolved
Hide resolved
experimentalFeatures: ExperimentalFeatures | undefined; | ||
} | ||
/** | ||
* bulk create and suppress alerts in memory, |
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: these comments don't provide much more information than the name does. I would mention that this is explicitly for new terms alerts, or at least deals with EventsAndTerms[]
instead of normal alerts.
The note about missing fields logic is nice, but it would be nice to discuss it more explicitly, i.e. "if {@param alertSuppression.missingFieldsStrategy} is X, alerts missing the suppression field(s) will not be suppressed."
But: typing out the above, this doesn't feel like the right place to document that behavior. I would capture it in documentation, and ideally a unit test for this function, otherwise an integration test of some kind.
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 added a bit more details to description, mentioned difference with common utility.
There multiple FTR tests that cover missing fields logic:
Lines 874 to 981 in ff8a41f
it('should not suppress alerts with missing fields if configured so', async () => { | |
const id = uuidv4(); | |
const firstTimestamp = '2020-10-28T05:45:00.000Z'; | |
const secondTimestamp = '2020-10-28T06:10:00.000Z'; | |
const historicalDocuments = [ | |
{ | |
host: { name: 'host-a', ip: '127.0.0.1' }, | |
'agent.version': 1, | |
id, | |
'@timestamp': historicalWindowStart, | |
}, | |
]; | |
const firstExecutionDocuments = [ | |
{ | |
id, | |
'@timestamp': firstTimestamp, | |
host: { name: 'host-a', ip: '127.0.0.3' }, | |
}, | |
{ | |
id, | |
'@timestamp': firstTimestamp, | |
host: { name: 'host-a', ip: '127.0.0.4' }, | |
}, | |
{ | |
id, | |
'@timestamp': firstTimestamp, | |
host: { ip: '127.0.0.5' }, // doc 1 with missing host.name field | |
}, | |
{ | |
id, | |
'@timestamp': firstTimestamp, | |
host: { ip: '127.0.0.6' }, // doc 2 with missing host.name field | |
}, | |
]; | |
const secondExecutionDocuments = [ | |
{ | |
host: { name: 'host-a', ip: '127.0.0.10' }, | |
id, | |
'@timestamp': secondTimestamp, | |
}, | |
{ | |
host: { ip: '127.0.0.11' }, // doc 3 with missing host.name field | |
id, | |
'@timestamp': secondTimestamp, | |
}, | |
]; | |
await indexListOfDocuments([ | |
...historicalDocuments, | |
...firstExecutionDocuments, | |
...secondExecutionDocuments, | |
]); | |
const rule: NewTermsRuleCreateProps = { | |
...getCreateNewTermsRulesSchemaMock('rule-1', true), | |
new_terms_fields: ['host.ip'], | |
query: `id: "${id}"`, | |
index: ['ecs_compliant'], | |
history_window_start: historicalWindowStart, | |
alert_suppression: { | |
group_by: ['host.name'], | |
duration: { | |
value: 300, | |
unit: 'm', | |
}, | |
missing_fields_strategy: 'doNotSuppress', | |
}, | |
from: 'now-35m', | |
interval: '30m', | |
}; | |
const { previewId } = await previewRule({ | |
supertest, | |
rule, | |
timeframeEnd: new Date('2020-10-28T06:30:00.000Z'), | |
invocationCount: 2, | |
}); | |
const previewAlerts = await getPreviewAlerts({ | |
es, | |
previewId, | |
sort: ['host.name', ALERT_ORIGINAL_TIME], | |
}); | |
expect(previewAlerts.length).toEqual(4); | |
expect(previewAlerts[0]._source).toEqual({ | |
...previewAlerts[0]._source, | |
[ALERT_SUPPRESSION_TERMS]: [ | |
{ | |
field: 'host.name', | |
value: ['host-a'], | |
}, | |
], | |
[ALERT_ORIGINAL_TIME]: firstTimestamp, | |
[ALERT_SUPPRESSION_START]: firstTimestamp, | |
[ALERT_SUPPRESSION_END]: secondTimestamp, | |
[ALERT_SUPPRESSION_DOCS_COUNT]: 2, | |
}); | |
// rest of alerts are not suppressed and do not have suppress properties | |
previewAlerts.slice(1).forEach((previewAlert) => { | |
const source = previewAlert._source; | |
expect(source).toHaveProperty('id', id); | |
expect(source).not.toHaveProperty(ALERT_SUPPRESSION_DOCS_COUNT); | |
expect(source).not.toHaveProperty(ALERT_SUPPRESSION_END); | |
expect(source).not.toHaveProperty(ALERT_SUPPRESSION_TERMS); | |
expect(source).not.toHaveProperty(ALERT_SUPPRESSION_DOCS_COUNT); | |
}); | |
}); |
...ity_solution/server/lib/detection_engine/rule_types/utils/get_is_alert_suppression_active.ts
Show resolved
Hide resolved
...ins/security_solution/server/lib/detection_engine/rule_types/utils/suppression_utils.test.ts
Outdated
Show resolved
Hide resolved
...ins/security_solution/server/lib/detection_engine/rule_types/utils/wrap_suppressed_alerts.ts
Show resolved
Hide resolved
x-pack/test/functional/es_archives/security_solution/ecs_compliant/mappings.json
Show resolved
Hide resolved
...security_solution_api_integration/test_suites/detections_response/utils/alerts/get_alerts.ts
Show resolved
Hide resolved
...y_solution_api_integration/test_suites/detections_response/utils/alerts/get_opened_alerts.ts
Outdated
Show resolved
Hide resolved
Thanks for review and feedback, @rylnd |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: cc @vitaliidm |
This was missed in a previous merge of #178294, but was somehow not caught by CI. Suspicious!
Summary
getOpenAlerts
test function, which returned closed alerts as wellUI
Checklist
Functional changes are hidden behind a feature flag
Feature flag
alertSuppressionForNewTermsRuleEnabled
Functional changes are covered with a test plan and automated tests.
Test plan: https://github.com/elastic/security-team/pull/9045
Stability of new and changed tests is verified using the Flaky Test Runner.
Cypress ESS: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5547
Cypress Serverless: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5548
FTR ESS: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5596
FTR Serverless: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5597
Comprehensive manual testing is done by two engineers: the PR author and one of the PR reviewers. Changes are tested in both ESS and Serverless.
Mapping changes are accompanied by a technical design document. It can be a GitHub issue or an RFC explaining the changes. The design document is shared with and approved by the appropriate teams and individual stakeholders.
Existing AlertSuppression schema field is used for New terms rule, the one that used for Query and IM rules.
where
elastic/security-docs#5030