-
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] Implement per-field text-based diffs for the prebuilt rule upgrade flyout #166489
Comments
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
@dplumlee I updated the ticket description according to our recent discussions. cc @approksiu @nikitaindik @jpdjere |
As a follow-up to the discussion held in the meeting yesterday, I'm listing out the field groups we are displaying in the upgrade flyout so we can determine how to order them. Currently, there's no discernible order to the fields, but we talked about either matching the order to that of an existing pattern (e.g. the field order of the rule details flyout or the rule creation form) or an order of "importance" (e.g. query fields more important, description fields less important). @approksiu @ARWNightingale "Common" Fields
"Per rule type" fields(these field groupings will all be displayed in the same component for context)
|
Reviewing. |
## Summary Addresses #166489 Docs issue: elastic/security-docs#4783 Adds per-field diffs for the rule upgrade flyout ### Acceptance Criteria - [x] The tab with per-field diffs is hidden behind a new feature flag. When the flag is off, the tab does not appear in the flyout. The tab should work regardless of the value of `jsonPrebuiltRulesDiffingEnabled`. - [x] Per-field diffs are read-only components. We don't need to let the user "merge" differences using these components. - [x] Diffs for complex fields are rendered as JSON diffs using the same component used for rendering the JSON diff for the whole rule. This means this component should be abstracted away and should accept `unknown` values in props instead of `RuleResponse`. - [x] Diffs for related fields are grouped or rendered close to each other. For example: - [x] Index patterns + Data view id - [x] Custom query + Filters + Language + Saved query id - [x] The tab uses the response from the `upgrade/_review` API endpoint and doesn't need any other API calls to render itself. - [x] The tab renders itself under 150ms. ### Screenshots <img width="1587" alt="Screenshot 2024-02-07 at 1 36 34 AM" src="https://github.com/elastic/kibana/assets/56367316/85dce529-064e-4025-b82c-2e89f6ec800b"> <img width="994" alt="Screenshot 2024-02-07 at 1 36 52 AM" src="https://github.com/elastic/kibana/assets/56367316/c226973f-ad46-4565-90c0-437316b138b4"> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials ### 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) --------- Co-authored-by: jpdjere <[email protected]>
@approksiu #176767 here is the PR for the tour |
…176767) ## Summary Issue: #166489 Adds a tour and tooltips highlighting and describing the new diff features of the prebuilt rule update flyout. #### To test: Enable the `jsonPrebuiltRulesDiffingEnabled` and/or `perFieldPrebuiltRulesDiffingEnabled` feature flags and clear your browser of the local storage `securitySolution.rulesManagementPage.newFeaturesTour.v8.13` token. This should allow you to see the tour and both new tabs highlighted by tooltips as shown in the screenshots below. ### Screenshots **Upgrade tour** <img width="1406" alt="Screenshot 2024-02-13 at 2 14 07 PM" src="https://github.com/elastic/kibana/assets/56367316/20d3fd8e-fc39-4ae2-a627-272ae14e9aac"> **Diff tab tooltips** <img width="1176" alt="Screenshot 2024-02-13 at 2 14 22 PM" src="https://github.com/elastic/kibana/assets/56367316/8810c17e-4dfb-4758-b4c7-199a44531a4e"> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) ### 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 Addresses elastic#166489 Docs issue: elastic/security-docs#4783 Adds per-field diffs for the rule upgrade flyout ### Acceptance Criteria - [x] The tab with per-field diffs is hidden behind a new feature flag. When the flag is off, the tab does not appear in the flyout. The tab should work regardless of the value of `jsonPrebuiltRulesDiffingEnabled`. - [x] Per-field diffs are read-only components. We don't need to let the user "merge" differences using these components. - [x] Diffs for complex fields are rendered as JSON diffs using the same component used for rendering the JSON diff for the whole rule. This means this component should be abstracted away and should accept `unknown` values in props instead of `RuleResponse`. - [x] Diffs for related fields are grouped or rendered close to each other. For example: - [x] Index patterns + Data view id - [x] Custom query + Filters + Language + Saved query id - [x] The tab uses the response from the `upgrade/_review` API endpoint and doesn't need any other API calls to render itself. - [x] The tab renders itself under 150ms. ### Screenshots <img width="1587" alt="Screenshot 2024-02-07 at 1 36 34 AM" src="https://github.com/elastic/kibana/assets/56367316/85dce529-064e-4025-b82c-2e89f6ec800b"> <img width="994" alt="Screenshot 2024-02-07 at 1 36 52 AM" src="https://github.com/elastic/kibana/assets/56367316/c226973f-ad46-4565-90c0-437316b138b4"> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials ### 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) --------- Co-authored-by: jpdjere <[email protected]>
…lastic#176767) ## Summary Issue: elastic#166489 Adds a tour and tooltips highlighting and describing the new diff features of the prebuilt rule update flyout. #### To test: Enable the `jsonPrebuiltRulesDiffingEnabled` and/or `perFieldPrebuiltRulesDiffingEnabled` feature flags and clear your browser of the local storage `securitySolution.rulesManagementPage.newFeaturesTour.v8.13` token. This should allow you to see the tour and both new tabs highlighted by tooltips as shown in the screenshots below. ### Screenshots **Upgrade tour** <img width="1406" alt="Screenshot 2024-02-13 at 2 14 07 PM" src="https://github.com/elastic/kibana/assets/56367316/20d3fd8e-fc39-4ae2-a627-272ae14e9aac"> **Diff tab tooltips** <img width="1176" alt="Screenshot 2024-02-13 at 2 14 22 PM" src="https://github.com/elastic/kibana/assets/56367316/8810c17e-4dfb-4758-b4c7-199a44531a4e"> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) ### 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)
@vgomez-el we're hoping to get the remaining release progress steps checked off soon (ideally by end-of-week), do you have everything you need for exploratory testing on this? |
@dplumlee Just to clarify, will the feature be included in the next 8.13 BC2 planned for Feb 22nd? Or should I test the feature on the feature branch? |
@vgomez-el The feature is already available in The flag can be set in the kibana config this way: xpack.securitySolution.enableExperimental: [
'perFieldPrebuiltRulesDiffingEnabled'
] |
@banderror @dplumlee I have exploratory tested the feature activating the feature flag in a cloud environment after upgrading a 8.9.2 instance to 8.13 so I have several prebuilt rules to work with. |
## Summary Addresses test coverage acceptance criteria for #166489 Adds test coverage in accordance to the recently merged [test plan](#176474) [Flaky test runner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)
## Summary Addresses test coverage acceptance criteria for elastic#166489 Adds test coverage in accordance to the recently merged [test plan](elastic#176474) [Flaky test runner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279) (cherry picked from commit 3c34b53)
…177645) # Backport This will backport the following commits from `main` to `8.13`: - [[Security Solution] Per-field diffs test coverage (#177399)](#177399) <!--- 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-02-22T19:57:41Z","message":"[Security Solution] Per-field diffs test coverage (#177399)\n\n## Summary\r\n\r\nAddresses test coverage acceptance criteria for\r\nhttps://github.com//issues/166489\r\n\r\nAdds test coverage in accordance to the recently merged [test\r\nplan](https://github.com/elastic/kibana/pull/176474)\r\n\r\n[Flaky test\r\nrunner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)","sha":"3c34b535ceac5a5c869719998d9e03a0d44ce21a","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","test-coverage","Team:Detections and Resp","Team: SecuritySolution","Team:Detection Rule Management","Feature:Prebuilt Detection Rules","v8.13.0","v8.14.0"],"title":"[Security Solution] Per-field diffs test coverage","number":177399,"url":"https://github.com/elastic/kibana/pull/177399","mergeCommit":{"message":"[Security Solution] Per-field diffs test coverage (#177399)\n\n## Summary\r\n\r\nAddresses test coverage acceptance criteria for\r\nhttps://github.com//issues/166489\r\n\r\nAdds test coverage in accordance to the recently merged [test\r\nplan](https://github.com/elastic/kibana/pull/176474)\r\n\r\n[Flaky test\r\nrunner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)","sha":"3c34b535ceac5a5c869719998d9e03a0d44ce21a"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","branchLabelMappingKey":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/177399","number":177399,"mergeCommit":{"message":"[Security Solution] Per-field diffs test coverage (#177399)\n\n## Summary\r\n\r\nAddresses test coverage acceptance criteria for\r\nhttps://github.com//issues/166489\r\n\r\nAdds test coverage in accordance to the recently merged [test\r\nplan](https://github.com/elastic/kibana/pull/176474)\r\n\r\n[Flaky test\r\nrunner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)","sha":"3c34b535ceac5a5c869719998d9e03a0d44ce21a"}}]}] BACKPORT--> Co-authored-by: Davis Plumlee <[email protected]>
@ARWNightingale @dplumlee How's it going with acceptance testing? |
@banderror all looks good and everything as expected. |
**Addresses:** #166489 ## Summary Turns on the `perFieldPrebuiltRulesDiffingEnabled` feature flag by default. This will enable the `Updates` tab containing per-field rule diffs in the rule upgrade flyout. The feature will be enabled in `8.13.0` and Serverless. See more info in the related ticket. ### Checklist - [ ] [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
**Addresses:** elastic#166489 ## Summary Turns on the `perFieldPrebuiltRulesDiffingEnabled` feature flag by default. This will enable the `Updates` tab containing per-field rule diffs in the rule upgrade flyout. The feature will be enabled in `8.13.0` and Serverless. See more info in the related ticket. ### Checklist - [ ] [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 (cherry picked from commit 61b7f67) # Conflicts: # x-pack/plugins/security_solution/common/experimental_features.ts
…177708) # Backport This will backport the following commits from `main` to `8.13`: - [[Security Solution] Enable per field diffs feature (#177495)](#177495) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Georgii Gorbachev","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-02-23T12:28:22Z","message":"[Security Solution] Enable per field diffs feature (#177495)\n\n**Addresses:** https://github.com/elastic/kibana/issues/166489\r\n\r\n## Summary\r\n\r\nTurns on the `perFieldPrebuiltRulesDiffingEnabled` feature flag by\r\ndefault.\r\n\r\nThis will enable the `Updates` tab containing per-field rule diffs in\r\nthe rule upgrade flyout. The feature will be enabled in `8.13.0` and\r\nServerless. See more info in the related ticket.\r\n\r\n\r\n### Checklist\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","sha":"61b7f67bda066d57455f5ca2ed69fd4e61657ffb","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Detections and Resp","Team: SecuritySolution","Team:Detection Rule Management","Feature:Prebuilt Detection Rules","ci:cloud-deploy","ci:project-deploy-security","v8.13.0","v8.14.0"],"number":177495,"url":"https://github.com/elastic/kibana/pull/177495","mergeCommit":{"message":"[Security Solution] Enable per field diffs feature (#177495)\n\n**Addresses:** https://github.com/elastic/kibana/issues/166489\r\n\r\n## Summary\r\n\r\nTurns on the `perFieldPrebuiltRulesDiffingEnabled` feature flag by\r\ndefault.\r\n\r\nThis will enable the `Updates` tab containing per-field rule diffs in\r\nthe rule upgrade flyout. The feature will be enabled in `8.13.0` and\r\nServerless. See more info in the related ticket.\r\n\r\n\r\n### Checklist\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","sha":"61b7f67bda066d57455f5ca2ed69fd4e61657ffb"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","labelRegex":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/177495","number":177495,"mergeCommit":{"message":"[Security Solution] Enable per field diffs feature (#177495)\n\n**Addresses:** https://github.com/elastic/kibana/issues/166489\r\n\r\n## Summary\r\n\r\nTurns on the `perFieldPrebuiltRulesDiffingEnabled` feature flag by\r\ndefault.\r\n\r\nThis will enable the `Updates` tab containing per-field rule diffs in\r\nthe rule upgrade flyout. The feature will be enabled in `8.13.0` and\r\nServerless. See more info in the related ticket.\r\n\r\n\r\n### Checklist\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","sha":"61b7f67bda066d57455f5ca2ed69fd4e61657ffb"}}]}] BACKPORT-->
## Summary Addresses elastic#166489 Docs issue: elastic/security-docs#4783 Adds per-field diffs for the rule upgrade flyout ### Acceptance Criteria - [x] The tab with per-field diffs is hidden behind a new feature flag. When the flag is off, the tab does not appear in the flyout. The tab should work regardless of the value of `jsonPrebuiltRulesDiffingEnabled`. - [x] Per-field diffs are read-only components. We don't need to let the user "merge" differences using these components. - [x] Diffs for complex fields are rendered as JSON diffs using the same component used for rendering the JSON diff for the whole rule. This means this component should be abstracted away and should accept `unknown` values in props instead of `RuleResponse`. - [x] Diffs for related fields are grouped or rendered close to each other. For example: - [x] Index patterns + Data view id - [x] Custom query + Filters + Language + Saved query id - [x] The tab uses the response from the `upgrade/_review` API endpoint and doesn't need any other API calls to render itself. - [x] The tab renders itself under 150ms. ### Screenshots <img width="1587" alt="Screenshot 2024-02-07 at 1 36 34 AM" src="https://github.com/elastic/kibana/assets/56367316/85dce529-064e-4025-b82c-2e89f6ec800b"> <img width="994" alt="Screenshot 2024-02-07 at 1 36 52 AM" src="https://github.com/elastic/kibana/assets/56367316/c226973f-ad46-4565-90c0-437316b138b4"> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials ### 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) --------- Co-authored-by: jpdjere <[email protected]>
…lastic#176767) ## Summary Issue: elastic#166489 Adds a tour and tooltips highlighting and describing the new diff features of the prebuilt rule update flyout. #### To test: Enable the `jsonPrebuiltRulesDiffingEnabled` and/or `perFieldPrebuiltRulesDiffingEnabled` feature flags and clear your browser of the local storage `securitySolution.rulesManagementPage.newFeaturesTour.v8.13` token. This should allow you to see the tour and both new tabs highlighted by tooltips as shown in the screenshots below. ### Screenshots **Upgrade tour** <img width="1406" alt="Screenshot 2024-02-13 at 2 14 07 PM" src="https://github.com/elastic/kibana/assets/56367316/20d3fd8e-fc39-4ae2-a627-272ae14e9aac"> **Diff tab tooltips** <img width="1176" alt="Screenshot 2024-02-13 at 2 14 22 PM" src="https://github.com/elastic/kibana/assets/56367316/8810c17e-4dfb-4758-b4c7-199a44531a4e"> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) ### 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 Addresses test coverage acceptance criteria for elastic#166489 Adds test coverage in accordance to the recently merged [test plan](elastic#176474) [Flaky test runner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)
**Addresses:** elastic#166489 ## Summary Turns on the `perFieldPrebuiltRulesDiffingEnabled` feature flag by default. This will enable the `Updates` tab containing per-field rule diffs in the rule upgrade flyout. The feature will be enabled in `8.13.0` and Serverless. See more info in the related ticket. ### Checklist - [ ] [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
The feature went live today 🎉 |
Epics: https://github.com/elastic/security-team/issues/1974 (internal), #174167
Related to: #169160
Summary
The initial plan was to build a new tab in the prebuilt rule upgrade flyout where we'd show per-field diffs for rule fields using custom diff components. These custom components were designed within #166159. This ticket was originally for implementing these custom components.
To move faster and implement this tab as soon as possible, the team has decided to first implement it using text-based diffs based on the same mechanism used for showing JSON diffs (#169160).
Acceptance criteria
jsonPrebuiltRulesDiffingEnabled
.unknown
values in props instead ofRuleResponse
.upgrade/_review
API endpoint and doesn't need any other API calls to render itself.Release progress
Planned release date in Serverless: March 4th.
Planned release date in ESS: March 19th (
v8.13.0
).Designs
Latest Figma Designs
The text was updated successfully, but these errors were encountered: