From 9a6c81620a28b1727e7fa210c37ba7f768dd08cf Mon Sep 17 00:00:00 2001 From: Alon Girmonsky <1990761+alongir@users.noreply.github.com> Date: Thu, 5 Feb 2026 17:41:01 -0800 Subject: [PATCH] Address code review comments for MCP implementation - Add 30s timeout to HTTP client to prevent hanging requests - Add scanner.Err() check after stdin processing loop - Close HTTP response bodies to prevent resource leaks - Add goroutine to wait on started process to prevent zombies - Simplify polling loop by removing ineffective context check - Advertise check_kubeshark_status in URL mode (was callable but hidden) - Update documentation to clarify URL mode only disables start/stop --- cmd/mcp.go | 6 ++--- cmd/mcpRunner.go | 59 ++++++++++++++++++++++++++++-------------------- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/cmd/mcp.go b/cmd/mcp.go index 9be9511a8..1a6d0c936 100644 --- a/cmd/mcp.go +++ b/cmd/mcp.go @@ -58,8 +58,8 @@ you can connect directly without needing kubectl/kubeconfig: } } -In URL mode, cluster management tools (start/stop/check) are disabled since -Kubeshark is managed externally. +In URL mode, destructive tools (start/stop) are disabled since Kubeshark is +managed externally. The check_kubeshark_status tool remains available to confirm connectivity. DESTRUCTIVE OPERATIONS: @@ -117,7 +117,7 @@ Multiple --set flags can be used for different settings.`, func init() { rootCmd.AddCommand(mcpCmd) - mcpCmd.Flags().StringVar(&mcpURL, "url", "", "Direct URL to Kubeshark (e.g., https://kubeshark.example.com). When set, connects directly without kubectl/proxy and disables start/stop/check tools.") + mcpCmd.Flags().StringVar(&mcpURL, "url", "", "Direct URL to Kubeshark (e.g., https://kubeshark.example.com). When set, connects directly without kubectl/proxy and disables start/stop tools.") mcpCmd.Flags().StringVar(&mcpKubeconfig, "kubeconfig", "", "Path to kubeconfig file (e.g., /Users/me/.kube/config)") mcpCmd.Flags().BoolVar(&mcpListTools, "list-tools", false, "List available MCP tools and exit") mcpCmd.Flags().BoolVar(&mcpConfig, "mcp-config", false, "Print MCP client configuration JSON and exit") diff --git a/cmd/mcpRunner.go b/cmd/mcpRunner.go index 68e765103..a32105754 100644 --- a/cmd/mcpRunner.go +++ b/cmd/mcpRunner.go @@ -166,7 +166,7 @@ func runMCPWithConfig(setFlags []string, directURL string, allowDestructive bool zerolog.SetGlobalLevel(zerolog.Disabled) server := &mcpServer{ - httpClient: &http.Client{}, + httpClient: &http.Client{Timeout: 30 * time.Second}, stdin: os.Stdin, stdout: os.Stdout, setFlags: setFlags, @@ -250,9 +250,12 @@ func (s *mcpServer) ensureBackendConnection() string { return "Kubeshark is not running. Use the 'start_kubeshark' tool to start it first." } - // Start proxy to frontend + // Start proxy to frontend and verify connectivity frontURL := kubernetes.GetProxyOnPort(config.Config.Tap.Proxy.Front.Port) response, err := http.Get(fmt.Sprintf("%s/", frontURL)) + if response != nil && response.Body != nil { + defer func() { _ = response.Body.Close() }() + } if err != nil || response.StatusCode != 200 { startProxyReportErrorIfAny( kubernetesProvider, @@ -345,6 +348,11 @@ func (s *mcpServer) run() { s.handleRequest(&req) } + + // Check for scanner errors (e.g., stdin closed, read errors) + if err := scanner.Err(); err != nil { + writeErrorToStderr("[kubeshark-mcp] Scanner error: %v", err) + } } func (s *mcpServer) handleRequest(req *jsonRPCRequest) { @@ -433,22 +441,20 @@ Use the MCP tools - do NOT use kubectl, helm, or curl directly.` func (s *mcpServer) handleListTools(req *jsonRPCRequest) { var tools []mcpTool - // Add check_kubeshark_status if not in URL mode (safe, read-only) - if !s.urlMode { - tools = append(tools, mcpTool{ - Name: "check_kubeshark_status", - Description: "Safe: Checks if Kubeshark is currently running in the cluster. This is a read-only operation that does not modify anything.", - InputSchema: json.RawMessage(`{ - "type": "object", - "properties": { - "release_namespace": { - "type": "string", - "description": "Namespace where Kubeshark is installed (default: 'default')" - } + // Add check_kubeshark_status - safe, read-only operation that works in both modes + tools = append(tools, mcpTool{ + Name: "check_kubeshark_status", + Description: "Safe: Checks if Kubeshark is currently running and accessible. In URL mode, confirms connectivity to the remote instance. In local mode, checks cluster pods. This is a read-only operation.", + InputSchema: json.RawMessage(`{ + "type": "object", + "properties": { + "release_namespace": { + "type": "string", + "description": "Namespace where Kubeshark is installed (default: 'default'). Only used in local mode." } - }`), - }) - } + } + }`), + }) // Add destructive tools only if --allow-destructive flag was set (and not in URL mode) if !s.urlMode && s.allowDestructive { @@ -756,6 +762,11 @@ func (s *mcpServer) callStartKubeshark(args map[string]any) (string, bool) { return fmt.Sprintf("Failed to start Kubeshark: %v", err), true } + // Wait for the process in a goroutine to prevent zombie processes + go func() { + _ = cmd.Wait() + }() + logProgress("Kubeshark process started, waiting for pods to be ready...") // Wait for Kubeshark to be ready (poll for pods) @@ -786,13 +797,8 @@ func (s *mcpServer) callStartKubeshark(args map[string]any) (string, bool) { if ready { break } - select { - case <-context.Background().Done(): - return "Kubeshark start interrupted", true - default: - // Sleep 5 seconds before next check - <-time.After(5 * time.Second) - } + // Sleep 5 seconds before next check + time.Sleep(5 * time.Second) } if !ready { @@ -959,9 +965,12 @@ func establishProxyConnection(timeout time.Duration) (string, error) { return "", fmt.Errorf("not running (use start_kubeshark to start)") } - // Start proxy to frontend + // Start proxy to frontend and verify connectivity frontURL := kubernetes.GetProxyOnPort(config.Config.Tap.Proxy.Front.Port) response, err := http.Get(fmt.Sprintf("%s/", frontURL)) + if response != nil && response.Body != nil { + defer func() { _ = response.Body.Close() }() + } if err != nil || response.StatusCode != 200 { startProxyReportErrorIfAny( kubernetesProvider,