[Security Solution] A proper location for OpenAPI specs

maximpn commented 5 months ago

Epic: https://github.com/elastic/security-team/issues/9401 (internal)

Summary

While working on adding missing OpenAPI spec (OAS) for Detections API it became prominent that the current OAS placement in security_solution/common/api brings the following challenges

Bad DX when using OAS references e.g. ../../../../../../../lists/common/api/exceptions/exception.schema.yaml

Some API endpoints (e.g. POST /api/detection_engine/rules/{id}/exceptions) have external dependencies. It requires referencing shared OAS schemas via $ref from the other plugins (e.g. lists). Having such long relative paths is brittle and inconvenient. Moving OAS files to another folder will be painful. There is no aliasing solution out of the box.
A necessity to import generated Zod schemas and inferred types from the other plugins.

Currently we have io-ts schemas defined in plugins e.g. kbn-securitysolution-io-ts-list-types.

Primitives (NonEmptyString or UUID) location. What's a proper location for them?

There some shared OAS schemas considered like primitives like NonEmptyString and UUID defined in Security Solution plugin. Obviously such primitives need to be reused in the other plugins.

elasticmachine commented 5 months ago

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

maximpn commented 5 months ago

One of the ideas suggested by @banderror is to use Security Solution domain packages like kbn-security-solution-detections-api and kbn-security-solution-exceptions-api. It solves the above problems in the following way

Packages reside one to the other which simplifies OAS references usage while not completely eliminates the problem
Generated artefacts can be easily imported by any package or plugin
It follows a similar approach we have with io-ts schemas
An OAS common or primitives package can have shared primitives like NonEmptyString or UUID

The only question is to decide where should resulting OAS bundles be placed

in each domain package
in meta package like kbn-security-solution-oas-bundles
in a plugin, e.g. security_solution

maximpn commented 5 months ago

Zoomed with @marshallmain and @banderror regarding this topic. Discussed possible ways to solve bad DX related to relative schema imports like ../../../../../../../lists/common/api/exceptions/exception.schema.yaml and reusing of shared primitives. Based on the discussion we worked out the following actions items

Investigate usage of Base URI

Moving OAS files into packages only mitigates bad DX and doesn't solve it completely. We need something like aliasing where reference can start from an alias like @lists/ or @shared/ but it's not supported by OAS tooling. Since OAS is based on JSON Schema it's worth to investigate Base URI usage. Ideally we could set the base path like <Kibana root folder>/x-pack/plugins/ or <Kibana root folder>/packages and references like /lists/path/to/my_schema.schema.yaml. But we still should have an option to use relative path when it's shorter. It doesn't solve the problem completely but significantly improves DX.
Investigate OAS re-exporting

There is an idea to export only a limited set of shared OAS schemas to be reused in the other plugins or packages. The idea is based on adding a mediator OAS file somewhere like lists/common/api (at the top level folder as possible) to reference it from another plugin or package.
Investigate placing OAS primitives like NonEmptyString into a package

OAS primitives like NonEmptyString will be required almost everywhere so having them in a specially dedicated packages looks reasonable. We need to understand easiness to consume that primitives from a package.

maximpn commented 5 months ago

Hi @marshallmain and @banderror,

I've investigated usage of Base URI, OAS shared schemas re-exporting and using a package for schema primitives.

TLDR: There is a no working solution out of the box. We need noticeable amount of effort to make it working.

Details

Investigate usage of Base URI

I analyzed it from two different sides OAS and JSON schema specifications and tooling (kbn-openapi-generator uses @apidevtools/swagger-parser package).

OAS and JSON schema specifications

Checking OAS 3.0 specification page there is no mention of base URI. There is an additional page with usage examples limited to relative path or urls. Where the first page's note says

Relative references used in $ref are processed as per JSON Reference, using the URL of the current document as the base URI. See also the Reference Object.

Analyzing JSON schema specification it becomes clear that references are considered as links. Links could be relative or absolute. Relative links resolved by using baseURI which is a URI the specification was retrieved from. Or baseURI can be explicitly specified via $id property. However $id property (or any alternative) isn't mentioned anywhere in OAS 3.0 specification.

Tooling

kbn-openapi-generator uses @apidevtools/swagger-parser package. It allows to parse OAS files and get it's representation in memory. I analyzed @apidevtools/swagger-parser source code and looked at its dependency @apidevtools/json-schema-ref-parser as well. There are no signs of baseURI functionality we could use to improve DX. It doesn't provide a way to specify a base path/base url to resolve references.

Taking the above into account there is no way to use Base URI in OAS.

Investigate OAS re-exporting

I approached this part by creating a shared list schema in List plugin at x-pack/plugins/lists/common/api/values/model/list_common.schema.yaml and adding a re-export spec at x-pack/plugins/lists/common/api/export.schema.yaml looking like

openapi: 3.0.0
info:
  title: Error Schema
  version: 'not applicable'
paths: {}
components:
  x-codegen-enabled: false
  schemas:
    List:
      $ref: './values/model/list_common.schema.yaml#/components/schemas/List'

then I referenced re-exported List schema in Security Solution plugin at x-pack/plugins/security_solution/common/api/model/test_re_export.gen.ts like

openapi: 3.0.0
info:
  title: Test shared schemas re-export
  version: 'not applicable'
paths: {}
components:
  x-codegen-enabled: true
  schemas:
    TestSchema:
      type: object
      properties:
        localId:
          $ref: './primitives.schema.yaml#/components/schemas/NonEmptyString'
        listData:
          $ref: '../../../../lists/common/api/export.schema.yaml#/components/schemas/List'

The problem with that approach is that kbn-openapi-generator doesn't unfold references (a folded reference means a chain of referenced like above '../../../../lists/common/api/export.schema.yaml#/components/schemas/List' -> './values/model/list_common.schema.yaml#/components/schemas/List'). Its implementation is pretty straightforward. It simply read an OAS file and generates artefacts add imports for external references. The generated code relies on that List Zod schema exists next to export.schema.yaml in the List plugin. But it's a proxy schema we don't want to generate an artefact for.

Investigate placing OAS primitives like `NonEmptyString` into a package

It's not a problem to place shared schema primitives in a package. The only problem here is caused by two bullets above. Reference paths are gonna be long and looking like ../../../../../../../packages/kbn-oas-common/primitives.schema.yaml.

banderror commented 5 months ago

@maximpn Thank you for doing this research and describing the results. My suggestion is:

Let's not invest any more time in any tooling related to refs. We have a very big scope of work that we can't avoid in the bundler and other epics, and we have limited time till the first deadline due to time off in the summer.
I think we can tolerate - at least for some time - long reference paths that would look like ../../../../../../../packages/kbn-oas-common/primitives.schema.yaml. Probably it's not the best DX, but it's also not a high-impact issue.
Let's create a new package for storing primitives. It could be a kbn-openapi-common or something else.
Let's start adding Lists API and Exceptions API schemas to their own dedicated packages from the beginning. Naming is arguable, but could be something like:
- kbn-securitysolution-lists-api and kbn-securitysolution-exceptions-api
- kbn-securitysolution-lists-common/api and kbn-securitysolution-exceptions-common/api (preferred from my side)

maximpn commented 4 months ago

Based on the research we decided to proceed with packages for OpenAPI specs as describe in the previous comment https://github.com/elastic/kibana/issues/184859#issuecomment-2160485108.

Actual code change will be addressed as a part of separate tickets

elastic / kibana