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] Restructuring folders of Detection Engine + refactoring Rule Management #142950

Merged
merged 29 commits into from
Oct 21, 2022

Conversation

banderror
Copy link
Contributor

@banderror banderror commented Oct 9, 2022

Partially addresses: #138600, #92169, #138606
Addresses: #136957, #136962, #138614

Summary

In this PR we are:

  • Splitting the Detection Engine into subdomains (ticket). Every subdomain got its own folder under detection_engine, and we moved some (not all) code into them. More on that is below. New subdomains introduced:
    • fleet_integrations
    • prebuilt_rules
    • rule_actions_legacy
    • rule_exceptions
    • rule_management
    • rule_preview
    • rule_schema
    • rule_creation_ui
    • rule_details_ui
    • rule_management_ui
    • rule_exceptions_ui
  • Updating the CODEOWNERS file accordingly.
  • Refactoring the Rule Management page and the Rules table. Our main focus was on the way how we communicate with the API endpoints, how we cache and invalidate the fetched data, and how this code is organized in the codebase. More on that is below.
  • Increasing the bundle size limit. This is going to be decreased back in a follow-up PR (ticket)

Restructuring folders into subdomains

For the background and problem statement, please refer to #138600

We were focusing on code that is closely related to the Rules area: either owned by us de facto (we work on it) or owned by us de jure (according to the CODEOWNERS file). Or goal was to explicitly extract code that we don't own de facto into separate subdomains, transfer ownership to other area teams, and reflect this in the CODEOWNERS file. On the other hand, we wanted the code that we own to also be organized in clear subdomains that we could easily own via CODEOWNERS. We didn't touch the code that is already explicitly owned by other area teams, e.g. x-pack/plugins/security_solution/server/lib/detection_engine/rule_types.

This is a draft "domain map" - an architectural diagram that shows how the Detection Engine could be split into subdomains. It's more a TO-BE idea/aspiration rather than an AS-IS statement. Any feedback, critiques, and suggestions would be extremely appreciated!

Screenshot 2022-10-18 at 16 08 40

It shows the flow of dependencies between subdomains and proposes some rules:

  • The whole graph of dependencies between all subdomains should be a DAG. There should not be bi-directional or circular dependencies between them.
  • Generic subdomains represent some general knowledge that can be used/applied outside of the Detection Engine.
    • Can depend on some generic kbn packages, npm packages or utils.
    • Can't depend on any other Detection Engine subdomains.
  • Crosscutting subdomains represent some code that can be common to / shared between many other subdomains. This could be some very common domain models and API schemas.
    • Can depend on generic subdomains.
    • Can depend on other crosscutting subdomains (dependencies between them must form a DAG).
    • Can't depend on core or UI subdomains.
  • Core subdomains contain most of the "meat" of the Detection Engine: domain models, server-side and client-side business logic, server-side API endpoints, client-side UI (potentially shareable between several pages).
    • Can depend on crosscutting and generic subdomains.
    • Can depend on other core subdomains (dependencies between them must form a DAG).
    • Can't depend on UI subdomains.
  • UI subdomains contain the implementation of pages related to the Detection Engine. Every page can easily depend on several core subdomains, so these subdomain are on top of everything.
    • Can depend on any other subdomains. Dependencies must form a DAG.

Dashed lines show some existing dependencies that we think should be eliminated.

Ownership TO-BE is color-coded. We updated the CODEOWNERS file according to the new folders.

The folder restructuring is not 100% finished but we did a big part of it. Most of the FE code continues to live in legacy folders, e.g. see x-pack/plugins/security_solution/public/detections. So this work is to be continued...

Refactoring of Rule Management FE

  • [Security Solution] Migrate hooks that fetch data and mutate server state to react-query #136957 For effective HTTP requests caching and deduplication, we've migrated all data fetching logic to useQuery and useMutation hooks from react-query. That allowed us to introduce the following improvements to our codebase:
    • All outgoing HTTP requests are now automatically deduplicated. That means that data fetching hooks like useRule could be used on any level in the component tree to access response data directly. So, no need to put the hook on the top level anymore and use prop-drilling to make the response data available to all children components that require it.
    • All HTTP responses are now cached with the default TTL of 5 minutes—no more redundant requests. With a hot cache, transitions to some pages now happen immediately.
  • [Security Solution] Organize data fetching hooks in one place #136962 Data fetching hooks of the Rules Area are now organized in one place. security_solution/public/detection_engine/rule_management/api/hooks contains abstraction layer on top of the Kibana's HTTP client. All data fetching should happen exclusively through that layer to ensure that:
    • Mutation queries automatically invalidate associated cache entries.
    • Optimistic updates or updates from mutation responses could be implemented centrally where possible.
  • [Security Solution] Extract frontend rule management logic from React components #92169 From some of the Rule Management components, logic was extracted to hooks located in security_solution/public/detection_engine/rule_management/logic.

