[Security Solution] Migrate remaining public Detection Engine APIs to OpenAPI and code generation #170330
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
xcrzx
added
Team:Detections and Resp
xcrzx
force-pushed
the
openapi-migration-v2
branch
12 times, most recently
from
3ebb54d to
e9ec18a
Compare
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
xcrzx
changed the title
xcrzx
added
release_note:skip
xcrzx
force-pushed
the
openapi-migration-v2
branch
from
e9ec18a to
899bf8e
Compare
Files by Code Ownerelastic/security-defend-workflows
elastic/security-detection-engine
elastic/security-detection-rule-management
elastic/security-entity-analytics
elastic/security-solution
elastic/security-threat-hunting-investigations
|
tomsonpl
approved these changes
Nov 7, 2023
| @@ -47,13 +38,13 @@ export const OsqueryParamsCamelCase = t.type({ | |||
| // When we create new response action types, create a union of types | |||
| export type RuleResponseOsqueryAction = t.TypeOf<typeof RuleResponseOsqueryAction>; | |||
| export const RuleResponseOsqueryAction = t.strict({ | |||
| actionTypeId: t.literal(RESPONSE_ACTION_TYPES.OSQUERY), | |||
| actionTypeId: t.literal('.osquery'), | |||
There was a problem hiding this comment.
can we use ResponseActionTypesEnum.OSQUERY , or at least ResponseActionTypesEnum['.osquery'] ?
There was a problem hiding this comment.
Since this is a legacy schema set for removal in upcoming PRs, I've inlined the literal to prevent creating dependencies between the new and the old schemas.
| @@ -85,7 +90,7 @@ export const transformRuleToAlertResponseAction = ({ | |||
| action_type_id: actionTypeId, | |||
| params, | |||
| }: ResponseAction): RuleResponseAction => { | |||
| if (actionTypeId === RESPONSE_ACTION_TYPES.OSQUERY) { | |||
| if (actionTypeId === ResponseActionTypesEnum['.osquery']) { | |||
There was a problem hiding this comment.
why not ResponseActionTypesEnum.OSQUERY ?
There was a problem hiding this comment.
Since the OpenAPI schema is now the source of truth for data structures and enums in our codebase, I am replacing legacy structures with the generated ones. The generated enum looks as follows:
export const ResponseActionTypes = z.enum(['.osquery', '.endpoint']);
export type ResponseActionTypes = z.infer<typeof ResponseActionTypes>;
export const ResponseActionTypesEnum = ResponseActionTypes.enum;The enum keys mirror the enum values, which is why we reference them using ResponseActionTypesEnum['.osquery'].
But for convenience, we could introduce an alias like this and use it everywhere:
const RESPONSE_ACTION_TYPES = {
OSQUERY: ResponseActionTypesEnum['.osquery'],
ENDPOINT: ResponseActionTypesEnum['.endpoint'],
};I don't have a strong opinion on that tbh.
jpdjere
reviewed
Nov 7, 2023
| .default('false') | ||
| .transform((value) => value === 'true') | ||
| ), | ||
| exclude_export_details: BooleanFromString.optional().default(false), |
There was a problem hiding this comment.
So, just to understand what happened here:
You created the BooleanFromString helper in this same PR, but the schema file for export_rules_route wasn't updated in any way.
So how did this get updated? Did you do it manually?
There was a problem hiding this comment.
I've updated the code generator for booleans and arrays in request queries. You can find the changes in the template file at packages/kbn-openapi-generator/src/template_service/templates/zod_query_item.handlebars.
jpdjere
reviewed
Nov 7, 2023
Comment on lines
85
to
98
| - 'created_at' | ||
| - 'createdAt' | ||
| - 'enabled' | ||
| - 'execution_summary.last_execution.date' | ||
| - 'execution_summary.last_execution.metrics.execution_gap_duration_s' | ||
| - 'execution_summary.last_execution.metrics.total_indexing_duration_ms' | ||
| - 'execution_summary.last_execution.metrics.total_search_duration_ms' | ||
| - 'execution_summary.last_execution.status' | ||
| - 'name' | ||
| - 'risk_score' | ||
| - 'riskScore' | ||
| - 'severity' | ||
| - 'updated_at' | ||
| - 'updatedAt' |
There was a problem hiding this comment.
Should we keep the comments here for the camelCase fields being legacy, and we're keeping them for backwards compatibility?
jpdjere
reviewed
Nov 7, 2023
|
|
||
| /** | ||
| * An array of supported log levels. | ||
| */ | ||
| export const LOG_LEVELS = Object.values(LogLevel); | ||
| export const LOG_LEVELS = LogLevel.options; |
There was a problem hiding this comment.
What's the purpose of this variable?
There was a problem hiding this comment.
It holds an array of all possible LogLevel values: ["trace", "debug", "info", "warn", "error"]
jpdjere
reviewed
Nov 7, 2023
| description: End date of the time range to query | ||
| schema: | ||
| type: string | ||
| format: date-time |
There was a problem hiding this comment.
Not modified in this PR, but I see that the parms sort_order, page and per_page are marked as required: false, while in GetRuleExecutionEventsRequestQuery in the deleted x-pack/plugins/security_solution/common/api/detection_engine/rule_monitoring/rule_execution_logs/get_rule_execution_events/get_rule_execution_events_route.ts they are not marked as partial.
I realize that these three fields have a default value here in the schema, but just wanted to note the difference in the schemas.
There was a problem hiding this comment.
Yeah, it looks like a mistake in the previous schema. Defaultable fields should not be required
jpdjere
reviewed
Nov 7, 2023
| */ | ||
| import type { ErrorSchema } from './error_schema'; | ||
|
|
||
| export const getErrorSchemaMock = ( |
There was a problem hiding this comment.
Oh, there was a wrong import. It is used in adjacent error_schema.test.ts.
jpdjere
approved these changes
Nov 7, 2023
janmonschke
reviewed
Nov 8, 2023
Comment on lines
29
to
34
| page: Page.optional(), | ||
| per_page: PerPage.optional(), | ||
| /** | ||
| * Total number of items | ||
| */ | ||
| total: z.number().int().min(0).optional(), |
There was a problem hiding this comment.
@xcrzx Are all of these really optional?
There was a problem hiding this comment.
They should not be optional, and thanks for pointing that out! 👍
xcrzx
force-pushed
the
openapi-migration-v2
branch
from
899bf8e to
68464e2
Compare
janmonschke
approved these changes
Nov 8, 2023
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @xcrzx |
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.
Related to: https://github.com/elastic/security-team/issues/7491
Summary
Migrated remaining public Detection Engine endpoints to OpenAPI schema and code generation:
POST /api/detection_engine/rules/_bulk_actionGET /api/detection_engine/rules/_findAlso completed the migration of internal APIs:
GET /internal/detection_engine/rules/{ruleId}/execution/eventsGET /internal/detection_engine/rules/{ruleId}/execution/resultsOther notable changes
packages/kbn-zod-helpers/src/stringify_zod_error.ts. Now we are trying to list the validation errors of all union members but limiting the total number of validation errors displayed to users.TODO https://github.com/elastic/security-team/issues/7491BooleanFromStringandArrayFromString