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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Makes rule_source a required field in RuleResponse #193636

Merged
merged 12 commits into from
Oct 7, 2024
Merged

[Security Solution] Makes rule_source a required field in RuleResponse #193636

merged 12 commits into from
Oct 7, 2024
+47 −10

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Sep 20, 2024 •
edited by kibanamachine
Loading

Resolves #180270

Summary

Sets rule_source to be a required field in the RuleResponse type

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dplumlee dplumlee self-assigned this Sep 20, 2024
@dplumlee dplumlee added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 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 area v8.16.0 labels Sep 24, 2024
@banderror banderror added the backport:version Backport to applied version labels label Sep 26, 2024
@dplumlee dplumlee marked this pull request as ready for review September 26, 2024 19:02
@dplumlee dplumlee requested review from a team as code owners September 26, 2024 19:02
@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 review from xcrzx and removed request for nikitaindik September 26, 2024 19:02
@banderror
Copy link
Contributor

@maximpn @dplumlee FYI this PR interferes with #194132 - when one of them gets merged first, the other one will need to be rebased on top to make sure output OAS bundles are properly regenerated

dhurley14
dhurley14 approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

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

I tested importing a rule where rule_source does not exist and then exporting that same rule. The exported rule has the rule_source property. LGTM.

xcrzx
xcrzx approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Just one question about a possible redundant conversion.

...ine/rule_management/logic/detection_rules_client/converters/internal_rule_to_api_response.ts Outdated
@@ -66,5 +67,6 @@ export const internalRuleToAPIResponse = (
actions: [...actions, ...(systemActions ?? [])],
// Execution summary
execution_summary: executionSummary ?? undefined,
rule_source: snakecaseKeys(normalizedRuleParams.ruleSource, { deep: true }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't rule_source already being converted by commonParamsCamelToSnake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xcrzx It does, this was more of a typing thing as commonParamsCamelToSnake takes a type of BaseRuleParams which still declares rule_source as optional so it doesn't fit the expected return type in this file (rule_source might be undefined). My other idea was to cast with a custom type when we spread the common params into this object, that might be better since it's not actually performing logic twice

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for the clarification. We might need a separate type to represent normalized rule parameters where rule_source is required. That way, we can make commonParamsCamelToSnake a generic function that works with both standard rule parameters and normalized ones.

I'll leave it up to you, both approaches work for me!

@dplumlee dplumlee force-pushed the rule-source-required branch from 552d5f3 to 7a39293 Compare October 3, 2024 18:47
@dplumlee dplumlee force-pushed the rule-source-required branch from b8c8252 to 68e85e2 Compare October 4, 2024 18:09
@dplumlee
Copy link
Contributor Author

dplumlee commented Oct 4, 2024

@elasticmachine merge upstream

@dplumlee
Copy link
Contributor Author

dplumlee commented Oct 4, 2024

@elasticmachine merge upstream

@banderror
Copy link
Contributor

@elasticmachine merge upstream

@dplumlee
Copy link
Contributor Author

dplumlee commented Oct 7, 2024

@elasticmachine merge upstream

@dplumlee dplumlee enabled auto-merge (squash) October 7, 2024 14:30
@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.6MB 20.6MB -41.0B

History

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

cc @dplumlee

@dplumlee dplumlee merged commit 484f95e into elastic:main Oct 7, 2024
43 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11221182473

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 7, 2024
@dplumlee
[Security Solution] Makes rule_source a required field in `RuleResp…
dc9ba6d 
…onse` (elastic#193636)

**Resolves elastic#180270

## Summary

Sets `rule_source` to be a required field in the `RuleResponse` type

### Checklist

Delete any items that are not applicable to this PR.

- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [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

### 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: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 484f95e)
@kibanamachine kibanamachine mentioned this pull request Oct 7, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 7, 2024
@kibanamachine @dplumlee
[8.x] [Security Solution] Makes &#x60;rule_source&#x60; a required fi…
2d7971e 
…eld in &#x60;RuleResponse&#x60; (#193636) (#195303)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Makes &#x60;rule_source&#x60; a required field in
&#x60;RuleResponse&#x60;
(#193636)](#193636)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"56367316+dplumlee@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-10-07T17:56:12Z","message":"[Security
Solution] Makes `rule_source` a required field in `RuleResponse`
(#193636)\n\n**Resolves
https://github.com/elastic/kibana/issues/180270**\r\n\r\n##
Summary\r\n\r\nSets `rule_source` to be a required field in the
`RuleResponse` type\r\n\r\n### Checklist\r\n\r\nDelete any items that
are not applicable to this PR.\r\n\r\n- [
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"484f95e7335a5b8d8df0d8c321d2b2e74db668a8","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","v8.16.0","backport:version"],"title":"[Security Solution] Makes
`rule_source` a required field in
`RuleResponse`","number":193636,"url":"https://github.com/elastic/kibana/pull/193636","mergeCommit":{"message":"[Security
Solution] Makes `rule_source` a required field in `RuleResponse`
(#193636)\n\n**Resolves
https://github.com/elastic/kibana/issues/180270**\r\n\r\n##
Summary\r\n\r\nSets `rule_source` to be a required field in the
`RuleResponse` type\r\n\r\n### Checklist\r\n\r\nDelete any items that
are not applicable to this PR.\r\n\r\n- [
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"484f95e7335a5b8d8df0d8c321d2b2e74db668a8"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193636","number":193636,"mergeCommit":{"message":"[Security
Solution] Makes `rule_source` a required field in `RuleResponse`
(#193636)\n\n**Resolves
https://github.com/elastic/kibana/issues/180270**\r\n\r\n##
Summary\r\n\r\nSets `rule_source` to be a required field in the
`RuleResponse` type\r\n\r\n### Checklist\r\n\r\nDelete any items that
are not applicable to this PR.\r\n\r\n- [
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"484f95e7335a5b8d8df0d8c321d2b2e74db668a8"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>
@dplumlee dplumlee deleted the rule-source-required branch October 7, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xcrzx xcrzx xcrzx approved these changes

@dhurley14 dhurley14 dhurley14 approved these changes

Assignees

@dplumlee dplumlee

Labels
backport:version Backport to applied version labels Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area 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 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Security Solution] Make rule_source field required in RuleResponse
7 participants
@dplumlee @elasticmachine @banderror @kibana-ci @kibanamachine @dhurley14 @xcrzx