[Security Solution] Two-way prebuilt rule diff by dplumlee · Pull Request #233045 · elastic/kibana

dplumlee · 2025-08-27T04:05:19Z

Partially addresses: https://github.com/elastic/security-team/issues/12507 (internal)

Summary

Adds a 2-way diff comparison algorithm to the codebase and shifts the existing diff code to a "three-way" naming scheme.

In order for the telemetry work described in #230856 to occur, we need to refactor our diff calculation method to return rule schema fields instead of the DiffableRule schema that we currently return. This required a lot of exploratory work to determine the best method in which to accomplish this refactor, as the core function, calculateRuleDiffFields, is used as a core piece of logic in all our prebuilt rule customization related endpoints, and most of our UI components are reliant on this DiffableRule based return structure.

I settled on an approach that adds a separate 2-way diff function that never converts the rule into the diffable rule structure, allowing us to directly compare the rule objects and return exact rule schema fields. This also involved refactoring the normalization process we did in convertToDiffable and extracting out the comparison functions we used in the diff algorithms, so that both diff algorithms would be returning identical "is_customized" calculations given a set of RuleResponse objects.

This PR adds a comprehensive set of tests that verify the synchronization of these two diff algorithms for ease of maintainability and to greatly lessen the possibility of divergence.

This PR also swaps out the is_customized rule source calculation with the new 2-way diff calculation, all tests are running (and passing) with this new diff calculation.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
- FTR tests run
- Cypress tests run

elasticmachine · 2025-09-04T15:15:07Z

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

elasticmachine · 2025-09-04T15:15:08Z

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

elasticmachine · 2025-09-04T15:15:10Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

jkelas

@dplumlee I reviewed the code thoroughly. Also thank you for the session with explanation of lots of the intricacies of this code, this helped me much.

The code looks very good. I only left several remarks, non-critical.

I did manual testing of the code by running Kibana and making sure the diffing and comparing functionality didn't change versus baseline.

Please also run Flaky Test Runner

...ver/lib/detection_engine/prebuilt_rules/logic/diff/calculation/calculate_rule_fields_diff.ts

jkelas · 2025-09-05T10:57:23Z

...ver/lib/detection_engine/prebuilt_rules/logic/diff/calculation/calculate_rule_fields_diff.ts

+
+  const fieldsDiff: Partial<RuleDiffOutcome<TwoWayDiffRule>> = {};
+
+  const keys = new Set([...Object.keys(normalizedBaseRule), ...Object.keys(normalizedCurrentRule)]);


I am wondering if we should not rather take keys from allFieldsComparators and not from the compared rules.
That way the source of the truth for this comparison logic would be our comparators definition here, not the RuleResponse. If we some day add a field to a rule, and forget adding a comparator for it here, it would silently be ignored. We should rather detect it, and be forced to add also a comparator (thus I would also suggest removing the if (comparator) ).

I thought through this some more but am leaning more toward leaving it as is with the added comment descriptions. I think keeping the source of truth as the RuleResponse schema should be the preferred method. The types are already set up to throw an error if a RuleResponse field is missing from the list of comparators if not already omitted on purpose (like what we do with actions, exceptions_list, etc.) and would do so if another field were added to RuleResponse, ensuring a field wouldn't be ignored unless explicitly defined to be ignored.

This way, we also return only the fields in the rules given instead of having a superset of all rule type fields in the outcome object. Currently, we only use this object to sort through for fields not equal between two rules, but it might be useful down the line to have a list of fields present instead of that superset. Not to mention we wouldn't have to compare a bunch of non-existent fields every time we do this diff calculation

jkelas · 2025-09-05T11:42:43Z

...lution/common/detection_engine/prebuilt_rules/diff/normalizers/normalize_query_field.test.ts

