-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[RAM] Bulk update on rules #124715
Comments
Pinging @elastic/response-ops (Team:ResponseOps) |
I created #124850. Please let us know when you would like this completed and we'll plan accordingly 🙏 |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
@XavierM
|
for question in item 2 (ids and KQL query by ids) curl --location --request GET 'http://localhost:5601/kbn/api/detection_engine/rules/_find?page=1&sort_field=enabled&sort_order=desc&filter=alert._id: "alert:8e5a93a0-9320-11ec-9265-8b772383a08d"&per_page=100' \
--header 'kbn-xsrf: reporting' \
--header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \
--data-raw '' response {
"message": "This key 'alert._id' does NOT exist in alert saved object index patterns: Bad Request",
"status_code": 400
} where search query is So it looks like, search by id is not supported. It located outside of Should we consider introducing ids parameter? Or play with underlying savedObject client |
@vitaliidm thank you for proving me wrong, I created this PR to fix it https://github.com/elastic/kibana/pull/126933/files |
@vitaliidm and I met yesterday and talked about his observations. I do agree with observation 1#, I missed it and we should do something different. The data attributes will be just a rewrite for now until we get more requirements for the other attributes. export interface BulkUpdateOptions<Params extends AlertTypeParams> {
filter: KueryNode;
data: {
name?: string;
schedule?: IntervalSchedule;
actions?: NormalizedAlertAction[];
params?: Params;
throttle?: string | null;
notifyWhen?: AlertNotifyWhenType | null;
},
actions: Array<{ type: 'add' | 'delete' | 'set', field: 'params.index' | 'tags', value: string[]}>[]
} I talked to my team about the validation and we do think that we can use the validation around rule's params and if we get an error here we do not add this rule in our
|
Thanks @XavierM for coming up with the solution so quick regarding search by ids. I have prepared the first working POC: https://github.com/elastic/kibana/pull/126904/files#diff-6736e143ede2dc06e825bddcdc23b4d088a6620805751db4eddc5900d586c4df And while working on it, have a couple questions
Instead of 'game over', why can't we move this rule into errors array and proceed with updating the rest of rules?
export type BulkUpdateAction =
| {
action: 'add' | 'delete' | 'set';
field: 'tags';
value: string[];
}
| {
action: 'add' | 'delete' | 'set';
field: 'params.index';
value: string[];
}
| {
action: 'add' | 'set';
field: 'actions';
value: NormalizedAlertAction[];
}; as one of the requirements to add action to existed list of actions in rules or rewrite existing actions |
@XavierM Ok, sorry for being late here, but I agree with all the concerns shared by @vitaliidm in #124715 (comment) Let me reiterate over the main points.
If we decide to combine the bulk get-modify-update functionality in a single method of RulesClient (instead of separate bulk get and bulk update methods), I'd suggest to expose it as 2 separate type Rule<Params extends AlertTypeParams> = SanitizedAlert<Params>;
interface RulePropsModified<Params extends AlertTypeParams> {
name?: string;
tags?: string[];
schedule?: IntervalSchedule;
actions?: NormalizedAlertAction[];
params?: Params;
throttle?: string | null;
notifyWhen?: RuleNotifyWhenType | null;
}
type RuleModifier<Params extends AlertTypeParams> = (rule: Rule<Params>) => RulePropsModified<Params>;
interface BulkEditByFilterArgs<Params extends AlertTypeParams> {
filter: string; // KQL string
modifier: RuleModifier<Params>;
}
interface BulkEditByIdsArgs<Params extends AlertTypeParams> {
ids: string[]; // rule SO ids
modifier: RuleModifier<Params>;
} I don't think it'd be a good idea to expose something like this:
or this:
Bulk editing logic can contain something specific to our solution, so I'd keep it flexible by using a callback-based approach (like |
@vitaliidm and @banderror and I had a good conversation today about validation and mutation of our attributes in the rule's saved object. I was able to bring back our idea to my team too. And it is pretty close to what we decided with each other, so you should not be surprise about our decision. First of all, we think it will be good idea to add a new property in export interface AlertTypeParamsValidator<Params extends AlertTypeParams> {
validate: (object: unknown) => Params;
validateMutatedParams?: (mutatedOject: unknown, origObject?: unknown) => Params;
} I think we all agree that it is better to combine the bulk get-modify-update functionality in a single method of RulesClient, I think the main reason is to avoid multiple queries on the rules and also to validate the authorization of user on the rules. So We think that @banderror is right to call it type RuleParamsModifier<Params extends AlertTypeParams> = (params: Params) => Params;
type BulkEditActionRuleFields = keyof Pick<Alert, 'name' | 'actions' | 'schedule' | 'tags' | 'throttle' | 'notifyWhen' ;
interface BulkEditAction {
type: 'add' | 'delete' | 'set';
field: BulkEditActionRuleFields;
value: Rule[BulkEditActionRuleFields];
}
interface BulkEditByFilterArgs<Params extends AlertTypeParams> {
actions?: BulkEditAction[];
filter: KueryNode;
paramsModifier?: RuleParamsModifier<Params>;
} |
I was checking our bulk actions endpoint implementation and noticed a few things that I think need to be discussed. Please refer to these files:
You can notice that there are some actions we do before and after calling the
How are we going to support all of that using the new
@vitaliidm do you have an idea how we're going to handle each and would we need any specific API like async hooks in the bulkEdit method? |
@vitaliidm @XavierM Do we have a proposal or a draft of the bulk edit method result? Since it's gonna be used in the existing bulk actions endpoint with a specific existing response structure, it will need to return all the data required to build the response. Here's where the response is being built: Lines 86 to 135 in 3713296
|
@XavierM so regarding the filter (
|
this is a draft POC PR. After rulesClient.bulkEdit method is implemented - it will be placed instead of current implementation in a separate PR
Would make sense to do this check in async
agree
Looks like, it mostly patch specific logic (calculating params using
I can see, it validates not only params(which can be validated in
As update of these properties will be done inside bulkEdit method, looks like we don't need to do additional validation, as it should be done within bulkEdit method. Unless, these properties can be deemed as valid for rulesClient, but invalid for us(securitySolution/detections). Can this happen? |
Had a chat with @vitaliidm on the above points, here's the summary. Aligned on
We agreed that doing this ML auth would be best in the type RuleParamsModifier<Params extends AlertTypeParams> = async (params: Params) => Params; So it would be expected that this function could do pre-validation and throw exceptions with validation messages.
We will just increment it in the
The bulk edit method will validate the rule name, tags, etc itself. If we need any additional validation for our rule params object, we will be able to move it to Some other outstanding questions@XavierM we have a few new questions to clarify and would appreciate any feedback from your side, if you have some time to review them: export interface AlertTypeParamsValidator<Params extends AlertTypeParams> {
validate: (object: unknown) => Params;
validateMutatedParams?: (mutatedOject: unknown, origObject?: unknown) => Params;
}
|
We agreed on introducing
We can extend it to passing the whole rule object, rather than only params. But in this case
Yes, params can be changed, similarly to |
@vitaliidm thanks for the update. Sounds good to me!
I'm not aware of any immediate use cases for this right now. |
@XavierM Currently, each rule on our side has so called internals tags GET /.kibana/_doc/alert:9c4bbac0-b5cb-11ec-8f1e-adaa7d7d57e5 {
"_index" : ".kibana_8.3.0_001",
"_id" : "alert:9c4bbac0-b5cb-11ec-8f1e-adaa7d7d57e5",
"_version" : 1,
"_seq_no" : 2116,
"_primary_term" : 1,
"found" : true,
"_source" : {
"alert" : {
"name" : "Web Application Suspicious Activity: sqlmap User Agent [Duplicate]",
"tags" : [
"Elastic",
"APM",
"__internal_rule_id:2e41245d-4dee-42f7-92da-0701e9567463",
"__internal_immutable:false"
],
"alertTypeId" : "siem.queryRule",
"consumer" : "siem",
"params" : {
"author" : [
"Elastic"
],
"description" : "This is an example of how to detect an unwanted web client user agent. This search matches the user agent for sqlmap 1.3.11, which is a popular FOSS tool for testing web applications for SQL injection vulnerabilities.",
"ruleId" : "2e41245d-4dee-42f7-92da-0701e9567463",
"falsePositives" : [
"This rule does not indicate that a SQL injection attack occurred, only that the `sqlmap` tool was used. Security scans and tests may result in these errors. If the source is not an authorized security tester, this is generally suspicious or malicious activity."
],
"from" : "now-6m",
"immutable" : false,
"license" : "Elastic License v2",
"outputIndex" : ".siem-signals-default",
"maxSignals" : 100,
"riskScore" : 47,
"riskScoreMapping" : [ ],
"severity" : "medium",
"severityMapping" : [ ],
"threat" : [ ],
"timestampOverride" : "event.ingested",
"to" : "now",
"references" : [
"http://sqlmap.org/"
],
"version" : 7,
"exceptionsList" : [ ],
"index" : [
"apm-*-transaction*",
"traces-apm*"
],
"query" : """user_agent.original:"sqlmap/1.3.11#stable (http://sqlmap.org)"
""",
"language" : "kuery",
"type" : "query"
},
"schedule" : {
"interval" : "5m"
},
"enabled" : false,
"actions" : [ ],
"throttle" : null,
"notifyWhen" : "onActiveAlert",
"apiKeyOwner" : null,
"apiKey" : null,
"legacyId" : null,
"createdBy" : "elastic",
"updatedBy" : "elastic",
"createdAt" : "2022-04-06T17:04:24.687Z",
"updatedAt" : "2022-04-06T17:04:24.687Z",
"snoozeEndTime" : null,
"muteAll" : false,
"mutedInstanceIds" : [ ],
"executionStatus" : {
"status" : "pending",
"lastExecutionDate" : "2022-04-06T17:04:24.687Z",
"error" : null,
"warning" : null
},
"monitoring" : {
"execution" : {
"history" : [ ],
"calculated_metrics" : {
"success_ratio" : 0
}
}
},
"mapped_params" : {
"risk_score" : 47,
"severity" : "40-medium"
},
"meta" : {
"versionApiKeyLastmodified" : "8.3.0"
}
},
"type" : "alert",
"references" : [ ],
"namespaces" : [
"default"
],
"migrationVersion" : {
"alert" : "8.2.0"
},
"coreMigrationVersion" : "8.3.0",
"updated_at" : "2022-04-06T17:04:24.687Z"
}
}
Here is tags: "tags" : [
"Elastic",
"APM",
"__internal_rule_id:2e41245d-4dee-42f7-92da-0701e9567463",
"__internal_immutable:false"
],
These tags cannot be rewritten by user and set internally by calling this method With current implementation, such payload of bulkEdit would wipe out internal tags {
"ids": ["9c4bbac0-b5cb-11ec-8f1e-adaa7d7d57e5"],
"operations": [{
"operation": "set",
"field": "tags",
"value": ["alerting-in-default"]
}]
} and field tags in rule would become But expectation on our side would be, it to become: "tags" : [
"alerting-in-defaul",
"__internal_rule_id:2e41245d-4dee-42f7-92da-0701e9567463",
"__internal_immutable:false"
], How we can resolve this?
|
@vitaliidm Perhaps the best place for ensuring these internal tags are always there would be the The presence of these internal tags is an invariant that must be always respected. Actually, there are two invariants:
Since these are invariants and they do not depend on any external variables, only on the rule object itself, I think we could keep this normalization logic in |
@banderror, @vitaliidm, I am thinking about it a little bit differently here, why we are not moving these internal tags in a different attributes like |
@XavierM , that would require to change our logic that uses internal tags across the app. For example filtering Elastic/custom rules and so on. I had a quick look, there are not that many places that uses it(few places for immutable tag, few for internal rule) @banderror , any additional concerns you can think of, that might rise if try to move internal tags to a separate field within params? |
@XavierM @vitaliidm I like the idea 👍 But I suspect we could even completely get rid of all those internal tags and wouldn't need to migrate them to I don't have any historical context on why we have internal tags, but my guess is because we were not able to filter by I'll try to figure out if that makes sense or there's another use case for internal tags that I'm missing. |
Yup! That's correct -- I created this issue (#124899) a little while back to remove our |
Awesome-awesome! So my vote would go for addressing #124899 first and then finalizing the |
Fantastic, thanks @XavierM , @spong and @banderror for clearing out situation with internal tags |
Addresses - #124715 ## Summary - adds bulkEdit method to rulesClient - adds multi_terms bucket aggregations to savedObjectClient - adds createPointInTimeFinderAsInternalUser to encryptedSavedObjectClient - adds alerting API for bulkEdit ```bash curl --location --request POST 'http://localhost:5601/kbn/internal/alerting/rules/_bulk_edit' \ --header 'kbn-xsrf: reporting' \ --header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \ --header 'Content-Type: application/json' \ --data-raw '{ "ids": ["4cb80374-b5c7-11ec-8f1e-adaa7d7d57e5"], "operations": [{ "operation": "add", "field": "tags", "value": ["foo"] }] }' ``` ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ## Release note Adds new `bulkEdit` method to alerting rulesClient and internal _bulk_edit API, that allow bulk editing of rules.
Addresses - elastic#124715 ## Summary - adds bulkEdit method to rulesClient - adds multi_terms bucket aggregations to savedObjectClient - adds createPointInTimeFinderAsInternalUser to encryptedSavedObjectClient - adds alerting API for bulkEdit ```bash curl --location --request POST 'http://localhost:5601/kbn/internal/alerting/rules/_bulk_edit' \ --header 'kbn-xsrf: reporting' \ --header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \ --header 'Content-Type: application/json' \ --data-raw '{ "ids": ["4cb80374-b5c7-11ec-8f1e-adaa7d7d57e5"], "operations": [{ "operation": "add", "field": "tags", "value": ["foo"] }] }' ``` ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ## Release note Adds new `bulkEdit` method to alerting rulesClient and internal _bulk_edit API, that allow bulk editing of rules.
Implemented in #126904 |
This work will be able to allow our solution to create bulk update on Rule with the bulk api from our saved object. Below, I am going to describe step by step how we want this bulk update to work but before that we want to make sure that will have our new bulk_update using our
retryIfConflicts
like we do for all our mutations.Our bulk_update interface should looks like that:
Step by Step:
terms
aggs with the filter (caveat if we think that we are going to have more than 1000 unique terms, we will have to use an aggs but I do think that we should be ok withterms
for now)createPointInTimeFinder
to build our bulk update objects SavedObjectsBulkUpdateObjectGame over
Game over
The text was updated successfully, but these errors were encountered: