[Security Solution] Handle specific fields in /upgrade/_review endpoint and refactor diff logic to use Zod
#186615
Conversation
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
jpdjere
changed the title
/upgrade/_review endpoint and refactor diff logic to use Zod
jpdjere
commented
Jun 27, 2024
x-pack/plugins/security_solution/common/api/detection_engine/model/rule_schema/time_duration.ts
Show resolved
Hide resolved
jpdjere
force-pushed
the
handle-revew-fields
branch
from
330ba29
to
c3f7492
Compare
|
/ci |
|
/ci |
jpdjere
added
refactoring
backport:skip
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
|
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
jpdjere
added
release_note:skip
jpdjere
force-pushed
the
handle-revew-fields
branch
from
1afbb00
to
9fefee6
Compare
banderror
approved these changes
Jul 10, 2024
Comment on lines
+40
to
+61
| // `response_actions` is only part of the optional fields in QueryRuleCreateFields and SavedQueryRuleCreateFields | ||
| const TYPE_SPECIFIC_PROPS_REMOVED_FROM_PREBUILT_RULE_ASSET = zodMaskFor< | ||
| QueryRuleCreateFields | SavedQueryRuleCreateFields | ||
| >()(['response_actions']); | ||
|
|
||
| const QueryRuleAssetFields = QueryRuleCreateFields.omit( | ||
| TYPE_SPECIFIC_PROPS_REMOVED_FROM_PREBUILT_RULE_ASSET | ||
| ); | ||
| const SavedQueryRuleAssetFields = SavedQueryRuleCreateFields.omit( | ||
| TYPE_SPECIFIC_PROPS_REMOVED_FROM_PREBUILT_RULE_ASSET | ||
| ); | ||
|
|
||
| export const RuleAssetTypeSpecificCreateProps = z.discriminatedUnion('type', [ | ||
| EqlRuleCreateFields, | ||
| QueryRuleAssetFields, | ||
| SavedQueryRuleAssetFields, | ||
| ThresholdRuleCreateFields, | ||
| ThreatMatchRuleCreateFields, | ||
| MachineLearningRuleCreateFields, | ||
| NewTermsRuleCreateFields, | ||
| EsqlRuleCreateFields, | ||
| ]); |
There was a problem hiding this comment.
The added complexity worries me a bit, + this is yet another place where we should add a new rule type once it is added to RuleResponse. Let's chat over zoom.
|
Four pending follow-ups from this PR, as discussed with @banderror during sync review:
|
This was referenced Jul 11, 2024
jpdjere
added a commit
that referenced
this pull request
Jul 12, 2024
…h Zod transform (#188092) ## Summary Pending work from: #186615 - The previous implementation to create `PrebuiltRuleAsset` with some RuleResponse fields ommited from it had the disadvantage of being built with a discriminated union where all rule types had to be re-listed. If a new type was added, then it would have required manually adding the type to that union as well, which would have been surely forgotten. - This replaces that schema construction to use a Zod transform which simply eliminates the omitted fields using a Zod transform. ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
jpdjere
added a commit
that referenced
this pull request
Jul 22, 2024
#188079) ## Summary Leftover work from #186615 - Removes remaining usage of `rule_schema_legacy` types. In this PR, simply inlines the last io-ts types used, to be able to get rid of the legacy folder. - The remaining files that need to be migrated to using Zod schema types are: - `x-pack/plugins/security_solution/common/api/detection_engine/rule_exceptions/find_exception_references/find_exception_references_route.ts` - `x-pack/plugins/security_solution/common/api/timeline/model/api.ts` ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes: #180393
Summary
Handles specific fields in
/upgrade/_reviewendpoint upgrade workflow, as described in #180393.Achieves this with two mechanisms:
PrebuiltRuleAssetschema, which excludes the field from the diff calculation completely./common/api/detection_engine/prebuilt_rules/model/diff/diffable_rule/diffable_rule.tsAlso, refactors a part of the codebase from its prior usage of
io-tsschema types to use autogenerated Zod types.With this refactor, most of the
x-pack/plugins/security_solution/common/api/detection_engine/model/rule_schema_legacycould be deleted. Unluckily some of the types manually created there are still used in some complex types elsewhere, so I added a note to that file indicating that those should be migrated to Zod, so that the legacy folder can finally be deleted.Checklist
Delete any items that are not applicable to this PR.
For maintainers