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

[Draft] A plugin that dynamically generates an OpenAPI spec #11843

Closed
wants to merge 7 commits into from

Conversation

dblock
Copy link
Member

@dblock dblock commented Jan 10, 2024

Description

This POC generates a partial OpenAPI spec from the existing registered REST handlers. It can be used, for example, by assembling a distribution of OpenSearch with all its plugins, and then generating the OpenAPI spec from that instance at runtime like in opensearch-project/opensearch-api-specification#179 where a report is being generated about what APIs are and aren't documented in the OpenAPI spec.

Looking for feedback.

  • Is this something every installation of OpenSearch should have?
  • Would we want this in core, or in a separate repo/plugin?
  • Is it better to implement RestController#toXContent or make getHandlers() and RestHandler public?

Currently it returns something like this:

curl http://localhost:9200/_plugins/api | jq
{
  "openapi": "3.0.1",
  "info": {
    "title": "opensearch",
    "description": "The OpenSearch Project: https://opensearch.org/",
    "version": "3.0.0-SNAPSHOT"
  },
  "paths": {
    "/_nodes": {
      "get": {}
    },
    "/_cluster/state/:metric": {
      "get": {}
    },
    "/:index/_aliases/:name": {
      "put": {},
      "post": {},
      "delete": {}
    },
    "/:index/_msearch": {
      "get": {},
      "post": {}
    },
    ...
}

In its complete form, this plugin could offers a few capabilities.

  1. Users could generate clients or use OpenAPI-compatible tools such as Swagger directly from a running instance of OpenSearch server.
  2. The output can be compared to https://github.com/opensearch-project/opensearch-api-specification and potentially automatically keep that Smithy specification up-to-date, or even make it obsolete if we chose to generate a Smithy spec instead of OpenAPI.
  3. The output can be published with every build/release of OpenSearch giving a reliable API specification for that distribution, including for multiple distributions.

2.x, https://github.com/dblock/OpenSearch/tree/2.x-api-plugin

Related Issues

Other

Running this plugin on 2.11.

Build OpenSearch 2.11 + plugin

git clone [email protected]:dblock/OpenSearch.git
cd OpenSearch
git checkout 2.11.1-api-plugin

./gradlew :server:assemble -Dbuild.snapshot=false
./gradlew :libs:opensearch-core:assemble -Dbuild.snapshot=false
./gradlew :plugins:api:assemble -Dbuild.snapshot=false

Create a Dockerfile

FROM opensearchproject/opensearch:2.11.1
ADD ./plugins/api/build/distributions/api-2.11.1.zip /tmp/
COPY ./server/build/distributions/opensearch-2.11.1.jar /usr/share/opensearch/lib/
COPY ./libs/core/build/distributions/opensearch-core-2.11.1.jar /usr/share/opensearch/lib/
RUN /usr/share/opensearch/bin/opensearch-plugin install --batch file:/tmp/api-2.11.1.zip

Build and Run Docker Image

docker build . --tag opensearch-with-api-plugin
docker run -d -p 9200:9200 -p 9600:9600 -e "discovery.type=single-node" opensearch-with-api-plugin

Curl

git clone https://github.com/opensearch-project/opensearch-api-specification
cd opensearch-api-specification
curl --insecure -u admin:admin -v https://localhost:9200/_plugins/api | jq > OpenSearch.auto.openapi.json

Compare

The spec repo has OpenSearch.openapi.json. We created OpenSearch.auto.openapi.json above.

docker run --mount type=bind,source=.,target=/specs openapitools/openapi-diff:latest /specs/OpenSearch.auto.openapi.json /specs/OpenSearch.openapi.json

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

❌ Gradle check result for f717c19: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jan 10, 2024

Compatibility status:

Checks if related components are compatible with change 5e7b87e

Incompatible components

Incompatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/k-nn.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/performance-analyzer.git]

Passing ./gradlew plugins:api:yamlRestTest.

Signed-off-by: dblock <[email protected]>
Copy link
Contributor

❌ Gradle check result for 3504590: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for 5cae02a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for c70d02f: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for f44b777: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

dblock added a commit to dblock/OpenSearch that referenced this pull request Jan 11, 2024
Copy link
Contributor

❌ Gradle check result for 205de2e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

dblock added a commit to dblock/OpenSearch that referenced this pull request Jan 11, 2024
dblock added a commit to dblock/OpenSearch that referenced this pull request Jan 11, 2024
Copy link
Contributor

❕ Gradle check result for de6c3f7: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.remotestore.RemoteIndexPrimaryRelocationIT.testPrimaryRelocationWhileIndexing

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 77 lines in your changes are missing coverage. Please review.

