Skip to content

Commit

Permalink
exposes additional errors when ES failures occur during the swap of o…
Browse files Browse the repository at this point in the history 
…ld, pre-8.0 action ids with the new migrated action SO ids. Ref: elastic#120975 (comment) for more information
  • Loading branch information
@dhurley14
dhurley14 committed Jan 14, 2022
1 parent c526ff9 commit 72c6180
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 30 deletions.
73 changes: 65 additions & 8 deletions x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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],
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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: [] },
],
Expand All @@ -808,6 +863,7 @@ describe.each([
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',
},
})
Expand Down Expand Up @@ -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: {
Expand Down
43 changes: 21 additions & 22 deletions x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
};

/**
Expand Down Expand Up @@ -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)) {
Expand All @@ -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();
};

/**
Expand Down

0 comments on commit 72c6180

Please sign in to comment.