[Security Solution][Alerts] Remove legacy rules schema #137605

marshallmain · 2022-07-29T15:58:45Z

Summary

Follow up to #82462

Removes remaining usages of the legacy RulesSchema, replacing it with FullResponseSchema which is built from the single source of truth for rules schemas.

elasticmachine · 2022-08-23T15:43:43Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2022-08-23T15:43:44Z

Pinging @elastic/security-solution (Team: SecuritySolution)

jpdjere · 2022-08-24T15:41:16Z

x-pack/plugins/security_solution/common/detection_engine/schemas/response/rules_schema.mocks.ts

-export const getThreatMatchingSchemaMock = (anchorDate: string = ANCHOR_DATE): RulesSchema => {
+export const getThreatMatchingSchemaMock = (
+  anchorDate: string = ANCHOR_DATE
+): FullResponseSchema => {


Any reason why getThreatMatchingSchemaMock couldn't have return type of ThreatMatchResponseSchema?
As well as the above getRulesMlSchemaMock have a retrun type of MachineLearningResponseSchema?

Nope, good point - updated to use more specific return types throughout this file.

jpdjere · 2022-08-24T15:46:42Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/validate.ts

-    return validateNonExact(transformed, rulesSchema);
-  }
-};
-
 export const newTransformValidate = (


If we are getting rid of the original transformValidate, couldn't we rename newTransformValidate to transformValidate?

Yeah, updated to transformValidate

e40pud

lgtm

banderror · 2022-09-06T14:34:42Z

@elasticmachine merge upstream

banderror

That's exciting @marshallmain -- another improvement of the rule schemas! 🙌

Overall the PR looks great. Left a few comments, most of which are about reinforcing static types.

...plugins/security_solution/common/detection_engine/schemas/response/rules_bulk_schema.test.ts

banderror · 2022-09-07T11:13:49Z

x-pack/plugins/security_solution/common/detection_engine/schemas/response/rules_schema.mocks.ts

@@ -6,13 +6,18 @@
 */

 import { DEFAULT_INDICATOR_SOURCE_PATH } from '../../../constants';
+import type {


After the changes, now we have rules_schema.mocks.ts and rules_schema.test.ts under both directories: request and response. Some of the tests under response have been deleted.

What's the difference between those tests and mocks that are still there?

Can we consolidate all of them under the request folder?

What's the difference between those tests and mocks that are still there?

The tests and mocks in each folder still correspond to the request and response schemas respectively. It definitely deserves to be reorganized, but I tried to avoid doing reorganization at the same time as the more technical changes. request/rule_schemas.test.ts is focused on testing the createRulesSchema - we don't have independent tests for updateRulesSchema at this point, but it's almost identical to createRulesSchema. Similarly the mocks in request/rule_schemas.mock.ts are all for the create schemas.

Since request/rule_schemas.ts is where all of the core schema logic lives, it ends up being the source for the new response schemas and replaces response/rules_schema.ts - which is simply deleted. However, I kept response/rules_schema.test.ts and response/rules_schema.mock.ts and updated them to use the new schemas for better assurance that the switch over to the new schema isn't breaking things.

Can we consolidate all of them under the request folder?

I think we can consolidate by moving most of request/rule_schemas.ts to common/rule_schemas.ts and better separating the "base" and "type specific" schema logic from create, update, response, etc. This way we can keep request and response schemas organized into their own sections, and also remove the dependency of response on request/rule_schemas.ts.

Great, thank you for clarifying this! Could you please add a few comments to those files to explain this:

The tests and mocks in each folder still correspond to the request and response schemas respectively. It definitely deserves to be reorganized, but I tried to avoid doing reorganization at the same time as the more technical changes. request/rule_schemas.test.ts is focused on testing the createRulesSchema - we don't have independent tests for updateRulesSchema at this point, but it's almost identical to createRulesSchema. Similarly the mocks in request/rule_schemas.mock.ts are all for the create schemas.

Since request/rule_schemas.ts is where all of the core schema logic lives, it ends up being the source for the new response schemas and replaces response/rules_schema.ts - which is simply deleted. However, I kept response/rules_schema.test.ts and response/rules_schema.mock.ts and updated them to use the new schemas for better assurance that the switch over to the new schema isn't breaking things.

I think we can consolidate by moving most of request/rule_schemas.ts to common/rule_schemas.ts and better separating the "base" and "type specific" schema logic from create, update, response, etc. This way we can keep request and response schemas organized into their own sections, and also remove the dependency of response on request/rule_schemas.ts.

++ Makes total sense. I added this to #138606 as a sub-task.

x-pack/plugins/security_solution/common/detection_engine/schemas/response/rules_schema.mocks.ts

banderror · 2022-09-07T11:19:00Z

x-pack/plugins/security_solution/common/detection_engine/schemas/request/rule_schemas.ts

+const sharedResponseSchema = t.intersection([
  baseResponseParams,
-  responseTypeSpecific,
  t.exact(t.type(responseRequiredFields)),
  t.exact(t.partial(responseOptionalFields)),
 ]);
+type SharedResponseSchema = t.TypeOf<typeof sharedResponseSchema>;


Nit: let's use either base or shared naming but not both

sharedResponseSchema here is equivalent to sharedCreateSchema or sharedUpdateSchema but for the response instead, whereas base comes from the baseParams that form the basis of the create, update, patch, and response schemas. What do you think we should rename here?

I'd probably rename baseParams to sharedParams etc. But I'm nitpicking here, the naming as it is now is clear enough, so feel free to ignore the suggestion.

x-pack/plugins/security_solution/cypress/objects/rule.ts

x-pack/test/detection_engine_api_integration/utils/get_simple_rule_output.ts

...k/test/detection_engine_api_integration/utils/get_simple_rule_output_with_web_hook_action.ts

x-pack/test/detection_engine_api_integration/utils/get_simple_rule_output_without_rule_id.ts

x-pack/test/detection_engine_api_integration/utils/remove_server_generated_properties.ts

banderror · 2022-09-07T12:12:10Z

x-pack/test/detection_engine_api_integration/utils/remove_server_generated_properties.ts

+  // We're only removing undefined values, so this cast correctly narrows the type
+  return pickBy(removedProperties, (value) => value !== undefined) as Omit<


Why removing undefined values is necessary?

We have to remove them because in the switch to FullResponseSchema, the mock data now has to specify fields that are undefined explicitly (e.g. https://github.com/elastic/kibana/pull/137605/files#diff-d162965f34c11868eafde7e03ca9bfe234c93845bc38bc7c441494b4d7cf5ca9R47-R59), but when we compare the mocks to actual outputs in the integration tests the extra undefined fields cause the equality check to fail.

This should be fixed as a follow up when we make a response type that does not require all fields to be specified, but for now I figured getting everything onto the same base set of schemas should happen first, then we can continue to expand the capabilities of the schema definition system.

but when we compare the mocks to actual outputs in the integration tests the extra undefined fields cause the equality check to fail.

Ah, I see! Thank you.

This should be fixed as a follow up when we make a response type that does not require all fields to be specified

FWIW I think the problem could be in assertions rather than mocks. Instead of expect(<actual>).toEqual(<expected>) we could assert only a subset of the properties that would be relevant to each particular test: expect(<actual>).toEqual(expect.objectContaining(<expected subset>)).

Anyway, that would be a separate story to work on...

…Properties

banderror

@marshallmain Thank you for addressing the comments! The rest of them are nits.
🚀

…ules-schema

kibana-ci · 2022-09-12T17:00:07Z

💚 Build Succeeded

Buildkite Build
Commit: 1649b84

Metrics [docs]

✅ unchanged

History

💚 Build #70888 succeeded 560bb8e
💛 Build #70195 was flaky 0ecb956
💛 Build #69731 was flaky 2e7f0bc
💛 Build #68344 was flaky f43bedc
💚 Build #67255 succeeded 147b8bd
💚 Build #67197 succeeded 17cddda

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Remove legacy rules schema

845f152

marshallmain and others added 9 commits August 1, 2022 06:59

Remove RulesSchema from cypress test

38317c1

Merge branch 'main' into remove-legacy-rules-schema

07c2e3c

Fix ml rule API test mock

4c01f5b

Fix type error

7a79e6e

Merge branch 'main' into remove-legacy-rules-schema

44ab4a9

Explicitly specify undefined fields in response mocks

5a0a141

Merge branch 'main' into remove-legacy-rules-schema

63f7ed1

Fix types

b64f9fa

Remove undefined properties so expect doesn't mess it up

9b8ffc6

marshallmain marked this pull request as ready for review August 23, 2022 15:43

marshallmain requested review from a team as code owners August 23, 2022 15:43

marshallmain requested a review from jpdjere August 23, 2022 15:43

jpdjere reviewed Aug 24, 2022

View reviewed changes

banderror self-requested a review August 24, 2022 16:04

marshallmain and others added 3 commits August 24, 2022 09:11

Use more specific types and rename newTransformValidate

9ca181f

Fix types in schema tests

17cddda

Merge branch 'main' into remove-legacy-rules-schema

147b8bd

Merge branch 'main' into remove-legacy-rules-schema

f43bedc

e40pud approved these changes Aug 30, 2022

View reviewed changes

Merge branch 'main' into remove-legacy-rules-schema

2e7f0bc

banderror reviewed Sep 7, 2022

View reviewed changes

banderror mentioned this pull request Sep 7, 2022

[Security Solution] Rule schemas tech debt #138606

Open

10 tasks

marshallmain added 2 commits September 7, 2022 10:11

Review comments - explicit return types, better removeServerGenerated…

5d3bd8a

…Properties

Re-add deleted test with modified assertion

0ecb956

banderror approved these changes Sep 8, 2022

View reviewed changes

marshallmain and others added 2 commits September 9, 2022 07:57

Merge branch 'main' into remove-legacy-rules-schema

560bb8e

Merge branch 'main' of github.com:elastic/kibana into remove-legacy-r…

1649b84

…ules-schema

semd approved these changes Sep 13, 2022

View reviewed changes

marshallmain merged commit 24d4865 into elastic:main Sep 13, 2022

kibanamachine added the backport:skip This commit does not require backporting label Sep 13, 2022

marshallmain deleted the remove-legacy-rules-schema branch September 13, 2022 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Alerts] Remove legacy rules schema #137605

[Security Solution][Alerts] Remove legacy rules schema #137605

marshallmain commented Jul 29, 2022

elasticmachine commented Aug 23, 2022

elasticmachine commented Aug 23, 2022

jpdjere Aug 24, 2022 •

edited

Loading

marshallmain Aug 24, 2022

jpdjere Aug 24, 2022

marshallmain Aug 24, 2022

e40pud left a comment

banderror commented Sep 6, 2022

banderror left a comment

banderror Sep 7, 2022

marshallmain Sep 7, 2022

banderror Sep 8, 2022

banderror Sep 8, 2022

banderror Sep 7, 2022

marshallmain Sep 7, 2022

banderror Sep 8, 2022

banderror Sep 7, 2022

marshallmain Sep 7, 2022

banderror Sep 8, 2022

banderror left a comment

kibana-ci commented Sep 12, 2022

		// We're only removing undefined values, so this cast correctly narrows the type
		return pickBy(removedProperties, (value) => value !== undefined) as Omit<

[Security Solution][Alerts] Remove legacy rules schema #137605

[Security Solution][Alerts] Remove legacy rules schema #137605

Conversation

marshallmain commented Jul 29, 2022

Summary

elasticmachine commented Aug 23, 2022

elasticmachine commented Aug 23, 2022

jpdjere Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e40pud left a comment

Choose a reason for hiding this comment

banderror commented Sep 6, 2022

banderror left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 12, 2022

💚 Build Succeeded

Metrics [docs]

History

jpdjere Aug 24, 2022 •

edited

Loading