Skip to content

[Security Solution] Get rid of AlertIds schema duplication#177425

Merged
maximpn merged 7 commits intoelastic:mainfrom
maximpn:get-rid-of-alert-ids-schema-duplication
Feb 23, 2024
Merged

[Security Solution] Get rid of AlertIds schema duplication#177425
maximpn merged 7 commits intoelastic:mainfrom
maximpn:get-rid-of-alert-ids-schema-duplication

Conversation

@maximpn
Copy link
Copy Markdown
Contributor

@maximpn maximpn commented Feb 21, 2024 •
edited
Loading

Summary

This PR moves AlertIds schema definition to one schema to avoid duplication.

Details

OAS should as simple and straightforward as possible. Having the same entities defined in different schemas may spawn ambiguity issues. On top of that OAS docs bundler requires unique names for shared schemas.

AlertIds definition is duplicated in x-pack/plugins/security_solution/common/api/endpoint/model/schema/common.schema.yaml and x-pack/plugins/security_solution/common/api/detection_engine/alert_assignees/set_alert_assignees_route.schema.yaml.

To get rid of this duplication AlertIds definition has been moved in a common Shared Alert Schema Primitives and referenced accordingly in the schema files it's used. Additionally NonEmptyString and UUID schemas were moved from rule schema to a common Shared Primitives Schema as it's not only related to the rule schema.

@maximpn maximpn added technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. 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 labels Feb 21, 2024
@maximpn maximpn self-assigned this Feb 21, 2024
@maximpn maximpn requested review from banderror and e40pud February 21, 2024 12:08
@maximpn maximpn marked this pull request as ready for review February 21, 2024 12:08
@maximpn maximpn requested review from a team as code owners February 21, 2024 12:08
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Feb 21, 2024

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

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Feb 21, 2024

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

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Feb 21, 2024

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

@maximpn
Copy link
Copy Markdown
Contributor Author

maximpn commented Feb 21, 2024

/ci

@maximpn maximpn force-pushed the get-rid-of-alert-ids-schema-duplication branch from 648f189 to 4331037 Compare February 21, 2024 14:07
@maximpn maximpn requested a review from a team as a code owner February 21, 2024 14:07
banderror
banderror approved these changes Feb 21, 2024
View reviewed changes
Copy link
Copy Markdown
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.

LGTM 👍
@marshallmain Do you want to check this as well?

marshallmain
marshallmain reviewed Feb 21, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

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

Looks fine overall, just a folder naming suggestion

.github/CODEOWNERS Outdated
Copy link
Copy Markdown
Contributor

@marshallmain marshallmain Feb 21, 2024

Choose a reason for hiding this comment

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

Suggested change
/x-pack/plugins/security_solution/common/api/common_schemas @elastic/security-detection-rule-management @elastic/security-detection-engine
/x-pack/plugins/security_solution/common/api/model @elastic/security-detection-rule-management @elastic/security-detection-engine

For each individual domain, shared components within the domain go into a model folder - I think we should use the same naming scheme at the top level of the /api folder as well. Shared components across the entire security solution API would be found in /api/model.

Copy link
Copy Markdown
Contributor Author

@maximpn maximpn Feb 22, 2024

Choose a reason for hiding this comment

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

Agree. I hesitated between model and something less generic but unified naming is much better.

e40pud
e40pud approved these changes Feb 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@e40pud e40pud left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@maximpn maximpn force-pushed the get-rid-of-alert-ids-schema-duplication branch from 5425c8e to 093c1ab Compare February 22, 2024 08:57
tomsonpl
tomsonpl approved these changes Feb 23, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

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

Defend Workflows LGTM 👍 Thank you!

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Feb 23, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5011 5013 +2

Async chunks

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

id before after diff
securitySolution 13.0MB 13.0MB +121.0B

History

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

cc @maximpn

@maximpn maximpn merged commit 2fa44a5 into elastic:main Feb 23, 2024
@maximpn maximpn deleted the get-rid-of-alert-ids-schema-duplication branch February 23, 2024 11:56
@kibanamachine kibanamachine added v8.14.0 backport:skip This PR does not require backporting labels Feb 23, 2024
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
@maximpn @fkanout
[Security Solution] Get rid of AlertIds schema duplication (elastic#1…
f351523 
…77425)

## Summary

This PR moves `AlertIds` schema definition to one schema to avoid duplication.

## Details

OAS should as simple and straightforward as possible. Having the same entities defined in different schemas may spawn ambiguity issues. On top of that [OAS docs bundler](elastic#171526) requires unique names for shared schemas.

`AlertIds` definition is duplicated in `x-pack/plugins/security_solution/common/api/endpoint/model/schema/common.schema.yaml` and `x-pack/plugins/security_solution/common/api/detection_engine/alert_assignees/set_alert_assignees_route.schema.yaml`.

To get rid of this duplication `AlertIds` definition has been moved in a common `Shared Alert Schema Primitives` and referenced accordingly in the schema files it's used. Additionally `NonEmptyString` and `UUID` schemas were moved from rule schema to a common `Shared Primitives Schema` as it's not only related to the rule schema.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marshallmain marshallmain marshallmain left review comments

@e40pud e40pud e40pud approved these changes

@banderror banderror banderror approved these changes

@tomsonpl tomsonpl tomsonpl approved these changes

@szwarckonrad szwarckonrad Awaiting requested review from szwarckonrad szwarckonrad was automatically assigned from elastic/security-defend-workflows

Assignees

@maximpn maximpn

Labels

backport:skip This PR does not require backporting impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. 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. technical debt Improvement of the software architecture and operational architecture v8.14.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@maximpn @elasticmachine @kibana-ci @e40pud @banderror @tomsonpl @marshallmain @kibanamachine