Experimental gRPC server #16534

finnegancarroll · 2024-10-31T21:36:21Z

Description

gRPC server implemented as a network plugin.

Running with transport-grpc plugin

Note: The gRPC server is additionally gated by a feature flag: "opensearch.experimental.feature.grpc.enabled"
This is currently enabled by default as some additional details with setting feature flags with gradle and providing port permissions need to be sorted out.

./gradlew run -PinstalledPlugins="['transport-grpc']"

Testing (grpcurl):

The gRPC server supports a few basic services.

Default service for listing available services + health check/ping service.

grpcurl -plaintext localhost:9400 list
grpcurl -plaintext localhost:9400 grpc.health.v1.Health/Check

Related Issues

RFCs detailing generation of protobuf files from api-specification - 1 2.

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 · 2024-10-31T21:42:05Z

❌ Gradle check result for b5fddb1: 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 · 2024-10-31T22:03:29Z

❌ Gradle check result for 24a40ad: 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 · 2024-11-07T19:32:31Z

❌ Gradle check result for 3bcd5bb: 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 · 2024-11-07T20:47:37Z

❌ Gradle check result for b2a1078: 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 · 2024-11-08T01:50:26Z

❌ Gradle check result for ee85066: 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 · 2024-11-14T17:46:09Z

❌ Gradle check result for eda1c04: 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 · 2024-11-14T18:59:39Z

❌ Gradle check result for 5038cf3: 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 · 2024-11-14T20:05:37Z

❌ Gradle check result for 1e5d354: 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 · 2024-11-14T20:45:02Z

❌ Gradle check result for 00d4eb1: 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 · 2024-11-14T23:48:38Z

❌ Gradle check result for 6151622: 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 · 2024-11-15T19:22:13Z

❌ Gradle check result for b8d88d2: 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 · 2024-11-15T22:02:00Z

❌ Gradle check result for b404cee: 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 · 2024-11-15T22:59:47Z

❌ Gradle check result for 41e70eb: 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 · 2024-11-15T23:42:27Z

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

server/src/main/proto/spec/common.proto

github-actions · 2024-11-20T00:47:55Z

❌ Gradle check result for bbabecd: 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 · 2024-11-20T23:59:03Z

❌ Gradle check result for 853b206: 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 · 2024-11-22T21:37:25Z

❌ Gradle check result for da16c66: 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 · 2024-12-09T20:58:29Z

❌ Gradle check result for c49cbc0: 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 · 2024-12-09T21:27:29Z

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

plugins/transport-grpc/src/main/proto/opensearch/main_action.proto

github-actions · 2024-12-09T22:00:29Z

❌ Gradle check result for 6de6037: 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 · 2024-12-09T22:19:58Z

❌ Gradle check result for 62d4522: 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 · 2024-12-09T22:42:50Z

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

gradle/forbidden-dependencies.gradle

andrross · 2024-12-09T23:34:28Z


FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':plugins:transport-grpc:dependencyLicenses'.
> Missing SHA for guava-32.1.1-jre.jar. Run "gradle updateSHAs" to create them

Can we get the build issues sorted out and get a passing gradle check? You should be able to run ./gradlew precommit locally to find a lot of these problems before pushing your commits.

server/src/main/java/org/opensearch/common/util/FeatureFlags.java

plugins/transport-grpc/build.gradle

github-actions · 2024-12-11T17:39:46Z

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

@finnegancarroll This looks good to me. Let's get this across the finish line. Just a few things remain:

mark the PR as not draft
fix the license issue by running ./gradlew updateSHAs
get the build passing by removing the empty integration tests
confirm the full ./gradlew check passes on the PR

.../src/internalClusterTest/java/org/opensearch/transport/grpc/OpenSearchGrpcIntegTestCase.java

reta · 2024-12-13T18:35:42Z

plugins/transport-grpc/src/main/java/org/opensearch/transport/grpc/GrpcModulePlugin.java

