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] Create RulesManagementClient, initial implementation #182802

Merged
merged 49 commits into from
May 28, 2024

Conversation

jpdjere
Copy link
Contributor

@jpdjere jpdjere commented May 7, 2024

Partially addresses: #180128

Summary

  • Creates RulesManagementClient, which centralizes CRUD utilites for rules, at x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/crud/rules_management_client.ts
    • Move ML Auth validation from endpoints to the new client utils.
  • Adds client to SecuritySolution API requests context.
  • Deletes createRules, deletesRules,updateRules and patchRules utils and replaces them with new client methods.
  • Testing:
    • Creates individual test files for each "public" method of the RulesManagementClient.
    • Creates importable mock of the client

To-Do:

  • Replace readRules method for a new public method within the API (left out of this PR to keep scope and size manageable)

Flaky Test Runner

Checklist

Delete any items that are not applicable to this PR.

@jpdjere
Copy link
Contributor Author

jpdjere commented May 14, 2024

/ci

@jpdjere jpdjere changed the title [Security Solution] Create RulesManagamentClient to refactor utilities [Security Solution] Create RulesManagementClient to refactor utilities May 14, 2024
@jpdjere
Copy link
Contributor Author

jpdjere commented May 14, 2024

/ci

4 similar comments
@jpdjere
Copy link
Contributor Author

jpdjere commented May 15, 2024

/ci

@jpdjere
Copy link
Contributor Author

jpdjere commented May 15, 2024

/ci

@jpdjere
Copy link
Contributor Author

jpdjere commented May 15, 2024

/ci

@jpdjere
Copy link
Contributor Author

jpdjere commented May 18, 2024

/ci

@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@elastic elastic deleted a comment from kibanamachine May 24, 2024
@jpdjere
Copy link
Contributor Author

jpdjere commented May 24, 2024

@e40pud I addressed the comments and did some refactoring.

I ran the flaky test runner on the Exceptions code that I touched, which is what your team owns.

Can you please take another look at the PR? Thanks!!

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6099

[❌] Security Solution Rule Management - Cypress: 74/100 tests passed.
[✅] [Serverless] Security Solution Rule Management - Cypress: 100/100 tests passed.

see run history

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

Super nit - gonna be that person that asks about naming 😄 Any thoughts on making it securitySolutionRulesManagementClient or securityRulesManagementClient - for new devs and anyone quickly digging through the code it might be confusing to see rulesClient and rulesManagementClient.

Copy link
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.

LGTM! @yctercero has a valid point regarding two clients with similar names. Would be nice to consider how to solve that.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
securitySolution 120 121 +1

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
securitySolution 36 37 +1
Unknown metric groups

API count

id before after diff
securitySolution 190 191 +1

ESLint disabled in files

id before after diff
securitySolution 82 81 -1

ESLint disabled line counts

id before after diff
securitySolution 522 523 +1

History

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

cc @jpdjere

@banderror
Copy link
Contributor

@yctercero @e40pud No worries, as everyone knows, naming is the hardest problem in programming 🙂 We will think about a better name and probably open a separate PR for renaming it. cc @nikitaindik

@xcrzx xcrzx merged commit 5bf276d into elastic:main May 28, 2024
42 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 28, 2024
@xcrzx
Copy link
Contributor

xcrzx commented May 28, 2024

I've listed all remaining work in this follow-up ticket: #184364.

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:Rule Management Security Solution Detection Rule Management area refactoring 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.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants