[Security Solution] Initial migration of API endpoints to OpenAPI and…

… code generation (elastic#164482) **Part of: elastic/security-team#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: <img width="627" alt="image" src="https://github.com/elastic/kibana/assets/1938181/93002573-972f-42e1-901d-01a19937f568">
kibanamachine · Aug 25, 2023 · 2171ecc · 2171ecc
1 parent 168e3dc
commit 2171ecc
Show file tree

Hide file tree

Showing 12 changed files with 159 additions and 95 deletions.
diff --git 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
@@ -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 '[email protected]'
       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
@@ -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
diff --git 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<errors.ResponseError>): 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<errors.ResponseError>): Outp
     }
   }
 };
+
+export function stringifyZodError(err: ZodError<any>) {
+  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/..._prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.gen.ts b/..._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/...uilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.schema.yaml b/...uilt_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/...prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.test.ts b/...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/.../get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts b/.../get_prebuilt_rules_and_timelines_status/get_prebuilt_rules_and_timelines_status_route.ts
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
@@ -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
@@ -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<Input, Output>(
+  result: SafeParseReturnType<Input, Output>
+): asserts result is SafeParseError<Input> {
+  expect(result.success).toEqual(false);
+}
+
+export function expectParseSuccess<Input, Output>(
+  result: SafeParseReturnType<Input, Output>
+): asserts result is SafeParseSuccess<Output> {
+  expect(result.success).toEqual(true);
+}
diff --git a/...ugins/security_solution/scripts/openapi/template_service/templates/schema_item.handlebars b/...ugins/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"~}}