[Security Solution][Data Views] - Add alerts on alerts data view warning #138186

yctercero · 2022-08-04T22:56:38Z

Summary

The default security data view includes the alerts index. This means that a rule that uses this data view can result in alerts on alerts. At first glance, it seems the default data view is equivalent to the default index patterns we normally display on rule creation, but it is not in that it includes the alerts index.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

… view includes alerts index

elasticmachine · 2022-08-04T22:56:41Z

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

dhurley14

LGTM!

...ck/plugins/security_solution/public/detections/components/rules/data_view_selector/index.tsx

dhurley14 · 2022-08-08T14:43:41Z

@elasticmachine merge upstream

joepeeples

Thanks for looping in the docs team! I added a couple of suggestions for the callout message.

...ins/security_solution/public/detections/components/rules/data_view_selector/translations.tsx

… data_views_warning

...ins/security_solution/public/detections/components/rules/data_view_selector/translations.tsx

yctercero · 2022-08-08T19:59:49Z

@elasticmachine merge upstream

yctercero · 2022-08-09T01:44:14Z

@elasticmachine merge upstream

kibana-ci · 2022-08-09T02:47:47Z

💚 Build Succeeded

Buildkite Build
Commit: a748eb3

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	5.6MB	5.6MB	+755.0B

History

💛 Build #63746 was flaky a44c334
💚 Build #63696 succeeded 8af3989
💚 Build #63636 succeeded 8c909a5
💚 Build #63360 succeeded 4e6000d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @yctercero

banderror

Just some nits @yctercero, thanks for the enhancement!

Thinking about this from the usability side, why do we allow the user to select this data view if likely in most situations this will lead to users shooting themselves in the foot? Some thoughts off the top of my head:

this default data view seems to be suitable for "Explore" pages like Hosts, but is dangerous for rules to be used => so we might want to filter it out when showing the data view selector
some kind of "default data view for security rules" could help - this one would exclude the alerts index and include the source event indices
"alerts on alerts" could be an explicit "data source" in the UI in addition to index patterns and data views

cc @jethr0null

banderror · 2022-08-09T13:39:51Z

...ck/plugins/security_solution/public/detections/components/rules/data_view_selector/index.tsx

+      const dataViewsTitle = kibanaDataViews[dataViewId].title;
+      const dataViewsId = kibanaDataViews[dataViewId].id;
+
+      setShowDataViewAlertsOnAlertsWarning(dataViewsId === 'security-solution-default');


Nit: is there a subtle reason why this is implemented via useEffect when showDataViewAlertsOnAlertsWarning is a state derivative from dataViewId? Seems like could be implemented via useMemo.

banderror · 2022-08-09T13:40:08Z

...ck/plugins/security_solution/public/detections/components/rules/data_view_selector/index.tsx

+      const dataViewsTitle = kibanaDataViews[dataViewId].title;
+      const dataViewsId = kibanaDataViews[dataViewId].id;
+
+      setShowDataViewAlertsOnAlertsWarning(dataViewsId === 'security-solution-default');


Is there a common constant for security-solution-default?

banderror · 2022-08-09T13:41:05Z

...ck/plugins/security_solution/public/detections/components/rules/data_view_selector/index.tsx

