[Security Solution] [Platform] Return additional errors during import…

… rule + action migration process (#123088) (#123329) * exposes additional errors when ES failures occur during the swap of old, pre-8.0 action ids with the new migrated action SO ids. Ref: #120975 (comment) for more information * fix test error Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 467d111) Co-authored-by: Devin W. Hurley <[email protected]>
elastic · Jan 19, 2022 · 35aec57 · 35aec57
1 parent 3d11c35
commit 35aec57
Show file tree

Hide file tree

Showing 2 changed files with 88 additions and 32 deletions.
diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils.test.ts
@@ -26,7 +26,7 @@ import {
 import { getAlertMock } from '../__mocks__/request_responses';
 import { INTERNAL_IDENTIFIER } from '../../../../../common/constants';
 import { PartialFilter } from '../../types';
-import { BulkError } from '../utils';
+import { BulkError, createBulkErrorObject } from '../utils';
 import { getOutputRuleAlertForRest } from '../__mocks__/utils';
 import { PartialAlert } from '../../../../../../alerting/server';
 import { createRulesAndExceptionsStreamFromNdJson } from '../../rules/create_rules_stream_from_ndjson';
@@ -46,9 +46,24 @@ import { LegacyRulesActionsSavedObject } from '../../rule_actions/legacy_get_rul
 // eslint-disable-next-line no-restricted-imports
 import { LegacyRuleAlertAction } from '../../rule_actions/legacy_types';
 import { RuleExceptionsPromiseFromStreams } from './utils/import_rules_utils';
+import { partition } from 'lodash/fp';
 
 type PromiseFromStreams = ImportRulesSchemaDecoded | Error;
 
+const createMockImportRule = async (rule: ReturnType<typeof getCreateRulesSchemaMock>) => {
+  const ndJsonStream = new Readable({
+    read() {
+      this.push(`${JSON.stringify(rule)}\n`);
+      this.push(null);
+    },
+  });
+  const [{ rules }] = await createPromiseFromStreams<RuleExceptionsPromiseFromStreams[]>([
+    ndJsonStream,
+    ...createRulesAndExceptionsStreamFromNdJson(1000),
+  ]);
+  return rules;
+};
+
 describe.each([
   ['Legacy', false],
   ['RAC', true],
@@ -667,12 +682,10 @@ describe.each([
       soClient.find.mockClear();
     });
 
-    test('returns original action if Elasticsearch query fails', async () => {
-      clients.core.savedObjects
-        .getClient()
-        .find.mockRejectedValueOnce(new Error('failed to query'));
+    test('returns error if Elasticsearch query fails', async () => {
+      soClient.find.mockRejectedValue(new Error('failed to query'));
       const result = await swapActionIds(mockAction, soClient);
-      expect(result).toEqual(mockAction);
+      expect((result as Error).message).toEqual('failed to query');
     });
 
     test('returns original action if Elasticsearch query returns no hits', async () => {
@@ -781,16 +794,58 @@ describe.each([
       ]);
     });
 
+    test('returns import rules schemas + one migrated action + one error', async () => {
+      const rule: ReturnType<typeof getCreateRulesSchemaMock> = {
+        ...getCreateRulesSchemaMock('rule-1'),
+        actions: [mockAction, { ...mockAction, id: 'different-id' }],
+      };
+      const rules = await createMockImportRule(rule);
+      soClient.find.mockImplementationOnce(async () => ({
+        total: 0,
+        per_page: 0,
+        page: 1,
+        saved_objects: [
+          { score: 0, id: 'new-post-8.0-id', type: 'action', attributes: {}, references: [] },
+        ],
+      }));
+
+      soClient.find.mockRejectedValueOnce(new Error('failed to query'));
+
+      const res = await migrateLegacyActionsIds(rules, soClient);
+      expect(soClient.find.mock.calls).toHaveLength(2);
+      const [error, ruleRes] = partition<PromiseFromStreams, Error>(
+        (item): item is Error => item instanceof Error
+      )(res);
+
+      expect(ruleRes[0]).toEqual({
+        ...rules[0],
+        actions: [{ ...mockAction, id: 'new-post-8.0-id' }],
+      });
+      expect(error[0]).toEqual(
+        new Error(
+          JSON.stringify(
+            createBulkErrorObject({
+              ruleId: rule.rule_id,
+              statusCode: 409,
+              message: `${[new Error('failed to query')].map((e: Error) => e.message).join(',')}`,
+            })
+          )
+        )
+      );
+    });
+
     test('returns import rules schemas + migrated action resulting in error', async () => {
       const rule: ReturnType<typeof getCreateRulesSchemaMock> = {
         ...getCreateRulesSchemaMock('rule-1'),
+        // only passing in one action here
         actions: [mockAction],
       };
       soClient.find.mockImplementationOnce(async () => ({
         total: 0,
         per_page: 0,
         page: 1,
         saved_objects: [
+          // two actions are being returned, thus resulting in a conflict
           { score: 0, id: 'new-post-8.0-id', type: 'action', attributes: {}, references: [] },
           { score: 0, id: 'new-post-8.0-id-2', type: 'action', attributes: {}, references: [] },
         ],
@@ -801,13 +856,14 @@ describe.each([
         [rule],
         soClient
       );
-      expect(res[0] instanceof Error).toBeTruthy();
-      expect((res[0] as unknown as Error).message).toEqual(
+      expect(res[1] instanceof Error).toBeTruthy();
+      expect((res[1] as unknown as Error).message).toEqual(
         JSON.stringify({
           rule_id: 'rule-1',
           error: {
             status_code: 409,
             message:
+              // error message for when two or more action connectors are found for a single id
               'Found two action connectors with originId or _id: some-7.x-id The upload cannot be completed unless the _id or the originId of the action connector is changed. See https://www.elastic.co/guide/en/kibana/current/sharing-saved-objects.html for more details',
           },
         })
@@ -843,8 +899,9 @@ describe.each([
         soClient
       );
       expect(res[0]).toEqual({ ...rule, actions: [{ ...mockAction, id: 'new-post-8.0-id' }] });
-      expect(res[1] instanceof Error).toBeTruthy();
-      expect((res[1] as unknown as Error).message).toEqual(
+      expect(res[1]).toEqual({ ...rule, actions: [] });
+      expect(res[2] instanceof Error).toBeTruthy();
+      expect((res[2] as unknown as Error).message).toEqual(
         JSON.stringify({
           rule_id: 'rule-1',
           error: {

diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils.ts
@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import { countBy } from 'lodash/fp';
+import { countBy, partition } from 'lodash/fp';
 import uuid from 'uuid';
 import { Action } from '@kbn/securitysolution-io-ts-alerting-types';
 import { SavedObjectsClientContract } from 'kibana/server';
@@ -229,10 +229,10 @@ export const swapActionIds = async (
         `Found two action connectors with originId or _id: ${action.id} The upload cannot be completed unless the _id or the originId of the action connector is changed. See https://www.elastic.co/guide/en/kibana/current/sharing-saved-objects.html for more details`
       );
     }
-  } catch (exc) {
     return action;
+  } catch (exc) {
+    return exc;
   }
-  return action;
 };
 
 /**
@@ -271,7 +271,7 @@ export const migrateLegacyActionsIds = async (
 ): Promise<PromiseFromStreams[]> => {
   const isImportRule = (r: unknown): r is ImportRulesSchemaDecoded => !(r instanceof Error);
 
-  return pMap(
+  const toReturn = await pMap(
     rules,
     async (rule) => {
       if (isImportRule(rule)) {
@@ -284,33 +284,32 @@ export const migrateLegacyActionsIds = async (
         );
 
         // were there any errors discovered while trying to migrate and swap the action connector ids?
-        const actionMigrationErrors = newActions.filter(
-          (action): action is Error => action instanceof Error
-        );
-
-        const newlyMigratedActions: Action[] = newActions.filter(
-          (action): action is Action => !(action instanceof Error)
-        );
+        const [actionMigrationErrors, newlyMigratedActions] = partition<Action | Error, Error>(
+          (item): item is Error => item instanceof Error
+        )(newActions);
 
         if (actionMigrationErrors == null || actionMigrationErrors.length === 0) {
           return { ...rule, actions: newlyMigratedActions };
         }
-        // return an Error object with the rule_id and the error messages
-        // for the actions associated with that rule.
-        return new Error(
-          JSON.stringify(
-            createBulkErrorObject({
-              ruleId: rule.rule_id,
-              statusCode: 409,
-              message: `${actionMigrationErrors.map((error: Error) => error.message).join(',')}`,
-            })
-          )
-        );
+
+        return [
+          { ...rule, actions: newlyMigratedActions },
+          new Error(
+            JSON.stringify(
+              createBulkErrorObject({
+                ruleId: rule.rule_id,
+                statusCode: 409,
+                message: `${actionMigrationErrors.map((error: Error) => error.message).join(',')}`,
+              })
+            )
+          ),
+        ];
       }
       return rule;
     },
     { concurrency: MAX_CONCURRENT_SEARCHES }
   );
+  return toReturn.flat();
 };
 
 /**