Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Response Ops] Keep task document when enabling/disabling rules #139826

Merged
merged 27 commits into from
Sep 12, 2022
Merged

[Response Ops] Keep task document when enabling/disabling rules #139826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
ff61e14
wip
ymao1 Aug 30, 2022
1a56865
Merge branch 'main' of https://github.com/elastic/kibana into task-ma…
ymao1 Aug 31, 2022
b9c2d86
wip
ymao1 Aug 31, 2022
531ceb6
Fixing types and adding unit tests for task manager disable
ymao1 Aug 31, 2022
bdff214
Updating to enable/disable. Update rules client to use new fns
ymao1 Aug 31, 2022
feff3da
Merge branch 'main' of https://github.com/elastic/kibana into task-ma…
ymao1 Sep 1, 2022
69397ff
Updating unit tests. Fixing enable to still schedule task if necessary
ymao1 Sep 1, 2022
f239fe9
Adding functional test for task manager migration
ymao1 Sep 1, 2022
2fa3a44
Fixing query. Updating functional tests
ymao1 Sep 1, 2022
638852e
Setting scheduledTaskId to null on disable only if it does not match …
ymao1 Sep 1, 2022
0869306
Merge branch 'main' of https://github.com/elastic/kibana into task-ma…
ymao1 Sep 1, 2022
46bfc04
Updating README
ymao1 Sep 1, 2022
0833943
Fixing tests
ymao1 Sep 1, 2022
fa4a907
Merge branch 'main' of https://github.com/elastic/kibana into task-ma…
ymao1 Sep 1, 2022
a2db534
Task manager runner doesn't overwrite enabled on update
ymao1 Sep 1, 2022
4ee56b7
Updating migration to set enabled: false for failed and unrecognized …
ymao1 Sep 1, 2022
482d58d
Merge branch 'main' into task-manager/enabled
kibanamachine Sep 1, 2022
4dd9ade
Fixing tests
ymao1 Sep 2, 2022
95b9e8c
Merge branch 'task-manager/enabled' of https://github.com/ymao1/kiban…
ymao1 Sep 2, 2022
8dc3c62
Merge branch 'main' into task-manager/enabled
kibanamachine Sep 2, 2022
869be57
Merge branch 'main' into task-manager/enabled
kibanamachine Sep 6, 2022
397495e
Merge branch 'main' into task-manager/enabled
kibanamachine Sep 6, 2022
1123e3a
Merge branch 'main' into task-manager/enabled
kibanamachine Sep 7, 2022
c9390eb
Merge branch 'main' into task-manager/enabled
kibanamachine Sep 8, 2022
bb04cff
Merge branch 'main' into task-manager/enabled
kibanamachine Sep 9, 2022
2e25e39
Merge branch 'main' of https://github.com/elastic/kibana into task-ma…
ymao1 Sep 12, 2022
3bd7a3a
PR feedback
ymao1 Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions x-pack/plugins/alerting/server/rules_client/rules_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ export interface GetActionErrorLogByIdParams {
sort: estypes.Sort;
}

interface ScheduleRuleOptions {
interface ScheduleTaskOptions {
id: string;
consumer: string;
ruleTypeId: string;
Expand Down Expand Up @@ -589,7 +589,7 @@ export class RulesClient {
if (data.enabled) {
let scheduledTask;
try {
scheduledTask = await this.scheduleRule({
scheduledTask = await this.scheduleTask({
id: createdAlert.id,
consumer: data.consumer,
ruleTypeId: rawRule.alertTypeId,
Expand Down Expand Up @@ -2138,7 +2138,24 @@ export class RulesClient {
} catch (e) {
throw e;
}
const scheduledTask = await this.scheduleRule({
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update to the enable logic takes into account the fact that there may be already disabled rules with no corresponding task.

If the rule is disabled with no corresponding task, it's scheduledTaskId will be undefined. In this case, we want to schedule a task on enable.

If a rule somehow has a scheduledTaskId defined but it doesn't actually exist, we want to schedule a task on enable.

Finally, if a rule has a scheduledTaskId defined and the task exists, we enable the task.

let scheduledTaskIdToCreate: string | null = null;
ymao1 marked this conversation as resolved.
Show resolved Hide resolved
if (attributes.scheduledTaskId) {
// If scheduledTaskId defined in rule SO, make sure it exists
try {
await this.taskManager.get(attributes.scheduledTaskId);
ymao1 marked this conversation as resolved.
Show resolved Hide resolved
} catch (err) {
scheduledTaskIdToCreate = id;
}
} else {
// If scheduledTaskId doesn't exist in rule SO, set it to rule ID
scheduledTaskIdToCreate = id;
}

if (scheduledTaskIdToCreate) {
// Schedule the task if it doesn't exist
const scheduledTask = await this.scheduleTask({
id,
consumer: attributes.consumer,
ruleTypeId: attributes.alertTypeId,
Expand All @@ -2148,6 +2165,9 @@ export class RulesClient {
await this.unsecuredSavedObjectsClient.update('alert', id, {
scheduledTaskId: scheduledTask.id,
});
} else {
// Task exists so set enabled to true
await this.taskManager.bulkEnableDisable([attributes.scheduledTaskId!], true);
}
}

Expand Down Expand Up @@ -2282,14 +2302,21 @@ export class RulesClient {
this.updateMeta({
...attributes,
enabled: false,
scheduledTaskId: null,
scheduledTaskId: attributes.scheduledTaskId === id ? attributes.scheduledTaskId : null,
updatedBy: await this.getUserName(),
updatedAt: new Date().toISOString(),
}),
{ version }
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update to the disable logic takes into account that there may be pre 8.0 rules running where the scheduled task ID does not equal the rule ID. In these cases, we still want to remove the task document so that a new one matching the rule id can be created on enable.

If the scheduledTaskId already matches the rule ID, this will set the task to disabled.

// If the scheduledTaskId does not match the rule id, we should
// remove the task, otherwise mark the task as disabled
if (attributes.scheduledTaskId) {
await this.taskManager.removeIfExists(attributes.scheduledTaskId);
if (attributes.scheduledTaskId !== id) {
await this.taskManager.removeIfExists(attributes.scheduledTaskId);
} else {
await this.taskManager.bulkEnableDisable([attributes.scheduledTaskId], false);
}
}
}
}
Expand Down Expand Up @@ -2767,7 +2794,7 @@ export class RulesClient {
return this.spaceId;
}

private async scheduleRule(opts: ScheduleRuleOptions) {
private async scheduleTask(opts: ScheduleTaskOptions) {
const { id, consumer, ruleTypeId, schedule, throwOnConflict } = opts;
const taskInstance = {
id, // use the same ID for task document as the rule
Expand All @@ -2784,6 +2811,7 @@ export class RulesClient {
alertInstances: {},
},
scope: ['alerting'],
enabled: true,
};
try {
return await this.taskManager.schedule(taskInstance);
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/rules_client/tests/create.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,7 @@ describe('create()', () => {
expect(taskManager.schedule.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Object {
"enabled": true,
"id": "1",
"params": Object {
"alertId": "1",
Expand Down
153 changes: 126 additions & 27 deletions x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const rulesClientParams: jest.Mocked<ConstructorOptions> = {
beforeEach(() => {
getBeforeSetup(rulesClientParams, taskManager, ruleTypeRegistry);
taskManager.get.mockResolvedValue({
id: 'task-123',
id: '1',
taskType: 'alerting:123',
scheduledAt: new Date(),
attempts: 1,
Expand All @@ -81,15 +81,15 @@ setGlobalDate();

describe('disable()', () => {
let rulesClient: RulesClient;
const existingAlert = {
const existingRule = {
id: '1',
type: 'alert',
attributes: {
consumer: 'myApp',
schedule: { interval: '10s' },
alertTypeId: 'myType',
enabled: true,
scheduledTaskId: 'task-123',
scheduledTaskId: '1',
actions: [
{
group: 'default',
Expand All @@ -105,10 +105,10 @@ describe('disable()', () => {
version: '123',
references: [],
};
const existingDecryptedAlert = {
...existingAlert,
const existingDecryptedRule = {
...existingRule,
attributes: {
...existingAlert.attributes,
...existingRule.attributes,
apiKey: Buffer.from('123:abc').toString('base64'),
apiKeyOwner: 'elastic',
},
Expand All @@ -118,12 +118,12 @@ describe('disable()', () => {

beforeEach(() => {
rulesClient = new RulesClient(rulesClientParams);
unsecuredSavedObjectsClient.get.mockResolvedValue(existingAlert);
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedAlert);
unsecuredSavedObjectsClient.get.mockResolvedValue(existingRule);
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedRule);
});

describe('authorization', () => {
test('ensures user is authorised to disable this type of alert under the consumer', async () => {
test('ensures user is authorised to disable this type of rule under the consumer', async () => {
await rulesClient.disable({ id: '1' });

expect(authorization.ensureAuthorized).toHaveBeenCalledWith({
Expand All @@ -134,7 +134,7 @@ describe('disable()', () => {
});
});

test('throws when user is not authorised to disable this type of alert', async () => {
test('throws when user is not authorised to disable this type of rule', async () => {
authorization.ensureAuthorized.mockRejectedValue(
new Error(`Unauthorized to disable a "myType" alert for "myApp"`)
);
Expand Down Expand Up @@ -191,7 +191,7 @@ describe('disable()', () => {
});
});

test('disables an alert', async () => {
test('disables an rule', async () => {
await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
Expand All @@ -208,7 +208,7 @@ describe('disable()', () => {
meta: {
versionApiKeyLastmodified: 'v7.10.0',
},
scheduledTaskId: null,
scheduledTaskId: '1',
apiKey: 'MTIzOmFiYw==',
apiKeyOwner: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
Expand All @@ -229,11 +229,12 @@ describe('disable()', () => {
version: '123',
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(taskManager.bulkEnableDisable).toHaveBeenCalledWith(['1'], false);
expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
});

test('disables the rule with calling event log to "recover" the alert instances from the task state', async () => {
const scheduledTaskId = 'task-123';
const scheduledTaskId = '1';
taskManager.get.mockResolvedValue({
id: scheduledTaskId,
taskType: 'alerting:123',
Expand Down Expand Up @@ -278,7 +279,7 @@ describe('disable()', () => {
meta: {
versionApiKeyLastmodified: 'v7.10.0',
},
scheduledTaskId: null,
scheduledTaskId: '1',
apiKey: 'MTIzOmFiYw==',
apiKeyOwner: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
Expand All @@ -299,7 +300,8 @@ describe('disable()', () => {
version: '123',
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(taskManager.bulkEnableDisable).toHaveBeenCalledWith(['1'], false);
expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();

expect(eventLogger.logEvent).toHaveBeenCalledTimes(1);
expect(eventLogger.logEvent.mock.calls[0][0]).toStrictEqual({
Expand Down Expand Up @@ -359,7 +361,7 @@ describe('disable()', () => {
meta: {
versionApiKeyLastmodified: 'v7.10.0',
},
scheduledTaskId: null,
scheduledTaskId: '1',
apiKey: 'MTIzOmFiYw==',
apiKeyOwner: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
Expand All @@ -380,7 +382,8 @@ describe('disable()', () => {
version: '123',
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(taskManager.bulkEnableDisable).toHaveBeenCalledWith(['1'], false);
expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();

expect(eventLogger.logEvent).toHaveBeenCalledTimes(0);
expect(rulesClientParams.logger.warn).toHaveBeenCalledWith(
Expand All @@ -403,7 +406,7 @@ describe('disable()', () => {
schedule: { interval: '10s' },
alertTypeId: 'myType',
enabled: false,
scheduledTaskId: null,
scheduledTaskId: '1',
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
Expand All @@ -422,30 +425,33 @@ describe('disable()', () => {
version: '123',
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(taskManager.bulkEnableDisable).toHaveBeenCalledWith(['1'], false);
expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
});

test(`doesn't disable already disabled alerts`, async () => {
test(`doesn't disable already disabled rules`, async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
...existingDecryptedAlert,
...existingDecryptedRule,
attributes: {
...existingDecryptedAlert.attributes,
...existingDecryptedRule.attributes,
actions: [],
enabled: false,
},
});

await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled();
expect(taskManager.removeIfExists).not.toHaveBeenCalled();
expect(taskManager.bulkEnableDisable).not.toHaveBeenCalled();
expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
});

test('swallows error when failing to load decrypted saved object', async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail'));

await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled();
expect(taskManager.removeIfExists).toHaveBeenCalled();
expect(taskManager.bulkEnableDisable).toHaveBeenCalled();
expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
expect(rulesClientParams.logger.error).toHaveBeenCalledWith(
'disable(): Failed to load API key of alert 1: Fail'
);
Expand All @@ -457,13 +463,106 @@ describe('disable()', () => {
await expect(rulesClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
`"Failed to update"`
);
expect(taskManager.bulkEnableDisable).not.toHaveBeenCalled();
expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
});

test('throws when failing to remove task from task manager', async () => {
taskManager.removeIfExists.mockRejectedValueOnce(new Error('Failed to remove task'));
test('throws when failing to disable task', async () => {
taskManager.bulkEnableDisable.mockRejectedValueOnce(new Error('Failed to disable task'));

await expect(rulesClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
`"Failed to disable task"`
);
expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
});

test('removes task document if scheduled task id does not match rule id', async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
...existingRule,
attributes: {
...existingRule.attributes,
scheduledTaskId: 'task-123',
},
});
await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
namespace: 'default',
});
expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith(
'alert',
'1',
{
consumer: 'myApp',
schedule: { interval: '10s' },
alertTypeId: 'myType',
enabled: false,
scheduledTaskId: null,
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
{
group: 'default',
id: '1',
actionTypeId: '1',
actionRef: '1',
params: {
foo: true,
},
},
],
},
{
version: '123',
}
);
expect(taskManager.bulkEnableDisable).not.toHaveBeenCalled();
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
});

test('throws when failing to remove existing task', async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
...existingRule,
attributes: {
...existingRule.attributes,
scheduledTaskId: 'task-123',
},
});
taskManager.removeIfExists.mockRejectedValueOnce(new Error('Failed to remove task'));
ymao1 marked this conversation as resolved.
Show resolved Hide resolved
await expect(rulesClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
`"Failed to remove task"`
);
expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
namespace: 'default',
});
expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith(
'alert',
'1',
{
consumer: 'myApp',
schedule: { interval: '10s' },
alertTypeId: 'myType',
enabled: false,
scheduledTaskId: null,
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
{
group: 'default',
id: '1',
actionTypeId: '1',
actionRef: '1',
params: {
foo: true,
},
},
],
},
{
version: '123',
}
);
expect(taskManager.bulkEnableDisable).not.toHaveBeenCalled();
});
});
Loading