Skip to content

Commit 56f7ede

Browse files
manusamanusa
authored
fix(nodes): reviewed kubernetes.nodes implementation (containers#399)
* 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]> * test(nodes): add test for nodes_log tool with negative tail argument Signed-off-by: Marc Nuri <[email protected]> --------- Signed-off-by: Marc Nuri <[email protected]>
1 parent b1e4757 commit 56f7ede

10 files changed

+221
-76
lines changed

‎pkg/kubernetes/accesscontrol_clientset.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,22 @@ func (a *AccessControlClientset) DiscoveryClient() discovery.DiscoveryInterface
3939
return a.discoveryClient
4040
}
4141

42+
func (a *AccessControlClientset) NodesLogs(ctx context.Context, name, logPath string) (*rest.Request, error) {
43+
gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Node"}
44+
if !isAllowed(a.staticConfig, gvk) {
45+
return nil, isNotAllowedError(gvk)
46+
}
47+
48+
if _, err := a.delegate.CoreV1().Nodes().Get(ctx, name, metav1.GetOptions{}); err != nil {
49+
return nil, fmt.Errorf("failed to get node %s: %w", name, err)
50+
}
51+
52+
url := []string{"api", "v1", "nodes", name, "proxy", "logs", logPath}
53+
return a.delegate.CoreV1().RESTClient().
54+
Get().
55+
AbsPath(url...), nil
56+
}
57+
4258
func (a *AccessControlClientset) Pods(namespace string) (corev1.PodInterface, error) {
4359
gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}
4460
if !isAllowed(a.staticConfig, gvk) {

‎pkg/kubernetes/nodes.go‎

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,33 +5,21 @@ import (
55
"fmt"
66
)
77

8-
func (k *Kubernetes) NodeLog(ctx context.Context, name string, logPath string, tail int64) (string, error) {
8+
func (k *Kubernetes) NodesLog(ctx context.Context, name string, logPath string, tail int64) (string, error) {
99
// Use the node proxy API to access logs from the kubelet
1010
// Common log paths:
1111
// - /var/log/kubelet.log - kubelet logs
1212
// - /var/log/kube-proxy.log - kube-proxy logs
1313
// - /var/log/containers/ - container logs
1414

15-
if logPath == "" {
16-
logPath = "kubelet.log"
15+
req, err := k.AccessControlClientset().NodesLogs(ctx, name, logPath)
16+
if err != nil {
17+
return "", err
1718
}
1819

19-
// Build the URL for the node proxy logs endpoint
20-
url := []string{"api", "v1", "nodes", name, "proxy", "logs", logPath}
21-
2220
// Query parameters for tail
23-
params := make(map[string]string)
2421
if tail > 0 {
25-
params["tailLines"] = fmt.Sprintf("%d", tail)
26-
}
27-
28-
req := k.manager.discoveryClient.RESTClient().
29-
Get().
30-
AbsPath(url...)
31-
32-
// Add tail parameter if specified
33-
for key, value := range params {
34-
req.Param(key, value)
22+
req.Param("tailLines", fmt.Sprintf("%d", tail))
3523
}
3624

3725
result := req.Do(ctx)

‎pkg/mcp/events_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ func (s *EventsSuite) TestEventsListDenied() {
126126
s.InitMcpClient()
127127
s.Run("events_list (denied)", func() {
128128
toolResult, err := s.CallTool("events_list", map[string]interface{}{})
129+
s.Require().NotNil(toolResult, "toolResult should not be nil")
129130
s.Run("has error", func() {
130131
s.Truef(toolResult.IsError, "call tool should fail")
131132
s.Nilf(err, "call tool should not return error object")

‎pkg/mcp/nodes_test.go‎

Lines changed: 185 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,66 +1,205 @@
11
package mcp
22

33
import (
4-
"strings"
4+
"net/http"
55
"testing"
66

7+
"github.com/BurntSushi/toml"
8+
"github.com/containers/kubernetes-mcp-server/internal/test"
79
"github.com/mark3labs/mcp-go/mcp"
8-
corev1 "k8s.io/api/core/v1"
9-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
"github.com/stretchr/testify/suite"
1011
)
1112

12-
func TestNodeLog(t *testing.T) {
13-
testCase(t, func(c *mcpContext) {
14-
c.withEnvTest()
15-
16-
// Create test node
17-
kubernetesAdmin := c.newKubernetesClient()
18-
node := &corev1.Node{
19-
ObjectMeta: metav1.ObjectMeta{
20-
Name: "test-node-log",
21-
},
22-
Status: corev1.NodeStatus{
23-
Addresses: []corev1.NodeAddress{
24-
{Type: corev1.NodeInternalIP, Address: "192.168.1.10"},
25-
},
26-
},
27-
}
13+
type NodesSuite struct {
14+
BaseMcpSuite
15+
mockServer *test.MockServer
16+
}
2817

29-
_, _ = kubernetesAdmin.CoreV1().Nodes().Create(c.ctx, node, metav1.CreateOptions{})
18+
func (s *NodesSuite) SetupTest() {
19+
s.BaseMcpSuite.SetupTest()
20+
s.mockServer = test.NewMockServer()
21+
s.Cfg.KubeConfig = s.mockServer.KubeconfigFile(s.T())
22+
}
3023

31-
// Test node_log tool
32-
toolResult, err := c.callTool("node_log", map[string]interface{}{
33-
"name": "test-node-log",
34-
})
24+
func (s *NodesSuite) TearDownTest() {
25+
s.BaseMcpSuite.TearDownTest()
26+
if s.mockServer != nil {
27+
s.mockServer.Close()
28+
}
29+
}
3530

36-
t.Run("node_log returns successfully or with expected error", func(t *testing.T) {
37-
if err != nil {
38-
t.Fatalf("call tool failed: %v", err)
39-
}
40-
// Node logs might not be available in test environment
41-
// We just check that the tool call completes
42-
if toolResult.IsError {
43-
content := toolResult.Content[0].(mcp.TextContent).Text
44-
// Expected error messages in test environment
45-
if !strings.Contains(content, "failed to get node logs") &&
46-
!strings.Contains(content, "not logged any message yet") {
47-
t.Logf("tool returned error (expected in test environment): %v", content)
31+
func (s *NodesSuite) TestNodesLog() {
32+
s.mockServer.Handle(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
33+
// Get Node response
34+
if req.URL.Path == "/api/v1/nodes/existing-node" {
35+
w.Header().Set("Content-Type", "application/json")
36+
w.WriteHeader(http.StatusOK)
37+
_, _ = w.Write([]byte(`{
38+
"apiVersion": "v1",
39+
"kind": "Node",
40+
"metadata": {
41+
"name": "existing-node"
4842
}
43+
}`))
44+
return
45+
}
46+
// Get Empty Log response
47+
if req.URL.Path == "/api/v1/nodes/existing-node/proxy/logs/empty.log" {
48+
w.Header().Set("Content-Type", "text/plain")
49+
w.WriteHeader(http.StatusOK)
50+
_, _ = w.Write([]byte(``))
51+
return
52+
}
53+
// Get Kubelet Log response
54+
if req.URL.Path == "/api/v1/nodes/existing-node/proxy/logs/kubelet.log" {
55+
w.Header().Set("Content-Type", "text/plain")
56+
w.WriteHeader(http.StatusOK)
57+
logContent := "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\n"
58+
if req.URL.Query().Get("tailLines") != "" {
59+
logContent = "Line 4\nLine 5\n"
4960
}
61+
_, _ = w.Write([]byte(logContent))
62+
return
63+
}
64+
w.WriteHeader(http.StatusNotFound)
65+
}))
66+
s.InitMcpClient()
67+
s.Run("nodes_log(name=nil)", func() {
68+
toolResult, err := s.CallTool("nodes_log", map[string]interface{}{})
69+
s.Require().NotNil(toolResult, "toolResult should not be nil")
70+
s.Run("has error", func() {
71+
s.Truef(toolResult.IsError, "call tool should fail")
72+
s.Nilf(err, "call tool should not return error object")
73+
})
74+
s.Run("describes missing name", func() {
75+
expectedMessage := "failed to get node log, missing argument name"
76+
s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text,
77+
"expected descriptive error '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text)
78+
})
79+
})
80+
s.Run("nodes_log(name=inexistent-node)", func() {
81+
toolResult, err := s.CallTool("nodes_log", map[string]interface{}{
82+
"name": "inexistent-node",
83+
})
84+
s.Require().NotNil(toolResult, "toolResult should not be nil")
85+
s.Run("has error", func() {
86+
s.Truef(toolResult.IsError, "call tool should fail")
87+
s.Nilf(err, "call tool should not return error object")
88+
})
89+
s.Run("describes missing node", func() {
90+
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)"
91+
s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text,
92+
"expected descriptive error '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text)
93+
})
94+
})
95+
s.Run("nodes_log(name=existing-node, log_path=missing.log)", func() {
96+
toolResult, err := s.CallTool("nodes_log", map[string]interface{}{
97+
"name": "existing-node",
98+
"log_path": "missing.log",
99+
})
100+
s.Require().NotNil(toolResult, "toolResult should not be nil")
101+
s.Run("has error", func() {
102+
s.Truef(toolResult.IsError, "call tool should fail")
103+
s.Nilf(err, "call tool should not return error object")
104+
})
105+
s.Run("describes missing log file", func() {
106+
expectedMessage := "failed to get node log for existing-node: failed to get node logs: the server could not find the requested resource"
107+
s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text,
108+
"expected descriptive error '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text)
109+
})
110+
})
111+
s.Run("nodes_log(name=existing-node, log_path=empty.log)", func() {
112+
toolResult, err := s.CallTool("nodes_log", map[string]interface{}{
113+
"name": "existing-node",
114+
"log_path": "empty.log",
115+
})
116+
s.Require().NotNil(toolResult, "toolResult should not be nil")
117+
s.Run("no error", func() {
118+
s.Falsef(toolResult.IsError, "call tool should succeed")
119+
s.Nilf(err, "call tool should not return error object")
120+
})
121+
s.Run("describes empty log", func() {
122+
expectedMessage := "The node existing-node has not logged any message yet or the log file is empty"
123+
s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text,
124+
"expected descriptive message '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text)
125+
})
126+
})
127+
s.Run("nodes_log(name=existing-node, log_path=kubelet.log)", func() {
128+
toolResult, err := s.CallTool("nodes_log", map[string]interface{}{
129+
"name": "existing-node",
130+
"log_path": "kubelet.log",
131+
})
132+
s.Require().NotNil(toolResult, "toolResult should not be nil")
133+
s.Run("no error", func() {
134+
s.Falsef(toolResult.IsError, "call tool should succeed")
135+
s.Nilf(err, "call tool should not return error object")
136+
})
137+
s.Run("returns full log", func() {
138+
expectedMessage := "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\n"
139+
s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text,
140+
"expected log content '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text)
50141
})
51142
})
143+
for _, tailCase := range []interface{}{2, int64(2), float64(2)} {
144+
s.Run("nodes_log(name=existing-node, log_path=kubelet.log, tail=2)", func() {
145+
toolResult, err := s.CallTool("nodes_log", map[string]interface{}{
146+
"name": "existing-node",
147+
"log_path": "kubelet.log",
148+
"tail": tailCase,
149+
})
150+
s.Require().NotNil(toolResult, "toolResult should not be nil")
151+
s.Run("no error", func() {
152+
s.Falsef(toolResult.IsError, "call tool should succeed")
153+
s.Nilf(err, "call tool should not return error object")
154+
})
155+
s.Run("returns tail log", func() {
156+
expectedMessage := "Line 4\nLine 5\n"
157+
s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text,
158+
"expected log content '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text)
159+
})
160+
})
161+
s.Run("nodes_log(name=existing-node, log_path=kubelet.log, tail=-1)", func() {
162+
toolResult, err := s.CallTool("nodes_log", map[string]interface{}{
163+
"name": "existing-node",
164+
"log_path": "kubelet.log",
165+
"tail": -1,
166+
})
167+
s.Require().NotNil(toolResult, "toolResult should not be nil")
168+
s.Run("no error", func() {
169+
s.Falsef(toolResult.IsError, "call tool should succeed")
170+
s.Nilf(err, "call tool should not return error object")
171+
})
172+
s.Run("returns full log", func() {
173+
expectedMessage := "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\n"
174+
s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text,
175+
"expected log content '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text)
176+
})
177+
})
178+
}
52179
}
53180

54-
func TestNodeLogMissingArguments(t *testing.T) {
55-
testCase(t, func(c *mcpContext) {
56-
c.withEnvTest()
57-
58-
t.Run("node_log requires name", func(t *testing.T) {
59-
toolResult, err := c.callTool("node_log", map[string]interface{}{})
60-
61-
if err == nil && !toolResult.IsError {
62-
t.Fatal("expected error when name is missing")
63-
}
181+
func (s *NodesSuite) TestNodesLogDenied() {
182+
s.Require().NoError(toml.Unmarshal([]byte(`
183+
denied_resources = [ { version = "v1", kind = "Node" } ]
184+
`), s.Cfg), "Expected to parse denied resources config")
185+
s.InitMcpClient()
186+
s.Run("nodes_log (denied)", func() {
187+
toolResult, err := s.CallTool("nodes_log", map[string]interface{}{
188+
"name": "does-not-matter",
189+
})
190+
s.Require().NotNil(toolResult, "toolResult should not be nil")
191+
s.Run("has error", func() {
192+
s.Truef(toolResult.IsError, "call tool should fail")
193+
s.Nilf(err, "call tool should not return error object")
194+
})
195+
s.Run("describes denial", func() {
196+
expectedMessage := "failed to get node log for does-not-matter: resource not allowed: /v1, Kind=Node"
197+
s.Equalf(expectedMessage, toolResult.Content[0].(mcp.TextContent).Text,
198+
"expected descriptive error '%s', got %v", expectedMessage, toolResult.Content[0].(mcp.TextContent).Text)
64199
})
65200
})
66201
}
202+
203+
func TestNodes(t *testing.T) {
204+
suite.Run(t, new(NodesSuite))
205+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@
6565
"name"
6666
]
6767
},
68-
"name": "node_log"
68+
"name": "nodes_log"
6969
},
7070
{
7171
"annotations": {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@
235235
"name"
236236
]
237237
},
238-
"name": "node_log"
238+
"name": "nodes_log"
239239
},
240240
{
241241
"annotations": {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@
211211
"name"
212212
]
213213
},
214-
"name": "node_log"
214+
"name": "nodes_log"
215215
},
216216
{
217217
"annotations": {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@
171171
"name"
172172
]
173173
},
174-
"name": "node_log"
174+
"name": "nodes_log"
175175
},
176176
{
177177
"annotations": {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@
171171
"name"
172172
]
173173
},
174-
"name": "node_log"
174+
"name": "nodes_log"
175175
},
176176
{
177177
"annotations": {

0 commit comments

Comments
 (0)