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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Test plan for query fields diff algorithm #192529

Merged
merged 2 commits into from
Sep 16, 2024
Merged

[Security Solution] Test plan for query fields diff algorithm #192529

merged 2 commits into from
Sep 16, 2024

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Sep 10, 2024 •
edited
Loading

Summary

Related ticket: #187658

Adds test plan for diff algorithm for query field diff algorithms implemented here: #190179

For maintainers

@dplumlee dplumlee added release_note:skip Skip the PR/issue when compiling release notes test-plan 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 Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules v8.16.0 labels Sep 10, 2024
@dplumlee dplumlee self-assigned this Sep 10, 2024
@dplumlee dplumlee requested a review from a team as a code owner September 10, 2024 22:01
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@dplumlee dplumlee requested review from jpdjere and removed request for maximpn September 10, 2024 22:01
This was referenced Sep 12, 2024
Merged
Closed
jpdjere
jpdjere approved these changes Sep 16, 2024
View reviewed changes
Copy link
Contributor

@jpdjere jpdjere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✔️

pborgonovi
pborgonovi reviewed Sep 16, 2024
View reviewed changes
Copy link

@pborgonovi pborgonovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dplumlee

  • I noticed that the test plan covers arrays, scalar values, and objects. However, I don’t see any explicit mention of boolean fields. Is it being covered somehow?
  • Regarding a data type mismatch, how would the algorithm handle a scenario where the current version has a field as a string, but the target version has it as an object?

@dplumlee
Copy link
Contributor Author

dplumlee commented Sep 16, 2024 •
edited
Loading

@pborgonovi

I noticed that the test plan covers arrays, scalar values, and objects. However, I don’t see any explicit mention of boolean fields. Is it being covered somehow?

We don't have any fields in the DiffableRule type (the fields that we're performing these 3 way diffs on) that are of boolean type. Fields like enable sit outside of the diffable fields. That being said, any fields that we don't cover with specific tests would be defaulted to using the simpleDiffAlgorithm which just directly compares each version to each other and would work as expected for a boolean type field

Regarding a data type mismatch, how would the algorithm handle a scenario where the current version has a field as a string, but the target version has it as an object?

Unless an algorithm is set up to handle these cases with a merge strategy, it would just deep compare both fields and determine equality. In the case of string vs object, all the algorithms we have now would just see them as two different data types and mark it as a non-solvable conflict depending on what the base_version was in the scenario. We'd just return the current_version and let the user handle that case in the UI. I think most if not all data type mismatches are covered in the test plan for the existing field algorithms.

There might be other fields that have the potential to get in that scenario when upgrading but we haven't created a specific algorithm for them yet and they will use the simpleDiffAlgorithm which doesn't attempt to merge fields anyways.

@dplumlee dplumlee merged commit 6680f35 into elastic:main Sep 16, 2024
7 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 16, 2024
@dplumlee
[Security Solution] Test plan for query fields diff algorithm (elas…
6f5044c 
…tic#192529)

## Summary

Related ticket: elastic#187658

Adds test plan for diff algorithm for arrays of scalar values
implemented here: elastic#190179

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

(cherry picked from commit 6680f35)
@kibanamachine kibanamachine mentioned this pull request Sep 16, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

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

Questions ?

Please refer to the Backport tool documentation

@dplumlee dplumlee deleted the query-diff-algorithm-test-plan branch September 16, 2024 17:44
kibanamachine added a commit that referenced this pull request Sep 16, 2024
@kibanamachine @dplumlee
[8.x] [Security Solution] Test plan for `query` fields diff…
f2fd8c7 
… algorithm (#192529) (#193062)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Test plan for `query` fields diff
algorithm (#192529)](#192529)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-16T17:19:39Z","message":"[Security
Solution] Test plan for `query` fields diff algorithm (#192529)\n\n##
Summary\r\n\r\nRelated ticket:
https://github.com/elastic/kibana/issues/187658\r\n\r\nAdds test plan
for diff algorithm for arrays of scalar values\r\nimplemented here:
https://github.com/elastic/kibana/pull/190179\r\n\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6680f35ef94286d34dd5c56e28575b31eab70836","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","test-plan","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","v8.16.0"],"title":"[Security Solution] Test plan for `query`
fields diff
algorithm","number":192529,"url":"https://github.com/elastic/kibana/pull/192529","mergeCommit":{"message":"[Security
Solution] Test plan for `query` fields diff algorithm (#192529)\n\n##
Summary\r\n\r\nRelated ticket:
https://github.com/elastic/kibana/issues/187658\r\n\r\nAdds test plan
for diff algorithm for arrays of scalar values\r\nimplemented here:
https://github.com/elastic/kibana/pull/190179\r\n\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6680f35ef94286d34dd5c56e28575b31eab70836"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192529","number":192529,"mergeCommit":{"message":"[Security
Solution] Test plan for `query` fields diff algorithm (#192529)\n\n##
Summary\r\n\r\nRelated ticket:
https://github.com/elastic/kibana/issues/187658\r\n\r\nAdds test plan
for diff algorithm for arrays of scalar values\r\nimplemented here:
https://github.com/elastic/kibana/pull/190179\r\n\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6680f35ef94286d34dd5c56e28575b31eab70836"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Davis Plumlee <[email protected]>
dplumlee added a commit that referenced this pull request Sep 16, 2024
@dplumlee
[Security Solution] Integration tests for query diff algorithms (#1…
ceb1b1a 
…92655)

## Summary

Completes #187658


Switches `kql_query`, `eql_query`, and `esql_query` fields to use the
implemented diff algorithms assigned to them in
#190179


Adds integration tests in accordance to
#192529 for the `upgrade/_review`
API endpoint for the `query` field diff algorithms.

### Checklist

Delete any items that are not applicable to this PR.

- [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


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 17, 2024
@dplumlee
[Security Solution] Integration tests for query diff algorithms (el…
8feec13 
…astic#192655)

## Summary

Completes elastic#187658

Switches `kql_query`, `eql_query`, and `esql_query` fields to use the
implemented diff algorithms assigned to them in
elastic#190179

Adds integration tests in accordance to
elastic#192529 for the `upgrade/_review`
API endpoint for the `query` field diff algorithms.

### Checklist

Delete any items that are not applicable to this PR.

- [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

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

(cherry picked from commit ceb1b1a)
kibanamachine added a commit that referenced this pull request Sep 17, 2024
@kibanamachine @dplumlee
[8.x] [Security Solution] Integration tests for &#x60;query&#x60; dif…
9d09dbf 
…f algorithms (#192655) (#193108)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Integration tests for &#x60;query&#x60; diff
algorithms (#192655)](#192655)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-16T23:58:55Z","message":"[Security
Solution] Integration tests for `query` diff algorithms (#192655)\n\n##
Summary\r\n\r\nCompletes
https://github.com/elastic/kibana/issues/187658\r\n\r\n\r\nSwitches
`kql_query`, `eql_query`, and `esql_query` fields to use
the\r\nimplemented diff algorithms assigned to them
in\r\nhttps://github.com//pull/190179\r\n\r\n\r\nAdds
integration tests in accordance
to\r\nhttps://github.com//pull/192529 for the
`upgrade/_review`\r\nAPI endpoint for the `query` field diff
algorithms.\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"ceb1b1a4bf253ac94f9ba0ba649e9a4908a76c51","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["test","release_note:skip","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","v8.16.0"],"title":"[Security Solution] Integration tests for
`query` diff
algorithms","number":192655,"url":"https://github.com/elastic/kibana/pull/192655","mergeCommit":{"message":"[Security
Solution] Integration tests for `query` diff algorithms (#192655)\n\n##
Summary\r\n\r\nCompletes
https://github.com/elastic/kibana/issues/187658\r\n\r\n\r\nSwitches
`kql_query`, `eql_query`, and `esql_query` fields to use
the\r\nimplemented diff algorithms assigned to them
in\r\nhttps://github.com//pull/190179\r\n\r\n\r\nAdds
integration tests in accordance
to\r\nhttps://github.com//pull/192529 for the
`upgrade/_review`\r\nAPI endpoint for the `query` field diff
algorithms.\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"ceb1b1a4bf253ac94f9ba0ba649e9a4908a76c51"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192655","number":192655,"mergeCommit":{"message":"[Security
Solution] Integration tests for `query` diff algorithms (#192655)\n\n##
Summary\r\n\r\nCompletes
https://github.com/elastic/kibana/issues/187658\r\n\r\n\r\nSwitches
`kql_query`, `eql_query`, and `esql_query` fields to use
the\r\nimplemented diff algorithms assigned to them
in\r\nhttps://github.com//pull/190179\r\n\r\n\r\nAdds
integration tests in accordance
to\r\nhttps://github.com//pull/192529 for the
`upgrade/_review`\r\nAPI endpoint for the `query` field diff
algorithms.\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"ceb1b1a4bf253ac94f9ba0ba649e9a4908a76c51"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Davis Plumlee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pborgonovi pborgonovi pborgonovi approved these changes

@jpdjere jpdjere jpdjere approved these changes

Assignees

@dplumlee dplumlee

Labels
Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules 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. test-plan v8.16.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dplumlee @elasticmachine @kibanamachine @jpdjere @pborgonovi