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] Get rid of the OrUndefined types #138614

Closed
2 tasks done
banderror opened this issue Aug 11, 2022 · 3 comments
Closed
2 tasks done

[Security Solution] Get rid of the OrUndefined types #138614

banderror opened this issue Aug 11, 2022 · 3 comments
Assignees
Labels
Feature:Detection Rules Security Solution rules and Detection Engine refactoring 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

Comments

@banderror
Copy link
Contributor

banderror commented Aug 11, 2022

Background: #132409 (comment)

Summary

We use an OrUndefined pattern when typing various rule parameters:

export const index = t.array(t.string);
export type Index = t.TypeOf<typeof index>;

export const indexOrUndefined = t.union([index, t.undefined]);
export type IndexOrUndefined = t.TypeOf<typeof indexOrUndefined>;

However, while it's useful to create types for most of the parts of a domain model (for both primitive types like Index here and complex data structures like RelatedIntegrationArray) to be able to trace their usage in the codebase,

  • Whether a value can be undefined or null or combined with any other type depends on the context (usage). It seems to be more natural to define it in the place where it's used.
  • Typescript allows making all properties X | undefined or X | null etc in bulk - no need to create N special types for that. It should be possible to do the same with io-ts.
  • No OrUndefined types means less code to write, export, and import. Should be good for the compiler.
  • The original type (like Index) can be annotated with a jsdoc comment.

Let's move away from this OrUndefined pattern, delete the OrUndefined types and values, and clean up the remaining types like that:

/**
 * An array of index patterns of a rule.
 */
export type Index = t.TypeOf<typeof Index>;
export const Index = t.array(t.string);

This leverages TypeScript's type aliases (notice that the const declaration is the same name as the type) to comment both the variable and the type at the same time. This is a nice little usability perk in the IDE.

Example of a type that sticks to this new pattern:

/**
* Related integration is a potential dependency of a rule. It's assumed that if the user installs
* one of the related integrations of a rule, the rule might start to work properly because it will
* have source events (generated by this integration) potentially matching the rule's query.
*
* NOTE: Proper work is not guaranteed, because a related integration, if installed, can be
* configured differently or generate data that is not necessarily relevant for this rule.
*
* Related integration is a combination of a Fleet package and (optionally) one of the
* package's "integrations" that this package contains. It is represented by 3 properties:
*
* - `package`: name of the package (required, unique id)
* - `version`: version of the package (required, semver-compatible)
* - `integration`: name of the integration of this package (optional, id within the package)
*
* There are Fleet packages like `windows` that contain only one integration; in this case,
* `integration` should be unspecified. There are also packages like `aws` and `azure` that contain
* several integrations; in this case, `integration` should be specified.
*
* @example
* const x: RelatedIntegration = {
* package: 'windows',
* version: '1.5.x',
* };
*
* @example
* const x: RelatedIntegration = {
* package: 'azure',
* version: '~1.1.6',
* integration: 'activitylogs',
* };
*/
export type RelatedIntegration = t.TypeOf<typeof RelatedIntegration>;
export const RelatedIntegration = t.exact(
t.intersection([
t.type({
package: NonEmptyString,
version: NonEmptyString,
}),
t.partial({
integration: NonEmptyString,
}),
])
);

Todo

  • Delete the OrUndefined types and values.
  • Refactor the remaining types.
@banderror banderror added refactoring technical debt Improvement of the software architecture and operational architecture Feature:Detection Rules Security Solution rules and Detection Engine 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 Aug 11, 2022
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

banderror added a commit that referenced this issue Oct 21, 2022
…toring Rule Management (#142950)

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

## Summary

In this PR we are:

- Splitting the Detection Engine into subdomains ([ticket](#138600)). 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](#143532))

## 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!

<img width="2592" alt="Screenshot 2022-10-18 at 16 08 40" src="https://user-images.githubusercontent.com/7359339/196453965-b65f5b49-9a33-4d90-bb48-1347e9576223.png">

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

- [x] #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. 
- [x] #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.
- [x] #92169 From some of the Rule Management components, logic was extracted to hooks located in `security_solution/public/detection_engine/rule_management/logic`. 

### Checklist

- [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
@banderror banderror self-assigned this Oct 21, 2022
@banderror
Copy link
Contributor Author

Done in #142950

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Rules Security Solution rules and Detection Engine refactoring 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

No branches or pull requests

2 participants