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 <[email protected]>

Author: manusa

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) {

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)

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")

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))
+}

pkg/mcp/testdata/toolsets-core-tools.json

@@ -65,7 +65,7 @@
         "name"
       ]
     },
-    "name": "node_log"
+    "name": "nodes_log"
   },
   {
     "annotations": {

pkg/mcp/testdata/toolsets-full-tools-multicluster-enum.json

@@ -235,7 +235,7 @@
         "name"
       ]
     },
-    "name": "node_log"
+    "name": "nodes_log"
   },
   {
     "annotations": {

pkg/mcp/testdata/toolsets-full-tools-multicluster.json

@@ -211,7 +211,7 @@
         "name"
       ]
     },
-    "name": "node_log"
+    "name": "nodes_log"
   },
   {
     "annotations": {

pkg/mcp/testdata/toolsets-full-tools-openshift.json

@@ -171,7 +171,7 @@
         "name"
       ]
     },
-    "name": "node_log"
+    "name": "nodes_log"
   },
   {
     "annotations": {

pkg/mcp/testdata/toolsets-full-tools.json

@@ -171,7 +171,7 @@
         "name"
       ]
     },
-    "name": "node_log"
+    "name": "nodes_log"
   },
   {
     "annotations": {

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 == "" {