Allow gRPC service registration through extensible plugins

[finnegancarroll]

Description

Adds a GrpcServiceFactory interface as an extension point for other plugins to implement. GrpcServiceFactory provides an interface for wrapping a predefined io.grpc.BindableService which can be registered on the gRPC transport. Enabling plugins to provide their own service definitions opens the door for supporting pre-defined service schemas within the gRPC transport (such as Flight or OTEL) proto types, as well as custom hand rolled services which do not mirror the REST API and are not tracked by the API spec.

Related Issues

Resolves #19025

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[github-actions]

✅ Gradle check result for 813ede2: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 93.61702% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.19%. Comparing base (a250c35) to head (352dbf6).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...java/org/opensearch/transport/grpc/GrpcPlugin.java	93.02%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #19304      +/-   ##
============================================
+ Coverage     73.15%   73.19%   +0.04%     
- Complexity    70958    71018      +60     
============================================
  Files          5736     5737       +1     
  Lines        324734   324766      +32     
  Branches      46979    46982       +3     
============================================
+ Hits         237548   237710     +162     
+ Misses        68031    67921     -110     
+ Partials      19155    19135      -20     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[finnegancarroll]

Hi @andrross @karenyrx can I get your thoughts on this approach?

Intentionally not including GrpcServiceFactory in the SPI as BindableService needs io.grpc:grpc-core. Wondering if a second SPI is best here to avoid leaking org.opensearch:protobufs into plugins which may not ever use it.

[github-actions]

❌ Gradle check result for 280fd95: 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?

[github-actions]

❌ Gradle check result for bf90052: 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?

[andrross]

Intentionally not including GrpcServiceFactory in the SPI as BindableService needs io.grpc:grpc-core. Wondering if a second SPI is best here to avoid leaking org.opensearch:protobufs into plugins which may not ever use it.

@finnegancarroll I see you created a second SPI. Isn't BindableService a part of grpc-api, which only has dependencies on findbugs and errorprone annotations? That seems pretty lightweight and would be okay in a single SPI. What am I missing?

[finnegancarroll]

@finnegancarroll I see you created a second SPI. Isn't BindableService a part of grpc-api, which only has dependencies on findbugs and errorprone annotations? That seems pretty lightweight and would be okay in a single SPI. What am I missing?

Hi @andrross, the main reason to separate this SPI was the guava runtime dependency. Guava is runtime only in the main module as well and plugins such as KNN which only want the protobuf definitions run into version and subsequent runtime issues within the plugin test infrastructure. Here is the workaround for the original KNN PR:
https://github.com/opensearch-project/k-NN/pull/2817/files#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R343-R351

[andrross]

@finnegancarroll I see you created a second SPI. Isn't BindableService a part of grpc-api, which only has dependencies on findbugs and errorprone annotations? That seems pretty lightweight and would be okay in a single SPI. What am I missing?

Hi @andrross, the main reason to separate this SPI was the guava runtime dependency. Guava is runtime only in the main module as well and plugins such as KNN which only want the protobuf definitions run into version and subsequent runtime issues within the plugin test infrastructure. Here is the workaround for the original KNN PR: opensearch-project/k-NN#2817 (files)

@finnegancarroll Does the new SPI really need a Guava runtime dependency? I guess I was thinking this new code would only require adding the grpc-api dependency, and that it could just be added to the existing SPI which would stay pretty lightweight. Does that not work?

[finnegancarroll]

@finnegancarroll Does the new SPI really need a Guava runtime dependency? I guess I was thinking this new code would only require adding the grpc-api dependency, and that it could just be added to the existing SPI which would stay pretty lightweight. Does that not work?

Hi @andrross , doing some experimenting and guava can be excluded here and listed under ignoreMissingClasses in third party audit. The result would be guava is a unlisted runtime dependency however this seems relatively safe since it will probably never be the case that plugins using this interface will attempt to implement a BindableService from scratch and will almost certainly already have guava at runtime through dependencies they use to generate their BindableService implementations.

One other consideration I think is extending this SPI to include other grpc-api definitions in the future. What comes to mind is ServerInterceptor which is much more likely to be directly implemented, however i'm not sure this changes much as most plugins already have guava at runtime so it's difficult to imagine this being an issue still.

What do you think? Is it preferable to stick with the single SPI implementation and exclude guava or list it explicitly in a separate SPI?

[github-actions]

❌ Gradle check result for 28b757b: null

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?

[finnegancarroll]

Per offline convo with @andrross the second SPI is not needed as guava which is a transitive runtime dependency of grpc-api will of course always be present since implemented services are executed within the gRPC transport module. Removed the duplicate SPI and updated the readme to include GrpcServiceFactory endpoint.

[github-actions]

❌ Gradle check result for 28b757b: 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?

[github-actions]

✅ Gradle check result for 28b757b: SUCCESS

[github-actions]

❌ Gradle check result for 17594d9: 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?

[finnegancarroll]

* What went wrong:
Execution failed for task ':distribution:bwc:minor:buildBwcLinuxTar'.
> Building 3.3.0 didn't generate expected file /var/jenkins/workspace/gradle-check/search/distribution/bwc/minor/build/bwc/checkout-3.3/distribution/archives/linux-tar/build/distributions/opensearch-min-3.3.0-SNAPSHOT-linux-x64.tar.gz

Seems related to recent 3.3.1 minor version.

[github-actions]

❌ Gradle check result for 6c7bca0: 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?

[github-actions]

❌ Gradle check result for 6c7bca0: 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?

[github-actions]

❌ Gradle check result for a78b5ec: 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?

[github-actions]

❌ Gradle check result for a78b5ec: 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?

[github-actions]

❌ Gradle check result for a78b5ec: 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?

[finnegancarroll]

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':distribution:bwc:bugfix:buildBwcLinuxTar'.
> Building 3.3.1 didn't generate expected file /var/jenkins/workspace/gradle-check/search/distribution/bwc/bugfix/build/bwc/checkout-3.3/distribution/archives/linux-tar/build/distributions/opensearch-min-3.3.1-SNAPSHOT-linux-x64.tar.gz

Still seeing build failures. Up to date with main.

[github-actions]

❌ Gradle check result for dbb963d: 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?

[finnegancarroll]

Task :distribution:bwc:maintenance:buildBwcLinuxTar
 [2.19.4] .....................10%......................20%......................30%......................40%............
Exception in thread "main" java.net.SocketException: Connection reset

Network issue pulling a snapshot?

[github-actions]

✅ Gradle check result for 352dbf6: SUCCESS