Skip to content
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][Detections]Adds dry_run mode description to _bulk_action API #2210

Merged
merged 28 commits into from
Aug 2, 2022

Conversation

vitaliidm
Copy link
Contributor

@vitaliidm vitaliidm commented Jul 20, 2022

@vitaliidm vitaliidm marked this pull request as draft July 20, 2022 11:20
@mergify
Copy link
Contributor

mergify bot commented Jul 20, 2022

This pull request does not have a backport label. Could you fix it @vitaliidm? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • v7.x is the label to automatically backport to the 7.x branch.
  • v7./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@vitaliidm vitaliidm changed the title Adds dry_run mode description to _bulk_action API [Security Solution][Detections]Adds dry_run mode description to _bulk_action API Jul 20, 2022
@vitaliidm vitaliidm marked this pull request as ready for review July 20, 2022 17:19
@vitaliidm vitaliidm requested review from a team and banderror and removed request for a team July 21, 2022 14:44
@vitaliidm vitaliidm self-assigned this Jul 21, 2022
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for documenting this mode @vitaliidm. I have a bunch of suggestions regarding the wording and would like a native English speaker to check them (@joepeeples could you please help with this or delegate it to someone?). Also, how can we build the docs for this PR to be able to preview the changes?

@jmikell821
Copy link
Contributor

Hey @banderror, we'll definitely review for language and consistency. I added the preview link to @vitaliidm's first comment - link is here.

Copy link
Contributor

@nastasha-solomon nastasha-solomon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up @vitaliidm and @banderror! I left some suggestions for your consideration and one question. Hopefully they're helpful!

Co-authored-by: nastasha-solomon <79124755+nastasha-solomon@users.noreply.github.com>
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you for your help @jmikell821 and @nastasha-solomon 🙏

Co-authored-by: Georgii Gorbachev <banderror@gmail.com>
Copy link
Contributor

@nastasha-solomon nastasha-solomon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@banderror @vitaliidm left a few edits and some more comments for your consideration. Feel free to ping me if you have any questions!

[[bulk-actions-rules-api-dry-run]]
[discrete]
==== Dry run mode
Enable dry run mode before you bulk update or delete rules to verify that the selected rules can be modified _before_ you permanently change them. When dry mode is enabled, the bulk action is temporarily applied to selected rules. These updates are not written to {es}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@banderror @vitaliidm should we tweak the first sentence a bit more? Looking at this again, I'm not sure that it fully encompasses the main use cases that this feature addresses. Here's my understanding:

  • The dry run allows users do a test run of the bulk action they select (enable, disable, delete, etc.) to help them verify that the rules they specified can actually be updated.
  • Users can complete a pelimiary and temporary bulk rule update with this feature. This would give them an extra buffer layer, or another layer of checks, before they permanently alter their rules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the above is correct, here's another revision that includes both. Let me know what you think:

Suggested change
Enable dry run mode before you bulk update or delete rules to verify that the selected rules can be modified _before_ you permanently change them. When dry mode is enabled, the bulk action is temporarily applied to selected rules. These updates are not written to {es}.
Enable dry run mode to allow test runs when applying bulk actions to rules. Dry runs can help you verify that the rules you specified can actually be updated. They're also useful for testing bulk rule updates before they're permanently applied. When dry mode is enabled, bulk actions are temporarily applied to specified rules to show you a realistic set of results. Rule updates are not written to {es}.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nastasha-solomon I don't think that strictly speaking this is a correct statement:

When dry mode is enabled, bulk actions are temporarily applied to specified rules to show you a realistic set of results.

In the dry run mode, we don't return any results in the response body. We only return errors if there are any. So it's correct to say that this mode "can help you verify that the rules you specified can actually be updated". But it won't help the user to preview what would be the result (the updated, created, or deleted rules) of an action. So, you can think about this mode as a validation function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@banderror ah, thanks for clarifying that. Would it be correct to say that dry run mode identifies rules that will not accept bulk actions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nastasha-solomon yep, that would be correct!

Copy link
Contributor

@benironside benironside left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few minor suggestions, overall LGTM!

vitaliidm and others added 10 commits July 27, 2022 16:39
Co-authored-by: Benjamin Ironside Goldstein <91905639+benironside@users.noreply.github.com>
Co-authored-by: Benjamin Ironside Goldstein <91905639+benironside@users.noreply.github.com>
Co-authored-by: nastasha-solomon <79124755+nastasha-solomon@users.noreply.github.com>
Co-authored-by: nastasha-solomon <79124755+nastasha-solomon@users.noreply.github.com>
Co-authored-by: nastasha-solomon <79124755+nastasha-solomon@users.noreply.github.com>
Co-authored-by: nastasha-solomon <79124755+nastasha-solomon@users.noreply.github.com>
@vitaliidm
Copy link
Contributor Author

@nastasha-solomon, @benironside thanks for the feedback. Have applied the suggestions with some small amends. Please, have a look at final version. thanks

Copy link
Contributor

@nastasha-solomon nastasha-solomon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some final comments for your consideration. Thank you again for all your work on these docs. :)

vitaliidm and others added 3 commits July 29, 2022 11:30
Co-authored-by: nastasha-solomon <79124755+nastasha-solomon@users.noreply.github.com>
Co-authored-by: nastasha-solomon <79124755+nastasha-solomon@users.noreply.github.com>
Copy link
Contributor

@nastasha-solomon nastasha-solomon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small nit. Otherwise LGTM! Thanks, both!

Co-authored-by: nastasha-solomon <79124755+nastasha-solomon@users.noreply.github.com>
Copy link
Contributor

@jmikell821 jmikell821 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated files LGTM! 👍 Thanks for getting these changes in.

@vitaliidm vitaliidm merged commit 14f75a2 into elastic:main Aug 2, 2022
mergify bot pushed a commit that referenced this pull request Aug 2, 2022
vitaliidm added a commit that referenced this pull request Aug 2, 2022
…_action API (#2210) (#2239)

Adds dry_run mode description to _bulk_action API

Preview [here](https://security-docs_2210.docs-preview.app.elstc.co/guide/en/security/master/bulk-actions-rules-api.html).

- issue elastic/kibana#125512
- PR elastic/kibana#134664

(cherry picked from commit 14f75a2)

Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants