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 skipped rules to Bulk Edit rules API response #147345
[Security Solution] Add skipped rules to Bulk Edit rules API response #147345
Conversation
8d326f6
to
dd7aeb1
Compare
Fix test Fix import Update tests Modified group 3 test
e388a82
to
e531558
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
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.
Once again - this is fantastic work @jpdjere and a super high-quality PR! No doubt this was a huge undertaking, and the end result looks very solid. Thank you for all the improvements, including further refactoring of the RulesClient.bulkEdit
method, adding an exhaustive set of (super clear and clean) tests at various levels, making the bulk actions response schema explicit, etc etc. Thank you very much for doing all that.
If you agree, I'd suggest to prioritize merging this PR compared to other PRs that are in progress or review, and actively seek for codeowners reviews from other teams.
Files by Code Ownerelastic/response-ops
elastic/security-detections-response-alerts
elastic/security-detections-response-rules
elastic/security-solution
|
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.
Alerts area changes LGTM
const { modifiedAttributes, isAttributeModified } = applyBulkEditOperation( | ||
{ | ||
field: 'schedule', | ||
value: { interval: '1d' }, | ||
operation: 'set', | ||
}, | ||
ruleMock | ||
); | ||
expect(modifiedAttributes).toHaveProperty('schedule', { interval: '1d' }); | ||
expect(isAttributeModified).toBe(true); |
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.
Quick question with regards to not seeing the skipped
behavior when updating schedules. E.g. if you update the schedule to the same values twice, it doesn't say any rules were skipped for the second action:
Is this because the set_schedule
BulkActionEditType is converted to a set
operation, and so it always overrides (as also mentioned in the UI warning callout)?
Debugging a bit shows isAttributesUpdateSkipped
is true
as the interval value hadn't changed ("interval": "5m"
), however it looks like isParamsUpdateSkipped
is false
, presumably because this operation needed to be broken out to separate param updates (to
, from
and interval
)?
Just curious here as I was hoping to piggy-back off your skipped logic here for deciding when revision
should be updated over in #147398. Would appreciate any additional details here -- thanks! 🙂
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.
Seeing similar behavior for Rule Actions
and Timeline Templates
. This makes sense for Rule Actions
since it's either an append or overwrite (so always updates), but why for timeline templates? Is this because it results in similar param updates as well? i.e.
Lines 142 to 151 in 3287afd
// timeline actions | |
case BulkActionEditType.set_timeline: { | |
ruleParams = { | |
...ruleParams, | |
timelineId: action.value.timeline_id || undefined, | |
timelineTitle: action.value.timeline_title || undefined, | |
}; | |
break; | |
} |
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.
Maybe this is caused by the fact that currently we always increment the params.version
?..
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.
So I think you have a point here @spong. The issue you are seeing is that we are not currently checking in the case of set_timeline
or set_schedule
if the update should be skipped (like we do for index patterns, for e.g.):
Lines 153 to 170 in f9fb144
case BulkActionEditType.set_schedule: { | |
const interval = parseInterval(action.value.interval) ?? moment.duration(0); | |
const parsedFrom = parseInterval(action.value.lookback) ?? moment.duration(0); | |
const from = parsedFrom.asSeconds() + interval.asSeconds(); | |
ruleParams = { | |
...ruleParams, | |
meta: { | |
...ruleParams.meta, | |
from: action.value.lookback, | |
}, | |
from: `now-${from}s`, | |
}; | |
break; | |
} | |
} |
I think we should add two checks shouldSkipTimelineBulkAction
and shouldSkipScheduleBulkAction
which should check if the new value is the same as the existing one saved in params. (For the case of Schedule, it is true that the update is split between params
and attributes
, but as you said the attributes checks already returns true
because it is a generic check for all updates; so adding the check in the params
will make the whole update to be marked as skipped
).
The case of actions
I still think needs a little more discussion. My initial thought was that if an action is:
add
ed: the result will always be different to the existing one, so the rule is modified.set
: even in the case of overwriting all actions with an action, that action is new and different to the existing one, so it also modifies the rule. My doubt now is: that is true for users using the UI, but we might have the case of a user using the API overwriting the actions to be exactly what they currently have? In that case, we'd need to mark it as skipped.
What do yo think?
cc @banderror
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 would preferably do it in a follow up PR, if that doesn't disturb your workflow @spong . So we can get this one merged.
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.
Thank you for the details @jpdjere! This all makes sense now 🙂
I'm +1 for adding two additional checks for skipping timeline
and schedule
updates based on params
(and thus aligning with the checks against attributes
).
As for actions
, I understand your doubt, and so it would probably make sense to do a skip check for the sake of API consistency. That said, this may get a little interesting with how the SO references array gets converted over to the rule actions object. The id
ends up being the connector id (the actual action SO id isn't leaked), so you can end up with what looks like two 'identical' actions on the rule itself:
The rule update API handles this based on the array order IIRC....looking further that all appears to be isolated to extractReferences() and denormalizeActions(), so I might be over complicating this, and we can just do the skip diff once the actions are denormalized.
As for timing, I say let's get this PR in and we can tackle the above in a follow-up PR. It's no issue for me either way (I'm rebasing off this branch for the time being), but appreciate the thoughtfulness!
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.
Checked out, tested locally, and LGTM! 👍 🎉 As with @banderror, appreciate the refactoring and cleanup + extensive tests here -- great work @jpdjere! 🙂
I mostly desk tested these changes, and did a cursory review of the code, so am approving from that perspective. @banderror and @maximpn look to have given quite a bit initial feedback over in #144461, so this all looks good!
I did have a couple questions around certain consecutive updates not being identified as skipped
, so if you could provide any details/commentary there that would be greatly appreciated (pretty sure this is as-designed with how param updates are managed, but want to make sure as I'm hoping to piggy-back off this work for the implementation of the revision
field over in #147398).
const errors: BulkOperationError[] = []; | ||
const apiKeysToInvalidate: string[] = []; | ||
const apiKeysMap = new Map<string, { oldApiKey?: string; newApiKey?: string }>(); | ||
const apiKeysMap: ApiKeysMap = new Map(); | ||
const username = await context.getUserName(); |
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 you can create this variable inside updateRuleAttributesAndParamsInMemory
. We do not need to pass it as a parameter. We already pass context
to updateRuleAttributesAndParamsInMemory
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 but I don't think I can: I need to acumualte the apiKeys of all the rules we loop through. apiKeysMap
then gets passed down to saveBulkUpdatedRules
to create apiKeysToInvalidate
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 am talking about username. We do not use it here, but use inside updateRuleAttributesAndParamsInMemory
. No need to pass it there. We can just call context.getUserName
inside updateRuleAttributesAndParamsInMemory
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, now I understood. The reason I'm placing it outside updateRuleAttributesAndParamsInMemory
is because the username will be the same for all rules being edited. If I call context.getUserName
inside updateRuleAttributesAndParamsInMemory
it is an additional async call that will be called for each rule. If we are editing hundreds of rules, it will sum up. I'd rather call it once and pass the calculated value to the function. (AFAIK the function is not memoized)
apiKeysToInvalidate, | ||
resultSavedObjects: result.saved_objects, | ||
errors, | ||
rules, |
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.
Do we need both rules
and resultSavedObjects
fields here?
It's hard to tell if we use it from our side. I did not find evidence still.
Maybe you use both?
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, we need both in the retryIfBulkEditConflicts
function in x-pack/plugins/alerting/server/rules_client/common/retry_if_bulk_edit_conflicts.ts
// Create array of unique skipped rules by id | ||
const skipped = [ | ||
...new Map([...accSkipped, ...localSkipped].map((item) => [item.id, item])).values(), | ||
]; |
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.
Why do you need to do this operation to guarantee that all skipped rules have unique id?
As I see, they will be already unique. Maybe I'm 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.
This is to make sure that we prevent skipped rules from potentially accumulate the same skip results multiple times when this function is be called recursively. That's the test case I added in x-pack/plugins/alerting/server/rules_client/common/retry_if_bulk_edit_conflicts.test.ts
which had the issue with the casting.
|
||
validateScheduleInterval( | ||
context, | ||
attributes.schedule.interval as string, |
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 do not need type casting 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.
Thanks, updated!
context, | ||
attributes.schedule.interval as string, | ||
ruleType.id, | ||
attributes.id as string |
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.
attributes
has a type RawRule. We do not have field id
there. But you can use rule.id
instead.
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, fixed!
skipped: [ | ||
{ id: 'skip-1', name: 'skip-1', skip_reason: 'RULE_NOT_MODIFIED' as BulkEditSkipReason }, | ||
{ id: 'skip-2', name: 'skip-2', skip_reason: 'RULE_NOT_MODIFIED' as BulkEditSkipReason }, | ||
{ id: 'skip-5', name: 'skip-5', skip_reason: 'RULE_NOT_MODIFIED' as BulkEditSkipReason }, |
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.
You do not need these castings 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.
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.
You are right. My linter behaved weird.
@@ -9,6 +9,7 @@ import { KueryNode } from '@kbn/es-query'; | |||
import { retryIfBulkEditConflicts } from './retry_if_bulk_edit_conflicts'; | |||
import { RETRY_IF_CONFLICTS_ATTEMPTS } from './wait_before_next_retry'; | |||
import { loggingSystemMock } from '@kbn/core/server/mocks'; | |||
import { BulkEditSkipReason } from '../../types'; |
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 path where you can find a type.
I found it here: import { BulkEditSkipReason } from '../../../common/bulk_edit';
Not sure why build did not fail.
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, this is weird.
Thanks for catching, fixed it.
context: RulesClientContext, | ||
operations: BulkEditOperation[], | ||
rule: SavedObjectsFindResult<RawRule> | ||
): Promise<void> { |
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 check if (rule.attributes.actions.length === 0) return
in first line here , so we skip entire function in that case
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.
True, thanks, corrected.
await context.actionsAuthorization.ensureAuthorized('execute'); | ||
} catch (error) { | ||
throw Error(`Rule not authorized for bulk ${field} update - ${error.message}`); | ||
} |
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 we meet certain condition first time we will check actions authorization. As I understood it's enough to do once. So we can break
the look in that case.
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.
Added break
@guskovaue I addressed your comments, thanks for the review! Let me know what you think and if you have any other feedback. |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @jpdjere |
@jpdjere LGTM! |
…elastic#147345) **Addresses:** elastic#145093 **Related to:** elastic#139802 ## Summary - Extends Bulk Edit API to return a new `skipped` property for rules whose updating was skipped. See [elastic#145093](elastic#145093) for details on when a rule is skipped. - In `x-pack/plugins/alerting/server/rules_client/methods/bulk_edit.ts`, refactored the methods `bulkEdit` and `bulkEditOcc` to smaller methods, following an immutable approach. - Updated all related tests and expanded coverage. (unit, integration and e2e) - Update success toast message so that the user is informed if rules were skipped. https://user-images.githubusercontent.com/5354282/199806913-eb70e7a6-0435-486a-96f1-dd0e8abaffe2.mp4 ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - elastic/security-docs#2684 - [ ] [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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) Co-authored-by: Xavier Mouligneau <xavier.mouligneau@elastic.co>
Addresses: #145093
Related to: #139802
Summary
skipped
property for rules whose updating was skipped. See #145093 for details on when a rule is skipped.x-pack/plugins/alerting/server/rules_client/methods/bulk_edit.ts
, refactored the methodsbulkEdit
andbulkEditOcc
to smaller methods, following an immutable approach.Skipped.bulk.edit.rules.mp4
Checklist
Delete any items that are not applicable to this PR.
skipped
response parameter of the _bulk_action endpoint security-docs#2684For maintainers