feat: consolidate tools and reduce total number of tools #96

feiskyer · 2025-07-25T06:34:25Z

This PR consolidates the AKS MCP tools to reduce the total number of registered tools while maintaining the same functionality. The consolidation moves from individual command-specific tools to unified operation-based tools that handle multiple related commands.

Tools reduced from 47-> 18.

Key Changes:

Consolidated AKS Operations: Replaced individual az aks command tools with a single az_aks_operations tool that handles all AKS cluster,
nodepool, and account operations
Added Operation Executor: Introduced AksOperationsExecutor in azaks/executor.go to handle unified command execution with proper access
validation
Simplified Tool Registration: Streamlined the tool registration process by replacing multiple command-specific registrations with
operation-based registration
Enhanced Access Control: Improved access level validation that works consistently across all operations
Reduced Complexity: Removed redundant code in monitor and network components, consolidating handlers and simplifying registry logic

Technical Details:

Files Modified: 12 files with 787 additions and 807 deletions (net reduction)
New Components: Added unified executor pattern for command handling
Maintained Compatibility: All existing functionality preserved through operation mapping
Improved Testing: Updated test suites to reflect the new consolidated architecture

Benefits:

Reduces the total number of MCP tools registered, improving agent performance
Simplifies maintenance with unified command handling
Maintains security through consistent access validation
Preserves all existing functionality while reducing code duplication

Copilot

Pull Request Overview

This PR consolidates the AKS MCP tools from 47 to 18 tools while maintaining all functionality. The consolidation moves from individual command-specific tools to unified operation-based tools, introducing a new executor pattern for handling multiple related operations through single tools.

Consolidates AKS operations into a single az_aks_operations tool with operation-based routing
Unifies monitoring operations into an az_monitoring tool supporting metrics, resource health, app insights, diagnostics, and logs
Consolidates network resource tools into a single az_network_resources tool with resource-type-based routing

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`internal/server/server.go`	Streamlines tool registration by replacing multiple command-specific registrations with unified tools
`internal/k8s/adapter.go`	Minor comment cleanup removing redundant text
`internal/components/network/registry_test.go`	Removes individual tool registration tests, keeping handler tests
`internal/components/network/registry.go`	Replaces multiple network tool registrations with unified `az_network_resources` tool
`internal/components/network/handlers.go`	Adds unified handler for network resources with operation routing
`internal/components/monitor/registry_test.go`	Updates tests to match new consolidated monitoring tool structure
`internal/components/monitor/registry.go`	Consolidates monitoring commands into unified `az_monitoring` tool
`internal/components/monitor/handlers_test.go`	Removes obsolete tool registration test
`internal/components/monitor/handlers.go`	Adds unified monitoring handler with operation-based routing
`internal/components/azaks/registry_test.go`	Updates tests for new consolidated AKS operations approach
`internal/components/azaks/registry.go`	Replaces command lists with operation-based tool registration and access validation
`internal/components/azaks/executor.go`	New executor implementing operation-to-command mapping with access control
`internal/components/advisor/aks_recommendations.go`	Removes commented code for cleaner implementation

Comments suppressed due to low confidence (1)

internal/components/azaks/registry.go:99

