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.

Sign up for GitHub

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

Jump to bottom

Ability to delete alerts even when AAD is out of sync #56543

Merged
merged 3 commits into from
Feb 3, 2020
Merged

Ability to delete alerts even when AAD is out of sync #56543

merged 3 commits into from
Feb 3, 2020

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Jan 31, 2020 •
edited
Loading

Solves the delete API for #56619.

In this PR, I'm allowing alerts to be deleted even when the AAD is out of sync. I'm also refactoring the delete unit tests of the alerts client.

@mikecote mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v7.6.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Jan 31, 2020
@mikecote mikecote self-assigned this Jan 31, 2020
@elasticmachine
Copy link
Contributor

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

This was referenced Feb 3, 2020
Merged
Closed
@mikecote mikecote removed the v7.6.0 label Feb 3, 2020
@mikecote mikecote marked this pull request as ready for review February 3, 2020 15:37
@mikecote mikecote added the review label Feb 3, 2020
@mikecote
Copy link
Contributor Author

mikecote commented Feb 3, 2020

@elasticmachine merge upstream

@mikecote mikecote mentioned this pull request Feb 3, 2020
Merged
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

peterschretlen
peterschretlen approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor

@peterschretlen peterschretlen left a comment

Choose a reason for hiding this comment

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

LGTM

YulNaumenko
YulNaumenko approved these changes Feb 3, 2020
View reviewed changes
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.

LGTM

@mikecote mikecote merged commit 24ed071 into elastic:master Feb 3, 2020
@mikecote mikecote mentioned this pull request Feb 3, 2020
Merged
mikecote added a commit to mikecote/kibana that referenced this pull request Feb 3, 2020
@mikecote @elasticmachine
Ability to delete alerts even when AAD is out of sync (elastic#56543)
68cb039 
* Ability to delete alerts even when AAD is bad

* Small code fixes

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@mikecote mikecote mentioned this pull request Feb 3, 2020
Merged
mikecote added a commit to mikecote/kibana that referenced this pull request Feb 3, 2020
@mikecote @elasticmachine
Ability to delete alerts even when AAD is out of sync (elastic#56543)
4f7f28d 
* Ability to delete alerts even when AAD is bad

* Small code fixes

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@peterschretlen
Copy link
Contributor

I validated this in SIEM:

  • set xpack.encryptedSavedObjects.encryptionKey in kibana.yml
  • load the pre-built rules, activate a few
  • shut down kibana, change xpack.encryptedSavedObjects.encryptionKey
  • restart Kibana. rules will start generating errors:
server    log   [16:34:50.467] [error][alerting][plugins] Executing Alert "067609e8-b5e9-4718-a3d1-d9cedb4b4a78" has resulted in Error: Unable to decrypt attribute "apiKey"
  • change the page size in SIEM rules to 300, and delete all the rules (you must also delete the deactivated ones). you'll see a bunch of delete errors in the log as expected, and rules will go away:
server    log   [17:05:54.376] [error][encryptedSavedObjects][plugins] Failed to decrypt "apiKey" attribute: Unsupported state or unable to authenticate data
server    log   [17:05:54.376] [error][alerting][plugins] delete(): Failed to load API key to invalidate on alert bf238e43-ab72-48ed-aa1b-79dc44c7d463: Unable to decrypt attribute "apiKey"
  • click the "load prebuilt detection rules" button and re-activate a few - the rules work again!

@peterschretlen
Copy link
Contributor

Also as a side effect, the API keys view is full of dangling API keys. You can search for them by alert ID though (which appears in the name), so it is at least possible to clean them up either in the UI or programmatically.

pmuellr
pmuellr reviewed Feb 4, 2020
View reviewed changes
x-pack/legacy/plugins/alerting/server/alerts_client.ts
RawAlert
>('alert', id, { namespace: this.namespace });
const [taskIdToRemove, apiKeyToInvalidate] = await Promise.all([
this.savedObjectsClient
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we should - in a future PR - try to fix this code so we only make one SO get call instead of two. I think, but am not positive, that getDecryptedAsInternalUser() returns the un-encrypted properties also returned from SOC.get(). Not worth figuring out for 7.6 tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, so instead of doing two calls in parallel, you only do SOC.get() if ever getDecryptedAsInternalUser() throws an error (in series)?

Copy link
Member

Choose a reason for hiding this comment

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

I'd have to look closer at the impact, but it just doesn't seem right that we're doing two SO lookups, back-to-back. Used to be serially, I think, now it's in parallel, which is better perf-wise for this API, but still hits ES twice, which seems unfortunate, and hopefully, not actually required. If that seems possible, let's open another issue to investigate later. It would be a performance improvement ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created #56777 to fix this.

pmuellr
pmuellr reviewed Feb 4, 2020
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM, but made note about the double-lookup we do with SO/ESO - it's even more obvious now! I think we should open an issue to fix that, later.

mikecote added a commit that referenced this pull request Feb 4, 2020
@mikecote @elasticmachine
Ability to delete alerts even when AAD is out of sync (#56543) (#56686)
c4c0e74 
* Ability to delete alerts even when AAD is bad

* Small code fixes

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 4, 2020
@gmmorris
Merge branch 'master' into alerting/get-alert-state
28e7c7e 
* master: (42 commits)
  Move kuery_autocomplete ⇒ NP (elastic#56607)
  [ML] Functional tests - stabilize job row and analytics result view assertions (elastic#56595)
  [Discover] Inline angular directives only used in this plugin (elastic#56119)
  [Discover] Migrate get_sort.js test from mocha to TypeScript (elastic#56011)
  [SIEM] Enable flow_target_select_connected unit tests (elastic#55618)
  Start consuming np logging config (elastic#56480)
  [SIEM] Add eslint-plugin-react-perf (elastic#55960)
  Mention changed SAML ACS endpoint URL in breaking changes doc. (elastic#56613)
  Add `getServerInfo` API to http setup contract (elastic#56636)
  Updates Monitoring alert Jest snapshots
  Kibana property config migrations (elastic#55937)
  Vislib replacement toggle (elastic#56439)
  [Uptime] Add unit tests for QueryContext time calculation (elastic#56671)
  [SIEM][Detection Engine] Critical blocker, fixes pre-packaged rule miscounts
  Upgrade EUI to v18.3.0 (elastic#56228)
  [Maps] Fix server log (elastic#56679)
  [SIEM] Fixes FTUE when APM node is present (elastic#56574)
  [Reporting/FieldFormats] expose `setFieldFormats` and call from ReportingPlugin.start (elastic#56563)
  Update EMS API urls for production (elastic#56657)
  Ability to delete alerts even when AAD is out of sync (elastic#56543)
  ...
@mikecote mikecote mentioned this pull request Feb 4, 2020
Closed
mikecote added a commit that referenced this pull request Feb 4, 2020
@mikecote @elasticmachine
Ability to delete alerts even when AAD is out of sync (#56543) (#56687)
6d76951 
* Ability to delete alerts even when AAD is bad

* Small code fixes

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@mikecote mikecote added the v7.6.0 label Feb 4, 2020
@mikecote mikecote mentioned this pull request Feb 6, 2020
Merged
@mikecote mikecote added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmuellr pmuellr pmuellr left review comments

@YulNaumenko YulNaumenko YulNaumenko approved these changes

@peterschretlen peterschretlen peterschretlen approved these changes

Assignees

@mikecote mikecote

Labels
Feature:Alerting release_note:fix review Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.6.0 v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mikecote @elasticmachine @kibanamachine @peterschretlen @pmuellr @YulNaumenko