Checklist

@banderror banderror self-assigned this Oct 9, 2022
@banderror banderror added refactoring technical debt Improvement of the software architecture and operational architecture 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. Feature:Rule Exceptions Security Solution Detection Rule Exceptions area Feature:Rule Actions Security Solution Detection Rule Actions area Feature:Rule Management Security Solution Detection Rule Management area Feature:Detection Rule Preview Security Solution Detection Rule Preview feature Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area Feature:Rule Creation Security Solution Detection Rule Creation workflow Feature:Rule Details Security Solution Detection Rule Details page labels Oct 9, 2022
@banderror banderror changed the title [Security Solution] Refactoring rule management [Security Solution] Extract Detection Engine code into subdomains Oct 9, 2022
@banderror banderror force-pushed the onweek-refactoring-rule-management branch 5 times, most recently from 912c1f0 to 82c825b Compare October 11, 2022 13:20
@banderror banderror changed the title [Security Solution] Extract Detection Engine code into subdomains [Security Solution] Refactoring Rule Management Oct 11, 2022
@banderror banderror added the ci:no-auto-commit Disable auto-committing changes on CI label Oct 11, 2022
@banderror banderror force-pushed the onweek-refactoring-rule-management branch 4 times, most recently from 6c53dd6 to 72dbef3 Compare October 13, 2022 13:13
@banderror banderror force-pushed the onweek-refactoring-rule-management branch 2 times, most recently from f94c54b to 66cf3f7 Compare October 13, 2022 18:21
@banderror banderror force-pushed the onweek-refactoring-rule-management branch from 7de12d7 to 8f55566 Compare October 20, 2022 16:40
@banderror
Copy link
Contributor Author

banderror commented Oct 20, 2022

@marshallmain @vitaliidm Thanks for your great questions about the semantics of what is a model.

Is there a definition somewhere for what we expect to find in model folders vs api folders in security_solution/common? It seems like schemas can live in either folder, but I don't know what makes a schema an API vs a model that's used in an API.

Is the one of ideas to use terms model/schema interchangeably?
As I can see, the schemas are just put in the model folder.
To my understanding and experience, the term model is usually used as rather an interface of access/write data, which can be described by schemas.

What's a model? Sometimes people call "models" data that they store in a DB, e.g. in Rails, it's an Active Record pattern. I use the term model in a broader sense. It's our domain model, meaning "things" that our app "consists of", "operates on", "shows", "allows the user to interact with", etc. Normally these things are known to the end users. Examples: rule, rule action, rule execution status, alert, source event, etc.

It can be:

  • At the common level (isomorphic model):
    • "Things" that can be accepted by or returned from the API (common to multiple endpoints or the whole API)
    • "Things" that can be used in-memory on both FE and BE. So not related to the API, but shared between FE and BE.
    • What "things"? Entities (objects with ids), value objects (objects without ids), or simple values (e.g. enums, well-known constants, etc). Interfaces, types, schemas, and specs for them.
    • In-memory operations on those things (i.e. "what can I do with this model"). Normally, it's various calculations and transformations.
  • At the public level (FE-side model):
    • "Things" that only make sense or are implemented on the FE side.
    • In-memory operations on those things (i.e. "what can I do with this model"). Normally, it's various calculations and transformations. Example: "thing" is RelatedIntegrationDetails value object, operation is "given a list of related integrations of a rule and a list of installed integrations in the system, calculate a list of RelatedIntegrationDetails objects to be shown in the UI.
  • At the server level (BE-side model):
    • "Things" that only make sense or are implemented on the BE side. We could keep our Saved Object types, rule parameter schemas, and other persistent data here.
    • In-memory operations on those things (i.e. "what can I do with this model"). Normally, it's various calculations and transformations. Example: given RuleCreateProps (common, API-level model), create an object that would describe rule creation parameters for the RulesClient.

So this is stuff I suggest keeping in the model folders. As for normalization code (i.e. transformations between common and BE-side models), it's arguable where to keep it. It could be a separate normalization folder. Let's see what would be the cleanest way when we continue this refactoring.

As for the api folders:

  • At the common level:
    • Request and response parameters of individual endpoints. Schemas, types, and values.
    • Some of them can be common to multiple endpoints, e.g. "bulk crud response". It doesn't make them domain models, though, because they are technical details of how our app works.
    • URLs of the API endpoints
    • Well-known constants, enums, and other values related to the API itself
  • At the public level:
    • API client
    • API hooks based on React Query
  • At the server level:
    • Route handlers
    • Any helper functions for route handlers that can implement parts of their logic (if this logic is not shared between routes)

Is the one of ideas to use terms model/schema interchangeably?

Schema is all about validation. You can have a type (e.g. an interface) without a schema for it if it gives you enough safety. If you need an additional safety net (or you just have a weak type system) you can write a schema for this type to implement parsing and additional validation (e.g. allowed range of values for a property, etc).

You can have a model without a schema.

Hope this clarifies it a bit. Feel free to drop any other questions on me until we're aligned.

@banderror
Copy link
Contributor Author

@marshallmain

We have a lot of new files called request_schema.ts, response_schema.ts, route.ts, etc, so with many files open or when searching we have to look at more of the path to determine which one is the one we want. Can we make the file names unique in more cases?

It seemed to me acceptable in this case because I wouldn't expect us to be working with a lot of endpoints at the same time. So compared to calling all components index.tsx I don't think it's a big deal :) On the other hand, I don't have a strong opinion on that so if other people support this renaming I'll do it.

@xcrzx @maximpn thoughts?

What's the difference between detections and detection_engine folders? e.g. public/detection_engine and public/detections. Should we the remaining pieces from detections to detection_engine eventually?

Yes, the idea is to continue moving stuff from detections to detection_engine subdomains and get rid of the detections folders eventually.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3202 3232 +30

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
@kbn/securitysolution-io-ts-alerting-types 127 119 -8

Async chunks

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

id before after diff
securitySolution 10.0MB 9.5MB -436.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 50.9KB 50.6KB -324.0B
Unknown metric groups

API count

id before after diff
@kbn/securitysolution-io-ts-alerting-types 145 138 -7

ESLint disabled in files

id before after diff
@kbn/securitysolution-io-ts-alerting-types 13 9 -4

ESLint disabled line counts

id before after diff
securitySolution 413 438 +25

Total ESLint disabled count

id before after diff
@kbn/securitysolution-io-ts-alerting-types 13 9 -4
securitySolution 490 515 +25
total +21

History

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

cc @xcrzx @banderror

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.

Approving without review, so Alerts Area doesn't block the merge.
cc @marshallmain

@xcrzx
Copy link
Contributor

xcrzx commented Oct 24, 2022

We have a lot of new files called request_schema.ts, response_schema.ts, route.ts, etc, so with many files open or when searching we have to look at more of the path to determine which one is the one we want. Can we make the file names unique in more cases?

It seemed to me acceptable in this case because I wouldn't expect us to be working with a lot of endpoints at the same time. So compared to calling all components index.tsx I don't think it's a big deal :) On the other hand, I don't have a strong opinion on that so if other people support this renaming I'll do it.

@xcrzx @maximpn thoughts?

I'm on board with making those file names unique. Besides making it easier to navigate between open tabs, it also makes it much easier to find a particular file by its name in IDE, Chrome DevTools, or in the console.

jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 24, 2022
* main: (57 commits)
  [Files] Filepicker (elastic#143111)
  [Infrastructure UI] Replace Lens table with EUI table and own api (elastic#142871)
  [api-docs] Daily api_docs build (elastic#143829)
  [api-docs] Daily api_docs build (elastic#143825)
  [api-docs] Daily api_docs build (elastic#143823)
  [Security Solution] Restructuring folders of Detection Engine + refactoring Rule Management (elastic#142950)
  [Dev tools] Fix performance issue with autocomplete suggestions (elastic#143428)
  [Security Solution] Disable ML rule's edit button link under basic license (elastic#143260)
  [Lens]  Use the language-documentation package for formula (elastic#143649)
  [api-docs] Daily api_docs build (elastic#143811)
  [Security Solution] Fix missing title on inspect pop-up (elastic#143601)
  fix incorrect filters being passed to events table causing duplicate entries in our inpsect tool request tab (elastic#143239)
  [Security Solution][Endpoint] `get-file` response action kibana download file API (elastic#143708)
  Rely on refresh context to update stats independently of overview cards. (elastic#143308)
  [RAM] Rule event log - Fix incorrect results when filtering by message and outcome simultaneously (elastic#143119)
  [ML] Display link to create data view from error cases in data frame analytics results pages (elastic#143596)
  Update links in README :) (elastic#143675)
  Add more tests for ml_inference_logic (elastic#143764)
  skip failing test suite (elastic#143717)
  [DOCS] Add assignees to case APIs (elastic#143610)
  ...
e40pud added a commit that referenced this pull request Dec 2, 2022
…out (#146904)

## Summary

These changes fixes the issue with the unexpected "Failed to Fetch Rule"
error toast that appears when user opens the alert details from the Rule
Preview page.

The issue was introduced by [recent
refactoring](#142950) where we
switched from the optional toast to showing error toast always when it
is not possible to fetch the rule (see `useRuleWithFallback` hook). In
case of Rule Preview the rule does not exist and thus server returns 404
(not found) error. In case of Rule Preview we would like to ignore this
error and build the rule from the alert.

Bug Ticket: #146858
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 2, 2022
…out (elastic#146904)

## Summary

These changes fixes the issue with the unexpected "Failed to Fetch Rule"
error toast that appears when user opens the alert details from the Rule
Preview page.

The issue was introduced by [recent
refactoring](elastic#142950) where we
switched from the optional toast to showing error toast always when it
is not possible to fetch the rule (see `useRuleWithFallback` hook). In
case of Rule Preview the rule does not exist and thus server returns 404
(not found) error. In case of Rule Preview we would like to ignore this
error and build the rule from the alert.

Bug Ticket: elastic#146858

(cherry picked from commit 1b4c661)
kibanamachine added a commit that referenced this pull request Dec 2, 2022
…ew Flyout (#146904) (#146921)

# Backport

This will backport the following commits from `main` to `8.6`:
- [[Security Solution] Failed to Fetch Rules and Timeline in Preview
Flyout (#146904)](#146904)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT [{"author":{"name":"Ievgen
Sorokopud","email":"[email protected]"},"sourceCommit":{"committedDate":"2022-12-02T20:17:54Z","message":"[Security
Solution] Failed to Fetch Rules and Timeline in Preview Flyout
(#146904)\n\n## Summary\r\n\r\nThese changes fixes the issue with the
unexpected \"Failed to Fetch Rule\"\r\nerror toast that appears when
user opens the alert details from the Rule\r\nPreview page.\r\n\r\nThe
issue was introduced by
[recent\r\nrefactoring](#142950)
where we\r\nswitched from the optional toast to showing error toast
always when it\r\nis not possible to fetch the rule (see
`useRuleWithFallback` hook). In\r\ncase of Rule Preview the rule does
not exist and thus server returns 404\r\n(not found) error. In case of
Rule Preview we would like to ignore this\r\nerror and build the rule
from the alert.\r\n\r\nBug Ticket:
#146858","sha":"1b4c66101b02dd4c01582a0e7b684eb5b499101a","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:
SecuritySolution","Team:Detection Rules","Team:Detection
Alerts","backport:prev-minor","ci:cloud-deploy","v8.7.0"],"number":146904,"url":"https://github.com/elastic/kibana/pull/146904","mergeCommit":{"message":"[Security
Solution] Failed to Fetch Rules and Timeline in Preview Flyout
(#146904)\n\n## Summary\r\n\r\nThese changes fixes the issue with the
unexpected \"Failed to Fetch Rule\"\r\nerror toast that appears when
user opens the alert details from the Rule\r\nPreview page.\r\n\r\nThe
issue was introduced by
[recent\r\nrefactoring](#142950)
where we\r\nswitched from the optional toast to showing error toast
always when it\r\nis not possible to fetch the rule (see
`useRuleWithFallback` hook). In\r\ncase of Rule Preview the rule does
not exist and thus server returns 404\r\n(not found) error. In case of
Rule Preview we would like to ignore this\r\nerror and build the rule
from the alert.\r\n\r\nBug Ticket:
#146858","sha":"1b4c66101b02dd4c01582a0e7b684eb5b499101a"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/146904","number":146904,"mergeCommit":{"message":"[Security
Solution] Failed to Fetch Rules and Timeline in Preview Flyout
(#146904)\n\n## Summary\r\n\r\nThese changes fixes the issue with the
unexpected \"Failed to Fetch Rule\"\r\nerror toast that appears when
user opens the alert details from the Rule\r\nPreview page.\r\n\r\nThe
issue was introduced by
[recent\r\nrefactoring](#142950)
where we\r\nswitched from the optional toast to showing error toast
always when it\r\nis not possible to fetch the rule (see
`useRuleWithFallback` hook). In\r\ncase of Rule Preview the rule does
not exist and thus server returns 404\r\n(not found) error. In case of
Rule Preview we would like to ignore this\r\nerror and build the rule
from the alert.\r\n\r\nBug Ticket:
#146858","sha":"1b4c66101b02dd4c01582a0e7b684eb5b499101a"}}]}]
BACKPORT-->

Co-authored-by: Ievgen Sorokopud <[email protected]>
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 ci:no-auto-commit Disable auto-committing changes on CI Feature:Detection Rule Preview Security Solution Detection Rule Preview feature Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area Feature:Rule Actions Security Solution Detection Rule Actions area Feature:Rule Creation Security Solution Detection Rule Creation workflow Feature:Rule Details Security Solution Detection Rule Details page Feature:Rule Exceptions Security Solution Detection Rule Exceptions area 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. technical debt Improvement of the software architecture and operational architecture v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.