add http2 grpc app #3039
add http2 grpc app #3039
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Summary of ChangesHello @randygrok, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the gRPC client's interoperability by enabling it to connect to a wider range of gRPC servers. By introducing a custom HTTP/2 client that supports cleartext connections and explicitly configuring the native gRPC protocol, the client can now seamlessly interact with standard gRPC implementations, including those without TLS. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude finished @randygrok's task in 2m 41s —— View job Code Review CompleteI've performed a comprehensive review of the HTTP/2 gRPC client changes. Here are my findings: SummaryThis PR adds HTTP/2 cleartext (h2c) support to the gRPC client, enabling it to connect to native gRPC servers without TLS. The changes are minimal and focused, adding a custom HTTP/2 client and enforcing the native gRPC protocol. Overall Assessment: The implementation is functionally sound and achieves its stated goal. However, there are several areas that need attention regarding resource management, security configuration, testing, and documentation. Critical Issues1. Resource Leak: Missing Timeout Configuration 🔴File: The Issue: return &http.Client{
Transport: &http2.Transport{
AllowHTTP: true,
DialTLSContext: func(ctx context.Context, network, addr string, _ *tls.Config) (net.Conn, error) {
var d net.Dialer
return d.DialContext(ctx, network, addr)
},
},
}The Impact:
Recommendation: func newHTTP2Client() *http.Client {
return &http.Client{
Timeout: 30 * time.Second, // Overall request timeout
Transport: &http2.Transport{
AllowHTTP: true,
DialTLSContext: func(ctx context.Context, network, addr string, _ *tls.Config) (net.Conn, error) {
d := net.Dialer{
Timeout: 10 * time.Second, // Connection timeout
KeepAlive: 30 * time.Second, // TCP keepalive
}
return d.DialContext(ctx, network, addr)
},
// Consider adding these for production resilience:
ReadIdleTimeout: 30 * time.Second,
PingTimeout: 15 * time.Second,
MaxReadFrameSize: 1 << 20, // 1MB
},
}
}2. HTTP/2 Transport Configuration Mismatch 🟡File: The client's HTTP/2 transport configuration is minimal compared to the server's well-configured settings. Server configuration (handler.go): return h2c.NewHandler(mux, &http2.Server{
IdleTimeout: 120 * time.Second,
MaxReadFrameSize: 1 << 24, // 16MB
MaxConcurrentStreams: 100,
ReadIdleTimeout: 30 * time.Second,
PingTimeout: 15 * time.Second,
})Client configuration (client.go): Transport: &http2.Transport{
AllowHTTP: true,
DialTLSContext: func(...) {...},
// Missing: frame size limits, stream limits, idle timeouts, ping config
}Recommendation: Align client transport settings with server expectations for better reliability and performance. See the timeout fix above. 3. Security: TLS Config Completely Bypassed 🟡File: The DialTLSContext: func(ctx context.Context, network, addr string, _ *tls.Config) (net.Conn, error) {
var d net.Dialer
return d.DialContext(ctx, network, addr)
},Issue:
Impact:
Recommendation:
Example of scheme-based approach: func NewClient(url string, opts ...connect.ClientOption) *Client {
httpClient := newHTTP2Client(url) // Detects http:// vs https://
opts = append([]connect.ClientOption{connect.WithGRPC()}, opts...)
return &Client{
client: v1connect.NewExecutorServiceClient(httpClient, url, opts...),
}
}
func newHTTP2Client(url string) *http.Client {
// If https://, use proper TLS transport
// If http://, use h2c transport
}Architecture & Code Quality4. Function Signature Breaking Change 🟡File: The func NewClient(url string, opts ...connect.ClientOption) *Client {
// Prepend WithGRPC to use the native gRPC protocol (required for tonic/gRPC servers)
opts = append([]connect.ClientOption{connect.WithGRPC()}, opts...)
return &Client{
client: v1connect.NewExecutorServiceClient(
newHTTP2Client(),
url,
opts...,
),
}
}Issue:
Impact:
Recommendation:
5. Missing Documentation 📄File: The
Current documentation: // newHTTP2Client creates an HTTP/2 client that supports cleartext (h2c) connections.
// This is required to connect to native gRPC servers without TLS.Recommended documentation: // newHTTP2Client creates an HTTP/2 client that supports cleartext (h2c) connections.
// This is required to connect to native gRPC servers (e.g., tonic, grpc-go) without TLS.
//
// Security Warning: This client sends all data in cleartext without encryption.
// Only use this for:
// - Local development and testing
// - Internal networks with network-level security
// - Communication within a trusted environment
//
// For production deployments over untrusted networks, use TLS-enabled connections.
//
// The client uses HTTP/2 protocol negotiation via h2c (HTTP/2 Cleartext) as defined
// in RFC 7540. This allows direct HTTP/2 communication without the TLS ALPN handshake.Testing6. No Test Coverage for New Code 🔴Files: The existing tests use Missing test coverage:
Recommendation:
Example test structure: func TestClient_HTTP2ClearText(t *testing.T) {
// Start real h2c server (not httptest)
handler := NewExecutorServiceHandler(&mockExecutor{})
listener, err := net.Listen("tcp", "127.0.0.1:0")
require.NoError(t, err)
defer listener.Close()
server := &http.Server{Handler: handler}
go server.Serve(listener)
defer server.Close()
// Test client connection
client := NewClient("http://" + listener.Addr().String())
// ... test operations
}7. Behavioral Change Not Reflected in Tests 🟡The change from Recommendation: Add tests that verify:
Performance & Resource Efficiency8. New Client Created Per Instance 🟡File: Each call to client: v1connect.NewExecutorServiceClient(
newHTTP2Client(), // New client every time
url,
opts...,
),Issue:
Current behavior: Each executor client has isolated connections. Consideration: This may be intentional for isolation, but consider:
Additional Observations9. Positive: Minimal and Focused Change ✅The PR makes minimal changes to achieve its goal:
10. Positive: Good Code Comments ✅The inline comment explaining why // Prepend WithGRPC to use the native gRPC protocol (required for tonic/gRPC servers)11. Empty PR Description 📄The PR description only contains "## Overview" with no content. Consider adding:
Recommendations SummaryMust Fix (Critical)
Should Fix (Important)
Consider
ConclusionThe implementation achieves its goal of h2c support but needs refinement before production use. The most critical issues are missing timeouts (resource leak risk) and lack of test coverage for the new code path. The security implications of cleartext-only communication should be clearly documented or addressed with TLS support. After addressing the critical issues, this will be a solid addition to ev-node's modular architecture. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for connecting to gRPC servers over HTTP/2 cleartext (h2c). This is achieved by creating a custom http.Client with a specialized transport. The changes are functionally correct, but I've identified a potential issue with the lack of timeouts in the new HTTP client, which could lead to hanging connections. My review includes a suggestion to add appropriate timeouts to improve the client's robustness.
| return &http.Client{ | ||
| Transport: &http2.Transport{ | ||
| AllowHTTP: true, | ||
| DialTLSContext: func(ctx context.Context, network, addr string, _ *tls.Config) (net.Conn, error) { | ||
| var d net.Dialer | ||
| return d.DialContext(ctx, network, addr) | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The custom http.Client is created without any timeouts. This can lead to the client hanging indefinitely if the server is unresponsive, potentially causing resource exhaustion.
It's a good practice to configure timeouts for production-ready clients. Since all the RPCs in this service are unary, you can safely set a timeout on the http.Client for the entire request-response cycle. Additionally, setting a timeout on the net.Dialer is recommended to avoid long waits during connection establishment.
return &http.Client{
Timeout: 60 * time.Second,
Transport: &http2.Transport{
AllowHTTP: true,
DialTLSContext: func(ctx context.Context, network, addr string, _ *tls.Config) (net.Conn, error) {
d := net.Dialer{Timeout: 15 * time.Second}
return d.DialContext(ctx, network, addr)
},
},
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3039 +/- ##
==========================================
+ Coverage 55.33% 55.37% +0.03%
==========================================
Files 117 117
Lines 11685 11694 +9
==========================================
+ Hits 6466 6475 +9
Misses 4494 4494
Partials 725 725
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overview