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

[Detections][Discuss] Detection engine tech debt/refactor opportunities #80792

Open
Tracked by #165878
marshallmain opened this issue Oct 16, 2020 · 3 comments
Open
Tracked by #165878
Assignees
Labels
Feature:Detection Rules Anything related to Security Solution's Detection Rules Team:Detection Engine Security Solution Detection Engine Area Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM technical debt Improvement of the software architecture and operational architecture

Comments

@marshallmain
Copy link
Contributor

marshallmain commented Oct 16, 2020

Summary

Now with 6 different rule types in the detection engine we've accumulated some tech debt. Below are areas that I think we can refactor to reduce maintenance burden and make the detection engine easier to reason about.

High level goals:

  • Reduce data structure duplication by moving towards a single source of truth for schemas

    • We have 3 different schemas that each redefine parts of the rule schema and therefore must remain in sync - signalSchema, RuleTypeParams, and RulesSchema. A single schema should represent rules and should be used as the basis for derivative schemas (for example, the rule APIs return a flattened version of the rule in the response - this flattened schema can be defined in terms of the source of truth so we avoid duplication)
      • When validating rule params in the executor, we use signalSchema for validation, but then use the params as type RuleTypeParams which is defined independently. Some fields are defined with schema.nullable in signalSchema, but the corresponding field in RuleTypeParams is possibly undefined not null. This can lead to some unexpected behavior with null values showing up in rules and eventually in signals created by those rules.
    • Most or all of the rule params schema is copied in 30+ places as parameters for various routes and functions, e.g. create/update/patch rule. This makes it tedious and error prone to add new fields to the rule schema. Additionally, it's not always obvious when certain fields are purposely excluded from the list of parameters. In general I propose defining the parameters by using Pick or Omit on the source of truth so it's easy to update and clear when fields are included/omitted.
    • Rules are destructured and passed as a long list of parameters into functions such as searchAfterAndBulkCreate, among others. Often many of these parameters are then passed through to other functions inside. This makes it hard to add new fields to rules since every function definition on the call stack has to be modified to pass each field through. Passing the entire rule through as a single unit makes it easier to add new fields, and IMO easier to reason about where each parameter comes from.
  • Establish clear divisions between rule types by creating separate schemas for each rule type, merged (via union) into the single source of truth

    • Each rule type could have its own params schema, e.g. KQLRuleParams, EQLRuleParams, ThreatRuleParams, etc. The overall rule params schema then becomes KQLRuleParams | EQLRuleParams | ThreatRuleParams | ....
      • This would eliminate the need for extra functions that do validation that depends on the type of the rule
    • Currently, the general design is one large params schema that covers all rule types. Fields that are specific to some subset of rule types are optional in the complete schema, even if they are required for the rule type (e.g. threshold is required for threshold rules, therefore it is optional in the complete rule schema). This means we have a number of functions that do some additional validation based on the type when creating/updating rules, ensuring that these optional fields are there when required. However, these functions have to be maintained in sync with the schema and add complexity which is hard to reason about.

I propose that the single source of truth is the rule SO data structure, which in turn is represented using a union of the individual rule types. The schemas for the various create/update/patch requests would be defined separately and should be the few places where we see some schema duplication due to the need for setting defaults for certain fields in the routes. However, unit tests can verify that the output from decoding with the create/update/patch schemas is a valid rule by following up with a decode using the source of truth rule schema. This ensures that the route schemas stay in sync with the source of truth.

Prior refactoring art

@marshallmain marshallmain added Team:SIEM Feature:Detection Rules Anything related to Security Solution's Detection Rules labels Oct 16, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Oct 27, 2020
@peluja1012 peluja1012 added the Team:Detections and Resp Security Detection Response Team label Oct 28, 2020
@peluja1012
Copy link
Contributor

Related PR: #82462

@peluja1012 peluja1012 added Team:Detection Alerts Security Detection Alerts Area Team technical debt Improvement of the software architecture and operational architecture labels Sep 15, 2021
@marshallmain
Copy link
Contributor Author

Remaining work on this issue is to remove the legacy RulesSchema (kibana/x-pack/plugins/security_solution/common/detection_engine/schemas/response/rules_schema.ts) in favor of the new schemas introduced in #82462.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Rules Anything related to Security Solution's Detection Rules Team:Detection Engine Security Solution Detection Engine Area Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

5 participants