-
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] Add bulk action outcome to the Bulk API response #125551
Conversation
c70dbef
to
44a53c2
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
44a53c2
to
0e20d19
Compare
rules: Array<{ id: string; name?: string }>; | ||
} | ||
|
||
export interface BulkActionResponse { |
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.
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, Vitalii👍 I was trying to find where the error response was used but didn't find anything.
I removed bulkActionPartialErrorResponseSchema
as it is not used for validation, only to derive types. And io-ts
is a pretty heavy library to use for that purpose only. Also, I co-located all Bulk API response types in detections/containers/detection_engine/rules/types.ts
for better discoverability.
@@ -269,9 +291,9 @@ describe.each([ | |||
], | |||
}, | |||
], | |||
results: getResultsStructure(), |
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.
would be nice to actually check results content.
As with getResultsStructure
looks like that:
return {
created: expect.any(Array),
deleted: expect.any(Array),
updated: expect.any(Array),
};
we can't be fully confident in a test
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 don't think we should check the results content here as it will make these unit tests unnecessarily bloated. Instead, unit tests should be focused on testing specific behavior. For example, this test case verifies that an error is returned if deleted all index patterns. And it should verify only this logic in isolation. We shouldn't try to cover all possible cases in one test suite.
And as for checking the bulk API result content, we have integration tests that cover this functionality (security_and_spaces/tests/perform_bulk_action.ts
).
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 don't think it would make it bloated.
What would mock for results
in this particular test?
results: {
created:[],
deleted: [],
updated: [],
}
is that correct? If it is, it doesn't look too much.
And it should verify only this logic in isolation. We shouldn't try to cover all possible cases in one test suite.
Yes, we shouldn't, but the purpose of these tests(perform_bulk_action_route.test.ts
) to test API's response as well and we omitting a chunk from it.
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 think a test that checks whether an error is returned when all index patterns are deleted should check only this logic. Checking something else in this test case doesn't add any value. We have a separate set of tests that check the rules update logic, including the results
field.
We've already discussed tests a couple of times during our tech meetings. Everyone seemed to agree that tests should be as isolated as possible. But we can always reconsider our approach. So feel free to raise this topic during our next tech time.
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 don't think your example applies to this particular case.
Tests in this file tests body responses for particular error. Don't see how ensuring that results are correct, harm it.
We have a separate set of tests that check the rules update logic, including the results field.
Are there any tests that test results
in case of request failure? I think these ones are perfect for this. How else would you effectively do this? Functional tests? Aren't they too heavy for this?
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.
Ok, I'll try to clarify my position:
- I'm not against increasing test coverage.
- I'm firmly against mixing test cases and checking many irrelevant things in a single test case. When you check the entire response object, you're essentially creating a snapshot test with all the drawbacks snapshot tests have. They are brittle and produce a lot of false-positive failures on code changes (as you can already see in this PR, all test failures were false positive).
- If the
results
field needs additional coverage, it should be done in a separate test case (no matter unit or integrational). So when this functionality changes, we have a single test failure instead of tens of randomly failing tests.
If you disagree with this approach and think we should leverage "snapshot"-style testing instead of focused tests, please raise this topic during our next tech time.
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.
As I mentioned before, now some chunk of response logic is missing from tests(results object)
I've suggested to test it within other tests as I don't think it's irrelevant. If request fails partially, we still need to return successfully updated items. And in case of failure of all items, all arrays should be empty.
We also test whole responses in these tests, so it would make a perfect sense to add check of results there.
But if you prefer having a separate tests to cover results object, let's add them as separate test cases 👍
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.
If request fails partially, we still need to return successfully updated items
And in case of failure of all items, all arrays should be empty.
You just described two isolated test cases. They have nothing to do with an error is returned if deleted all index patterns
. So let's test all of them separately.
Let's also address all outstanding test coverage issues in separate PRs.
}, | ||
message: 'Bulk edit partially failed', | ||
status_code: 500, |
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.
Replaced the usage of response.custom for error response with response.customError to be consistent with other Kibana plugins. This introduces a slight breaking change as response.customError doesn't pass the status code to the response body.
can we actually introduce a breaking change in minor release?
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.
We don't document error responses, so I think we should be fine here.
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 remember, including status_code: 500
to final error response was also done to keep it in line with SecuritySolution error responses, that uses this property as part of error response.
@banderror, @spong , what are your thoughts? Is it safe to introduce such change in minor release?
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.
If you're talking about SiemResponseFactory
, it's pretty limited and looks like a relic of the past to me. For example, it accepts a body
param, which is actually a message
in disguise; it doesn't allow for additional error attributes to be passed, and its schema doesn't comply with the core framework's error responses.
So, in my opinion, we should gradually migrate away from the SiemResponseFactory
to the standard core functionality.
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 agree on that, but not sure this is something we have to do before BC, introducing breaking change to error response schema as well
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 think I'm in agreement with @xcrzx -- SiemResponseFactory is almost a couple years old now (so would be good to review its uses/abstraction), and it has been copied elsewhere like lists
as well (source), so we may want to check with the @elastic/security-solution-platform folks and see if they have any plans for it.
As for if this is an API breaking change, I think that line is blurred. Sure we don't document the error structure , but it is a documented API, and I think it's fair to say consumers of the API are going to end up relying on the response error structure (and perhaps that's just a release notes blurb that it's been updated?).
That said, if we're okay with this change, it shouldn't be an issue merging post-FF as we're still in the early BC's and this is a follow-up fix PR finalizing the updated bulk actions routes.
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 we don't document the error structure , but it is a documented API, and I think it's fair to say consumers of the API are going to end up relying on the response error structure (
Yeah, that's my concern too.
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 don't think we should introduce any breaking changes in this PR. Thoughts here: #125551 (comment)
0e20d19
to
c91f0af
Compare
abortSignal, | ||
}); | ||
if (errors.length > 0) { | ||
return response.customError({ |
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.
Not sure where it's coming from, but there is another property error
appeared to be added in error response:
attributes: {errors: [{,…}],…}
error: "Internal Server Error"
message: "Bulk edit partially failed"
statusCode: 500
error
property is not needed here, I think we have to remove it, so it won't confuse API's users
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; I don't think we need to duplicate statusCode
in the response body. We can find all the places the status code is coming from and clean them up. I think it would be OK to do that in a follow-up PR.
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 meant property error
:
error: "Internal Server Error"
and then we have property message:
message: "Bulk edit partially failed"
Not sure we need error here as message already describes reason of failure
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 error
field is added by the Kibana core server and is not configurable.
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.
Is this coming from response.customError
method? Do we need to use it then?
As I can see we still use for general error handler siemResponse
return siemResponse.error({
body: error.message,
statusCode: error.statusCode,
});
Looks like we can end up with different error response structure depends on when error was thrown
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.
Reading the description
Replaced the usage of response.custom for error response with response.customError to be consistent with other Kibana plugins. This introduces a slight breaking change as response.customError doesn't pass the status code to the response body.
So this is not entirely true, because as far as I can see from the response:
- the body contains
statusCode
- also, the body now contains the new
error
property
I'd suggest not to introduce this error
property yet, and so not use the response.customError
. Let's figure out the problem with siemResponse
etc 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.
Also please use the underscore case: it should be status_code
and not statusCode
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 body contains statusCode
also, the body now contains the new error property
It turns out these properties are added automatically by the Kibana server when response.customError
is returned from a route handler. It's interesting because our test didn't show these properties in the error response body. Probably, they are added somewhere deep inside the server before returning a response. So unit test doesn't capture them.
But okay, let's revert the error response related changes for not and figure out how to migrate away from SiemResponseFactory
later.
x-pack/plugins/security_solution/server/lib/machine_learning/validation.ts
Show resolved
Hide resolved
c91f0af
to
3d0c578
Compare
.map(({ result }) => result && internalRuleToAPIResponse(result)), | ||
deleted: bulkActionOutcome.results | ||
.filter(({ result }) => result == null) | ||
.map(({ item }) => internalRuleToAPIResponse(item)), | ||
}; |
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.
this logic looks a bit unobvious to me, plus, can be there situations, when rule gets in any both properties? For example in deleted & updated?
I would rather have some excluding condition:
if (isUpdated) {
// add to updated
} else if (ifCreated) {
// add to created
} else if(isDeleted) {
// add toDeleted
}
or even more robust approach(I would prefer this one personally):
- take deleted array only from BulkAction.delete action result,
- created only from BulkAction.duplicate
- updated from the rest
this way, we won't need any additional logic for parsing results + it will ensure rule will be added to only one list
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.
this logic looks a bit unobvious to me, plus, can be there situations, when rule gets in any both properties? For example in deleted & updated?
Not sure I understand you, how a rule could be updated and deleted at the same time?
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.
Not sure I understand you, how a rule could be updated and deleted at the same time?
Rule can't be, indeed. But because, we multiple times map through bulkActionOutcome
could be there situations when not only single, but few properties get populated out of three(deleted, created, updated).
In my opinion, it would be more readable and robust per traverse of a single time add it on of the arrays.
Or populate arrays from particular actions only:
take deleted array only from BulkAction.delete action result,
created only from BulkAction.duplicate
updated from the rest
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 current logic is pretty straightforward:
- We had a rule, now it's null - the rule was deleted
- We had a rule, now there's a rule with a different id - a new rule was created
- We had a rule, now there's a rule with the same id - the existing rule was modified
It doesn't depend on our business logic, so there's no need to tightly couple this with bulk actions whatsoever. Instead, I prefer these pieces to be separated.
const { message, statusCode } = | ||
error instanceof Error ? transformError(error) : { message: String(error), statusCode: 500 }; | ||
// Promise pool error is either a rule ID string or a rule object itself | ||
const rule = typeof item === 'string' ? { id: item } : { id: item.id, name: item.name }; |
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.
in which case pool error can be rule ID? Can you please give me an example?
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.
Yea, there's a typo; should be item
instead of error
.
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.
can you please fix it? It's a bit misleading.
Also, in which case item can be a string, but not rule? Can you please add additional details that explains that to 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.
Updated the comment
thanks for the PR @xcrzx, it's very important step towards proper caching on FE 👍 . I have left a couple comments on it, mainly regarding response error schema and breaking changes in it |
export interface BulkActionSummary { | ||
failed: number; | ||
succeeded: number; | ||
total: number; | ||
} | ||
|
||
export interface BulkActionResult { | ||
success: boolean; | ||
rules_count: number; | ||
updated: Rule[]; | ||
created: Rule[]; | ||
deleted: Rule[]; | ||
} | ||
|
||
export interface BulkActionAggregatedError { | ||
message: string; | ||
status_code: number; | ||
rules: Array<{ id: string; name?: string }>; | ||
} | ||
|
||
export interface BulkActionResponse { |
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 think we need to move all these types to x-pack/plugins/security_solution/common/detection_engine/schemas/response
and x-pack/plugins/security_solution/common/detection_engine/schemas/request
so that we could use them both in the route handlers and client-side code. Even if they are not io-ts schemas but simple TS interfaces.
I'd suggest to address it in a follow-up PR targeting 8.2.
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.
Separate FE and BE schemas are one of my biggest pain points. But this is not easy to address; not even sure we'll have the capacity to fix that in 8.2.
@@ -79,7 +79,7 @@ export const createRulesRoute = ( | |||
request, | |||
savedObjectsClient, | |||
}); | |||
throwHttpError(await mlAuthz.validateRuleType(internalRule.params.type)); | |||
throwAuthzError(await mlAuthz.validateRuleType(internalRule.params.type)); |
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.
Out of the scope of this PR, but still: why mlAuthz.validateRuleType
doesn't throw itself? I see this pattern of throwAuthzError(await mlAuthz.validateRuleType(...))
everywhere, but I haven't seen any code that would process the validation result differently. I'd make it throw which would eliminate the need for importing throwAuthzError
.
Could be addressed in a follow-up.
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.
Hmm, a quick check shows that mlAuthz.validateRuleType
is used exclusively with throwAuthzError
. So I agree, there's no need for these two methods to be separated.
}); | ||
}); | ||
|
||
function getResultsStructure() { |
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: I'd rename it to someBulkActionResults
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.
Yup, makes sense
attributes: { | ||
errors: [ | ||
{ | ||
message: 'Elastic rule can`t be edited', | ||
status_code: 403, | ||
status_code: 400, |
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.
👍
// Transforms the data but will remove any null or undefined it encounters and not include | ||
// those on the export | ||
export const transformAlertToRule = ( | ||
rule: SanitizedAlert<RuleParams>, | ||
ruleExecutionSummary?: RuleExecutionSummary | null, | ||
legacyRuleActions?: LegacyRulesActionsSavedObject | null | ||
): Partial<RulesSchema> => { | ||
return internalRuleToAPIResponse(rule, ruleExecutionSummary, legacyRuleActions); | ||
}; |
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.
Less legacy 🎉
expect(body.attributes.summary).to.eql({ failed: 0, succeeded: 1, total: 1 }); | ||
|
||
// Check that the deleted rule is returned with the response | ||
expect(body.attributes.results.deleted[0].name).to.eql(testRule.name); | ||
|
||
await await fetchRule(ruleId).expect(404); | ||
// Check that the updates have been persisted | ||
await fetchRule(ruleId).expect(404); |
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 a lot for adding the comments, very readable and helpful!
errorsMap.set(message, { | ||
message: error.message, | ||
status_code: error.statusCode, | ||
message: truncate(message, { length: MAX_ERROR_MESSAGE_LENGTH }), |
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.
👍
const results = { | ||
updated: bulkActionOutcome.results | ||
.filter(({ item, result }) => item.id === result?.id) | ||
.map(({ result }) => result && internalRuleToAPIResponse(result)), | ||
created: bulkActionOutcome.results | ||
.filter(({ item, result }) => result != null && result.id !== item.id) | ||
.map(({ result }) => result && internalRuleToAPIResponse(result)), | ||
deleted: bulkActionOutcome.results | ||
.filter(({ result }) => result == null) | ||
.map(({ item }) => internalRuleToAPIResponse(item)), |
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.
Shouldn't this logic be based on the bulk action type? Currently, the endpoint can be called only with 1 bulk action type (duplicate, enable, edit, etc); every action can end up with rules either being created or updated or deleted. Am I missing something?
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.
we already started to discuss this part of logic here: #125551 (comment)
I leaning towards having these properties populated depends on action, rather than map multiple times through results and trying to figure out which result should go where
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.
Already explained here: #125551 (comment):
The current logic is pretty straightforward:
- We had a rule, now it's null - the rule was deleted
- We had a rule, now there's a rule with a different id - a new rule was created
- We had a rule, now there's a rule with the same id - the existing rule was modified
It doesn't depend on our business logic, so there's no need to tightly couple this with bulk actions whatsoever. Instead, I prefer these pieces to be separated.
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.
Ok, makes sense, although for someone like me jumping into this code it wasn't too straightforward and obvious. :)
Maybe adding comments to the code could help:
// Whether rules will be updated, created or deleted depends on the bulk action type being processed.
// However, in order to avoid doing a switch-case by the action type, we can figure it out indirectly.
const results = {
// We had a rule, now there's a rule with the same id - the existing rule was modified
updated: bulkActionOutcome.results
.filter(({ item, result }) => item.id === result?.id)
.map(({ result }) => result && internalRuleToAPIResponse(result)),
// We had a rule, now there's a rule with a different id - a new rule was created
created: bulkActionOutcome.results
.filter(({ item, result }) => result != null && result.id !== item.id)
.map(({ result }) => result && internalRuleToAPIResponse(result)),
// We had a rule, now it's null - the rule was deleted
deleted: bulkActionOutcome.results
.filter(({ result }) => result == null)
.map(({ item }) => internalRuleToAPIResponse(item)),
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.
Sounds good, I'll add the comments with my next PR 👍
abortSignal, | ||
}); | ||
if (errors.length > 0) { | ||
return response.customError({ |
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.
Reading the description
Replaced the usage of response.custom for error response with response.customError to be consistent with other Kibana plugins. This introduces a slight breaking change as response.customError doesn't pass the status code to the response body.
So this is not entirely true, because as far as I can see from the response:
- the body contains
statusCode
- also, the body now contains the new
error
property
I'd suggest not to introduce this error
property yet, and so not use the response.customError
. Let's figure out the problem with siemResponse
etc later.
Tested locally, happy paths work as expected! 🎉 I think the only thing that needs to be reverted in this PR is the introduction of the new |
Noticed a bug with rules selection after applying a bulk action, but that seems like something we had before this PR: Screen.Recording.2022-02-16.at.12.23.40.movUPD: opened an issue for it #125775 |
3d0c578
to
cf7b28f
Compare
Yea, it's not related to the changes in this PR. Let's fix it separately |
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, thank you for the fix! 🚀 🚢
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @xcrzx |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit e1b9f93)
(cherry picked from commit e1b9f93) Co-authored-by: Dmitrii Shevchenko <[email protected]>
Addresses: #125500
Summary
Support returning updated/created/deleted rules with the Bulk API response. These schema changes will allow us to synchronize the server-side state with the front-end state without sending additional HTTP requests.
Bulk API response schema now looks like this:
We return the
attributes
property for both OK and error responses.Other notable changes
response.custom
for error response withresponse.customError
to be consistent with other Kibana plugins. This introduces a slight breaking change asresponse.customError
doesn't pass the status code to the response body.throwHttpError
tothrowAuthzError
to better reflect what the method does. Replace incorrect usages of thethrowHttpError
withthrow new BadRequestError
.initPromisePool
interface. Now along with execution results or errors, it returns original items. That helped eliminate extra wrappers inperform_bulk_action_route
and make bulk operations more straightforward.