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

[Security Solution] [Detections] Fixes validation on response from find status route #93684

Merged

Conversation

dhurley14
Copy link
Contributor

@dhurley14 dhurley14 commented Mar 4, 2021

Summary

  • fixes validation on response of find status route when rule has a partial failure status
  • Also replaces 'warning' string on status field of SO with 'partial failure' to maintain backwards compatibility.

We are storing the status saved object with a 'partial failure' status but displaying it as a 'warning' on the UI.

warning_ui

partial_failure_api_resopnse

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dhurley14 dhurley14 changed the title fix validation on response of find status route when rule has a parti… [DRAFT] [Security Solution] [Detections] Fixes validation on response from find status route Mar 4, 2021
@dhurley14 dhurley14 self-assigned this Mar 4, 2021
@dhurley14 dhurley14 added bug Fixes for quality problems that affect the customer experience review Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team v7.11.2 v7.12.0 v7.13.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 4, 2021
@dhurley14 dhurley14 marked this pull request as ready for review March 4, 2021 21:47
@dhurley14 dhurley14 requested a review from a team as a code owner March 4, 2021 21:47
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

…ain backwards compatibility from an API standpoint, also displays 'warning' on UI if a rule's status is partial failure
…e test to check for partial failure not warning
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

#91051 introduced substantial changes in the RuleStatusComponent (x-pack/plugins/security_solution/public/detections/components/rules/rule_status/index.tsx), while this PR is related to rule statuses too. Maybe it makes sense to backport #91051 to 7.11?

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Mostly optional comments, except backporting #91051 to 7.11. Feels like it might be important, but I'm not sure, some careful checking of git history is needed to make sure that this is indeed the only PR related to rule statuses that can be backported. If it's not the only one, this can be risky.

I'll start testing now.

@@ -328,7 +328,10 @@ const RuleDetailsPageComponent = () => {
</EuiFlexItem>
) : (
<>
<RuleStatus status={currentStatus?.status} statusDate={currentStatus?.status_date}>
<RuleStatus
status={currentStatus?.status === 'partial failure' ? 'warning' : currentStatus?.status}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: Probably this mapping is not needed, at least in the master branch. Can't be sure about 7.11 branch though, because RuleStatus component has a different implementation there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we will have merge conflicts in 7.11 but I'll just make the necessary changes there when this backports.

@banderror
Copy link
Contributor

Rule details page 🟢

Rule management table 🟢

Rule monitoring table 🔴

@peluja1012 peluja1012 changed the title [DRAFT] [Security Solution] [Detections] Fixes validation on response from find status route [Security Solution] [Detections] Fixes validation on response from find status route Mar 5, 2021
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.

Thanks for the fix here @dhurley14! LGTM

@dhurley14 dhurley14 added blocker auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 5, 2021
@dhurley14 dhurley14 enabled auto-merge (squash) March 5, 2021 22:07
@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 7.8MB 7.8MB +398.0B

Page load bundle

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

id before after diff
securitySolution 237.2KB 237.3KB +23.0B

History

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

cc @dhurley14

@dhurley14 dhurley14 merged commit fd1d796 into elastic:master Mar 5, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 5, 2021
…nd status route (elastic#93684)

* fix validation on response of find status route when rule has a partial failure status

* replaces warning in rule status service with partial failure to maintain backwards compatibility from an API standpoint, also displays 'warning' on UI if a rule's status is partial failure

* display partial failure as 'warning' on all rules table and update e2e test to check for partial failure not warning

* add util function, show 'warning' on monitoring table, fix e2e tests
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 5, 2021
…nd status route (elastic#93684)

* fix validation on response of find status route when rule has a partial failure status

* replaces warning in rule status service with partial failure to maintain backwards compatibility from an API standpoint, also displays 'warning' on UI if a rule's status is partial failure

* display partial failure as 'warning' on all rules table and update e2e test to check for partial failure not warning

* add util function, show 'warning' on monitoring table, fix e2e tests
@kibanamachine
Copy link
Contributor

💔 Backport failed

❌ 7.11: Commit could not be cherrypicked due to conflicts
7.12 / #93853
7.x / #93854

Successful backport PRs will be merged automatically after passing CI.

To backport manually, check out the target branch and run:
node scripts/backport --pr 93684

dhurley14 added a commit to dhurley14/kibana that referenced this pull request Mar 5, 2021
…nd status route (elastic#93684)

* fix validation on response of find status route when rule has a partial failure status

* replaces warning in rule status service with partial failure to maintain backwards compatibility from an API standpoint, also displays 'warning' on UI if a rule's status is partial failure

* display partial failure as 'warning' on all rules table and update e2e test to check for partial failure not warning

* add util function, show 'warning' on monitoring table, fix e2e tests
# Conflicts:
#	x-pack/plugins/security_solution/common/detection_engine/utils.ts
#	x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx
#	x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts
#	x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts
dhurley14 added a commit that referenced this pull request Mar 6, 2021
…nd status route (#93684) (#93857)

* fix validation on response of find status route when rule has a partial failure status

* replaces warning in rule status service with partial failure to maintain backwards compatibility from an API standpoint, also displays 'warning' on UI if a rule's status is partial failure

* display partial failure as 'warning' on all rules table and update e2e test to check for partial failure not warning

* add util function, show 'warning' on monitoring table, fix e2e tests
# Conflicts:
#	x-pack/plugins/security_solution/common/detection_engine/utils.ts
#	x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx
#	x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts
#	x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts
kibanamachine added a commit that referenced this pull request Mar 6, 2021
…nd status route (#93684) (#93853)

* fix validation on response of find status route when rule has a partial failure status

* replaces warning in rule status service with partial failure to maintain backwards compatibility from an API standpoint, also displays 'warning' on UI if a rule's status is partial failure

* display partial failure as 'warning' on all rules table and update e2e test to check for partial failure not warning

* add util function, show 'warning' on monitoring table, fix e2e tests

Co-authored-by: Devin W. Hurley <devin.hurley@elastic.co>
kibanamachine added a commit that referenced this pull request Mar 6, 2021
…nd status route (#93684) (#93854)

* fix validation on response of find status route when rule has a partial failure status

* replaces warning in rule status service with partial failure to maintain backwards compatibility from an API standpoint, also displays 'warning' on UI if a rule's status is partial failure

* display partial failure as 'warning' on all rules table and update e2e test to check for partial failure not warning

* add util function, show 'warning' on monitoring table, fix e2e tests

Co-authored-by: Devin W. Hurley <devin.hurley@elastic.co>
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 blocker bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes review Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.11.2 v7.12.0 v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants