feat(httpgate): h1/grpc working in same domain#6464
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the httpgate proxy to support both HTTP/1.1 and gRPC traffic on the same domain by shifting from hostname-based protocol detection (using devbox- vs devboxgrpc- prefixes) to content-based detection using HTTP version and content-type headers.
Key Changes:
- Protocol detection now uses HTTP/2 version +
application/grpccontent-type instead of hostname prefixes - Simplified host parsing to return only
(uniqueID, port)instead of(protocol, uniqueID, port) - Updated ingress configuration to route gRPC requests using content-type header matching
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| service-rs/httpgate/src/proxy.rs | Refactored protocol detection from prefix-based to content-based (HTTP/2 + content-type header), simplified HostParser to remove protocol from return value, updated comments and documentation, removed obsolete gRPC prefix tests |
| service-rs/httpgate/deploy/charts/httpgate/templates/ingress.yaml | Updated ingress annotations to use unified devbox- prefix with Higress content-type matching for gRPC routing instead of separate devboxgrpc- prefix |
| // Detect protocol: use gRPC/H2 when request is HTTP/2 AND content-type starts with "application/grpc" | ||
| let is_h2 = session.req_header().version == Version::HTTP_2; | ||
| let is_grpc_content_type = session | ||
| .req_header() | ||
| .headers | ||
| .get("content-type") | ||
| .and_then(|ct| ct.to_str().ok()) | ||
| .is_some_and(|ct| ct.starts_with("application/grpc")); | ||
|
|
||
| let protocol = if is_h2 && is_grpc_content_type { | ||
| UpstreamProtocol::Grpc | ||
| } else { | ||
| UpstreamProtocol::Http | ||
| }; |
There was a problem hiding this comment.
The new protocol detection logic (lines 214-227) lacks test coverage. The test suite only covers host parsing but doesn't verify that protocol detection correctly identifies gRPC requests based on HTTP/2 version and content-type header. Consider adding tests for scenarios like:
- HTTP/2 request with "application/grpc" content-type (should be Grpc)
- HTTP/2 request without grpc content-type (should be Http)
- HTTP/1.1 request with grpc content-type (should be Http)
- Request without content-type header (should be Http)
| let is_grpc_content_type = session | ||
| .req_header() | ||
| .headers | ||
| .get("content-type") | ||
| .and_then(|ct| ct.to_str().ok()) | ||
| .is_some_and(|ct| ct.starts_with("application/grpc")); | ||
|
|
||
| let protocol = if is_h2 && is_grpc_content_type { | ||
| UpstreamProtocol::Grpc | ||
| } else { | ||
| UpstreamProtocol::Http | ||
| }; |
There was a problem hiding this comment.
The protocol detection logic only checks for the presence of a content-type header starting with "application/grpc", but doesn't handle requests that lack a content-type header (common for GET/HEAD requests in gRPC). According to the gRPC specification, gRPC requests should always include the content-type header, but the current implementation silently falls back to HTTP/1.1 mode if the header is missing, even for HTTP/2 requests. This could lead to incorrect routing for malformed gRPC requests. Consider logging a warning when an HTTP/2 request lacks a content-type header, or validating that gRPC content-type is present for HTTP/2 requests that use gRPC methods (POST).
| higress.io/prefix-match-header-host: devbox- | ||
| higress.io/prefix-match-header-content-type: application/grpc |
There was a problem hiding this comment.
The gRPC ingress configuration uses a custom annotation "higress.io/prefix-match-header-content-type: application/grpc" which appears to be Higress-specific. This annotation may not work with other ingress controllers (like NGINX Ingress Controller specified on line 67). The annotation on line 13 (higress.io/prefix-match-header-host) also suggests Higress-specific behavior. If this deployment is intended to work with standard NGINX Ingress Controller (indicated by the nginx.ingress.kubernetes.io annotations), the content-type based routing may not function as expected, since NGINX Ingress doesn't natively support header-based routing beyond host. Consider documenting that this configuration requires Higress ingress controller, or provide an alternative configuration for NGINX-only deployments.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2 participants
No description provided.