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

[Response Ops][Alerting] Use ES client to update rule SO at end of rule run instead of SO client. #193341

Merged
merged 10 commits into from
Sep 30, 2024
Merged

[Response Ops][Alerting] Use ES client to update rule SO at end of rule run instead of SO client. #193341

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c234082
Using es partial update at end of rule run
ymao1 Sep 18, 2024
d6c5925
Cleanup
ymao1 Sep 18, 2024
0e92d58
[CI] Auto-commit changed files from 'node scripts/notice'
kibanamachine Sep 18, 2024
9429448
Merge branch 'main' of github.com:elastic/kibana into alerting/es-par…
ymao1 Sep 20, 2024
4683078
Adding allowlist for attributes we can partially update with ES
ymao1 Sep 20, 2024
f3946ff
Merge branch 'main' of github.com:elastic/kibana into alerting/es-par…
ymao1 Sep 20, 2024
ddb40c9
Merge branch 'main' into alerting/es-partial-update
elasticmachine Sep 23, 2024
24e5726
Merging in main
ymao1 Sep 27, 2024
7221c28
Merge branch 'main' into alerting/es-partial-update
elasticmachine Sep 30, 2024
409a2fa
Adding comment about bypassing audit logging
ymao1 Sep 30, 2024
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
2 changes: 1 addition & 1 deletion x-pack/plugins/alerting/server/saved_objects/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { RawRule } from '../types';
import { getImportWarnings } from './get_import_warnings';
import { isRuleExportable } from './is_rule_exportable';
import { RuleTypeRegistry } from '../rule_type_registry';
export { partiallyUpdateRule } from './partially_update_rule';
export { partiallyUpdateRule, partiallyUpdateRuleWithEs } from './partially_update_rule';
import {
RULES_SETTINGS_SAVED_OBJECT_TYPE,
MAINTENANCE_WINDOW_SAVED_OBJECT_TYPE,
Expand Down
116 changes: 112 additions & 4 deletions x-pack/plugins/alerting/server/saved_objects/partially_update_rule.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,22 @@ import {
ISavedObjectsRepository,
SavedObjectsErrorHelpers,
} from '@kbn/core/server';

import { PartiallyUpdateableRuleAttributes, partiallyUpdateRule } from './partially_update_rule';
import { savedObjectsClientMock } from '@kbn/core/server/mocks';
import {
PartiallyUpdateableRuleAttributes,
partiallyUpdateRule,
partiallyUpdateRuleWithEs,
} from './partially_update_rule';
import { elasticsearchServiceMock, savedObjectsClientMock } from '@kbn/core/server/mocks';
import { RULE_SAVED_OBJECT_TYPE } from '.';
import { ALERTING_CASES_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server';
import { estypes } from '@elastic/elasticsearch';

const MockSavedObjectsClientContract = savedObjectsClientMock.create();
const MockISavedObjectsRepository =
MockSavedObjectsClientContract as unknown as jest.Mocked<ISavedObjectsRepository>;
const esClient = elasticsearchServiceMock.createClusterClient().asInternalUser;

describe('partially_update_rule', () => {
describe('partiallyUpdateRule', () => {
beforeEach(() => {
jest.resetAllMocks();
});
Expand Down Expand Up @@ -104,6 +110,98 @@ describe('partially_update_rule', () => {
});
});

describe('partiallyUpdateRuleWithEs', () => {
beforeEach(() => {
jest.resetAllMocks();
jest.clearAllMocks();
});

test('should work with no options', async () => {
esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId));

await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes);
expect(esClient.update).toHaveBeenCalledTimes(1);
expect(esClient.update).toHaveBeenCalledWith({
id: `alert:${MockRuleId}`,
index: ALERTING_CASES_SAVED_OBJECT_INDEX,
doc: {
alert: DefaultAttributes,
},
});
});

test('should work with extraneous attributes ', async () => {
const attributes = ExtraneousAttributes as unknown as PartiallyUpdateableRuleAttributes;
esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId));

await partiallyUpdateRuleWithEs(esClient, MockRuleId, attributes);
expect(esClient.update).toHaveBeenCalledWith({
id: `alert:${MockRuleId}`,
index: ALERTING_CASES_SAVED_OBJECT_INDEX,
doc: {
alert: ExtraneousAttributes,
},
});
});

test('should handle ES errors', async () => {
esClient.update.mockRejectedValueOnce(new Error('wops'));

await expect(
partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes)
).rejects.toThrowError('wops');
});

