test(handler/router): fix review findings - error matching, version test, cache assertion, entrypoint sync

handler/router/associate_test.go

@@ -3,6 +3,7 @@ package router
 import (
 	"bytes"
 	"context"
+	"io"
 	"net"
 	"testing"
 	"time"
@@ -27,9 +28,12 @@ func TestHandleAssociate_NoIngress(t *testing.T) {
 
 	conn := &fakeConn{buf: reqData}
 	err := h.handleAssociate(context.Background(), conn, "ip", "10.0.0.1", rid, &testLogger{})
-	// The error comes from the packet read loop when the fake conn has no
-	// more data — it is not an associate failure.
-	if err != nil && err.Error() != "unexpected EOF" {
+	// handleAssociate returns io.EOF when the packet read loop exhausts the
+	// fake connection's data — that signals normal completion, not a failure.
+	// packetConn.Read uses io.ReadFull, which returns io.ErrUnexpectedEOF
+	// when the underlying connection has been exhausted mid-frame, rather
+	// than the plain io.EOF that the loop checks for.
+	if err != nil && err != io.EOF && err != io.ErrUnexpectedEOF {
 		t.Fatalf("handleAssociate: %v", err)
 	}
 
@@ -58,9 +62,7 @@ func TestHandleAssociate_IngressMatch(t *testing.T) {
 	reqData := buildRelayAssociateRequest(t, "10.0.0.1:0", rid, "ip")
 	conn := &fakeConn{buf: reqData}
 	err := h.handleAssociate(context.Background(), conn, "ip", "10.0.0.1", rid, &testLogger{})
-	// The error comes from the packet read loop when the fake conn has no
-	// more data — it is not an associate failure.
-	if err != nil && err.Error() != "unexpected EOF" {
+	if err != nil && err != io.EOF && err != io.ErrUnexpectedEOF {
 		t.Fatalf("handleAssociate: %v", err)
 	}
 
@@ -249,10 +251,19 @@ func TestGetRoute_CacheDisabled(t *testing.T) {
 	h := newInitdHandler(t)
 	h.md.routerCacheEnabled = false
 
-	// With no registry router and no fallback, should return nil.
+	// With cache disabled, getRoute always performs a lookup.
+	// Since cache is disabled, a cached value should NOT be returned even if one exists.
+	h.routeCache.Set("10.0.0.1", cache.NewItem(&router.Route{
+		Dst:     "10.0.0.0/24",
+		Gateway: "10.0.0.254",
+	}, time.Minute))
+
 	route := h.getRoute(context.Background(), "test-rid", "10.0.0.1")
+	// With cache disabled, the cached route must NOT be returned.
+	// Since there is no registry router registered for "test-rid" and no
+	// fallback router, the result should be nil.
 	if route != nil {
-		t.Log("route is not nil (may have registry router)")
+		t.Errorf("getRoute returned %+v, want nil (cache disabled, no fallback)", route)
 	}
 }
 

handler/router/entrypoint_test.go

@@ -72,9 +72,10 @@ func TestHandleEntrypoint_NonAssociateCmd(t *testing.T) {
 	h.epConn = fpc
 	h.pool.Add(rid, c)
 
-	errCh := make(chan error, 1)
+	doneCh := make(chan struct{})
 	go func() {
-		errCh <- h.handleEntrypoint(&testLogger{})
+		h.handleEntrypoint(&testLogger{})
+		close(doneCh)
 	}()
 
 	// Send request with non-Associate cmd (CmdConnect)
@@ -93,7 +94,7 @@ func TestHandleEntrypoint_NonAssociateCmd(t *testing.T) {
 
 	// Close to stop the loop, then wait for goroutine to exit.
 	fpc.Close()
-	<-errCh
+	<-doneCh
 
 	if connBuf.Len() > 0 {
 		t.Error("data was forwarded to connector despite non-Associate cmd")
@@ -110,9 +111,10 @@ func TestHandleEntrypoint_NoMatchingConnector(t *testing.T) {
 	h.epConn = fpc
 	// No connector added to pool
 
-	errCh := make(chan error, 1)
+	doneCh := make(chan struct{})
 	go func() {
-		errCh <- h.handleEntrypoint(&testLogger{})
+		h.handleEntrypoint(&testLogger{})
+		close(doneCh)
 	}()
 
 	req := relay.Request{
@@ -132,9 +134,9 @@ func TestHandleEntrypoint_NoMatchingConnector(t *testing.T) {
 	fpc.dataCh <- buf.Bytes()
 	fpc.addrCh <- laddr
 
-	// Should not panic or error, just silently drop.
+	// Signal the loop to stop by sending EOF via the pipe concurrency pattern.
 	fpc.Close()
-	<-errCh
+	<-doneCh
 }
 
 func TestHandleEntrypoint_ReadError(t *testing.T) {

handler/router/handler_test.go

@@ -126,8 +126,11 @@ func TestInit_WithLimiter(t *testing.T) {
 func TestHandle_BadVersion(t *testing.T) {
 	h := newInitdHandler(t)
 
+	// Version2 causes relay.Request.ReadFrom to return an error because the
+	// relay protocol only supports Version1. The Handle method returns the
+	// read error directly — it never reaches the version check.
 	req := relay.Request{
-		Version: 0x02, // not Version1 — triggers error in ReadFrom
+		Version: 0x02,
 		Cmd:     relay.CmdAssociate,
 	}
 	var buf bytes.Buffer
@@ -139,8 +142,47 @@ func TestHandle_BadVersion(t *testing.T) {
 		t.Fatal("expected error for bad version")
 	}
 
-	// The response is not written when ReadFrom fails, so we only verify
-	// that the connection was closed (defer conn.Close in Handle).
+	// Connection must be closed via defer.
+	if !conn.closed {
+		t.Error("connection was not closed")
+	}
+}
+
+// TestHandle_BadRequestVersion verifies that Handle rejects requests where
+// the version byte has been tampered with. relay.Request.ReadFrom checks the
+// version byte before the handler does — the handler's own version check
+// (req.Version != relay.Version1) is a defensive secondary guard. Since
+// ReadFrom catches bad versions first, Handle returns relay.ErrBadVersion
+// from ReadFrom without writing a response.
+func TestHandle_BadRequestVersion(t *testing.T) {
+	h := newInitdHandler(t)
+
+	// Build a valid relay request with Version1, then override the version byte.
+	rid := relay.NewTunnelID([]byte("0123456789abcdef"))
+	req := relay.Request{
+		Version: relay.Version1,
+		Cmd:     relay.CmdAssociate,
+		Features: []relay.Feature{
+			&relay.TunnelFeature{ID: rid},
+			&relay.NetworkFeature{Network: relay.NetworkIP},
+		},
+	}
+	var buf bytes.Buffer
+	req.WriteTo(&buf)
+
+	// Override the version byte to something that is not Version1.
+	b := buf.Bytes()
+	if len(b) > 0 {
+		b[0] = 0x02
+	}
+
+	conn := &fakeConn{buf: b}
+	err := h.Handle(context.Background(), conn)
+	if err == nil {
+		t.Fatal("expected error for bad request version")
+	}
+
+	// Connection must be closed via defer.
 	if !conn.closed {
 		t.Error("connection was not closed")
 	}