Comparison is base (3d57714) 71.36% compared to head (5e7b87e) 71.44%.
Report is 3 commits behind head on main.

Files Patch % Lines
.../org/opensearch/plugin/api/action/APIResponse.java 0.00% 28 Missing ⚠️
.../main/java/org/opensearch/rest/RestController.java 0.00% 13 Missing ⚠️
...main/java/org/opensearch/common/path/PathTrie.java 0.00% 11 Missing ⚠️
.../java/org/opensearch/plugin/api/RestAPIAction.java 0.00% 9 Missing ⚠️
...a/org/opensearch/plugin/api/action/APIRequest.java 0.00% 4 Missing ⚠️
...ensearch/plugin/api/action/TransportAPIAction.java 0.00% 4 Missing ⚠️
...va/org/opensearch/plugin/api/action/APIAction.java 0.00% 3 Missing ⚠️
...main/java/org/opensearch/plugin/api/APIPlugin.java 50.00% 2 Missing ⚠️
...pensearch/plugin/api/action/APIRequestBuilder.java 0.00% 2 Missing ⚠️
.../main/java/org/opensearch/rest/MethodHandlers.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #11843      +/-   ##
============================================
+ Coverage     71.36%   71.44%   +0.08%     
- Complexity    59291    59374      +83     
============================================
  Files          4919     4926       +7     
  Lines        278956   279035      +79     
  Branches      40550    40555       +5     
============================================
+ Hits         199079   199361     +282     
+ Misses        63334    63150     -184     
+ Partials      16543    16524      -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dblock added a commit to dblock/OpenSearch that referenced this pull request Jan 11, 2024
dblock added a commit to dblock/OpenSearch that referenced this pull request Jan 11, 2024
dblock added a commit to dblock/OpenSearch that referenced this pull request Jan 11, 2024
dblock added a commit to dblock/OpenSearch that referenced this pull request Jan 11, 2024
@andrross
Copy link
Member

Is this something every installation of OpenSearch should have?

Is there a question-behind-the-question here? Architecturally, I like this as a plugin/module because it doesn't need deep integration into parts of the core. Are you questioning whether this should be a module (aka a plugin that is always installed in the min distribution)? I would guess most users would not use the OpenAPI spec generated by this plugin, but also this plugin is quite lean so including it by default is probably fine.

Would we want this in core, or in a separate repo/plugin?

Given the size and complexity of core my default position is to create it as a separate repo. As it is, this plugin is quite lean so including in the core repo is probably fine. But if we have any expectation that we might want to add functionality or add additional OpenAPI-related tooling to this plugin, then that would push me even stronger towards a separate repo.

Is it better to implement RestController#toXContent or make getHandlers() and RestHandler public?

I lean towards RestController having a public method that describes all REST methods it provides, versus exposing the more detailed and functional getHandlers()/RestHandler. That is exactly what RestController#toXContent is doing, but I haven't thought deeply whether xcontent is the right way to return this information (versus, say, some basic POJOs that describe the APIs).

Copy link
Contributor

✅ Gradle check result for 5e7b87e: SUCCESS

Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

This is amazing! A few thoughts:

  1. If this could also generate the params for the APIs.
  2. If this completely generates the specs for the server in the future, would this not go against the generation of server if we want to do that some day?

@dblock
Copy link
Member Author

dblock commented Jan 12, 2024

@andrross

Is there a question-behind-the-question here?

Yes, I'll try to be clearer. Assuming it can be complete, do we want dynamic OpenAPI spec a core feature enabled by default in OpenSearch? That would likely imply building facilities to describe the API in annotations. That has advantages, such as clients being able to rely on the fact that each different deployment of OpenSearch can dynamically express what its API is at runtime, and therefore we can use generic clients and frameworks that are OpenAPI-compatible.

I lean towards RestController having a public method that describes all REST methods it provides, versus exposing the more detailed and functional getHandlers()/RestHandler.

I'll make a PR that just exposes the minimum methods I would need to move this code to a plugin in a separate repo. Probably not an entire POJO just yet. #11876

@VachaShah

If this could also generate the params for the APIs.

I am looking at that next.

If this completely generates the specs for the server in the future, would this not go against the generation of server if we want to do that some day?

You are correctly pointing out that we can do either spec -> server or server -> spec, but we don't need to do both.

@dblock
Copy link
Member Author

dblock commented Jan 19, 2024

With #11876 re-extracted this code into a separate plugin, https://github.com/dblock/opensearch-api (dblock/opensearch-api@5adec70).

@dblock dblock closed this Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants