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

Add rule model versions in alerting #171927

Merged
merged 9 commits into from
Nov 28, 2023
+168 −5
Merged

Add rule model versions in alerting #171927

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b070ada
Add rule model versions in alerting
ersin-erdal Nov 24, 2023
2c916a3
[CI] Auto-commit changed files from 'node scripts/jest_integration -u…
kibanamachine Nov 24, 2023
e9f502d
update filters.schema.meta.field schema
ersin-erdal Nov 24, 2023
77496a5
Merge remote-tracking branch 'origin/166967-model-versions' into 1669…
ersin-erdal Nov 24, 2023
8612f9e
Merge branch 'main' into 166967-model-versions
ersin-erdal Nov 24, 2023
72ecc73
Merge branch 'main' into 166967-model-versions
ersin-erdal Nov 27, 2023
52a33ff
add unit test for rule model versions
ersin-erdal Nov 28, 2023
3a74ac3
Merge remote-tracking branch 'origin/166967-model-versions' into 1669…
ersin-erdal Nov 28, 2023
c893a8d
fix typo
ersin-erdal Nov 28, 2023
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 src/core/server/integration_tests/ci_checks/saved_objects/check_registered_types.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ describe('checking migration metadata changes on all registered SO types', () =>
Object {
"action": "cc93fe2c0c76e57c2568c63170e05daea897c136",
"action_task_params": "96e27e7f4e8273ffcd87060221e2b75e81912dd5",
"alert": "dc710bc17dfc98a9a703d388569abccce5f8bf07",
"alert": "3a67d3f1db80af36bd57aaea47ecfef87e43c58f",
"api_key_pending_invalidation": "1399e87ca37b3d3a65d269c924eda70726cfe886",
"apm-custom-dashboards": "b67128f78160c288bd7efe25b2da6e2afd5e82fc",
"apm-indices": "8a2d68d415a4b542b26b0d292034a28ffac6fed4",
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/lib/create_get_alert_indices_alias.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ describe('createGetAlertIndicesAliasFn', () => {
licensing: licensingMock.createSetup(),
minimumScheduleInterval: { value: '1m', enforce: false },
inMemoryMetrics,
latestRuleVersion: 1,
};
const registry = new RuleTypeRegistry(ruleTypeRegistryParams);
registry.register({
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/alerting/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ import {
} from './types';
import { registerAlertingUsageCollector } from './usage';
import { initializeAlertingTelemetry, scheduleAlertingTelemetry } from './usage/task';
import { setupSavedObjects } from './saved_objects';
import { setupSavedObjects, getLatestRuleVersion } from './saved_objects';
import {
initializeApiKeyInvalidator,
scheduleApiKeyInvalidatorTask,
Expand Down Expand Up @@ -305,6 +305,7 @@ export class AlertingPlugin {
alertsService: this.alertsService,
minimumScheduleInterval: this.config.rules.minimumScheduleInterval,
inMemoryMetrics: this.inMemoryMetrics,
latestRuleVersion: getLatestRuleVersion(),
});
this.ruleTypeRegistry = ruleTypeRegistry;

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/rule_type_registry.mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const createRuleTypeRegistryMock = () => {
list: jest.fn(),
getAllTypes: jest.fn(),
ensureRuleTypeEnabled: jest.fn(),
getLatestRuleVersion: jest.fn(),
};
return mocked;
};
Expand Down
11 changes: 11 additions & 0 deletions x-pack/plugins/alerting/server/rule_type_registry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ beforeEach(() => {
licensing: licensingMock.createSetup(),
minimumScheduleInterval: { value: '1m', enforce: false },
inMemoryMetrics,
latestRuleVersion: 1,
};
});

Expand Down Expand Up @@ -912,6 +913,16 @@ describe('Create Lifecycle', () => {
).toThrowErrorMatchingInlineSnapshot(`"Fail"`);
});
});

describe('getLatestRuleVersion', () => {
test('should return the latest rule version', async () => {
const ruleTypeRegistry = new RuleTypeRegistry({
...ruleTypeRegistryParams,
latestRuleVersion: 5,
});
expect(ruleTypeRegistry.getLatestRuleVersion()).toBe(5);
});
});
});

