From a83506cef1a54deadd3523e997049d4d08037350 Mon Sep 17 00:00:00 2001 From: Marc Nuri Date: Thu, 23 Oct 2025 12:26:30 +0200 Subject: [PATCH 1/2] fix(nodes): reviewed kubernetes.nodes implementation - Changed node_log tool name to nodes_log for consistency - Added access control check for nodes and configured denied resources - Migrated tests to testify - Added complete test coverage for nodes_log https://kubernetes.io/docs/concepts/cluster-administration/system-logs/#log-query Signed-off-by: Marc Nuri --- pkg/kubernetes/accesscontrol_clientset.go | 16 ++ pkg/kubernetes/nodes.go | 22 +- pkg/mcp/events_test.go | 1 + pkg/mcp/nodes_test.go | 214 ++++++++++++++---- pkg/mcp/testdata/toolsets-core-tools.json | 2 +- ...toolsets-full-tools-multicluster-enum.json | 2 +- .../toolsets-full-tools-multicluster.json | 2 +- .../toolsets-full-tools-openshift.json | 2 +- pkg/mcp/testdata/toolsets-full-tools.json | 2 +- pkg/toolsets/core/nodes.go | 17 +- 10 files changed, 204 insertions(+), 76 deletions(-) diff --git a/pkg/kubernetes/accesscontrol_clientset.go b/pkg/kubernetes/accesscontrol_clientset.go index ed875c64..0ce64c49 100644 --- a/pkg/kubernetes/accesscontrol_clientset.go +++ b/pkg/kubernetes/accesscontrol_clientset.go @@ -39,6 +39,22 @@ func (a *AccessControlClientset) DiscoveryClient() discovery.DiscoveryInterface return a.discoveryClient } +func (a *AccessControlClientset) NodesLogs(ctx context.Context, name, logPath string) (*rest.Request, error) { + gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Node"} + if !isAllowed(a.staticConfig, gvk) { + return nil, isNotAllowedError(gvk) + } + + if _, err := a.delegate.CoreV1().Nodes().Get(ctx, name, metav1.GetOptions{}); err != nil { + return nil, fmt.Errorf("failed to get node %s: %w", name, err) + } + + url := []string{"api", "v1", "nodes", name, "proxy", "logs", logPath} + return a.delegate.CoreV1().RESTClient(). + Get(). + AbsPath(url...), nil +} + func (a *AccessControlClientset) Pods(namespace string) (corev1.PodInterface, error) { gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"} if !isAllowed(a.staticConfig, gvk) { diff --git a/pkg/kubernetes/nodes.go b/pkg/kubernetes/nodes.go index c6bb58fe..76d9cc92 100644 --- a/pkg/kubernetes/nodes.go +++ b/pkg/kubernetes/nodes.go @@ -5,33 +5,21 @@ import ( "fmt" ) -func (k *Kubernetes) NodeLog(ctx context.Context, name string, logPath string, tail int64) (string, error) { +func (k *Kubernetes) NodesLog(ctx context.Context, name string, logPath string, tail int64) (string, error) { // Use the node proxy API to access logs from the kubelet // Common log paths: // - /var/log/kubelet.log - kubelet logs // - /var/log/kube-proxy.log - kube-proxy logs // - /var/log/containers/ - container logs - if logPath == "" { - logPath = "kubelet.log" + req, err := k.AccessControlClientset().NodesLogs(ctx, name, logPath) + if err != nil { + return "", err } - // Build the URL for the node proxy logs endpoint - url := []string{"api", "v1", "nodes", name, "proxy", "logs", logPath} - // Query parameters for tail - params := make(map[string]string) if tail > 0 { - params["tailLines"] = fmt.Sprintf("%d", tail) - } - - req := k.manager.discoveryClient.RESTClient(). - Get(). - AbsPath(url...) - - // Add tail parameter if specified - for key, value := range params { - req.Param(key, value) + req.Param("tailLines", fmt.Sprintf("%d", tail)) } result := req.Do(ctx) diff --git a/pkg/mcp/events_test.go b/pkg/mcp/events_test.go index 6d771bca..68ca85a8 100644 --- a/pkg/mcp/events_test.go +++ b/pkg/mcp/events_test.go @@ -126,6 +126,7 @@ func (s *EventsSuite) TestEventsListDenied() { s.InitMcpClient() s.Run("events_list (denied)", func() { toolResult, err := s.CallTool("events_list", map[string]interface{}{}) + s.Require().NotNil(toolResult, "toolResult should not be nil") s.Run("has error", func() { s.Truef(toolResult.IsError, "call tool should fail") s.Nilf(err, "call tool should not return error object") diff --git a/pkg/mcp/nodes_test.go b/pkg/mcp/nodes_test.go index b9915778..12f59fb0 100644 --- a/pkg/mcp/nodes_test.go +++ b/pkg/mcp/nodes_test.go @@ -1,66 +1,188 @@ package mcp import ( - "strings" + "net/http" "testing" + "github.com/BurntSushi/toml" + "github.com/containers/kubernetes-mcp-server/internal/test" "github.com/mark3labs/mcp-go/mcp" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/stretchr/testify/suite" ) -func TestNodeLog(t *testing.T) { - testCase(t, func(c *mcpContext) { - c.withEnvTest() - - // Create test node - kubernetesAdmin := c.newKubernetesClient() - node := &corev1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-node-log", - }, - Status: corev1.NodeStatus{ - Addresses: []corev1.NodeAddress{ - {Type: corev1.NodeInternalIP, Address: "192.168.1.10"}, - }, - }, - } +type NodesSuite struct { + BaseMcpSuite + mockServer *test.MockServer +} - _, _ = kubernetesAdmin.CoreV1().Nodes().Create(c.ctx, node, metav1.CreateOptions{}) +func (s *NodesSuite) SetupTest() { + s.BaseMcpSuite.SetupTest() + s.mockServer = test.NewMockServer() + s.Cfg.KubeConfig = s.mockServer.KubeconfigFile(s.T()) +} - // Test node_log tool - toolResult, err := c.callTool("node_log", map[string]interface{}{ - "name": "test-node-log", - }) +func (s *NodesSuite) TearDownTest() { + s.BaseMcpSuite.TearDownTest() + if s.mockServer != nil { + s.mockServer.Close() + } +} - t.Run("node_log returns successfully or with expected error", func(t *testing.T) { - if err != nil { - t.Fatalf("call tool failed: %v", err) - } - // Node logs might not be available in test environment - // We just check that the tool call completes - if toolResult.IsError { - content := toolResult.Content[0].(mcp.TextContent).Text - // Expected error messages in test environment - if !strings.Contains(content, "failed to get node logs") && - !strings.Contains(content, "not logged any message yet") { - t.Logf("tool returned error (expected in test environment): %v", content) +func (s *NodesSuite) TestNodesLog() { + s.mockServer.Handle(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + // Get Node response + if req.URL.Path == "/api/v1/nodes/existing-node" { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{ + "apiVersion": "v1", + "kind": "Node", + "metadata": { + "name": "existing-node" } + }`)) + return + } + // Get Empty Log response + if req.URL.Path == "/api/v1/nodes/existing-node/proxy/logs/empty.log" { + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(``)) + return + } + // Get Kubelet Log response + if req.URL.Path == "/api/v1/nodes/existing-node/proxy/logs/kubelet.log" { + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusOK) + logContent := "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\n" + if req.URL.Query().Get("tailLines") != "" { + logContent = "Line 4\nLine 5\n" } + _, _ = w.Write([]byte(logContent)) + return + } + w.WriteHeader(http.StatusNotFound) + })) + s.InitMcpClient() + s.Run("nodes_log(name=nil)", func() { + toolResult, err := s.CallTool("nodes_log", map[string]interface{}{}) + s.Require().NotNil(toolResult, "toolResult should not be nil") + s.Run("has error", func() { + s.Truef(toolResult.IsError, "call tool should fail") + s.Nilf(err, "call tool should not return error object") + }) + s.Run("describes missing name", func() { + expectedMessage := "failed to get node log, missing argument name" + s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text, + "expected descriptive error '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text) }) }) + s.Run("nodes_log(name=inexistent-node)", func() { + toolResult, err := s.CallTool("nodes_log", map[string]interface{}{ + "name": "inexistent-node", + }) + s.Require().NotNil(toolResult, "toolResult should not be nil") + s.Run("has error", func() { + s.Truef(toolResult.IsError, "call tool should fail") + s.Nilf(err, "call tool should not return error object") + }) + s.Run("describes missing node", func() { + expectedMessage := "failed to get node log for inexistent-node: failed to get node inexistent-node: the server could not find the requested resource (get nodes inexistent-node)" + s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text, + "expected descriptive error '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text) + }) + }) + s.Run("nodes_log(name=existing-node, log_path=missing.log)", func() { + toolResult, err := s.CallTool("nodes_log", map[string]interface{}{ + "name": "existing-node", + "log_path": "missing.log", + }) + s.Require().NotNil(toolResult, "toolResult should not be nil") + s.Run("has error", func() { + s.Truef(toolResult.IsError, "call tool should fail") + s.Nilf(err, "call tool should not return error object") + }) + s.Run("describes missing log file", func() { + expectedMessage := "failed to get node log for existing-node: failed to get node logs: the server could not find the requested resource" + s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text, + "expected descriptive error '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text) + }) + }) + s.Run("nodes_log(name=existing-node, log_path=empty.log)", func() { + toolResult, err := s.CallTool("nodes_log", map[string]interface{}{ + "name": "existing-node", + "log_path": "empty.log", + }) + s.Require().NotNil(toolResult, "toolResult should not be nil") + s.Run("no error", func() { + s.Falsef(toolResult.IsError, "call tool should succeed") + s.Nilf(err, "call tool should not return error object") + }) + s.Run("describes empty log", func() { + expectedMessage := "The node existing-node has not logged any message yet or the log file is empty" + s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text, + "expected descriptive message '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text) + }) + }) + s.Run("nodes_log(name=existing-node, log_path=kubelet.log)", func() { + toolResult, err := s.CallTool("nodes_log", map[string]interface{}{ + "name": "existing-node", + "log_path": "kubelet.log", + }) + s.Require().NotNil(toolResult, "toolResult should not be nil") + s.Run("no error", func() { + s.Falsef(toolResult.IsError, "call tool should succeed") + s.Nilf(err, "call tool should not return error object") + }) + s.Run("returns full log", func() { + expectedMessage := "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\n" + s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text, + "expected log content '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text) + }) + }) + for _, tailCase := range []interface{}{2, int64(2), float64(2)} { + s.Run("nodes_log(name=existing-node, log_path=kubelet.log, tail=2)", func() { + toolResult, err := s.CallTool("nodes_log", map[string]interface{}{ + "name": "existing-node", + "log_path": "kubelet.log", + "tail": tailCase, + }) + s.Require().NotNil(toolResult, "toolResult should not be nil") + s.Run("no error", func() { + s.Falsef(toolResult.IsError, "call tool should succeed") + s.Nilf(err, "call tool should not return error object") + }) + s.Run("returns tail log", func() { + expectedMessage := "Line 4\nLine 5\n" + s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text, + "expected log content '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text) + }) + }) + } } -func TestNodeLogMissingArguments(t *testing.T) { - testCase(t, func(c *mcpContext) { - c.withEnvTest() - - t.Run("node_log requires name", func(t *testing.T) { - toolResult, err := c.callTool("node_log", map[string]interface{}{}) - - if err == nil && !toolResult.IsError { - t.Fatal("expected error when name is missing") - } +func (s *NodesSuite) TestNodesLogDenied() { + s.Require().NoError(toml.Unmarshal([]byte(` + denied_resources = [ { version = "v1", kind = "Node" } ] + `), s.Cfg), "Expected to parse denied resources config") + s.InitMcpClient() + s.Run("nodes_log (denied)", func() { + toolResult, err := s.CallTool("nodes_log", map[string]interface{}{ + "name": "does-not-matter", + }) + s.Require().NotNil(toolResult, "toolResult should not be nil") + s.Run("has error", func() { + s.Truef(toolResult.IsError, "call tool should fail") + s.Nilf(err, "call tool should not return error object") + }) + s.Run("describes denial", func() { + expectedMessage := "failed to get node log for does-not-matter: resource not allowed: /v1, Kind=Node" + s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text, + "expected descriptive error '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text) }) }) } + +func TestNodes(t *testing.T) { + suite.Run(t, new(NodesSuite)) +} diff --git a/pkg/mcp/testdata/toolsets-core-tools.json b/pkg/mcp/testdata/toolsets-core-tools.json index 5039cf24..37345100 100644 --- a/pkg/mcp/testdata/toolsets-core-tools.json +++ b/pkg/mcp/testdata/toolsets-core-tools.json @@ -65,7 +65,7 @@ "name" ] }, - "name": "node_log" + "name": "nodes_log" }, { "annotations": { diff --git a/pkg/mcp/testdata/toolsets-full-tools-multicluster-enum.json b/pkg/mcp/testdata/toolsets-full-tools-multicluster-enum.json index cec61b95..7041bd3f 100644 --- a/pkg/mcp/testdata/toolsets-full-tools-multicluster-enum.json +++ b/pkg/mcp/testdata/toolsets-full-tools-multicluster-enum.json @@ -235,7 +235,7 @@ "name" ] }, - "name": "node_log" + "name": "nodes_log" }, { "annotations": { diff --git a/pkg/mcp/testdata/toolsets-full-tools-multicluster.json b/pkg/mcp/testdata/toolsets-full-tools-multicluster.json index 7f1f91da..a454f1ef 100644 --- a/pkg/mcp/testdata/toolsets-full-tools-multicluster.json +++ b/pkg/mcp/testdata/toolsets-full-tools-multicluster.json @@ -211,7 +211,7 @@ "name" ] }, - "name": "node_log" + "name": "nodes_log" }, { "annotations": { diff --git a/pkg/mcp/testdata/toolsets-full-tools-openshift.json b/pkg/mcp/testdata/toolsets-full-tools-openshift.json index 066eee0b..5e5fa4ea 100644 --- a/pkg/mcp/testdata/toolsets-full-tools-openshift.json +++ b/pkg/mcp/testdata/toolsets-full-tools-openshift.json @@ -171,7 +171,7 @@ "name" ] }, - "name": "node_log" + "name": "nodes_log" }, { "annotations": { diff --git a/pkg/mcp/testdata/toolsets-full-tools.json b/pkg/mcp/testdata/toolsets-full-tools.json index 17b5e7e2..56a160ed 100644 --- a/pkg/mcp/testdata/toolsets-full-tools.json +++ b/pkg/mcp/testdata/toolsets-full-tools.json @@ -171,7 +171,7 @@ "name" ] }, - "name": "node_log" + "name": "nodes_log" }, { "annotations": { diff --git a/pkg/toolsets/core/nodes.go b/pkg/toolsets/core/nodes.go index 5516dce6..6c669398 100644 --- a/pkg/toolsets/core/nodes.go +++ b/pkg/toolsets/core/nodes.go @@ -13,7 +13,7 @@ import ( func initNodes() []api.ServerTool { return []api.ServerTool{ {Tool: api.Tool{ - Name: "node_log", + Name: "nodes_log", Description: "Get logs from a Kubernetes node (kubelet, kube-proxy, or other system logs). This accesses node logs through the Kubernetes API proxy to the kubelet", InputSchema: &jsonschema.Schema{ Type: "object", @@ -48,12 +48,12 @@ func initNodes() []api.ServerTool { } func nodesLog(params api.ToolHandlerParams) (*api.ToolCallResult, error) { - name := params.GetArguments()["name"] - if name == nil { + name, ok := params.GetArguments()["name"].(string) + if !ok || name == "" { return api.NewToolCallResult("", errors.New("failed to get node log, missing argument name")), nil } - logPath := params.GetArguments()["log_path"] - if logPath == nil { + logPath, ok := params.GetArguments()["log_path"].(string) + if !ok || logPath == "" { logPath = "kubelet.log" } tail := params.GetArguments()["tail"] @@ -63,13 +63,14 @@ func nodesLog(params api.ToolHandlerParams) (*api.ToolCallResult, error) { switch v := tail.(type) { case float64: tailInt = int64(v) - case int: case int64: - tailInt = int64(v) + case int: + case int64: + tailInt = v default: return api.NewToolCallResult("", fmt.Errorf("failed to parse tail parameter: expected integer, got %T", tail)), nil } } - ret, err := params.NodeLog(params, name.(string), logPath.(string), tailInt) + ret, err := params.NodesLog(params, name, logPath, tailInt) if err != nil { return api.NewToolCallResult("", fmt.Errorf("failed to get node log for %s: %v", name, err)), nil } else if ret == "" { From 32ebdd2b5c3d88013de2fb4fed31d42cdfc52ecb Mon Sep 17 00:00:00 2001 From: Marc Nuri Date: Thu, 23 Oct 2025 15:51:36 +0200 Subject: [PATCH 2/2] test(nodes): add test for nodes_log tool with negative tail argument Signed-off-by: Marc Nuri --- pkg/mcp/nodes_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/pkg/mcp/nodes_test.go b/pkg/mcp/nodes_test.go index 12f59fb0..ce2cbc7e 100644 --- a/pkg/mcp/nodes_test.go +++ b/pkg/mcp/nodes_test.go @@ -158,6 +158,23 @@ func (s *NodesSuite) TestNodesLog() { "expected log content '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text) }) }) + s.Run("nodes_log(name=existing-node, log_path=kubelet.log, tail=-1)", func() { + toolResult, err := s.CallTool("nodes_log", map[string]interface{}{ + "name": "existing-node", + "log_path": "kubelet.log", + "tail": -1, + }) + s.Require().NotNil(toolResult, "toolResult should not be nil") + s.Run("no error", func() { + s.Falsef(toolResult.IsError, "call tool should succeed") + s.Nilf(err, "call tool should not return error object") + }) + s.Run("returns full log", func() { + expectedMessage := "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\n" + s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text, + "expected log content '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text) + }) + }) } }