From 2171ecc719d14a8cf7b1b4c333a687eed5e61294 Mon Sep 17 00:00:00 2001 From: Dmitrii Shevchenko Date: Fri, 25 Aug 2023 20:01:31 +0200 Subject: [PATCH] [Security Solution] Initial migration of API endpoints to OpenAPI and code generation (#164482) **Part of: https://github.com/elastic/security-team/issues/6726** ## Summary Migrates the prebuilt rules and timelines status API route schema to OpenAPI. This is exploratory work to assess the level of effort required to migrate API route schemas from `io-ts` to `zod` generated by OpenAPI codegen. **Summary of the changes:** - Added a CI job that runs code generation in Security Solution and comments change if there are any. - Migrated the `/api/detection_engine/rules/prepackaged/_status` route to use generated `zod` schemas - Updated schema tests - Adjusted the code generator templates to handle `strict` schemas, i.e., schemas that do not allow any extra params - Updated the error transformation code to work with zod errors. Validation errors are converted to string representations, like the following: image --- .../pull_request/security_solution.yml | 15 +++- .buildkite/scripts/common/util.sh | 4 +- .../security_solution_codegen.sh | 12 +++ .../src/transform_error/index.ts | 22 +++++ ...lt_rules_and_timelines_status_route.gen.ts | 49 +++++++++++ ...les_and_timelines_status_route.schema.yaml | 5 +- ...t_rules_and_timelines_status_route.test.ts | 84 ++++++++----------- ...ebuilt_rules_and_timelines_status_route.ts | 25 ------ .../detection_engine/prebuilt_rules/index.ts | 2 +- .../common/test/zod_helpers.ts | 20 +++++ .../templates/schema_item.handlebars | 1 + ...ebuilt_rules_and_timelines_status_route.ts | 15 +--- 12 files changed, 159 insertions(+), 95 deletions(-) create mode 100755 .buildkite/scripts/steps/code_generation/security_solution_codegen.sh create mode 100644 x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.gen.ts delete mode 100644 x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts create mode 100644 x-pack/plugins/security_solution/common/test/zod_helpers.ts diff --git a/.buildkite/pipelines/pull_request/security_solution.yml b/.buildkite/pipelines/pull_request/security_solution.yml index 4c30d6a4356724..58b416548ec5f6 100644 --- a/.buildkite/pipelines/pull_request/security_solution.yml +++ b/.buildkite/pipelines/pull_request/security_solution.yml @@ -11,7 +11,7 @@ steps: - exit_status: '*' limit: 1 artifact_paths: - - "target/kibana-security-solution/**/*" + - 'target/kibana-security-solution/**/*' - command: .buildkite/scripts/steps/functional/security_solution_explore.sh label: 'Explore - Security Solution Cypress Tests' @@ -25,7 +25,7 @@ steps: - exit_status: '*' limit: 1 artifact_paths: - - "target/kibana-security-solution/**/*" + - 'target/kibana-security-solution/**/*' - command: .buildkite/scripts/steps/functional/security_solution_investigations.sh label: 'Investigations - Security Solution Cypress Tests' @@ -39,7 +39,7 @@ steps: - exit_status: '*' limit: 1 artifact_paths: - - "target/kibana-security-solution/**/*" + - 'target/kibana-security-solution/**/*' - command: .buildkite/scripts/steps/functional/security_solution_burn.sh label: 'Security Solution Cypress tests, burning changed specs' @@ -52,4 +52,11 @@ steps: automatic: false soft_fail: true artifact_paths: - - "target/kibana-security-solution/**/*" + - 'target/kibana-security-solution/**/*' + + - command: .buildkite/scripts/steps/code_generation/security_solution_codegen.sh + label: 'Security Solution OpenAPI codegen' + agents: + queue: n2-2-spot + timeout_in_minutes: 60 + parallelism: 1 diff --git a/.buildkite/scripts/common/util.sh b/.buildkite/scripts/common/util.sh index e22a807fc1830f..eca5fe352abcf5 100755 --- a/.buildkite/scripts/common/util.sh +++ b/.buildkite/scripts/common/util.sh @@ -31,7 +31,7 @@ check_for_changed_files() { SHOULD_AUTO_COMMIT_CHANGES="${2:-}" CUSTOM_FIX_MESSAGE="${3:-}" - GIT_CHANGES="$(git ls-files --modified -- . ':!:.bazelrc')" + GIT_CHANGES="$(git status --porcelain -- . ':!:.bazelrc')" if [ "$GIT_CHANGES" ]; then if ! is_auto_commit_disabled && [[ "$SHOULD_AUTO_COMMIT_CHANGES" == "true" && "${BUILDKITE_PULL_REQUEST:-}" ]]; then @@ -54,7 +54,7 @@ check_for_changed_files() { git config --global user.name kibanamachine git config --global user.email '42973632+kibanamachine@users.noreply.github.com' gh pr checkout "${BUILDKITE_PULL_REQUEST}" - git add -u -- . ':!.bazelrc' + git add -A -- . ':!.bazelrc' git commit -m "$NEW_COMMIT_MESSAGE" git push diff --git a/.buildkite/scripts/steps/code_generation/security_solution_codegen.sh b/.buildkite/scripts/steps/code_generation/security_solution_codegen.sh new file mode 100755 index 00000000000000..f6d31f3e94bbc6 --- /dev/null +++ b/.buildkite/scripts/steps/code_generation/security_solution_codegen.sh @@ -0,0 +1,12 @@ +#!/usr/bin/env bash + +set -euo pipefail + +source .buildkite/scripts/common/util.sh + +.buildkite/scripts/bootstrap.sh + +echo --- Security Solution OpenAPI Code Generation + +(cd x-pack/plugins/security_solution && yarn openapi:generate) +check_for_changed_files "yarn openapi:generate" true \ No newline at end of file diff --git a/packages/kbn-securitysolution-es-utils/src/transform_error/index.ts b/packages/kbn-securitysolution-es-utils/src/transform_error/index.ts index b532dc5d1b6d07..3a4386547e1c04 100644 --- a/packages/kbn-securitysolution-es-utils/src/transform_error/index.ts +++ b/packages/kbn-securitysolution-es-utils/src/transform_error/index.ts @@ -8,6 +8,7 @@ import Boom from '@hapi/boom'; import { errors } from '@elastic/elasticsearch'; +import { ZodError } from 'zod'; import { BadRequestError } from '../bad_request_error'; export interface OutputError { @@ -21,6 +22,15 @@ export const transformError = (err: Error & Partial): Outp message: err.output.payload.message, statusCode: err.output.statusCode, }; + } else if (err instanceof ZodError) { + const message = stringifyZodError(err); + + return { + message, + // These errors can occur when handling requests after validation and can + // indicate of issues in business logic, so they are 500s instead of 400s + statusCode: 500, + }; } else { if (err.statusCode != null) { if (err.body != null && err.body.error != null) { @@ -50,3 +60,15 @@ export const transformError = (err: Error & Partial): Outp } } }; + +export function stringifyZodError(err: ZodError) { + return err.issues + .map((issue) => { + // If the path is empty, the error is for the root object + if (issue.path.length === 0) { + return issue.message; + } + return `${issue.path.join('.')}: ${issue.message}`; + }) + .join(', '); +} diff --git a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.gen.ts b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.gen.ts new file mode 100644 index 00000000000000..9f9abdf51dc4e6 --- /dev/null +++ b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.gen.ts @@ -0,0 +1,49 @@ +/* + * 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 { z } from 'zod'; + +/* + * NOTICE: Do not edit this file manually. + * This file is automatically generated by the OpenAPI Generator `yarn openapi:generate`. + */ + +export type GetPrebuiltRulesAndTimelinesStatusResponse = z.infer< + typeof GetPrebuiltRulesAndTimelinesStatusResponse +>; +export const GetPrebuiltRulesAndTimelinesStatusResponse = z + .object({ + /** + * The total number of custom rules + */ + rules_custom_installed: z.number().min(0), + /** + * The total number of installed prebuilt rules + */ + rules_installed: z.number().min(0), + /** + * The total number of available prebuilt rules that are not installed + */ + rules_not_installed: z.number().min(0), + /** + * The total number of outdated prebuilt rules + */ + rules_not_updated: z.number().min(0), + /** + * The total number of installed prebuilt timelines + */ + timelines_installed: z.number().min(0), + /** + * The total number of available prebuilt timelines that are not installed + */ + timelines_not_installed: z.number().min(0), + /** + * The total number of outdated prebuilt timelines + */ + timelines_not_updated: z.number().min(0), + }) + .strict(); diff --git a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.schema.yaml b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.schema.yaml index deea1b32aa3aa0..889d7321c0bee3 100644 --- a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.schema.yaml +++ b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.schema.yaml @@ -5,8 +5,8 @@ info: paths: /api/detection_engine/rules/prepackaged/_status: get: - operationId: GetPrebuiltRulesStatus - x-codegen-enabled: false + operationId: GetPrebuiltRulesAndTimelinesStatus + x-codegen-enabled: true summary: Get the status of Elastic prebuilt rules tags: - Prebuilt Rules API @@ -17,6 +17,7 @@ paths: application/json: schema: type: object + additionalProperties: false properties: rules_custom_installed: type: integer diff --git a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.test.ts b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.test.ts index 83c09d2dcab26f..8be56e9d414164 100644 --- a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.test.ts +++ b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.test.ts @@ -5,11 +5,9 @@ * 2.0. */ -import { left } from 'fp-ts/lib/Either'; -import { pipe } from 'fp-ts/lib/pipeable'; -import { exactCheck, foldLeftRight, getPaths } from '@kbn/securitysolution-io-ts-utils'; - -import { GetPrebuiltRulesAndTimelinesStatusResponse } from './get_prebuilt_rules_and_timelines_status_route'; +import { stringifyZodError } from '@kbn/securitysolution-es-utils'; +import { expectParseError, expectParseSuccess } from '../../../../test/zod_helpers'; +import { GetPrebuiltRulesAndTimelinesStatusResponse } from './get_prebuilt_rules_and_timelines_status_route.gen'; describe('Get prebuilt rules and timelines status response schema', () => { test('it should validate an empty prepackaged response with defaults', () => { @@ -22,12 +20,10 @@ describe('Get prebuilt rules and timelines status response schema', () => { timelines_not_installed: 0, timelines_not_updated: 0, }; - const decoded = GetPrebuiltRulesAndTimelinesStatusResponse.decode(payload); - const checked = exactCheck(payload, decoded); - const message = pipe(checked, foldLeftRight); + const result = GetPrebuiltRulesAndTimelinesStatusResponse.safeParse(payload); - expect(getPaths(left(message.errors))).toEqual([]); - expect(message.schema).toEqual(payload); + expectParseSuccess(result); + expect(result.data).toEqual(payload); }); test('it should not validate an extra invalid field added', () => { @@ -41,12 +37,12 @@ describe('Get prebuilt rules and timelines status response schema', () => { timelines_not_installed: 0, timelines_not_updated: 0, }; - const decoded = GetPrebuiltRulesAndTimelinesStatusResponse.decode(payload); - const checked = exactCheck(payload, decoded); - const message = pipe(checked, foldLeftRight); + const result = GetPrebuiltRulesAndTimelinesStatusResponse.safeParse(payload); - expect(getPaths(left(message.errors))).toEqual(['invalid keys "invalid_field"']); - expect(message.schema).toEqual({}); + expectParseError(result); + expect(stringifyZodError(result.error)).toEqual( + "Unrecognized key(s) in object: 'invalid_field'" + ); }); test('it should NOT validate an empty prepackaged response with a negative "rules_installed" number', () => { @@ -59,14 +55,12 @@ describe('Get prebuilt rules and timelines status response schema', () => { timelines_not_installed: 0, timelines_not_updated: 0, }; - const decoded = GetPrebuiltRulesAndTimelinesStatusResponse.decode(payload); - const checked = exactCheck(payload, decoded); - const message = pipe(checked, foldLeftRight); + const result = GetPrebuiltRulesAndTimelinesStatusResponse.safeParse(payload); - expect(getPaths(left(message.errors))).toEqual([ - 'Invalid value "-1" supplied to "rules_installed"', - ]); - expect(message.schema).toEqual({}); + expectParseError(result); + expect(stringifyZodError(result.error)).toEqual( + 'rules_installed: Number must be greater than or equal to 0' + ); }); test('it should NOT validate an empty prepackaged response with a negative "rules_not_installed"', () => { @@ -79,14 +73,12 @@ describe('Get prebuilt rules and timelines status response schema', () => { timelines_not_installed: 0, timelines_not_updated: 0, }; - const decoded = GetPrebuiltRulesAndTimelinesStatusResponse.decode(payload); - const checked = exactCheck(payload, decoded); - const message = pipe(checked, foldLeftRight); + const result = GetPrebuiltRulesAndTimelinesStatusResponse.safeParse(payload); - expect(getPaths(left(message.errors))).toEqual([ - 'Invalid value "-1" supplied to "rules_not_installed"', - ]); - expect(message.schema).toEqual({}); + expectParseError(result); + expect(stringifyZodError(result.error)).toEqual( + 'rules_not_installed: Number must be greater than or equal to 0' + ); }); test('it should NOT validate an empty prepackaged response with a negative "rules_not_updated"', () => { @@ -99,14 +91,12 @@ describe('Get prebuilt rules and timelines status response schema', () => { timelines_not_installed: 0, timelines_not_updated: 0, }; - const decoded = GetPrebuiltRulesAndTimelinesStatusResponse.decode(payload); - const checked = exactCheck(payload, decoded); - const message = pipe(checked, foldLeftRight); + const result = GetPrebuiltRulesAndTimelinesStatusResponse.safeParse(payload); - expect(getPaths(left(message.errors))).toEqual([ - 'Invalid value "-1" supplied to "rules_not_updated"', - ]); - expect(message.schema).toEqual({}); + expectParseError(result); + expect(stringifyZodError(result.error)).toEqual( + 'rules_not_updated: Number must be greater than or equal to 0' + ); }); test('it should NOT validate an empty prepackaged response with a negative "rules_custom_installed"', () => { @@ -119,14 +109,12 @@ describe('Get prebuilt rules and timelines status response schema', () => { timelines_not_installed: 0, timelines_not_updated: 0, }; - const decoded = GetPrebuiltRulesAndTimelinesStatusResponse.decode(payload); - const checked = exactCheck(payload, decoded); - const message = pipe(checked, foldLeftRight); + const result = GetPrebuiltRulesAndTimelinesStatusResponse.safeParse(payload); - expect(getPaths(left(message.errors))).toEqual([ - 'Invalid value "-1" supplied to "rules_custom_installed"', - ]); - expect(message.schema).toEqual({}); + expectParseError(result); + expect(stringifyZodError(result.error)).toEqual( + 'rules_custom_installed: Number must be greater than or equal to 0' + ); }); test('it should NOT validate an empty prepackaged response if "rules_installed" is not there', () => { @@ -141,13 +129,9 @@ describe('Get prebuilt rules and timelines status response schema', () => { }; // @ts-expect-error delete payload.rules_installed; - const decoded = GetPrebuiltRulesAndTimelinesStatusResponse.decode(payload); - const checked = exactCheck(payload, decoded); - const message = pipe(checked, foldLeftRight); + const result = GetPrebuiltRulesAndTimelinesStatusResponse.safeParse(payload); - expect(getPaths(left(message.errors))).toEqual([ - 'Invalid value "undefined" supplied to "rules_installed"', - ]); - expect(message.schema).toEqual({}); + expectParseError(result); + expect(stringifyZodError(result.error)).toEqual('rules_installed: Required'); }); }); diff --git a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts deleted file mode 100644 index f5c26b81682a13..00000000000000 --- a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts +++ /dev/null @@ -1,25 +0,0 @@ -/* - * 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 * as t from 'io-ts'; -import { PositiveInteger } from '@kbn/securitysolution-io-ts-types'; - -export type GetPrebuiltRulesAndTimelinesStatusResponse = t.TypeOf< - typeof GetPrebuiltRulesAndTimelinesStatusResponse ->; -export const GetPrebuiltRulesAndTimelinesStatusResponse = t.exact( - t.type({ - rules_custom_installed: PositiveInteger, - rules_installed: PositiveInteger, - rules_not_installed: PositiveInteger, - rules_not_updated: PositiveInteger, - - timelines_installed: PositiveInteger, - timelines_not_installed: PositiveInteger, - timelines_not_updated: PositiveInteger, - }) -); diff --git a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/index.ts b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/index.ts index 38607fbd305132..87b43732bb6d26 100644 --- a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/index.ts +++ b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/index.ts @@ -5,7 +5,7 @@ * 2.0. */ -export * from './get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route'; +export * from './get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.gen'; export * from './get_prebuilt_rules_status/get_prebuilt_rules_status_route'; export * from './install_prebuilt_rules_and_timelines/install_prebuilt_rules_and_timelines_route'; export * from './perform_rule_installation/perform_rule_installation_route'; diff --git a/x-pack/plugins/security_solution/common/test/zod_helpers.ts b/x-pack/plugins/security_solution/common/test/zod_helpers.ts new file mode 100644 index 00000000000000..fd7b8cc8ff3f7f --- /dev/null +++ b/x-pack/plugins/security_solution/common/test/zod_helpers.ts @@ -0,0 +1,20 @@ +/* + * 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 { SafeParseError, SafeParseReturnType, SafeParseSuccess } from 'zod'; + +export function expectParseError( + result: SafeParseReturnType +): asserts result is SafeParseError { + expect(result.success).toEqual(false); +} + +export function expectParseSuccess( + result: SafeParseReturnType +): asserts result is SafeParseSuccess { + expect(result.success).toEqual(true); +} diff --git a/x-pack/plugins/security_solution/scripts/openapi/template_service/templates/schema_item.handlebars b/x-pack/plugins/security_solution/scripts/openapi/template_service/templates/schema_item.handlebars index 87ce8e58105c79..2d544a702ac004 100644 --- a/x-pack/plugins/security_solution/scripts/openapi/template_service/templates/schema_item.handlebars +++ b/x-pack/plugins/security_solution/scripts/openapi/template_service/templates/schema_item.handlebars @@ -78,6 +78,7 @@ z.unknown() {{@key}}: {{> schema_item requiredBool=(includes ../required @key)}}, {{/each}} }) + {{#if (eq additionalProperties false)}}.strict(){{/if}} {{~/inline~}} {{~#*inline "type_string"~}} diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts index 4e8cce8f8dd35f..4848cb5e52c4e4 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts @@ -13,8 +13,8 @@ import type { SetupPlugins } from '../../../../../plugin'; import type { SecuritySolutionPluginRouter } from '../../../../../types'; import { - PREBUILT_RULES_STATUS_URL, GetPrebuiltRulesAndTimelinesStatusResponse, + PREBUILT_RULES_STATUS_URL, } from '../../../../../../common/api/detection_engine/prebuilt_rules'; import { getExistingPrepackagedRules } from '../../../rule_management/logic/search/get_existing_prepackaged_rules'; @@ -89,16 +89,9 @@ export const getPrebuiltRulesAndTimelinesStatusRoute = ( timelines_not_updated: validatedPrebuiltTimelineStatus?.timelinesToUpdate.length ?? 0, }; - const [validatedBody, validationError] = validate( - responseBody, - GetPrebuiltRulesAndTimelinesStatusResponse - ); - - if (validationError != null) { - return siemResponse.error({ statusCode: 500, body: validationError }); - } else { - return response.ok({ body: validatedBody ?? {} }); - } + return response.ok({ + body: GetPrebuiltRulesAndTimelinesStatusResponse.parse(responseBody), + }); } catch (err) { const error = transformError(err); return siemResponse.error({