function ruleTypeWithVariables<ActionGroupIds extends string>(
Expand Down
12 changes: 10 additions & 2 deletions x-pack/plugins/alerting/server/rule_type_registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { Logger } from '@kbn/core/server';
import { LicensingPluginSetup } from '@kbn/licensing-plugin/server';
import { RunContext, TaskManagerSetupContract } from '@kbn/task-manager-plugin/server';
import { stateSchemaByVersion } from '@kbn/alerting-state-types';
import { rawRuleSchema } from './raw_rule_schema';
import { TaskRunnerFactory } from './task_runner';
import {
RuleType,
Expand All @@ -40,6 +39,7 @@ import { AlertingRulesConfig } from '.';
import { AlertsService } from './alerts_service/alerts_service';
import { getRuleTypeIdValidLegacyConsumers } from './rule_type_registry_deprecated_consumers';
import { AlertingConfig } from './config';
import { rawRuleSchemaV1 } from './saved_objects/schemas/raw_rule';

export interface ConstructorOptions {
config: AlertingConfig;
Expand All @@ -51,6 +51,7 @@ export interface ConstructorOptions {
minimumScheduleInterval: AlertingRulesConfig['minimumScheduleInterval'];
inMemoryMetrics: InMemoryMetrics;
alertsService: AlertsService | null;
latestRuleVersion: number;
}

export interface RegistryRuleType
Expand Down Expand Up @@ -160,6 +161,7 @@ export class RuleTypeRegistry {
private readonly licensing: LicensingPluginSetup;
private readonly inMemoryMetrics: InMemoryMetrics;
private readonly alertsService: AlertsService | null;
private readonly latestRuleVersion: number;

constructor({
config,
Expand All @@ -171,6 +173,7 @@ export class RuleTypeRegistry {
minimumScheduleInterval,
inMemoryMetrics,
alertsService,
latestRuleVersion,
}: ConstructorOptions) {
this.config = config;
this.logger = logger;
Expand All @@ -181,6 +184,7 @@ export class RuleTypeRegistry {
this.minimumScheduleInterval = minimumScheduleInterval;
this.inMemoryMetrics = inMemoryMetrics;
this.alertsService = alertsService;
this.latestRuleVersion = latestRuleVersion;
}

public has(id: string) {
Expand Down Expand Up @@ -311,7 +315,7 @@ export class RuleTypeRegistry {
spaceId: schema.string(),
consumer: schema.maybe(schema.string()),
}),
indirectParamsSchema: rawRuleSchema,
indirectParamsSchema: rawRuleSchemaV1,
},
});

Expand Down Expand Up @@ -434,6 +438,10 @@ export class RuleTypeRegistry {
public getAllTypes(): string[] {
return [...this.ruleTypes.keys()];
}

public getLatestRuleVersion() {
return this.latestRuleVersion;
}
}

function normalizedActionVariables(actionVariables: RuleType['actionVariables']) {
Expand Down
3 changes: 3 additions & 0 deletions 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 @@ -24,10 +24,12 @@ import { getImportWarnings } from './get_import_warnings';
import { isRuleExportable } from './is_rule_exportable';
import { RuleTypeRegistry } from '../rule_type_registry';
export { partiallyUpdateAlert } from './partially_update_alert';
export { getLatestRuleVersion, getMinimumCompatibleVersion } from './rule_model_versions';
import {
RULES_SETTINGS_SAVED_OBJECT_TYPE,
MAINTENANCE_WINDOW_SAVED_OBJECT_TYPE,
} from '../../common';
import { ruleModelVersions } from './rule_model_versions';

// Use caution when removing items from this array! Any field which has
// ever existed in the rule SO must be included in this array to prevent
Expand Down Expand Up @@ -106,6 +108,7 @@ export function setupSavedObjects(
return isRuleExportable(ruleSavedObject, ruleTypeRegistry, logger);
},
},
modelVersions: ruleModelVersions,
});

savedObjects.registerType({
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/saved_objects/is_rule_exportable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ beforeEach(() => {
licensing: licensingMock.createSetup(),
minimumScheduleInterval: { value: '1m', enforce: false },
inMemoryMetrics,
latestRuleVersion: 1,
};
});

Expand Down
45 changes: 45 additions & 0 deletions x-pack/plugins/alerting/server/saved_objects/rule_model_versions.ts
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is used by Kibana to validate the saved_object just before saving it, we can remove the below schema from createRule method of the rulesClient.

https://github.com/elastic/kibana/blob/main/x-pack/plugins/alerting/server/application/rule/methods/create/schemas/create_rule_data_schema.ts

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import {
SavedObjectsModelVersion,
SavedObjectsModelVersionMap,
} from '@kbn/core-saved-objects-server';
import { RawRule } from '../types';
import { rawRuleSchemaV1 } from './schemas/raw_rule';

interface CustomSavedObjectsModelVersion extends SavedObjectsModelVersion {
isCompatibleWithPreviousVersion: (param: RawRule) => boolean;
}

interface CustomSavedObjectsModelVersionMap extends SavedObjectsModelVersionMap {
[modelVersion: string]: CustomSavedObjectsModelVersion;
}

export const ruleModelVersions: CustomSavedObjectsModelVersionMap = {
'1': {
changes: [],
schemas: {
create: rawRuleSchemaV1,
},
isCompatibleWithPreviousVersion: (rawRule) => true,
},
};

export const getLatestRuleVersion = () => Math.max(...Object.keys(ruleModelVersions).map(Number));

export function getMinimumCompatibleVersion(version: number, rawRule: RawRule): number {
if (version === 1) {
return 1;
}

if (ruleModelVersions[version].isCompatibleWithPreviousVersion(rawRule)) {
return getMinimumCompatibleVersion(version - 1, rawRule);
}

return version;
}
8 changes: 8 additions & 0 deletions x-pack/plugins/alerting/server/saved_objects/schemas/raw_rule/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export { rawRuleSchema as rawRuleSchemaV1 } from './v1';
5 changes: 4 additions & 1 deletion ...lugins/alerting/server/raw_rule_schema.ts → ...rver/saved_objects/schemas/raw_rule/v1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ const rawRuleAlertsFilterSchema = schema.object({
key: schema.maybe(schema.string()),
params: schema.maybe(schema.recordOf(schema.string(), schema.any())), // better type?
value: schema.maybe(schema.string()),
field: schema.maybe(schema.string()),
}),
$state: schema.maybe(
schema.object({
Expand Down Expand Up @@ -209,6 +210,7 @@ const rawRuleActionSchema = schema.object({
})
),
alertsFilter: schema.maybe(rawRuleAlertsFilterSchema),
useAlertDataForTemplate: schema.maybe(schema.boolean()),
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 is/was a new field and causes a validation error.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the purpose of field and useAlertDataForTemplate. Is for use in the future? It seems like it could have been a test you were running locally, didn't intend to commit, but you added a comment, so seems not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Someone added that new field and forgot to update the rawRuleSchema, therefore createRule method fails.
I added it to fix the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how they missed that, task execution would be skipped when that field is in any rule.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting! I wonder if it coincided with some other "leniency" PRs where we might have been more lenient in accepting some objects validating schemas (allowing extra fields, but ignoring).

I was going to suggest opening an issue to figure out how this happened, because it doesn't seem good. However, we ARE now catching it :-), so ... not sure it matters. Presumably we'd catch the next time this happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that's why we decided to use modelVersions. It will force developers to bump the version.
No overlooked new fields anymore :)

});

export const rawRuleSchema = schema.object({
Expand Down Expand Up @@ -266,5 +268,6 @@ export const rawRuleSchema = schema.object({
severity: schema.maybe(schema.string()),
})
),
params: schema.recordOf(schema.string(), schema.any()),
params: schema.recordOf(schema.string(), schema.maybe(schema.any())),
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why we needed the schema.maybe() - I don't think it needs to be, unless someone is referencing fields in pararms and the typing was messed up. And I don't see any references to the field in the PR ... But I don't think there's any harm in this either. I'm more curious than concerned. :-)

Though there is a different field params in this file ^^^ (above the new field property) which uses the same schema bits, but differently:

  params: schema.maybe(schema.recordOf(schema.string(), schema.any())), // better type?

I just tried the following, and it only failed on the last one - I wasn't sure about the second, but it appears to have validated .

  it('tests schema.any()', () => {
    const testSchema = schema.object({
      params: schema.recordOf(schema.string(), schema.maybe(schema.any())),
    });

    testSchema.validate({ params: { test: 'test' } });
    testSchema.validate({ params: { test: undefined } });
    testSchema.validate({ params: {} });
    testSchema.validate({});
  });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In one of the test a team uses the second. { params: { test: undefined } } and it blows up schema.any() :)

Copy link
Member

Choose a reason for hiding this comment

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

My "test" was using your code, I wanted to remove the schema.maybe() to see if the second would pass - but I believe you! :-) . Thx!

typeVersion: schema.maybe(schema.number()),
});