Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] OpenAPI linter #171851

Merged
merged 5 commits into from
Jan 5, 2024
Merged

Conversation

maximpn
Copy link
Contributor

@maximpn maximpn commented Nov 23, 2023

Resolves: https://github.com/elastic/security-team/issues/8099

Summary

This PR adds an a command to lint OpenAPI specs defined in Security Solution plugin.

Details

We have a number of OpenAPI specs defined in Security Solution plugin. While @kbn/openapi-generator package processes the specs and makes sure the specs are parsable and processable we don't have proper specs linter set up.
This PR introduces OpenAPI specs linting by leveraging Redocly CLI's lint command with a custom configuration placed in @kbn/openapi-generator package . Configuration includes reasonable best practices by using built-in Redocly rules.

The lint utility fulfil the following requirements

  • Validates yaml files against OpenAPI specification. It supports 3.0 and 3.1.
  • Validates x-modify property to have only partial, required or requiredOptional values.
  • Checks for reasonable best practices and displays a warning message when it's not met. Reasonable best practices are based on the recommended ruleset.

The lint utility has been incorporated into the existing OpenAPI generator, and linting is performed before generation.

Tool selection

Swagger CLI is a well known tool to validate OpenAPI specs. On November 15th 2023 the repo has been archived with a message in README

⚠️ Swagger CLI has been deprecated, due to the maintenance burnden of trying to keep up with expectations of a huge userbase with little to no pull requests or support. Redocly CLI covers all of the same functionality, and has more advanced linting with custom rules, and we highly recommend using that instead. They have conveniently provided a migration guide for existing Swagger CLI users. Read the review of Redocly CLI from APIs You Won't Hate.

Taking it into account choice falls on Redocly CLI.

How to test?

Change directory to Security Solution plugin's root and run linting by using the following commands from Kibana root

cd x-pack/plugins/security_solution
yarn openapi:generate

@maximpn maximpn added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Project:Serverless Work as part of the Serverless project for its initial release v8.12.0 labels Nov 23, 2023
@maximpn maximpn self-assigned this Nov 23, 2023
@maximpn maximpn marked this pull request as ready for review November 23, 2023 13:24
@maximpn maximpn requested review from a team as code owners November 23, 2023 13:24
@maximpn maximpn requested a review from banderror November 23, 2023 13:24
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@maximpn maximpn requested a review from a team as a code owner November 23, 2023 13:33
Copy link
Contributor

@jpdjere jpdjere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, tested adding an x-modify prop and I see the linter failing:
image

LGTM 👍

One question though: I see that the Redocly CLI tool has a tool for bundling multiple OpenAPI files. Was that considered and discarded for some reason before creating the package in-house in #171526?

@maximpn
Copy link
Contributor Author

maximpn commented Nov 25, 2023

One question though: I see that the Redocly CLI tool has a tool for bundling multiple OpenAPI files. Was that considered and discarded for some reason before creating the package in-house in #171526?

Right, there is join command Redocly provides. It has a lot of limitations and after playing with it it was clear the command doesn't give enough flexibility. The tool was discarded by gave some inspirations.

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and got 95 linter warnings and 0 errors which makes the lining script succeed. Looks great @maximpn!

I do have a couple of suggestions though, please check my comments.

@maximpn maximpn force-pushed the openapi-linter branch 3 times, most recently from 81b1d70 to 01294a4 Compare November 27, 2023 18:43
@maximpn maximpn requested a review from banderror November 27, 2023 18:51
watson
watson previously requested changes Nov 28, 2023
Copy link
Contributor

@watson watson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using npx in a scenario like this might have unintended side-effects and can be a potential security risk. I prefer if you call the installed "binary" directly without invoking it through npx

x-pack/plugins/osquery/package.json Outdated Show resolved Hide resolved
packages/kbn-openapi-generator/src/openapi_linter.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@gergoabraham gergoabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elastic/security-defend-workflows related changes (which is not much actually) look good 💯

(also, script runs nicely, was able to see the mentioned 95 warnings and fix one)

@banderror
Copy link
Contributor

@maximpn Just in case you're watching this PR, I'm trying to fix CI and force-pushing into your origin branch.

@banderror banderror force-pushed the openapi-linter branch 3 times, most recently from 4b8d0aa to be9f265 Compare November 30, 2023 12:51
Copy link
Contributor

@watson watson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM - just found a tiny yarn.lock nit 😊

yarn.lock Outdated Show resolved Hide resolved
Copy link
Contributor

@jaredburgettelastic jaredburgettelastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entity Analytics changes LGTM 👍

@banderror banderror force-pushed the openapi-linter branch 2 times, most recently from 39cb454 to c4fec96 Compare January 4, 2024 16:22
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/openapi-generator 7 13 +6

Async chunks

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

id before after diff
dashboard 377.5KB 377.5KB +20.0B
fleet 1.2MB 1.2MB +20.0B
ingestPipelines 361.5KB 361.6KB +20.0B
total +60.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
kbnUiSharedDeps-npmDll 6.2MB 6.2MB -648.0B
Unknown metric groups

API count

id before after diff
@kbn/openapi-generator 7 13 +6

ESLint disabled in files

id before after diff
@kbn/openapi-generator 1 2 +1

Total ESLint disabled count

id before after diff
@kbn/openapi-generator 3 4 +1

History

  • 💔 Build #185911 failed c4fec9688d92889d3d7f4c76fac7ae8cbc4c5ea6
  • 💔 Build #185867 failed 39cb4544a7032740243fadaa2984e948ed57f628
  • 💔 Build #185155 failed f8d601435e433b24fc3305eb5f265ed8a052ebf7
  • 💔 Build #185138 failed bf34c0a62e838c665d713e35a0c72636c58962f5
  • 💔 Build #185123 failed 7be633ff21bcb3853e0c169915c88cf01c349558

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

cc @maximpn

@jbudz jbudz dismissed watson’s stale review January 5, 2024 13:18

out of date

@jbudz
Copy link
Member

jbudz commented Jan 5, 2024

I reviewed the changes requested by watson and verified they have all been addressed. I dismissed the review to unblock this PR.

@banderror banderror merged commit 35cccc2 into elastic:main Jan 5, 2024
38 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 5, 2024
@maximpn maximpn deleted the openapi-linter branch January 29, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.