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] Removes items_per_search and concurrent_searches from upgrade/_review API endpoint logic #190440

Merged

Conversation

dplumlee
Copy link
Contributor

Summary

Addresses #188061

Removes the threat match fields items_per_search and concurrent_searches from the DiffableRule type we utilize in the upgrade/_review endpoint logic. This omits these fields from the upgrade review workflow as we will never have incoming updates for the fields.

For maintainers

@dplumlee dplumlee added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules v8.16.0 labels Aug 13, 2024
@dplumlee dplumlee self-assigned this Aug 13, 2024
@dplumlee dplumlee requested a review from a team as a code owner August 13, 2024 17:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@dplumlee dplumlee requested a review from jpdjere August 13, 2024 17:57
Copy link
Contributor

@nikitaindik nikitaindik left a comment

Choose a reason for hiding this comment

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

Thanks @dplumlee! I've taken a look. Left one comment.

@@ -207,8 +207,6 @@ const extractDiffableThreatMatchFieldsFromRuleObject = (
threat_index: rule.threat_index,
threat_mapping: rule.threat_mapping,
threat_indicator_path: rule.threat_indicator_path,
concurrent_searches: rule.concurrent_searches,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be a good idea to add a comment explaining why these two fields are not included in the DiffableRule object?
Otherwise maybe one day someone will notice that they are missing and add them back.

@banderror
Copy link
Contributor

@dplumlee What about the changes in the upgrade/_perform endpoint?

@dplumlee
Copy link
Contributor Author

@banderror: @jpdjere was going to do that in his upcoming upgrade/_perform PR, but if the work is not too invasive, it could probably be done here too

@banderror
Copy link
Contributor

banderror commented Aug 27, 2024

@dplumlee It will take a lot of time to finalize the upgrade API endpoint. Is there a strong reason for doing it with all the other changes we need to do in it, or we can do it in this PR instead? @jpdjere any opinion?

@jpdjere
Copy link
Contributor

jpdjere commented Aug 27, 2024

@dplumlee @banderror

I don't think it makes sense to introduce logic in the current shape of the /upgrade/_perform endpoint to force the update of these two fields to the current version. Whatever changes are introduced as part of this PR will definitely be overwritten by the new upgrade logic that I'm working on (although the result will be the same - always update to current under the hood)

The reason I don't think we need to include those changes in this PR is because this is an internal endpoint which we currently use only using a global pick_version of TARGET, so that the fields are overwritten if they are custozimed. But since prebuilt rules are still officially not customizable that's exactly the same behaviour with all other fields that might be customized now via API. So until we actually release the Rule Customization feature, forcing the update of these two fields to CURRENT doesn't make sense specifically. So I'd leave this to the work I'm doing on #191439

Copy link
Contributor

@jpdjere jpdjere left a comment

Choose a reason for hiding this comment

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

LGTM ✅

Tested and working as expected:

Before:
image

After:
image

I will take care of removing logic related to these fields in /upgrade/_perform

@dplumlee
Copy link
Contributor Author

dplumlee commented Sep 4, 2024

@elasticmachine merge upstream

@dplumlee dplumlee enabled auto-merge (squash) September 4, 2024 14:49
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 20.7MB 20.7MB -900.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dplumlee

@dplumlee dplumlee merged commit 2ac4a48 into elastic:main Sep 4, 2024
44 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants