Skip to content

[Security Solution] Fix DataSource payload creation during rule upgrade with MERGED pick_version#197262

Merged
jpdjere merged 8 commits intoelastic:mainfrom
jpdjere:fix-bug-merge-upgrade
Oct 23, 2024
Merged

[Security Solution] Fix DataSource payload creation during rule upgrade with MERGED pick_version#197262
jpdjere merged 8 commits intoelastic:mainfrom
jpdjere:fix-bug-merge-upgrade

Conversation

@jpdjere
Copy link
Copy Markdown
Contributor

@jpdjere jpdjere commented Oct 22, 2024

Summary

The PR #191439 enhanced the /upgrade/_perform API contract and functionality to allow the users of the endpoint to upgrade rules to their MERGED version.

However, a bug slipped in, where the two different types of DataSource (type: index_patterns or type: data_view_id) weren't properly handled and would cause, in some cases, a rule payload to be created having both an index and data_view field, causing upgrade to fail.

This PR fixes the issue by handling these two field in a specific way, checking what the DataSource diffable field's type is, and setting the other field to undefined.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@jpdjere jpdjere self-assigned this Oct 22, 2024
@jpdjere jpdjere added 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 area backport:prev-minor v9.0.0 v8.16.0 v8.17.0 labels Oct 22, 2024
@jpdjere jpdjere marked this pull request as ready for review October 22, 2024 19:18
@jpdjere jpdjere requested a review from a team as a code owner October 22, 2024 19:18
@jpdjere jpdjere requested a review from xcrzx October 22, 2024 19:18
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@elasticmachine
Copy link
Copy Markdown
Contributor

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

@elasticmachine
Copy link
Copy Markdown
Contributor

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

@jpdjere jpdjere changed the title [Security Solution] Fix DataSource payload creation during upgrade with MERGED pick_version [Security Solution] Fix DataSource payload creation during rule upgrade with MERGED pick_version Oct 22, 2024
@jpdjere jpdjere added the release_note:skip Skip the PR/issue when compiling release notes label Oct 22, 2024
Copy link
Copy Markdown
Contributor

@dplumlee dplumlee left a comment

Choose a reason for hiding this comment

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

LGTM @jpdjere! Just left one nit

Comment on lines +200 to +203
} else if (fieldName === 'data_view_id' && isDataSourceIndexPatterns(diffableFieldValue)) {
return { type: 'TRANSFORMED_FIELD', value: undefined };
} else if (fieldName === 'index' && isDataSourceDataView(diffableFieldValue)) {
return { type: 'TRANSFORMED_FIELD', value: undefined };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: could use a short comment as to why we do this similar to the one on line 198

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

cc @jpdjere

@banderror banderror added backport:version Backport to applied version labels and removed backport:prev-minor labels Oct 23, 2024
Comment on lines +259 to +260
targetObject['security-rule'].name = TARGET_NAME;
targetObject['security-rule'].tags = TARGET_TAGS;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are these fields relevant for the test?

Comment on lines +280 to +281
// Check that the updated rules has an `index` field which equals the output of the diff algorithm
// for the DataSource diffable field, and that the data_view_id is correspondingly set to undefined.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you expand on in which cases the data_view_id field can be set together with index? I'm not entirely sure I fully understand the nature of the bug.

@jpdjere jpdjere merged commit 9656621 into elastic:main Oct 23, 2024
@kibanamachine
Copy link
Copy Markdown
Contributor

Starting backport for target branches: 8.16, 8.x

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

@jpdjere
Copy link
Copy Markdown
Contributor Author

jpdjere commented Oct 23, 2024

@dplumlee @xcrzx Thanks for the quick review. I merged to get this safely into 8.16, but I'm going to address your comments in a less urgent follow-up PR.

Also, @xcrzx we can discuss the bug during today's session.

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 23, 2024
…rade with `MERGED` pick_version (elastic#197262)

## Summary

The PR elastic#191439 enhanced the
`/upgrade/_perform` API contract and functionality to allow the users of
the endpoint to upgrade rules to their `MERGED` version.

However, a bug slipped in, where the two different types of `DataSource`
(`type: index_patterns` or `type: data_view_id`) weren't properly
handled and would cause, in some cases, a rule payload to be created
having both an `index` and `data_view` field, causing upgrade to fail.

This PR fixes the issue by handling these two field in a specific way,
checking what the `DataSource` diffable field's type is, and setting the
other field to `undefined`.

### Checklist

Delete any items that are not applicable to this PR.

- [ ] [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
- [ ] [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#_add_your_labels)
- [ ] This will appear in the **Release Notes** and follow the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

(cherry picked from commit 9656621)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 23, 2024
…rade with `MERGED` pick_version (elastic#197262)

## Summary

The PR elastic#191439 enhanced the
`/upgrade/_perform` API contract and functionality to allow the users of
the endpoint to upgrade rules to their `MERGED` version.

However, a bug slipped in, where the two different types of `DataSource`
(`type: index_patterns` or `type: data_view_id`) weren't properly
handled and would cause, in some cases, a rule payload to be created
having both an `index` and `data_view` field, causing upgrade to fail.

This PR fixes the issue by handling these two field in a specific way,
checking what the `DataSource` diffable field's type is, and setting the
other field to `undefined`.

### Checklist

Delete any items that are not applicable to this PR.

- [ ] [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
- [ ] [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#_add_your_labels)
- [ ] This will appear in the **Release Notes** and follow the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

(cherry picked from commit 9656621)
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 All backports created successfully

Status Branch Result
8.16
8.x

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 23, 2024
… during rule upgrade with `MERGED` pick_version (#197262) (#197467)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Fix `DataSource` payload creation
during rule upgrade with `MERGED` pick_version
(#197262)](#197262)

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

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

<!--BACKPORT [{"author":{"name":"Juan Pablo
Djeredjian","email":"jpdjeredjian@gmail.com"},"sourceCommit":{"committedDate":"2024-10-23T14:44:13Z","message":"[Security
Solution] Fix `DataSource` payload creation during rule upgrade with
`MERGED` pick_version (#197262)\n\n## Summary\r\n\r\nThe PR
#191439 enhanced
the\r\n`/upgrade/_perform` API contract and functionality to allow the
users of\r\nthe endpoint to upgrade rules to their `MERGED`
version.\r\n\r\nHowever, a bug slipped in, where the two different types
of `DataSource`\r\n(`type: index_patterns` or `type: data_view_id`)
weren't properly\r\nhandled and would cause, in some cases, a rule
payload to be created\r\nhaving both an `index` and `data_view` field,
causing upgrade to fail.\r\n\r\nThis PR fixes the issue by handling
these two field in a specific way,\r\nchecking what the `DataSource`
diffable field's type is, and setting the\r\nother field to
`undefined`.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are
not applicable to this PR.\r\n\r\n- [ ] [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- [ ] [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\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#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"9656621fcc8f6f9a615b0a27d45db9722e047a10","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","v8.16.0","backport:version","v8.17.0"],"title":"[Security
Solution] Fix `DataSource` payload creation during rule upgrade with
`MERGED`
pick_version","number":197262,"url":"https://github.com/elastic/kibana/pull/197262","mergeCommit":{"message":"[Security
Solution] Fix `DataSource` payload creation during rule upgrade with
`MERGED` pick_version (#197262)\n\n## Summary\r\n\r\nThe PR
#191439 enhanced
the\r\n`/upgrade/_perform` API contract and functionality to allow the
users of\r\nthe endpoint to upgrade rules to their `MERGED`
version.\r\n\r\nHowever, a bug slipped in, where the two different types
of `DataSource`\r\n(`type: index_patterns` or `type: data_view_id`)
weren't properly\r\nhandled and would cause, in some cases, a rule
payload to be created\r\nhaving both an `index` and `data_view` field,
causing upgrade to fail.\r\n\r\nThis PR fixes the issue by handling
these two field in a specific way,\r\nchecking what the `DataSource`
diffable field's type is, and setting the\r\nother field to
`undefined`.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are
not applicable to this PR.\r\n\r\n- [ ] [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- [ ] [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\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#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"9656621fcc8f6f9a615b0a27d45db9722e047a10"}},"sourceBranch":"main","suggestedTargetBranches":["8.16","8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197262","number":197262,"mergeCommit":{"message":"[Security
Solution] Fix `DataSource` payload creation during rule upgrade with
`MERGED` pick_version (#197262)\n\n## Summary\r\n\r\nThe PR
#191439 enhanced
the\r\n`/upgrade/_perform` API contract and functionality to allow the
users of\r\nthe endpoint to upgrade rules to their `MERGED`
version.\r\n\r\nHowever, a bug slipped in, where the two different types
of `DataSource`\r\n(`type: index_patterns` or `type: data_view_id`)
weren't properly\r\nhandled and would cause, in some cases, a rule
payload to be created\r\nhaving both an `index` and `data_view` field,
causing upgrade to fail.\r\n\r\nThis PR fixes the issue by handling
these two field in a specific way,\r\nchecking what the `DataSource`
diffable field's type is, and setting the\r\nother field to
`undefined`.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are
not applicable to this PR.\r\n\r\n- [ ] [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- [ ] [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\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#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"9656621fcc8f6f9a615b0a27d45db9722e047a10"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Juan Pablo Djeredjian <jpdjeredjian@gmail.com>
kibanamachine added a commit that referenced this pull request Oct 23, 2024
…n during rule upgrade with &#x60;MERGED&#x60; pick_version (#197262) (#197466)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[Security Solution] Fix &#x60;DataSource&#x60; payload creation
during rule upgrade with &#x60;MERGED&#x60; pick_version
(#197262)](#197262)

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

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

<!--BACKPORT [{"author":{"name":"Juan Pablo
Djeredjian","email":"jpdjeredjian@gmail.com"},"sourceCommit":{"committedDate":"2024-10-23T14:44:13Z","message":"[Security
Solution] Fix `DataSource` payload creation during rule upgrade with
`MERGED` pick_version (#197262)\n\n## Summary\r\n\r\nThe PR
#191439 enhanced
the\r\n`/upgrade/_perform` API contract and functionality to allow the
users of\r\nthe endpoint to upgrade rules to their `MERGED`
version.\r\n\r\nHowever, a bug slipped in, where the two different types
of `DataSource`\r\n(`type: index_patterns` or `type: data_view_id`)
weren't properly\r\nhandled and would cause, in some cases, a rule
payload to be created\r\nhaving both an `index` and `data_view` field,
causing upgrade to fail.\r\n\r\nThis PR fixes the issue by handling
these two field in a specific way,\r\nchecking what the `DataSource`
diffable field's type is, and setting the\r\nother field to
`undefined`.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are
not applicable to this PR.\r\n\r\n- [ ] [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- [ ] [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\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#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"9656621fcc8f6f9a615b0a27d45db9722e047a10","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","v8.16.0","backport:version","v8.17.0"],"title":"[Security
Solution] Fix `DataSource` payload creation during rule upgrade with
`MERGED`
pick_version","number":197262,"url":"https://github.com/elastic/kibana/pull/197262","mergeCommit":{"message":"[Security
Solution] Fix `DataSource` payload creation during rule upgrade with
`MERGED` pick_version (#197262)\n\n## Summary\r\n\r\nThe PR
#191439 enhanced
the\r\n`/upgrade/_perform` API contract and functionality to allow the
users of\r\nthe endpoint to upgrade rules to their `MERGED`
version.\r\n\r\nHowever, a bug slipped in, where the two different types
of `DataSource`\r\n(`type: index_patterns` or `type: data_view_id`)
weren't properly\r\nhandled and would cause, in some cases, a rule
payload to be created\r\nhaving both an `index` and `data_view` field,
causing upgrade to fail.\r\n\r\nThis PR fixes the issue by handling
these two field in a specific way,\r\nchecking what the `DataSource`
diffable field's type is, and setting the\r\nother field to
`undefined`.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are
not applicable to this PR.\r\n\r\n- [ ] [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- [ ] [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\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#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"9656621fcc8f6f9a615b0a27d45db9722e047a10"}},"sourceBranch":"main","suggestedTargetBranches":["8.16","8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197262","number":197262,"mergeCommit":{"message":"[Security
Solution] Fix `DataSource` payload creation during rule upgrade with
`MERGED` pick_version (#197262)\n\n## Summary\r\n\r\nThe PR
#191439 enhanced
the\r\n`/upgrade/_perform` API contract and functionality to allow the
users of\r\nthe endpoint to upgrade rules to their `MERGED`
version.\r\n\r\nHowever, a bug slipped in, where the two different types
of `DataSource`\r\n(`type: index_patterns` or `type: data_view_id`)
weren't properly\r\nhandled and would cause, in some cases, a rule
payload to be created\r\nhaving both an `index` and `data_view` field,
causing upgrade to fail.\r\n\r\nThis PR fixes the issue by handling
these two field in a specific way,\r\nchecking what the `DataSource`
diffable field's type is, and setting the\r\nother field to
`undefined`.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are
not applicable to this PR.\r\n\r\n- [ ] [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- [ ] [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\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#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"9656621fcc8f6f9a615b0a27d45db9722e047a10"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Juan Pablo Djeredjian <jpdjeredjian@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area 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. v8.16.0 v8.17.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants