-
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
[EDR Workflows] Enable response actions in base rule params #194796
[EDR Workflows] Enable response actions in base rule params #194796
Conversation
/ci |
/ci |
…nto response-actions-in-base-rule
/ci |
/ci |
/ci |
/ci |
…nto response-actions-in-base-rule
/ci |
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
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, given the following:
- adds optional parameter to some rule types
- will be rolled out with an "intermediate release" to allow rollback /zdt in serverless (as noted in PR top comment)
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.
Files impacting elastic/security-defend-workflows
team LGTM 👍
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @tomsonpl |
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.
@tomsonpl The changes look good and straightforward overall. Moving response_actions
to BaseOptionalFields
schema allows to simplify the implementation. My concern that not all rule types support response actions. shouldShowResponseActions()
defines which rule type support them. Meantime API uses Zod schemas to verify accepted fields. It means that response_actions
field will be accepted by all rule types and saved in a corresponding SO. It doesn't look like expected behavior. In that case route's implementation should be tweaked to clear sent response actions for unsupported rule types.
@@ -74,6 +74,11 @@ components: | |||
throttle: | |||
$ref: './common_attributes.schema.yaml#/components/schemas/RuleActionThrottle' | |||
|
|||
response_actions: |
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.
Previously we discussed that only specific rule types support response actions. Adding it to base BaseOptionalFields
schema will make it possible passing response_actions
via rule CRUD API. Does it mean all rule types support response actions now?
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.
Thanks Maxim 👍
Yes, you got it right. And it's my fault that I brought the confusion. Previously we were more than sure that not all rules support agent.id in alerts, but apparently it is depending fully on the source of alert. We discussed this more in slack and I tested with more rules and apparently we can enable that in ALL rule types.
The shouldShowResponseActions
is going to be there just for the first release of serverless (intermediate release) to have the logic on backend, but not expose response_actions in the UI of newly added types of rules. But after a week with another serverless release I am going to remove this check totally.
How does it sound?
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.
@tomsonpl Thanks for the explanation. I recommend adding to the PR description before merging it.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
cc @tomsonpl |
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.
Detection Engine changes look good to me! No real concerns or objects, and I tested what I could with the feature flag.
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.
DW changes LGTM!
isQueryRule(ruleType) || | ||
isEsqlRule(ruleType) || | ||
isEqlRule(ruleType) || | ||
isNewTermsRule(ruleType) || | ||
(automatedResponseActionsForAllRulesEnabled && | ||
(isThresholdRule(ruleType) || isThreatMatchRule(ruleType) || isMlRule(ruleType))) |
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.
Consider increasing readability by going, for example, this route:
export const shouldShowResponseActions = (
ruleType: Type | undefined,
automatedResponseActionsForAllRulesEnabled: boolean
): boolean => {
const isEligibleRuleType =
isQueryRule(ruleType) ||
isEsqlRule(ruleType) ||
isEqlRule(ruleType) ||
isNewTermsRule(ruleType);
const isAutomatedResponseApplicable =
automatedResponseActionsForAllRulesEnabled &&
(isThresholdRule(ruleType) || isThreatMatchRule(ruleType) || isMlRule(ruleType));
return isEligibleRuleType || isAutomatedResponseApplicable;
};
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.
Yes, I agree 👍 this would increase readability. However, this check will be removed next week altogether with the feature flag, so I would prefer not to change it now, and just merge the thing :P
Are you ok with that? :)
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.
Sure :)
Starting backport for target branches: 8.x |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…94796) (#195611) # Backport This will backport the following commits from `main` to `8.x`: - [[EDR Workflows] Enable response actions in base rule params (#194796)](#194796) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Tomasz Ciecierski","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-09T14:06:02Z","message":"[EDR Workflows] Enable response actions in base rule params (#194796)","sha":"c103d2d21452f6c73b79036c5d10a24c018e1831","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Defend Workflows","v8.16.0","backport:version"],"title":"[EDR Workflows] Enable response actions in base rule params","number":194796,"url":"https://github.com/elastic/kibana/pull/194796","mergeCommit":{"message":"[EDR Workflows] Enable response actions in base rule params (#194796)","sha":"c103d2d21452f6c73b79036c5d10a24c018e1831"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194796","number":194796,"mergeCommit":{"message":"[EDR Workflows] Enable response actions in base rule params (#194796)","sha":"c103d2d21452f6c73b79036c5d10a24c018e1831"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Tomasz Ciecierski <[email protected]>
Another iteration (and the last one) of enabling automated response actions in rules - ML, Threshold, Indicator Match.
Initially we thought we couldn't support automated response_actions in all rule types. We were proven to be wrong, and happily are exposing response_actions to all rules now 🥳
This PR removes the optional response_actions parameter from type specific rules, and enable it as common in all rules.
The UI of the functionality is hidden behind the feature flag for purpose of intermediate release.
The feature flag and checks for ruleTypes will be removed in a following PR - which will result in all rule types being supported.