-
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
[Security Solution] Extend upgrade perform endpoint logic #191439
Merged
jpdjere
merged 21 commits into
elastic:main
from
jpdjere:extend-upgrade-perform-endpoint-logic
Oct 16, 2024
+3,202
−268
Merged
Changes from 15 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
31fff5e
Extend /upgrade/_perform endpoint logic
jpdjere 0ebcd5e
Types
jpdjere 1a6cd6e
simplify createModifiedPrebuiltRuleAsset
maximpn be3f451
fix naming
maximpn 39c560c
simplify createFieldUpgradeSpecifier function
maximpn c205813
fix types
maximpn 7051ca0
Merge branch 'main' into extend-upgrade-perform-endpoint-logic
jpdjere a413000
Change index pattern of mock query rule
jpdjere 8dd3d12
Fix tests
jpdjere f319de8
Fix
jpdjere 9f04894
Revert change
jpdjere 3602f3a
Merge branch 'main' into extend-upgrade-perform-endpoint-logic
jpdjere 87bc2e9
Added index containing logs-test to update_actions failing test AND r…
jpdjere 05bc246
Fix export test
jpdjere b66d52e
Merge branch 'main' into extend-upgrade-perform-endpoint-logic
jpdjere d542461
Added tests about maintiating current values for specific fields
jpdjere aaada40
Merge branch 'main' into extend-upgrade-perform-endpoint-logic
jpdjere 0579da7
Merge branch 'main' into extend-upgrade-perform-endpoint-logic
jpdjere c244e70
fix types
jpdjere 5235856
Fix unrelated conflict
jpdjere 0612387
Merge branch 'main' into extend-upgrade-perform-endpoint-logic
jpdjere File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,30 +11,65 @@ import { RuleResponse } from '../../model/rule_schema/rule_schemas.gen'; | |||||
import { AggregatedPrebuiltRuleError, DiffableAllFields } from '../model'; | ||||||
import { RuleSignatureId, RuleVersion } from '../../model'; | ||||||
|
||||||
export type Mode = z.infer<typeof Mode>; | ||||||
export const Mode = z.enum(['ALL_RULES', 'SPECIFIC_RULES']); | ||||||
export type ModeEnum = typeof Mode.enum; | ||||||
export const ModeEnum = Mode.enum; | ||||||
|
||||||
export type PickVersionValues = z.infer<typeof PickVersionValues>; | ||||||
export const PickVersionValues = z.enum(['BASE', 'CURRENT', 'TARGET', 'MERGED']); | ||||||
export type PickVersionValuesEnum = typeof PickVersionValues.enum; | ||||||
export const PickVersionValuesEnum = PickVersionValues.enum; | ||||||
|
||||||
// Specific handling of special fields according to: | ||||||
// https://github.com/elastic/kibana/issues/186544 | ||||||
export const FIELDS_TO_UPGRADE_TO_CURRENT_VERSION = [ | ||||||
'enabled', | ||||||
'exceptions_list', | ||||||
'alert_suppression', | ||||||
'actions', | ||||||
'throttle', | ||||||
'response_actions', | ||||||
'meta', | ||||||
'output_index', | ||||||
'namespace', | ||||||
'alias_purpose', | ||||||
'alias_target_id', | ||||||
'outcome', | ||||||
'concurrent_searches', | ||||||
'items_per_search', | ||||||
] as const; | ||||||
|
||||||
export const NON_UPGRADEABLE_DIFFABLE_FIELDS = [ | ||||||
'type', | ||||||
'rule_id', | ||||||
'version', | ||||||
'author', | ||||||
'license', | ||||||
] as const; | ||||||
|
||||||
type NON_UPGRADEABLE_DIFFABLE_FIELDS_TO_OMIT_TYPE = { | ||||||
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. not: |
||||||
readonly [key in (typeof NON_UPGRADEABLE_DIFFABLE_FIELDS)[number]]: true; | ||||||
}; | ||||||
|
||||||
// This transformation is needed to have Zod's `omit` accept the rule fields that need to be omitted | ||||||
export const DiffableFieldsToOmit = NON_UPGRADEABLE_DIFFABLE_FIELDS.reduce((acc, field) => { | ||||||
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.
Suggested change
|
||||||
return { ...acc, [field]: true }; | ||||||
}, {} as NON_UPGRADEABLE_DIFFABLE_FIELDS_TO_OMIT_TYPE); | ||||||
|
||||||
/** | ||||||
* Fields upgradable by the /upgrade/_perform endpoint. | ||||||
* Specific fields are omitted because they are not upgradeable, and | ||||||
* handled under the hood by endpoint logic. | ||||||
* See: https://github.com/elastic/kibana/issues/186544 | ||||||
*/ | ||||||
export type DiffableUpgradableFields = z.infer<typeof DiffableUpgradableFields>; | ||||||
export const DiffableUpgradableFields = DiffableAllFields.omit({ | ||||||
type: true, | ||||||
rule_id: true, | ||||||
version: true, | ||||||
author: true, | ||||||
license: true, | ||||||
}); | ||||||
export const DiffableUpgradableFields = DiffableAllFields.omit(DiffableFieldsToOmit); | ||||||
|
||||||
export type FieldUpgradeSpecifier<T> = z.infer< | ||||||
ReturnType<typeof fieldUpgradeSpecifier<z.ZodType<T>>> | ||||||
>; | ||||||
const fieldUpgradeSpecifier = <T extends z.ZodTypeAny>(fieldSchema: T) => | ||||||
export const fieldUpgradeSpecifier = <T extends z.ZodTypeAny>(fieldSchema: T) => | ||||||
z.discriminatedUnion('pick_version', [ | ||||||
z | ||||||
.object({ | ||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
60 changes: 60 additions & 0 deletions
60
...ne/prebuilt_rules/api/perform_rule_upgrade/assert_diffable_fields_match_rule_type.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/* | ||
* 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 { assertDiffableFieldsMatchRuleType } from './assert_diffable_fields_match_rule_type'; | ||
import { DIFFABLE_RULE_TYPE_FIELDS_MAP } from '../../../../../../common/api/detection_engine'; | ||
|
||
describe('assertDiffableFieldsMatchRuleType', () => { | ||
describe('valid scenarios -', () => { | ||
it('should validate all fields in DIFFABLE_RULE_TYPE_FIELDS_MAP', () => { | ||
DIFFABLE_RULE_TYPE_FIELDS_MAP.forEach((fields, ruleType) => { | ||
expect(() => { | ||
assertDiffableFieldsMatchRuleType(fields, ruleType); | ||
}).not.toThrow(); | ||
}); | ||
}); | ||
|
||
it('should not throw an error for valid upgradeable fields', () => { | ||
expect(() => { | ||
assertDiffableFieldsMatchRuleType(['name', 'description', 'severity'], 'query'); | ||
}).not.toThrow(); | ||
}); | ||
|
||
it('should handle valid rule type correctly', () => { | ||
expect(() => { | ||
assertDiffableFieldsMatchRuleType(['eql_query'], 'eql'); | ||
}).not.toThrow(); | ||
}); | ||
|
||
it('should handle empty upgradeable fields array', () => { | ||
expect(() => { | ||
assertDiffableFieldsMatchRuleType([], 'query'); | ||
}).not.toThrow(); | ||
}); | ||
}); | ||
|
||
describe('invalid scenarios -', () => { | ||
it('should throw an error for invalid upgradeable fields', () => { | ||
expect(() => { | ||
assertDiffableFieldsMatchRuleType(['invalid_field'], 'query'); | ||
}).toThrow("invalid_field is not a valid upgradeable field for type 'query'"); | ||
}); | ||
|
||
it('should throw for incompatible rule types', () => { | ||
expect(() => { | ||
assertDiffableFieldsMatchRuleType(['eql_query'], 'query'); | ||
}).toThrow("eql_query is not a valid upgradeable field for type 'query'"); | ||
}); | ||
|
||
it('should throw an error for an unknown rule type', () => { | ||
expect(() => { | ||
// @ts-expect-error - unknown rule | ||
assertDiffableFieldsMatchRuleType(['name'], 'unknown_type'); | ||
}).toThrow(); | ||
}); | ||
}); | ||
}); |
40 changes: 40 additions & 0 deletions
40
..._engine/prebuilt_rules/api/perform_rule_upgrade/assert_diffable_fields_match_rule_type.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
/* | ||
* 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 type { DiffableRuleTypes } from '../../../../../../common/api/detection_engine'; | ||
import { DIFFABLE_RULE_TYPE_FIELDS_MAP } from '../../../../../../common/api/detection_engine'; | ||
|
||
/** | ||
* Validates that the upgradeable (diffable) fields match the target rule type's diffable fields. | ||
* | ||
* This function is used in the rule upgrade process to ensure that the fields | ||
* specified for upgrade in the request body are valid for the target rule type. | ||
* It checks each upgradeable field provided in body.rule[].fields against the | ||
* set of diffable fields for the target rule type. | ||
* | ||
* @param {string[]} diffableFields - An array of field names to be upgraded. | ||
* @param {string} ruleType - A rule type (e.g., 'query', 'eql', 'machine_learning'). | ||
* @throws {Error} If an upgradeable field is not valid for the target rule type. | ||
* | ||
* @examples | ||
* assertDiffableFieldsMatchRuleType(['kql_query', 'severity'], 'query'); | ||
* assertDiffableFieldsMatchRuleType(['esql_query', 'description'], 'esql'); | ||
* assertDiffableFieldsMatchRuleType(['machine_learning_job_id'], 'eql'); // throws error | ||
* | ||
* @see {@link DIFFABLE_RULE_TYPE_FIELDS_MAP} in diffable_rule.ts for the mapping of rule types to their diffable fields. | ||
*/ | ||
export const assertDiffableFieldsMatchRuleType = ( | ||
diffableFields: string[], | ||
ruleType: DiffableRuleTypes | ||
) => { | ||
const diffableFieldsForType = new Set(DIFFABLE_RULE_TYPE_FIELDS_MAP.get(ruleType)); | ||
for (const diffableField of diffableFields) { | ||
if (!diffableFieldsForType.has(diffableField)) { | ||
throw new Error(`${diffableField} is not a valid upgradeable field for type '${ruleType}'`); | ||
} | ||
} | ||
}; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
nit: There is no benefit having a
Map
instead of aRecord
. I'd recommend defining as a readonlyRecord
like below since it's simplerThere 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 there is a benefit. By having a Map with a specific type as here, when I later do:
the types is maintained.
If I create it as a Record, and do:
then the
ruleType
is simply astring
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.
vs.