mirror of
https://github.com/luxfi/zap.git
synced 2026-07-26 22:53:31 +00:00
zapclient: fix native-TCP Client→Server path (opcode routing + static dial)
Two defects made the canonical zapclient native-TCP path non-functional (undetected — no test did a real Client→Server TCP round-trip; zapweb works via Dispatch, inter-shard uses raw zap.Node): 1. Server.Register bound the node handler under the full opcode, but the Node routes inbound by Flags()>>8 (the high byte). Mismatch → handler never found → call hung. Register now uses op>>8; dispatch still reads the full Flags() to select the procedure. 2. Connect with a static Discovery set the node NoDiscovery, so it never learned peer addresses and could not dial. Added WithStaticPeers + a static Discovery, ConnectDirectID (returns the handshake-learned NodeID), and client-side dial-by-address (memoized) so static peers (placeholder NodeID + real Address — the cloud/K8s Service-DNS path) are reachable. mDNS path unchanged. Adds TestNativeTCPRoundTrip as the regression guard.
This commit is contained in:
@@ -737,14 +737,27 @@ func (n *Node) getOrConnect(peerID string) (*Conn, error) {
|
||||
return conn, nil
|
||||
}
|
||||
|
||||
// ConnectDirect connects directly to a peer at the given address (bypasses mDNS).
|
||||
// ConnectDirect connects directly to a peer at the given address
|
||||
// (bypasses mDNS). Use ConnectDirectID when you need the handshake-
|
||||
// learned peer NodeID back — e.g. to address subsequent Calls to a
|
||||
// static peer whose advertised NodeID is only a placeholder.
|
||||
func (n *Node) ConnectDirect(addr string) error {
|
||||
_, err := n.ConnectDirectID(addr)
|
||||
return err
|
||||
}
|
||||
|
||||
// ConnectDirectID dials addr, performs the NodeID handshake, registers
|
||||
// the connection, and returns the peer's learned NodeID. Idempotent: if
|
||||
// a connection to that peer already exists it returns the existing
|
||||
// peer's NodeID and drops the duplicate dial. (TCP transport; the QUIC
|
||||
// path registers the peer internally and returns an empty id.)
|
||||
func (n *Node) ConnectDirectID(addr string) (string, error) {
|
||||
if n.transport == TransportQUIC {
|
||||
return n.quicConnectDirect(n.ctx, addr)
|
||||
return "", n.quicConnectDirect(n.ctx, addr)
|
||||
}
|
||||
netConn, err := net.DialTimeout("tcp", addr, 5*time.Second)
|
||||
if err != nil {
|
||||
return fmt.Errorf("failed to connect to %s: %w", addr, err)
|
||||
return "", fmt.Errorf("failed to connect to %s: %w", addr, err)
|
||||
}
|
||||
if n.tlsCfg != nil {
|
||||
netConn = tls.Client(netConn, n.tlsCfg)
|
||||
@@ -753,19 +766,19 @@ func (n *Node) ConnectDirect(addr string) error {
|
||||
// Send handshake.
|
||||
if err := writeMessage(netConn, EncodeNodeIDHandshake(n.nodeID)); err != nil {
|
||||
netConn.Close()
|
||||
return err
|
||||
return "", err
|
||||
}
|
||||
|
||||
// Read handshake response.
|
||||
data, err := readMessageRaw(netConn)
|
||||
if err != nil {
|
||||
netConn.Close()
|
||||
return err
|
||||
return "", err
|
||||
}
|
||||
peerID, ok := DecodeNodeIDHandshake(data)
|
||||
if !ok {
|
||||
netConn.Close()
|
||||
return fmt.Errorf("invalid peer handshake")
|
||||
return "", fmt.Errorf("invalid peer handshake")
|
||||
}
|
||||
|
||||
conn := &Conn{
|
||||
@@ -780,7 +793,7 @@ func (n *Node) ConnectDirect(addr string) error {
|
||||
if _, ok := n.conns[peerID]; ok {
|
||||
n.connsMu.Unlock()
|
||||
netConn.Close()
|
||||
return nil // Already connected, that's fine
|
||||
return peerID, nil // Already connected, that's fine
|
||||
}
|
||||
n.conns[peerID] = conn
|
||||
n.connsMu.Unlock()
|
||||
@@ -804,7 +817,7 @@ func (n *Node) ConnectDirect(addr string) error {
|
||||
n.dispatchLoop(netConn, conn, peerID)
|
||||
}()
|
||||
|
||||
return nil
|
||||
return peerID, nil
|
||||
}
|
||||
|
||||
// Send sends a message over the connection.
|
||||
|
||||
+65
-2
@@ -55,6 +55,11 @@ type ClientOptions struct {
|
||||
// Pluggable wiring. nil means "use default for this concern."
|
||||
Discovery Discovery
|
||||
Picker Picker
|
||||
|
||||
// StaticPeers is an explicit "host:port" peer set, bypassing mDNS.
|
||||
// The cloud/K8s path — pass the Service DNS name(s). When set (and
|
||||
// Discovery is nil) Connect builds a static Discovery over them.
|
||||
StaticPeers []string
|
||||
}
|
||||
|
||||
// ClientOption is the functional-option constructor knob.
|
||||
@@ -103,6 +108,14 @@ func WithDiscovery(d Discovery) ClientOption {
|
||||
return func(o *ClientOptions) { o.Discovery = d }
|
||||
}
|
||||
|
||||
// WithStaticPeers sets explicit "host:port" peer addresses, bypassing
|
||||
// mDNS. The cloud/K8s path: pass Service DNS names
|
||||
// (e.g. "ta.ns.svc.cluster.local:7801"). An explicit WithDiscovery wins
|
||||
// over this.
|
||||
func WithStaticPeers(addrs ...string) ClientOption {
|
||||
return func(o *ClientOptions) { o.StaticPeers = addrs }
|
||||
}
|
||||
|
||||
// WithPicker overrides the per-Call peer selector.
|
||||
func WithPicker(p Picker) ClientOption {
|
||||
return func(o *ClientOptions) { o.Picker = p }
|
||||
@@ -120,6 +133,13 @@ type Client struct {
|
||||
mu sync.Mutex
|
||||
closed bool
|
||||
stopDisc func()
|
||||
|
||||
// dialMu guards addrID, the memo of peer Address → handshake-learned
|
||||
// NodeID. Static peers advertise a placeholder NodeID; we dial their
|
||||
// Address once (ConnectDirectID) and address subsequent Calls to the
|
||||
// real NodeID the handshake returned.
|
||||
dialMu sync.Mutex
|
||||
addrID map[string]string
|
||||
}
|
||||
|
||||
// Connect resolves a service via Discovery and returns a Client.
|
||||
@@ -135,6 +155,11 @@ func Connect(ctx context.Context, serviceType string, opts ...ClientOption) (*Cl
|
||||
if o.NodeID == "" {
|
||||
o.NodeID = "zapclient-" + randomSuffix()
|
||||
}
|
||||
// Static peers (cloud/K8s Service DNS) build a static Discovery when
|
||||
// no explicit Discovery was supplied.
|
||||
if o.Discovery == nil && len(o.StaticPeers) > 0 {
|
||||
o.Discovery = newStaticDiscovery(serviceType, o.StaticPeers)
|
||||
}
|
||||
|
||||
// The client-side Node has no listener of its own — discovery is
|
||||
// the only thing it advertises. ZAP's Node currently always opens
|
||||
@@ -181,6 +206,7 @@ func Connect(ctx context.Context, serviceType string, opts ...ClientOption) (*Cl
|
||||
timeout: o.CallTimeout,
|
||||
logger: o.Logger,
|
||||
stopDisc: stopDisc,
|
||||
addrID: map[string]string{},
|
||||
}, nil
|
||||
}
|
||||
|
||||
@@ -209,11 +235,15 @@ func (c *Client) Call(ctx context.Context, procedure string, req *zap.Message) (
|
||||
ctx, cancel = context.WithTimeout(ctx, c.timeout)
|
||||
defer cancel()
|
||||
}
|
||||
target, err := c.resolvePeer(peer)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
// Tag the message's flags with the procedure opcode. The node
|
||||
// stamps a separate request-correlation header on top; the
|
||||
// procedure opcode rides in the message flags field.
|
||||
tagged := withFlags(req, op)
|
||||
return c.node.Call(ctx, peer.NodeID, tagged)
|
||||
return c.node.Call(ctx, target, tagged)
|
||||
}
|
||||
|
||||
// Send invokes procedure as fire-and-forget — no response awaited.
|
||||
@@ -226,7 +256,11 @@ func (c *Client) Send(ctx context.Context, procedure string, req *zap.Message) e
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
return c.node.Send(ctx, peer.NodeID, withFlags(req, op))
|
||||
target, err := c.resolvePeer(peer)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
return c.node.Send(ctx, target, withFlags(req, op))
|
||||
}
|
||||
|
||||
// Broadcast sends procedure to every current peer. Returns a map of
|
||||
@@ -239,6 +273,35 @@ func (c *Client) Broadcast(ctx context.Context, procedure string, req *zap.Messa
|
||||
return c.node.Broadcast(ctx, withFlags(req, op))
|
||||
}
|
||||
|
||||
// resolvePeer returns the NodeID to address a Call/Send at. A peer that
|
||||
// carries an Address (static discovery, or mDNS) is dialed directly once
|
||||
// via ConnectDirectID and its handshake-learned NodeID is memoized — this
|
||||
// is what makes static peers (placeholder NodeID + real Address)
|
||||
// reachable. A peer without an Address falls back to its advertised
|
||||
// NodeID (the node's own mDNS auto-connect path).
|
||||
func (c *Client) resolvePeer(peer Peer) (string, error) {
|
||||
if peer.Address == "" {
|
||||
return peer.NodeID, nil
|
||||
}
|
||||
c.dialMu.Lock()
|
||||
if id, ok := c.addrID[peer.Address]; ok {
|
||||
c.dialMu.Unlock()
|
||||
return id, nil
|
||||
}
|
||||
c.dialMu.Unlock()
|
||||
id, err := c.node.ConnectDirectID(peer.Address)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("zapclient: dial %s: %w", peer.Address, err)
|
||||
}
|
||||
if id == "" { // QUIC path returns no id — fall back to advertised
|
||||
id = peer.NodeID
|
||||
}
|
||||
c.dialMu.Lock()
|
||||
c.addrID[peer.Address] = id
|
||||
c.dialMu.Unlock()
|
||||
return id, nil
|
||||
}
|
||||
|
||||
// Peers returns the current peer snapshot.
|
||||
func (c *Client) Peers() []Peer { return c.disc.Peers() }
|
||||
|
||||
|
||||
@@ -0,0 +1,91 @@
|
||||
package zapclient
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"net"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
zap "github.com/luxfi/zap"
|
||||
)
|
||||
|
||||
func freePort(t *testing.T) int {
|
||||
t.Helper()
|
||||
l, err := net.Listen("tcp", "127.0.0.1:0")
|
||||
if err != nil {
|
||||
t.Fatalf("freePort: %v", err)
|
||||
}
|
||||
defer l.Close()
|
||||
return l.Addr().(*net.TCPAddr).Port
|
||||
}
|
||||
|
||||
func textMsg(field int, s string) *zap.Message {
|
||||
b := zap.NewBuilder(128)
|
||||
ob := b.StartObject(64)
|
||||
ob.SetText(field, s)
|
||||
ob.FinishAsRoot()
|
||||
m, _ := zap.Parse(b.Finish())
|
||||
return m
|
||||
}
|
||||
|
||||
// TestNativeTCPRoundTrip is the regression guard that was missing: a real
|
||||
// zapclient.Client → zapclient.Server call over native TCP using static
|
||||
// peers (no mDNS). It exercises BOTH fixes shipped in v0.8.6:
|
||||
//
|
||||
// - server opcode→handler routing: Register binds the node handler
|
||||
// under op>>8 to match the inbound Flags()>>8 lookup (previously the
|
||||
// full op was used, so the handler was never found and the call hung);
|
||||
// - client static-peer dial: WithStaticPeers + dial-by-address via
|
||||
// ConnectDirectID (previously a static-discovery node was NoDiscovery
|
||||
// and could not dial the peer at all).
|
||||
func TestNativeTCPRoundTrip(t *testing.T) {
|
||||
port := freePort(t)
|
||||
srv, err := NewServer("_lux-test._tcp", WithServerPort(port), WithNoDiscovery())
|
||||
if err != nil {
|
||||
t.Fatalf("NewServer: %v", err)
|
||||
}
|
||||
if err := srv.Register("test.echo", func(ctx context.Context, peer PeerInfo, req *zap.Message) (*zap.Message, error) {
|
||||
in := req.Root().Text(0)
|
||||
b := zap.NewBuilder(128)
|
||||
ob := b.StartObject(64)
|
||||
ob.SetText(0, "echo:"+in)
|
||||
ob.FinishAsRoot()
|
||||
out, _ := zap.Parse(b.Finish())
|
||||
return out, nil
|
||||
}); err != nil {
|
||||
t.Fatalf("Register: %v", err)
|
||||
}
|
||||
if err := srv.Start(); err != nil {
|
||||
t.Fatalf("Start: %v", err)
|
||||
}
|
||||
defer srv.Stop()
|
||||
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||
defer cancel()
|
||||
c, err := Connect(ctx, "_lux-test._tcp",
|
||||
WithStaticPeers(fmt.Sprintf("127.0.0.1:%d", port)),
|
||||
WithCallTimeout(3*time.Second),
|
||||
)
|
||||
if err != nil {
|
||||
t.Fatalf("Connect: %v", err)
|
||||
}
|
||||
defer c.Close()
|
||||
|
||||
resp, err := c.Call(ctx, "test.echo", textMsg(0, "hi"))
|
||||
if err != nil {
|
||||
t.Fatalf("Call over native TCP: %v", err)
|
||||
}
|
||||
if got := resp.Root().Text(0); got != "echo:hi" {
|
||||
t.Fatalf("echo: got %q want %q", got, "echo:hi")
|
||||
}
|
||||
|
||||
// Second call reuses the memoized address→NodeID mapping.
|
||||
resp2, err := c.Call(ctx, "test.echo", textMsg(0, "again"))
|
||||
if err != nil {
|
||||
t.Fatalf("second Call: %v", err)
|
||||
}
|
||||
if got := resp2.Root().Text(0); got != "echo:again" {
|
||||
t.Fatalf("echo2: got %q want %q", got, "echo:again")
|
||||
}
|
||||
}
|
||||
+7
-1
@@ -151,7 +151,13 @@ func (s *Server) Register(procedure string, h ProcedureHandler) error {
|
||||
}
|
||||
s.procedures[op] = registration{procedure: procedure, handler: h}
|
||||
s.procNames[procedure] = op
|
||||
s.node.Handle(op, s.dispatch)
|
||||
// The Node routes inbound messages by msgType == Flags()>>8 (the high
|
||||
// byte; see Node.acceptLoop). Procedure opcodes live in that high byte
|
||||
// (ProcedureOpcode returns code<<8, low byte zero), so register the
|
||||
// node handler under op>>8 — registering under the full op would never
|
||||
// match the inbound lookup and the wire call would hang. s.dispatch
|
||||
// then re-reads the full Flags() to select the procedure.
|
||||
s.node.Handle(op>>8, s.dispatch)
|
||||
return nil
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,58 @@
|
||||
// static.go — a fixed peer-address Discovery, bypassing mDNS.
|
||||
//
|
||||
// This is what WithStaticPeers builds: the cloud/K8s path where peers
|
||||
// are reached by a known address (a Service DNS name). The NodeID is a
|
||||
// placeholder for observability — the real peer identity is learned at
|
||||
// dial time (Node.ConnectDirectID) and authenticated by the mTLS peer
|
||||
// cert, so the client addresses Calls by the learned id, not this one.
|
||||
|
||||
package zapclient
|
||||
|
||||
import (
|
||||
"strconv"
|
||||
"sync"
|
||||
"time"
|
||||
)
|
||||
|
||||
type staticDiscovery struct {
|
||||
serviceType string
|
||||
mu sync.RWMutex
|
||||
peers []Peer
|
||||
}
|
||||
|
||||
func newStaticDiscovery(serviceType string, addrs []string) *staticDiscovery {
|
||||
now := time.Now()
|
||||
peers := make([]Peer, 0, len(addrs))
|
||||
for i, addr := range addrs {
|
||||
if addr == "" {
|
||||
continue
|
||||
}
|
||||
peers = append(peers, Peer{
|
||||
NodeID: serviceType + "-static-" + strconv.Itoa(i),
|
||||
ServiceType: serviceType,
|
||||
Address: addr,
|
||||
LastSeen: now,
|
||||
})
|
||||
}
|
||||
return &staticDiscovery{serviceType: serviceType, peers: peers}
|
||||
}
|
||||
|
||||
func (s *staticDiscovery) Peers() []Peer {
|
||||
s.mu.RLock()
|
||||
defer s.mu.RUnlock()
|
||||
out := make([]Peer, len(s.peers))
|
||||
copy(out, s.peers)
|
||||
return out
|
||||
}
|
||||
|
||||
func (s *staticDiscovery) PeerCount() int {
|
||||
s.mu.RLock()
|
||||
defer s.mu.RUnlock()
|
||||
return len(s.peers)
|
||||
}
|
||||
|
||||
func (s *staticDiscovery) ServiceType() string { return s.serviceType }
|
||||
func (s *staticDiscovery) Start() error { return nil }
|
||||
func (s *staticDiscovery) Stop() {}
|
||||
|
||||
var _ Discovery = (*staticDiscovery)(nil)
|
||||
Reference in New Issue
Block a user