Skip to content

Commit

Permalink
[Security Solution] Initial migration of API endpoints to OpenAPI and…
Browse files Browse the repository at this point in the history
… 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">
  • Loading branch information
xcrzx authored Aug 25, 2023
1 parent 168e3dc commit 2171ecc
Show file tree
Hide file tree
Showing 12 changed files with 159 additions and 95 deletions.
15 changes: 11 additions & 4 deletions .buildkite/pipelines/pull_request/security_solution.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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'
Expand All @@ -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'
Expand All @@ -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
4 changes: 2 additions & 2 deletions .buildkite/scripts/common/util.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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) {
Expand Down Expand Up @@ -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(', ');
}
Original file line number Diff line number Diff line change
@@ -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();
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -17,6 +17,7 @@ paths:
application/json:
schema:
type: object
additionalProperties: false
properties:
rules_custom_installed:
type: integer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -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"', () => {
Expand All @@ -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"', () => {
Expand All @@ -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"', () => {
Expand All @@ -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', () => {
Expand All @@ -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');
});
});

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
20 changes: 20 additions & 0 deletions x-pack/plugins/security_solution/common/test/zod_helpers.ts
Original file line number Diff line number Diff line change
@@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ z.unknown()
{{@key}}: {{> schema_item requiredBool=(includes ../required @key)}},
{{/each}}
})
{{#if (eq additionalProperties false)}}.strict(){{/if}}
{{~/inline~}}

{{~#*inline "type_string"~}}
Expand Down
Loading

0 comments on commit 2171ecc

Please sign in to comment.