+
+import { normalizeQueryField } from './normalize_query_field';
+
+describe('normalizeQueryField', () => {


I would add a few more tests to cover for spaces, \t, null, undefined.

Added in cb3638b

...y_solution/common/detection_engine/prebuilt_rules/diff/normalizers/normalize_filter_array.ts

...ution/common/detection_engine/prebuilt_rules/diff/normalizers/normalize_filter_array.test.ts

...ins/security_solution/common/detection_engine/prebuilt_rules/comparators/are_fields_equal.ts

.../common/api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_rule_diff.ts

...mmon/api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_diff_outcome.ts

...ion_engine/prebuilt_rules/logic/diff/calculation/calculate_rule_diff_synchronization.test.ts

dplumlee · 2025-09-08T19:34:39Z

@jkelas thanks for reviewing, I addressed all your comments and added further test coverage and documentation in the code, please take another look!

kibanamachine · 2025-09-08T22:24:11Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#9302

[✅] Security Solution Rule Management - Prebuilt Rules Customization - Cypress: 100/100 tests passed.
[✅] [Serverless] Security Solution Rule Management - Prebuilt Rules Customization - Cypress: 100/100 tests passed.

see run history

kibanamachine · 2025-09-09T03:18:59Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#9301

[✅] x-pack/solutions/security/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/customization_enabled/configs/ess.config.ts: 100/100 tests passed.
[✅] x-pack/solutions/security/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/customization_enabled/configs/serverless.config.ts: 100/100 tests passed.

see run history

jkelas

Thank you @dplumlee for addressing my comments. I reviewed and I think the code looks very good. I am approving.

banderror

Awesome, this looks like the right direction to move forward. Overall changes LGTM.

I ended up posting a lot of comments, all of them are pretty minor. I'm suggesting to do a bit more polishing and improve the structure of the code and naming to make it easier to read, navigate, maintain, and make it consistent with the three-way diff model.

I didn't review the synchronization tests and didn't test the PR for any potential regressions. Please review and consider addressing my comments, so hopefully we can wrap up this review and merge the PR tomorrow.

I really liked the refactoring that separated concerns properly and introduced these new extractors and normalizers 👍

Thanks @dplumlee!

...security/plugins/security_solution/common/api/detection_engine/prebuilt_rules/model/index.ts

...curity_solution/common/api/detection_engine/prebuilt_rules/model/diff/rule_diff/rule_diff.ts

...olution/common/api/detection_engine/prebuilt_rules/model/diff/rule_diff/rule_diff_outcome.ts

...ins/security_solution/common/detection_engine/prebuilt_rules/diff/normalize_rule_response.ts

banderror · 2025-09-09T18:51:46Z

...ver/lib/detection_engine/prebuilt_rules/logic/diff/calculation/calculate_rule_fields_diff.ts

+    const comparator = allFieldsComparators[fieldKey] as
+      | ((a: unknown, b: unknown) => boolean)
+      | undefined;


There's too many as type castings in this function which is a code smell. There's probably something we can do to make the code type safe.

...ion_engine/prebuilt_rules/logic/diff/calculation/calculate_rule_diff_synchronization.test.ts

banderror · 2025-09-10T14:35:28Z

@elasticmachine merge upstream

banderror

@dplumlee The resulting PR looks perfect. Thanks for the great work!

...ion_engine/prebuilt_rules/logic/diff/calculation/calculate_rule_diff_synchronization.test.ts

kibanamachine · 2025-09-11T06:01:17Z

Starting backport for target branches: 9.1

https://github.com/elastic/kibana/actions/runs/17635485268

elasticmachine · 2025-09-11T06:01:48Z

💚 Build Succeeded

Buildkite Build
Commit: 03b99c8

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	7973	7977	+4

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.5MB	10.5MB	+3.0KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id	before	after	diff
`_data_stream_timestamp`	1	-	-1
`_doc_count`	1	-	-1
`_ignored_source`	1	-	-1
`_index_mode`	1	-	-1
`_inference_fields`	1	-	-1
`_tier`	1	-	-1
`apm-custom-dashboards`	5	-	-5
`apm-server-schema`	2	-	-2
`apm-service-group`	5	-	-5
`application_usage_daily`	2	-	-2
`config`	2	-	-2
`config-global`	2	-	-2
`coreMigrationVersion`	1	-	-1
`created_at`	1	-	-1
`created_by`	1	-	-1
`entity-definition`	9	-	-9
`entity-discovery-api-key`	2	-	-2
`event_loop_delays_daily`	2	-	-2
`favorites`	4	-	-4
`file`	11	-	-11
`file-upload-usage-collection-telemetry`	3	-	-3
`fileShare`	5	-	-5
`infra-custom-dashboards`	4	-	-4
`infrastructure-monitoring-log-view`	2	-	-2
`intercept_trigger_record`	5	-	-5
`legacy-url-alias`	7	-	-7
`managed`	1	-	-1
`ml-job`	6	-	-6
`ml-module`	13	-	-13
`ml-trained-model`	7	-	-7
`monitoring-telemetry`	2	-	-2
`namespace`	1	-	-1
`namespaces`	1	-	-1
`observability-onboarding-state`	2	-	-2
`originId`	1	-	-1
`product-doc-install-status`	7	-	-7
`references`	4	-	-4
`sample-data-telemetry`	3	-	-3
`security-ai-prompt`	8	-	-8
`slo`	11	-	-11
`space`	5	-	-5
`synthetics-monitor`	34	-	-34
`synthetics-monitor-multi-space`	34	-	-34
`tag`	4	-	-4
`type`	1	-	-1
`typeMigrationVersion`	1	-	-1
`ui-metric`	2	-	-2
`updated_at`	1	-	-1
`updated_by`	1	-	-1
`upgrade-assistant-ml-upgrade-operation`	3	-	-3
`upgrade-assistant-reindex-operation`	3	-	-3
`uptime-synthetics-api-key`	2	-	-2
`url`	5	-	-5
`usage-counters`	2	-	-2
total			-246

History

💔 Build #338143 failed c55b345
💔 Build #338063 failed d5569fe
💚 Build #337968 succeeded 732bd91
💛 Build #337620 was flaky 4aa01c9
💚 Build #337100 succeeded c6a07d4
💔 Build #337045 failed f309e38

cc @dplumlee

**Partially addresses:** elastic/security-team#12507 (internal) ## Summary Adds a 2-way diff comparison algorithm to the codebase and shifts the existing diff code to a "three-way" naming scheme. In order for the telemetry work described in elastic#230856 to occur, we need to refactor our diff calculation method to return rule schema fields instead of the `DiffableRule` schema that we currently return. This required a lot of exploratory work to determine the best method in which to accomplish this refactor, as the core function, `calculateRuleDiffFields`, is used as a core piece of logic in all our prebuilt rule customization related endpoints, and most of our UI components are reliant on this `DiffableRule` based return structure. I settled on an approach that adds a separate 2-way diff function that never converts the rule into the diffable rule structure, allowing us to directly compare the rule objects and return exact rule schema fields. This also involved refactoring the normalization process we did in `convertToDiffable` and extracting out the comparison functions we used in the diff algorithms, so that both diff algorithms would be returning identical "is_customized" calculations given a set of `RuleResponse` objects. This PR adds a comprehensive set of tests that verify the synchronization of these two diff algorithms for ease of maintainability and to greatly lessen the possibility of divergence. This PR also swaps out the `is_customized` rule source calculation with the new 2-way diff calculation, all tests are running (and passing) with this new diff calculation. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] [FTR tests run](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/9301) - [x] [Cypress tests run](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/9302) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> (cherry picked from commit b08abf7)

kibanamachine · 2025-09-11T06:08:32Z

💚 All backports created successfully

Status	Branch	Result
✅	9.1

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `9.1`: - [[Security Solution] Two-way prebuilt rule diff (#233045)](#233045)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

**Partially addresses:** elastic/security-team#12507 (internal) ## Summary Adds a 2-way diff comparison algorithm to the codebase and shifts the existing diff code to a "three-way" naming scheme. In order for the telemetry work described in elastic#230856 to occur, we need to refactor our diff calculation method to return rule schema fields instead of the `DiffableRule` schema that we currently return. This required a lot of exploratory work to determine the best method in which to accomplish this refactor, as the core function, `calculateRuleDiffFields`, is used as a core piece of logic in all our prebuilt rule customization related endpoints, and most of our UI components are reliant on this `DiffableRule` based return structure. I settled on an approach that adds a separate 2-way diff function that never converts the rule into the diffable rule structure, allowing us to directly compare the rule objects and return exact rule schema fields. This also involved refactoring the normalization process we did in `convertToDiffable` and extracting out the comparison functions we used in the diff algorithms, so that both diff algorithms would be returning identical "is_customized" calculations given a set of `RuleResponse` objects. This PR adds a comprehensive set of tests that verify the synchronization of these two diff algorithms for ease of maintainability and to greatly lessen the possibility of divergence. This PR also swaps out the `is_customized` rule source calculation with the new 2-way diff calculation, all tests are running (and passing) with this new diff calculation. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] [FTR tests run](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/9301) - [x] [Cypress tests run](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/9302) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

**Partially addresses:** elastic/security-team#12507 (internal) ## Summary Adds a 2-way diff comparison algorithm to the codebase and shifts the existing diff code to a "three-way" naming scheme. In order for the telemetry work described in #230856 to occur, we need to refactor our diff calculation method to return rule schema fields instead of the `DiffableRule` schema that we currently return. This required a lot of exploratory work to determine the best method in which to accomplish this refactor, as the core function, `calculateRuleDiffFields`, is used as a core piece of logic in all our prebuilt rule customization related endpoints, and most of our UI components are reliant on this `DiffableRule` based return structure. I settled on an approach that adds a separate 2-way diff function that never converts the rule into the diffable rule structure, allowing us to directly compare the rule objects and return exact rule schema fields. This also involved refactoring the normalization process we did in `convertToDiffable` and extracting out the comparison functions we used in the diff algorithms, so that both diff algorithms would be returning identical "is_customized" calculations given a set of `RuleResponse` objects. This PR adds a comprehensive set of tests that verify the synchronization of these two diff algorithms for ease of maintainability and to greatly lessen the possibility of divergence. This PR also swaps out the `is_customized` rule source calculation with the new 2-way diff calculation, all tests are running (and passing) with this new diff calculation. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] [FTR tests run](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/9301) - [x] [Cypress tests run](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/9302) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

adds 2 way diff

8617df0

dplumlee self-assigned this Aug 27, 2025

dplumlee added 3 commits August 27, 2025 00:10

switch is_customized to use new diff

f4504e3

changes naming scheme

dafd3c2

fixes normalization

b381d5c

dplumlee force-pushed the rule-diff-2-way branch from 9eb327a to b381d5c Compare September 2, 2025 21:41

dplumlee added 2 commits September 2, 2025 23:12

updates normalization tests

b9d490f

Merge remote-tracking branch 'upstream/main' into rule-diff-2-way

acb3094

dplumlee force-pushed the rule-diff-2-way branch from 9de6a21 to acb3094 Compare September 3, 2025 03:13

adds sync tests

0440bd3

Merge remote-tracking branch 'upstream/main' into rule-diff-2-way

2178204

dplumlee marked this pull request as ready for review September 4, 2025 15:15

dplumlee requested review from a team as code owners September 4, 2025 15:15

dplumlee requested a review from maximpn September 4, 2025 15:15

dplumlee changed the title ~~[Security Solution] 2 way rule diff POC~~ [Security Solution] 2 way rule diff Sep 4, 2025

dplumlee changed the title ~~[Security Solution] 2 way rule diff~~ [Security Solution] Two-way prebuilt rule diff Sep 4, 2025

banderror requested review from banderror and jkelas and removed request for maximpn September 4, 2025 15:35

banderror added the refactoring label Sep 5, 2025

jkelas reviewed Sep 8, 2025

View reviewed changes

dplumlee added 3 commits September 8, 2025 14:59

addresses comments

cb3638b

Merge remote-tracking branch 'upstream/main' into rule-diff-2-way

2d43ade

add comments

f309e38

dplumlee requested a review from jkelas September 8, 2025 19:33

Merge remote-tracking branch 'upstream/main' into rule-diff-2-way

c6a07d4

jkelas approved these changes Sep 9, 2025

View reviewed changes

dplumlee added the v9.1.4 label Sep 9, 2025

banderror reviewed Sep 9, 2025

View reviewed changes

addresses comments

4aa01c9

banderror approved these changes Sep 10, 2025

View reviewed changes

...ion_engine/prebuilt_rules/logic/diff/calculation/calculate_rule_diff_synchronization.test.ts Show resolved Hide resolved

dplumlee enabled auto-merge (squash) September 10, 2025 17:25

Merge remote-tracking branch 'upstream/main' into rule-diff-2-way

03b99c8

dplumlee force-pushed the rule-diff-2-way branch from c55b345 to 03b99c8 Compare September 11, 2025 03:54

dplumlee merged commit b08abf7 into elastic:main Sep 11, 2025
12 checks passed

kibanamachine mentioned this pull request Sep 11, 2025

[9.1] [Security Solution] Two-way prebuilt rule diff (#233045) #234707

Merged

dplumlee deleted the rule-diff-2-way branch September 11, 2025 13:07


		const fieldsDiff: Partial<RuleDiffOutcome<TwoWayDiffRule>> = {};

		const keys = new Set([...Object.keys(normalizedBaseRule), ...Object.keys(normalizedCurrentRule)]);


		import { normalizeQueryField } from './normalize_query_field';

		describe('normalizeQueryField', () => {

Conversation

dplumlee commented Aug 27, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Sep 4, 2025

Uh oh!

elasticmachine commented Sep 4, 2025

Uh oh!

elasticmachine commented Sep 4, 2025

Uh oh!

jkelas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkelas Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

dplumlee Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

jkelas Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

dplumlee Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dplumlee commented Sep 8, 2025

Uh oh!

kibanamachine commented Sep 8, 2025

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#9302

Uh oh!

kibanamachine commented Sep 9, 2025

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#9301

Uh oh!

jkelas left a comment

Choose a reason for hiding this comment

Uh oh!

banderror left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

banderror Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

banderror commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

banderror left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Sep 11, 2025

Uh oh!

elasticmachine commented Sep 11, 2025

💚 Build Succeeded

Metrics [docs]

Module Count

dplumlee commented Aug 27, 2025 •

edited by kibanamachine

Loading

jkelas left a comment •

edited

Loading

banderror left a comment •

edited

Loading

banderror commented Sep 10, 2025 •

edited

Loading

banderror left a comment •

edited

Loading