Add external tools MCP server#15
Conversation
Meina-rh
force-pushed
the
external_tools
branch
from
fad3ef1 to
1644993
Compare
Meina-rh
force-pushed
the
external_tools
branch
2 times, most recently
from
e025b45 to
655d563
Compare
|
/cc @arkadeepsen |
Sure @Meina-rh. Can your remove the |
Also remove |
Meina-rh
force-pushed
the
external_tools
branch
from
5e25c14 to
c5994cd
Compare
pkg/external_tools/mcp/tcpdump.go
Outdated
| return nil, types.CommandResult{}, fmt.Errorf("tcpdump %w", err) | ||
| } | ||
|
|
||
| if err := validateIntMax(in.Duration, MaxTcpdumpDuration, "duration", "seconds"); err != nil { |
There was a problem hiding this comment.
Duration is validated but never used.
There was a problem hiding this comment.
I just realized that we don't need Duration param, it doesn't support in tcpdump. We can use packet_count to limit directly, so I delete Duration param
pkg/external_tools/mcp/utils.go
Outdated
| // runPwruDebugNode creates a specialized pwru debug pod and runs a command in it | ||
| func (s *MCPServer) runPwruDebugNode(ctx context.Context, req *mcp.CallToolRequest, nodeName string, command []string) (*mcp.CallToolResult, types.CommandResult, error) { | ||
| if nodeName == "" { | ||
| return nil, types.CommandResult{}, fmt.Errorf("node_name is required for pwru") | ||
| } | ||
|
|
||
| namespace := metav1.NamespaceDefault | ||
| debugPodName, cleanupPod, err := s.createPwruPod(ctx, nodeName, namespace) | ||
| if err != nil { | ||
| return nil, types.CommandResult{}, err | ||
| } | ||
|
|
||
| if cleanupPod != nil { | ||
| defer cleanupPod() | ||
| } | ||
|
|
||
| _, output, err := s.k8sMcpServer.ExecPod(ctx, req, k8stypes.ExecPodParams{ | ||
| NamespacedNameParams: k8stypes.NamespacedNameParams{ | ||
| Name: debugPodName, | ||
| Namespace: namespace, | ||
| }, | ||
| Container: "pwru", | ||
| Command: command, | ||
| }) | ||
| if err != nil { | ||
| return nil, types.CommandResult{}, fmt.Errorf("failed to execute command in pwru debug pod: %w", err) | ||
| } | ||
|
|
||
| return nil, types.CommandResult{Output: output.Stdout}, nil | ||
| } | ||
|
|
||
| // createPwruPod creates a specialized debug pod for pwru with required eBPF capabilities. | ||
| // This pod requires: | ||
| // - cilium/pwru:latest image | ||
| // - /sys/kernel/debug mounted for eBPF | ||
| // - privileged security context | ||
| // - hostNetwork and hostPID for packet tracing | ||
| func (s *MCPServer) createPwruPod(ctx context.Context, node, namespace string) (string, func(), error) { | ||
| hostPathType := corev1.HostPathDirectoryOrCreate | ||
| sleepCommand := []string{"sleep", "3600"} | ||
|
|
||
| // Create a specialized pwru debug pod | ||
| debugPod := &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| GenerateName: "pwru-", | ||
| Namespace: namespace, | ||
| }, | ||
| Spec: corev1.PodSpec{ | ||
| NodeSelector: map[string]string{ | ||
| "kubernetes.io/hostname": node, | ||
| }, | ||
| RestartPolicy: corev1.RestartPolicyNever, | ||
| Tolerations: []corev1.Toleration{ | ||
| { | ||
| Operator: corev1.TolerationOpExists, | ||
| }, | ||
| }, | ||
| HostNetwork: true, | ||
| HostPID: true, | ||
| Volumes: []corev1.Volume{ | ||
| { | ||
| Name: "sys-kernel-debug", | ||
| VolumeSource: corev1.VolumeSource{ | ||
| HostPath: &corev1.HostPathVolumeSource{ | ||
| Path: "/sys/kernel/debug", | ||
| Type: &hostPathType, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Containers: []corev1.Container{ | ||
| { | ||
| Name: "pwru", | ||
| Image: "docker.io/cilium/pwru:latest", | ||
| Command: sleepCommand, | ||
| SecurityContext: &corev1.SecurityContext{ | ||
| Privileged: ptr.To(true), | ||
| }, | ||
| VolumeMounts: []corev1.VolumeMount{ | ||
| { | ||
| Name: "sys-kernel-debug", | ||
| MountPath: "/sys/kernel/debug", | ||
| }, | ||
| }, | ||
| }}, | ||
| }, | ||
| } | ||
|
|
||
| clientSet := s.k8sMcpServer.GetClientSet().GetKubernetesClientSet() | ||
| createdDebugPod, err := clientSet.CoreV1().Pods(namespace).Create(ctx, debugPod, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| return "", nil, fmt.Errorf("failed to create pwru debug pod: %w", err) | ||
| } | ||
|
|
||
| cleanupPod := func() { | ||
| err := clientSet.CoreV1().Pods(namespace).Delete(ctx, createdDebugPod.Name, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| log.Printf("failed to cleanup pwru debug pod: %v", err) | ||
| } | ||
| } | ||
|
|
||
| err = wait.PollUntilContextTimeout(ctx, time.Millisecond*500, time.Minute*1, true, func(ctx context.Context) (bool, error) { | ||
| pod, err := clientSet.CoreV1().Pods(namespace).Get(ctx, createdDebugPod.Name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| return pod.Status.Phase == corev1.PodRunning, nil | ||
| }) | ||
| if err != nil { | ||
| cleanupPod() | ||
| return "", nil, fmt.Errorf("pwru debug pod did not reach running state within timeout of 1 minute: %w", err) | ||
| } | ||
|
|
||
| return createdDebugPod.Name, cleanupPod, nil | ||
| } |
There was a problem hiding this comment.
If we need additonal parameterization of k8s layer's DebugNode functionality, then lets do that. Ideally we should keep the operations specific for a layer separate.
Meina-rh
force-pushed
the
external_tools
branch
from
c5994cd to
f21b45a
Compare
pkg/external_tools/mcp/mcp.go
Outdated
| Description: `Capture network packets with strict safety controls. | ||
|
|
||
| Parameters: | ||
| - Must specify at least 2 of: packet_count, bpf_filter |
There was a problem hiding this comment.
The wording is a little confusing. This kind of translates to "at least 2 of 2". Better to mention both fields are required.
Meina-rh
force-pushed
the
external_tools
branch
2 times, most recently
from
64e49b6 to
3a31f18
Compare
|
@arkadeepsen I've made some updates in tcpdump and pwru according to above comments. could you help review again? thanks! |
|
@coderabbitai: please review |
|
@tssurya: I'll review the changes in this PR. ✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntegrates nettools (tcpdump, pwru) as MCP tools, adds validation/helpers/tests and nettools types, extends Kubernetes debug APIs to accept/validate configurable host/mount paths, and wires nettools into ovnk-mcp-server with new --pwru-image and --tcpdump-image flags. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OVN_MCP_Server
participant NetTools_MCP
participant K8s_MCP
participant Node_Pod
Client->>OVN_MCP_Server: CallTool(tcpdump|pwru, params)
OVN_MCP_Server->>NetTools_MCP: Route request
NetTools_MCP->>NetTools_MCP: Validate params (paths, iface, bpf, limits)
alt validation fails
NetTools_MCP-->>OVN_MCP_Server: Return validation error
else
NetTools_MCP->>K8s_MCP: DebugNode / ExecPod (image, hostPath, mountPath, pod/container)
K8s_MCP->>Node_Pod: Create/exec pod and run command
Node_Pod-->>K8s_MCP: stdout/stderr
K8s_MCP-->>NetTools_MCP: CommandResult
NetTools_MCP-->>OVN_MCP_Server: CallToolResult
end
OVN_MCP_Server-->>Client: Tool execution result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (3)cmd/ovnk-mcp-server/main.go (3)
pkg/nettools/mcp/utils.go (5)
pkg/nettools/mcp/pwru.go (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (21)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/kubernetes/client/nodes.go (1)
128-128: Fix the timeout duration in the error message.The error message mentions "5 minutes" but the actual timeout on Line 118 is
time.Minute*1(1 minute). The message should state "1 minute" for accuracy.🔎 Apply this diff to fix the error message:
- return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 5 minutes: %w", err) + return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 1 minute: %w", err)
♻️ Duplicate comments (1)
pkg/kubernetes/types/nodes.go (1)
12-13: Address the previous review feedback about volume fields.A past review comment suggested using simpler
HostPathandMountPathstring fields instead of fullVolumesandVolumeMountsarrays, noting that debug pods are host-networked and only need basic host path mounting. The comment recommended both fields should be empty or both set, with defaults to "/" and "/host" respectively.The current implementation uses the full Kubernetes Volume/VolumeMount types, which appears to diverge from that feedback. Please clarify if this design decision was intentional and addresses the security/simplicity concerns raised.
Based on learnings, a maintainer previously requested this change.
🧹 Nitpick comments (7)
pkg/external_tools/mcp/mcp.go (1)
14-18: Consider adding nil check for the k8sMcpServer parameter.If
k8sMcpServerisnil, calling methods likeTcpdumporPwruthat uses.k8sMcpServerwill cause a nil pointer dereference at runtime. Adding defensive validation here would fail fast with a clear error message.🔎 Suggested fix
// NewMCPServer creates a new MCP server instance -func NewMCPServer(k8sMcpServer *kubernetesmcp.MCPServer) *MCPServer { +func NewMCPServer(k8sMcpServer *kubernetesmcp.MCPServer) (*MCPServer, error) { + if k8sMcpServer == nil { + return nil, fmt.Errorf("k8sMcpServer cannot be nil") + } return &MCPServer{ k8sMcpServer: k8sMcpServer, - } + }, nil }pkg/external_tools/mcp/utils_test.go (1)
212-225: Consider usingslices.Equalfor cleaner slice comparison.The manual element-by-element comparison works correctly but can be simplified with the standard library.
🔎 Suggested fix
+import "slices" + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := tt.builder().build() - if len(got) != len(tt.want) { - t.Errorf("commandBuilder.build() length = %v, want %v", len(got), len(tt.want)) - return - } - for i := range got { - if got[i] != tt.want[i] { - t.Errorf("commandBuilder.build()[%d] = %v, want %v", i, got[i], tt.want[i]) - } + if !slices.Equal(got, tt.want) { + t.Errorf("commandBuilder.build() = %v, want %v", got, tt.want) } }) }pkg/external_tools/mcp/tcpdump.go (1)
75-77: Remove extraneous blank line before closing brace.Minor style issue: there's an unnecessary blank line at line 76.
🔎 Suggested fix
default: return nil, types.CommandResult{}, fmt.Errorf("invalid target_type: %s (must be 'node' or 'pod')", in.TargetType) - }pkg/external_tools/mcp/pwru.go (1)
56-61: Consider validatingNodeNameis not empty.If
in.NodeNameis empty, the error fromrunDebugNodemay not be immediately clear. Early validation would provide a better error message.🔎 Suggested fix
+ if in.NodeName == "" { + return nil, types.CommandResult{}, fmt.Errorf("node_name is required") + } + target := types.DebugNodeParams{ NodeName: in.NodeName, NodeImage: "docker.io/cilium/pwru:v1.0.10",This would require adding
"fmt"to the imports.pkg/external_tools/mcp/tcpdump_test.go (1)
429-431: Consider the default packet count behavior.Similar to the pwru test, when
packetCountis 0, it defaults toMaxPacketCount. However, Line 443 showssnaplendefaults toDefaultSnaplen(96 bytes), which seems more user-friendly. Consider whether packet count should also default to a reasonable value rather than the maximum.pkg/external_tools/mcp/utils.go (2)
24-34: Consider including stderr in the command result.The function returns only
output.Stdout(Line 34) but discardsoutput.Stderr. For debugging purposes, stderr often contains valuable diagnostic information. Consider including both in theCommandResultor at least logging stderr when it's non-empty.
38-56: Good validation and defaulting logic.The namespace defaulting to "default" (Lines 42-44) and validation checks are appropriate. However, similar to
runDebugNode, consider including stderr in the output for better debugging support.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/ovnk-mcp-server/main.go(2 hunks)pkg/external_tools/mcp/mcp.go(1 hunks)pkg/external_tools/mcp/pwru.go(1 hunks)pkg/external_tools/mcp/pwru_test.go(1 hunks)pkg/external_tools/mcp/tcpdump.go(1 hunks)pkg/external_tools/mcp/tcpdump_test.go(1 hunks)pkg/external_tools/mcp/utils.go(1 hunks)pkg/external_tools/mcp/utils_test.go(1 hunks)pkg/external_tools/types/manifest.go(1 hunks)pkg/kubernetes/client/nodes.go(4 hunks)pkg/kubernetes/mcp/nodes.go(1 hunks)pkg/kubernetes/types/nodes.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/external_tools/mcp/tcpdump.go (2)
pkg/external_tools/types/manifest.go (4)
TcpdumpParams(24-41)CommandResult(54-58)DebugNodeParams(8-13)ExecPodParams(16-20)pkg/external_tools/mcp/utils.go (2)
ValidateInterface(97-111)ValidateBPFFilter(114-129)
pkg/kubernetes/client/nodes.go (1)
pkg/kubernetes/client/client.go (1)
OVNKMCPServerClientSet(13-20)
pkg/external_tools/mcp/tcpdump_test.go (2)
pkg/external_tools/mcp/tcpdump.go (3)
MaxPacketCount(13-13)MaxSnaplen(15-15)DefaultSnaplen(14-14)pkg/external_tools/mcp/utils.go (2)
ValidateInterface(97-111)ValidateBPFFilter(114-129)
pkg/external_tools/mcp/utils.go (4)
pkg/external_tools/mcp/mcp.go (1)
MCPServer(9-11)pkg/external_tools/types/manifest.go (3)
DebugNodeParams(8-13)CommandResult(54-58)ExecPodParams(16-20)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(8-14)pkg/kubernetes/types/common.go (1)
NamespacedNameParams(37-40)
🔇 Additional comments (21)
pkg/external_tools/mcp/mcp.go (1)
21-59: LGTM! Tool registration with comprehensive descriptions.The tool descriptions are well-structured with clear parameter documentation, constraints (defaults/max values), and practical examples. The pwru image version is now correctly specified as
v1.0.10(addressing the past review comment about the "latest tag").cmd/ovnk-mcp-server/main.go (1)
50-53: LGTM! Clean integration following established patterns.The external tools server is wired in consistently with the other MCP servers (OVS, kernel). The placement after kernel tools and the logging convention match the existing code structure.
pkg/external_tools/mcp/utils_test.go (2)
7-98: LGTM! Comprehensive boundary testing for validateIntMax.Excellent coverage including zero, minimum valid, boundary (equals max, just over), and negative value cases. The table-driven approach is clean and maintainable.
100-141: LGTM! Good edge case coverage for stringWithDefault.The whitespace test case (lines 126-130) is particularly valuable—it correctly documents that whitespace-only strings are not treated as empty.
pkg/external_tools/mcp/tcpdump.go (2)
21-27: LGTM! Input validation is properly ordered.Interface and BPF filter validation using the centralized helpers ensures consistent security checks before any command execution.
61-77: The implementation is correct. The method signature follows the official MCP SDK's generic tool handler pattern (ToolHandlerFor[In, Out]), which returns(*mcp.CallToolResult, Out, error). ReturningnilforCallToolResultwhile using the generic output type (types.CommandResult) as the result carrier is the standard and documented pattern in the official Go SDK. This same pattern is consistently applied across all tool handlers in the codebase (tcpdump, pwru, DebugNode, ExecPod). No changes are needed.Likely an incorrect or invalid review comment.
pkg/external_tools/mcp/pwru.go (2)
28-30: PcapFilter validation now implemented.This addresses the past review comment requesting validation of the
PcapFilterfield before command execution.
35-54: LGTM! Volume configuration for eBPF debugging.The
/sys/kernel/debugmount withHostPathDirectoryOrCreateis correctly configured for pwru's eBPF requirements. The volume naming is clear and the mount path matches the host path appropriately.pkg/kubernetes/mcp/nodes.go (1)
12-12: LGTM!The parameter pass-through correctly forwards the new volume configuration fields to the underlying client.
pkg/external_tools/mcp/pwru_test.go (2)
7-257: Excellent test coverage!The test cases comprehensively cover valid BPF filters, injection attack vectors, and output limit boundaries. The security-focused validation tests provide strong protection against command injection attempts.
272-274: No action needed - implementation is correct.The default behavior is sound. When
outputLimitLinesis 0, it defaults toMaxOutputLimitLines, which is defined as 1000. This is a reasonable default, not an edge case to the maximum. The test correctly reflects the implementation inpwru.go.Likely an incorrect or invalid review comment.
pkg/kubernetes/client/nodes.go (2)
15-17: LGTM!The signature updates correctly extend both
DebugNodeandcreatePodto accept volume configuration, maintaining consistency across the call chain.
39-62: Well-designed defaulting logic.The nil checks ensure safe defaults (mounting host "/" to "/host"), which aligns well with typical debug pod requirements while still allowing custom volume configurations when needed.
pkg/external_tools/mcp/tcpdump_test.go (1)
7-405: Comprehensive validation test suite.Excellent coverage of interface validation, BPF filter security checks, and numeric boundary conditions. The injection attack tests provide strong protection against command injection vulnerabilities.
pkg/external_tools/types/manifest.go (2)
11-12: Appropriate use of json:"-" tags.Excluding
VolumesandVolumeMountsfrom JSON serialization is correct since these are populated internally by the MCP server rather than provided by users in the API request.
37-50: jsonschema max values are correctly aligned with implementation constants.Verified that all three max values in the jsonschema annotations match their corresponding implementation constants:
packet_countmax 1000 matchesMaxPacketCountin tcpdump.gosnaplenmax 262 matchesMaxSnaplenin tcpdump.gooutput_limit_linesmax 1000 matchesMaxOutputLimitLinesin pwru.gopkg/external_tools/mcp/utils.go (5)
59-94: Well-designed command builder.The fluent interface provides a clean, readable way to construct commands with conditional arguments. The pattern helps prevent common mistakes in command construction.
96-111: Solid interface name validation.The 15-character limit matches Linux
IFNAMSIZand the pattern correctly enforces safe interface naming conventions while preventing injection attacks.
113-129: Robust BPF filter validation.The validation effectively prevents command injection by blocking dangerous shell metacharacters. The comprehensive list (semicolon, pipe, ampersand, backtick, dollar, command substitution, newline, null byte) provides strong security.
Note: The list includes both "$" and "$(" separately, which provides defense in depth but is slightly redundant since "$" alone would catch both patterns.
131-143: Clean validation helper.The function provides clear error messages and handles both unit-based and unitless numeric validation appropriately.
145-151: LGTM!Simple, readable helper for string defaulting that improves command construction clarity.
tssurya
left a comment
tssurya
left a comment
There was a problem hiding this comment.
testing approval won't merge this yet.
tssurya
left a comment
tssurya
left a comment
There was a problem hiding this comment.
nice test works, removing approval.
Meina-rh
force-pushed
the
external_tools
branch
from
3a31f18 to
147cbed
Compare
Meina-rh
force-pushed
the
external_tools
branch
from
147cbed to
221fa1d
Compare
arkadeepsen
left a comment
arkadeepsen
left a comment
There was a problem hiding this comment.
The .idea directory again got added. It's better to add .idea/ in the .gitignore file so that the directory is ignored while pushing the commits.
Check the comments by @coderabbitai as well and try to incorporate the suggestions.
pkg/external_tools/mcp/mcp.go
Outdated
| - bpf_filter: BPF filter expression (optional) | ||
| - snaplen: Snapshot length in bytes (default: 96, max: 262) | ||
| - interface: Network interface name or 'any' (optional, uses default if not specified) | ||
| - target_type: 'node' or 'pod' (optional, uses default interface if not specified) |
There was a problem hiding this comment.
The target type field is not optional. The validation checks whether the value is set to either node or pod, otherwise an error is returned.
There was a problem hiding this comment.
yep, I deleted optional. And it will check the value if it's set to node or pod in tcpdump.go switch in.TargetType
| err := ValidateInterface(tt.iface) | ||
| if err != nil { | ||
| if !tt.wantError { | ||
| t.Errorf("ValidateInterface(%q) error = %v, wantError %v", tt.iface, err, tt.wantError) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // Validate BPF filter | ||
| err = ValidateBPFFilter(tt.filter) | ||
| if err != nil { | ||
| if !tt.wantError { | ||
| t.Errorf("ValidateBPFFilter(%q) error = %v, wantError %v", tt.filter, err, tt.wantError) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // Validate packet count | ||
| packetCount := tt.packetCount | ||
| if packetCount == 0 { | ||
| packetCount = MaxPacketCount | ||
| } | ||
| err = validateIntMax(packetCount, MaxPacketCount, "packet_count", "") | ||
| if err != nil { | ||
| if !tt.wantError { | ||
| t.Errorf("validateIntMax(packetCount=%d) error = %v, wantError %v", tt.packetCount, err, tt.wantError) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // Validate snaplen | ||
| snaplen := tt.snaplen | ||
| if snaplen == 0 { | ||
| snaplen = DefaultSnaplen | ||
| } | ||
| err = validateIntMax(snaplen, MaxSnaplen, "snaplen", "bytes") | ||
| if (err != nil) != tt.wantError { | ||
| t.Errorf("validateIntMax(snaplen=%d) error = %v, wantError %v", tt.snaplen, err, tt.wantError) | ||
| } |
There was a problem hiding this comment.
Let's just test all the validation functions in the utils_test.go file.
There was a problem hiding this comment.
@arkadeepsen I've removed all the validation functions in utils_test.go. But seems like pwru and tcpdump don't need extra unit tests after removing. I just left some comments in pwru_test.go and tcpdump_test.go
There was a problem hiding this comment.
Let's remove the files altogether if there are no tests in them.
Meina-rh
force-pushed
the
external_tools
branch
from
221fa1d to
0a18efd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/kubernetes/client/nodes.go (2)
90-96: HardcodedHOSTenvironment variable may conflict with custommountPath.The
HOSTenvironment variable is hardcoded to"/host", but the actual mount path is now configurable. If a caller sets a differentmountPath, tools relying on theHOSTenv var (like sosreport) may look in the wrong location.🔎 Proposed fix: Use mountPath for HOST env var
Env: []corev1.EnvVar{ { // to collect more sos report requires this env var is set Name: "HOST", - Value: "/host", + Value: mountPath, }, },
126-126: Error message mentions 5 minutes but timeout is 1 minute.The error message says "within timeout of 5 minutes" but Line 116 sets timeout to
time.Minute*1.🔎 Proposed fix
-return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 5 minutes: %w", err) +return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 1 minute: %w", err)
🧹 Nitpick comments (3)
pkg/external_tools/mcp/utils_test.go (1)
499-582: Command builder tests cover key scenarios.Tests verify the fluent API including base args, chaining, conditional adds, and
addIfNotEmpty. Consider usingreflect.DeepEqualorslices.Equalfor slice comparison instead of manual loop.🔎 Optional improvement using slices.Equal
+import "slices" + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := tt.builder().build() - if len(got) != len(tt.want) { - t.Errorf("commandBuilder.build() length = %v, want %v", len(got), len(tt.want)) - return - } - for i := range got { - if got[i] != tt.want[i] { - t.Errorf("commandBuilder.build()[%d] = %v, want %v", i, got[i], tt.want[i]) - } - } + if !slices.Equal(got, tt.want) { + t.Errorf("commandBuilder.build() = %v, want %v", got, tt.want) + } }) }pkg/external_tools/mcp/pwru.go (1)
32-33: ClarifyaddIfNotEmptyusage.The call
addIfNotEmpty(in.PcapFilter, in.PcapFilter)works but is not immediately intuitive. Consider adding a brief comment or using a more explicit pattern.🔎 Optional: More explicit approach
cmd := newCommand("pwru", "--output-limit-lines", strconv.Itoa(outputLimitLines)) -cmd.addIfNotEmpty(in.PcapFilter, in.PcapFilter) +if in.PcapFilter != "" { + cmd.add(in.PcapFilter) +}pkg/external_tools/mcp/utils.go (1)
113-129: Packet filter validation provides command injection protection.The blocklist approach covers common shell injection vectors. Consider whether additional patterns like
>,<, or'(single quotes) should be blocked.🔎 Consider expanding dangerous patterns
-dangerous := []string{";", "|", "&", "`", "$", "$(", "\n", "\x00"} +dangerous := []string{";", "|", "&", "`", "$", "$(", "\n", "\x00", ">", "<", "'", "\"", "\\"}This would block output redirection (
>,<), quote escapes (',"), and backslash escapes that could be used in injection attempts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/ovnk-mcp-server/main.gopkg/external_tools/mcp/mcp.gopkg/external_tools/mcp/pwru.gopkg/external_tools/mcp/pwru_test.gopkg/external_tools/mcp/tcpdump.gopkg/external_tools/mcp/tcpdump_test.gopkg/external_tools/mcp/utils.gopkg/external_tools/mcp/utils_test.gopkg/external_tools/types/manifest.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.go
✅ Files skipped from review due to trivial changes (1)
- pkg/external_tools/mcp/pwru_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/kubernetes/types/nodes.go
- pkg/external_tools/mcp/tcpdump_test.go
- pkg/external_tools/mcp/tcpdump.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/kubernetes/client/nodes.go (1)
pkg/kubernetes/client/client.go (1)
OVNKMCPServerClientSet(13-20)
pkg/external_tools/mcp/utils.go (4)
pkg/external_tools/mcp/mcp.go (1)
MCPServer(9-11)pkg/external_tools/types/manifest.go (3)
DebugNodeParams(4-9)CommandResult(50-54)ExecPodParams(12-16)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)pkg/kubernetes/types/common.go (1)
NamespacedNameParams(37-40)
cmd/ovnk-mcp-server/main.go (5)
pkg/external_tools/mcp/mcp.go (1)
NewMCPServer(14-18)pkg/kernel/mcp/mcp.go (1)
NewMCPServer(18-22)pkg/ovs/mcp/mcp.go (1)
NewMCPServer(20-24)pkg/sosreport/mcp/mcp.go (1)
NewMCPServer(15-17)pkg/kubernetes/mcp/mcp.go (1)
NewMCPServer(19-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (25)
pkg/external_tools/mcp/utils_test.go (4)
7-98: Well-structured boundary tests forvalidateIntMax.Good coverage of zero, minimum valid, boundary, over-limit, and negative value cases with clear test naming.
143-296: Comprehensive interface validation tests.Excellent coverage of valid patterns (eth0, VLAN interfaces, hyphens, underscores) and security-critical invalid patterns (injection characters, length limits, invalid starting characters).
298-497: Thorough packet filter validation tests with injection prevention.Good coverage of command injection attempts (semicolon, pipe, backtick, dollar sign, command substitution) and length validation. The test logic at lines 483-488 correctly handles the length test cases by replacing null bytes with valid characters.
1-5: The package declaration is consistent with all other files in thepkg/external_tools/mcp/directory. All files in this directory declarepackage external_tools, notmcp. The directory structure and package naming follow standard Go conventions where the package name doesn't need to match the directory name.Likely an incorrect or invalid review comment.
cmd/ovnk-mcp-server/main.go (2)
14-14: New import follows existing conventions.The import alias
externaltoolsmcpis consistent with other MCP package imports in this file.
50-53: External tools server integration follows established pattern.The initialization and tool registration mirrors the pattern used for kernel and OVS servers. Clean integration into the live-cluster flow.
pkg/kubernetes/mcp/nodes.go (1)
11-12: Parameter forwarding correctly implemented.The
HostPathandMountPathfields fromDebugNodeParamsare now correctly passed to the client'sDebugNodemethod.pkg/external_tools/mcp/mcp.go (3)
13-18: Constructor follows established pattern.The
NewMCPServerfunction matches the pattern used by kernel and OVS MCP servers (accepts*kubernetesmcp.MCPServerand stores it).
21-67: Tool descriptions are comprehensive and well-documented.The tool descriptions include clear parameter documentation with types, defaults, and constraints, plus practical examples. The pwru image is correctly pinned to
v1.0.10rather thanlatest.
1-6: No changes needed. The package nameexternal_toolsis correct and follows the established codebase convention where packages in subdirectories are named after their parent directory (matchingkernel,kubernetes, and other top-level package names). Onlyovs/mcpusesmcpas an exception.Likely an incorrect or invalid review comment.
pkg/external_tools/mcp/pwru.go (3)
11-14: Sensible defaults and limits for output.Default of 100 lines and max of 1000 are reasonable constraints for trace output.
19-30: Input validation properly implemented.The method validates
OutputLimitLinesagainst the maximum and validatesPcapFilterfor dangerous characters. This addresses the previous review comment about validatingPcapFilterbefore use.
35-49: Debug node setup with eBPF mounts looks correct.The
/sys/kernel/debugmount is required for eBPF operations, and the pinned pwru image version ensures reproducibility.pkg/kubernetes/client/nodes.go (2)
15-17: Extended signature correctly propagates new parameters.The
DebugNodemethod now accepts and forwardshostPathandmountPathtocreatePod.
39-45: Sensible defaults for path parameters.Defaulting
hostPathto"/"andmountPathto"/host"maintains backward compatibility while allowing customization.pkg/external_tools/mcp/utils.go (6)
14-14: Regex compiled at package level for performance.Good practice to compile the regex once rather than on each validation call.
16-35:runDebugNodevalidates required parameters and delegates correctly.Properly checks for
NodeNameandNodeImagebefore delegating to the Kubernetes MCP server with structured parameters.
37-57:runExecPodhandles namespace defaulting properly.Good defensive coding to default
PodNamespaceto"default"if empty.
59-94: Fluent command builder API is clean and reusable.The builder pattern with
add,addIf, andaddIfNotEmptyprovides a clear API for constructing commands.
96-111: Interface validation covers Linux naming constraints.Correctly validates length (max 15 chars per IFNAMSIZ), allows "any" keyword, and uses regex to enforce valid characters while blocking dangerous ones.
131-151: Utility functions are straightforward and correct.Both
validateIntMaxandstringWithDefaultare simple helpers with clear semantics.pkg/external_tools/types/manifest.go (4)
18-37: Well-structured multi-target design.The
TcpdumpParamsstructure effectively handles both node-level and pod-level packet capture scenarios through theTargetTypediscriminator. The separation of target-specific fields with appropriateomitemptytags for optional parameters is clean and maintainable.
39-47: LGTM: Clean parameter structure.The
PwruParamsstructure is appropriately scoped for the pwru tool with a requiredNodeNamefield and optional filtering/output parameters. The documentation clearly explains the tool's purpose.
49-54: LGTM: Standard result structure.The
CommandResultstructure follows conventional patterns for representing command execution outcomes with output, exit code, and error information.
4-16: Address code duplication between k8s layer and external_tools type definitions.These types are redefined in both locations but with significant differences:
- DebugNodeParams: k8s version (
pkg/kubernetes/types/nodes.go:4) includesCommandfield and uses"hostPath,omitempty"JSON tags, while this version omitsCommandand usesjson:"-"tags. Field names also differ (Name/ImagevsNodeName/NodeImage).- ExecPodParams: k8s version (
pkg/kubernetes/types/pods.go:17) includesCommandfield and embedsNamespacedNameParams, while this version defines fields explicitly asPodName/PodNamespaceand lacksCommand.This duplication creates a maintenance burden and risks divergence. Either consolidate to use the k8s layer types (adjusting if needed), or document why separate definitions are necessary and ensure both stay synchronized.
Meina-rh
force-pushed
the
external_tools
branch
from
0a18efd to
45bfd77
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @pkg/external_tools/mcp/utils.go:
- Line 17: The function signature for runDebugNode currently returns an unused
first value (*mcp.CallToolResult) that is always nil; remove this unused return
from runDebugNode (and any sibling functions with the same pattern on lines
~25-29) so the signature returns only (types.CommandResult, error), update all
callers to expect the new two-value return, and adjust any documentation/tests
to reflect the changed return type; if there is intended logic to populate
*mcp.CallToolResult instead, implement that population in runDebugNode rather
than keeping a consistently nil return.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/kubernetes/client/nodes.go (1)
90-95: Potential inconsistency: HOST env var hardcoded while mountPath is configurable.The
HOSTenvironment variable is hardcoded to/host(line 94), but the actual mount path can now be configured via themountPathparameter (line 87). If a caller sets a custommountPath, tools relying on theHOSTenv var may reference an incorrect path.Consider using the
mountPathvariable for the env var value to maintain consistency:🔎 Suggested fix
Env: []corev1.EnvVar{ { // to collect more sos report requires this env var is set Name: "HOST", - Value: "/host", + Value: mountPath, }, },
♻️ Duplicate comments (1)
pkg/external_tools/mcp/utils.go (1)
33-33: Unused first return value in function signature.Similar to
runDebugNode, this function always returnsnilas the first value. The same design issue applies here - either populate the value or remove it from the signature to avoid confusion.🔎 Suggested fix: Remove unused return value
-func (s *MCPServer) runExecPod(ctx context.Context, req *mcp.CallToolRequest, target k8stypes.ExecPodParams, command []string) (*mcp.CallToolResult, types.CommandResult, error) { +func (s *MCPServer) runExecPod(ctx context.Context, req *mcp.CallToolRequest, target k8stypes.ExecPodParams, command []string) (types.CommandResult, error) { if target.NamespacedNameParams.Name == "" { - return nil, types.CommandResult{}, fmt.Errorf("pod_name is required when target_type is 'pod'") + return types.CommandResult{}, fmt.Errorf("pod_name is required when target_type is 'pod'") } if target.NamespacedNameParams.Namespace == "" { target.NamespacedNameParams.Namespace = "default" } target.Command = command _, output, err := s.k8sMcpServer.ExecPod(ctx, req, target) if err != nil { - return nil, types.CommandResult{}, err + return types.CommandResult{}, err } - return nil, types.CommandResult{Output: output.Stdout}, nil + return types.CommandResult{Output: output.Stdout}, nil }Also applies to: 41-45
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/ovnk-mcp-server/main.gopkg/external_tools/mcp/mcp.gopkg/external_tools/mcp/pwru.gopkg/external_tools/mcp/pwru_test.gopkg/external_tools/mcp/tcpdump.gopkg/external_tools/mcp/tcpdump_test.gopkg/external_tools/mcp/utils.gopkg/external_tools/mcp/utils_test.gopkg/external_tools/types/manifest.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.go
✅ Files skipped from review due to trivial changes (1)
- pkg/external_tools/mcp/pwru_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/ovnk-mcp-server/main.go
- pkg/external_tools/mcp/tcpdump_test.go
- pkg/kubernetes/types/nodes.go
- pkg/external_tools/mcp/tcpdump.go
- pkg/kubernetes/mcp/nodes.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/kubernetes/client/nodes.go (1)
pkg/kubernetes/client/client.go (1)
OVNKMCPServerClientSet(13-20)
pkg/external_tools/mcp/pwru.go (3)
pkg/external_tools/mcp/mcp.go (1)
MCPServer(9-11)pkg/external_tools/types/manifest.go (2)
PwruParams(26-32)CommandResult(35-39)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (14)
pkg/kubernetes/client/nodes.go (1)
39-45: LGTM! Clear defaulting logic for mount parameters.The defaulting logic ensures safe fallback values when hostPath or mountPath are not provided. The choice of "/" for hostPath (entire host filesystem) and "/host" for mountPath (conventional mount point) are sensible defaults for debug containers.
pkg/external_tools/mcp/mcp.go (3)
8-18: LGTM! Clean wrapper design.The MCPServer wrapper provides a clean abstraction around the Kubernetes MCP server, with a straightforward constructor pattern.
20-46: Comprehensive tcpdump tool description with clear examples.The tool description effectively documents parameters, requirements, and usage patterns for both node-level and pod-level packet capture scenarios.
47-68: The cilium/pwru:v1.0.10 image version is verified as current and secure. This is the latest stable release with no known pwru-specific CVEs or security advisories. Docker images are actively maintained and published to docker.io/cilium/pwru. Note that kernel-level eBPF vulnerabilities (e.g., CVE-2025-37959) may affect pwru behavior, but these are infrastructure concerns requiring kernel or distro patches—not pwru issues.pkg/external_tools/mcp/utils_test.go (1)
1-582: Excellent test coverage with strong security focus.This test suite demonstrates thorough validation testing:
- Comprehensive boundary and edge case coverage for integer validation
- Extensive interface name validation preventing command injection
- Strong packet filter validation with explicit injection attack scenarios
- Clean command builder API tests
The security-conscious approach (testing against semicolons, pipes, backticks, dollar signs, etc.) is particularly commendable for network tool wrappers.
pkg/external_tools/mcp/pwru.go (3)
12-15: LGTM! Constants align with tool description.The default (100) and maximum (1000) output line limits match the descriptions in
pkg/external_tools/mcp/mcp.go(line 61).
20-31: Solid validation with proper defaulting.The method correctly:
- Defaults
outputLimitLinesto 100 when zero- Validates against the maximum limit of 1000
- Validates the PcapFilter for safety (preventing injection attacks)
36-41: Correct mount configuration for eBPF tracing.The configuration appropriately:
- Mounts
/sys/kernel/debug(required for eBPF programs to access kernel debug filesystem)- Uses the same path for both HostPath and MountPath (direct access to kernel debug fs)
- Specifies the pwru image version explicitly
pkg/external_tools/types/manifest.go (1)
1-39: LGTM! Well-structured type definitions.The parameter and result types are cleanly designed:
- Appropriate use of
omitemptyfor optional fields- Required fields (e.g.,
NodeNamein PwruParams,TargetTypein TcpdumpParams) correctly lackomitempty- Field types match their intended usage
- Clear separation of concerns between tcpdump, pwru, and result types
pkg/external_tools/mcp/utils.go (5)
48-83: LGTM! Clean fluent interface for command building.The
commandBuilderprovides a clear, fluent API for constructing command arguments with conditional logic. The implementation is straightforward and will help maintain readable code when building complex commands for tcpdump and pwru.
85-100: LGTM! Proper interface name validation.The validation correctly enforces Linux interface name constraints (15 character limit, valid character set) and appropriately handles the special "any" value used by packet capture tools.
120-132: LGTM! Clear validation with descriptive errors.The function provides appropriate bounds checking with context-aware error messages that will help users understand validation failures.
134-140: LGTM! Simple utility function.Clear helper for handling default values.
102-118: The code already implements proper defense-in-depth for packet filter handling.The
validatePacketFilterblacklist correctly rejects shell metacharacters. Additionally, both tcpdump.go (line 49) and pwru.go (line 34) pass filters as separate command arguments via thecommandBuilderpattern rather than shell string concatenation. This means filters are processed byexec.Command-like mechanisms without shell interpretation, providing a second layer of defense. No changes needed.
Meina-rh
force-pushed
the
external_tools
branch
from
45bfd77 to
16e0cbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/external_tools/mcp/pwru_test.go (1)
1-9: Consider removing this empty test file or adding Pwru-specific integration tests.While the comment explains that validation functions are tested in
utils_test.go, having an empty test file serves no purpose. Consider either:
- Removing this file entirely (recommended)
- Adding integration tests specific to the Pwru method (e.g., testing the complete flow, command construction, or edge cases specific to pwru behavior)
pkg/external_tools/mcp/tcpdump.go (1)
20-86: Unused first return value in function signature.The function always returns
nilfor*mcp.CallToolResult(lines 70, 82). This pattern was already addressed inrunDebugNodeandrunExecPod(per past review comments), which now return only two values. For consistency and API clarity, consider removing the unused return value from theTcpdumpsignature as well.🔎 Proposed signature update
-func (s *MCPServer) Tcpdump(ctx context.Context, req *mcp.CallToolRequest, in types.TcpdumpParams) (*mcp.CallToolResult, types.CommandResult, error) { +func (s *MCPServer) Tcpdump(ctx context.Context, req *mcp.CallToolRequest, in types.TcpdumpParams) (types.CommandResult, error) {Then update return statements:
case "node": result, err := s.runDebugNode(ctx, req, k8stypes.DebugNodeParams{ Name: in.NodeName, Image: in.NodeImage, }, cmd.build()) if err != nil { - return nil, types.CommandResult{}, err + return types.CommandResult{}, err } - return nil, result, nil + return result, nil case "pod": result, err := s.runExecPod(ctx, req, k8stypes.ExecPodParams{ NamespacedNameParams: k8stypes.NamespacedNameParams{ Name: in.PodName, Namespace: in.PodNamespace, }, Container: in.ContainerName, }, cmd.build()) if err != nil { - return nil, types.CommandResult{}, err + return types.CommandResult{}, err } - return nil, result, nil + return result, nil default: - return nil, types.CommandResult{}, fmt.Errorf("invalid target_type: %s (must be 'node' or 'pod')", in.TargetType) + return types.CommandResult{}, fmt.Errorf("invalid target_type: %s (must be 'node' or 'pod')", in.TargetType)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/ovnk-mcp-server/main.gopkg/external_tools/mcp/mcp.gopkg/external_tools/mcp/pwru.gopkg/external_tools/mcp/pwru_test.gopkg/external_tools/mcp/tcpdump.gopkg/external_tools/mcp/tcpdump_test.gopkg/external_tools/mcp/utils.gopkg/external_tools/mcp/utils_test.gopkg/external_tools/types/manifest.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/kubernetes/types/nodes.go
- pkg/external_tools/mcp/utils_test.go
- pkg/external_tools/mcp/tcpdump_test.go
- pkg/kubernetes/client/nodes.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/ovnk-mcp-server/main.go (5)
pkg/external_tools/mcp/mcp.go (1)
NewMCPServer(14-18)pkg/kernel/mcp/mcp.go (1)
NewMCPServer(18-22)pkg/ovs/mcp/mcp.go (1)
NewMCPServer(20-24)pkg/sosreport/mcp/mcp.go (1)
NewMCPServer(15-17)pkg/kubernetes/mcp/mcp.go (1)
NewMCPServer(19-42)
pkg/external_tools/mcp/pwru.go (3)
pkg/external_tools/mcp/mcp.go (1)
MCPServer(9-11)pkg/external_tools/types/manifest.go (2)
PwruParams(26-32)CommandResult(35-39)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)
🔇 Additional comments (17)
cmd/ovnk-mcp-server/main.go (1)
14-14: LGTM! Clean integration following established patterns.The external tools server integration follows the same pattern as the existing Kernel and OVS MCP servers, maintaining consistency across the codebase.
Also applies to: 51-53
pkg/kubernetes/mcp/nodes.go (1)
12-12: LGTM! Proper parameter propagation.The addition of
hostPathandmountPathparameters correctly propagates the new configuration options from the input to the client method, enabling flexible volume mounting for debug operations.pkg/external_tools/mcp/mcp.go (2)
13-18: LGTM! Constructor follows established pattern.The constructor correctly follows the same pattern used by Kernel, OVS, and other MCP servers in the codebase, maintaining consistency.
20-68: LGTM! Comprehensive tool registration.The tool registration provides clear, detailed descriptions with examples for both tcpdump and pwru. The documentation properly distinguishes required from optional parameters and provides practical usage examples.
pkg/external_tools/mcp/pwru.go (4)
21-27: LGTM! Proper validation with sensible defaults.The output limit validation correctly defaults to 100 and enforces a maximum of 1000, providing reasonable bounds for trace output.
29-31: LGTM! Filter validation added.The PcapFilter validation correctly addresses the past review comment requesting validation before using the filter.
36-41: LGTM! Appropriate debug configuration for eBPF tracing.The DebugNodeParams configuration is correct:
- Uses a specific, versioned pwru image
- Mounts
/sys/kernel/debugwhich is required for eBPF-based packet tracing
33-34: The code at line 34 is correct; no changes needed.The
addIfNotEmptymethod signature isfunc (cb *commandBuilder) addIfNotEmpty(value string, args ...string), which checks ifvalueis non-empty and appendsargs...to the command if so. The callcmd.addIfNotEmpty(in.PcapFilter, in.PcapFilter)correctly adds the PcapFilter value if non-empty, following the same pattern used elsewhere in the codebase (e.g.,cmd.addIfNotEmpty(in.BPFFilter, in.BPFFilter)in tcpdump.go andcmd.addIfNotEmpty(in.Options, in.Options)in ip.go).pkg/external_tools/mcp/tcpdump.go (4)
13-18: LGTM!The constants are well-chosen: default packet count of 100 and snaplen of 96 bytes are reasonable for typical captures, while the maximums prevent excessive resource usage.
22-43: LGTM!The validation logic is thorough and well-structured: input validation occurs first, sensible defaults are applied for zero values, and bounds checking prevents abuse. Error propagation is clean.
45-59: LGTM!The command construction correctly follows tcpdump syntax: the interface flag pair (
-i <interface>) and bare BPF filter are added conditionally, and output format handling appropriately selects verbose mode for text or stdout redirection for pcap.
61-85: LGTM!The dispatch logic correctly routes execution to node or pod targets with appropriate parameter mapping. Error handling is consistent, and the default case provides a clear error message for invalid target types.
pkg/external_tools/types/manifest.go (1)
1-39: LGTM!The type definitions are clean and well-structured:
TcpdumpParamsflexibly supports both node and pod targets,PwruParamsfocuses on node-level kernel tracing, andCommandResultappropriately captures execution results. JSON tags and omitempty usage are correct.pkg/external_tools/mcp/utils.go (4)
16-46: LGTM!The execution helpers are well-implemented:
runDebugNodevalidates required node parameters,runExecPodsensibly defaults namespace to "default" when unspecified, and both provide clear error messages with clean delegation to the Kubernetes layer.
48-83: LGTM!The
commandBuilderprovides a clean fluent interface for constructing command arguments: the conditional methods (addIf,addIfNotEmpty) reduce boilerplate, and the unexported type is appropriately scoped for internal use.
85-118: LGTM!The validation functions provide solid security boundaries:
validateInterfacecorrectly enforces the 15-character Linux interface name limit and pattern matching, whilevalidatePacketFilterdefensively blocks shell metacharacters. The length limits prevent resource abuse.
120-140: LGTM!The utility functions are straightforward and correct:
validateIntMaxprovides clear bounds checking with optional unit formatting, andstringWithDefaulthandles empty-string defaulting cleanly.
Meina-rh
force-pushed
the
external_tools
branch
from
bf11863 to
92aea73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/kubernetes/client/nodes.go (1)
121-132: Timeout message is inconsistent with actual timeout value.The error message states "5 minutes" but
PollUntilContextTimeoutis configured withtime.Minute*1(1 minute).🐛 Proposed fix
if err != nil { cleanupPod() - return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 5 minutes: %w", err) + return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 1 minute: %w", err) }
♻️ Duplicate comments (1)
pkg/kubernetes/mcp/nodes.go (1)
35-43: Character whitelist may reject legitimate Kubernetes paths.The whitelist allows only
a-z,A-Z,0-9,/,-,_,.but Kubernetes volume paths can contain~(e.g.,kubernetes.io~secretused in projected volumes). Consider adding~to the allowed characters.♻️ Proposed fix
// Check for dangerous characters (null bytes, control characters, shell special characters) for i, r := range path { - // Allow alphanumeric, /, -, _, . + // Allow alphanumeric, /, -, _, ., ~ if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || - r == '/' || r == '-' || r == '_' || r == '.' { + r == '/' || r == '-' || r == '_' || r == '.' || r == '~' { continue } return fmt.Errorf("%s contains unsafe character at position %d: %c (U+%04X)", pathType, i, r, r) }
🧹 Nitpick comments (1)
pkg/nettools/mcp/utils_test.go (1)
468-497: Consider simplifying the length validation test cases.The workaround at lines 483-490 replaces null bytes with valid characters, but this adds unnecessary complexity. The past review suggested a cleaner approach that directly defines valid character strings in the test cases.
♻️ Proposed simplification
// Invalid filters - length validation { name: "filter at max length (1024 chars)", - filter: string(make([]byte, 1024)), + filter: strings.Repeat("a", 1024), wantError: false, }, { name: "filter exceeds max length (1025 chars)", - filter: string(make([]byte, 1025)), + filter: strings.Repeat("a", 1025), wantError: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // For the length tests, fill with valid characters - filter := tt.filter - if len(filter) >= 1024 { - validFilter := make([]byte, len(filter)) - for i := range validFilter { - validFilter[i] = 'a' - } - filter = string(validFilter) - } - - err := validatePacketFilter(filter) + err := validatePacketFilter(tt.filter)Note: You'll need to add
"strings"to the imports.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/ovnk-mcp-server/main.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.gopkg/nettools/mcp/mcp.gopkg/nettools/mcp/pwru.gopkg/nettools/mcp/tcpdump.gopkg/nettools/mcp/utils.gopkg/nettools/mcp/utils_test.gopkg/nettools/types/manifest.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/nettools/mcp/utils.go
- pkg/nettools/mcp/tcpdump.go
- cmd/ovnk-mcp-server/main.go
- pkg/nettools/types/manifest.go
- pkg/nettools/mcp/pwru.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/kubernetes/client/nodes.go (1)
pkg/kubernetes/client/client.go (1)
OVNKMCPServerClientSet(13-20)
pkg/kubernetes/mcp/nodes.go (3)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-12)pkg/kubernetes/mcp/mcp.go (1)
MCPServer(15-17)pkg/kubernetes/types/nodes.go (2)
DebugNodeParams(4-10)DebugNodeResult(13-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (11)
pkg/kubernetes/types/nodes.go (1)
3-10: LGTM!The struct properly defines the new optional
HostPathandMountPathfields with appropriateomitemptyJSON tags. Path validation is correctly delegated to the MCP layer (pkg/kubernetes/mcp/nodes.go), keeping the type definition clean.pkg/kubernetes/mcp/nodes.go (1)
48-65: LGTM!The validation flow correctly validates both paths before invoking the client. Error handling properly returns early on validation failures.
pkg/kubernetes/client/nodes.go (3)
15-33: LGTM!The
DebugNodemethod correctly delegates tocreatePodwith the new path parameters and properly handles cleanup via defer.
35-56: LGTM!The defaulting logic and conditional
HOSTenvironment variable handling are correctly implemented. TheHOSTenv var now properly uses the dynamicmountPathvalue as suggested in prior reviews.
58-104: LGTM!The pod specification correctly uses the dynamic
hostPathandmountPathvalues for volume configuration. The privileged debug pod setup with host network, PID, and IPC namespaces is appropriate for node debugging purposes.pkg/nettools/mcp/utils_test.go (4)
8-99: LGTM!Good boundary testing for
validateIntMaxcovering zero, positive values at/near max, exceeds max, and negative values.
101-142: LGTM!Proper coverage of the default value fallback behavior, including the edge case where whitespace is treated as a non-empty value.
144-297: LGTM!Comprehensive test coverage for interface name validation including the Linux
IFNAMSIZlimit (15 chars) and shell metacharacter injection prevention.
500-577: LGTM!Good coverage of the fluent command builder API including conditional additions and method chaining.
pkg/nettools/mcp/mcp.go (2)
8-20: LGTM!Clean struct design wrapping the Kubernetes MCP server with the node image configuration.
24-51: LGTM!Comprehensive tool description with clear parameter documentation and practical examples for node-level and pod-level captures.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Meina-rh
force-pushed
the
external_tools
branch
from
92aea73 to
fe5ebe7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/kubernetes/client/nodes.go (1)
120-132: Fix inconsistent timeout value in error message.The error message states "5 minutes" but the actual timeout is 1 minute (
time.Minute*1).🐛 Proposed fix
if err != nil { cleanupPod() - return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 5 minutes: %w", err) + return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 1 minute: %w", err) }
♻️ Duplicate comments (3)
pkg/kubernetes/mcp/nodes.go (1)
35-43: Character allowlist should include~to allow legitimate kubelet paths.Kubernetes kubelet uses
~as a separator in volume plugin directory names (e.g.,kubernetes.io~secret,kubernetes.io~empty-dir). The current allowlist rejects this character, which would fail validation if paths containing these standard kubelet patterns are passed tovalidatePath.Proposed fix
for i, r := range path { - // Allow alphanumeric, /, -, _, . + // Allow alphanumeric, /, -, _, ., ~ if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || - r == '/' || r == '-' || r == '_' || r == '.' { + r == '/' || r == '-' || r == '_' || r == '.' || r == '~' { continue }pkg/nettools/mcp/mcp.go (1)
52-75: Image configuration conflict between tcpdump and pwru remains unresolved.This concern was raised in a previous review: tcpdump requires
nicolaka/netshoot:v0.13while pwru requiresdocker.io/cilium/pwru:v1.0.10. With a singlenodeImagefield, users cannot run both tools with their correct images in the same session.Consider:
- Separate image fields (
tcpdumpImage,pwruImage) in MCPServer- Per-tool image parameters in the tool input
- Documentation clarifying this limitation
pkg/nettools/mcp/tcpdump.go (1)
55-63: Binary pcap output will be corrupted when stored in string field.When
outputFormatis "pcap",tcpdump -w -outputs raw binary pcap data. Storing this inCommandResult.Output(a string field) will corrupt the binary data due to UTF-8 encoding/decoding.Consider:
- Base64-encode the pcap output and document the encoding
- Use MCP's blob/binary content mechanism
- Temporarily restrict to text output until binary handling is implemented
🧹 Nitpick comments (3)
cmd/ovnk-mcp-server/main.go (1)
144-144: Consider documenting that--node-imageis required for network tools.The empty default means the pwru tool will fail at runtime if not provided. This is acceptable for flexibility, but adding a note in the help text like
"Container image for node operations (required for pwru)"would improve discoverability.pkg/nettools/mcp/utils_test.go (1)
468-490: Simplify the length validation tests by using valid characters directly.The test cases at lines 469-476 use
make([]byte, N)which creates null bytes, then lines 483-490 replace the filter with 'a' characters. This indirection makes the test intent less clear. Consider defining the test data with valid characters directly.Suggested simplification
// Invalid filters - length validation { name: "filter at max length (1024 chars)", - filter: string(make([]byte, 1024)), + filter: strings.Repeat("a", 1024), wantError: false, }, { name: "filter exceeds max length (1025 chars)", - filter: string(make([]byte, 1025)), + filter: strings.Repeat("a", 1025), wantError: true, },Then remove lines 482-490 (the replacement logic).
pkg/nettools/mcp/utils.go (1)
100-116: Good security validation for shell injection prevention.The blocklist covers critical shell metacharacters. Consider whether
>and<should also be blocked to prevent potential redirect injection if the command is ever constructed with shell interpretation.♻️ Optional: extend dangerous pattern list
- dangerous := []string{";", "|", "&", "`", "$", "$(", "\n", "\x00"} + dangerous := []string{";", "|", "&", "`", "$", "$(", "\n", "\x00", ">", "<"}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/ovnk-mcp-server/main.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.gopkg/nettools/mcp/mcp.gopkg/nettools/mcp/pwru.gopkg/nettools/mcp/tcpdump.gopkg/nettools/mcp/utils.gopkg/nettools/mcp/utils_test.gopkg/nettools/types/manifest.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/kubernetes/types/nodes.go
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/kubernetes/client/nodes.go (1)
pkg/kubernetes/client/client.go (1)
OVNKMCPServerClientSet(13-20)
pkg/nettools/mcp/pwru.go (3)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-12)pkg/nettools/types/manifest.go (2)
PwruParams(31-36)CommandResult(39-43)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)
pkg/nettools/mcp/tcpdump.go (5)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-12)pkg/nettools/types/manifest.go (2)
TcpdumpParams(10-27)CommandResult(39-43)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)pkg/kubernetes/types/pods.go (1)
ExecPodParams(17-21)pkg/kubernetes/types/common.go (1)
NamespacedNameParams(37-40)
cmd/ovnk-mcp-server/main.go (1)
pkg/nettools/mcp/mcp.go (1)
NewMCPServer(15-20)
pkg/nettools/mcp/utils.go (5)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-12)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)pkg/nettools/types/manifest.go (1)
CommandResult(39-43)pkg/kubernetes/types/pods.go (1)
ExecPodParams(17-21)pkg/kubernetes/types/common.go (1)
NamespacedNameParams(37-40)
pkg/kubernetes/mcp/nodes.go (1)
pkg/kubernetes/types/nodes.go (2)
DebugNodeParams(4-10)DebugNodeResult(13-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (23)
pkg/nettools/types/manifest.go (1)
1-43: LGTM! Well-structured type definitions.The data structures are cleanly organized with appropriate JSON tags and embedding for shared fields. The separation between
TcpdumpParams(supporting both node and pod targets) andPwruParams(node-only) correctly reflects the different capabilities of each tool.cmd/ovnk-mcp-server/main.go (1)
46-49: LGTM! Network tools server integration follows established patterns.The wiring correctly follows the same structure as other MCP servers (kernel, OVS). The
nodeImageis passed through and validated at tool execution time inpwru.go.pkg/nettools/mcp/utils_test.go (3)
1-99: LGTM! Comprehensive validation tests with good boundary coverage.The
TestValidateIntMaxtests cover all important boundary cases including zero, minimum valid, below max, equals max, just over max, and negative values.
144-297: LGTM! Thorough interface validation testing.Excellent coverage of valid interface patterns (eth0, lo, br-int, veth, wlan) and security-sensitive invalid inputs (injection characters, length limits, control characters).
500-577: LGTM! Well-designed command builder tests.The fluent API testing covers all builder methods with clear expectations for chained operations.
pkg/kubernetes/mcp/nodes.go (1)
48-65: LGTM! Path validation properly integrated before debug node execution.The validation correctly occurs before the client call, and the error handling appropriately returns early with empty results on validation failures.
pkg/nettools/mcp/pwru.go (2)
13-16: LGTM! Sensible defaults and limits for output lines.The constants provide reasonable bounds: 100 lines default keeps output manageable while 1000 max prevents excessive resource usage.
21-53: LGTM! Comprehensive validation before execution.The method properly validates all inputs (node image, output limit, BPF filter) before constructing the command. The hardcoded paths for
/sys/kernel/debugare appropriate since pwru requires kernel debug filesystem access.pkg/nettools/mcp/utils.go (7)
17-29: LGTM on validation logic.The validation correctly ensures required fields are present before delegating to the Kubernetes MCP server. The function appropriately returns only stdout in the CommandResult.
32-44: LGTM!Good practice defaulting the namespace to "default" when not specified, aligning with Kubernetes conventions.
46-81: LGTM!Clean fluent builder pattern implementation for command construction. The methods are intuitive and enable readable command building.
83-98: LGTM!The validation correctly enforces Linux interface naming constraints (max 15 characters, valid character set) and properly handles the special "any" value used by tcpdump.
118-130: LGTM!The validation correctly enforces both non-negativity and the upper bound, with clear error messages.
132-138: LGTM!Simple and useful utility for defaulting empty strings.
1-12: All files inpkg/nettools/mcp/consistently usepackage nettools, but this deviates from Go convention.The package name should match the directory name. Either rename the package to
package mcpor rename the directory to align with the package semantics. This affects all files in the directory (mcp.go, pwru.go, tcpdump.go, utils.go, utils_test.go), not just utils.go.pkg/nettools/mcp/mcp.go (2)
8-20: LGTM on struct and constructor.The MCPServer struct cleanly encapsulates the Kubernetes MCP server dependency and node image configuration.
24-51: LGTM on tcpdump tool registration.The tool description is comprehensive with clear parameter documentation and practical examples.
pkg/kubernetes/client/nodes.go (3)
15-33: LGTM on DebugNode signature expansion.The method now properly forwards hostPath and mountPath parameters, enabling configurable volume mounting for different tool requirements.
35-56: LGTM on defaulting and environment variable setup.The HOST environment variable now correctly uses the actual
mountPathvalue, addressing the previous review feedback.
75-101: LGTM on pod spec configuration.The volume and mount configuration correctly uses the dynamic
hostPathandmountPathvariables.pkg/nettools/mcp/tcpdump.go (3)
13-22: LGTM on constants definition.The constants are well-documented with clear rationale for the default and maximum values. MaxSnaplen at 1500 (standard Ethernet MTU) is appropriate.
25-47: LGTM on validation logic.The validation correctly applies defaults and validates bounds. The
validateIntMaxfunction handles negative value rejection as confirmed in previous reviews.
65-94: LGTM on target type dispatch.The dispatch logic correctly validates
nodeImagefor node targets and delegates validation of other required fields torunDebugNodeandrunExecPod. The error handling is consistent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Meina-rh
force-pushed
the
external_tools
branch
from
fe5ebe7 to
7ef539d
Compare
|
@pperiyasamy I think I've resolved all comments. could you help look again? thanks |
cmd/ovnk-mcp-server/main.go
Outdated
| flag.StringVar(&cfg.Transport, "transport", "stdio", "Transport to use: stdio or http") | ||
| flag.StringVar(&cfg.Port, "port", "8080", "Port to use") | ||
| flag.StringVar(&cfg.Kubernetes.Kubeconfig, "kubeconfig", "", "Path to the kubeconfig file") | ||
| flag.StringVar(&cfg.NodeImage, "node-image", "", "Container image for node operations") |
There was a problem hiding this comment.
don't you need two different container images for tcpdump and pwru commands ? I think this is what you had earlier.
why don't we initialize these images with default ones ? the empty string is passed now.
There was a problem hiding this comment.
@pperiyasamy do you know if LLM model will request these tools at the same time and use the same configuration? I just thought if we describe in detail, LLM model will not set the same images
There was a problem hiding this comment.
why don't we initialize these images with default ones ? the empty string is passed now.
I added empty string and set default in tcpdump.go and pwru.go respectively
@Meina-rh can you also address coderabbit's duplicate and nitpick comments posted here ? |
Meina-rh
force-pushed
the
external_tools
branch
from
7ef539d to
cb75b36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/nettools/mcp/mcp.go`:
- Around line 22-74: The tool descriptions in AddTools for the tcpdump and pwru
mcp.Tool entries should explicitly state that the --node-image flag is global
and overrides the per-tool defaults; update the Description string for the
tcpdump tool (Name: "tcpdump") and the pwru tool (Name: "pwru") to add a
sentence like: "The node image can be configured via the --node-image flag
(applies to all tools) or will default to [tool-specific default image]." Ensure
this exact clarification is added without changing any other behavior.
🧹 Nitpick comments (5)
pkg/nettools/mcp/utils_test.go (1)
483-491: Remove redundant filter replacement logic.Lines 471 and 476 already use
strings.Repeat("a", N)to create valid-character filters for length testing. The replacement logic at lines 485-491 is now redundant and can be removed to simplify the test.♻️ Suggested cleanup
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // For the length tests, fill with valid characters - filter := tt.filter - if len(filter) >= 1024 { - validFilter := make([]byte, len(filter)) - for i := range validFilter { - validFilter[i] = 'a' - } - filter = string(validFilter) - } - - err := validatePacketFilter(filter) + err := validatePacketFilter(tt.filter) if (err != nil) != tt.wantError { - t.Errorf("validatePacketFilter(%q) error = %v, wantError %v", filter, err, tt.wantError) + t.Errorf("validatePacketFilter(%q) error = %v, wantError %v", tt.filter, err, tt.wantError) } }) }pkg/nettools/mcp/utils.go (2)
16-29: Consider returning stderr for debugging purposes.
runDebugNodereturns onlyoutput.Stdout(line 28), discardingoutput.Stderr. For network debugging tools, stderr often contains valuable diagnostic information (warnings, connection details). Consider including stderr in the result or logging it.♻️ Optional: Include stderr in CommandResult
If the
CommandResulttype supports it, consider:- return types.CommandResult{Output: output.Stdout}, nil + return types.CommandResult{Output: output.Stdout, Stderr: output.Stderr}, nilOr at minimum, log stderr if non-empty to aid debugging.
31-44: Same consideration for runExecPod—stderr is discarded.Similar to
runDebugNode, this function only returns stdout. For consistency and debugging utility, consider preserving stderr.pkg/nettools/mcp/pwru.go (1)
18-55: Implementation looks correct with proper validation and defaults.The method correctly:
- Falls back to a secure default image when
s.nodeImageis empty- Validates output limit lines (both default assignment and max validation)
- Validates BPF filter before use
- Mounts the required
/sys/kernel/debugpath for eBPF tracingOne minor observation on line 40:
cmd.addIfNotEmpty(in.BPFFilter, in.BPFFilter)passes the same value twice. This appears intentional (condition and value are the same), but consider whether a single-argument variant likecmd.addIfNotEmpty(in.BPFFilter)would be cleaner if the helper supports it.pkg/nettools/types/manifest.go (1)
37-42: CommandResult provides a clean result structure.Minor consideration:
ExitCodeusesomitempty, so a successful exit (code 0) won't be serialized in JSON output. This is typically fine, but if consumers need to distinguish between "exit code not set" vs "exit code is 0", consider removingomitemptyfromExitCodeto always include it explicitly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/ovnk-mcp-server/main.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.gopkg/nettools/mcp/mcp.gopkg/nettools/mcp/pwru.gopkg/nettools/mcp/tcpdump.gopkg/nettools/mcp/utils.gopkg/nettools/mcp/utils_test.gopkg/nettools/types/manifest.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/ovnk-mcp-server/main.go
- pkg/kubernetes/client/nodes.go
- pkg/nettools/mcp/tcpdump.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/kubernetes/mcp/nodes.go (3)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-12)pkg/kubernetes/mcp/mcp.go (1)
MCPServer(15-17)pkg/kubernetes/types/nodes.go (2)
DebugNodeParams(4-10)DebugNodeResult(13-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (17)
pkg/kubernetes/types/nodes.go (1)
5-9: LGTM!The struct properly defines the optional
HostPathandMountPathfields with appropriate JSON tags andomitemptyfor optional handling. This aligns with the requested simplification from previous review comments.pkg/kubernetes/mcp/nodes.go (2)
13-46: LGTM!The path validation implementation correctly addresses the previous review feedback:
- Uses the original
path(notfilepath.Clean(path)) for the ".." traversal check at line 29, which properly catches traversal attempts.- Includes
~in the allowed characters (line 39) to support Kubernetes volume paths likekubernetes.io~secret.- Validates absolute paths and rejects dangerous characters.
48-65: LGTM!The
DebugNodemethod properly validates bothHostPathandMountPathbefore delegating to the underlying client, ensuring security checks are enforced at the MCP layer.pkg/nettools/mcp/utils_test.go (4)
9-100: Good test coverage for validation utilities.The
TestValidateIntMaxfunction provides comprehensive boundary testing including zero, boundary values, over-max, and negative value cases. Well-structured table-driven tests.
145-298: Thorough interface validation tests.Excellent coverage of valid interface names (eth0, lo, br-int, vlan interfaces) and invalid patterns (too long, shell metacharacters, leading special chars). The 15-character limit aligns with Linux
IFNAMSIZconstraints.
300-479: Comprehensive packet filter validation tests.Good coverage of valid BPF filter expressions and command injection attempts (semicolon, pipe, backtick,
$(), newline, null byte). This provides defense against shell injection attacks.
501-578: LGTM!The
TestCommandBuildertests validate the fluent API methods (add,addIf,addIfNotEmpty) and chained operations correctly.pkg/nettools/mcp/mcp.go (1)
8-20: LGTM!The
MCPServerstruct and constructor are straightforward. ThenodeImagefield provides a configurable default for debug pods.pkg/nettools/mcp/utils.go (5)
14-14: LGTM!The regex
^[a-zA-Z0-9][a-zA-Z0-9._-]*$correctly validates Linux interface names—must start with alphanumeric and can contain dots, underscores, and hyphens.
46-81: LGTM!The
commandBuilderprovides a clean fluent API for constructing commands. TheaddIfandaddIfNotEmptymethods are useful for conditional argument building.
83-98: LGTM!The interface validation correctly:
- Allows empty string (no interface specified)
- Allows "any" as a special case for tcpdump
- Enforces 15-character limit (
IFNAMSIZ - 1)- Validates against a safe character pattern
100-116: LGTM!The packet filter validation provides good defense against shell injection by blocking dangerous characters (
;,|,&,`,$,$(,\n,\x00,>,<). The 1024-character limit is reasonable for BPF filters.
118-138: LGTM!The
validateIntMaxandstringWithDefaulthelper functions are straightforward and well-implemented.pkg/nettools/mcp/pwru.go (1)
1-16: LGTM on package setup and constants.The imports are appropriate, and the constants establish sensible defaults and limits for output lines. The hardcoded default pwru image version (
v1.0.10) is acceptable per the discussion in prior comments about security concerns with arbitrary images.pkg/nettools/types/manifest.go (3)
3-6: Base params type is well-designed.
BaseNetworkDiagParamsprovides a clean abstraction for the shared BPF filter parameter, enabling consistent validation across both tcpdump and pwru tools.
8-26: TcpdumpParams struct is well-organized.The type correctly:
- Embeds
BaseNetworkDiagParamsfor shared BPF filter- Separates node-specific and pod-specific fields with clear comments
- Uses
omitemptyappropriately for optional fields- Makes
TargetTyperequired to drive conditional validation logicThe conditional requirement of
NodeNamevs pod fields (PodName,PodNamespace) based onTargetTypeshould be validated at runtime in the handler—ensure that validation exists in the tcpdump implementation.
28-35: PwruParams correctly enforces node-level targeting.
NodeNameis appropriately required (noomitempty) since pwru needs access to/sys/kernel/debugand only operates at the node level. This aligns with the implementation inpwru.go.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Meina-rh
force-pushed
the
external_tools
branch
from
cb75b36 to
54c1a75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/kubernetes/client/nodes.go (1)
120-132: Fix: Error message timeout does not match actual timeout value.The error message states "5 minutes" but the actual timeout is 1 minute (
time.Minute*1on line 121).🐛 Proposed fix
- return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 5 minutes: %w", err) + return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 1 minute: %w", err)
🧹 Nitpick comments (5)
cmd/ovnk-mcp-server/main.go (1)
144-144: Consider documenting fallback behavior in flag help text.The empty default for
--node-imagerelies on tool-specific fallbacks (netshoot for tcpdump, cilium/pwru for pwru). Consider expanding the description to clarify this, e.g.,"Container image for node operations (defaults to tool-specific images if empty)".Suggested improvement
- flag.StringVar(&cfg.NodeImage, "node-image", "", "Container image for node operations") + flag.StringVar(&cfg.NodeImage, "node-image", "", "Container image for node operations (defaults to tool-specific images if empty)")pkg/nettools/mcp/mcp.go (1)
22-74: Tool registrations look good.The tool descriptions are comprehensive with:
- Clear parameter documentation
- Helpful examples for common use cases
- Clarification that
--node-imageoverrides tool-specific defaults (addressing prior review feedback)Minor note: Line 33 and 62 are missing a period at the end ("The image must contain the tcpdump utility" / "The image must contain the pwru utility").
pkg/nettools/mcp/utils.go (3)
17-29: Consider including stderr in the result for diagnostic completeness.The function discards
stderrfrom the debug node execution. For network diagnostic tools like tcpdump, error output may contain valuable information (warnings, interface issues, permission errors).♻️ Proposed change
- return types.CommandResult{Output: output.Stdout}, nil + result := types.CommandResult{Output: output.Stdout} + if output.Stderr != "" { + result.Error = output.Stderr + } + return result, nil
31-44: Same stderr consideration applies here.For consistency with
runDebugNode, consider capturing stderr in theErrorfield ofCommandResult.
100-116: Consider adding backslash to the dangerous characters list.The dangerous pattern list is comprehensive for shell injection prevention, but
\(backslash) is missing and could be used for escape sequence attacks depending on how the filter is passed to the shell.♻️ Proposed addition
- dangerous := []string{";", "|", "&", "`", "$", "$(", "\n", "\x00", ">", "<"} + dangerous := []string{";", "|", "&", "`", "$", "$(", "\n", "\x00", ">", "<", "\\"}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/ovnk-mcp-server/main.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.gopkg/nettools/mcp/mcp.gopkg/nettools/mcp/pwru.gopkg/nettools/mcp/tcpdump.gopkg/nettools/mcp/utils.gopkg/nettools/mcp/utils_test.gopkg/nettools/types/manifest.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/nettools/mcp/pwru.go
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/nettools/mcp/utils.go (5)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-12)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)pkg/nettools/types/manifest.go (1)
CommandResult(38-42)pkg/kubernetes/types/pods.go (1)
ExecPodParams(17-21)pkg/kubernetes/types/common.go (1)
NamespacedNameParams(37-40)
pkg/nettools/mcp/tcpdump.go (5)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-12)pkg/nettools/types/manifest.go (2)
TcpdumpParams(10-26)CommandResult(38-42)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)pkg/kubernetes/types/pods.go (1)
ExecPodParams(17-21)pkg/kubernetes/types/common.go (1)
NamespacedNameParams(37-40)
pkg/kubernetes/client/nodes.go (1)
pkg/kubernetes/client/client.go (1)
OVNKMCPServerClientSet(13-20)
cmd/ovnk-mcp-server/main.go (1)
pkg/nettools/mcp/mcp.go (1)
NewMCPServer(15-20)
pkg/kubernetes/mcp/nodes.go (3)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-12)pkg/kubernetes/mcp/mcp.go (1)
MCPServer(15-17)pkg/kubernetes/types/nodes.go (2)
DebugNodeParams(4-10)DebugNodeResult(13-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (22)
pkg/kubernetes/types/nodes.go (1)
5-9: LGTM!The
HostPathandMountPathfields are correctly added withomitemptytags, allowing optional configuration while defaulting behavior is handled elsewhere. This aligns with the simplified approach requested in past reviews.cmd/ovnk-mcp-server/main.go (1)
46-49: LGTM!The nettools MCP server initialization follows the established pattern used for other tool servers (OVS, kernel). The
nodeImageparameter is correctly passed through from the CLI flag.pkg/kubernetes/mcp/nodes.go (2)
18-46: Path validation implementation looks correct.The
validatePathfunction properly:
- Allows empty paths (defaults handled downstream)
- Requires absolute paths
- Checks the original path for
..elements (correctly addressing the prior review feedback aboutfilepath.Cleanmasking traversal)- Validates characters against a reasonable allowlist including
~for Kubernetes-style paths
50-59: LGTM!Path validation is correctly applied before invoking the underlying client, ensuring invalid paths are rejected early with clear error messages.
pkg/nettools/mcp/mcp.go (1)
8-20: LGTM!The
MCPServerstruct and constructor are well-designed, holding a reference to the Kubernetes MCP server for node operations and the optionalnodeImageconfiguration.pkg/nettools/mcp/utils_test.go (3)
9-100: Good boundary testing for integer validation.Tests cover zero, minimum, boundary (equal to max, just over max), and negative values with clear naming.
300-489: Comprehensive security testing for packet filters.Excellent coverage of:
- Valid BPF filter expressions (protocols, hosts, ports, operators)
- Injection attempts (semicolon, pipe, backtick, command substitution, null bytes)
- Length boundary validation (1024 max)
The prior review feedback about using
strings.Repeat()for length tests has been correctly addressed.
491-568: LGTM!The
commandBuildertests verify the fluent API works correctly for building commands with conditional arguments.pkg/nettools/mcp/utils.go (4)
1-12: LGTM!The imports and package structure are well-organized. The utility functions are appropriately scoped as unexported helpers for the MCP server.
46-81: LGTM!The fluent command builder pattern is well-implemented and provides a clean API for constructing command arguments.
83-98: LGTM!The interface validation correctly enforces the Linux
IFNAMSIZlimit of 15 characters and uses an appropriate regex pattern. The special case for "any" (tcpdump wildcard) is properly handled.
118-138: LGTM!Both utility functions are correct.
validateIntMaxproperly validates both negative values and upper bounds, whilestringWithDefaultprovides a clean null-coalescing pattern.pkg/kubernetes/client/nodes.go (2)
15-33: LGTM!The signature expansion to accept
hostPathandmountPathparameters provides good flexibility for the debug node functionality while maintaining backward compatibility through defaults.
35-56: The HOST environment variable fix is correctly implemented.The
HOSTenvironment variable now correctly uses themountPathvalue (line 53) instead of the previously hardcoded"/host". This ensures tools relying on theHOSTvariable will look in the correct location when custom mount paths are used.pkg/nettools/mcp/tcpdump.go (4)
13-23: LGTM!The constants are well-documented and use sensible defaults:
DefaultSnaplen = 96captures headers for diagnosticsMaxSnaplen = 1500aligns with standard Ethernet MTUDefaultTcpdumpImageuses a pinned version for security (as discussed in past reviews)
26-48: LGTM!The validation flow is correct: interface and BPF filter validation first, then defaults are applied before bounds checking. The
validateIntMaxfunction handles negative value checks as confirmed in past reviews.
50-54: LGTM!The tcpdump command is correctly constructed:
-ndisables DNS lookups for faster output-venables verbose mode-sand-cset snaplen and packet count- Interface and BPF filter are conditionally added
56-86: LGTM with minor observation.The dispatch logic correctly handles both node and pod target types with appropriate fallback for the node image. The validation for required fields (node name, pod name) is handled in
runDebugNodeandrunExecPodas confirmed in past reviews.Note:
HostPathandMountPathare not exposed inTcpdumpParams, so they use defaults. This is likely intentional since tcpdump primarily needs network access rather than filesystem access.pkg/nettools/types/manifest.go (4)
1-6: LGTM!The
BaseNetworkDiagParamstype provides a clean abstraction for shared parameters across network diagnostic tools.
8-26: LGTM!The
TcpdumpParamstype is well-structured with clear separation between node-specific and pod-specific fields. TheTargetTypefield correctly lacksomitemptysince it's required to determine the execution path.
28-35: LGTM!The
PwruParamstype correctly requiresNodeName(noomitempty) since pwru is an eBPF-based tool that only runs at the node level, unlike tcpdump which supports both node and pod targets.
37-42: LGTM!The
CommandResulttype provides a clean structure for command execution results. TheOutputfield appropriately lacksomitemptyto always be present in responses, whileExitCodeandErrorare optional.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Meina-rh
force-pushed
the
external_tools
branch
from
54c1a75 to
474f2db
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/kubernetes/client/nodes.go (1)
121-132: Timeout error message is inconsistent with actual timeout value.The error message on line 131 states "5 minutes" but the actual timeout configured on line 121 is
time.Minute*1(1 minute).Proposed fix
if err != nil { cleanupPod() - return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 5 minutes: %w", err) + return "", nil, fmt.Errorf("debug pod did not reach running state within timeout of 1 minute: %w", err) }
🧹 Nitpick comments (4)
pkg/nettools/mcp/mcp.go (1)
15-22: Consider adding nil check for k8sMcpServer parameter.If
k8sMcpServerisnil, theMCPServerwill be created but will panic when tools try to use it. While the current call site inmain.goalways provides a valid server, defensive validation could prevent subtle bugs if this constructor is reused.Proposed defensive check
// NewMCPServer creates a new MCP server instance func NewMCPServer(k8sMcpServer *kubernetesmcp.MCPServer, pwruImage, tcpdumpImage string) *MCPServer { + if k8sMcpServer == nil { + panic("k8sMcpServer cannot be nil") + } return &MCPServer{ k8sMcpServer: k8sMcpServer, pwruImage: pwruImage, tcpdumpImage: tcpdumpImage, } }cmd/ovnk-mcp-server/main.go (1)
145-146: Update the netshoot container image to a more recent version.The
docker.io/cilium/pwru:v1.0.10is the latest available version and appropriate. However,nicolaka/netshoot:v0.13is outdated; the latest tag was updated on January 9, 2026, and should be used or the pinned version should be updated to a more recent release.pkg/nettools/mcp/utils.go (1)
100-116: Consider adding single/double quotes to the dangerous patterns list.The validation blocks common shell metacharacters, but single (
') and double (") quotes can also be used for injection in certain contexts. While the current list covers the most critical vectors, adding quotes would provide defense-in-depth.Suggested improvement
func validatePacketFilter(filter string) error { if filter == "" { return nil } if len(filter) > 1024 { return fmt.Errorf("packet filter too long (max 1024 characters)") } - dangerous := []string{";", "|", "&", "`", "$", "$(", "\n", "\x00", ">", "<"} + dangerous := []string{";", "|", "&", "`", "$", "$(", "\n", "\x00", ">", "<", "'", "\""} for _, pattern := range dangerous { if strings.Contains(filter, pattern) { return fmt.Errorf("packet filter contains potentially dangerous characters") } } return nil }pkg/nettools/mcp/pwru.go (1)
34-35: Clarify command construction for BPF filter argument.The
addIfNotEmptycall adds thein.BPFFiltervalue directly as a command argument. Forpwru, the filter should be passed as a positional argument (which this does correctly), but consider adding a comment to clarify that this is intentional behavior sincepwruexpects the pcap filter as the final positional argument(s).Suggested clarity improvement
cmd := newCommand("pwru", "--output-limit-lines", strconv.Itoa(outputLimitLines)) + // pwru accepts pcap filter as positional argument(s) cmd.addIfNotEmpty(in.BPFFilter, in.BPFFilter)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/ovnk-mcp-server/main.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.gopkg/nettools/mcp/mcp.gopkg/nettools/mcp/pwru.gopkg/nettools/mcp/tcpdump.gopkg/nettools/mcp/utils.gopkg/nettools/mcp/utils_test.gopkg/nettools/types/manifest.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/nettools/mcp/utils_test.go
- pkg/nettools/types/manifest.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/kubernetes/mcp/nodes.go (2)
pkg/kubernetes/mcp/mcp.go (1)
MCPServer(15-17)pkg/kubernetes/types/nodes.go (2)
DebugNodeParams(4-10)DebugNodeResult(13-16)
pkg/kubernetes/client/nodes.go (1)
pkg/kubernetes/client/client.go (1)
OVNKMCPServerClientSet(13-20)
pkg/nettools/mcp/pwru.go (3)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-13)pkg/nettools/types/manifest.go (2)
PwruParams(30-35)CommandResult(38-42)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (21)
pkg/kubernetes/types/nodes.go (1)
5-9: LGTM!The
DebugNodeParamsstruct is well-defined with appropriate JSON tags. The optionalHostPathandMountPathfields useomitemptycorrectly, and validation is properly delegated to the MCP layer as indicated in the AI summary.pkg/kubernetes/client/nodes.go (2)
39-56: Default values and environment variable handling look correct.The defaulting logic properly handles empty paths, and the
HOSTenvironment variable now correctly uses themountPathvalue (addressing the previous review feedback).
75-101: Pod volume configuration is well-structured.The pod spec correctly uses the dynamic
hostPathandmountPathvalues for the volume source and container mount, enabling flexible host filesystem access for debug operations.pkg/kubernetes/mcp/nodes.go (2)
18-46: Path validation implementation addresses previous feedback correctly.The traversal check now operates on the original path (not
filepath.Clean'd), correctly detecting explicit..elements. The character whitelist appropriately includes~for Kubernetes-style paths likekubernetes.io~secret.
49-64: LGTM!The
DebugNodemethod properly validates both paths before delegating to the client, following a fail-fast pattern that prevents pod creation with invalid paths.cmd/ovnk-mcp-server/main.go (1)
47-51: Network tools integration follows the established pattern.The nettools MCP server is properly instantiated with separate images for pwru and tcpdump, and tools are registered consistently with other MCP servers in the codebase.
pkg/nettools/mcp/mcp.go (1)
25-76: Tool descriptions are comprehensive and well-documented.Both tools have clear parameter documentation, practical examples, and correctly reference the CLI flags (
--tcpdump-image,--pwru-image) for image configuration. This addresses the previous review feedback about clarifying image configuration.pkg/nettools/mcp/utils.go (7)
1-12: LGTM!Package declaration and imports are appropriate for the utilities provided.
14-14: LGTM!The regex pattern correctly validates Linux interface names: starts with alphanumeric, followed by alphanumeric, dots, underscores, or hyphens.
17-29: LGTM!The
runDebugNodehelper properly validates required fields (NameandImage) and delegates to the Kubernetes MCP server. This centralized validation addresses concerns raised in previous reviews about empty field validation.
32-44: LGTM!The
runExecPodhelper validates the requiredNamefield and sensibly defaultsNamespaceto"default"following Kubernetes conventions.
46-81: LGTM!The fluent
commandBuilderpattern provides a clean, readable way to construct command arguments with conditional additions.
84-98: LGTM!Interface validation correctly enforces the Linux
IFNAMSIZlimit (15 characters), allows the special"any"value, and validates the character pattern.
118-138: LGTM!The
validateIntMaxfunction correctly handles both negative values and upper bounds with clear error messages. ThestringWithDefaultutility is straightforward.pkg/nettools/mcp/pwru.go (3)
1-16: LGTM!Package declaration, imports, and constants are well-defined. The
DefaultPwruImageconstant addresses previous review feedback about making the image configurable while providing a sensible default.
18-32: LGTM!The validation logic correctly handles the
outputLimitLinesparameter with proper defaulting and bounds checking viavalidateIntMax(which handles negative values as confirmed in past reviews). The BPF filter validation reuses the sharedvalidatePacketFilterhelper.
37-49: LGTM!The
DebugNodeParamsconstruction correctly configures the debug pod with:
- Node name and image from parameters
- Required
/sys/kernel/debugmount for eBPF functionality- The constructed command
The execution flow through
runDebugNodeensures proper validation of required fields.pkg/nettools/mcp/tcpdump.go (4)
1-23: LGTM!Package declaration, imports, and constants are well-documented. The snaplen defaults and limits are appropriately chosen:
DefaultSnaplen = 96captures headers while minimizing dataMaxSnaplen = 1500allows full packet capture when needed
26-48: LGTM!The validation and defaulting logic for interface, BPF filter, packet count, and snaplen is comprehensive. All concerns about negative values are addressed by the centralized
validateIntMaxfunction.
50-54: LGTM!The tcpdump command construction uses appropriate flags:
-n: Numeric output (no DNS resolution)-v: Verbose output-s: Snaplen for packet capture size-c: Packet count limit-i: Interface (conditional)- BPF filter as positional argument (conditional)
56-82: LGTM!The dispatch logic cleanly handles both target types:
"node": UsesrunDebugNodewith the configured image"pod": UsesrunExecPodwith namespace/name/containerThe field validation (node name, pod name, etc.) is correctly delegated to the helper functions as confirmed in past review discussions. The default case provides a clear error for invalid target types.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Meina-rh
force-pushed
the
external_tools
branch
from
474f2db to
23fe355
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/kubernetes/client/nodes.go`:
- Line 131: The error message returned by the function that formats the pod
timeout (the fmt.Errorf call producing "debug pod did not reach running state
within timeout of 1 minutes: %w") has a grammar mistake; change "1 minutes" to
"1 minute" in that fmt.Errorf string so the message reads "debug pod did not
reach running state within timeout of 1 minute: %w".
🧹 Nitpick comments (2)
cmd/ovnk-mcp-server/main.go (1)
145-146: Consider centralizing default image constants.The default images are defined both here in the CLI flags and in the respective tool files (
DefaultPwruImagein pwru.go,DefaultTcpdumpImagein tcpdump.go). While this works correctly, you could reference the constants from the tool packages to avoid duplication:Optional: Reference constants from tool packages
- flag.StringVar(&cfg.PwruImage, "pwru-image", "docker.io/cilium/pwru:v1.0.10", "Container image for pwru operations") - flag.StringVar(&cfg.TcpdumpImage, "tcpdump-image", "nicolaka/netshoot:v0.13", "Container image for tcpdump operations") + flag.StringVar(&cfg.PwruImage, "pwru-image", nettoolsmcp.DefaultPwruImage, "Container image for pwru operations") + flag.StringVar(&cfg.TcpdumpImage, "tcpdump-image", nettoolsmcp.DefaultTcpdumpImage, "Container image for tcpdump operations")pkg/nettools/mcp/utils_test.go (1)
1-1: Package naming violates Go conventions, but is consistent throughout the directory.All .go files in
pkg/nettools/mcp/(includingutils.go,mcp.go,pwru.go, andtcpdump.go) declarepackage nettoolsrather thanpackage mcp. While this is intentional and consistent with the rest of the codebase in that directory, it violates the Go convention that package names should match their directory names. Either accept this as a deliberate design choice for white-box testing of unexported functions, or refactor to usepackage mcpfor standard Go conventions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/ovnk-mcp-server/main.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.gopkg/nettools/mcp/mcp.gopkg/nettools/mcp/pwru.gopkg/nettools/mcp/tcpdump.gopkg/nettools/mcp/utils.gopkg/nettools/mcp/utils_test.gopkg/nettools/types/manifest.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/kubernetes/types/nodes.go
- pkg/nettools/mcp/mcp.go
- pkg/nettools/mcp/utils.go
- pkg/kubernetes/mcp/nodes.go
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/ovnk-mcp-server/main.go (2)
pkg/kubernetes/mcp/mcp.go (2)
Config(11-13)NewMCPServer(19-42)pkg/nettools/mcp/mcp.go (1)
NewMCPServer(16-22)
pkg/kubernetes/client/nodes.go (1)
pkg/kubernetes/client/client.go (1)
OVNKMCPServerClientSet(13-20)
pkg/nettools/mcp/tcpdump.go (5)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-13)pkg/nettools/types/manifest.go (2)
TcpdumpParams(10-26)CommandResult(38-42)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)pkg/kubernetes/types/pods.go (1)
ExecPodParams(17-21)pkg/kubernetes/types/common.go (1)
NamespacedNameParams(37-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (10)
pkg/kubernetes/client/nodes.go (1)
39-56: Defaulting and HOST env var logic looks correct.The defaults for
hostPathandmountPathare sensible, and theHOSTenvironment variable now correctly usesmountPathinstead of being hardcoded to"/host". This ensures tools relying on theHOSTvariable will reference the actual mount location.pkg/nettools/types/manifest.go (1)
1-42: Type definitions are well-structured.The parameter types provide clean separation between base, tcpdump-specific, and pwru-specific fields. The
CommandResultstruct appropriately captures command execution output with optional error information. The JSON tags are consistent.pkg/nettools/mcp/pwru.go (1)
18-51: Pwru implementation is well-structured.The method properly:
- Applies defaults for
outputLimitLinesand validates bounds- Validates the BPF filter before use
- Sets appropriate
HostPathandMountPathto/sys/kernel/debug(required for eBPF tracing)- Uses the configurable
s.pwruImageas discussed in prior reviewspkg/nettools/mcp/tcpdump.go (1)
25-83: Tcpdump implementation is well-structured and addresses prior feedback.The implementation:
- Validates interface and BPF filter upfront
- Applies sensible defaults for
packetCountandsnaplenwith proper bounds checking- Uses text output (
-v) instead of binary pcap, avoiding the string corruption issue raised previously- Uses configurable
s.tcpdumpImageas discussed in prior reviews- Cleanly dispatches between node and pod execution paths
- Centralizes validation for required fields (
node_name,pod_name, etc.) inrunDebugNode/runExecPodas confirmed in earlier commentscmd/ovnk-mcp-server/main.go (1)
47-50: Network tools integration follows established patterns.The
netToolsServerinitialization andAddToolscall follow the same pattern as other MCP servers (ovsServer,kernelMcpServer), maintaining consistency.pkg/nettools/mcp/utils_test.go (5)
9-100: LGTM!Comprehensive boundary testing for
validateIntMaxcovering zero, positive bounds, over-max values, and negative inputs. The table-driven structure is idiomatic and maintainable.
102-143: LGTM!Test cases cover the key scenarios including the deliberate behavior that whitespace-only strings are treated as non-empty values.
145-298: LGTM!Thorough validation test suite covering valid Linux interface naming conventions, length boundaries (IFNAMSIZ constraint), and comprehensive shell injection vectors. The security-focused test cases for special characters are well-designed.
300-489: LGTM!Comprehensive packet filter validation tests with excellent coverage of valid BPF filter syntax and security-focused injection tests. The length boundary tests correctly use
strings.Repeat()to generate valid character sequences.
491-568: LGTM!Well-structured tests for the fluent command builder API, covering all methods (
add,addIf,addIfNotEmpty) and method chaining. The test cases effectively validate both positive and negative conditional additions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Meina-rh
force-pushed
the
external_tools
branch
from
23fe355 to
5ace57e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/nettools/mcp/pwru.go`:
- Around line 1-16: Replace the hardcoded pwru image string used when
registering the flag with the DefaultPwruImage constant: locate the flag
registration that calls flag.StringVar to populate cfg.PwruImage (the call
currently passes the literal "docker.io/cilium/pwru:v1.0.10") and change it to
pass nettoolsmcp.DefaultPwruImage (using the package alias where nettools is
imported) so the default comes from the defined DefaultPwruImage constant.
🧹 Nitpick comments (1)
pkg/kubernetes/client/nodes.go (1)
112-118: Consider using a background context for cleanup.The cleanup function captures the
ctxfromcreatePod. If the context is cancelled (e.g., due to timeout during command execution), the pod deletion may fail silently. Usingcontext.Background()for cleanup ensures the pod is deleted even if the original context is cancelled.♻️ Suggested improvement
cleanupPod := func() { // Delete the pod. - err := c.clientSet.CoreV1().Pods(namespace).Delete(ctx, createdDebugPod.Name, metav1.DeleteOptions{}) + err := c.clientSet.CoreV1().Pods(namespace).Delete(context.Background(), createdDebugPod.Name, metav1.DeleteOptions{}) if err != nil { log.Printf("failed to cleanup debug pod: %v", err) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/ovnk-mcp-server/main.gopkg/kubernetes/client/nodes.gopkg/kubernetes/mcp/nodes.gopkg/kubernetes/types/nodes.gopkg/nettools/mcp/mcp.gopkg/nettools/mcp/pwru.gopkg/nettools/mcp/tcpdump.gopkg/nettools/mcp/utils.gopkg/nettools/mcp/utils_test.gopkg/nettools/types/manifest.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/kubernetes/types/nodes.go
- cmd/ovnk-mcp-server/main.go
- pkg/nettools/mcp/tcpdump.go
- pkg/nettools/mcp/utils.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/kubernetes/client/nodes.go (1)
pkg/kubernetes/client/client.go (1)
OVNKMCPServerClientSet(13-20)
pkg/kubernetes/mcp/nodes.go (3)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-13)pkg/kubernetes/mcp/mcp.go (1)
MCPServer(15-17)pkg/kubernetes/types/nodes.go (2)
DebugNodeParams(4-10)DebugNodeResult(13-16)
pkg/nettools/mcp/pwru.go (3)
pkg/nettools/mcp/mcp.go (1)
MCPServer(9-13)pkg/nettools/types/manifest.go (2)
PwruParams(30-35)CommandResult(38-42)pkg/kubernetes/types/nodes.go (1)
DebugNodeParams(4-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (16)
pkg/kubernetes/client/nodes.go (3)
15-33: LGTM!The updated
DebugNodesignature correctly accepts and forwards the newhostPathandmountPathparameters tocreatePod. The function structure remains clean with proper error handling and cleanup.
35-56: LGTM!The default handling for
hostPathandmountPathis well-implemented. TheHOSTenvironment variable is now correctly set tomountPath(addressing the previous review feedback), which ensures tools relying on this variable will reference the correct mount location.
58-104: LGTM!The pod specification correctly incorporates the dynamic
hostPathandmountPathvalues. The privileged security context and host networking configuration are appropriate for a debug pod that needs full access to node resources.pkg/kubernetes/mcp/nodes.go (2)
13-46: LGTM!The
validatePathfunction correctly validates paths for safe mounting:
- Checks for absolute paths
- Properly detects path traversal by checking the original input (addressing previous feedback about
filepath.Cleanmasking "..")- Character whitelist includes necessary special characters (
~for Kubernetes volume names likekubernetes.io~secret)
48-65: LGTM!The
DebugNodemethod correctly validates both paths before creating the debug pod. The validation-first approach ensures malicious or malformed paths are rejected early, preventing potential security issues.pkg/nettools/types/manifest.go (1)
1-42: LGTM!The type definitions are well-structured:
BaseNetworkDiagParamsprovides shared BPF filter functionalityTcpdumpParamscorrectly distinguishes between node and pod targets with conditional required fieldsPwruParamsappropriately marksNodeNameas required (noomitempty) since pwru only operates at node levelCommandResultcaptures all necessary output informationpkg/nettools/mcp/utils_test.go (5)
1-100: LGTM!
TestValidateIntMaxprovides comprehensive coverage of boundary conditions including zero, minimum valid, at-max, over-max, and negative values. The test structure is clean and readable.
102-143: LGTM!
TestStringWithDefaultcorrectly tests the fallback behavior for empty strings while preserving whitespace-only values, which is the expected behavior for this utility.
145-298: LGTM!
TestValidateInterfaceprovides excellent coverage of Linux interface naming rules:
- Valid patterns (alphanumeric, dots, hyphens, underscores)
- Length constraints (max 15 characters per IFNAMSIZ)
- Invalid starting characters and dangerous shell metacharacters
300-489: LGTM!
TestValidatePacketFilterprovides thorough security testing:
- Valid BPF filter expressions including complex nested filters
- Command injection vectors (semicolon, pipe, backtick,
$(),&&,||)- Null byte injection test
- Length boundary tests now correctly use
strings.Repeat("a", N)(addressing previous feedback)
491-568: LGTM!
TestCommandBuildervalidates the fluent builder API comprehensively, covering base commands, argument addition, conditional additions (addIf,addIfNotEmpty), and method chaining.pkg/nettools/mcp/mcp.go (2)
8-22: LGTM!The
MCPServerstruct correctly separatespwruImageandtcpdumpImagefields (addressing previous feedback about the singlenodeImagefield causing confusion). TheNewMCPServerconstructor properly initializes all fields.
24-76: LGTM!The
AddToolsmethod provides comprehensive tool registrations:
- Clear documentation of supported parameters with defaults and constraints
- Practical examples for common use cases
- Correct separation of image configuration flags (
--tcpdump-imageand--pwru-image)- Tool descriptions align with the parameter types defined in
pkg/nettools/types/manifest.gopkg/nettools/mcp/pwru.go (3)
22-28: Output limit handling is correct.The defaulting logic (when
outputLimitLines == 0) follows the standard Go pattern for optional int fields, and the validation viavalidateIntMaxproperly enforces both negative value rejection and upper bound as confirmed in prior review comments.
38-50: Target construction and execution look good.The
DebugNodeParamscorrectly:
- Uses
s.pwruImagefor configurability- Mounts
/sys/kernel/debugrequired for eBPF/pwru access- Passes the built command
The execution flow and error handling are straightforward.
34-36: No changes needed. TheaddIfNotEmptyAPI usage is correct and idiomatic.The method signature is
addIfNotEmpty(value string, args ...string), where the first parameter is a condition to check and the remaining parameters are arguments to append only if the value is non-empty. Passingin.BPFFilterfor both parameters is the standard pattern throughout the codebase—the same approach is used intcpdump.go(line 54),iptables.go(line 40), andip.go(line 37). This usage is explicitly tested inutils_test.go(line 528) and correctly adds the BPF filter as a positional argument when it is not empty, which matches the intent of the comment.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Meina-rh
force-pushed
the
external_tools
branch
from
5ace57e to
5b53adb
Compare
This commit add network external tools: tcpdump and pwru - tcpdump: Capture and analyze network packets with BPF filters - pwru: allows fine-grained introspection of kernel state to facilitate debugging network connectivity issues Assisted-By: Claude Code Signed-off-by: Meina-rh <meinli@redhat.com>
Meina-rh
force-pushed
the
external_tools
branch
from
5b53adb to
165c0b1
Compare
|
@pperiyasamy I've resolved conflict and both tools can work well in Claude |
arkadeepsen
dismissed
tssurya’s stale review
Dismissing Surya's review to merge the PR as this was a test review.
5 participants
This PR add network external tools: tcpdump and pwru
Test with Claude Code, it can work well
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.