[nitpick] The tool name 'az_aks_operations' is inconsistent with the naming convention used for other tools like 'az_monitoring' and 'az_network_resources'. Consider using 'az_aks' for consistency.

	return mcp.NewTool("az_aks_operations",

Copilot · 2025-07-28T03:03:36Z

internal/components/network/handlers.go

+	result := make(map[string]interface{})
+
+	// Get VNet info
+	if vnetResult, err := handleVNetResource(client, subID, rg, clusterName); err == nil {


The error handling pattern using assignment in if condition could mask errors. If handleVNetResource returns a non-nil error, it's assigned to err but then the error is only stored in the result map as a string. This could make debugging difficult as the original error context is lost.

Copilot · 2025-07-28T03:03:36Z

internal/components/azaks/executor.go

+	}
+
+	// If the command is not an az command, return an error
+	if binaryName != "az" {


The security check only validates that the command starts with 'az', but doesn't prevent command injection through the arguments. An attacker could potentially inject malicious commands through semicolons, pipes, or other shell operators in the args parameter.

Copilot · 2025-07-28T03:03:37Z

internal/components/monitor/handlers.go

 	"github.com/Azure/aks-mcp/internal/config"
 	"github.com/Azure/aks-mcp/internal/tools"
 )

+// mergeMonitoringParams merges top-level parameters with nested "parameters" JSON string


The mergeMonitoringParams function lacks documentation explaining its purpose, parameters, and return values. This is a key function that handles parameter merging logic and should be properly documented.

Suggested change

// mergeMonitoringParams merges top-level parameters with nested "parameters" JSON string

// mergeMonitoringParams merges top-level parameters with nested "parameters" JSON string.

//

// Parameters:

// - params: A map[string]interface{} containing the top-level parameters. If a key "parameters"

// exists and its value is a JSON string, the function will attempt to parse and merge it.

//

// Returns:

// - A map[string]interface{} containing the merged parameters. Top-level parameters take precedence

// over nested parameters in case of key conflicts.

// - An error if the "parameters" key contains an invalid JSON string.

Copilot · 2025-07-28T03:03:37Z

internal/components/network/handlers.go

+		// Extract resource_type parameter
+		resourceType, ok := params["resource_type"].(string)
+		if !ok {
+			return "", fmt.Errorf("missing or invalid 'resource_type' parameter")
+		}
+
+		// Validate resource type
+		if !ValidateNetworkResourceType(resourceType) {
+			supportedTypes := GetSupportedNetworkResourceTypes()
+			return "", fmt.Errorf("unsupported resource type: %s. Supported types: %v", resourceType, supportedTypes)
+		}
+
+		// Extract common AKS parameters
+		subID, rg, clusterName, err := common.ExtractAKSParameters(params)
+		if err != nil {
+			return "", err
+		}
+
+		// Handle different resource types
+		switch resourceType {
+		case string(ResourceTypeAll):
+			return handleAllNetworkResources(client, subID, rg, clusterName)
+		case string(ResourceTypeVNet):
+			return handleVNetResource(client, subID, rg, clusterName)
+		case string(ResourceTypeNSG):
+			return handleNSGResource(client, subID, rg, clusterName)
+		case string(ResourceTypeRouteTable):
+			return handleRouteTableResource(client, subID, rg, clusterName)
+		case string(ResourceTypeSubnet):
+			return handleSubnetResource(client, subID, rg, clusterName)
+		case string(ResourceTypeLoadBalancer):
+			return handleLoadBalancerResource(client, subID, rg, clusterName)
+		case string(ResourceTypePrivateEndpoint):
+			return handlePrivateEndpointResource(client, subID, rg, clusterName)
+		default:
+			return "", fmt.Errorf("resource type '%s' not implemented", resourceType)
+		}
+	})
+}
+


The GetAzNetworkResourcesHandler function is quite large (over 160 lines) and handles multiple concerns. Consider breaking it down into smaller, more focused functions for better maintainability and testability.

Suggested change

// Extract resource_type parameter

resourceType, ok := params["resource_type"].(string)

if !ok {

return "", fmt.Errorf("missing or invalid 'resource_type' parameter")

}

// Validate resource type

if !ValidateNetworkResourceType(resourceType) {

supportedTypes := GetSupportedNetworkResourceTypes()

return "", fmt.Errorf("unsupported resource type: %s. Supported types: %v", resourceType, supportedTypes)

}

// Extract common AKS parameters

subID, rg, clusterName, err := common.ExtractAKSParameters(params)

if err != nil {

return "", err

}

// Handle different resource types

switch resourceType {

case string(ResourceTypeAll):

return handleAllNetworkResources(client, subID, rg, clusterName)

case string(ResourceTypeVNet):

return handleVNetResource(client, subID, rg, clusterName)

case string(ResourceTypeNSG):

return handleNSGResource(client, subID, rg, clusterName)

case string(ResourceTypeRouteTable):

return handleRouteTableResource(client, subID, rg, clusterName)

case string(ResourceTypeSubnet):

return handleSubnetResource(client, subID, rg, clusterName)

case string(ResourceTypeLoadBalancer):

return handleLoadBalancerResource(client, subID, rg, clusterName)

case string(ResourceTypePrivateEndpoint):

return handlePrivateEndpointResource(client, subID, rg, clusterName)

default:

return "", fmt.Errorf("resource type '%s' not implemented", resourceType)

}

})

}

// Extract and validate parameters

resourceType, subID, rg, clusterName, err := extractAndValidateParams(params)

if err != nil {

return "", err

}

// Handle resource type

return handleResourceType(client, resourceType, subID, rg, clusterName)

})

}

func extractAndValidateParams(params map[string]interface{}) (string, string, string, string, error) {

// Extract resource_type parameter

resourceType, ok := params["resource_type"].(string)

if !ok {

return "", "", "", "", fmt.Errorf("missing or invalid 'resource_type' parameter")

}

// Validate resource type

if !ValidateNetworkResourceType(resourceType) {

supportedTypes := GetSupportedNetworkResourceTypes()

return "", "", "", "", fmt.Errorf("unsupported resource type: %s. Supported types: %v", resourceType, supportedTypes)

}

// Extract common AKS parameters

subID, rg, clusterName, err := common.ExtractAKSParameters(params)

if err != nil {

return "", "", "", "", err

}

return resourceType, subID, rg, clusterName, nil

}

func handleResourceType(client *azureclient.AzureClient, resourceType, subID, rg, clusterName string) (string, error) {

switch resourceType {

case string(ResourceTypeAll):

return handleAllNetworkResources(client, subID, rg, clusterName)

case string(ResourceTypeVNet):

return handleVNetResource(client, subID, rg, clusterName)

case string(ResourceTypeNSG):

return handleNSGResource(client, subID, rg, clusterName)

case string(ResourceTypeRouteTable):

return handleRouteTableResource(client, subID, rg, clusterName)

case string(ResourceTypeSubnet):

return handleSubnetResource(client, subID, rg, clusterName)

case string(ResourceTypeLoadBalancer):

return handleLoadBalancerResource(client, subID, rg, clusterName)

case string(ResourceTypePrivateEndpoint):

return handlePrivateEndpointResource(client, subID, rg, clusterName)

default:

return "", fmt.Errorf("resource type '%s' not implemented", resourceType)

}

}

gossion · 2025-07-28T03:16:09Z

internal/components/monitor/registry.go

-	return []MonitorCommand{}
-}
+Examples:
+- List metrics: operation="metrics", query_type="list", parameters="{\"resource\":\"<aks-cluster-id>\"}"


my test on getting aks/lb metrics passed. but failed on getting vm metrics sometimes, it would be better if we can provide different examples for different resource, I guess.

yes, I think so. What common scenarios do you suggest to add here?

internal/components/monitor/registry.go

feiskyer force-pushed the consolidate-tools branch from 93388b4 to 151502e Compare July 25, 2025 06:37

feiskyer added 4 commits July 28, 2025 10:20

feat: consolidate tools and reduce total number of tools

293836f

fix: fix parameters for monitor tools

536e2f3

fix: set correct access level for k8s tools

264a385

Fix the monitor params

29a3ba8

feiskyer force-pushed the consolidate-tools branch from 173aea7 to 29a3ba8 Compare July 28, 2025 02:21

feiskyer requested a review from Copilot July 28, 2025 03:02

Copilot AI reviewed Jul 28, 2025

View reviewed changes

gossion reviewed Jul 28, 2025

View reviewed changes

chore: rename general logs to control_plane_logs

d5e3814

gossion approved these changes Jul 28, 2025

View reviewed changes

feiskyer merged commit f1bd2be into main Jul 28, 2025
9 checks passed

feiskyer deleted the consolidate-tools branch July 28, 2025 05:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: consolidate tools and reduce total number of tools #96

feat: consolidate tools and reduce total number of tools #96

Uh oh!

feiskyer commented Jul 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 28, 2025

Uh oh!

Copilot AI Jul 28, 2025

Uh oh!

Copilot AI Jul 28, 2025

Uh oh!

Copilot AI Jul 28, 2025

Uh oh!

gossion Jul 28, 2025

Uh oh!

feiskyer Jul 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

-// mergeMonitoringParams merges top-level parameters with nested "parameters" JSON string
+// mergeMonitoringParams merges top-level parameters with nested "parameters" JSON string.
+//
+// Parameters:
+//   - params: A map[string]interface{} containing the top-level parameters. If a key "parameters"
+//     exists and its value is a JSON string, the function will attempt to parse and merge it.
+//
+// Returns:
+//   - A map[string]interface{} containing the merged parameters. Top-level parameters take precedence
+//     over nested parameters in case of key conflicts.
+//   - An error if the "parameters" key contains an invalid JSON string.

feat: consolidate tools and reduce total number of tools #96

feat: consolidate tools and reduce total number of tools #96

Uh oh!

Conversation

feiskyer commented Jul 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

gossion Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

feiskyer Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!