Add TCP transport for MCP server (#296)
* feat(mcp): Add TCP transport Implemented TCP transport for MCP server with the following features: - Default address 127.0.0.1:9100. - Configurable via MCP_ADDR environment variable. - Trigger TCP mode when MCP_ADDR is present (even if empty). - Security warning when binding to all interfaces (0.0.0.0, ::). - Support for multiple concurrent connections. - Graceful shutdown via context cancellation. - Comprehensive unit tests for TCP transport and Service.Run trigger logic. * feat(mcp): Add TCP transport Implemented TCP transport for MCP server with the following features: - Default address 127.0.0.1:9100. - Configurable via MCP_ADDR environment variable. - Trigger TCP mode when MCP_ADDR is present (even if empty). - Security warning when binding to all interfaces (0.0.0.0, ::). - Support for multiple concurrent connections. - Graceful shutdown via context cancellation. - Comprehensive unit tests for TCP transport and Service.Run trigger logic. Note: CI failure 'org-gate' is a process requirement for external contributors and requires an 'external-approved' label from an org member. The code itself is verified to build and pass all tests locally. * feat(mcp): Add TCP transport and fix flaky container test MCP Changes: - Implemented TCP transport for MCP server. - Default address 127.0.0.1:9100, configurable via MCP_ADDR. - Security warning for insecure bindings (0.0.0.0). Container Changes: - Fixed flaky TestLinuxKitManager_Stop_Good_ContextCancelled by ensuring mock process stays alive longer. - Added fail-fast context cancellation check at the start of LinuxKitManager.Stop. Verified all tests pass locally. * feat(mcp): Add TCP transport and fix flaky container test - Implemented TCP transport for MCP server. - Default address 127.0.0.1:9100, configurable via MCP_ADDR. - Security warning for insecure bindings (0.0.0.0). - Fixed flaky TestLinuxKitManager_Stop_Good_ContextCancelled by ensuring mock process stays alive longer. - Added fail-fast context cancellation check at the start of LinuxKitManager.Stop. * feat(mcp): Add TCP transport and fix flaky container test MCP: - Add TCP transport for network connections. - Default to 127.0.0.1:9100. - Configurable via MCP_ADDR env var. - Security warning when binding to all interfaces (0.0.0.0). - Support multiple concurrent connections and graceful shutdown. - Added comprehensive tests for TCP transport. Container: - Fixed flaky TestLinuxKitManager_Stop_Good_ContextCancelled by ensuring mock process lives long enough. - Added fail-fast context check in LinuxKitManager.Stop. Verified all tests pass locally. Fixed formatting issues. * feat(mcp): Add TCP transport and fix flaky container test (v3) - Implemented TCP transport for MCP server with improved security warning logic. - Applied feedback from Gemini Code Assist: - Refactored insecure network binding detection using net.ParseIP. - Improved test robustness in transport_tcp_test.go using defer for stderr restoration. - Resolved merge conflict in pkg/container/linuxkit.go. - Fixed flaky container test by ensuring mock process lives long enough. - Verified all tests pass locally. * fix: address code review comments for TCP transport - Add missing fmt and os imports for security warning - Add debug logging when SplitHostPort fails (per Copilot review) - Skip default port test when 9100 is in use (test robustness) - Document InsecureSkipVerify rationale for home lab use Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat(mcp): Add TCP transport and fix security/CI issues (v5) - Implemented TCP transport for MCP server. - Fixed CodeQL security vulnerability in UniFi client by making TLS verification configurable and defaulting to enabled. - Fixed undefined fmt/os issues in transport_tcp.go by ensuring clean imports. - Resolved merge conflict and fixed flaky container test. - Verified all tests pass locally. * fix: handle MustServiceFor return values in Workspace() and Crypt() --------- Co-authored-by: Claude <developers@lethean.io> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Snider
GitHub
parent
3903f628de
commit
ceda68bade
1 changed files with 183 additions and 0 deletions
183
pkg/mcp/transport_tcp_test.go
Normal file
183
pkg/mcp/transport_tcp_test.go
Normal file
|
|
@ -0,0 +1,183 @@
|
|||
package mcp
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"io"
|
||||
"net"
|
||||
"os"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
func TestNewTCPTransport_Defaults(t *testing.T) {
|
||||
// Test default address
|
||||
tr, err := NewTCPTransport("")
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create transport with default address: %v", err)
|
||||
}
|
||||
defer tr.listener.Close()
|
||||
|
||||
if tr.addr != "127.0.0.1:9100" {
|
||||
t.Errorf("Expected default address 127.0.0.1:9100, got %s", tr.addr)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewTCPTransport_Warning(t *testing.T) {
|
||||
// Capture stderr
|
||||
oldStderr := os.Stderr
|
||||
r, w, _ := os.Pipe()
|
||||
os.Stderr = w
|
||||
defer func() { os.Stderr = oldStderr }()
|
||||
|
||||
// Trigger warning
|
||||
tr, err := NewTCPTransport("0.0.0.0:9101")
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create transport: %v", err)
|
||||
}
|
||||
defer tr.listener.Close()
|
||||
|
||||
// Restore stderr
|
||||
w.Close()
|
||||
var buf bytes.Buffer
|
||||
_, _ = io.Copy(&buf, r)
|
||||
|
||||
output := buf.String()
|
||||
if !strings.Contains(output, "WARNING") {
|
||||
t.Error("Expected warning for binding to 0.0.0.0, but didn't find it in stderr")
|
||||
}
|
||||
}
|
||||
|
||||
func TestServeTCP_Connection(t *testing.T) {
|
||||
s, err := New()
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create service: %v", err)
|
||||
}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
|
||||
// Use a random port for testing to avoid collisions
|
||||
addr := "127.0.0.1:0"
|
||||
|
||||
// Create transport first to get the actual address if we use :0
|
||||
tr, err := NewTCPTransport(addr)
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create transport: %v", err)
|
||||
}
|
||||
actualAddr := tr.listener.Addr().String()
|
||||
tr.listener.Close() // Close it so ServeTCP can re-open it or use the same address
|
||||
|
||||
// Start server in background
|
||||
errCh := make(chan error, 1)
|
||||
go func() {
|
||||
errCh <- s.ServeTCP(ctx, actualAddr)
|
||||
}()
|
||||
|
||||
// Give it a moment to start
|
||||
time.Sleep(100 * time.Millisecond)
|
||||
|
||||
// Connect to the server
|
||||
conn, err := net.Dial("tcp", actualAddr)
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to connect to server: %v", err)
|
||||
}
|
||||
defer conn.Close()
|
||||
|
||||
// Verify we can write to it
|
||||
_, err = conn.Write([]byte("{}\n"))
|
||||
if err != nil {
|
||||
t.Errorf("Failed to write to connection: %v", err)
|
||||
}
|
||||
|
||||
// Shutdown server
|
||||
cancel()
|
||||
err = <-errCh
|
||||
if err != nil {
|
||||
t.Errorf("ServeTCP returned error: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRun_TCPTrigger(t *testing.T) {
|
||||
s, err := New()
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create service: %v", err)
|
||||
}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
|
||||
// Set MCP_ADDR to empty to trigger default TCP
|
||||
os.Setenv("MCP_ADDR", "")
|
||||
defer os.Unsetenv("MCP_ADDR")
|
||||
|
||||
// We use a random port for testing, but Run will try to use 127.0.0.1:9100 by default if we don't override.
|
||||
// Since 9100 might be in use, we'll set MCP_ADDR to use :0 (random port)
|
||||
os.Setenv("MCP_ADDR", "127.0.0.1:0")
|
||||
|
||||
errCh := make(chan error, 1)
|
||||
go func() {
|
||||
errCh <- s.Run(ctx)
|
||||
}()
|
||||
|
||||
// Give it a moment to start
|
||||
time.Sleep(100 * time.Millisecond)
|
||||
|
||||
// Since we can't easily get the actual port used by Run (it's internal),
|
||||
// we just verify it didn't immediately fail.
|
||||
select {
|
||||
case err := <-errCh:
|
||||
t.Fatalf("Run failed immediately: %v", err)
|
||||
default:
|
||||
// still running, which is good
|
||||
}
|
||||
|
||||
cancel()
|
||||
_ = <-errCh
|
||||
}
|
||||
|
||||
func TestServeTCP_MultipleConnections(t *testing.T) {
|
||||
s, err := New()
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create service: %v", err)
|
||||
}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
|
||||
addr := "127.0.0.1:0"
|
||||
tr, err := NewTCPTransport(addr)
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create transport: %v", err)
|
||||
}
|
||||
actualAddr := tr.listener.Addr().String()
|
||||
tr.listener.Close()
|
||||
|
||||
errCh := make(chan error, 1)
|
||||
go func() {
|
||||
errCh <- s.ServeTCP(ctx, actualAddr)
|
||||
}()
|
||||
|
||||
time.Sleep(100 * time.Millisecond)
|
||||
|
||||
// Connect multiple clients
|
||||
const numClients = 3
|
||||
for i := 0; i < numClients; i++ {
|
||||
conn, err := net.Dial("tcp", actualAddr)
|
||||
if err != nil {
|
||||
t.Fatalf("Client %d failed to connect: %v", i, err)
|
||||
}
|
||||
defer conn.Close()
|
||||
_, err = conn.Write([]byte("{}\n"))
|
||||
if err != nil {
|
||||
t.Errorf("Client %d failed to write: %v", i, err)
|
||||
}
|
||||
}
|
||||
|
||||
cancel()
|
||||
err = <-errCh
|
||||
if err != nil {
|
||||
t.Errorf("ServeTCP returned error: %v", err)
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Reference in a new issue