Skip to content

Commit

Permalink
[ResponseOps][Rules] Fix inconsistencies in bulk enable/disable rules (…
Browse files Browse the repository at this point in the history 
…elastic#191492)

Resolves elastic#181050

## Summary
This PR 
- fixes `Inconsistent responses from RulesClient` by updating the
`bulkEnable` and `bulkDisable` rules apis to return all the rules that
were in the payload.
So below scenarios will return:
    - Both rules initially disabled:
      ```typescript
      { 
        "errors": [], 
        "rules": [{...}, {...}], // <- all rules are returned
        "total": 2 
      }
      ``` 
   - One rule disabled, one enabled:
      ```typescript
      { 
        "errors": [], 
        "rules": [{...}, {...}], // <- all rules are returned
        "total": 2 
      }
      ``` 
   - Both rules initially enabled:
      ```typescript
      { 
        "errors": [], 
        "rules": [{...}, {...}], // <- all rules are returned
        "total": 2 
      }
      ``` 

### How to verify
- Create two (detection engine SIEM / Observability / Stack) rules, one
enable and one disable
- Select both rules and bulk enable them via UI OR
- Enable 2 rules via API 
- Verify both rules are returned in response 

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
  • Loading branch information
@js-jankisalvi @gergoabraham
js-jankisalvi authored and gergoabraham committed Sep 13, 2024
1 parent 3281a14 commit 8121f36
Show file tree
Hide file tree
Showing 7 changed files with 671 additions and 55 deletions.
103 changes: 95 additions & 8 deletions .../plugins/alerting/server/application/rule/methods/bulk_disable/bulk_disable_rules.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ describe('bulkDisableRules', () => {
});
});

test('should thow an error if number of matched rules greater than 10,000', async () => {
test('should throw an error if number of matched rules greater than 10,000', async () => {
unsecuredSavedObjectsClient.find.mockResolvedValue({
aggregations: {
alertTypeId: {
Expand Down Expand Up @@ -464,12 +464,81 @@ describe('bulkDisableRules', () => {
);
});

test('should skip rule if it is already disabled', async () => {
test('should return both rules if one is already disabled and one is enabled when bulk disable is based on ids', async () => {
mockCreatePointInTimeFinderAsInternalUser({
saved_objects: [enabledRuleForBulkOps1, disabledRuleForBulkDisable2],
});
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [disabledRuleForBulkDisable1],
saved_objects: [disabledRuleForBulkDisable1, disabledRuleForBulkDisable2],
});

const result = await rulesClient.bulkDisableRules({ ids: ['id1', 'id2'] });

expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith(
expect.arrayContaining([
expect.objectContaining({
id: 'id1',
attributes: expect.objectContaining({
enabled: false,
}),
}),
expect.objectContaining({
id: 'id2',
attributes: expect.objectContaining({
enabled: false,
}),
}),
]),
{ overwrite: true }
);

expect(result.rules[0].id).toBe('id1');
expect(result.rules[1].id).toBe('id2');
});

test('should return rules in correct format', async () => {
mockCreatePointInTimeFinderAsInternalUser({
saved_objects: [enabledRuleForBulkOps1, disabledRuleForBulkDisable2],
});
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [disabledRuleForBulkDisable1, disabledRuleForBulkDisable2],
});

const result = await rulesClient.bulkDisableRules({ ids: ['id1', 'id2'] });

expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith(
expect.arrayContaining([
expect.objectContaining({
id: 'id1',
attributes: expect.objectContaining({
enabled: false,
}),
}),
expect.objectContaining({
id: 'id2',
attributes: expect.objectContaining({
enabled: false,
}),
}),
]),
{ overwrite: true }
);

expect(result).toStrictEqual({
errors: [],
rules: [returnedRuleForBulkDisable1, returnedRuleForBulkDisable2],
total: 2,
});
});

test('should return both rules if one is already disabled and one is enabled when bulk disable is based on filter', async () => {
mockCreatePointInTimeFinderAsInternalUser({
saved_objects: [enabledRuleForBulkOps1, disabledRuleForBulkDisable2],
});
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [disabledRuleForBulkDisable1, disabledRuleForBulkDisable2],
});

const result = await rulesClient.bulkDisableRules({ filter: 'fake_filter' });
Expand All @@ -483,13 +552,19 @@ describe('bulkDisableRules', () => {
enabled: false,
}),
}),
expect.objectContaining({
id: 'id2',
attributes: expect.objectContaining({
enabled: false,
}),
}),
]),
{ overwrite: true }
);

expect(result).toStrictEqual({
errors: [],
rules: [returnedRuleForBulkDisable1],
rules: [returnedRuleForBulkDisable1, returnedRuleForBulkDisable2],
total: 2,
});
});
Expand Down Expand Up @@ -712,8 +787,14 @@ describe('bulkDisableRules', () => {
saved_objects: [
enabledRuleForBulkOps1,
enabledRuleForBulkOps2,
siemRuleForBulkOps1,
siemRuleForBulkOps2,
{
...siemRuleForBulkOps1,
attributes: { ...siemRuleForBulkOps1.attributes, enabled: true },
},
{
...siemRuleForBulkOps2,
attributes: { ...siemRuleForBulkOps2.attributes, enabled: true },
},
],
};
},
Expand All @@ -723,8 +804,14 @@ describe('bulkDisableRules', () => {
saved_objects: [
enabledRuleForBulkOps1,
enabledRuleForBulkOps2,
siemRuleForBulkOps1,
siemRuleForBulkOps2,
{
...siemRuleForBulkOps1,
attributes: { ...siemRuleForBulkOps1.attributes, enabled: true },
},
{
...siemRuleForBulkOps2,
attributes: { ...siemRuleForBulkOps2.attributes, enabled: true },
},
],
});

Expand Down
4 changes: 1 addition & 3 deletions x-pack/plugins/alerting/server/application/rule/methods/bulk_disable/bulk_disable_rules.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,6 @@ const bulkDisableRulesWithOCC = async (
untrack: boolean;
}
) => {
const additionalFilter = nodeBuilder.is('alert.attributes.enabled', 'true');

const rulesFinder = await withSpan(
{
name: 'encryptedSavedObjectsClient.createPointInTimeFinderDecryptedAsInternalUser',
Expand All @@ -143,7 +141,7 @@ const bulkDisableRulesWithOCC = async (
() =>
context.encryptedSavedObjectsClient.createPointInTimeFinderDecryptedAsInternalUser<RuleAttributes>(
{
filter: filter ? nodeBuilder.and([filter, additionalFilter]) : additionalFilter,
filter,
type: RULE_SAVED_OBJECT_TYPE,
perPage: 100,
...(context.namespace ? { namespaces: [context.namespace] } : undefined),
Expand Down
62 changes: 54 additions & 8 deletions ...ck/plugins/alerting/server/application/rule/methods/bulk_enable/bulk_enable_rules.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,26 +210,26 @@ describe('bulkEnableRules', () => {
});
});

test('should enable two rule and return right actions', async () => {
test('should enable two rules and return right actions', async () => {
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [enabledRuleForBulkOpsWithActions1, enabledRuleForBulkOpsWithActions2],
});

const result = await rulesClient.bulkDisableRules({ filter: 'fake_filter' });
const result = await rulesClient.bulkEnableRules({ filter: 'fake_filter' });

expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith(
expect.arrayContaining([
expect.objectContaining({
id: 'id1',
attributes: expect.objectContaining({
enabled: false,
enabled: true,
}),
}),
expect.objectContaining({
id: 'id2',
attributes: expect.objectContaining({
enabled: false,
enabled: true,
}),
}),
]),
Expand All @@ -239,6 +239,7 @@ describe('bulkEnableRules', () => {
expect(result).toStrictEqual({
errors: [],
rules: [returnedRuleForBulkEnableWithActions1, returnedRuleForBulkEnableWithActions2],
taskIdsFailedToBeEnabled: [],
total: 2,
});
});
Expand Down Expand Up @@ -432,15 +433,22 @@ describe('bulkEnableRules', () => {
});
});

test('should skip rule if it is already enabled', async () => {
test('should return both rules if one is already enabled and one is disabled when bulk enable is based on ids', async () => {
mockCreatePointInTimeFinderAsInternalUser({
saved_objects: [disabledRule1, enabledRuleForBulkOps2],
});
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [enabledRuleForBulkOps1],
saved_objects: [enabledRuleForBulkOps1, enabledRuleForBulkOps2],
});

const result = await rulesClient.bulkEnableRules({ filter: 'fake_filter' });
const result = await rulesClient.bulkEnableRules({
ids: ['id1', 'id2'],
});

expect(taskManager.bulkSchedule).not.toHaveBeenCalled();

expect(taskManager.bulkEnable).toHaveBeenCalledTimes(1);
expect(taskManager.bulkEnable).toHaveBeenCalledWith(['id1', 'id2']);

expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith(
Expand All @@ -457,7 +465,45 @@ describe('bulkEnableRules', () => {

expect(result).toStrictEqual({
errors: [],
rules: [returnedRuleForBulkOps1],
rules: [returnedRuleForBulkOps1, returnedRuleForBulkOps2],
total: 2,
taskIdsFailedToBeEnabled: [],
});
});

test('should return both rules if one is already enabled and one is disabled when bulk enable is based on filters', async () => {
mockCreatePointInTimeFinderAsInternalUser({
saved_objects: [disabledRule1, enabledRuleForBulkOps2],
});
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [enabledRuleForBulkOps1, enabledRuleForBulkOps2],
});

const result = await rulesClient.bulkEnableRules({
filter: 'fake_filter',
});

expect(taskManager.bulkSchedule).not.toHaveBeenCalled();

expect(taskManager.bulkEnable).toHaveBeenCalledTimes(1);
expect(taskManager.bulkEnable).toHaveBeenCalledWith(['id1', 'id2']);

expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith(
expect.arrayContaining([
expect.objectContaining({
id: 'id1',
attributes: expect.objectContaining({
enabled: true,
}),
}),
]),
{ overwrite: true }
);

expect(result).toStrictEqual({
errors: [],
rules: [returnedRuleForBulkOps1, returnedRuleForBulkOps2],
total: 2,
taskIdsFailedToBeEnabled: [],
});
Expand Down
8 changes: 2 additions & 6 deletions x-pack/plugins/alerting/server/application/rule/methods/bulk_enable/bulk_enable_rules.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,6 @@ const bulkEnableRulesWithOCC = async (
context: RulesClientContext,
{ filter }: { filter: KueryNode | null }
) => {
const additionalFilter = nodeBuilder.is('alert.attributes.enabled', 'false');

const rulesFinder = await withSpan(
{
name: 'encryptedSavedObjectsClient.createPointInTimeFinderDecryptedAsInternalUser',
Expand All @@ -163,7 +161,7 @@ const bulkEnableRulesWithOCC = async (
async () =>
await context.encryptedSavedObjectsClient.createPointInTimeFinderDecryptedAsInternalUser<RuleAttributes>(
{
filter: filter ? nodeBuilder.and([filter, additionalFilter]) : additionalFilter,
filter,
type: RULE_SAVED_OBJECT_TYPE,
perPage: 100,
...(context.namespace ? { namespaces: [context.namespace] } : undefined),
Expand All @@ -187,9 +185,7 @@ const bulkEnableRulesWithOCC = async (
}
await rulesFinder.close();

const updatedInterval = rulesFinderRules
.filter((rule) => !rule.attributes.enabled)
.map((rule) => rule.attributes.schedule?.interval);
const updatedInterval = rulesFinderRules.map((rule) => rule.attributes.schedule?.interval);

const validationPayload = await validateScheduleLimit({
context,
Expand Down
23 changes: 1 addition & 22 deletions .../lib/detection_engine/rule_management/api/rules/bulk_actions/bulk_enable_disable_rules.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export const bulkEnableDisableRules = async ({
mlAuthz,
}: BulkEnableDisableRulesArgs): Promise<BulkEnableDisableRulesOutcome> => {
const errors: Array<PromisePoolError<RuleAlertType, Error>> = [];
const updatedRules: RuleAlertType[] = [];

// In the first step, we validate if the rules can be enabled
const validatedRules: RuleAlertType[] = [];
Expand Down Expand Up @@ -64,26 +63,6 @@ export const bulkEnableDisableRules = async ({
? await rulesClient.bulkEnableRules({ ids: ruleIds })
: await rulesClient.bulkDisableRules({ ids: ruleIds });

const failedRuleIds = results.errors.map(({ rule: { id } }) => id);

// We need to go through the original rules array and update rules that were
// not returned as failed from the bulkEnableRules. We cannot rely on the
// results from the bulkEnableRules because the response is not consistent.
// Some rules might be missing in the response if they were skipped by
// Alerting Framework. See this issue for more details:
// https://github.com/elastic/kibana/issues/181050
updatedRules.push(
...rules.flatMap((rule) => {
if (failedRuleIds.includes(rule.id)) {
return [];
}
return {
...rule,
enabled: operation === 'enable',
};
})
);

// Rule objects returned from the bulkEnableRules are not
// compatible with the response type. So we need to map them to
// the original rules and update the enabled field
Expand All @@ -99,7 +78,7 @@ export const bulkEnableDisableRules = async ({
);

return {
updatedRules,
updatedRules: results.rules as RuleAlertType[],
errors,
};
};
Loading

0 comments on commit 8121f36

Please sign in to comment.