refactor: use pure gRPC instead of grpc-web wrapper on the provider side#2194
Open
nimrod-teich wants to merge 5 commits intomainfrom
Open
refactor: use pure gRPC instead of grpc-web wrapper on the provider side#2194nimrod-teich wants to merge 5 commits intomainfrom
nimrod-teich wants to merge 5 commits intomainfrom
Conversation
nimrod-teich
force-pushed
the
refactor/provider-listener-pure-grpc
branch
from
603a3fc to
cadf97a
Compare
NadavLevi
force-pushed
the
refactor/provider-listener-pure-grpc
branch
from
df807cf to
88bb60e
Compare
BREAKING CHANGE: gRPC-web protocol is no longer supported. Browser-based clients that relied on grpc-web to communicate directly with providers will no longer work. Only native gRPC clients are supported. The previous implementation used grpc-web and h2c HTTP wrappers to support the Lava SDK (lava-sdk/lavajs) which allowed browser-based JavaScript clients to communicate with providers. Since SDK support has been removed (see #2186), this infrastructure is no longer needed. Replace the grpc-web/h2c/HTTP hybrid server with a cleaner architecture: - Use cmux to multiplex HTTP health checks and native gRPC on same port - Remove grpcweb.WrapServer and h2c.NewHandler complexity - Add gRPC health checking protocol support (grpc_health_v1) - Keep HTTP health endpoint for Kubernetes probes compatibility - Optimize cmux matching: HTTP1Fast() first, then Any() for gRPC (avoids expensive HTTP/2 header parsing at high scale) Benefits: - Native gRPC connection management (keepalives, stream limits) - Proper graceful shutdown via grpcServer.GracefulStop() - Simpler, more maintainable code - Better performance at scale (no protocol translation overhead) Also adds comprehensive test suite for provider_listener.go with 14 tests and 2 benchmarks covering HTTP health, gRPC health, relay/probe RPCs, concurrent requests, and error handling.
gRPC handles compression automatically when the gzip compressor is registered (via import) and the client uses grpc.UseCompressor(gzip.Name). Removed: - Manual compression in provider (common.CompressData) - Manual decompression in consumer/smartrouter (common.DecompressData) - Custom LavaCompressionSupportHeader and LavaCompressionHeader headers - Obsolete rpcsmartrouter_compression_test.go The --enable-application-level-compression flag still works, but now uses native gRPC compression instead of manual application-level compression. This is simpler, more efficient, and follows gRPC best practices. Note: common.CompressData/DecompressData utilities are kept as they're still used for cache storage compression.
- Renamed from --enable-application-level-compression to --enable-grpc-compression (more accurate now that we use native gRPC compression) - Updated flag description to explain the trade-off (bandwidth vs CPU) - Improved log message when compression is enabled
Add comprehensive tests verifying native gRPC compression works correctly between consumer and provider: - TestGRPCCompressionEnabled: verifies data is compressed (99%+ reduction) - TestGRPCCompressionDisabled: verifies no compression without flag - TestGRPCCompressionBidirectional: verifies both request and response compression - TestGRPCCompressionWithConnectGRPCClient: tests via actual ConnectGRPCClient function - TestGRPCCompressionSmallPayload: verifies small payload handling - BenchmarkGRPCWithCompression/WithoutCompression: performance comparison Uses gRPC stats handler to measure actual wire vs logical payload sizes.
nimrod-teich
force-pushed
the
refactor/provider-listener-pure-grpc
branch
from
88bb60e to
975a583
Compare
Review Summary by QodoReplace grpc-web with pure gRPC and native compression
WalkthroughsDescription• Replace grpc-web/h2c hybrid server with pure gRPC using cmux multiplexing - Use native gRPC compression instead of manual application-level compression - Add gRPC health checking protocol support (grpc_health_v1) - Keep HTTP health endpoint for Kubernetes compatibility • Remove custom compression headers and manual compress/decompress logic - Compression now handled automatically by gRPC layer via gzip compressor - Simplifies relay path in consumer and provider implementations • Rename flag from enable-application-level-compression to enable-grpc-compression - Update flag descriptions and log messages for clarity • Add comprehensive test suite with 14 tests and 2 benchmarks - Cover HTTP health checks, gRPC health service, relay/probe RPCs, concurrent requests Diagramflowchart LR
A["grpc-web/h2c<br/>Hybrid Server"] -->|refactor| B["Pure gRPC<br/>with cmux"]
C["Manual<br/>Compression"] -->|replace| D["Native gRPC<br/>gzip"]
B --> E["HTTP Health<br/>Listener"]
B --> F["gRPC<br/>Listener"]
E -->|multiplexed| G["Single Port"]
F -->|multiplexed| G
D --> H["Automatic<br/>Compression"]
File Changes1. protocol/lavasession/consumer_types.go
|
Code Review by Qodo
1. TLS breaks HTTP healthcheck
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+90
to
+104
| // Wrap with TLS if enabled | ||
| if !networkAddress.DisableTLS { | ||
| tlsConfig := lavasession.GetTlsConfig(networkAddress) | ||
| lis = tls.NewListener(lis, tlsConfig) | ||
| } else { | ||
| utils.LavaFormatInfo("Running with disabled TLS configuration") | ||
| } | ||
|
|
||
| pl.httpServer = http.Server{ | ||
| Handler: h2c.NewHandler(http.HandlerFunc(handler), &http2.Server{}), | ||
| } | ||
| // Create connection multiplexer to handle both HTTP and gRPC on same port | ||
| mux := cmux.New(lis) | ||
| // Match HTTP/1.1 first for health checks (fast prefix match) | ||
| httpListener := mux.Match(cmux.HTTP1Fast()) | ||
| // Everything else goes to gRPC (avoids expensive header parsing) | ||
| grpcListener := mux.Match(cmux.Any()) | ||
| pl.cmux = mux |
There was a problem hiding this comment.
1. Tls breaks http healthcheck 🐞 Bug ✓ Correctness
When TLS is enabled (DisableTLS=false), the listener is wrapped with TLS before cmux matching, so cmux.HTTP1Fast() cannot detect HTTP health probes and routes them to the gRPC listener. This breaks Kubernetes HTTP probes (and any HTTPS GET /lava/health) on TLS-enabled providers.
Agent Prompt
### Issue description
TLS wrapping occurs before cmux matching, which prevents cmux from distinguishing HTTP health probes from gRPC traffic. This makes the HTTP health endpoint unreachable when TLS is enabled.
### Issue Context
- `NetworkAddressData.DisableTLS` defaults to `false` when omitted, so TLS is commonly enabled.
- cmux matchers like `HTTP1Fast()` require plaintext HTTP method bytes.
### Fix Focus Areas
- protocol/rpcprovider/provider_listener.go[85-104]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
+98
to
+109
| // Create connection multiplexer to handle both HTTP and gRPC on same port | ||
| mux := cmux.New(lis) | ||
| // Match HTTP/1.1 first for health checks (fast prefix match) | ||
| httpListener := mux.Match(cmux.HTTP1Fast()) | ||
| // Everything else goes to gRPC (avoids expensive header parsing) | ||
| grpcListener := mux.Match(cmux.Any()) | ||
| pl.cmux = mux | ||
|
|
||
| var serveExecutor func() error | ||
| if networkAddress.DisableTLS { | ||
| utils.LavaFormatInfo("Running with disabled TLS configuration") | ||
| serveExecutor = func() error { | ||
| return pl.httpServer.Serve(lis) | ||
| } | ||
| } else { | ||
| pl.httpServer.TLSConfig = lavasession.GetTlsConfig(networkAddress) | ||
| serveExecutor = func() error { | ||
| return pl.httpServer.ServeTLS(lis, "", "") | ||
| } | ||
| // Build gRPC server | ||
| opts := []grpc.ServerOption{ | ||
| grpc.MaxRecvMsgSize(1024 * 1024 * 512), // 512MB for large debug responses | ||
| grpc.MaxSendMsgSize(1024 * 1024 * 512), // 512MB for large debug responses |
There was a problem hiding this comment.
grpc-web/h2c was removed from the provider listener so browser gRPC-Web clients now get plain gRPC with no CORS — should we restore grpcweb.WrapServer or add a dedicated grpc-web handler?
Finding type: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In protocol/rpcprovider/provider_listener.go around lines 98 to 161, the
NewProviderListener logic removed grpc-web/h2c support and now calls grpcServer.Serve
directly on the cmux gRPC listener, which breaks browser grpc-web clients (no CORS and
no grpc-web protocol handling). Reintroduce grpc-web handling by wrapping grpcServer
with grpcweb.WrapServer and creating an HTTP handler that: (1) sets CORS headers
(Access-Control-Allow-Origin: * and allowed headers including x-grpc-web and the LAVA
consumer header), (2) responds to the configured healthCheckPath GET with 200/"Healthy",
and (3) delegates other requests to the wrapped grpc-web server. Attach that handler to
an http.Server served on the cmux HTTP listener (use h2c.NewHandler if you need
cleartext HTTP/2 support), and keep grpcServer.Serve running on the grpcListener as-is.
Keep TLS wrapping and cmux matching unchanged.
Comment on lines
486
to
+488
| lavasession.AllowGRPCCompressionForConsumerProviderCommunication = viper.GetBool(lavasession.AllowGRPCCompressionFlag) | ||
| if lavasession.AllowGRPCCompressionForConsumerProviderCommunication { | ||
| utils.LavaFormatInfo("AllowGRPCCompressionForConsumerProviderCommunication is set to true, messages will be compressed", utils.Attribute{Key: lavasession.AllowGRPCCompressionFlag, Value: lavasession.AllowGRPCCompressionForConsumerProviderCommunication}) | ||
| utils.LavaFormatInfo("gRPC compression enabled, relay messages will use gzip compression", utils.Attribute{Key: lavasession.AllowGRPCCompressionFlag, Value: lavasession.AllowGRPCCompressionForConsumerProviderCommunication}) |
There was a problem hiding this comment.
The new block that reads lavasession.AllowGRPCCompressionForConsumerProviderCommunication from Viper and logs that compression is enabled is duplicated verbatim in protocol/rpcsmartrouter/rpcsmartrouter.go (lines 690‑693), and the Flags().Bool(lavasession.AllowGRPCCompressionFlag, …) registration right after it is also identical to the one in the smart router cli (lines 994‑996). Any future change to the flag name/description, default, or logging would have to be applied in both files, which is easy to miss. Can we extract a shared helper to register this flag and apply its configuration (e.g., common.RegisterGrpcCompressionFlag(cmd) + common.ApplyGrpcCompressionFlag(viper)) so the behaviour lives in one place instead of being copy-pasted?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
Replace the grpc-web/h2c/HTTP hybrid server with a cleaner architecture using pure gRPC and native compression.
Background
The previous implementation used grpc-web and h2c HTTP wrappers to support the Lava SDK (lava-sdk/lavajs), which allowed browser-based JavaScript clients to communicate with providers. Since SDK support has been removed (see #2186), this infrastructure is no longer needed.
Changes
1. Pure gRPC Provider Listener
Replace the hybrid server with a cleaner architecture:
cmuxto multiplex HTTP health checks and native gRPC on the same portgrpcweb.WrapServerandh2c.NewHandlercomplexitygrpc_health_v1)HTTP1Fast()first, thenAny()for gRPC (avoids expensive HTTP/2 header parsing at high scale)Benefits:
grpcServer.GracefulStop()2. Native gRPC Compression
Replace manual application-level compression with native gRPC gzip:
common.CompressData()/common.DecompressData()from relay pathLavaCompressionHeader,LavaCompressionSupportHeader)import _ "google.golang.org/grpc/encoding/gzip"grpc.UseCompressor(gzip.Name)when flag is enabled3. Flag Rename
--enable-application-level-compression→--enable-grpc-compressionBREAKING CHANGES
gRPC-web Protocol Removed
gRPC-web protocol is no longer supported. Browser-based clients that relied on grpc-web to communicate directly with providers will no longer work. Only native gRPC clients are supported.
Who is affected: Only the deprecated lava-sdk (browser clients) which has already been removed.
Compression Flag Renamed
The
--enable-application-level-compressionflag has been renamed to--enable-grpc-compression.Migration: Update any scripts, configurations, or Kubernetes manifests using the old flag name.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changemainbranchReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Replace the provider listener with a
cmux-managedgrpc.Serverthat keeps TLS, HTTP health probes, and gRPC health checks running without grpc-web while ensuring graceful shutdown and registered health states. Align the relay path with native gzip compression across the consumer, smart router, and provider flows so the existingAllowGRPCCompressionflag leverages gRPC’s built-in compressors instead of ad hoc headers.Modified files (7)
Latest Contributors(2)
Modified files (1)
Latest Contributors(1)
grpc.Servermultiplexed bycmux, register the gRPC health service, keep the HTTP health probe for Kubernetes, and handle TLS/graceful shutdown without grpc-web while including the new dependencies (cmuxupgrade, gzip registration) and expanded provider listener tests.Modified files (4)
Latest Contributors(2)