test('should handle the version option', async () => {
esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId));

await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes, {
version: 'WzQsMV0=',
});
expect(esClient.update).toHaveBeenCalledWith({
id: `alert:${MockRuleId}`,
index: ALERTING_CASES_SAVED_OBJECT_INDEX,
if_primary_term: 1,
if_seq_no: 4,
doc: {
alert: DefaultAttributes,
},
});
});

test('should handle the ignore404 option', async () => {
esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId));

await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes, { ignore404: true });
expect(esClient.update).toHaveBeenCalledWith(
{
id: `alert:${MockRuleId}`,
index: ALERTING_CASES_SAVED_OBJECT_INDEX,
doc: {
alert: DefaultAttributes,
},
},
{ ignore: [404] }
);
});

test('should handle the refresh option', async () => {
esClient.update.mockResolvedValueOnce(MockEsUpdateResponse(MockRuleId));

await partiallyUpdateRuleWithEs(esClient, MockRuleId, DefaultAttributes, {
refresh: 'wait_for',
});
expect(esClient.update).toHaveBeenCalledWith({
id: `alert:${MockRuleId}`,
index: ALERTING_CASES_SAVED_OBJECT_INDEX,
doc: {
alert: DefaultAttributes,
},
refresh: 'wait_for',
});
});
});

function getMockSavedObjectClients(): Record<
string,
jest.Mocked<SavedObjectsClientContract | ISavedObjectsRepository>
Expand Down Expand Up @@ -137,3 +235,13 @@ const MockUpdateValue = {
},
references: [],
};

const MockEsUpdateResponse = (id: string) => ({
_index: '.kibana_alerting_cases_9.0.0_001',
_id: `alert:${id}`,
_version: 3,
result: 'updated' as estypes.Result,
_shards: { total: 1, successful: 1, failed: 0 },
_seq_no: 5,
_primary_term: 1,
});
33 changes: 33 additions & 0 deletions x-pack/plugins/alerting/server/saved_objects/partially_update_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@

import { omit, pick } from 'lodash';
import {
ElasticsearchClient,
SavedObjectsClient,
SavedObjectsErrorHelpers,
SavedObjectsUpdateOptions,
} from '@kbn/core/server';
import { decodeRequestVersion } from '@kbn/core-saved-objects-base-server-internal';
import { ALERTING_CASES_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server';
import { RawRule } from '../types';

import {
Expand Down Expand Up @@ -67,3 +70,33 @@ export async function partiallyUpdateRule(
throw err;
}
}

// direct, partial update to a rule saved object via ElasticsearchClient
export async function partiallyUpdateRuleWithEs(
esClient: ElasticsearchClient,
id: string,
attributes: PartiallyUpdateableRuleAttributes,
options: PartiallyUpdateRuleSavedObjectOptions = {}
): Promise<void> {
// ensure we only have the valid attributes that are not encrypted and are excluded from AAD
const attributeUpdates = omit(attributes, [
Copy link
Member

Choose a reason for hiding this comment

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

This seems slighty dangerous, in case we forget to include one of the attribues in the lists used below.

Feels like it might be better to have an explicit list of attributes that we ALLOW to be partially updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

++

Copy link
Contributor

@jeramysoucy jeramysoucy Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree as well. Disclaimer - typically I would categorize something like this as "strongly not recommended", but with intimate knowledge of an ESO type and careful maintenance, this is possible to do safely.

Filtering by an explicit safe list of just the attributes that we expect to modify in this way, and also omitting any encrypted/AAD attributes (just in case), seems most prudent. Even if it requires a little bit more maintenance in the long run, this has potential to go quite poorly, so we should do what we can to prevent that possibility.

cc @azasypkin to get his 2 cents as well

Copy link
Contributor

Choose a reason for hiding this comment

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

@ymao1 Can we wait to merge this until Oleg has a chance to take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeramysoucy Yes absolutely! I'll make the changes for the AAD attributes and will wait to merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added allowlist in this commit: 4683078

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed with @azasypkin and he is 👍 with an additional comment about why we're bypassing audit logging (added in 409a2fa) and a followup issue for the Core team to investigate how they might expose a more performance SO client (#194435)

...RuleAttributesToEncrypt,
...RuleAttributesIncludedInAAD,
]);

const updateParams = {
id: `alert:${id}`,
index: ALERTING_CASES_SAVED_OBJECT_INDEX,
...(options.version ? decodeRequestVersion(options.version) : {}),
doc: {
alert: attributeUpdates,
},
...(options.refresh ? { refresh: options.refresh } : {}),
};

if (options.ignore404) {
await esClient.update(updateParams, { ignore: [404] });
} else {
await esClient.update(updateParams);
}
}
89 changes: 48 additions & 41 deletions x-pack/plugins/alerting/server/task_runner/fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import { TaskStatus } from '@kbn/task-manager-plugin/server';
import { SavedObject } from '@kbn/core/server';
import { ALERTING_CASES_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server';
import {
Rule,
RuleTypeParams,
Expand Down Expand Up @@ -64,7 +65,7 @@ const defaultHistory = [
},
];

export const generateSavedObjectParams = ({
export const generateRuleUpdateParams = ({
error = null,
warning = null,
status = 'ok',
Expand All @@ -83,53 +84,59 @@ export const generateSavedObjectParams = ({
history?: RuleMonitoring['run']['history'];
alertsCount?: Record<string, number>;
}) => [
RULE_SAVED_OBJECT_TYPE,
'1',
{
monitoring: {
run: {
calculated_metrics: {
success_ratio: successRatio,
id: `alert:1`,
index: ALERTING_CASES_SAVED_OBJECT_INDEX,
doc: {
alert: {
monitoring: {
run: {
calculated_metrics: {
success_ratio: successRatio,
},
history,
last_run: {
timestamp: '1970-01-01T00:00:00.000Z',
metrics: {
duration: 0,
gap_duration_s: null,
total_alerts_created: null,
total_alerts_detected: null,
total_indexing_duration_ms: null,
total_search_duration_ms: null,
},
},
},
},
history,
last_run: {
timestamp: '1970-01-01T00:00:00.000Z',
metrics: {
duration: 0,
gap_duration_s: null,
total_alerts_created: null,
total_alerts_detected: null,
total_indexing_duration_ms: null,
total_search_duration_ms: null,
executionStatus: {
error,
lastDuration: 0,
lastExecutionDate: '1970-01-01T00:00:00.000Z',
status,
warning,
},
lastRun: {
outcome,
outcomeOrder: RuleLastRunOutcomeOrderMap[outcome],
outcomeMsg:
(error?.message && [error?.message]) ||
(warning?.message && [warning?.message]) ||
null,
warning: error?.reason || warning?.reason || null,
alertsCount: {
active: 0,
ignored: 0,
new: 0,
recovered: 0,
...(alertsCount || {}),
},
},
nextRun,
running: false,
},
},
executionStatus: {
error,
lastDuration: 0,
lastExecutionDate: '1970-01-01T00:00:00.000Z',
status,
warning,
},
lastRun: {
outcome,
outcomeOrder: RuleLastRunOutcomeOrderMap[outcome],
outcomeMsg:
(error?.message && [error?.message]) || (warning?.message && [warning?.message]) || null,
warning: error?.reason || warning?.reason || null,
alertsCount: {
active: 0,
ignored: 0,
new: 0,
recovered: 0,
...(alertsCount || {}),
},
},
nextRun,
running: false,
},
{ refresh: false, namespace: undefined },
{ ignore: [404] },
];

export const GENERIC_ERROR_MESSAGE = 'GENERIC ERROR MESSAGE';
Expand Down
Loading