Skip to content
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

[Alerting] Export rules and connectors #98802

Merged
merged 17 commits into from
May 5, 2021

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Apr 29, 2021

Resolves #94151

Summary

Adds ability to export rules and connectors from the Saved Objects Management UI.

  • Rules will be exported as disabled, with apiKey information cleared.
  • Connectors will be exported with isMissingSecrets: true unless
    • Index and server log connectors have no secrets, so isMissingSecrets will not be set to true
    • Email and webhook connectors where hasAuth: false no secrets, so isMissingSecrets will not be set to true
  • Auditing the export event
  • Added hidden saved object types that are importableAndExportable to the Saved Object Management feature privilege.

How Rules and Connectors show up on Saved Objects Management UI
Screen Shot 2021-04-29 at 6 45 52 PM

Checklist

Delete any items that are not applicable to this PR.

@ymao1 ymao1 self-assigned this Apr 29, 2021
@ymao1 ymao1 added Feature:Alerting release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.14.0 v8.0.0 labels Apr 29, 2021
@ymao1 ymao1 marked this pull request as ready for review April 29, 2021 22:49
@ymao1 ymao1 requested review from a team as code owners April 29, 2021 22:49
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM, a few remarks.

Comment on lines +132 to +139
const savedObjectImportableAndExportableHiddenTypes = registry
.getImportableAndExportableTypes()
.filter((t) => registry.isHidden(t.name))
.map((t) => t.name);

