Security Detection Rule schema validation improvements

[nick-benoit]

• Extends existing conditionalValidation to support:
  • valueRequired - Requires a value exists if condition is satisfied
  • valueAllowed (existing prior use case) - Allows a value to be set only if condition is satisfied
  • valueForbidden - Prevents a value from being set if condition is satisfied
• Adds new Asserts validations eg ListAssert to verify dependent value matches allowed values (useful for composing with other validations to build conditional validations based on dependent properties)
• Removes rule type specific validations in models_* files in favor of schema validations
• Adds validation to enforce index or data_view_id is set (except in cases where they are not supported machine_learning, esql)

Rel: #1368

[Copilot]

Pull Request Overview

This PR enhances schema validation for Security Detection Rules by extending the conditional validation framework to support three validation modes (required, allowed, forbidden) and introducing assertion validators. The changes enforce that index or data_view_id must be set for most rule types while preventing their use in machine_learning and esql rules, moving rule-type-specific validations from model code into declarative schema validations.

Key Changes

• Extended conditionalValidation framework to support valueRequired, valueAllowed, and valueForbidden validation modes
• Added assertion validators (StringAssert, ListAssert, Int64Assert) for composable validation logic
• Removed rule-type-specific validation functions from model files in favor of schema-level validators

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/utils/validators/conditional.go	Refactored validation framework to support three modes and added assertion validators
internal/utils/validators/conditional_test.go	Added comprehensive tests for new validation modes and assertion validators
internal/kibana/security_detection_rule/schema.go	Added schema-level validators for index/data_view_id requirements and rule-type restrictions
internal/kibana/security_detection_rule/models_machine_learning.go	Removed applyMachineLearningValidations function
internal/kibana/security_detection_rule/models_esql.go	Removed applyEsqlValidations function
internal/fleet/output/schema.go	Updated validator function calls to use new naming convention
internal/kibana/security_detection_rule/acc_test.go	Removed redundant test assertions for data_view_id and index fields

[tobio]

One big comment :)

[tobio]

So I really struggled parsing these validations. There's a couple of things I think we should improve in this file:

• We have duplicate ListAssert/StringAssert style functions which just return a different interface. Idiomatic go would return the struct and allow the consumer to use it to fulfill the required interface which means we can have a single Assert function, and use it for both List and String validators.
• The condition and conditionValidator behaviour is similar, but different, and the methods for each are interleaved. So it's hard to track what code matches each struct, but also feels weird that condition performs one sort of validation, whilst conditionValidator performs 3 others.
• conditionValidator providing three different validation paths feels pretty messy.
• The Assert/Allowance/Requirement naming isn't obvious about the actual validator behaviour. I think we could be more explicit about what's being validated.

Most of this was like it when you found it, it was probably quite unwieldy before, and feels like we need to improve things now.

I ended up playing around with this locally, mostly to just focus myself on what was going on. I committed what I'd changed here.

• Changes the naming to DependantPathOneOf/AllowedIfDependantPathOneOf/ForbiddenIfDependantPathOneOf...
• Removes the conditionValidator entirely, all the different rules are provided as validate functions to the condition struct
• Return structs, accept interfaces

I'm not expecting my commit is ready to merge, and can almost certainly be improved upon, and maybe you've got an entirely different, better idea. But I think we should tidy up this file a bit.

[nick-benoit]

Most of this was like it when you found it, it was probably quite unwieldy before, and feels like we need to improve things now.

I think I took the existing unwieldyness and scaled it up quite a bit 😅. ++ on this path forward.

[nick-benoit]

After spending a bit more time here I realized composing validations at the property level results in pretty terrible error messages (example when both index / data_view_id are missing)

│ Error: Invalid Attribute Combination
│ 
│   with elasticstack_kibana_security_detection_rule.test,
│   on deployment.tf line 90, in resource "elasticstack_kibana_security_detection_rule" "test":
│   90: resource "elasticstack_kibana_security_detection_rule" "test" {
│ 
│ No attribute specified when one (and only one) of [index,data_view_id] is required
╵
╷
│ Error: Invalid Configuration
│ 
│   with elasticstack_kibana_security_detection_rule.test,
│   on deployment.tf line 90, in resource "elasticstack_kibana_security_detection_rule" "test":
│   90: resource "elasticstack_kibana_security_detection_rule" "test" {
│ 
│ Attribute 'type' is not one of [machine_learning esql], type is "query"
╵
╷
│ Error: Invalid Attribute Combination
│ 
│   with elasticstack_kibana_security_detection_rule.test,
│   on deployment.tf line 90, in resource "elasticstack_kibana_security_detection_rule" "test":
│   90: resource "elasticstack_kibana_security_detection_rule" "test" {
│ 
│ No attribute specified when one (and only one) of [index,data_view_id] is required
╵
╷
│ Error: Invalid Configuration
│ 
│   with elasticstack_kibana_security_detection_rule.test,
│   on deployment.tf line 90, in resource "elasticstack_kibana_security_detection_rule" "test":
│   90: resource "elasticstack_kibana_security_detection_rule" "test" {
│ 
│ Attribute 'type' is not one of [machine_learning esql], type is "query"

At least for the data_view_id / index validation I added a custom resource level validation: a8d9de3

I think this case is complicated enough it is worth using the custom validation. Though we are still using the property based validation for preventing these values being set for esql and machine_learning rules.

For the same example above now we get:

│ Error: Invalid Configuration
│ 
│   with elasticstack_kibana_security_detection_rule.test,
│   on deployment.tf line 90, in resource "elasticstack_kibana_security_detection_rule" "test":
│   90: resource "elasticstack_kibana_security_detection_rule" "test" {
│ 
│ One of 'index' or 'data_view_id' must be set.

[tobio]

LGTM, small nit