-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Array of scalar values diff algorithm test plan #186325
[Security Solution] Array of scalar values diff algorithm test plan #186325
Conversation
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
|
||
### Rule field doesn't have an update and has no custom value - `AAA` | ||
|
||
#### **Scenario: Rule field is any type** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to find a better name for all these scenarios that would be unique enough to use in a TOC, any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While not perfect, I think adding a three-letter type should work.
### Rule field doesn't have an update and has no custom value - `AAA`
#### **Scenario: AAA - Rule field is any type**
Yep, some duplication, but it's not too bad IMO.
And <field_name> field should be returned from the `upgrade/_review` API endpoint | ||
And <field_name> field should be shown in the upgrade preview UI | ||
|
||
CASE: array fields should work the same agnostic of order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that it's only applicable to some fields and for other fields order doesn't matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand the question, things that aren't array fields wouldn't have an order per say. This is basically saying that any fields that are arrays should perform the same no matter the order (e.g. an array of ["A", "B"]
would be treated the same by the algorithm as ["B", "A"]
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I should have phrased it better. I meant "do we have arrays where order matters and arrays where it doesn't?" I have just checked the code and seen that we don't ever expect the order to matter.
Then I am confused by the meaning of "CASE" keyword. I understand it as "note that you should also test this extra case in addition to a default one". As if by default the order is important, but there is also a case where it isn't.
But here we actually want to convey: "note that all arrays are order-agnostic".
IMO, if we had something like NOTE instead of CASE it wouldn't be confusing.
NOTE: array fields should work the same agnostic of order
I can't find CASE in the Gherkin spec. Is it something that we have introduced ourselves? What's your opinion on using NOTE vs CASE?
...ution/docs/testing/test_plans/detection_response/prebuilt_rules/upgrade_review_algorithms.md
Outdated
Show resolved
Hide resolved
...ution/docs/testing/test_plans/detection_response/prebuilt_rules/upgrade_review_algorithms.md
Outdated
Show resolved
Hide resolved
...ution/docs/testing/test_plans/detection_response/prebuilt_rules/upgrade_review_algorithms.md
Outdated
Show resolved
Hide resolved
...ution/docs/testing/test_plans/detection_response/prebuilt_rules/upgrade_review_algorithms.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dplumlee! I have reviewed and left a few questions and suggestions.
...ution/docs/testing/test_plans/detection_response/prebuilt_rules/upgrade_review_algorithms.md
Outdated
Show resolved
Hide resolved
...ution/docs/testing/test_plans/detection_response/prebuilt_rules/upgrade_review_algorithms.md
Outdated
Show resolved
Hide resolved
...ution/docs/testing/test_plans/detection_response/prebuilt_rules/upgrade_review_algorithms.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a look at the feedback Georgii left on #186323, I think we should reflect those test cases in the test plan. So I think it still makes sense to have a general test case scenario for "any type", but then for scalar arrays, we need a different section not only on scenarios ABC
and -BC
but in all. We should reflect cases in which, in the change from base to current and target:
- an element can be added to either/both of them
- an element can be removed from either/both of them
- an element can stay unchanged in both of them
- elements can be reordered in either/both of them in the same way or different ways
- elements can be duplicated at any given index in an array
Also, for scalar arrays, its especially important to describe the behaviour for how we handle:
- duplicated values
- reordering of values
- case-sensitivity
- empty arrays
as discussed in the PR linked above.
Also, let's change our examples to always use at least 3 elements as default, of course, except when needing to make an example out of adding or removing them.
And customized <field_name> field is different than the Elastic update in this upgrade (current version != target version) | ||
Then for <field_name> field the diff algorithm should output a custom merged version with a solvable conflict | ||
And arrays should be deduplicated before comparison | ||
And arrays should be compared insensitive of case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct to sensitive
And customized <field_name> field is different than the Elastic update in this upgrade (current version != target version) | ||
Then for <field_name> field the diff algorithm should output a custom merged version with a solvable conflict | ||
And arrays should be deduplicated before comparison | ||
And arrays should be compared insensitive of case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We ended up changing the logic for this, as explained in this thread. The diff should be case sensitive.
I would replace the last example for one that uses index patterns. In that case, it makes much more clear why it should be case-sensitive, as explained in the threat I linked above. We could have something like:
| algorithm | field_name | base_version | current_version | target_version | merged_version |
| array of scalars | index | ["logs-*", "endgame-*", "endpoint-*"] | ["Logs-*", "endgame-*"] | ["logs-*", "endgame-*", "new-*"] | ["Logs-*", "endgame-*", "new-*"] |
| array of scalars | index | ["logs-*"] | ["logs-*", "Logs-*"] | ["logs-*", "new-*"] | ["logs-*", "Logs-*", "new-*"] |
In the first case, "Logs-" would be preserved since it was added by the user, and "logs-" was removed by the user, so it does not appear in the final version.
In the second example, both are preserved, since they do not represent the same value to the algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put it in the example, but index
won't technically be covered by this algorithm as it's under the grouped data source field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much more cleaner and complete now 👍
But one more thing pending to update: our algorithm for scalar values should be case-sensitive, not insensitive. I left a comment with a link to the discussion we had about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the changes! LGTM ✅
…lastic#186325) ## Summary Related ticket: elastic#180162 Adds test plan for diff algorithm for arrays of scalar values implemented here: elastic#186323 ### 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 0b405a0)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
… plan (#186325) (#187873) # Backport This will backport the following commits from `main` to `8.15`: - [[Security Solution] Array of scalar values diff algorithm test plan (#186325)](#186325) <!--- 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-07-09T14:33:04Z","message":"[Security Solution] Array of scalar values diff algorithm test plan (#186325)\n\n## Summary\r\n\r\nRelated ticket: https://github.com/elastic/kibana/issues/180162\r\n\r\nAdds test plan for diff algorithm for arrays of scalar values\r\nimplemented here: https://github.com/elastic/kibana/pull/186323\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":"0b405a0d656e496775a2c6efa1392ec3ed3fdd4f","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","test-plan","Team:Detections and Resp","Team: SecuritySolution","Team:Detection Rule Management","Feature:Prebuilt Detection Rules","v8.15.0","v8.16.0"],"title":"[Security Solution] Array of scalar values diff algorithm test plan","number":186325,"url":"https://github.com/elastic/kibana/pull/186325","mergeCommit":{"message":"[Security Solution] Array of scalar values diff algorithm test plan (#186325)\n\n## Summary\r\n\r\nRelated ticket: https://github.com/elastic/kibana/issues/180162\r\n\r\nAdds test plan for diff algorithm for arrays of scalar values\r\nimplemented here: https://github.com/elastic/kibana/pull/186323\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":"0b405a0d656e496775a2c6efa1392ec3ed3fdd4f"}},"sourceBranch":"main","suggestedTargetBranches":["8.15"],"targetPullRequestStates":[{"branch":"8.15","label":"v8.15.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/186325","number":186325,"mergeCommit":{"message":"[Security Solution] Array of scalar values diff algorithm test plan (#186325)\n\n## Summary\r\n\r\nRelated ticket: https://github.com/elastic/kibana/issues/180162\r\n\r\nAdds test plan for diff algorithm for arrays of scalar values\r\nimplemented here: https://github.com/elastic/kibana/pull/186323\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":"0b405a0d656e496775a2c6efa1392ec3ed3fdd4f"}}]}] BACKPORT--> Co-authored-by: Davis Plumlee <[email protected]>
…#187778) ## Summary Completes #180162 Switches fields to use the scalar array diff algorithms assigned to them in the [overarching ticket](#180162 (comment)) Adds integration tests in accordance to #186325 for the upgrade/_review API endpoint for the scalar array diff algorithm. ### 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)
Summary
Related ticket: #180162
Adds test plan for diff algorithm for arrays of scalar values implemented here: #186323
For maintainers