+        if (FeatureFlags.isEnabled(FeatureFlags.GRPC_ENABLE_SETTING) == false) {
+            throw new IllegalArgumentException("transport-grpc is experimental and feature flag must be enabled before use");
+        }
+        this.transport = new Netty4GrpcServerTransport(clusterService.getSettings(), Collections.emptyList());


Curios why do you need access to clusterService.getSettings() here? I would suspect you could move instantiation to getHttpTransports and use provided settings instead?

We will definitely need the client instance, which is why initialization is happening here. An earlier revision that had a hand-rolled example of the MainAction implemented as gRPC showed this. We probably could stash off the client instance and then do initialization in the getHttpTransports() method though.

We probably could stash off the client instance and then do initialization in the getHttpTransports() method though.

Yeah. exactly

@reta Sorry, I remember now. We have to return the transport instance here to allow the core to manage its lifecycle (it implements LifecycleComponent). This is why we have to do the weird initialization here but grab the needed network dependency in the other callback.

The core doesn't need to know anything specific about this component, but it does need to start and stop it at node startup/shutdown.

Should be fully addressable by #16534 (comment)

reta · 2024-12-13T18:38:57Z

plugins/transport-grpc/src/main/java/org/opensearch/transport/grpc/GrpcModulePlugin.java

+        // This is something of a hack to capture the network service via the HTTP transport extension
+        // point, but unless/until a specific gRPC extension point is needed in the future this will work.
+        transport.setNetworkService(networkService);
+        return Collections.emptyMap();


I suspect we have to use GRPC + H2 (which means TLS), so the right hook would be getSecureHttpTransports, also please return the transport so it could be configured:

Suggested change

return Collections.emptyMap();

return Collections.singleton("grpc", transport);

The gRPC class isn't actually a HttpServerTransport so it can't be returned here. The whole HttpServerTransport framework is really a low-level transport that the server builds the REST framework on top of. The gRPC framework is both the low-level transport and the application protocol and doesn't really fit in the same way as HttpServerTransport.

Agree we'll probably need to hook into the secure version but figured we could punt that until TLS is implemented.

Agree we'll probably need to hook into the secure version but figured we could punt that until TLS is implemented.

Hm ... so basically the larger question here is:

we need new kind of transports support hooks, and

possibility to configure more than one (since those are complementary and not a replacement)

Yeah, I think there will be a lot more to come here, but I think this approach is okay to let us start chipping away at it. This basic structure allows the plugin to "own" pretty much everything about the new transport and the core doesn't need to really know anything. The only exception here was enabling the security manager to allow the port to be opened and I'm definitely open to better options there.

