Repository navigation
refactor: replace custom compression with native gRPC compression - #2328
Merged
Merged
Conversation
The provider listener wrapped its gRPC server in grpc-web and h2c so that browser based lava-sdk clients could reach it. The SDK is gone, so the wrapper only costs us the supported grpc.Server.Serve path: native connection management, graceful stop, and no protocol translation. The listener now runs a real grpc.Server, with cmux routing connections by the protocol they open with: the HTTP/2 client preface goes to gRPC, everything else to the HTTP/1.1 health endpoint that kubernetes httpGet probes use. Matching the preface is a cheap prefix compare and resolves at connection time, unlike matching the gRPC content-type which would have to wait for the first request. The gRPC health checking protocol is registered alongside it for native clients and load balancers, and Shutdown now drains both servers, reports NOT_SERVING while draining, and releases the port. When TLS is enabled the listener advertises ALPN explicitly. gRPC clients offer only "h2" and refuse a connection that negotiated no protocol at all, so terminating TLS without advertising it would have broken every consumer of a TLS enabled provider. "http/1.1" is listed first because ALPN resolves in server preference order, which keeps HTTP health probes on HTTP/1.1 while gRPC clients still get "h2". Relay compression is left to gRPC as well. The consumer already dials with grpc.UseCompressor(gzip) when compression is enabled, and the server echoes that compressor back on the response, so the hand rolled compress/decompress pass and its lava-compression headers were compressing already compressed payloads. The flag is renamed to --enable-grpc-compression accordingly, with the old name still accepted so existing deployments keep working. The gzip helpers stay for the cache, which compresses entries at rest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ka44Dzff68tLrAQ5SJPmaC
PerformCORSCheck required every provider to answer an HTTPS request on any path with Access-Control-Allow-Origin and an Access-Control-Allow-Headers list containing x-grpc-web. Those headers existed only so browsers could preflight grpc-web calls, and the provider listener no longer speaks grpc-web, so the check now fails against every provider. It is a hard failure in both callers: lavap test rpcprovider reports "invalid CORS check" for every endpoint, and the health monitor marks every provider unhealthy. Nothing else is lost by removing it, both callers probe the endpoint over gRPC first, and TLS reachability is already covered by ConnectRawClientWithTimeout and its insecure retry. The cors_test.go scaffolding went with it, including the self signed certificate it wrote into the package directory on every run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ka44Dzff68tLrAQ5SJPmaC
PR Summary by QodoRefactor: replace custom relay compression with native gRPC gzip + cmux listener
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Code Review by Qodo
1. gRPC-Web routing removed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+171
to
+175
| pl.httpServer = &http.Server{ | ||
| ReadHeaderTimeout: connectionMatchTimeout, | ||
| Handler: http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { | ||
| if req.URL.Path != healthCheckPath { | ||
| resp.WriteHeader(http.StatusNotFound) |
There was a problem hiding this comment.
1. Grpc-web routing removed 🐞 Bug ≡ Correctness
ProviderListener now routes all non-HTTP/2 connections to an HTTP handler that only serves GET on the health path, so gRPC-Web requests (HTTP/1.1 POST with application/grpc-web content-types) will be rejected (typically 404) instead of reaching the Relayer service.
Agent Prompt
## Issue description
The provider listener no longer forwards HTTP/1.1 requests to a gRPC-Web-compatible handler. Because cmux routes only HTTP/2 connections to the gRPC server, any gRPC-Web client (HTTP/1.1) will hit the health-only HTTP handler and fail all RPCs.
## Issue Context
`grpcwebcompat.WrapServer` exists to correctly serve gRPC-Web requests; previously the provider listener used it, but the new implementation returns 404/405 for anything except the health endpoint.
## Fix Focus Areas
- protocol/rpcprovider/provider_listener.go[144-185]
### Suggested direction
- Keep the dedicated health endpoint behavior for `GET healthCheckPath`.
- For other HTTP/1.1 requests, route them to a `grpcwebcompat.WrapServer(pl.grpcServer)` handler (and if browser clients are expected, restore the required CORS headers / OPTIONS handling).
- If gRPC-Web support is intentionally being dropped, explicitly document that and remove/disable any remaining grpc-web expectations in configs/docs/tests.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
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.
Description
This PR replaces the custom application-level compression implementation with native gRPC compression, which is more efficient and standards-compliant. The changes simplify the codebase by removing custom compression headers and logic from the consumer and provider, delegating compression to gRPC's built-in mechanisms.
Key Changes
Removed
LavaCompressionSupportHeaderandLavaCompressionHeaderare no longer usedRPCProviderServer.Relay()andRPCConsumerServer.relayInner()protocol/common/compression_integration_test.go(custom compression flow tests)protocol/rpcprovider/cors_test.go(no longer needed with new architecture)Added
ProviderListenerto use connection multiplexing (cmux) for serving both gRPC and HTTP/1.1 on the same portgoogle.golang.org/grpc/healthfor proper gRPC health status reportingprotocol/rpcprovider/provider_listener_test.gowith coverage for:Modified
enable-application-level-compressiontoenable-grpc-compression, now controls gRPC-level compression negotiationcompression.gois for stored payloads only; wire compression is handled by gRPCTechnical Details
The new architecture leverages gRPC's native compression support:
grpc.UseCompressor(gzip)when theenable-grpc-compressionflag is setTesting
Author Checklist
!in the type prefix if API or client breaking changemainbranchReviewers Checklist
https://claude.ai/code/session_01Ka44Dzff68tLrAQ5SJPmaC