fix: switch HTTP gateway dial to grpc.NewClient and add gRPC host config#2793
fix: switch HTTP gateway dial to grpc.NewClient and add gRPC host config#2793
Conversation
📝 WalkthroughWalkthroughServer host became configurable; gRPC client creation was refactored to use non-blocking client construction with composite host:port addressing for both gRPC client target and HTTP server binding, and connection-close error handling was adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config
participant Server as App Server
participant GRPC as gRPC Service
participant HTTP as HTTP Server
Note over Config,Server: Startup
Config->>Server: provide Host, GRPC.Port, HTTP.Port
Server->>GRPC: build targetAddr = JoinHostPort(Host, GRPC.Port)
Server->>GRPC: grpc.NewClient(targetAddr, options...) (non-blocking)
alt TLS present
Server->>GRPC: include transport credentials
end
Server->>HTTP: build httpAddr = JoinHostPort(Host, HTTP.Port)
Server->>HTTP: http.Server{Addr: httpAddr}.ListenAndServe()
Note right of Server: On shutdown
Server->>GRPC: client.Close()
GRPC-->>Server: closeErr (logged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/config/config.go (1)
46-49: LGTM — minor comment clarification opportunityThe new
Hostfield and its default value are correct. Worth noting: the gRPC server itself still binds to0.0.0.0(via:"+srv.GRPC.Portin server.go);Hostis used exclusively as the dial target for the HTTP→gRPC proxy connection. The struct comment says "Host for the gRPC server" which implies server-side bind, but it's really the dial address. Consider:- Host string `mapstructure:"host"` // Host for the gRPC server + Host string `mapstructure:"host"` // Host used by the HTTP gateway to dial the gRPC server🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 46 - 49, Update the struct comment for the GRPC.Host field to clarify it is the dial/target address used by the HTTP→gRPC proxy (client side), not the server bind address; reference the GRPC.Host field in the GRPC struct and mention that server binding is done elsewhere (e.g., server.go uses :"+srv.GRPC.Port") so maintainers understand Host is for dialing the gRPC server.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/servers/server.go`:
- Around line 344-347: The HTTP server is incorrectly using targetAddr (which is
net.JoinHostPort(srv.GRPC.Host, srv.GRPC.Port)) as its listen address causing
port conflicts; change the HTTP server Addr to the correct HTTP listen address
by constructing net.JoinHostPort(srv.HTTP.Host, srv.HTTP.Port) (or equivalent
httpListenAddr) when creating the http.Server that wraps corsHandler, and ensure
any related log messages use srv.HTTP.Port to reflect the actual HTTP bind.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 46-49: Update the struct comment for the GRPC.Host field to
clarify it is the dial/target address used by the HTTP→gRPC proxy (client side),
not the server bind address; reference the GRPC.Host field in the GRPC struct
and mention that server binding is done elsewhere (e.g., server.go uses
:"+srv.GRPC.Port") so maintainers understand Host is for dialing the gRPC
server.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2793 +/- ##
==========================================
- Coverage 82.64% 82.62% -0.02%
==========================================
Files 74 74
Lines 8125 8126 +1
==========================================
- Hits 6714 6713 -1
- Misses 892 894 +2
Partials 519 519 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/servers/server.go (1)
261-276:⚠️ Potential issue | 🟠 MajorUse
localhostor127.0.0.1explicitly as the gRPC dial target instead of relying onsrv.HostThe
grpc.NewClientAPI and option handling are correct for v1.78 (bothgrpc.WithTransportCredentials()andinsecure.NewCredentials()are fully supported, and removal ofgrpc.WithBlock()is the right call for non-blocking lazy dial).However, there is a semantic mismatch on line 272:
srv.Hostis defined as a server bind address in the configuration and defaults to an empty string. When empty,net.JoinHostPort("", srv.GRPC.Port)produces:port, which creates a dial target with an empty hostname. The gRPC resolver's behavior with empty hostnames is undefined and platform-dependent—on some systems this may resolve tolocalhost, on others it may fail.Since the gRPC server listener (line 229) binds to all interfaces with
net.Listen("tcp", ":"+srv.GRPC.Port), the HTTP gateway should consistently diallocalhostor127.0.0.1:-targetAddr := net.JoinHostPort(srv.Host, srv.GRPC.Port) +targetAddr := net.JoinHostPort("localhost", srv.GRPC.Port)This ensures reliable local in-process communication and removes the dependency on the server bind address configuration for client-side dialing.
Note: There is also an inconsistency where the gRPC server listener ignores
srv.Hostentirely (always binding to all interfaces), while the HTTP server respects it. If the intent is for both to respectsrv.Host, the gRPC listener at line 229 should also be updated to usenet.JoinHostPort(srv.Host, srv.GRPC.Port).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/server.go` around lines 261 - 276, The dial target should not use srv.Host (which may be empty) because net.JoinHostPort(srv.Host, srv.GRPC.Port) can yield an empty hostname; update the code that builds targetAddr (used by grpc.NewClient) to explicitly use "localhost" or "127.0.0.1" (e.g., net.JoinHostPort("127.0.0.1", srv.GRPC.Port)) so the HTTP gateway reliably dials the local gRPC server; also consider making the gRPC server listener (where net.Listen is called for the gRPC server) consistent with srv.Host if you intend both to respect the configured bind address.
🧹 Nitpick comments (1)
internal/servers/server.go (1)
228-229: gRPC server listener doesn't respectsrv.Host— inconsistent with the PR's stated goalThe HTTP server bind (Line 344) now uses
srv.Host, but the gRPC listener still hard-codes all-interfaces via":"+srv.GRPC.Port. For full "configurable host" coverage, consider aligning:♻️ Proposed change for consistency
- lis, err = net.Listen("tcp", ":"+srv.GRPC.Port) + lis, err = net.Listen("tcp", net.JoinHostPort(srv.Host, srv.GRPC.Port))Note: if
srv.Hostdefaults to""or"0.0.0.0", this produces the same behaviour as today (bind all interfaces), so it's backward-compatible in the common case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/server.go` around lines 228 - 229, The gRPC listener currently binds to all interfaces by using ":"+srv.GRPC.Port; update the listener creation (where lis and err are set via net.Listen) to use srv.Host together with srv.GRPC.Port (e.g., build an address from srv.Host and srv.GRPC.Port or use net.JoinHostPort) so the gRPC server respects the configured host like the HTTP server does; ensure empty or "0.0.0.0" host values continue to bind all interfaces to preserve backward compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/servers/server.go`:
- Around line 261-276: The dial target should not use srv.Host (which may be
empty) because net.JoinHostPort(srv.Host, srv.GRPC.Port) can yield an empty
hostname; update the code that builds targetAddr (used by grpc.NewClient) to
explicitly use "localhost" or "127.0.0.1" (e.g., net.JoinHostPort("127.0.0.1",
srv.GRPC.Port)) so the HTTP gateway reliably dials the local gRPC server; also
consider making the gRPC server listener (where net.Listen is called for the
gRPC server) consistent with srv.Host if you intend both to respect the
configured bind address.
---
Nitpick comments:
In `@internal/servers/server.go`:
- Around line 228-229: The gRPC listener currently binds to all interfaces by
using ":"+srv.GRPC.Port; update the listener creation (where lis and err are set
via net.Listen) to use srv.Host together with srv.GRPC.Port (e.g., build an
address from srv.Host and srv.GRPC.Port or use net.JoinHostPort) so the gRPC
server respects the configured host like the HTTP server does; ensure empty or
"0.0.0.0" host values continue to bind all interfaces to preserve backward
compatibility.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/config/config.gointernal/servers/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/config.go
1 participant
Summary by CodeRabbit
New Features
Improvements