[Security Solution] Add missing alerts (signals) API endpoints OpenAPI specs #184838

maximpn · 2024-06-05T13:35:22Z

Addresses: #183661

Summary

This PR adds missing OpenAPI specs for the following API endpoints (Alerts API and Alerts Migration API) available in both Serverless and ESS

POST /api/detection_engine/signals/status
POST /api/detection_engine/signals/tags
POST /api/detection_engine/signals/search

and API endpoints available only in ESS

POST /api/detection_engine/signals/migration_status
POST /api/detection_engine/signals/migration
POST /api/detection_engine/signals/finalize_migration
DELETE /api/detection_engine/signals/migration

maximpn · 2024-06-07T11:07:16Z

Hey @marcelhallmann,

Could you check added OpenAPI specs for correctness? Don't pay too much attention to summaries and descriptions since it will be further refined in #183702. I used some information from the documentation page.

elasticmachine · 2024-06-07T11:07:30Z

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

elasticmachine · 2024-06-07T11:07:31Z

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

elasticmachine · 2024-06-07T11:07:32Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

elasticmachine · 2024-06-07T11:07:33Z

Pinging @elastic/security-detection-engine (Team:Detection Engine)

marshallmain · 2024-06-07T17:51:30Z

x-pack/plugins/security_solution/common/api/detection_engine/model/error_response.schema.yaml

+paths: {}
+components:
+  schemas:
+    GenericErrorResponse:


I was confused here at first because the SiemResponseFactory uses status_code (https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts#L184) and message as the response if we throw during the executor and return a 500. Can we add a response schema for the siemResponse as well, and maybe rename this one to PlatformErrorResponse?

marshallmain · 2024-06-07T21:09:42Z

...lution/common/api/detection_engine/signals/set_signal_status/set_signals_status_route.gen.ts

+    conflicts: z.enum(['abort', 'proceed']).optional().default('abort'),
+  })
+  .merge(
+    z.union([


There's a type error here, can't merge a zod union with another object

marshallmain · 2024-06-07T21:12:54Z

.../detection_engine/signals_migration/create_signals_migration/create_signals_migration.gen.ts

+});
+
+export type CreateAlertsMigrationResponse = z.infer<typeof CreateAlertsMigrationResponse>;
+export const CreateAlertsMigrationResponse = z.object({


The migration response can have an error as part of the ok response: https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/create_signals_migration_route.ts#L142
Looks like this response should be a union of the error and regular responses

marshallmain · 2024-06-07T21:22:33Z

.../detection_engine/signals_migration/delete_signals_migration/delete_signals_migration.gen.ts

+
+export type CleanupAlertsMigrationRequestBody = z.infer<typeof CleanupAlertsMigrationRequestBody>;
+export const CleanupAlertsMigrationRequestBody = z.object({
+  migration_ids: z.array(z.string()).min(1).optional(),


This should be required

marshallmain · 2024-06-07T21:25:35Z

.../detection_engine/signals_migration/delete_signals_migration/delete_signals_migration.gen.ts

+      message: z.string().optional(),
+      status_code: z.number().int().optional(),


message and status_code within error are not optional, though error is optional

marshallmain · 2024-06-07T21:28:41Z

...gine/signals_migration/get_signals_migration_status/get_signals_migration_status.schema.yaml

+      tags:
+        - Alerts migration API
+      parameters:
+        - name: from


I don't see the codegen for this parameter?

It turns out request response types aren't generated when x-codegen-enabled is omitted but schemas are generated for common schemas. Since a valid spec may produce invalid code I explicitly added x-codegen-enabled: true.

marshallmain · 2024-06-07T21:29:44Z

...on_engine/signals_migration/get_signals_migration_status/get_signals_migration_status.gen.ts

+  index: NonEmptyString.optional(),
+  version: z.number().int().optional(),
+  signal_versions: z.array(AlertVersion).optional(),
+  migrations: z.array(MigrationStatus).optional(),
+  is_outdated: z.boolean().optional(),


Every field is required

marshallmain · 2024-06-07T21:30:17Z

...on_engine/signals_migration/get_signals_migration_status/get_signals_migration_status.gen.ts

+
+export type GetAlertsMigrationStatusResponse = z.infer<typeof GetAlertsMigrationStatusResponse>;
+export const GetAlertsMigrationStatusResponse = z.object({
+  indices: z.array(IndexMigrationStatus).optional(),


indices is required

marshallmain · 2024-06-07T21:30:51Z

...on_engine/signals_migration/get_signals_migration_status/get_signals_migration_status.gen.ts

+  version: z.number().int().optional(),
+  count: z.number().int().optional(),


These are required

marshallmain · 2024-06-07T21:34:25Z

...on_engine/signals_migration/get_signals_migration_status/get_signals_migration_status.gen.ts

+  id: NonEmptyString.optional(),
+  status: z.enum(['success', 'failure', 'pending']).optional(),
+  version: z.number().int().optional(),
+  updated: z.string().datetime().optional(),


All required

maximpn · 2024-06-08T10:03:51Z

@marshallmain thank you for the review!

I addressed your comments and also found a few things to fix. Explicitly enabled code generation via x-codegen-enabled: true forces code generator to produce request and response Zod schemas. I made it enabled in all added OAS files to verify generated code correctness. A valid OAS may produce an invalid TS file.

marshallmain

Looks much better, thanks for addressing the comments. I only see one remaining issue.

marshallmain · 2024-06-10T23:37:48Z

.../detection_engine/signals_migration/create_signals_migration/create_signals_migration.gen.ts

+  migration_id: z.string(),
+  migration_index: z.string(),


To match the API route behavior these should exist but be null for the error and skipped migration cases.

@marshallmain,

OAS doesn't provide a way to specify explicit null fields. There is a nullable modifier which marks a type as nullable. In this particular case it's possible to declare a single response type instead of AlertsIndexMigrationSuccess, AlertsIndexMigrationError and SkippedAlertsIndexMigration with migration_id and migration_index being string or null. Such schema won't describe the response precisely neither.

What's the approach you expect here?

We discussed on slack and the approach is ok as is. Better to leave the properties out of the response schema than have them typed incorrectly or make the rest of the schema less precise.

marshallmain

👍

kibana-ci · 2024-06-18T15:42:59Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 9ffe520

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	5507	5506	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	13.6MB	13.6MB	+9.5KB

History

💔 Build #216327 failed 9ffe520
💔 Build #216313 failed 5ace3d866014f4c988fcf4888cbf36bf2dec8625
💔 Build #216295 failed a6e7e34508063ffc0a27abf1d11fb22151ccb499
💔 Build #216167 failed 7ef4e7fff42376cf205c32a6bf0c113ff5f25bc3
💔 Build #216039 failed e7928213502f89950d5261537f4c855f7ac6f0cd

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

cc @maximpn

banderror

Rule Management changes LGTM 👍

I did a high-level review of the whole diff and checked the changes owned by our team.

Can we please add the new folders to the CODEOWNERS file in a separate PR?

…I specs (elastic#184838) **Addresses:** elastic#183661 ## Summary This PR adds missing OpenAPI specs for the following API endpoints (Alerts API and Alerts Migration API) available in both Serverless and ESS - `POST /api/detection_engine/signals/status` - `POST /api/detection_engine/signals/tags` - `POST /api/detection_engine/signals/search` and API endpoints available only in ESS - `POST /api/detection_engine/signals/migration_status` - `POST /api/detection_engine/signals/migration` - `POST /api/detection_engine/signals/finalize_migration` - `DELETE /api/detection_engine/signals/migration` **Note:** Code generation is enabled for the added specs to verify that it works and produces expected results. Generated Zod schemas and types aren't integrated in the route's code.

maximpn self-assigned this Jun 5, 2024

maximpn force-pushed the add-missing-alerts-api-oas branch 2 times, most recently from 6a3688a to 54c379b Compare June 7, 2024 11:04

maximpn requested a review from marshallmain June 7, 2024 11:04

maximpn marked this pull request as ready for review June 7, 2024 11:07

maximpn requested review from a team as code owners June 7, 2024 11:07

maximpn requested a review from dplumlee June 7, 2024 11:07

maximpn removed the request for review from dplumlee June 7, 2024 11:08

maximpn force-pushed the add-missing-alerts-api-oas branch from 54c379b to 68e7e95 Compare June 7, 2024 13:37

marshallmain reviewed Jun 7, 2024

View reviewed changes

maximpn force-pushed the add-missing-alerts-api-oas branch from b5ac814 to 6952fc1 Compare June 8, 2024 10:00

maximpn requested a review from marshallmain June 8, 2024 10:03

maximpn force-pushed the add-missing-alerts-api-oas branch 2 times, most recently from 6759a2e to 25db960 Compare June 10, 2024 18:56

marshallmain reviewed Jun 10, 2024

View reviewed changes

maximpn force-pushed the add-missing-alerts-api-oas branch from 26a8c52 to a6e7e34 Compare June 18, 2024 06:02

maximpn marked this pull request as ready for review June 18, 2024 07:56

maximpn requested a review from a team as a code owner June 18, 2024 07:56

maximpn added 15 commits June 18, 2024 10:01

add missing alerts API OAS

8cbb1c3

add alerts migration API OAS

6535f89

refine added OAS specs

4af63c3

add serverless and ess labels

263ace8

rename GenericErrorResponse to PlatformErrorResponse

98e4ff4

add siemErrorResponse schema

216175f

fix SetAlertsStatusRequestBody

bea918b

fix /api/detection_engine/signals/migration response

7e21240

fix /api/detection_engine/signals/migration schema

d88b7d9

fix /api/detection_engine/signals/finalize_migration schema

231c54e

fix /api/detection_engine/signals/migration_status schema

7b3f8e7

make code generation produced correct zod schemas and types

00ad2fd

extend error response schemas

87100da

fix 500 error response schema description

2ce4470

add explicit code generation flag

d81b80f

maximpn force-pushed the add-missing-alerts-api-oas branch from a6e7e34 to 5ace3d8 Compare June 18, 2024 08:01

apply generated from OAS TS types

9ffe520

maximpn force-pushed the add-missing-alerts-api-oas branch from 5ace3d8 to 9ffe520 Compare June 18, 2024 08:56

maximpn requested a review from banderror June 18, 2024 10:50

marshallmain approved these changes Jun 18, 2024

View reviewed changes

banderror approved these changes Jun 18, 2024

View reviewed changes

maximpn merged commit b169a9c into elastic:main Jun 18, 2024
42 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jun 18, 2024

maximpn deleted the add-missing-alerts-api-oas branch June 18, 2024 15:54

maximpn mentioned this pull request Jun 18, 2024

[Security Solution] Create missing OpenAPI specs for Detections API #183661

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Add missing alerts (signals) API endpoints OpenAPI specs #184838

[Security Solution] Add missing alerts (signals) API endpoints OpenAPI specs #184838

maximpn commented Jun 5, 2024 •

edited

Loading

maximpn commented Jun 7, 2024

elasticmachine commented Jun 7, 2024

elasticmachine commented Jun 7, 2024

elasticmachine commented Jun 7, 2024

elasticmachine commented Jun 7, 2024

marshallmain Jun 7, 2024

marshallmain Jun 7, 2024

marshallmain Jun 7, 2024

marshallmain Jun 7, 2024

marshallmain Jun 7, 2024

marshallmain Jun 7, 2024

maximpn Jun 8, 2024 •

edited

Loading

marshallmain Jun 7, 2024

marshallmain Jun 7, 2024

marshallmain Jun 7, 2024

marshallmain Jun 7, 2024

maximpn commented Jun 8, 2024

marshallmain left a comment

marshallmain Jun 10, 2024

maximpn Jun 11, 2024

marshallmain Jun 18, 2024

marshallmain left a comment

kibana-ci commented Jun 18, 2024

banderror left a comment

		message: z.string().optional(),
		status_code: z.number().int().optional(),

		version: z.number().int().optional(),
		count: z.number().int().optional(),

[Security Solution] Add missing alerts (signals) API endpoints OpenAPI specs #184838

[Security Solution] Add missing alerts (signals) API endpoints OpenAPI specs #184838

Conversation

maximpn commented Jun 5, 2024 • edited Loading

Summary

maximpn commented Jun 7, 2024

elasticmachine commented Jun 7, 2024

elasticmachine commented Jun 7, 2024

elasticmachine commented Jun 7, 2024

elasticmachine commented Jun 7, 2024

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

maximpn Jun 8, 2024 • 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

Choose a reason for hiding this comment

maximpn commented Jun 8, 2024

marshallmain 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

marshallmain left a comment

Choose a reason for hiding this comment

kibana-ci commented Jun 18, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Async chunks

History

banderror left a comment

Choose a reason for hiding this comment

maximpn commented Jun 5, 2024 •

edited

Loading

maximpn Jun 8, 2024 •

edited

Loading