[Security Solution] [Detections] Write failing status when executionStatus is in error #79311

dhurley14 · 2020-10-02T16:12:23Z

Summary

We can now set an error status when an exception occurs outside of our rule executor. For example, when an alert fails to decrypt or some other issue occurs in task manager where the rule executor function fails to execute, we can query the executionStatus property on the rule set by #75553

With this addition, we can now alert users when their rules are failing due to errors thrown outside of our code like so:

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-10-05T17:51:31Z

Pinging @elastic/siem (Team:SIEM)

...lugins/security_solution/server/lib/detection_engine/routes/__mocks__/rule_status_service.ts

x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.ts

...lugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_status_route.ts

...ns/security_solution/server/lib/detection_engine/signals/rule_status_saved_objects_client.ts

…object inside find rules status route, updates find rules route to display error if executionStatus is in error, but not be in charge of writing the status. That job belongs to the find rules status route.

…les_status_route and adds a test for general error checking

…ter to append rule ids to the end of query, removes object.keys()

…the executionStatus property and merge that result with our stored failures

…cessary references to mock in tests

FrankHassanabad · 2020-10-07T00:35:52Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts

+        convertToSnakeCase<IRuleStatusAttributes>(errorItem.attributes)
+      ),
+    },
+  } as RuleStatusResponse;


nit: Don't think you need the as cast, it will validate fine without it.

rylnd

I had a few nits but just one issue with unhandled promise rejections. Note: I did not exercise the functionality here, this was simply code review. However, I've got this on my list and will be testing during feature freeze.

rylnd · 2020-10-06T23:37:16Z

...s/security_solution/server/lib/detection_engine/routes/rules/find_rules_status_route.test.ts

@@ -46,6 +48,33 @@ describe('find_statuses', () => {
        status_code: 500,
      });
    });
+
+    test('returns success if rule status client writes an error status', async () => {
+      // 0. task manager tried to run the rule but couldn't, so the alerting framework


zero-indexed comments, I like it

rylnd · 2020-10-06T23:41:35Z

...s/security_solution/server/lib/detection_engine/routes/rules/find_rules_status_route.test.ts

+    test('returns success if rule status client writes an error status', async () => {
+      // 0. task manager tried to run the rule but couldn't, so the alerting framework
+      // wrote an error to the executionStatus.
+      const failingExecutionRule = getResult();


nit: better to call out the mutation of failingExecutionRule as part of its assignment:

const failingExecutionRule = { ...getResult(), ...overrides };

to prevent it from being overlooked

rylnd · 2020-10-06T23:47:35Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/read_rules_route.ts

-            ruleActions,
-            ruleStatuses.saved_objects[0]
-          );
+          const currentStatus = ruleStatuses.saved_objects[0];


Suggested change

const currentStatus = ruleStatuses.saved_objects[0];

const [currentStatus] = ruleStatuses.saved_objects;

rylnd · 2020-10-07T00:10:54Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts

+  id: string,
+  failures: Array<SavedObjectsFindResult<IRuleStatusAttributes>>,
+  acc: RuleStatusResponse
+) => {


Nit: explicit return type here. This might also remove the need for the as RuleStatusResponse below

edit: still need the as below, but I think the issue that it's resolving is correct (convertToSnakeCase is too fuzzy).

rylnd · 2020-10-07T00:23:41Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts

+          },
+          ...accum,
+        };
+      }, {} as Record<string, SanitizedAlert>)


nit: you can also pass this as a type argument to reduce

rylnd · 2020-10-07T00:25:13Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts

+};
+
+export const getFailingRules = (ids: string[], alertsClient: AlertsClient) =>
+  Promise.all(


I think we need to handle the potential rejections/errors thrown by the alertsClient, here. Might be easier to convert to a try/catch/await.

FrankHassanabad · 2020-10-07T00:42:49Z

...ck/plugins/security_solution/server/lib/detection_engine/signals/rule_status_service.mock.ts

+
+export type RuleStatusServiceMock = jest.Mocked<RuleStatusService>;
+
+export const ruleStatusServiceFactoryMock = async ({


I don't think this file is used anymore? If not, you want to delete it? You can keep it if you think these mocks will be useful for someone else though. Just pointing it out.

FrankHassanabad

This is a pre-emptive approval if we aren't around and FF is today.

Tested it a lot and it is looking pretty good, some comments about code from others below feel free to address them as you have time or do follow ups.

Otherwise, so so much goodness here with the refactoring and ❤️ how we can get these API key errors and others bubbled up now with this PR. Appreciate it.

kibanamachine · 2020-10-07T04:49:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0105320

Metrics [docs]

✅ unchanged

History

💔 Build #80377 failed a330f2d
💔 Build #80346 failed 28be8db
💔 Build #80337 failed 8624cdb
💔 Build #80323 failed 9c66410
💔 Build #80288 failed ab6ee8a

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

…tatus is in error (#79311) (#79812) * Write failing status when executionStatus is in error * adds unit test for error handling if rule status service throws an error * adds success test for when executionStatus is failed * moves logic for writing executionStatus failure to rule status saved object inside find rules status route, updates find rules route to display error if executionStatus is in error, but not be in charge of writing the status. That job belongs to the find rules status route. * test if we are writing an error status when calls are made to find_rules_status_route and adds a test for general error checking * adds JSDocs description for rules status route, updates findRules filter to append rule ids to the end of query, removes object.keys() * don't write an error to our rule status in the route, only read from the executionStatus property and merge that result with our stored failures * fixes tests * move mock rule status service out of __mocks__ folder and remove unnecessary references to mock in tests * fix type error * updates json.gzip for cypress * PR feedback * fix timing issue with integration tests * removes unzipped data.json

elasticmachine · 2021-09-23T14:24:23Z

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

dhurley14 self-assigned this Oct 2, 2020

dhurley14 added release_note:enhancement review Team:SIEM v7.10.0 v8.0.0 Feature:Detection Rules Anything related to Security Solution's Detection Rules Team:Detections and Resp Security Detection Response Team labels Oct 5, 2020

dhurley14 marked this pull request as ready for review October 5, 2020 17:51

dhurley14 requested review from a team as code owners October 5, 2020 17:51

dhurley14 force-pushed the exception-status-rules-2 branch from 482fd74 to 348263c Compare October 5, 2020 19:02