-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
Add rule model versions in alerting #171927
Changes from all commits
b070ada
2c916a3
e9f502d
77496a5
8612f9e
72ecc73
52a33ff
3a74ac3
c893a8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
/* | ||
* 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 { | ||
CustomSavedObjectsModelVersionMap, | ||
getLatestRuleVersion, | ||
getMinimumCompatibleVersion, | ||
} from './rule_model_versions'; | ||
import { schema } from '@kbn/config-schema'; | ||
import { RawRule } from '../types'; | ||
|
||
describe('rule model versions', () => { | ||
const ruleModelVersions: CustomSavedObjectsModelVersionMap = { | ||
'1': { | ||
changes: [], | ||
schemas: { | ||
create: schema.object({ | ||
name: schema.string(), | ||
}), | ||
}, | ||
isCompatibleWithPreviousVersion: (rawRule) => true, | ||
}, | ||
'2': { | ||
changes: [], | ||
schemas: { | ||
create: schema.object({ | ||
name: schema.string(), | ||
}), | ||
}, | ||
isCompatibleWithPreviousVersion: (rawRule) => false, | ||
}, | ||
'3': { | ||
changes: [], | ||
schemas: { | ||
create: schema.object({ | ||
name: schema.string(), | ||
}), | ||
}, | ||
isCompatibleWithPreviousVersion: (rawRule) => rawRule.name === 'test', | ||
}, | ||
'4': { | ||
changes: [], | ||
schemas: { | ||
create: schema.object({ | ||
name: schema.string(), | ||
}), | ||
}, | ||
isCompatibleWithPreviousVersion: (rawRule) => rawRule.name === 'test', | ||
}, | ||
}; | ||
|
||
const rawRule = { name: 'test' } as RawRule; | ||
const mismatchingRawRule = { enabled: true } as RawRule; | ||
|
||
describe('getMinimumCompatibleVersion', () => { | ||
it('should return the minimum compatible version for the matching rawRule', () => { | ||
expect(getMinimumCompatibleVersion(ruleModelVersions, 1, rawRule)).toBe(1); | ||
expect(getMinimumCompatibleVersion(ruleModelVersions, 2, rawRule)).toBe(2); | ||
expect(getMinimumCompatibleVersion(ruleModelVersions, 3, rawRule)).toBe(2); | ||
expect(getMinimumCompatibleVersion(ruleModelVersions, 4, rawRule)).toBe(2); | ||
}); | ||
it('should return the minimum compatible version for the mismatching rawRule', () => { | ||
expect(getMinimumCompatibleVersion(ruleModelVersions, 3, mismatchingRawRule)).toBe(3); | ||
expect(getMinimumCompatibleVersion(ruleModelVersions, 4, mismatchingRawRule)).toBe(4); | ||
}); | ||
}); | ||
|
||
describe('getLatestRuleVersion', () => { | ||
it('should return the latest rule model version', () => { | ||
expect(getLatestRuleVersion()).toBe(1); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* 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; | ||
} | ||
|
||
export 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( | ||
modelVersions: CustomSavedObjectsModelVersionMap, | ||
version: number, | ||
rawRule: RawRule | ||
): number { | ||
if (version === 1) { | ||
return 1; | ||
} | ||
|
||
if (modelVersions[version].isCompatibleWithPreviousVersion(rawRule)) { | ||
return getMinimumCompatibleVersion(modelVersions, version - 1, rawRule); | ||
} | ||
|
||
return version; | ||
} |
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'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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({ | ||
|
@@ -209,6 +210,7 @@ const rawRuleActionSchema = schema.object({ | |
}) | ||
), | ||
alertsFilter: schema.maybe(rawRuleAlertsFilterSchema), | ||
useAlertDataForTemplate: schema.maybe(schema.boolean()), | ||
}); | ||
|
||
export const rawRuleSchema = schema.object({ | ||
|
@@ -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())), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious why we needed the Though there is a different field 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({});
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In one of the test a team uses the second. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My "test" was using your code, I wanted to remove the |
||
typeVersion: schema.maybe(schema.number()), | ||
}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
anduseAlertDataForTemplate
. 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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)