+      {showDataViewAlertsOnAlertsWarning && (
+        <>
+          <EuiCallOut
+            title={i18n.DDATA_VIEW_ALERTS_ON_ALERTS_WARNING_LABEL}


Nit: DDATA

…ing (#138186) ## Summary The default security data view includes the alerts index. This means that a rule that uses this data view can result in alerts on alerts. At first glance, it seems the default data view is equivalent to the default index patterns we normally display on rule creation, but it is not in that it includes the alerts index. (cherry picked from commit b406632)

kibanamachine · 2022-08-09T13:54:52Z

💚 All backports created successfully

Status	Branch	Result
✅	8.4

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

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

yctercero · 2022-08-09T14:00:43Z

Just some nits @yctercero, thanks for the enhancement!

Thinking about this from the usability side, why do we allow the user to select this data view if likely in most situations this will lead to users shooting themselves in the foot? Some thoughts off the top of my head:

this default data view seems to be suitable for "Explore" pages like Hosts, but is dangerous for rules to be used => so we might want to filter it out when showing the data view selector

some kind of "default data view for security rules" could help - this one would exclude the alerts index and include the source event indices

"alerts on alerts" could be an explicit "data source" in the UI in addition to index patterns and data views

cc @jethr0null

Thanks for the review! I'll follow up to address the nits. They may just not go in 8.4. Completely agree with your observations. We're currently re-examining the sourcerer architecture and one of my suggestions includes breaking the default data view differently - at the very least having it not include alerts and having the alerts be a data view all its own. Please feel free to add any thoughts you might have in that ticket as well.

cc @YulNaumenko

…ing (elastic#138186) (elastic#138397) ## Summary The default security data view includes the alerts index. This means that a rule that uses this data view can result in alerts on alerts. At first glance, it seems the default data view is equivalent to the default index patterns we normally display on rule creation, but it is not in that it includes the alerts index. (cherry picked from commit b406632) Co-authored-by: Yara Tercero <yctercero@users.noreply.github.com>

yiyangliu9286

LGTM!

add data view warning on rule creation letting user know default data…

4e6000d

… view includes alerts index

yctercero requested a review from a team August 4, 2022 22:56

yctercero requested a review from a team as a code owner August 4, 2022 22:56

yctercero self-assigned this Aug 4, 2022

yctercero requested a review from xcrzx August 4, 2022 22:56

yctercero requested a review from yiyangliu9286 August 4, 2022 22:59

banderror requested review from vitaliidm and removed request for xcrzx August 7, 2022 10:59

dhurley14 approved these changes Aug 8, 2022

View reviewed changes

...ck/plugins/security_solution/public/detections/components/rules/data_view_selector/index.tsx Outdated Show resolved Hide resolved

Merge branch 'main' into data_views_warning

8c909a5

joepeeples reviewed Aug 8, 2022

View reviewed changes

...ins/security_solution/public/detections/components/rules/data_view_selector/translations.tsx Outdated Show resolved Hide resolved

...ins/security_solution/public/detections/components/rules/data_view_selector/translations.tsx Outdated Show resolved Hide resolved

yctercero added 3 commits August 8, 2022 10:04

update text per docs team feedback

8fcf0af

Merge branch 'main' of github.com:elastic/kibana into data_views_warning

b016d7f

Merge branch 'data_views_warning' of github.com:yctercero/kibana into…

8af3989

… data_views_warning

yctercero mentioned this pull request Aug 8, 2022

[DOCS][Rules][Data Views] - Data views available in rule creation flow elastic/security-docs#1832

Closed

joepeeples reviewed Aug 8, 2022

View reviewed changes

...ins/security_solution/public/detections/components/rules/data_view_selector/translations.tsx Outdated Show resolved Hide resolved

update text per docs team feedback

167a1fe

joepeeples approved these changes Aug 8, 2022

View reviewed changes

Merge branch 'main' into data_views_warning

a44c334

banderror requested review from banderror and removed request for vitaliidm August 8, 2022 20:50

yctercero enabled auto-merge (squash) August 9, 2022 01:43

Merge branch 'main' into data_views_warning

a748eb3

banderror approved these changes Aug 9, 2022

View reviewed changes

yctercero merged commit b406632 into elastic:main Aug 9, 2022

kibanamachine mentioned this pull request Aug 9, 2022

[8.4] [Security Solution][Data Views] - Add alerts on alerts data view warning (#138186) #138397

Merged

yctercero deleted the data_views_warning branch August 9, 2022 14:00

yiyangliu9286 reviewed Aug 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Data Views] - Add alerts on alerts data view warning #138186

[Security Solution][Data Views] - Add alerts on alerts data view warning #138186

yctercero commented Aug 4, 2022 •

edited

Loading

elasticmachine commented Aug 4, 2022

dhurley14 left a comment

dhurley14 commented Aug 8, 2022

joepeeples left a comment

yctercero commented Aug 8, 2022

yctercero commented Aug 9, 2022

kibana-ci commented Aug 9, 2022

banderror left a comment

banderror Aug 9, 2022

banderror Aug 9, 2022

banderror Aug 9, 2022

kibanamachine commented Aug 9, 2022

yctercero commented Aug 9, 2022

yiyangliu9286 left a comment

[Security Solution][Data Views] - Add alerts on alerts data view warning #138186

[Security Solution][Data Views] - Add alerts on alerts data view warning #138186

Conversation

yctercero commented Aug 4, 2022 • edited Loading

Summary

Checklist

elasticmachine commented Aug 4, 2022

dhurley14 left a comment

Choose a reason for hiding this comment

dhurley14 commented Aug 8, 2022

joepeeples left a comment

Choose a reason for hiding this comment

yctercero commented Aug 8, 2022

yctercero commented Aug 9, 2022

kibana-ci commented Aug 9, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History

banderror left a comment

Choose a reason for hiding this comment

banderror Aug 9, 2022

Choose a reason for hiding this comment

banderror Aug 9, 2022

Choose a reason for hiding this comment

banderror Aug 9, 2022

Choose a reason for hiding this comment

kibanamachine commented Aug 9, 2022

💚 All backports created successfully

Questions ?

yctercero commented Aug 9, 2022

yiyangliu9286 left a comment

Choose a reason for hiding this comment

yctercero commented Aug 4, 2022 •

edited

Loading