[Alerting] Enable rule import/export and allow rule types to exclude themselves from export #102999

ymao1 · 2021-06-22T19:36:41Z

Summary

This PR removes the feature flag for enabling import/export of rules and takes advantage of the new isExportable SO API to exclude security rule types and stack monitoring rule types from being exported.

It adds an isExportable flag to the AlertType type so that rule types can indicate whether or not they want they want to be exported from the SO Management UI at registration time. While the rule will still show up in the SO Management UI, a message will be logged for non-exportable rule types and the export file will include a listing and count of rules that were excluded from the export.

To Verify

Create some test rules, including stack monitoring & security rules.
Initiate a rule export from the SO Management UI
Verify that stack monitoring and security rules are excluded from the export file but other rule types are included.

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

…ing/enable-rule-export-with-exclusions

elasticmachine · 2021-06-23T15:00:54Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

elasticmachine · 2021-06-23T15:25:41Z

Pinging @elastic/uptime (Team:uptime)

elasticmachine · 2021-06-23T15:25:41Z

Pinging @elastic/apm-ui (Team:apm)

FrankHassanabad · 2021-06-28T16:33:54Z

x-pack/plugins/security_solution/server/lib/detection_engine/reference_rules/ml.ts

@@ -57,6 +57,7 @@ export const mlAlertType = createSecurityMlRuleType({
    context: [{ name: 'server', description: 'the server' }],
  },
  minimumLicenseRequired: 'basic',
+  isExportable: false,


Thanks, I realize this is commented out for some reason , but this should be good for when it's not.

FrankHassanabad · 2021-06-28T16:55:46Z

Tested and the export button exists and the export shows a download but then opening it up I see this I think is expected behavior that we do exports but some are not exportable and then we track those.

{"excludedObjects":[{"id":"9f28f534-c54c-11eb-844b-6346fd2e707c","reason":"excluded","type":"alert"}],"excludedObjectsCount":1,"exportedCount":0,"missingRefCount":0,"missingReferences":[]}

However, the DELETE behavior actually throws up an error dialog which is different UI/UX. If this is expected that is ok, sounds like it is. But just mentioning the not-exportable vs. the not-deletable look like two different things we do within the UI/UX

FrankHassanabad · 2021-06-28T16:58:11Z

On Import I see this with those types of exports, but I think this is also expected behavior?

I didn't try to do mixed and don't know what the expected behavior of re-importing things that contain unexportables are but this is a FYI.

I did get this crash in my console:

server    log   [10:56:19.148] [debug][0][0][endpoint:user-artifact-packager:1][plugins][securitySolution] User manifest not available yet.
Unhandled Promise rejection detected:

TypeError: Cannot destructure property 'title' of 'obj.attributes' as it is undefined.
    at /Users/frankhassanabad/projects/kibana/src/core/server/saved_objects/import/lib/collect_saved_objects.ts:47:15
    at Transform.transform [as _transform] (/Users/frankhassanabad/projects/kibana/node_modules/packages/kbn-utils/src/streams/filter_stream.ts:15:33)
    at Transform._read (internal/streams/transform.js:205:10)
    at Transform._write (internal/streams/transform.js:193:12)
    at writeOrBuffer (internal/streams/writable.js:358:12)
    at Transform.Writable.write (internal/streams/writable.js:303:10)
    at Transform.ondata (internal/streams/readable.js:745:22)
    at Transform.emit (events.js:376:20)
    at addChunk (internal/streams/readable.js:309:12)
    at readableAddChunk (internal/streams/readable.js:284:9)
    at Transform.Readable.push (internal/streams/readable.js:223:10)
    at Transform.push (internal/streams/transform.js:166:32)
    at Transform.done (internal/streams/transform.js:101:10)
    at Transform.transform [as _transform] (/Users/frankhassanabad/projects/kibana/src/core/server/saved_objects/import/lib/create_limit_stream.ts:21:7)
    at Transform._read (internal/streams/transform.js:205:10)
    at Transform._write (internal/streams/transform.js:193:12)

Terminating process...

So this might be something missing with the re-import and this PR.

FrankHassanabad · 2021-06-28T17:09:17Z

Tested import/export within the security_solutions and we look good there so far 👍 that we can still import export. We also have e2e tests and Cypress tests so those would have caught it as well

ymao1 · 2021-06-28T17:25:15Z

@FrankHassanabad Thank you for the thorough review!

The DELETE behavior is the current expected in the Saved Objects Management UI because rules are considered a hidden type of saved object and DELETE and COPY TO is not supported on hidden saved object types.

It looks like you've uncovered a bug with importing an ndjson file that only contains unexportable objects. I've filed a bug with the core team here: #103517

When exporting and then importing a mixture of exportable and non-exportable rules, this error does not show up.

spong · 2021-06-28T17:35:07Z

Tested and the export button exists and the export shows a download but then opening it up I see this I think is expected behavior that we do exports but some are not exportable and then we track those.

If these assets aren't exportable why does the UI let the user think they are? Additionally, there are no errors displayed in the UI as a result of the export, so without opening an .ndjson file the user has no indication that the export of these assets was a failure.

Will defer to the platform UX folks for the path forward here, but without any indication to the user in the UI that these assets weren't exported or aren't exportable, it feels like users could go about their way thinking the export was successful until they go and try to re-import and see it fail, at which point it could be too late. cc @MikePaquette and @paulewing

ymao1 · 2021-06-28T17:41:58Z

If these assets aren't exportable why does the UI let the user think they are? Additionally, there are no errors displayed in the UI as a result of the export, so without opening an .ndjson file the user has no indication that the export of these assets was a failure.

@spong See the original discussion in this issue for context, specifically this comment and its replies. Agree that it is not ideal UX but we were trying to balance level of effort with utility. Since the long term goal is to align security rule import/export with the framework, we opted for now a medium LOE in order to make rule import/export through the framework available sooner.

ymao1 · 2021-06-28T17:48:26Z

Will defer to the platform UX folks for the path forward here, but without any indication to the user in the UI that these assets weren't exported or aren't exportable, it feels like users could go about their way thinking the export was successful until they go and try to re-import and see it fail, at which point it could be too late. cc @MikePaquette and @paulewing

@spong It is not strictly accurate that there's no indication in the UI. There is a toaster that pops up with the following message:

spong · 2021-06-28T18:00:53Z

Will defer to the platform UX folks for the path forward here, but without any indication to the user in the UI that these assets weren't exported or aren't exportable, it feels like users could go about their way thinking the export was successful until they go and try to re-import and see it fail, at which point it could be too late. cc @MikePaquette and @paulewing

@spong It is not strictly accurate that there's no indication in the UI. There is a toaster that pops up with the following message:

I had missed that toast message, my apologies @ymao1! This is better, but I still think a little misleading when exporting just un-exportable security rules as we're using a success toast when no items were successful. I know these are corner cases and don't think they should hold up this feature, but just wanted to call attention to them as there is potential for confusion to security users here. Thank you for linking the associated issue/conversation as well, will read up from there 🙂

…ing/enable-rule-export-with-exclusions

ymao1 · 2021-06-28T22:27:26Z

Issue with importing a file with no export objects is fixed in this commit: 4a8fc64 Thanks to @pgayvallet!

TinaHeiligers

Saved Objects changes LGTM!

kibanamachine · 2021-06-29T00:34:09Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`alerting`	206	208	+2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`apm`	38.4KB	38.4KB	+64.0B

Unknown metric groups

API count

id	before	after	diff
`alerting`	214	216	+2

History

💛 Build #134539 was flaky 21a31d8
💚 Build #133618 succeeded 2feca91
💚 Build #133544 succeeded ea4cbcf
💔 Build #133531 failed 8086719
💚 Build #133383 succeeded 26a945b

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

cc @ymao1

…themselves from export (elastic#102999) * Removing feature flag changes * Adding isExportable flag to rule type definition * Adding isExportable flag to rule type definition * Adding isExportable flag to rule type definition * Filtering rule on export by rule type isExportable flag * Fixing types * Adding docs * Fix condition when exportCount is 0 * Unit test for fix condition when exportCount is 0 Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-06-29T00:37:51Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…themselves from export (#102999) (#103598) * Removing feature flag changes * Adding isExportable flag to rule type definition * Adding isExportable flag to rule type definition * Adding isExportable flag to rule type definition * Filtering rule on export by rule type isExportable flag * Fixing types * Adding docs * Fix condition when exportCount is 0 * Unit test for fix condition when exportCount is 0 Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: ymao1 <[email protected]>

ymao1 added 7 commits June 22, 2021 14:19

Removing feature flag changes

811cfe3

Adding isExportable flag to rule type definition

e7a08e0

Adding isExportable flag to rule type definition

969dbdb

Adding isExportable flag to rule type definition

26a945b

Filtering rule on export by rule type isExportable flag

d1bc9bb

Merge branch 'master' of https://github.com/elastic/kibana into alert…

8086719

…ing/enable-rule-export-with-exclusions

Fixing types

ea4cbcf

ymao1 changed the title ~~Alerting/enable rule export with exclusions~~ [Alerting] Enable rule import/export and allow rule types to exclude themselves from export Jun 23, 2021

ymao1 self-assigned this Jun 23, 2021

ymao1 added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.14.0 v8.0.0 labels Jun 23, 2021

ymao1 marked this pull request as ready for review June 23, 2021 15:00

ymao1 requested review from a team as code owners June 23, 2021 15:00

ymao1 requested a review from a team June 23, 2021 15:00

ymao1 requested a review from a team as a code owner June 23, 2021 15:00

Adding docs

2feca91

botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jun 23, 2021

chrisronline self-requested a review June 23, 2021 16:00

weltenwort removed their request for review June 28, 2021 13:15

FrankHassanabad reviewed Jun 28, 2021

View reviewed changes

FrankHassanabad approved these changes Jun 28, 2021

View reviewed changes

ymao1 added 2 commits June 28, 2021 18:25

Fix condition when exportCount is 0

4a8fc64

Merge branch 'master' of https://github.com/elastic/kibana into alert…

0554a52

…ing/enable-rule-export-with-exclusions

ymao1 requested a review from a team as a code owner June 28, 2021 22:26

Unit test for fix condition when exportCount is 0

f69be9d

ymao1 requested a review from TinaHeiligers June 28, 2021 22:31

TinaHeiligers approved these changes Jun 28, 2021

View reviewed changes

TinaHeiligers mentioned this pull request Jun 28, 2021

Fix condition when exportCount is 0 #103573

Closed

2 tasks

ymao1 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 28, 2021

This was referenced Jun 28, 2021

Error when importing a file with file containing only excluded saved objects #103517

Closed

Updating the import saved objects file to include by value saved objects panels and drilldowns #102114

Merged

ymao1 merged commit c05588f into elastic:master Jun 29, 2021

kibanamachine mentioned this pull request Jun 29, 2021

[7.x] [Alerting] Enable rule import/export and allow rule types to exclude themselves from export (#102999) #103598

Merged

This was referenced Jun 29, 2021

[Alerting] Include stack monitoring rules in import/export #103599

Open

[Alerting] Include security rules in import/export #103600

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alerting] Enable rule import/export and allow rule types to exclude themselves from export #102999

[Alerting] Enable rule import/export and allow rule types to exclude themselves from export #102999

ymao1 commented Jun 22, 2021 •

edited

Loading

elasticmachine commented Jun 23, 2021

elasticmachine commented Jun 23, 2021

elasticmachine commented Jun 23, 2021

FrankHassanabad Jun 28, 2021

FrankHassanabad commented Jun 28, 2021

FrankHassanabad commented Jun 28, 2021 •

edited

Loading

FrankHassanabad commented Jun 28, 2021

ymao1 commented Jun 28, 2021

spong commented Jun 28, 2021

ymao1 commented Jun 28, 2021

ymao1 commented Jun 28, 2021

spong commented Jun 28, 2021

ymao1 commented Jun 28, 2021

TinaHeiligers left a comment

kibanamachine commented Jun 29, 2021

API count

kibanamachine commented Jun 29, 2021

[Alerting] Enable rule import/export and allow rule types to exclude themselves from export #102999

[Alerting] Enable rule import/export and allow rule types to exclude themselves from export #102999

Conversation

ymao1 commented Jun 22, 2021 • edited Loading

Summary

To Verify

Checklist

elasticmachine commented Jun 23, 2021

elasticmachine commented Jun 23, 2021

elasticmachine commented Jun 23, 2021

FrankHassanabad Jun 28, 2021

Choose a reason for hiding this comment

FrankHassanabad commented Jun 28, 2021

FrankHassanabad commented Jun 28, 2021 • edited Loading

FrankHassanabad commented Jun 28, 2021

ymao1 commented Jun 28, 2021

spong commented Jun 28, 2021

ymao1 commented Jun 28, 2021

ymao1 commented Jun 28, 2021

spong commented Jun 28, 2021

ymao1 commented Jun 28, 2021

TinaHeiligers left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 29, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Page load bundle

API count

History

kibanamachine commented Jun 29, 2021

💚 Backport successful

ymao1 commented Jun 22, 2021 •

edited

Loading

FrankHassanabad commented Jun 28, 2021 •

edited

Loading