Suggestions provided (#16534 (comment)), very simple non-intrusive APIs to allow transport activation

I kind of like the idea of not touching the plugin interface until it was truly necessary and we had more clarity on what would be needed. Do you think it is better to add a new interface right away?

It is experimental anyway, right?

Yeah, definitely. Just chatted with Finn and we'll add this interface.

reta · 2024-12-13T18:42:53Z

server/src/main/java/org/opensearch/bootstrap/Security.java

+     * @param settings the {@link Settings} instance to read the gRPC settings from
+     */
+    private static void addSocketPermissionForGrpc(final Permissions policy, final Settings settings) {
+        final String grpcRange = GrpcTransportSettings.SETTING_GRPC_PORT.get(settings).getPortRangeString();


This looks wrong, the GRCP plugin policy has to include the permission, it may need placeholders support for port ranges though (but it could be implemented).

I think that was the hangup? The policy must be hard-coded but the specific port needs to be user-configurable.

We do allow placeholders (for code bases fe), we could implement the placeholders for ports (or open up some limited subset fe)

We'd need to get the specific value from the configuration at runtime at startup. Do we have any existing examples like that?

So something like addSocketPermissionForPluginTransports and allow each plugin to consume from the available pool?

We'd need to get the specific value from the configuration at runtime at startup. Do we have any existing examples like that?

Yes, we do, check please Security::addSocketPermissionForTransportProfiles

reta · 2024-12-13T18:44:46Z

plugins/transport-grpc/src/main/java/org/opensearch/transport/grpc/GrpcModulePlugin.java

+        Supplier<RepositoriesService> repositoriesServiceSupplier
+    ) {
+        if (FeatureFlags.isEnabled(FeatureFlags.GRPC_ENABLE_SETTING) == false) {
+            throw new IllegalArgumentException("transport-grpc is experimental and feature flag must be enabled before use");


This exception should only trigger when the http.type includes to grcp, installing the plugin without activation should cause no harm though

reta · 2024-12-13T18:46:22Z

server/src/main/java/org/opensearch/bootstrap/Security.java

@@ -402,6 +405,10 @@ static void addFilePermissions(Permissions policy, Environment environment) thro
    private static void addBindPermissions(Permissions policy, Settings settings) {
        addSocketPermissionForHttp(policy, settings);
        addSocketPermissionForTransportProfiles(policy, settings);
+
+        if (FeatureFlags.isEnabled(GRPC_ENABLE_SETTING)) {


I believe the bootstrap should know nothing about GRPC optional plugin

Signed-off-by: Finn Carroll <carrofin@amazon.com>

The transport-grpc plugin starts a gRPC server which is injected into the Node as a lifecycle component and gated behind an experimental feature flag. This server runs on a configurable port range in parallel to the transport-http server which still hosts the familiar REST API. Only two initial services are registered to the server, 'list' and 'Health/Check'. Add initial housekeeping for the new plugin and its dependencies: - Add boilerplate netty transport security policy to plugin - Populate plugins/transport-grpc/licenses with new gRPC dependency licenses & hashes - Initial server start/stop unit test - Initial integ test case example - Guava removed as a forbidden dependecy - Bump project wide gRPC version 1.68.0 -> 1.68.2 - Update discovery-gce/repository-gcs grpc sha1 to accommodate version bump - Add socket permissions for gRPC server in Security.java Signed-off-by: Finn Carroll <carrofin@amazon.com>

Signed-off-by: Finn Carroll <carrofin@amazon.com>

reta · 2024-12-13T18:56:56Z

server/src/main/java/org/opensearch/common/network/NetworkService.java

+     * @param publishInetAddress address published for the server
+     * @return Resolved port. If publishPort is negative and no port can be resolved return publishPort.
+     */
+    public static int resolvePublishPort(int publishPort, List<TransportAddress> boundAddresses, InetAddress publishInetAddress) {


I think we could generalize the implementation with TcpTransport.resolvePublishPort under Transport interface?

github-actions · 2024-12-13T19:01:22Z

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

reta · 2024-12-13T19:05:13Z

@finnegancarroll this is great draft but I think we are largely missing the integration with core at the moment. I believe this is by design, have no issues with that, but we have to at least draft the API hooks for generic transports exposure and activation. Something very basic would be fine:

public interface ServerTransport extends LifecycleComponent {
 ....
}

    default Map<String, Supplier<ServerTransport>> getServerTransports(
        Settings settings,
        ThreadPool threadPool,
        BigArrays bigArrays,
        PageCacheRecycler pageCacheRecycler,
        CircuitBreakerService circuitBreakerService,
        NamedXContentRegistry xContentRegistry,
        NetworkService networkService,
        HttpServerTransport.Dispatcher dispatcher,
        ClusterSettings clusterSettings,
        SecureHttpTransportSettingsProvider secureHttpTransportSettingsProvider,
        Tracer tracer
    ) {
         ....
    }

github-actions · 2024-12-13T19:20:59Z

❌ Gradle check result for 4d47a70: 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 mentioned this pull request Nov 4, 2024

[META] gRPC Server #16556

Open

1 task

finnegancarroll force-pushed the grpc branch from eda1c04 to 5038cf3 Compare November 14, 2024 18:54

amberzsy reviewed Nov 18, 2024

View reviewed changes

server/src/main/proto/spec/common.proto Outdated Show resolved Hide resolved

finnegancarroll changed the base branch from main to feature/grpc November 23, 2024 00:17

finnegancarroll force-pushed the grpc branch from da16c66 to 1937f5f Compare November 23, 2024 00:23

finnegancarroll marked this pull request as ready for review November 23, 2024 00:24

finnegancarroll requested review from peternied, anasalkouz, andrross, ashking94, Bukhtawar, CEHENKLE and dblock as code owners November 23, 2024 00:24

finnegancarroll force-pushed the grpc branch from c49cbc0 to 74680e8 Compare December 9, 2024 21:20

amberzsy reviewed Dec 9, 2024

View reviewed changes

plugins/transport-grpc/src/main/proto/opensearch/main_action.proto Outdated Show resolved Hide resolved

finnegancarroll force-pushed the grpc branch from f80ae9c to 62d4522 Compare December 9, 2024 22:10

andrross reviewed Dec 9, 2024

View reviewed changes

gradle/forbidden-dependencies.gradle Show resolved Hide resolved

andrross reviewed Dec 9, 2024

View reviewed changes

server/src/main/java/org/opensearch/common/util/FeatureFlags.java Outdated Show resolved Hide resolved

andrross reviewed Dec 9, 2024

View reviewed changes

plugins/transport-grpc/build.gradle Outdated Show resolved Hide resolved

finnegancarroll force-pushed the grpc branch from 7d90b9c to ac33ea2 Compare December 11, 2024 17:31

andrross reviewed Dec 13, 2024

View reviewed changes

.../src/internalClusterTest/java/org/opensearch/transport/grpc/OpenSearchGrpcIntegTestCase.java Outdated Show resolved Hide resolved

reta reviewed Dec 13, 2024

View reviewed changes

finnegancarroll added 4 commits December 13, 2024 10:52

Move resolvePublishPort to NetworkService for plugin re-use

09657d6

Signed-off-by: Finn Carroll <carrofin@amazon.com>

Update guava license sha

7781230

Signed-off-by: Finn Carroll <carrofin@amazon.com>

Remove IntegTestCase definition. Unused.

4d47a70

Signed-off-by: Finn Carroll <carrofin@amazon.com>

reta reviewed Dec 13, 2024

View reviewed changes

finnegancarroll changed the base branch from feature/grpc to main December 13, 2024 19:05

finnegancarroll force-pushed the grpc branch from 542656a to 4d47a70 Compare December 13, 2024 19:09

	return Collections.emptyMap();
	return Collections.singleton("grpc", transport);

Experimental gRPC server #16534

Are you sure you want to change the base?

Experimental gRPC server #16534

Conversation

finnegancarroll commented Oct 31, 2024 • edited Loading

Description

Running with transport-grpc plugin

Testing (grpcurl):

Related Issues

Check List

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Nov 7, 2024

github-actions bot commented Nov 7, 2024

github-actions bot commented Nov 8, 2024

github-actions bot commented Nov 14, 2024

github-actions bot commented Nov 14, 2024

github-actions bot commented Nov 14, 2024

github-actions bot commented Nov 14, 2024

github-actions bot commented Nov 14, 2024

github-actions bot commented Nov 15, 2024

github-actions bot commented Nov 15, 2024

github-actions bot commented Nov 15, 2024

github-actions bot commented Nov 15, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 22, 2024

github-actions bot commented Dec 9, 2024

github-actions bot commented Dec 9, 2024

github-actions bot commented Dec 9, 2024

github-actions bot commented Dec 9, 2024

github-actions bot commented Dec 9, 2024

andrross commented Dec 9, 2024

github-actions bot commented Dec 11, 2024

andrross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrross Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrross Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2024

reta commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

finnegancarroll commented Oct 31, 2024 •

edited

Loading

andrross Dec 13, 2024 •

edited

Loading

andrross Dec 13, 2024 •

edited

Loading