Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cmd/ovnk-mcp-server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

mcp "github.com/modelcontextprotocol/go-sdk/mcp"
kubernetesmcp "github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kubernetes/mcp"
ovsmcp "github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/ovs/mcp"
sosreportmcp "github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/sosreport/mcp"
)

Expand All @@ -37,6 +38,9 @@ func main() {
}
log.Println("Adding Kubernetes tools to OVN-K MCP server")
k8sMcpServer.AddTools(ovnkMcpServer)
ovsServer := ovsmcp.NewMCPServer(k8sMcpServer)
log.Println("Adding OVS tools to OVN-K MCP server")
ovsServer.AddTools(ovnkMcpServer)
}
if serverCfg.Mode == "offline" {
sosreportServer := sosreportmcp.NewMCPServer()
Expand Down
126 changes: 126 additions & 0 deletions pkg/ovs/mcp/commands.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
package mcp

import (
"context"
"fmt"
"regexp"
"strings"

"github.com/modelcontextprotocol/go-sdk/mcp"
k8stypes "github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kubernetes/types"
)

const defaultMaxLines = 100

func (s *MCPServer) runCommand(ctx context.Context, req *mcp.CallToolRequest, namespacedName k8stypes.NamespacedNameParams,
commands []string) ([]string, error) {
_, result, err := s.k8sMcpServer.ExecPod(ctx, req, k8stypes.ExecPodParams{NamespacedNameParams: namespacedName, Command: commands})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The container name is not taken as a parameter for the different ovs commands. Will it always be able to get the result from the 1st container in the pod?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, container name is not passed from here, seems like mcp pod client ensures it always run on the 1st container https://github.com/ovn-kubernetes/ovn-kubernetes-mcp/blob/main/pkg/kubernetes/client/pods.go#L51.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct that the k8s layer is always choosing the 1st container, but the question is that will it always work? What I mean is that will any of the container be able to provide the response or should container name be an optional parameter for the LLM to provide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! ok, we could run ovs commands on any container for ovnkube-node pod, so i don't see a need for taking container name as an optional parameter for now.

if err != nil {
return nil, err
}
if result.Stderr != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any kind of warnings may also be considered as error returned by the command. I think filtering warnings is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean warning messages from result.Stderr ? can you explain it a bit more ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hit an issue while working on a PR for kernel layer where CLI tool was returning a harmless warning along with stdout. But since the warning was being received by stderr, the whole function execution was failing and the LLM was not returning any data.

Copy link
Contributor Author

@pperiyasamy pperiyasamy Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok @arghosh93 , i saw those handling with kernel layer, but i don't see that is happening with ovs commands which runs on ovnkube-node pod.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

return nil, fmt.Errorf("error occurred while running command %v on pod %s/%s: %s", commands, namespacedName.Namespace,
namespacedName.Name, result.Stderr)
}
output := []string{} // Initialize with empty slice to ensure valid JSON when there's no output
for _, line := range strings.Split(result.Stdout, "\n") {
line = strings.TrimSpace(line)
if line != "" {
output = append(output, line)
}
}
return output, nil
}

// filterLines filters lines using a regex pattern.
func filterLines(lines []string, pattern string) ([]string, error) {
if pattern == "" {
return lines, nil
}

filterPattern, err := regexp.Compile(pattern)
if err != nil {
return nil, fmt.Errorf("invalid filter pattern %s: %w", pattern, err)
}

filtered := []string{} // Initialize with empty slice to ensure valid JSON when there's no output
for _, line := range lines {
if filterPattern.MatchString(line) {
filtered = append(filtered, line)
}
}
return filtered, nil
}

// limitLines limits the number of lines returned.
func limitLines(lines []string, maxLines int) []string {
if maxLines <= 0 {
maxLines = defaultMaxLines
}
if len(lines) > maxLines {
return lines[:maxLines]
}
return lines
}

// validateBridgeName validates that a bridge name is safe and non-empty.
// Bridge names should only contain alphanumeric characters, hyphens, and underscores.
func validateBridgeName(bridge string) error {
if bridge == "" {
return fmt.Errorf("bridge name cannot be empty")
}

// OVS bridge names typically follow naming conventions: alphanumeric, hyphens, underscores
validBridgeName := regexp.MustCompile(`^[a-zA-Z0-9_-]+$`)
if !validBridgeName.MatchString(bridge) {
return fmt.Errorf("invalid bridge name %q: must contain only alphanumeric characters, hyphens, and underscores", bridge)
}

return nil
}

// validateFlowSpec validates that a flow specification is safe and non-empty.
func validateFlowSpec(flow string) error {
if flow == "" {
return fmt.Errorf("flow specification cannot be empty")
}

// Check for potentially dangerous characters that shouldn't appear in flow specs
// Flow specs should contain: alphanumeric, commas, equals, colons, periods, slashes, parentheses, brackets
// We explicitly block: semicolons, pipes, backticks, dollar signs, and other shell metacharacters
dangerousChars := regexp.MustCompile(`[;&|$` + "`" + `<>\\]`)
if dangerousChars.MatchString(flow) {
return fmt.Errorf("invalid flow specification: contains potentially dangerous characters")
}

return nil
}
Comment on lines +66 to +97
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice validations to prevent command injection.


// validateConntrackParams validates that conntrack additional parameters are safe.
// Valid parameters for dpctl/dump-conntrack include: zone=N, mark=0xN, labels=0xN, -m, -s, etc.
func validateConntrackParams(params []string) error {
for _, param := range params {
if param == "" {
return fmt.Errorf("conntrack parameter cannot be empty")
}

// Check for potentially dangerous characters
// Valid conntrack params should contain: alphanumeric, equals, hyphens, underscores, periods, colons, commas, forward slashes
// We explicitly block: semicolons, pipes, backticks, dollar signs, ampersands, and other shell metacharacters
dangerousChars := regexp.MustCompile(`[;&|$` + "`" + `<>\\()]`)
if dangerousChars.MatchString(param) {
return fmt.Errorf("invalid conntrack parameter %q: contains potentially dangerous characters", param)
}

// Additional validation for common parameter patterns
// Valid patterns include:
// - Single-char flags: -m, -s (single hyphen followed by single letter)
// - Key=value pairs: zone=5, mark=0x1, src=10.0.0.1 (key must contain only alphanumeric, underscore, hyphen)
validParam := regexp.MustCompile(`^(-[a-zA-Z]|[a-zA-Z0-9_-]+=[a-zA-Z0-9x.:,/_-]+)$`)
if !validParam.MatchString(param) {
return fmt.Errorf("invalid conntrack parameter format %q: must be a flag (e.g., '-m') or key=value pair (e.g., 'zone=5')", param)
}
}

return nil
}
Loading
Loading