const savedObjectTypes = Array.from(
new Set([...savedObjectVisibleTypes, ...savedObjectImportableAndExportableHiddenTypes])
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.

Comment on lines +42 to +44
onExport<RawAction>(
context: SavedObjectsExportTransformContext,
objects: Array<SavedObject<RawAction>>
Copy link
Contributor

Choose a reason for hiding this comment

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

(Referring to the getTitle function on top, as comments are not allowed on non-diff lines...)

Ideally, we shouldn't need the object's type to be displayed in its associated title, as it introduces redundancy in some parts of the SOM UI.

Now, the main SOM table is only using an (EUI) icon to display the type associated with each object. Is there an icon associated with rules and connectors in the EUI library? If so, you could add them via the management.icon property of the types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgayvallet Discussed with @mdefazio and we currently don't have dedicated icons for rules or connectors. If we're using the default icon, should we leave Rules: and Connectors: in the title?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess so. This is low impact and can be changed later anyway, so I think it's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for being flexible. Our concern is associating icons with Rules and Connectors that may also be used elsewhere for different concepts. The viz type icons were created specifically for those types and aren't used for other things (or shouldn't be).

If we want an icon, we will just need to go the route of creating on specifically for this. However, this type column may quickly turn into a lot of different icons and likely need the title to be shown alongside them anyway.

So let's circle back to it in the future and determine if we want to only show the title.

@ymao1 ymao1 requested a review from gchaps April 30, 2021 23:21
docs/management/action-types.asciidoc Outdated Show resolved Hide resolved
docs/user/alerting/rule-management.asciidoc Outdated Show resolved Hide resolved
import { SavedObject } from 'kibana/server';
import { RawAction } from '../types';

const CONNECTORS_WITHOUT_SECRETS = ['.index', '.server-log'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe here we can replace the constants with the dynamic approach to check the action type schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YulNaumenko Great idea! I updated this in this commit 58dfb8a

I ran into the issue where the email and webhook secrets validation does not take into account whether hasAuth is set to false inside the connector parameters so in this case, if the secrets validation fails, I'm still checking for the existence of hasAuth. Do you think that works?

@ymao1 ymao1 requested a review from gchaps May 4, 2021 12:19
Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

@ymao1
Copy link
Contributor Author

ymao1 commented May 4, 2021

@elasticmachine merge upstream

@chrisronline chrisronline self-requested a review May 5, 2021 01:33
@chrisronline
Copy link
Contributor

Looking good!

One thing I noticed:

Screen Shot 2021-05-05 at 9 14 45 AM

Screen Shot 2021-05-05 at 9 15 18 AM

In this scenario, I don't have the right license level to use the connector. Did we build a specific experience to tell the user this at import time? I think what we have now is okay for now (and we could file an issue to improve it in the future) but I wasn't sure if it was supposed to show more

@chrisronline
Copy link
Contributor

Also, I have a connector that requires secrets, but I didn't export it. Instead, I just exported a server log connector and I see this:

{"attributes":{"actionTypeId":".server-log","config":{},"isMissingSecrets":true,"name":"Default"},"coreMigrationVersion":"8.0.0","id":"93c19030-ada4-11eb-8afc-57c69e570459","migrationVersion":{"action":"7.14.0"},"references":[],"type":"action","updated_at":"2021-05-05T13:19:53.025Z","version":"WzI1LDFd"}

@ymao1
Copy link
Contributor Author

ymao1 commented May 5, 2021

In this scenario, I don't have the right license level to use the connector. Did we build a specific experience to tell the user this at import time? I think what we have now is okay for now (and we could file an issue to improve it in the future) but I wasn't sure if it was supposed to show more

I added a comment to the import issue: #94152

@ymao1
Copy link
Contributor Author

ymao1 commented May 5, 2021

Also, I have a connector that requires secrets, but I didn't export it. Instead, I just exported a server log connector and I see this:

{"attributes":{"actionTypeId":".server-log","config":{},"isMissingSecrets":true,"name":"Default"},"coreMigrationVersion":"8.0.0","id":"93c19030-ada4-11eb-8afc-57c69e570459","migrationVersion":{"action":"7.14.0"},"references":[],"type":"action","updated_at":"2021-05-05T13:19:53.025Z","version":"WzI1LDFd"}

Should be fixed now! Unit tests actually caught that one too! 😅

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM! I'm not too familiar with the code yet so I just focused on the functionality. Looks great, nice work!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @ymao1

@ymao1 ymao1 added the auto-backport Deprecated - use backport:version if exact versions are needed label May 5, 2021
@ymao1 ymao1 merged commit 4ab86c7 into elastic:master May 5, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 5, 2021
* Adding importableAndExportable but hidden saved object types to saved object feature privilege

* Adding helper function for transforming rule for export. Added audit logging

* Adding helper function for transforming rule for export. Added audit logging

* Adding unit test for transforming rules for export

* Exporting connectors

* Removing auditing during export

* Adding import/export to docs

* PR fixes

* Using action type validation onExport

* Fixing logic for connectors with optional secrets

* Fixing logic for connectors with optional secrets

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request May 5, 2021
* Adding importableAndExportable but hidden saved object types to saved object feature privilege

* Adding helper function for transforming rule for export. Added audit logging

* Adding helper function for transforming rule for export. Added audit logging

* Adding unit test for transforming rules for export

* Exporting connectors

* Removing auditing during export

* Adding import/export to docs

* PR fixes

* Using action type validation onExport

* Fixing logic for connectors with optional secrets

* Fixing logic for connectors with optional secrets

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: ymao1 <ying.mao@elastic.co>
@ymao1 ymao1 deleted the alerting/export-rules-and-connectors branch May 19, 2021 13:50
WafaaNasr added a commit that referenced this pull request Feb 6, 2023
…148703)

## Summary

- [x] Addresses #118774 
- [x] Enable Security Rule to be **imported** even if one of its
connectors has a missing secret
- [x] Shows **Warning Callout** in the Import Modal when missing secrets
connector is imported.
- [x] Added Link `connectors` to the connectors page in the same tab, so
that the user can fix imported connectors.
- [x] Added `Overwrite existing connectors with conflicting action "id"`
option to the Import Modal

## Cases:

> **Export:**
> - Export Rule(s) with connectors through `Export All` or `Bulk
Actions`
> 
> **Import:**
>     - Import Rule with correct connectors data 
> - Import Rule with missing secrets' connectors by showing a warning
callout
> - Re-Import connectors even if they were stored when overwrite is true
> 
> **Error:**
> - Showing an error message when the user has a Read Actions permission
and stops the importing => ` You may not have actions privileges
required to import rules with actions ...`
> - Showing an error message when the user has an old imported rule
missing all the connectors data **OR** these connectors were not in the
user's env => `X connector is missing. Connector id missing is: X`
> - Showing an error if the new connectors defined in the exported file
are not corresponding to the actions array under the rules param => `X
connector is missing. Connector id missing is: X`
> - **Showing a ` conflict` error in case of existing connectors and
re-importing again with an `overwrite` false => this won't happen in
case of implementing the `Skipping action-connectors importing if all
connectors have been imported/created before`**
> 
> **Skip importing:**  
> - Skipping action-connectors importing if the `actions` array is
empty, even if the user has exported-connectors in the file
> - Skipping action-connectors importing if all connectors have been
imported/created before
> 



### Screenshots
> 
>  **1. Importing Connectors successfully**
> <img width="1219" alt="image"
src="https://user-images.githubusercontent.com/12671903/216049657-a313033b-e45e-4c99-b6ca-ed3070f15a97.png">
> 
>  **2. Importing Connectors with warnings**
<img width="1208" alt="image"
src="https://user-images.githubusercontent.com/12671903/216980057-b5cdfe38-da1b-479b-8cfd-81f16037ff1d.png">

**3.Connector Page**

<img width="1701" alt="image"
src="https://user-images.githubusercontent.com/12671903/216049911-da29abc8-e20c-49d2-a507-ab382372b4f6.png">


## New text: @nastasha-solomon

**1. Warning message**

 title => could be ` 1 connector imported` or `x connectors imported`
message => ` 1 connector has sensitive information that requires
updates. review in connectors` or `x connectors have sensitive
information that requires updates. review in connectors`

<img width="588" alt="image"
src="https://user-images.githubusercontent.com/12671903/216103805-9946b080-07d3-4e8b-93aa-b5e1dcaa415d.png">

**2. New `Overwrite` checkbox**
<img width="431" alt="image"
src="https://user-images.githubusercontent.com/12671903/216106354-3d435d64-0fa5-467b-90f1-effb2c0aef2a.png">


**3. Success Toast message**

<img width="434" alt="image"
src="https://user-images.githubusercontent.com/12671903/216104454-2d83744b-efbc-40c1-9e69-7e8b0670dd19.png">

**4. Error messages**
   a. Missing import action privileges
<img width="438" alt="image"
src="https://user-images.githubusercontent.com/12671903/216116350-f306d744-eef4-4064-b4f8-e794db4ad78e.png">

   b. Missing connectors  
<img width="353" alt="image"
src="https://user-images.githubusercontent.com/12671903/216104979-370f6826-8150-45d5-8724-6ca50f99ad71.png">
<img width="357" alt="image"
src="https://user-images.githubusercontent.com/12671903/216106067-e6132a93-d36e-4bdf-b1bf-e6ddd1cf8a4e.png">

 


- [x] References: Use **getImporter** and **getExporter** from Saved
Object [Connectors SO import/export
implementation](#98802) ,
[Kibana-Core
confirmation](https://elastic.slack.com/archives/C5TQ33ND8/p1673275186013589
)
- [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

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Alerting release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Alerting][Connectors] Export rules and connectors
9 participants