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

[Security Solution] [Detections] Create a 'partial failure' status for rules #84293

Merged
merged 2 commits into from
Dec 1, 2020
Merged

[Security Solution] [Detections] Create a 'partial failure' status for rules #84293

merged 2 commits into from
Dec 1, 2020

Conversation

dhurley14
Copy link
Contributor

@dhurley14 dhurley14 commented Nov 24, 2020 •
edited
Loading

Summary

With the allowance of querying indices with different timestamps (PR, issue) we need a way to tell the customer when a rule is unable to query neither of the two timestamp fields provided (timestamp override and / or @timestamp field). I have updated the rule status functions to write a "partial failure" status to indicate when we were able to successfully query some of the index patterns provided to the rule, but were unable to query others.

This will also be helpful when we add checks for read privileges for the rule itself (#83134)

partial_failure_ui

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dhurley14 dhurley14 mentioned this pull request Nov 24, 2020
Closed
4 tasks
@dhurley14 dhurley14 changed the title create a 'partial failure' status for rules [Security Solution] [Detections] Create a 'partial failure' status for rules Nov 24, 2020
@dhurley14 dhurley14 force-pushed the warning-msg-rule-details branch from 07f1af1 to 3e807e7 Compare November 25, 2020 14:49
@dhurley14 dhurley14 self-assigned this Nov 25, 2020
@dhurley14 dhurley14 added Feature:Detection Rules Security Solution rules and Detection Engine release_note:enhancement review Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team v7.11.0 v8.0.0 Feature:Detection Alerts Security Solution Detection Alerts Feature labels Nov 25, 2020
@dhurley14 dhurley14 marked this pull request as ready for review November 25, 2020 23:23
@dhurley14 dhurley14 requested review from a team as code owners November 25, 2020 23:23
@spong spong requested a review from a team November 25, 2020 23:56
@dhurley14
Copy link
Contributor Author

@elasticmachine merge upstream

peluja1012
peluja1012 reviewed Nov 30, 2020
View reviewed changes
x-pack/plugins/security_solution/server/lib/detection_engine/signals/rule_status_service.ts
@@ -93,6 +101,18 @@ export const ruleStatusServiceFactory = async ({
});
},

partialFailure: async (message, attributes) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to use this function in signal_rule_alert_type.ts or is it already being used somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, there's no place in the code where this function is being used. When this gets merged I will be incorporating it into the multiple timestamps pr #83134 I think by returning an error type instead of relying on the result.success boolean to write an error / success status.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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 8.0MB 8.0MB +1.0KB

Page load bundle

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

id before after diff
securitySolution 173.1KB 173.1KB +23.0B

History

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

peluja1012
peluja1012 approved these changes Dec 1, 2020
View reviewed changes
Copy link
Contributor

@peluja1012 peluja1012 left a comment

Choose a reason for hiding this comment

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

LGTM. The UI elements are displayed as expected. As I reviewed the PR I wondered if we should also store "partial failures" as part of the "Failure History"? I wonder if users would be interested in seeing a log of partial failures alongside actual failures. This is more of a Product question and it won't block this PR but it's something to think about.

@dhurley14 dhurley14 merged commit 6e80d9f into elastic:master Dec 1, 2020
@dhurley14 dhurley14 deleted the warning-msg-rule-details branch December 1, 2020 15:15
@dhurley14 dhurley14 mentioned this pull request Dec 1, 2020
Merged
dhurley14 added a commit to dhurley14/kibana that referenced this pull request Dec 1, 2020
@dhurley14 @kibanamachine
[Security Solution] [Detections] Create a 'partial failure' status fo…
9525cd8 
…r rules (elastic#84293)

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 1, 2020
@gmmorris
Merge branch 'master' into alerts/custom-recovery-action-group
3421a0f 
* master: (63 commits)
  Revert the Revert of "[Alerting] renames Resolved action group to Recovered (elastic#84123)"  (elastic#84662)
  declare kbn/monaco dependency on kbn/i18n explicitly (elastic#84660)
  Remove unscripted fields from sample data index-pattern saved objects (elastic#84659)
  [ML] Fix unnecessary trigger of wildcard field type search for ML plugin routes. (elastic#84605)
  Update create.asciidoc (elastic#84046)
  [Security Solution][Detections] Fix labels and issue with mandatory fields (elastic#84525)
  Fix flaky test suite (elastic#84602)
  [Security Solution] [Detections] Create a 'partial failure' status for rules (elastic#84293)
  Revert "[Alerting] renames Resolved action group to Recovered (elastic#84123)"
  Update code-comments describing babel plugins (elastic#84622)
  [Security Solution] [Cases] Cypress for case connector selector options (elastic#80745)
  [Discover] Unskip doc table tests (elastic#84564)
  [Lens] (Accessibility) Improve landmarks in Lens (elastic#84511)
  [Lens] (Accessibility) Focus mistakenly stops on righthand form (elastic#84519)
  Return early when parallel install process detected (elastic#84190)
  [Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides (elastic#83723)
  [Security Solution][Detections] Fix grammatical error in validation message for threshold field in "Create new rule" -> "Define rule" (elastic#84490)
  [Fleet] Update agent details page  (elastic#84434)
  adding documentation of use of NODE_EXTRA_CA_CERTS env var (elastic#84578)
  [Search] Integrate "Send to background" UI with session service (elastic#83073)
  ...
dhurley14 added a commit that referenced this pull request Dec 1, 2020
@dhurley14 @kibanamachine
[7.x] [Security Solution] [Detections] Create a 'partial failure' sta…
3ddbc57 
…tus for rules (#84293) (#84658)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peluja1012 peluja1012 peluja1012 approved these changes

Assignees

@dhurley14 dhurley14

Labels
Feature:Detection Alerts Security Solution Detection Alerts Feature Feature:Detection Rules Security Solution rules and Detection Engine release_note:enhancement review Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhurley14 @kibanamachine @peluja1012