Skip to content

Commit

Permalink
Prevent same rule_id from concurrent import
Browse files Browse the repository at this point in the history
  • Loading branch information
thejwuscript committed Apr 10, 2024
1 parent a6a6823 commit 3158cd6
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@ import {

import { createRules } from '../crud/create_rules';
import { updateRules } from '../crud/update_rules';
import { deleteRules } from '../crud/delete_rules';
import { importRules } from './import_rules_utils';

import type { SanitizedRule } from '@kbn/alerting-plugin/common';

jest.mock('../crud/create_rules');
jest.mock('../crud/update_rules');
jest.mock('../crud/delete_rules');

describe('importRules', () => {
const mlAuthz = {
Expand All @@ -39,6 +43,10 @@ describe('importRules', () => {
jest.clearAllMocks();
});

afterEach(() => {
(createRules as jest.Mock).mockReset();
});

it('returns rules response if no rules to import', async () => {
const result = await importRules({
ruleChunks: [],
Expand Down Expand Up @@ -74,6 +82,12 @@ describe('importRules', () => {
});

it('creates rule if no matching existing rule found', async () => {
(createRules as jest.Mock).mockResolvedValue(getRuleMock(getQueryRuleParams()));
clients.rulesClient.find
.mockResolvedValue(getEmptyFindResult())
.mockResolvedValueOnce(getEmptyFindResult())
.mockResolvedValueOnce(getFindResultWithSingleHit());

const result = await importRules({
ruleChunks: [[getImportRulesSchemaMock({ rule_id: 'rule-1' })]],
rulesResponseAcc: [],
Expand Down Expand Up @@ -250,4 +264,62 @@ describe('importRules', () => {
},
]);
});

describe('when multiple rules with the same rule_id are found right after rule creation', () => {
const mockRuleId = '2a0e0ddd-8fba-49e2-b1bf-ff90be3e6fe1';
const mockRuleOne = {
id: '97e61faf-dd3d-43bf-b1b5-0bf470423bb5',
} as SanitizedRule;
const mockRuleTwo = {
id: '3e592a38-5f88-43a9-b50e-b0236adecd67',
} as SanitizedRule;

beforeEach(() => {
clients.rulesClient.find.mockResolvedValue(
getFindResultWithMultiHits({ data: [mockRuleOne, mockRuleTwo], total: 2 })
);
});

it('keeps the rule that was created first', async () => {
(createRules as jest.Mock).mockResolvedValue(mockRuleOne);

const result = await importRules({
ruleChunks: [[getImportRulesSchemaMock({ rule_id: mockRuleId })]],
rulesResponseAcc: [],
mlAuthz,
overwriteRules: false,
rulesClient: context.alerting.getRulesClient(),
existingLists: {},
});

expect(result).toEqual([{ rule_id: mockRuleId, status_code: 200 }]);
expect(createRules).toHaveBeenCalled();
expect(deleteRules).not.toHaveBeenCalled();
});

it('deletes duplicate rules', async () => {
(createRules as jest.Mock).mockResolvedValue(mockRuleTwo);

const result = await importRules({
ruleChunks: [[getImportRulesSchemaMock({ rule_id: mockRuleId })]],
rulesResponseAcc: [],
mlAuthz,
overwriteRules: false,
rulesClient: context.alerting.getRulesClient(),
existingLists: {},
});

expect(result).toEqual([
{
rule_id: mockRuleId,
error: {
status_code: 409,
message: `rule_id: "${mockRuleId}" already exists`,
},
},
]);
expect(createRules).toHaveBeenCalled();
expect(deleteRules).toHaveBeenCalled();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import { createBulkErrorObject } from '../../../routes/utils';
import { createRules } from '../crud/create_rules';
import { readRules } from '../crud/read_rules';
import { updateRules } from '../crud/update_rules';
import { deleteRules } from '../crud/delete_rules';
import { findRules } from '../search/find_rules';
import type { MlAuthz } from '../../../../machine_learning/authz';
import { throwAuthzError } from '../../../../machine_learning/validation';
import { checkRuleExceptionReferences } from './check_rule_exception_references';
Expand Down Expand Up @@ -104,18 +106,41 @@ export const importRules = async ({
});

if (rule == null) {
await createRules({
const createdRule = await createRules({
rulesClient,
params: {
...parsedRule,
exceptions_list: [...exceptions],
},
allowMissingConnectorSecrets,
});
resolve({
rule_id: parsedRule.rule_id,
status_code: 200,
// find rules with the same rule_id, in case of concurrent imports
const rules = await findRules({
rulesClient,
filter: `alert.attributes.params.ruleId: "${parsedRule.rule_id}"`,
page: 1,
fields: undefined,
perPage: undefined,
sortField: 'created_at',
sortOrder: 'asc',
});
// OK if it is a unique rule or the earlier rule created
if (rules.total === 1 || createdRule.id === rules.data[0].id) {
resolve({
rule_id: parsedRule.rule_id,
status_code: 200,
});
// else delete the duplicate rule and return status 409
} else {
deleteRules({ ruleId: createdRule.id, rulesClient });
resolve(
createBulkErrorObject({
ruleId: parsedRule.rule_id,
statusCode: 409,
message: `rule_id: "${parsedRule.rule_id}" already exists`,
})
);
}
} else if (rule != null && overwriteRules) {
await updateRules({
rulesClient,
Expand Down

0 comments on commit 3158cd6

Please sign in to comment.