From ff9f2e63549ebbb2960accb286c0ecaf3384df9d Mon Sep 17 00:00:00 2001 From: Nader Ziada Date: Tue, 10 Feb 2026 16:42:43 -0500 Subject: [PATCH 1/3] feat(validation): add pre-execution validation layer Add validation middleware that catches errors before they reach the Kubernetes API. Signed-off-by: Nader Ziada --- README.md | 134 ++++++------ docs/VALIDATION.md | 136 ++++++++++++ pkg/api/config.go | 11 + pkg/api/validation.go | 123 +++++++++++ pkg/config/config.go | 8 + pkg/config/validation_config.go | 28 +++ pkg/kubernetes/auth.go | 54 +++++ pkg/kubernetes/kubernetes.go | 25 ++- pkg/kubernetes/rbac_validator.go | 52 +++++ pkg/kubernetes/rbac_validator_test.go | 155 ++++++++++++++ pkg/kubernetes/resource_validator.go | 55 +++++ pkg/kubernetes/resource_validator_test.go | 139 ++++++++++++ pkg/kubernetes/resources.go | 18 +- pkg/kubernetes/schema_validator.go | 130 ++++++++++++ pkg/kubernetes/validation_round_tripper.go | 236 +++++++++++++++++++++ pkg/kubernetes/validator_registry.go | 46 ++++ pkg/kubernetes/validator_registry_test.go | 53 +++++ pkg/mcp/events_test.go | 12 +- pkg/mcp/mcp.go | 1 + pkg/mcp/namespaces_test.go | 13 +- pkg/mcp/pods_test.go | 15 +- pkg/mcp/resources_test.go | 43 ++-- 22 files changed, 1349 insertions(+), 138 deletions(-) create mode 100644 docs/VALIDATION.md create mode 100644 pkg/api/validation.go create mode 100644 pkg/config/validation_config.go create mode 100644 pkg/kubernetes/auth.go create mode 100644 pkg/kubernetes/rbac_validator.go create mode 100644 pkg/kubernetes/rbac_validator_test.go create mode 100644 pkg/kubernetes/resource_validator.go create mode 100644 pkg/kubernetes/resource_validator_test.go create mode 100644 pkg/kubernetes/schema_validator.go create mode 100644 pkg/kubernetes/validation_round_tripper.go create mode 100644 pkg/kubernetes/validator_registry.go create mode 100644 pkg/kubernetes/validator_registry_test.go diff --git a/README.md b/README.md index 1eb1f284f..092c81e4d 100644 --- a/README.md +++ b/README.md @@ -311,10 +311,10 @@ The following sets of tools are available (toolsets marked with ✓ in the Defau | Toolset | Description | Default | |----------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------| +| kiali | Most common tools for managing Kiali, check the [Kiali documentation](https://github.com/containers/kubernetes-mcp-server/blob/main/docs/KIALI.md) for more details. | | | config | View and manage the current local Kubernetes configuration (kubeconfig) | ✓ | | core | Most common tools for Kubernetes management (Pods, Generic Resources, Events, etc.) | ✓ | | kcp | Manage kcp workspaces and multi-tenancy features | | -| kiali | Most common tools for managing Kiali, check the [Kiali documentation](https://github.com/containers/kubernetes-mcp-server/blob/main/docs/KIALI.md) for more details. | | | kubevirt | KubeVirt virtual machine management tools | | | helm | Tools for managing Helm charts and releases | ✓ | @@ -328,6 +328,72 @@ In case multi-cluster support is enabled (default) and you have access to multip
+kiali + +- **kiali_mesh_graph** - Returns the topology of a specific namespaces, health, status of the mesh and namespaces. Includes a mesh health summary overview with aggregated counts of healthy, degraded, and failing apps, workloads, and services. Use this for high-level overviews + - `graphType` (`string`) - Optional type of graph to return: 'versionedApp', 'app', 'service', 'workload', 'mesh' + - `namespace` (`string`) - Optional single namespace to include in the graph (alternative to namespaces) + - `namespaces` (`string`) - Optional comma-separated list of namespaces to include in the graph + - `rateInterval` (`string`) - Optional rate interval for fetching (e.g., '10m', '5m', '1h'). + +- **kiali_manage_istio_config_read** - Lists or gets Istio configuration objects (Gateways, VirtualServices, etc.) + - `action` (`string`) **(required)** - Action to perform: list or get + - `group` (`string`) - API group of the Istio object (e.g., 'networking.istio.io', 'gateway.networking.k8s.io') + - `kind` (`string`) - Kind of the Istio object (e.g., 'DestinationRule', 'VirtualService', 'HTTPRoute', 'Gateway') + - `name` (`string`) - Name of the Istio object + - `namespace` (`string`) - Namespace containing the Istio object + - `version` (`string`) - API version of the Istio object (e.g., 'v1', 'v1beta1') + +- **kiali_manage_istio_config** - Creates, patches, or deletes Istio configuration objects (Gateways, VirtualServices, etc.) + - `action` (`string`) **(required)** - Action to perform: create, patch, or delete + - `group` (`string`) - API group of the Istio object (e.g., 'networking.istio.io', 'gateway.networking.k8s.io') + - `json_data` (`string`) - JSON data to apply or create the object + - `kind` (`string`) - Kind of the Istio object (e.g., 'DestinationRule', 'VirtualService', 'HTTPRoute', 'Gateway') + - `name` (`string`) - Name of the Istio object + - `namespace` (`string`) - Namespace containing the Istio object + - `version` (`string`) - API version of the Istio object (e.g., 'v1', 'v1beta1') + +- **kiali_get_resource_details** - Gets lists or detailed info for Kubernetes resources (services, workloads) within the mesh + - `namespaces` (`string`) - Comma-separated list of namespaces to get services from (e.g. 'bookinfo' or 'bookinfo,default'). If not provided, will list services from all accessible namespaces + - `resource_name` (`string`) - Name of the resource to get details for (optional string - if provided, gets details; if empty, lists all). + - `resource_type` (`string`) - Type of resource to get details for (service, workload) + +- **kiali_get_metrics** - Gets lists or detailed info for Kubernetes resources (services, workloads) within the mesh + - `byLabels` (`string`) - Comma-separated list of labels to group metrics by (e.g., 'source_workload,destination_service'). Optional + - `direction` (`string`) - Traffic direction: 'inbound' or 'outbound'. Optional, defaults to 'outbound' + - `duration` (`string`) - Time range to get metrics for (optional string - if provided, gets metrics (e.g., '1m', '5m', '1h'); if empty, get default 30m). + - `namespace` (`string`) **(required)** - Namespace to get resources from + - `quantiles` (`string`) - Comma-separated list of quantiles for histogram metrics (e.g., '0.5,0.95,0.99'). Optional + - `rateInterval` (`string`) - Rate interval for metrics (e.g., '1m', '5m'). Optional, defaults to '10m' + - `reporter` (`string`) - Metrics reporter: 'source', 'destination', or 'both'. Optional, defaults to 'source' + - `requestProtocol` (`string`) - Filter by request protocol (e.g., 'http', 'grpc', 'tcp'). Optional + - `resource_name` (`string`) **(required)** - Name of the resource to get details for (optional string - if provided, gets details; if empty, lists all). + - `resource_type` (`string`) **(required)** - Type of resource to get details for (service, workload) + - `step` (`string`) - Step between data points in seconds (e.g., '15'). Optional, defaults to 15 seconds + +- **kiali_workload_logs** - Get logs for a specific workload's pods in a namespace. Only requires namespace and workload name - automatically discovers pods and containers. Optionally filter by container name, time range, and other parameters. Container is auto-detected if not specified. + - `container` (`string`) - Optional container name to filter logs. If not provided, automatically detects and uses the main application container (excludes istio-proxy and istio-init) + - `namespace` (`string`) **(required)** - Namespace containing the workload + - `since` (`string`) - Time duration to fetch logs from (e.g., '5m', '1h', '30s'). If not provided, returns recent logs + - `tail` (`integer`) - Number of lines to retrieve from the end of logs (default: 100) + - `workload` (`string`) **(required)** - Name of the workload to get logs for + +- **kiali_get_traces** - Gets traces for a specific resource (app, service, workload) in a namespace, or gets detailed information for a specific trace by its ID. If traceId is provided, it returns detailed trace information and other parameters are not required. + - `clusterName` (`string`) - Cluster name for multi-cluster environments (optional, only used when traceId is not provided) + - `endMicros` (`string`) - End time for traces in microseconds since epoch (optional, defaults to 10 minutes after startMicros if not provided, only used when traceId is not provided) + - `limit` (`integer`) - Maximum number of traces to return (default: 100, only used when traceId is not provided) + - `minDuration` (`integer`) - Minimum trace duration in microseconds (optional, only used when traceId is not provided) + - `namespace` (`string`) - Namespace to get resources from. Required if traceId is not provided. + - `resource_name` (`string`) - Name of the resource to get traces for. Required if traceId is not provided. + - `resource_type` (`string`) - Type of resource to get traces for (app, service, workload). Required if traceId is not provided. + - `startMicros` (`string`) - Start time for traces in microseconds since epoch (optional, defaults to 10 minutes before current time if not provided, only used when traceId is not provided) + - `tags` (`string`) - JSON string of tags to filter traces (optional, only used when traceId is not provided) + - `traceId` (`string`) - Unique identifier of the trace to retrieve detailed information for. If provided, this will return detailed trace information and other parameters (resource_type, namespace, resource_name) are not required. + +
+ +
+ config - **configuration_contexts_list** - List all available context names and associated server urls from the kubeconfig file @@ -453,72 +519,6 @@ In case multi-cluster support is enabled (default) and you have access to multip
-kiali - -- **kiali_mesh_graph** - Returns the topology of a specific namespaces, health, status of the mesh and namespaces. Includes a mesh health summary overview with aggregated counts of healthy, degraded, and failing apps, workloads, and services. Use this for high-level overviews - - `graphType` (`string`) - Optional type of graph to return: 'versionedApp', 'app', 'service', 'workload', 'mesh' - - `namespace` (`string`) - Optional single namespace to include in the graph (alternative to namespaces) - - `namespaces` (`string`) - Optional comma-separated list of namespaces to include in the graph - - `rateInterval` (`string`) - Optional rate interval for fetching (e.g., '10m', '5m', '1h'). - -- **kiali_manage_istio_config_read** - Lists or gets Istio configuration objects (Gateways, VirtualServices, etc.) - - `action` (`string`) **(required)** - Action to perform: list or get - - `group` (`string`) - API group of the Istio object (e.g., 'networking.istio.io', 'gateway.networking.k8s.io') - - `kind` (`string`) - Kind of the Istio object (e.g., 'DestinationRule', 'VirtualService', 'HTTPRoute', 'Gateway') - - `name` (`string`) - Name of the Istio object - - `namespace` (`string`) - Namespace containing the Istio object - - `version` (`string`) - API version of the Istio object (e.g., 'v1', 'v1beta1') - -- **kiali_manage_istio_config** - Creates, patches, or deletes Istio configuration objects (Gateways, VirtualServices, etc.) - - `action` (`string`) **(required)** - Action to perform: create, patch, or delete - - `group` (`string`) - API group of the Istio object (e.g., 'networking.istio.io', 'gateway.networking.k8s.io') - - `json_data` (`string`) - JSON data to apply or create the object - - `kind` (`string`) - Kind of the Istio object (e.g., 'DestinationRule', 'VirtualService', 'HTTPRoute', 'Gateway') - - `name` (`string`) - Name of the Istio object - - `namespace` (`string`) - Namespace containing the Istio object - - `version` (`string`) - API version of the Istio object (e.g., 'v1', 'v1beta1') - -- **kiali_get_resource_details** - Gets lists or detailed info for Kubernetes resources (services, workloads) within the mesh - - `namespaces` (`string`) - Comma-separated list of namespaces to get services from (e.g. 'bookinfo' or 'bookinfo,default'). If not provided, will list services from all accessible namespaces - - `resource_name` (`string`) - Name of the resource to get details for (optional string - if provided, gets details; if empty, lists all). - - `resource_type` (`string`) - Type of resource to get details for (service, workload) - -- **kiali_get_metrics** - Gets lists or detailed info for Kubernetes resources (services, workloads) within the mesh - - `byLabels` (`string`) - Comma-separated list of labels to group metrics by (e.g., 'source_workload,destination_service'). Optional - - `direction` (`string`) - Traffic direction: 'inbound' or 'outbound'. Optional, defaults to 'outbound' - - `duration` (`string`) - Time range to get metrics for (optional string - if provided, gets metrics (e.g., '1m', '5m', '1h'); if empty, get default 30m). - - `namespace` (`string`) **(required)** - Namespace to get resources from - - `quantiles` (`string`) - Comma-separated list of quantiles for histogram metrics (e.g., '0.5,0.95,0.99'). Optional - - `rateInterval` (`string`) - Rate interval for metrics (e.g., '1m', '5m'). Optional, defaults to '10m' - - `reporter` (`string`) - Metrics reporter: 'source', 'destination', or 'both'. Optional, defaults to 'source' - - `requestProtocol` (`string`) - Filter by request protocol (e.g., 'http', 'grpc', 'tcp'). Optional - - `resource_name` (`string`) **(required)** - Name of the resource to get details for (optional string - if provided, gets details; if empty, lists all). - - `resource_type` (`string`) **(required)** - Type of resource to get details for (service, workload) - - `step` (`string`) - Step between data points in seconds (e.g., '15'). Optional, defaults to 15 seconds - -- **kiali_workload_logs** - Get logs for a specific workload's pods in a namespace. Only requires namespace and workload name - automatically discovers pods and containers. Optionally filter by container name, time range, and other parameters. Container is auto-detected if not specified. - - `container` (`string`) - Optional container name to filter logs. If not provided, automatically detects and uses the main application container (excludes istio-proxy and istio-init) - - `namespace` (`string`) **(required)** - Namespace containing the workload - - `since` (`string`) - Time duration to fetch logs from (e.g., '5m', '1h', '30s'). If not provided, returns recent logs - - `tail` (`integer`) - Number of lines to retrieve from the end of logs (default: 100) - - `workload` (`string`) **(required)** - Name of the workload to get logs for - -- **kiali_get_traces** - Gets traces for a specific resource (app, service, workload) in a namespace, or gets detailed information for a specific trace by its ID. If traceId is provided, it returns detailed trace information and other parameters are not required. - - `clusterName` (`string`) - Cluster name for multi-cluster environments (optional, only used when traceId is not provided) - - `endMicros` (`string`) - End time for traces in microseconds since epoch (optional, defaults to 10 minutes after startMicros if not provided, only used when traceId is not provided) - - `limit` (`integer`) - Maximum number of traces to return (default: 100, only used when traceId is not provided) - - `minDuration` (`integer`) - Minimum trace duration in microseconds (optional, only used when traceId is not provided) - - `namespace` (`string`) - Namespace to get resources from. Required if traceId is not provided. - - `resource_name` (`string`) - Name of the resource to get traces for. Required if traceId is not provided. - - `resource_type` (`string`) - Type of resource to get traces for (app, service, workload). Required if traceId is not provided. - - `startMicros` (`string`) - Start time for traces in microseconds since epoch (optional, defaults to 10 minutes before current time if not provided, only used when traceId is not provided) - - `tags` (`string`) - JSON string of tags to filter traces (optional, only used when traceId is not provided) - - `traceId` (`string`) - Unique identifier of the trace to retrieve detailed information for. If provided, this will return detailed trace information and other parameters (resource_type, namespace, resource_name) are not required. - -
- -
- kubevirt - **vm_create** - Create a VirtualMachine in the cluster with the specified configuration, automatically resolving instance types, preferences, and container disk images. VM will be created in Halted state by default; use autostart parameter to start it immediately. diff --git a/docs/VALIDATION.md b/docs/VALIDATION.md new file mode 100644 index 000000000..d0346a3f3 --- /dev/null +++ b/docs/VALIDATION.md @@ -0,0 +1,136 @@ +# Pre-Execution Validation + +The kubernetes-mcp-server includes a validation layer that catches errors before they reach the Kubernetes API. This prevents AI hallucinations (like typos in resource names) and permission issues from causing confusing failures. + +## Why Validation? + +When an AI assistant makes a Kubernetes API call with errors, the raw Kubernetes error messages can be cryptic: + +``` +the server doesn't have a resource type "Deploymnt" +``` + +With validation enabled, you get clearer feedback: + +``` +Resource apps/v1/Deploymnt does not exist in the cluster +``` + +The validation layer catches three types of issues: + +1. **Resource Validation** - Catches typos like "Deploymnt" instead of "Deployment" +2. **Schema Validation** - Catches invalid fields like "spec.replcias" instead of "spec.replicas" +3. **RBAC Validation** - Pre-checks permissions before attempting operations + +## Configuration + +Validation is **disabled by default**. All validators (resource, schema, RBAC) run together when enabled. + +### Environment Variables + +```bash +# Enable all validation +MCP_VALIDATION_ENABLED=true +``` + +### TOML Configuration + +Add a `[validation]` section to your config file: + +```toml +[validation] +# Enable all validation (default: false) +enabled = true +``` + +### Configuration Reference + +| TOML Field | Environment Variable | Default | Description | +|------------|---------------------|---------|-------------| +| `enabled` | `MCP_VALIDATION_ENABLED` | `false` | Enable/disable all validators | + +Environment variables take precedence over TOML config. + +**Note:** The schema validator caches the OpenAPI schema for 15 minutes internally. + +## How It Works + +### Validation Flow + +Validation happens at the HTTP RoundTripper level, intercepting all Kubernetes API calls: + +``` +MCP Tool Call → Kubernetes Client → HTTP RoundTripper → Kubernetes API + ↓ + Access Control (deny list) + ↓ + Resource Validator + "Does this GVK exist?" + ↓ + Schema Validator + "Are the fields valid?" + ↓ + RBAC Validator + "Does the user have permission?" + ↓ + Forward to K8s API +``` + +This HTTP-layer approach ensures **all** Kubernetes API calls are validated, including those from plugins (KubeVirt, Kiali, Helm, etc.) - not just the core tools. + +If any validator fails, the request is rejected with a clear error message before reaching the Kubernetes API. + +### 1. Resource Validation + +Validates that the requested resource type (Group/Version/Kind) exists in the cluster. + +**What it catches:** +- Typos in Kind names: "Deploymnt" → should be "Deployment" +- Wrong API versions: "apps/v2" → should be "apps/v1" +- Non-existent custom resources + +**Example error:** +``` +RESOURCE_NOT_FOUND: Resource apps/v1/Deploymnt does not exist in the cluster +``` + +### 2. Schema Validation + +Validates resource manifests against the cluster's OpenAPI schema for create/update operations. + +**What it catches:** +- Invalid field names: "spec.replcias" → should be "spec.replicas" +- Wrong field types: string where integer expected +- Missing required fields + +**Example error:** +``` +INVALID_FIELD: unknown field "spec.replcias" +``` + +**Note:** Schema validation uses kubectl's validation library and caches the OpenAPI schema for 15 minutes. + +### 3. RBAC Validation + +Pre-checks permissions using Kubernetes `SelfSubjectAccessReview` before attempting operations. + +**What it catches:** +- Missing permissions: can't create Deployments in namespace X +- Cluster-scoped vs namespace-scoped mismatches +- Read-only access attempting writes + +**Example error:** +``` +PERMISSION_DENIED: Cannot create deployments.apps in namespace "production" +``` + +**Note:** RBAC validation uses the same credentials as the actual operation - either the server's service account or the user's token (when OAuth is enabled). + +## Error Codes + +| Code | Description | +|------|-------------| +| `RESOURCE_NOT_FOUND` | The requested resource type doesn't exist in the cluster | +| `INVALID_FIELD` | A field in the manifest doesn't exist or has the wrong type | +| `INVALID_MANIFEST` | The manifest is malformed (invalid YAML/JSON) | +| `PERMISSION_DENIED` | RBAC denies the requested operation | diff --git a/pkg/api/config.go b/pkg/api/config.go index 85c095cd8..cf6b8e438 100644 --- a/pkg/api/config.go +++ b/pkg/api/config.go @@ -53,6 +53,17 @@ type StsConfigProvider interface { GetStsScopes() []string } +// ValidationConfigProvider provides access to validation configuration. +// Implementations can return nil to use default validation settings. +type ValidationConfigProvider interface { + GetValidationConfig() ValidationConfig +} + +// ValidationConfig interface for validation settings. +type ValidationConfig interface { + IsEnabled() bool +} + type BaseConfig interface { AuthProvider ClusterProvider diff --git a/pkg/api/validation.go b/pkg/api/validation.go new file mode 100644 index 000000000..f39abf2ac --- /dev/null +++ b/pkg/api/validation.go @@ -0,0 +1,123 @@ +package api + +import ( + "context" + "fmt" + "strings" + + "k8s.io/apimachinery/pkg/runtime/schema" +) + +// HTTPValidationRequest contains info extracted from an HTTP request for validation. +type HTTPValidationRequest struct { + GVR *schema.GroupVersionResource + GVK *schema.GroupVersionKind + HTTPMethod string // GET, POST, PUT, DELETE, PATCH + Verb string // get, list, create, update, delete, patch + Namespace string + ResourceName string + Body []byte // For create/update validation + Path string +} + +// HTTPValidator validates HTTP requests before they reach the K8s API server. +type HTTPValidator interface { + Validate(ctx context.Context, req *HTTPValidationRequest) error + Name() string +} + +// ValidationErrorCode categorizes validation failures. +type ValidationErrorCode string + +const ( + ErrorCodeResourceNotFound ValidationErrorCode = "RESOURCE_NOT_FOUND" + ErrorCodeInvalidField ValidationErrorCode = "INVALID_FIELD" + ErrorCodePermissionDenied ValidationErrorCode = "PERMISSION_DENIED" + ErrorCodeInvalidManifest ValidationErrorCode = "INVALID_MANIFEST" +) + +// ValidationError provides AI-friendly error information for validation failures. +type ValidationError struct { + Code ValidationErrorCode + Message string + Field string // optional, for field-level errors +} + +// Error implements the error interface. +func (e *ValidationError) Error() string { + var sb strings.Builder + sb.WriteString(fmt.Sprintf("Validation Error [%s]: %s", e.Code, e.Message)) + + if e.Field != "" { + sb.WriteString(fmt.Sprintf("\n Field: %s", e.Field)) + } + + return sb.String() +} + +// NewResourceNotFoundError creates an error for unknown resource types. +func NewResourceNotFoundError(apiVersion, kind string) *ValidationError { + return &ValidationError{ + Code: ErrorCodeResourceNotFound, + Message: fmt.Sprintf("Resource %s/%s does not exist in the cluster", apiVersion, kind), + } +} + +// NewInvalidFieldError creates an error for invalid fields. +func NewInvalidFieldError(field, resourceKind string) *ValidationError { + return &ValidationError{ + Code: ErrorCodeInvalidField, + Message: fmt.Sprintf("Invalid field %q in %s", field, resourceKind), + Field: field, + } +} + +// NewPermissionDeniedError creates an error for RBAC permission failures. +func NewPermissionDeniedError(verb, resource, namespace string) *ValidationError { + var msg string + if namespace != "" { + msg = fmt.Sprintf("Cannot %s %s in namespace %q", verb, resource, namespace) + } else { + msg = fmt.Sprintf("Cannot %s %s (cluster-scoped)", verb, resource) + } + + return &ValidationError{ + Code: ErrorCodePermissionDenied, + Message: msg, + } +} + +// NewInvalidManifestError creates an error for malformed manifests. +func NewInvalidManifestError(reason string) *ValidationError { + return &ValidationError{ + Code: ErrorCodeInvalidManifest, + Message: fmt.Sprintf("Invalid resource manifest: %s", reason), + } +} + +// FormatValidationErrors formats multiple validation errors into a single string. +func FormatValidationErrors(errors []*ValidationError) string { + if len(errors) == 0 { + return "" + } + + var sb strings.Builder + for i, err := range errors { + if i > 0 { + sb.WriteString("\n\n") + } + sb.WriteString(err.Error()) + } + return sb.String() +} + +// FormatResourceName creates a human-readable resource identifier from GVR. +func FormatResourceName(gvr *schema.GroupVersionResource) string { + if gvr == nil { + return "unknown" + } + if gvr.Group == "" { + return gvr.Resource + } + return gvr.Resource + "." + gvr.Group +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 668cb6dd2..bac6b48f9 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -90,6 +90,10 @@ type StaticConfig struct { // These can also be configured via OTEL_* environment variables. Telemetry TelemetryConfig `toml:"telemetry,omitempty"` + // Validation contains pre-execution validation configuration. + // When enabled, validates tool calls before execution to catch errors early. + Validation ValidationConfig `toml:"validation,omitempty"` + // Internal: parsed provider configs (not exposed to TOML package) parsedClusterProviderConfigs map[string]api.ExtendedConfig // Internal: parsed toolset configs (not exposed to TOML package) @@ -341,3 +345,7 @@ func (c *StaticConfig) GetStsAudience() string { func (c *StaticConfig) GetStsScopes() []string { return c.StsScopes } + +func (c *StaticConfig) GetValidationConfig() api.ValidationConfig { + return &c.Validation +} diff --git a/pkg/config/validation_config.go b/pkg/config/validation_config.go new file mode 100644 index 000000000..ab1badb3f --- /dev/null +++ b/pkg/config/validation_config.go @@ -0,0 +1,28 @@ +package config + +import ( + "os" + "strconv" +) + +// ValidationConfig contains pre-execution validation configuration. +// When enabled, validates tool calls before execution +type ValidationConfig struct { + // Defaults to false. + Enabled *bool `toml:"enabled,omitempty"` +} + +// IsEnabled returns true if validation is enabled. +// Environment variable MCP_VALIDATION_ENABLED takes precedence over config. +// Defaults to false. +func (c *ValidationConfig) IsEnabled() bool { + if v := os.Getenv("MCP_VALIDATION_ENABLED"); v != "" { + if b, err := strconv.ParseBool(v); err == nil { + return b + } + } + if c.Enabled != nil { + return *c.Enabled + } + return false +} diff --git a/pkg/kubernetes/auth.go b/pkg/kubernetes/auth.go new file mode 100644 index 000000000..311b9285e --- /dev/null +++ b/pkg/kubernetes/auth.go @@ -0,0 +1,54 @@ +package kubernetes + +import ( + "context" + + authv1 "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/klog/v2" +) + +// CanI checks if the current identity can perform verb on resource. +// Uses SelfSubjectAccessReview to pre-check RBAC permissions. +func CanI( + ctx context.Context, + authClient authv1client.AuthorizationV1Interface, + gvr *schema.GroupVersionResource, + namespace, resourceName, verb string, +) (bool, error) { + if authClient == nil { + return true, nil + } + + accessReview := &authv1.SelfSubjectAccessReview{ + Spec: authv1.SelfSubjectAccessReviewSpec{ + ResourceAttributes: &authv1.ResourceAttributes{ + Namespace: namespace, + Verb: verb, + Group: gvr.Group, + Version: gvr.Version, + Resource: gvr.Resource, + Name: resourceName, + }, + }, + } + + response, err := authClient.SelfSubjectAccessReviews().Create(ctx, accessReview, metav1.CreateOptions{}) + if err != nil { + return false, err + } + + if klog.V(5).Enabled() { + if response.Status.Allowed { + klog.V(5).Infof("RBAC check: allowed %s on %s/%s in %s", + verb, gvr.Group, gvr.Resource, namespace) + } else { + klog.V(5).Infof("RBAC check: denied %s on %s/%s in %s: %s", + verb, gvr.Group, gvr.Resource, namespace, response.Status.Reason) + } + } + + return response.Status.Allowed, nil +} diff --git a/pkg/kubernetes/kubernetes.go b/pkg/kubernetes/kubernetes.go index 3cd47c3b4..5b7e5d3e9 100644 --- a/pkg/kubernetes/kubernetes.go +++ b/pkg/kubernetes/kubernetes.go @@ -12,6 +12,7 @@ import ( "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" + authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" _ "k8s.io/client-go/plugin/pkg/client/auth/oidc" "k8s.io/client-go/rest" "k8s.io/client-go/restmapper" @@ -50,21 +51,31 @@ type Kubernetes struct { var _ api.KubernetesClient = (*Kubernetes)(nil) -func NewKubernetes(config api.BaseConfig, clientCmdConfig clientcmd.ClientConfig, restConfig *rest.Config) (*Kubernetes, error) { +func NewKubernetes(baseConfig api.BaseConfig, clientCmdConfig clientcmd.ClientConfig, restConfig *rest.Config) (*Kubernetes, error) { k := &Kubernetes{ - config: config, + config: baseConfig, clientCmdConfig: clientCmdConfig, restConfig: rest.CopyConfig(restConfig), } if k.restConfig.UserAgent == "" { k.restConfig.UserAgent = rest.DefaultKubernetesUserAgent() } + + // Extract validation config if available + var validationConfig api.ValidationConfig + if vcp, ok := baseConfig.(api.ValidationConfigProvider); ok { + validationConfig = vcp.GetValidationConfig() + } + k.restConfig.Wrap(func(original http.RoundTripper) http.RoundTripper { - return &AccessControlRoundTripper{ - delegate: original, - deniedResourcesProvider: config, - restMapperProvider: func() meta.RESTMapper { return k.restMapper }, - } + return NewValidationRoundTripper(ValidationRoundTripperConfig{ + Delegate: original, + DeniedResourcesProvider: baseConfig, + RestMapperProvider: func() meta.RESTMapper { return k.restMapper }, + DiscoveryProvider: func() discovery.DiscoveryInterface { return k.discoveryClient }, + AuthClientProvider: func() authv1client.AuthorizationV1Interface { return k.AuthorizationV1() }, + ValidatorConfig: validationConfig, + }) }) k.restConfig.Wrap(func(original http.RoundTripper) http.RoundTripper { return &UserAgentRoundTripper{delegate: original} diff --git a/pkg/kubernetes/rbac_validator.go b/pkg/kubernetes/rbac_validator.go new file mode 100644 index 000000000..450330f5c --- /dev/null +++ b/pkg/kubernetes/rbac_validator.go @@ -0,0 +1,52 @@ +package kubernetes + +import ( + "context" + + "github.com/containers/kubernetes-mcp-server/pkg/api" + authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/klog/v2" +) + +// RBACValidator pre-checks RBAC permissions before execution. +type RBACValidator struct { + authClientProvider func() authv1client.AuthorizationV1Interface +} + +// NewRBACValidator creates a new RBAC validator. +func NewRBACValidator(authClientProvider func() authv1client.AuthorizationV1Interface) *RBACValidator { + return &RBACValidator{ + authClientProvider: authClientProvider, + } +} + +func (v *RBACValidator) Name() string { + return "rbac" +} + +func (v *RBACValidator) Validate(ctx context.Context, req *api.HTTPValidationRequest) error { + if req.GVR == nil || req.Verb == "" { + return nil + } + + authClient := v.authClientProvider() + if authClient == nil { + return nil + } + + allowed, err := CanI(ctx, authClient, req.GVR, req.Namespace, req.ResourceName, req.Verb) + if err != nil { + klog.V(4).Infof("RBAC pre-validation failed with error: %v", err) + return nil + } + + if !allowed { + return api.NewPermissionDeniedError( + req.Verb, + api.FormatResourceName(req.GVR), + req.Namespace, + ) + } + + return nil +} diff --git a/pkg/kubernetes/rbac_validator_test.go b/pkg/kubernetes/rbac_validator_test.go new file mode 100644 index 000000000..28328bbc6 --- /dev/null +++ b/pkg/kubernetes/rbac_validator_test.go @@ -0,0 +1,155 @@ +package kubernetes + +import ( + "context" + "errors" + "testing" + + "github.com/containers/kubernetes-mcp-server/pkg/api" + "github.com/stretchr/testify/suite" + authv1 "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/client-go/rest" +) + +type mockSelfSubjectAccessReviewInterface struct { + allowed bool + err error +} + +func (m *mockSelfSubjectAccessReviewInterface) Create(ctx context.Context, review *authv1.SelfSubjectAccessReview, opts metav1.CreateOptions) (*authv1.SelfSubjectAccessReview, error) { + if m.err != nil { + return nil, m.err + } + review.Status.Allowed = m.allowed + return review, nil +} + +type mockAuthorizationV1Interface struct { + authv1client.AuthorizationV1Interface + selfSubjectAccessReview *mockSelfSubjectAccessReviewInterface +} + +func (m *mockAuthorizationV1Interface) RESTClient() rest.Interface { + return nil +} + +func (m *mockAuthorizationV1Interface) SelfSubjectAccessReviews() authv1client.SelfSubjectAccessReviewInterface { + return m.selfSubjectAccessReview +} + +type RBACValidatorTestSuite struct { + suite.Suite +} + +func (s *RBACValidatorTestSuite) TestName() { + v := NewRBACValidator(nil) + s.Equal("rbac", v.Name()) +} + +func (s *RBACValidatorTestSuite) TestValidate() { + testCases := []struct { + name string + req *api.HTTPValidationRequest + authClient authv1client.AuthorizationV1Interface + expectError bool + errorCode api.ValidationErrorCode + }{ + { + name: "nil GVR passes validation", + req: &api.HTTPValidationRequest{GVR: nil, Verb: "get"}, + authClient: nil, + expectError: false, + }, + { + name: "empty verb passes validation", + req: &api.HTTPValidationRequest{ + GVR: &schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, + Verb: "", + }, + authClient: nil, + expectError: false, + }, + { + name: "nil auth client passes validation", + req: &api.HTTPValidationRequest{ + GVR: &schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, + Verb: "get", + }, + authClient: nil, + expectError: false, + }, + { + name: "allowed action passes validation", + req: &api.HTTPValidationRequest{ + GVR: &schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, + Verb: "get", + Namespace: "default", + }, + authClient: &mockAuthorizationV1Interface{ + selfSubjectAccessReview: &mockSelfSubjectAccessReviewInterface{allowed: true}, + }, + expectError: false, + }, + { + name: "denied action fails validation", + req: &api.HTTPValidationRequest{ + GVR: &schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, + Verb: "delete", + Namespace: "kube-system", + }, + authClient: &mockAuthorizationV1Interface{ + selfSubjectAccessReview: &mockSelfSubjectAccessReviewInterface{allowed: false}, + }, + expectError: true, + errorCode: api.ErrorCodePermissionDenied, + }, + { + name: "auth client error passes validation", + req: &api.HTTPValidationRequest{ + GVR: &schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, + Verb: "get", + Namespace: "default", + }, + authClient: &mockAuthorizationV1Interface{ + selfSubjectAccessReview: &mockSelfSubjectAccessReviewInterface{err: errors.New("connection refused")}, + }, + expectError: false, + }, + { + name: "cluster-scoped resource denied", + req: &api.HTTPValidationRequest{ + GVR: &schema.GroupVersionResource{Group: "", Version: "v1", Resource: "nodes"}, + Verb: "delete", + Namespace: "", + }, + authClient: &mockAuthorizationV1Interface{ + selfSubjectAccessReview: &mockSelfSubjectAccessReviewInterface{allowed: false}, + }, + expectError: true, + errorCode: api.ErrorCodePermissionDenied, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + v := NewRBACValidator(func() authv1client.AuthorizationV1Interface { return tc.authClient }) + err := v.Validate(context.Background(), tc.req) + + if tc.expectError { + s.Error(err) + if ve, ok := err.(*api.ValidationError); ok { + s.Equal(tc.errorCode, ve.Code) + } + } else { + s.NoError(err) + } + }) + } +} + +func TestRBACValidator(t *testing.T) { + suite.Run(t, new(RBACValidatorTestSuite)) +} diff --git a/pkg/kubernetes/resource_validator.go b/pkg/kubernetes/resource_validator.go new file mode 100644 index 000000000..eda466ff1 --- /dev/null +++ b/pkg/kubernetes/resource_validator.go @@ -0,0 +1,55 @@ +package kubernetes + +import ( + "context" + + "github.com/containers/kubernetes-mcp-server/pkg/api" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/klog/v2" +) + +// ResourceValidator validates that resource types (GVK) exist in the cluster. +type ResourceValidator struct { + restMapperProvider func() meta.RESTMapper +} + +// NewResourceValidator creates a new resource validator. +func NewResourceValidator(restMapperProvider func() meta.RESTMapper) *ResourceValidator { + return &ResourceValidator{ + restMapperProvider: restMapperProvider, + } +} + +func (v *ResourceValidator) Name() string { + return "resource" +} + +func (v *ResourceValidator) Validate(ctx context.Context, req *api.HTTPValidationRequest) error { + if req.GVK == nil { + return nil + } + + restMapper := v.restMapperProvider() + if restMapper == nil { + return nil + } + + _, err := restMapper.RESTMapping( + schema.GroupKind{Group: req.GVK.Group, Kind: req.GVK.Kind}, + req.GVK.Version, + ) + + if err != nil { + if meta.IsNoMatchError(err) { + return api.NewResourceNotFoundError( + req.GVK.GroupVersion().String(), + req.GVK.Kind, + ) + } + klog.V(4).Infof("RESTMapper error for %v: %v", req.GVK, err) + return nil + } + + return nil +} diff --git a/pkg/kubernetes/resource_validator_test.go b/pkg/kubernetes/resource_validator_test.go new file mode 100644 index 000000000..a56965d0f --- /dev/null +++ b/pkg/kubernetes/resource_validator_test.go @@ -0,0 +1,139 @@ +package kubernetes + +import ( + "context" + "testing" + + "github.com/containers/kubernetes-mcp-server/pkg/api" + "github.com/stretchr/testify/suite" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +type mockRESTMapper struct { + mappings map[schema.GroupKind]*meta.RESTMapping +} + +func (m *mockRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*meta.RESTMapping, error) { + if mapping, ok := m.mappings[gk]; ok { + return mapping, nil + } + return nil, &meta.NoResourceMatchError{PartialResource: schema.GroupVersionResource{Group: gk.Group, Resource: gk.Kind}} +} + +func (m *mockRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*meta.RESTMapping, error) { + return nil, nil +} + +func (m *mockRESTMapper) KindFor(resource schema.GroupVersionResource) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{}, nil +} + +func (m *mockRESTMapper) KindsFor(resource schema.GroupVersionResource) ([]schema.GroupVersionKind, error) { + return nil, nil +} + +func (m *mockRESTMapper) ResourceFor(input schema.GroupVersionResource) (schema.GroupVersionResource, error) { + return schema.GroupVersionResource{}, nil +} + +func (m *mockRESTMapper) ResourcesFor(input schema.GroupVersionResource) ([]schema.GroupVersionResource, error) { + return nil, nil +} + +func (m *mockRESTMapper) ResourceSingularizer(resource string) (singular string, err error) { + return "", nil +} + +type ResourceValidatorTestSuite struct { + suite.Suite +} + +func (s *ResourceValidatorTestSuite) TestName() { + v := NewResourceValidator(nil) + s.Equal("resource", v.Name()) +} + +func (s *ResourceValidatorTestSuite) TestValidate() { + testCases := []struct { + name string + req *api.HTTPValidationRequest + mapper meta.RESTMapper + expectError bool + errorCode api.ValidationErrorCode + }{ + { + name: "nil GVK passes validation", + req: &api.HTTPValidationRequest{GVK: nil}, + mapper: nil, + expectError: false, + }, + { + name: "nil RESTMapper passes validation", + req: &api.HTTPValidationRequest{ + GVK: &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}, + }, + mapper: nil, + expectError: false, + }, + { + name: "existing resource passes validation", + req: &api.HTTPValidationRequest{ + GVK: &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}, + }, + mapper: &mockRESTMapper{ + mappings: map[schema.GroupKind]*meta.RESTMapping{ + {Group: "", Kind: "Pod"}: { + Resource: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, + }, + }, + }, + expectError: false, + }, + { + name: "non-existent resource fails validation", + req: &api.HTTPValidationRequest{ + GVK: &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "FakeResource"}, + }, + mapper: &mockRESTMapper{ + mappings: map[schema.GroupKind]*meta.RESTMapping{}, + }, + expectError: true, + errorCode: api.ErrorCodeResourceNotFound, + }, + { + name: "apps group resource passes validation", + req: &api.HTTPValidationRequest{ + GVK: &schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, + }, + mapper: &mockRESTMapper{ + mappings: map[schema.GroupKind]*meta.RESTMapping{ + {Group: "apps", Kind: "Deployment"}: { + Resource: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, + }, + }, + }, + expectError: false, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + v := NewResourceValidator(func() meta.RESTMapper { return tc.mapper }) + err := v.Validate(context.Background(), tc.req) + + if tc.expectError { + s.Error(err) + if ve, ok := err.(*api.ValidationError); ok { + s.Equal(tc.errorCode, ve.Code) + } + } else { + s.NoError(err) + } + }) + } +} + +func TestResourceValidator(t *testing.T) { + suite.Run(t, new(ResourceValidatorTestSuite)) +} diff --git a/pkg/kubernetes/resources.go b/pkg/kubernetes/resources.go index 513a51be2..3769361bd 100644 --- a/pkg/kubernetes/resources.go +++ b/pkg/kubernetes/resources.go @@ -11,7 +11,6 @@ import ( "github.com/containers/kubernetes-mcp-server/pkg/api" "github.com/containers/kubernetes-mcp-server/pkg/version" - authv1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1" @@ -230,19 +229,6 @@ func (c *Core) supportsGroupVersion(groupVersion string) bool { } func (c *Core) canIUse(ctx context.Context, gvr *schema.GroupVersionResource, namespace, verb string) bool { - accessReviews := c.AuthorizationV1().SelfSubjectAccessReviews() - response, err := accessReviews.Create(ctx, &authv1.SelfSubjectAccessReview{ - Spec: authv1.SelfSubjectAccessReviewSpec{ResourceAttributes: &authv1.ResourceAttributes{ - Namespace: namespace, - Verb: verb, - Group: gvr.Group, - Version: gvr.Version, - Resource: gvr.Resource, - }}, - }, metav1.CreateOptions{}) - if err != nil { - // TODO: maybe return the error too - return false - } - return response.Status.Allowed + allowed, _ := CanI(ctx, c.AuthorizationV1(), gvr, namespace, "", verb) + return allowed } diff --git a/pkg/kubernetes/schema_validator.go b/pkg/kubernetes/schema_validator.go new file mode 100644 index 000000000..04bbcb44d --- /dev/null +++ b/pkg/kubernetes/schema_validator.go @@ -0,0 +1,130 @@ +package kubernetes + +import ( + "context" + "strings" + "sync" + "time" + + "github.com/containers/kubernetes-mcp-server/pkg/api" + "k8s.io/client-go/discovery" + "k8s.io/klog/v2" + kubectlopenapi "k8s.io/kubectl/pkg/util/openapi" + kubectlvalidation "k8s.io/kubectl/pkg/validation" +) + +const schemaCacheTTL = 15 * time.Minute + +// SchemaValidator validates resource manifests against the OpenAPI schema. +type SchemaValidator struct { + discoveryClientProvider func() discovery.DiscoveryInterface + kubectlValidator kubectlvalidation.Schema + validatorMu sync.Mutex + validatorCachedAt time.Time +} + +// NewSchemaValidator creates a new schema validator. +func NewSchemaValidator(discoveryClientProvider func() discovery.DiscoveryInterface) *SchemaValidator { + return &SchemaValidator{ + discoveryClientProvider: discoveryClientProvider, + } +} + +func (v *SchemaValidator) Name() string { + return "schema" +} + +func (v *SchemaValidator) Validate(ctx context.Context, req *api.HTTPValidationRequest) error { + if req.GVK == nil || len(req.Body) == 0 { + return nil + } + + // Only validate for create/update operations + if req.Verb != "create" && req.Verb != "update" && req.Verb != "patch" { + return nil + } + + validator, err := v.getValidator() + if err != nil { + klog.V(4).Infof("Failed to get schema validator: %v", err) + return nil + } + + if validator == nil { + return nil + } + + err = validator.ValidateBytes(req.Body) + if err != nil { + // Check if this is a parsing error (e.g., binary data that can't be parsed as YAML) + // In that case, skip validation rather than blocking the request + errMsg := err.Error() + if strings.Contains(errMsg, "yaml:") || strings.Contains(errMsg, "json:") { + klog.V(4).Infof("Schema validation skipped due to parsing error: %v", err) + return nil + } + return convertKubectlValidationError(err) + } + + return nil +} + +// openAPIResourcesAdapter adapts CachedOpenAPIParser to OpenAPIResourcesGetter interface. +type openAPIResourcesAdapter struct { + parser *kubectlopenapi.CachedOpenAPIParser +} + +func (a *openAPIResourcesAdapter) OpenAPISchema() (kubectlopenapi.Resources, error) { + return a.parser.Parse() +} + +func (v *SchemaValidator) getValidator() (kubectlvalidation.Schema, error) { + v.validatorMu.Lock() + defer v.validatorMu.Unlock() + + if v.kubectlValidator != nil && time.Since(v.validatorCachedAt) <= schemaCacheTTL { + return v.kubectlValidator, nil + } + + discoveryClient := v.discoveryClientProvider() + if discoveryClient == nil { + return nil, nil + } + + openAPIClient, ok := discoveryClient.(discovery.OpenAPISchemaInterface) + if !ok { + klog.V(4).Infof("Discovery client does not support OpenAPI schema") + return nil, nil + } + + parser := kubectlopenapi.NewOpenAPIParser(openAPIClient) + adapter := &openAPIResourcesAdapter{parser: parser} + + v.kubectlValidator = kubectlvalidation.NewSchemaValidation(adapter) + v.validatorCachedAt = time.Now() + + return v.kubectlValidator, nil +} + +func convertKubectlValidationError(err error) *api.ValidationError { + if err == nil { + return nil + } + + errMsg := err.Error() + + var field string + if strings.Contains(errMsg, "unknown field") { + if start := strings.Index(errMsg, "\""); start != -1 { + if end := strings.Index(errMsg[start+1:], "\""); end != -1 { + field = errMsg[start+1 : start+1+end] + } + } + } + + return &api.ValidationError{ + Code: api.ErrorCodeInvalidField, + Message: errMsg, + Field: field, + } +} diff --git a/pkg/kubernetes/validation_round_tripper.go b/pkg/kubernetes/validation_round_tripper.go new file mode 100644 index 000000000..314b31bd0 --- /dev/null +++ b/pkg/kubernetes/validation_round_tripper.go @@ -0,0 +1,236 @@ +package kubernetes + +import ( + "bytes" + "fmt" + "io" + "net/http" + "strings" + + "github.com/containers/kubernetes-mcp-server/pkg/api" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/discovery" + authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/klog/v2" +) + +// ValidationRoundTripper intercepts HTTP requests to validate them before they reach the K8s API. +// It extends the AccessControlRoundTripper pattern with additional validator chain execution. +type ValidationRoundTripper struct { + delegate http.RoundTripper + deniedResourcesProvider api.DeniedResourcesProvider + restMapperProvider func() meta.RESTMapper + discoveryProvider func() discovery.DiscoveryInterface + authClientProvider func() authv1client.AuthorizationV1Interface + validatorConfig api.ValidationConfig + validators []api.HTTPValidator +} + +// ValidationRoundTripperConfig configures the ValidationRoundTripper. +type ValidationRoundTripperConfig struct { + Delegate http.RoundTripper + DeniedResourcesProvider api.DeniedResourcesProvider + RestMapperProvider func() meta.RESTMapper + DiscoveryProvider func() discovery.DiscoveryInterface + AuthClientProvider func() authv1client.AuthorizationV1Interface + ValidatorConfig api.ValidationConfig +} + +// NewValidationRoundTripper creates a new ValidationRoundTripper. +func NewValidationRoundTripper(cfg ValidationRoundTripperConfig) *ValidationRoundTripper { + rt := &ValidationRoundTripper{ + delegate: cfg.Delegate, + deniedResourcesProvider: cfg.DeniedResourcesProvider, + restMapperProvider: cfg.RestMapperProvider, + discoveryProvider: cfg.DiscoveryProvider, + authClientProvider: cfg.AuthClientProvider, + validatorConfig: cfg.ValidatorConfig, + } + + // Create validators with providers (only if validation is enabled) + if cfg.ValidatorConfig != nil && cfg.ValidatorConfig.IsEnabled() { + rt.validators = CreateValidators(ValidatorProviders{ + RestMapper: cfg.RestMapperProvider, + Discovery: cfg.DiscoveryProvider, + AuthClient: cfg.AuthClientProvider, + }) + } + + return rt +} + +func (rt *ValidationRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + gvr, ok := parseURLToGVR(req.URL.Path) + if !ok { + return rt.delegate.RoundTrip(req) + } + + // Access control checks (from original AccessControlRoundTripper) - always enforced + restMapper := rt.restMapperProvider() + if restMapper == nil { + return nil, fmt.Errorf("failed to make request: restMapper not initialized") + } + + gvk, err := restMapper.KindFor(gvr) + if err != nil { + return nil, fmt.Errorf("failed to make request: failed to get kind for gvr %v: %w", gvr, err) + } + + if !rt.isAllowed(gvk) { + return nil, fmt.Errorf("resource not allowed: %s", gvk.String()) + } + + // Skip validators if disabled or if this is SelfSubjectAccessReview (used by RBAC validator) + skipValidation := rt.validatorConfig == nil || !rt.validatorConfig.IsEnabled() + skipValidation = skipValidation || (gvr.Group == "authorization.k8s.io" && gvr.Resource == "selfsubjectaccessreviews") + if skipValidation { + return rt.delegate.RoundTrip(req) + } + + // Extract namespace and resource name from URL + namespace, resourceName := parseURLToNamespaceAndName(req.URL.Path) + + // Map HTTP method to K8s verb + verb := httpMethodToVerb(req.Method, req.URL.Path) + + // Build validation request + validationReq := &api.HTTPValidationRequest{ + GVR: &gvr, + GVK: &gvk, + HTTPMethod: req.Method, + Verb: verb, + Namespace: namespace, + ResourceName: resourceName, + Path: req.URL.Path, + } + + // Buffer body for POST/PUT/PATCH (needed for schema validation) + if req.Body != nil && (req.Method == "POST" || req.Method == "PUT" || req.Method == "PATCH") { + body, readErr := io.ReadAll(req.Body) + _ = req.Body.Close() + if readErr != nil { + return nil, fmt.Errorf("failed to read request body: %w", readErr) + } + req.Body = io.NopCloser(bytes.NewReader(body)) + validationReq.Body = body + } + + // Run validators + for _, v := range rt.validators { + if validationErr := v.Validate(req.Context(), validationReq); validationErr != nil { + if ve, ok := validationErr.(*api.ValidationError); ok { + klog.V(4).Infof("Validation failed [%s]: %v", v.Name(), ve) + } + return nil, validationErr + } + } + + return rt.delegate.RoundTrip(req) +} + +// isAllowed checks the resource is in denied list or not. +func (rt *ValidationRoundTripper) isAllowed(gvk schema.GroupVersionKind) bool { + if rt.deniedResourcesProvider == nil { + return true + } + + for _, val := range rt.deniedResourcesProvider.GetDeniedResources() { + if val.Kind == "" { + if gvk.Group == val.Group && gvk.Version == val.Version { + return false + } + } + if gvk.Group == val.Group && gvk.Version == val.Version && gvk.Kind == val.Kind { + return false + } + } + + return true +} + +// parseURLToNamespaceAndName extracts namespace and resource name from K8s API URL. +func parseURLToNamespaceAndName(path string) (namespace, name string) { + parts := strings.Split(strings.Trim(path, "/"), "/") + + // Find "namespaces" segment + for i, part := range parts { + if part == "namespaces" && i+1 < len(parts) { + namespace = parts[i+1] + break + } + } + + // Find resource type position and check for name after it + resourceIdx := findResourceTypeIndex(parts) + if resourceIdx >= 0 && resourceIdx+1 < len(parts) { + name = parts[resourceIdx+1] + } + + return namespace, name +} + +// findResourceTypeIndex returns the index of the resource type segment in the URL parts. +// Uses the standard K8s API URL structure to locate the resource type. +func findResourceTypeIndex(parts []string) int { + if len(parts) == 0 { + return -1 + } + + switch parts[0] { + case "api": + // /api/v1/{resource} or /api/v1/namespaces/{ns}/{resource} + if len(parts) < 3 { + return -1 + } + if parts[2] == "namespaces" && len(parts) > 4 { + return 4 + } + return 2 + case "apis": + // /apis/{group}/{version}/{resource} or /apis/{group}/{version}/namespaces/{ns}/{resource} + if len(parts) < 4 { + return -1 + } + if parts[3] == "namespaces" && len(parts) > 5 { + return 5 + } + return 3 + } + return -1 +} + +// httpMethodToVerb maps HTTP method to K8s RBAC verb. +func httpMethodToVerb(method, path string) string { + switch method { + case "GET": + if isCollectionPath(path) { + return "list" + } + return "get" + case "POST": + return "create" + case "PUT": + return "update" + case "PATCH": + return "patch" + case "DELETE": + if isCollectionPath(path) { + return "deletecollection" + } + return "delete" + default: + return strings.ToLower(method) + } +} + +// isCollectionPath checks if the path targets a collection (list) vs a specific resource. +func isCollectionPath(path string) bool { + parts := strings.Split(strings.Trim(path, "/"), "/") + resourceIdx := findResourceTypeIndex(parts) + if resourceIdx < 0 { + return false + } + // Collection path if the resource type is the last segment (no name after it) + return resourceIdx == len(parts)-1 +} diff --git a/pkg/kubernetes/validator_registry.go b/pkg/kubernetes/validator_registry.go new file mode 100644 index 000000000..e50b0b2f0 --- /dev/null +++ b/pkg/kubernetes/validator_registry.go @@ -0,0 +1,46 @@ +package kubernetes + +import ( + "github.com/containers/kubernetes-mcp-server/pkg/api" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/client-go/discovery" + authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" +) + +// ValidatorProviders holds the providers needed to create validators. +type ValidatorProviders struct { + RestMapper func() meta.RESTMapper + Discovery func() discovery.DiscoveryInterface + AuthClient func() authv1client.AuthorizationV1Interface +} + +// ValidatorFactory creates a validator given the providers. +type ValidatorFactory func(ValidatorProviders) api.HTTPValidator + +var validatorFactories []ValidatorFactory + +// RegisterValidator adds a validator factory to the registry. +func RegisterValidator(factory ValidatorFactory) { + validatorFactories = append(validatorFactories, factory) +} + +// CreateValidators creates all registered validators with the given providers. +func CreateValidators(providers ValidatorProviders) []api.HTTPValidator { + validators := make([]api.HTTPValidator, 0, len(validatorFactories)) + for _, factory := range validatorFactories { + validators = append(validators, factory(providers)) + } + return validators +} + +func init() { + RegisterValidator(func(p ValidatorProviders) api.HTTPValidator { + return NewResourceValidator(p.RestMapper) + }) + RegisterValidator(func(p ValidatorProviders) api.HTTPValidator { + return NewSchemaValidator(p.Discovery) + }) + RegisterValidator(func(p ValidatorProviders) api.HTTPValidator { + return NewRBACValidator(p.AuthClient) + }) +} diff --git a/pkg/kubernetes/validator_registry_test.go b/pkg/kubernetes/validator_registry_test.go new file mode 100644 index 000000000..c99596b85 --- /dev/null +++ b/pkg/kubernetes/validator_registry_test.go @@ -0,0 +1,53 @@ +package kubernetes + +import ( + "testing" + + "github.com/stretchr/testify/suite" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/client-go/discovery" + authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" +) + +type ValidatorRegistryTestSuite struct { + suite.Suite +} + +func (s *ValidatorRegistryTestSuite) TestCreateValidatorsReturnsRegisteredValidators() { + providers := ValidatorProviders{ + RestMapper: func() meta.RESTMapper { return nil }, + Discovery: func() discovery.DiscoveryInterface { return nil }, + AuthClient: func() authv1client.AuthorizationV1Interface { return nil }, + } + + validators := CreateValidators(providers) + + s.GreaterOrEqual(len(validators), 3, "Expected at least 3 validators (resource, schema, rbac)") + + names := make(map[string]bool) + for _, v := range validators { + names[v.Name()] = true + } + + s.True(names["resource"], "Expected resource validator to be registered") + s.True(names["schema"], "Expected schema validator to be registered") + s.True(names["rbac"], "Expected rbac validator to be registered") +} + +func (s *ValidatorRegistryTestSuite) TestCreateValidatorsWithNilProviders() { + providers := ValidatorProviders{ + RestMapper: nil, + Discovery: nil, + AuthClient: nil, + } + + // Should not panic + s.NotPanics(func() { + validators := CreateValidators(providers) + s.NotEmpty(validators, "Expected validators to be created even with nil providers") + }) +} + +func TestValidatorRegistry(t *testing.T) { + suite.Run(t, new(ValidatorRegistryTestSuite)) +} diff --git a/pkg/mcp/events_test.go b/pkg/mcp/events_test.go index c034122ac..e18ac6377 100644 --- a/pkg/mcp/events_test.go +++ b/pkg/mcp/events_test.go @@ -3,7 +3,6 @@ package mcp import ( "strings" "testing" - "time" "github.com/BurntSushi/toml" "github.com/mark3labs/mcp-go/mcp" @@ -151,17 +150,12 @@ func (s *EventsSuite) TestEventsListForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("events_list (forbidden)", func() { - capture := s.StartCapturingLogNotifications() toolResult, _ := s.CallTool("events_list", map[string]interface{}{}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", - "error message should indicate forbidden") - }) - s.Run("sends log notification", func() { - logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) - s.Equal("error", logNotification.Level, "forbidden errors should log at error level") - s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") + msg := toolResult.Content[0].(mcp.TextContent).Text + s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), + "error message should indicate forbidden or permission denied, got: %s", msg) }) }) } diff --git a/pkg/mcp/mcp.go b/pkg/mcp/mcp.go index 1e5ac1b6d..30fa77c8e 100644 --- a/pkg/mcp/mcp.go +++ b/pkg/mcp/mcp.go @@ -110,6 +110,7 @@ func NewServer(configuration Configuration, targetProvider internalk8s.Provider) s.server.AddReceivingMiddleware(userAgentPropagationMiddleware(version.BinaryName, version.Version)) s.server.AddReceivingMiddleware(toolCallLoggingMiddleware) s.server.AddReceivingMiddleware(s.metricsMiddleware()) + err = s.reloadToolsets() if err != nil { return nil, err diff --git a/pkg/mcp/namespaces_test.go b/pkg/mcp/namespaces_test.go index de95aa8f1..db0d979b8 100644 --- a/pkg/mcp/namespaces_test.go +++ b/pkg/mcp/namespaces_test.go @@ -3,8 +3,8 @@ package mcp import ( "regexp" "slices" + "strings" "testing" - "time" "github.com/BurntSushi/toml" "github.com/mark3labs/mcp-go/mcp" @@ -75,17 +75,12 @@ func (s *NamespacesSuite) TestNamespacesListForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("namespaces_list (forbidden)", func() { - capture := s.StartCapturingLogNotifications() toolResult, _ := s.CallTool("namespaces_list", map[string]interface{}{}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", - "error message should indicate forbidden") - }) - s.Run("sends log notification", func() { - logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) - s.Equal("error", logNotification.Level, "forbidden errors should log at error level") - s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") + msg := toolResult.Content[0].(mcp.TextContent).Text + s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), + "error message should indicate forbidden or permission denied, got: %s", msg) }) }) } diff --git a/pkg/mcp/pods_test.go b/pkg/mcp/pods_test.go index 918f9811a..92ead48c8 100644 --- a/pkg/mcp/pods_test.go +++ b/pkg/mcp/pods_test.go @@ -66,6 +66,10 @@ func (s *PodsSuite) TestPodsListInAllNamespaces() { } func (s *PodsSuite) TestPodsListInAllNamespacesUnauthorized() { + // Disable validation for this test - we're testing the tool's fallback behavior + // when cluster-wide list fails but namespace-scoped list succeeds + validationDisabled := false + s.Cfg.Validation.Enabled = &validationDisabled s.InitMcpClient() defer restoreAuth(s.T().Context()) client := kubernetes.NewForConfigOrDie(envTestRestConfig) @@ -194,17 +198,12 @@ func (s *PodsSuite) TestPodsListForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("pods_list (forbidden)", func() { - capture := s.StartCapturingLogNotifications() toolResult, _ := s.CallTool("pods_list", map[string]interface{}{}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", - "error message should indicate forbidden") - }) - s.Run("sends log notification", func() { - logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) - s.Equal("error", logNotification.Level, "forbidden errors should log at error level") - s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") + msg := toolResult.Content[0].(mcp.TextContent).Text + s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), + "error message should indicate forbidden or permission denied, got: %s", msg) }) }) } diff --git a/pkg/mcp/resources_test.go b/pkg/mcp/resources_test.go index 394690b0a..5b02fe541 100644 --- a/pkg/mcp/resources_test.go +++ b/pkg/mcp/resources_test.go @@ -49,8 +49,11 @@ func (s *ResourcesSuite) TestResourcesList() { s.Run("resources_list with nonexistent apiVersion returns error", func() { toolResult, _ := s.CallTool("resources_list", map[string]interface{}{"apiVersion": "custom.non.existent.example.com/v1", "kind": "Custom"}) s.Truef(toolResult.IsError, "call tool should fail") - s.Equalf(`failed to list resources: no matches for kind "Custom" in version "custom.non.existent.example.com/v1"`, - toolResult.Content[0].(mcp.TextContent).Text, "invalid error message, got %v", toolResult.Content[0].(mcp.TextContent).Text) + msg := toolResult.Content[0].(mcp.TextContent).Text + // Accept both validation error and API error formats + hasValidationError := strings.Contains(msg, "RESOURCE_NOT_FOUND") && strings.Contains(msg, "Custom") + hasAPIError := strings.Contains(msg, "no matches for kind") + s.Truef(hasValidationError || hasAPIError, "error should indicate resource not found, got: %s", msg) }) s.Run("resources_list(apiVersion=v1, kind=Namespace) returns namespaces", func() { namespaces, err := s.CallTool("resources_list", map[string]interface{}{"apiVersion": "v1", "kind": "Namespace"}) @@ -207,17 +210,12 @@ func (s *ResourcesSuite) TestResourcesListForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("resources_list (forbidden)", func() { - capture := s.StartCapturingLogNotifications() toolResult, _ := s.CallTool("resources_list", map[string]interface{}{"apiVersion": "v1", "kind": "ConfigMap"}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", - "error message should indicate forbidden") - }) - s.Run("sends log notification", func() { - logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) - s.Equal("error", logNotification.Level, "forbidden errors should log at error level") - s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") + msg := toolResult.Content[0].(mcp.TextContent).Text + s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), + "error message should indicate forbidden or permission denied, got: %s", msg) }) }) } @@ -324,8 +322,11 @@ func (s *ResourcesSuite) TestResourcesGet() { s.Run("resources_get with nonexistent apiVersion returns error", func() { toolResult, _ := s.CallTool("resources_get", map[string]interface{}{"apiVersion": "custom.non.existent.example.com/v1", "kind": "Custom", "name": "a-custom"}) s.Truef(toolResult.IsError, "call tool should fail") - s.Equalf(`failed to get resource: no matches for kind "Custom" in version "custom.non.existent.example.com/v1"`, - toolResult.Content[0].(mcp.TextContent).Text, "invalid error message, got %v", toolResult.Content[0].(mcp.TextContent).Text) + msg := toolResult.Content[0].(mcp.TextContent).Text + // Accept both validation error and API error formats + hasValidationError := strings.Contains(msg, "RESOURCE_NOT_FOUND") && strings.Contains(msg, "Custom") + hasAPIError := strings.Contains(msg, "no matches for kind") + s.Truef(hasValidationError || hasAPIError, "error should indicate resource not found, got: %s", msg) }) s.Run("resources_get with missing name returns error", func() { toolResult, _ := s.CallTool("resources_get", map[string]interface{}{"apiVersion": "v1", "kind": "Namespace"}) @@ -589,18 +590,13 @@ func (s *ResourcesSuite) TestResourcesCreateOrUpdateForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("resources_create_or_update (forbidden)", func() { - capture := s.StartCapturingLogNotifications() configMapYaml := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a-forbidden-configmap\n namespace: default\n" toolResult, _ := s.CallTool("resources_create_or_update", map[string]interface{}{"resource": configMapYaml}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", - "error message should indicate forbidden") - }) - s.Run("sends log notification", func() { - logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) - s.Equal("error", logNotification.Level, "forbidden errors should log at error level") - s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") + msg := toolResult.Content[0].(mcp.TextContent).Text + s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), + "error message should indicate forbidden or permission denied, got: %s", msg) }) }) } @@ -630,8 +626,11 @@ func (s *ResourcesSuite) TestResourcesDelete() { s.Run("resources_delete with nonexistent apiVersion returns error", func() { toolResult, _ := s.CallTool("resources_delete", map[string]interface{}{"apiVersion": "custom.non.existent.example.com/v1", "kind": "Custom", "name": "a-custom"}) s.Truef(toolResult.IsError, "call tool should fail") - s.Equalf(`failed to delete resource: no matches for kind "Custom" in version "custom.non.existent.example.com/v1"`, - toolResult.Content[0].(mcp.TextContent).Text, "invalid error message, got %v", toolResult.Content[0].(mcp.TextContent).Text) + msg := toolResult.Content[0].(mcp.TextContent).Text + // Accept both validation error and API error formats + hasValidationError := strings.Contains(msg, "RESOURCE_NOT_FOUND") && strings.Contains(msg, "Custom") + hasAPIError := strings.Contains(msg, "no matches for kind") + s.Truef(hasValidationError || hasAPIError, "error should indicate resource not found, got: %s", msg) }) s.Run("resources_delete with missing name returns error", func() { toolResult, _ := s.CallTool("resources_delete", map[string]interface{}{"apiVersion": "v1", "kind": "Namespace"}) From f7aae2f5b2c23170e83b671fd1779b137ff29518 Mon Sep 17 00:00:00 2001 From: Nader Ziada Date: Tue, 17 Feb 2026 10:49:39 -0500 Subject: [PATCH 2/3] simplify config and merge into AccessControlRoundTripper Signed-off-by: Nader Ziada --- docs/VALIDATION.md | 22 +- pkg/api/config.go | 12 +- pkg/config/config.go | 11 +- pkg/config/validation_config.go | 28 --- pkg/kubernetes/accesscontrol_round_tripper.go | 157 +++++++++++- pkg/kubernetes/kubernetes.go | 11 +- pkg/kubernetes/validation_round_tripper.go | 236 ------------------ pkg/mcp/pods_test.go | 4 - 8 files changed, 173 insertions(+), 308 deletions(-) delete mode 100644 pkg/config/validation_config.go delete mode 100644 pkg/kubernetes/validation_round_tripper.go diff --git a/docs/VALIDATION.md b/docs/VALIDATION.md index d0346a3f3..10bc76f7d 100644 --- a/docs/VALIDATION.md +++ b/docs/VALIDATION.md @@ -26,30 +26,16 @@ The validation layer catches three types of issues: Validation is **disabled by default**. All validators (resource, schema, RBAC) run together when enabled. -### Environment Variables - -```bash -# Enable all validation -MCP_VALIDATION_ENABLED=true -``` - -### TOML Configuration - -Add a `[validation]` section to your config file: - ```toml -[validation] # Enable all validation (default: false) -enabled = true +validation_enabled = true ``` ### Configuration Reference -| TOML Field | Environment Variable | Default | Description | -|------------|---------------------|---------|-------------| -| `enabled` | `MCP_VALIDATION_ENABLED` | `false` | Enable/disable all validators | - -Environment variables take precedence over TOML config. +| TOML Field | Default | Description | +|------------|---------|-------------| +| `validation_enabled` | `false` | Enable/disable all validators | **Note:** The schema validator caches the OpenAPI schema for 15 minutes internally. diff --git a/pkg/api/config.go b/pkg/api/config.go index cf6b8e438..03900d391 100644 --- a/pkg/api/config.go +++ b/pkg/api/config.go @@ -53,15 +53,9 @@ type StsConfigProvider interface { GetStsScopes() []string } -// ValidationConfigProvider provides access to validation configuration. -// Implementations can return nil to use default validation settings. -type ValidationConfigProvider interface { - GetValidationConfig() ValidationConfig -} - -// ValidationConfig interface for validation settings. -type ValidationConfig interface { - IsEnabled() bool +// ValidationEnabledProvider provides access to validation enabled setting. +type ValidationEnabledProvider interface { + IsValidationEnabled() bool } type BaseConfig interface { diff --git a/pkg/config/config.go b/pkg/config/config.go index bac6b48f9..a4f8e53a1 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -90,9 +90,10 @@ type StaticConfig struct { // These can also be configured via OTEL_* environment variables. Telemetry TelemetryConfig `toml:"telemetry,omitempty"` - // Validation contains pre-execution validation configuration. - // When enabled, validates tool calls before execution to catch errors early. - Validation ValidationConfig `toml:"validation,omitempty"` + // ValidationEnabled enables pre-execution validation of tool calls. + // When enabled, validates resources, schemas, and RBAC before execution. + // Defaults to false. + ValidationEnabled bool `toml:"validation_enabled,omitempty"` // Internal: parsed provider configs (not exposed to TOML package) parsedClusterProviderConfigs map[string]api.ExtendedConfig @@ -346,6 +347,6 @@ func (c *StaticConfig) GetStsScopes() []string { return c.StsScopes } -func (c *StaticConfig) GetValidationConfig() api.ValidationConfig { - return &c.Validation +func (c *StaticConfig) IsValidationEnabled() bool { + return c.ValidationEnabled } diff --git a/pkg/config/validation_config.go b/pkg/config/validation_config.go deleted file mode 100644 index ab1badb3f..000000000 --- a/pkg/config/validation_config.go +++ /dev/null @@ -1,28 +0,0 @@ -package config - -import ( - "os" - "strconv" -) - -// ValidationConfig contains pre-execution validation configuration. -// When enabled, validates tool calls before execution -type ValidationConfig struct { - // Defaults to false. - Enabled *bool `toml:"enabled,omitempty"` -} - -// IsEnabled returns true if validation is enabled. -// Environment variable MCP_VALIDATION_ENABLED takes precedence over config. -// Defaults to false. -func (c *ValidationConfig) IsEnabled() bool { - if v := os.Getenv("MCP_VALIDATION_ENABLED"); v != "" { - if b, err := strconv.ParseBool(v); err == nil { - return b - } - } - if c.Enabled != nil { - return *c.Enabled - } - return false -} diff --git a/pkg/kubernetes/accesscontrol_round_tripper.go b/pkg/kubernetes/accesscontrol_round_tripper.go index 24bc513ee..a189cfc06 100644 --- a/pkg/kubernetes/accesscontrol_round_tripper.go +++ b/pkg/kubernetes/accesscontrol_round_tripper.go @@ -1,19 +1,58 @@ package kubernetes import ( + "bytes" "fmt" + "io" "net/http" "strings" "github.com/containers/kubernetes-mcp-server/pkg/api" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/discovery" + authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/klog/v2" ) +// AccessControlRoundTripper intercepts HTTP requests to enforce access control +// and optionally run validators before they reach the Kubernetes API. type AccessControlRoundTripper struct { delegate http.RoundTripper deniedResourcesProvider api.DeniedResourcesProvider restMapperProvider func() meta.RESTMapper + validationEnabled bool + validators []api.HTTPValidator +} + +// AccessControlRoundTripperConfig configures the AccessControlRoundTripper. +type AccessControlRoundTripperConfig struct { + Delegate http.RoundTripper + DeniedResourcesProvider api.DeniedResourcesProvider + RestMapperProvider func() meta.RESTMapper + DiscoveryProvider func() discovery.DiscoveryInterface + AuthClientProvider func() authv1client.AuthorizationV1Interface + ValidationEnabled bool +} + +// NewAccessControlRoundTripper creates a new AccessControlRoundTripper. +func NewAccessControlRoundTripper(cfg AccessControlRoundTripperConfig) *AccessControlRoundTripper { + rt := &AccessControlRoundTripper{ + delegate: cfg.Delegate, + deniedResourcesProvider: cfg.DeniedResourcesProvider, + restMapperProvider: cfg.RestMapperProvider, + validationEnabled: cfg.ValidationEnabled, + } + + if cfg.ValidationEnabled { + rt.validators = CreateValidators(ValidatorProviders{ + RestMapper: cfg.RestMapperProvider, + Discovery: cfg.DiscoveryProvider, + AuthClient: cfg.AuthClientProvider, + }) + } + + return rt } func (rt *AccessControlRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { @@ -28,17 +67,55 @@ func (rt *AccessControlRoundTripper) RoundTrip(req *http.Request) (*http.Respons // was created before restMapper was set (fixes issue #688) restMapper := rt.restMapperProvider() if restMapper == nil { - return nil, fmt.Errorf("failed to make request: AccessControlRoundTripper restMapper not initialized") + return nil, fmt.Errorf("failed to make request: restMapper not initialized") } gvk, err := restMapper.KindFor(gvr) if err != nil { - return nil, fmt.Errorf("failed to make request: AccessControlRoundTripper failed to get kind for gvr %v: %w", gvr, err) + return nil, fmt.Errorf("failed to make request: failed to get kind for gvr %v: %w", gvr, err) } if !rt.isAllowed(gvk) { return nil, fmt.Errorf("resource not allowed: %s", gvk.String()) } + // Skip validators if disabled or if this is SelfSubjectAccessReview (used by RBAC validator) + skipValidation := !rt.validationEnabled || (gvr.Group == "authorization.k8s.io" && gvr.Resource == "selfsubjectaccessreviews") + if skipValidation { + return rt.delegate.RoundTrip(req) + } + + namespace, resourceName := parseURLToNamespaceAndName(req.URL.Path) + verb := httpMethodToVerb(req.Method, req.URL.Path) + + validationReq := &api.HTTPValidationRequest{ + GVR: &gvr, + GVK: &gvk, + HTTPMethod: req.Method, + Verb: verb, + Namespace: namespace, + ResourceName: resourceName, + Path: req.URL.Path, + } + + if req.Body != nil && (req.Method == "POST" || req.Method == "PUT" || req.Method == "PATCH") { + body, readErr := io.ReadAll(req.Body) + _ = req.Body.Close() + if readErr != nil { + return nil, fmt.Errorf("failed to read request body: %w", readErr) + } + req.Body = io.NopCloser(bytes.NewReader(body)) + validationReq.Body = body + } + + for _, v := range rt.validators { + if validationErr := v.Validate(req.Context(), validationReq); validationErr != nil { + if ve, ok := validationErr.(*api.ValidationError); ok { + klog.V(4).Infof("Validation failed [%s]: %v", v.Name(), ve) + } + return nil, validationErr + } + } + return rt.delegate.RoundTrip(req) } @@ -102,3 +179,79 @@ func parseURLToGVR(path string) (gvr schema.GroupVersionResource, ok bool) { } return gvr, true } + +func parseURLToNamespaceAndName(path string) (namespace, name string) { + parts := strings.Split(strings.Trim(path, "/"), "/") + + for i, part := range parts { + if part == "namespaces" && i+1 < len(parts) { + namespace = parts[i+1] + break + } + } + + resourceIdx := findResourceTypeIndex(parts) + if resourceIdx >= 0 && resourceIdx+1 < len(parts) { + name = parts[resourceIdx+1] + } + + return namespace, name +} + +func findResourceTypeIndex(parts []string) int { + if len(parts) == 0 { + return -1 + } + + switch parts[0] { + case "api": + if len(parts) < 3 { + return -1 + } + if parts[2] == "namespaces" && len(parts) > 4 { + return 4 + } + return 2 + case "apis": + if len(parts) < 4 { + return -1 + } + if parts[3] == "namespaces" && len(parts) > 5 { + return 5 + } + return 3 + } + return -1 +} + +func httpMethodToVerb(method, path string) string { + switch method { + case "GET": + if isCollectionPath(path) { + return "list" + } + return "get" + case "POST": + return "create" + case "PUT": + return "update" + case "PATCH": + return "patch" + case "DELETE": + if isCollectionPath(path) { + return "deletecollection" + } + return "delete" + default: + return strings.ToLower(method) + } +} + +func isCollectionPath(path string) bool { + parts := strings.Split(strings.Trim(path, "/"), "/") + resourceIdx := findResourceTypeIndex(parts) + if resourceIdx < 0 { + return false + } + return resourceIdx == len(parts)-1 +} diff --git a/pkg/kubernetes/kubernetes.go b/pkg/kubernetes/kubernetes.go index 5b7e5d3e9..03cbd45d5 100644 --- a/pkg/kubernetes/kubernetes.go +++ b/pkg/kubernetes/kubernetes.go @@ -61,20 +61,19 @@ func NewKubernetes(baseConfig api.BaseConfig, clientCmdConfig clientcmd.ClientCo k.restConfig.UserAgent = rest.DefaultKubernetesUserAgent() } - // Extract validation config if available - var validationConfig api.ValidationConfig - if vcp, ok := baseConfig.(api.ValidationConfigProvider); ok { - validationConfig = vcp.GetValidationConfig() + var validationEnabled bool + if vep, ok := baseConfig.(api.ValidationEnabledProvider); ok { + validationEnabled = vep.IsValidationEnabled() } k.restConfig.Wrap(func(original http.RoundTripper) http.RoundTripper { - return NewValidationRoundTripper(ValidationRoundTripperConfig{ + return NewAccessControlRoundTripper(AccessControlRoundTripperConfig{ Delegate: original, DeniedResourcesProvider: baseConfig, RestMapperProvider: func() meta.RESTMapper { return k.restMapper }, DiscoveryProvider: func() discovery.DiscoveryInterface { return k.discoveryClient }, AuthClientProvider: func() authv1client.AuthorizationV1Interface { return k.AuthorizationV1() }, - ValidatorConfig: validationConfig, + ValidationEnabled: validationEnabled, }) }) k.restConfig.Wrap(func(original http.RoundTripper) http.RoundTripper { diff --git a/pkg/kubernetes/validation_round_tripper.go b/pkg/kubernetes/validation_round_tripper.go deleted file mode 100644 index 314b31bd0..000000000 --- a/pkg/kubernetes/validation_round_tripper.go +++ /dev/null @@ -1,236 +0,0 @@ -package kubernetes - -import ( - "bytes" - "fmt" - "io" - "net/http" - "strings" - - "github.com/containers/kubernetes-mcp-server/pkg/api" - "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/client-go/discovery" - authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" - "k8s.io/klog/v2" -) - -// ValidationRoundTripper intercepts HTTP requests to validate them before they reach the K8s API. -// It extends the AccessControlRoundTripper pattern with additional validator chain execution. -type ValidationRoundTripper struct { - delegate http.RoundTripper - deniedResourcesProvider api.DeniedResourcesProvider - restMapperProvider func() meta.RESTMapper - discoveryProvider func() discovery.DiscoveryInterface - authClientProvider func() authv1client.AuthorizationV1Interface - validatorConfig api.ValidationConfig - validators []api.HTTPValidator -} - -// ValidationRoundTripperConfig configures the ValidationRoundTripper. -type ValidationRoundTripperConfig struct { - Delegate http.RoundTripper - DeniedResourcesProvider api.DeniedResourcesProvider - RestMapperProvider func() meta.RESTMapper - DiscoveryProvider func() discovery.DiscoveryInterface - AuthClientProvider func() authv1client.AuthorizationV1Interface - ValidatorConfig api.ValidationConfig -} - -// NewValidationRoundTripper creates a new ValidationRoundTripper. -func NewValidationRoundTripper(cfg ValidationRoundTripperConfig) *ValidationRoundTripper { - rt := &ValidationRoundTripper{ - delegate: cfg.Delegate, - deniedResourcesProvider: cfg.DeniedResourcesProvider, - restMapperProvider: cfg.RestMapperProvider, - discoveryProvider: cfg.DiscoveryProvider, - authClientProvider: cfg.AuthClientProvider, - validatorConfig: cfg.ValidatorConfig, - } - - // Create validators with providers (only if validation is enabled) - if cfg.ValidatorConfig != nil && cfg.ValidatorConfig.IsEnabled() { - rt.validators = CreateValidators(ValidatorProviders{ - RestMapper: cfg.RestMapperProvider, - Discovery: cfg.DiscoveryProvider, - AuthClient: cfg.AuthClientProvider, - }) - } - - return rt -} - -func (rt *ValidationRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { - gvr, ok := parseURLToGVR(req.URL.Path) - if !ok { - return rt.delegate.RoundTrip(req) - } - - // Access control checks (from original AccessControlRoundTripper) - always enforced - restMapper := rt.restMapperProvider() - if restMapper == nil { - return nil, fmt.Errorf("failed to make request: restMapper not initialized") - } - - gvk, err := restMapper.KindFor(gvr) - if err != nil { - return nil, fmt.Errorf("failed to make request: failed to get kind for gvr %v: %w", gvr, err) - } - - if !rt.isAllowed(gvk) { - return nil, fmt.Errorf("resource not allowed: %s", gvk.String()) - } - - // Skip validators if disabled or if this is SelfSubjectAccessReview (used by RBAC validator) - skipValidation := rt.validatorConfig == nil || !rt.validatorConfig.IsEnabled() - skipValidation = skipValidation || (gvr.Group == "authorization.k8s.io" && gvr.Resource == "selfsubjectaccessreviews") - if skipValidation { - return rt.delegate.RoundTrip(req) - } - - // Extract namespace and resource name from URL - namespace, resourceName := parseURLToNamespaceAndName(req.URL.Path) - - // Map HTTP method to K8s verb - verb := httpMethodToVerb(req.Method, req.URL.Path) - - // Build validation request - validationReq := &api.HTTPValidationRequest{ - GVR: &gvr, - GVK: &gvk, - HTTPMethod: req.Method, - Verb: verb, - Namespace: namespace, - ResourceName: resourceName, - Path: req.URL.Path, - } - - // Buffer body for POST/PUT/PATCH (needed for schema validation) - if req.Body != nil && (req.Method == "POST" || req.Method == "PUT" || req.Method == "PATCH") { - body, readErr := io.ReadAll(req.Body) - _ = req.Body.Close() - if readErr != nil { - return nil, fmt.Errorf("failed to read request body: %w", readErr) - } - req.Body = io.NopCloser(bytes.NewReader(body)) - validationReq.Body = body - } - - // Run validators - for _, v := range rt.validators { - if validationErr := v.Validate(req.Context(), validationReq); validationErr != nil { - if ve, ok := validationErr.(*api.ValidationError); ok { - klog.V(4).Infof("Validation failed [%s]: %v", v.Name(), ve) - } - return nil, validationErr - } - } - - return rt.delegate.RoundTrip(req) -} - -// isAllowed checks the resource is in denied list or not. -func (rt *ValidationRoundTripper) isAllowed(gvk schema.GroupVersionKind) bool { - if rt.deniedResourcesProvider == nil { - return true - } - - for _, val := range rt.deniedResourcesProvider.GetDeniedResources() { - if val.Kind == "" { - if gvk.Group == val.Group && gvk.Version == val.Version { - return false - } - } - if gvk.Group == val.Group && gvk.Version == val.Version && gvk.Kind == val.Kind { - return false - } - } - - return true -} - -// parseURLToNamespaceAndName extracts namespace and resource name from K8s API URL. -func parseURLToNamespaceAndName(path string) (namespace, name string) { - parts := strings.Split(strings.Trim(path, "/"), "/") - - // Find "namespaces" segment - for i, part := range parts { - if part == "namespaces" && i+1 < len(parts) { - namespace = parts[i+1] - break - } - } - - // Find resource type position and check for name after it - resourceIdx := findResourceTypeIndex(parts) - if resourceIdx >= 0 && resourceIdx+1 < len(parts) { - name = parts[resourceIdx+1] - } - - return namespace, name -} - -// findResourceTypeIndex returns the index of the resource type segment in the URL parts. -// Uses the standard K8s API URL structure to locate the resource type. -func findResourceTypeIndex(parts []string) int { - if len(parts) == 0 { - return -1 - } - - switch parts[0] { - case "api": - // /api/v1/{resource} or /api/v1/namespaces/{ns}/{resource} - if len(parts) < 3 { - return -1 - } - if parts[2] == "namespaces" && len(parts) > 4 { - return 4 - } - return 2 - case "apis": - // /apis/{group}/{version}/{resource} or /apis/{group}/{version}/namespaces/{ns}/{resource} - if len(parts) < 4 { - return -1 - } - if parts[3] == "namespaces" && len(parts) > 5 { - return 5 - } - return 3 - } - return -1 -} - -// httpMethodToVerb maps HTTP method to K8s RBAC verb. -func httpMethodToVerb(method, path string) string { - switch method { - case "GET": - if isCollectionPath(path) { - return "list" - } - return "get" - case "POST": - return "create" - case "PUT": - return "update" - case "PATCH": - return "patch" - case "DELETE": - if isCollectionPath(path) { - return "deletecollection" - } - return "delete" - default: - return strings.ToLower(method) - } -} - -// isCollectionPath checks if the path targets a collection (list) vs a specific resource. -func isCollectionPath(path string) bool { - parts := strings.Split(strings.Trim(path, "/"), "/") - resourceIdx := findResourceTypeIndex(parts) - if resourceIdx < 0 { - return false - } - // Collection path if the resource type is the last segment (no name after it) - return resourceIdx == len(parts)-1 -} diff --git a/pkg/mcp/pods_test.go b/pkg/mcp/pods_test.go index 92ead48c8..1b6f69222 100644 --- a/pkg/mcp/pods_test.go +++ b/pkg/mcp/pods_test.go @@ -66,10 +66,6 @@ func (s *PodsSuite) TestPodsListInAllNamespaces() { } func (s *PodsSuite) TestPodsListInAllNamespacesUnauthorized() { - // Disable validation for this test - we're testing the tool's fallback behavior - // when cluster-wide list fails but namespace-scoped list succeeds - validationDisabled := false - s.Cfg.Validation.Enabled = &validationDisabled s.InitMcpClient() defer restoreAuth(s.T().Context()) client := kubernetes.NewForConfigOrDie(envTestRestConfig) From b140a123cff11764a640bd8e48f2e49effdbb64a Mon Sep 17 00:00:00 2001 From: Nader Ziada Date: Thu, 19 Feb 2026 10:27:26 -0500 Subject: [PATCH 3/3] remove redundant ResourceValidator and simplify validation cleanup up unused func and fields Signed-off-by: Nader Ziada --- docs/VALIDATION.md | 24 ++- docs/configuration.md | 2 +- pkg/api/config.go | 1 + pkg/api/validation.go | 42 ------ pkg/kubernetes/accesscontrol_round_tripper.go | 11 +- .../accesscontrol_round_tripper_test.go | 3 +- pkg/kubernetes/kubernetes.go | 7 +- pkg/kubernetes/resource_validator.go | 55 ------- pkg/kubernetes/resource_validator_test.go | 139 ------------------ pkg/kubernetes/schema_validator.go | 4 +- pkg/kubernetes/validator_registry.go | 5 - pkg/kubernetes/validator_registry_test.go | 6 +- pkg/mcp/events_test.go | 12 +- pkg/mcp/namespaces_test.go | 13 +- pkg/mcp/pods_test.go | 11 +- pkg/mcp/resources_test.go | 43 +++--- 16 files changed, 75 insertions(+), 303 deletions(-) delete mode 100644 pkg/kubernetes/resource_validator.go delete mode 100644 pkg/kubernetes/resource_validator_test.go diff --git a/docs/VALIDATION.md b/docs/VALIDATION.md index 10bc76f7d..003c289cc 100644 --- a/docs/VALIDATION.md +++ b/docs/VALIDATION.md @@ -16,15 +16,15 @@ With validation enabled, you get clearer feedback: Resource apps/v1/Deploymnt does not exist in the cluster ``` -The validation layer catches three types of issues: +The validation layer catches these types of issues: -1. **Resource Validation** - Catches typos like "Deploymnt" instead of "Deployment" +1. **Resource Existence** - Catches typos like "Deploymnt" instead of "Deployment" (checked in access control) 2. **Schema Validation** - Catches invalid fields like "spec.replcias" instead of "spec.replicas" 3. **RBAC Validation** - Pre-checks permissions before attempting operations ## Configuration -Validation is **disabled by default**. All validators (resource, schema, RBAC) run together when enabled. +Validation is **disabled by default**. Schema and RBAC validators run together when enabled. Resource existence is always checked as part of access control. ```toml # Enable all validation (default: false) @@ -48,15 +48,14 @@ Validation happens at the HTTP RoundTripper level, intercepting all Kubernetes A ``` MCP Tool Call → Kubernetes Client → HTTP RoundTripper → Kubernetes API ↓ - Access Control (deny list) + Access Control + - Check deny list + - Check resource exists ↓ - Resource Validator - "Does this GVK exist?" - ↓ - Schema Validator + Schema Validator (if enabled) "Are the fields valid?" ↓ - RBAC Validator + RBAC Validator (if enabled) "Does the user have permission?" ↓ Forward to K8s API @@ -66,9 +65,9 @@ This HTTP-layer approach ensures **all** Kubernetes API calls are validated, inc If any validator fails, the request is rejected with a clear error message before reaching the Kubernetes API. -### 1. Resource Validation +### 1. Resource Existence (Access Control) -Validates that the requested resource type (Group/Version/Kind) exists in the cluster. +The access control layer validates that the requested resource type exists in the cluster. This check runs regardless of whether validation is enabled. **What it catches:** - Typos in Kind names: "Deploymnt" → should be "Deployment" @@ -77,7 +76,7 @@ Validates that the requested resource type (Group/Version/Kind) exists in the cl **Example error:** ``` -RESOURCE_NOT_FOUND: Resource apps/v1/Deploymnt does not exist in the cluster +RESOURCE_NOT_FOUND: Resource deployments.apps does not exist in the cluster ``` ### 2. Schema Validation @@ -118,5 +117,4 @@ PERMISSION_DENIED: Cannot create deployments.apps in namespace "production" |------|-------------| | `RESOURCE_NOT_FOUND` | The requested resource type doesn't exist in the cluster | | `INVALID_FIELD` | A field in the manifest doesn't exist or has the wrong type | -| `INVALID_MANIFEST` | The manifest is malformed (invalid YAML/JSON) | | `PERMISSION_DENIED` | RBAC denies the requested operation | diff --git a/docs/configuration.md b/docs/configuration.md index 1c24e5608..7e9feb18f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -184,10 +184,10 @@ Toolsets group related tools together. Enable only the toolsets you need to redu | Toolset | Description | Default | |----------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------| +| kiali | Most common tools for managing Kiali, check the [Kiali documentation](https://github.com/containers/kubernetes-mcp-server/blob/main/docs/KIALI.md) for more details. | | | config | View and manage the current local Kubernetes configuration (kubeconfig) | ✓ | | core | Most common tools for Kubernetes management (Pods, Generic Resources, Events, etc.) | ✓ | | kcp | Manage kcp workspaces and multi-tenancy features | | -| kiali | Most common tools for managing Kiali, check the [Kiali documentation](https://github.com/containers/kubernetes-mcp-server/blob/main/docs/KIALI.md) for more details. | | | kubevirt | KubeVirt virtual machine management tools | | | helm | Tools for managing Helm charts and releases | ✓ | diff --git a/pkg/api/config.go b/pkg/api/config.go index 03900d391..929a6f3e6 100644 --- a/pkg/api/config.go +++ b/pkg/api/config.go @@ -64,4 +64,5 @@ type BaseConfig interface { DeniedResourcesProvider ExtendedConfigProvider StsConfigProvider + ValidationEnabledProvider } diff --git a/pkg/api/validation.go b/pkg/api/validation.go index f39abf2ac..36f1dcedd 100644 --- a/pkg/api/validation.go +++ b/pkg/api/validation.go @@ -33,7 +33,6 @@ const ( ErrorCodeResourceNotFound ValidationErrorCode = "RESOURCE_NOT_FOUND" ErrorCodeInvalidField ValidationErrorCode = "INVALID_FIELD" ErrorCodePermissionDenied ValidationErrorCode = "PERMISSION_DENIED" - ErrorCodeInvalidManifest ValidationErrorCode = "INVALID_MANIFEST" ) // ValidationError provides AI-friendly error information for validation failures. @@ -55,23 +54,6 @@ func (e *ValidationError) Error() string { return sb.String() } -// NewResourceNotFoundError creates an error for unknown resource types. -func NewResourceNotFoundError(apiVersion, kind string) *ValidationError { - return &ValidationError{ - Code: ErrorCodeResourceNotFound, - Message: fmt.Sprintf("Resource %s/%s does not exist in the cluster", apiVersion, kind), - } -} - -// NewInvalidFieldError creates an error for invalid fields. -func NewInvalidFieldError(field, resourceKind string) *ValidationError { - return &ValidationError{ - Code: ErrorCodeInvalidField, - Message: fmt.Sprintf("Invalid field %q in %s", field, resourceKind), - Field: field, - } -} - // NewPermissionDeniedError creates an error for RBAC permission failures. func NewPermissionDeniedError(verb, resource, namespace string) *ValidationError { var msg string @@ -87,30 +69,6 @@ func NewPermissionDeniedError(verb, resource, namespace string) *ValidationError } } -// NewInvalidManifestError creates an error for malformed manifests. -func NewInvalidManifestError(reason string) *ValidationError { - return &ValidationError{ - Code: ErrorCodeInvalidManifest, - Message: fmt.Sprintf("Invalid resource manifest: %s", reason), - } -} - -// FormatValidationErrors formats multiple validation errors into a single string. -func FormatValidationErrors(errors []*ValidationError) string { - if len(errors) == 0 { - return "" - } - - var sb strings.Builder - for i, err := range errors { - if i > 0 { - sb.WriteString("\n\n") - } - sb.WriteString(err.Error()) - } - return sb.String() -} - // FormatResourceName creates a human-readable resource identifier from GVR. func FormatResourceName(gvr *schema.GroupVersionResource) string { if gvr == nil { diff --git a/pkg/kubernetes/accesscontrol_round_tripper.go b/pkg/kubernetes/accesscontrol_round_tripper.go index a189cfc06..a97db9adf 100644 --- a/pkg/kubernetes/accesscontrol_round_tripper.go +++ b/pkg/kubernetes/accesscontrol_round_tripper.go @@ -46,7 +46,6 @@ func NewAccessControlRoundTripper(cfg AccessControlRoundTripperConfig) *AccessCo if cfg.ValidationEnabled { rt.validators = CreateValidators(ValidatorProviders{ - RestMapper: cfg.RestMapperProvider, Discovery: cfg.DiscoveryProvider, AuthClient: cfg.AuthClientProvider, }) @@ -67,12 +66,18 @@ func (rt *AccessControlRoundTripper) RoundTrip(req *http.Request) (*http.Respons // was created before restMapper was set (fixes issue #688) restMapper := rt.restMapperProvider() if restMapper == nil { - return nil, fmt.Errorf("failed to make request: restMapper not initialized") + return nil, fmt.Errorf("failed to make request: AccessControlRoundTripper restMapper not initialized") } gvk, err := restMapper.KindFor(gvr) if err != nil { - return nil, fmt.Errorf("failed to make request: failed to get kind for gvr %v: %w", gvr, err) + if meta.IsNoMatchError(err) { + return nil, &api.ValidationError{ + Code: api.ErrorCodeResourceNotFound, + Message: fmt.Sprintf("Resource %s does not exist in the cluster", api.FormatResourceName(&gvr)), + } + } + return nil, fmt.Errorf("failed to make request: AccessControlRoundTripper failed to get kind for gvr %v: %w", gvr, err) } if !rt.isAllowed(gvk) { return nil, fmt.Errorf("resource not allowed: %s", gvk.String()) diff --git a/pkg/kubernetes/accesscontrol_round_tripper_test.go b/pkg/kubernetes/accesscontrol_round_tripper_test.go index c8a5de34a..facfa5bbd 100644 --- a/pkg/kubernetes/accesscontrol_round_tripper_test.go +++ b/pkg/kubernetes/accesscontrol_round_tripper_test.go @@ -287,7 +287,8 @@ func (s *AccessControlRoundTripperTestSuite) TestRoundTripForDeniedAPIResources( s.Error(err) s.Nil(resp) s.False(delegateCalled, "Expected delegate not to be called when RESTMapper fails") - s.Contains(err.Error(), "failed to make request") + s.Contains(err.Error(), "RESOURCE_NOT_FOUND") + s.Contains(err.Error(), "does not exist in the cluster") }) } diff --git a/pkg/kubernetes/kubernetes.go b/pkg/kubernetes/kubernetes.go index 03cbd45d5..f44bd46de 100644 --- a/pkg/kubernetes/kubernetes.go +++ b/pkg/kubernetes/kubernetes.go @@ -61,11 +61,6 @@ func NewKubernetes(baseConfig api.BaseConfig, clientCmdConfig clientcmd.ClientCo k.restConfig.UserAgent = rest.DefaultKubernetesUserAgent() } - var validationEnabled bool - if vep, ok := baseConfig.(api.ValidationEnabledProvider); ok { - validationEnabled = vep.IsValidationEnabled() - } - k.restConfig.Wrap(func(original http.RoundTripper) http.RoundTripper { return NewAccessControlRoundTripper(AccessControlRoundTripperConfig{ Delegate: original, @@ -73,7 +68,7 @@ func NewKubernetes(baseConfig api.BaseConfig, clientCmdConfig clientcmd.ClientCo RestMapperProvider: func() meta.RESTMapper { return k.restMapper }, DiscoveryProvider: func() discovery.DiscoveryInterface { return k.discoveryClient }, AuthClientProvider: func() authv1client.AuthorizationV1Interface { return k.AuthorizationV1() }, - ValidationEnabled: validationEnabled, + ValidationEnabled: baseConfig.IsValidationEnabled(), }) }) k.restConfig.Wrap(func(original http.RoundTripper) http.RoundTripper { diff --git a/pkg/kubernetes/resource_validator.go b/pkg/kubernetes/resource_validator.go deleted file mode 100644 index eda466ff1..000000000 --- a/pkg/kubernetes/resource_validator.go +++ /dev/null @@ -1,55 +0,0 @@ -package kubernetes - -import ( - "context" - - "github.com/containers/kubernetes-mcp-server/pkg/api" - "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/klog/v2" -) - -// ResourceValidator validates that resource types (GVK) exist in the cluster. -type ResourceValidator struct { - restMapperProvider func() meta.RESTMapper -} - -// NewResourceValidator creates a new resource validator. -func NewResourceValidator(restMapperProvider func() meta.RESTMapper) *ResourceValidator { - return &ResourceValidator{ - restMapperProvider: restMapperProvider, - } -} - -func (v *ResourceValidator) Name() string { - return "resource" -} - -func (v *ResourceValidator) Validate(ctx context.Context, req *api.HTTPValidationRequest) error { - if req.GVK == nil { - return nil - } - - restMapper := v.restMapperProvider() - if restMapper == nil { - return nil - } - - _, err := restMapper.RESTMapping( - schema.GroupKind{Group: req.GVK.Group, Kind: req.GVK.Kind}, - req.GVK.Version, - ) - - if err != nil { - if meta.IsNoMatchError(err) { - return api.NewResourceNotFoundError( - req.GVK.GroupVersion().String(), - req.GVK.Kind, - ) - } - klog.V(4).Infof("RESTMapper error for %v: %v", req.GVK, err) - return nil - } - - return nil -} diff --git a/pkg/kubernetes/resource_validator_test.go b/pkg/kubernetes/resource_validator_test.go deleted file mode 100644 index a56965d0f..000000000 --- a/pkg/kubernetes/resource_validator_test.go +++ /dev/null @@ -1,139 +0,0 @@ -package kubernetes - -import ( - "context" - "testing" - - "github.com/containers/kubernetes-mcp-server/pkg/api" - "github.com/stretchr/testify/suite" - "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime/schema" -) - -type mockRESTMapper struct { - mappings map[schema.GroupKind]*meta.RESTMapping -} - -func (m *mockRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*meta.RESTMapping, error) { - if mapping, ok := m.mappings[gk]; ok { - return mapping, nil - } - return nil, &meta.NoResourceMatchError{PartialResource: schema.GroupVersionResource{Group: gk.Group, Resource: gk.Kind}} -} - -func (m *mockRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*meta.RESTMapping, error) { - return nil, nil -} - -func (m *mockRESTMapper) KindFor(resource schema.GroupVersionResource) (schema.GroupVersionKind, error) { - return schema.GroupVersionKind{}, nil -} - -func (m *mockRESTMapper) KindsFor(resource schema.GroupVersionResource) ([]schema.GroupVersionKind, error) { - return nil, nil -} - -func (m *mockRESTMapper) ResourceFor(input schema.GroupVersionResource) (schema.GroupVersionResource, error) { - return schema.GroupVersionResource{}, nil -} - -func (m *mockRESTMapper) ResourcesFor(input schema.GroupVersionResource) ([]schema.GroupVersionResource, error) { - return nil, nil -} - -func (m *mockRESTMapper) ResourceSingularizer(resource string) (singular string, err error) { - return "", nil -} - -type ResourceValidatorTestSuite struct { - suite.Suite -} - -func (s *ResourceValidatorTestSuite) TestName() { - v := NewResourceValidator(nil) - s.Equal("resource", v.Name()) -} - -func (s *ResourceValidatorTestSuite) TestValidate() { - testCases := []struct { - name string - req *api.HTTPValidationRequest - mapper meta.RESTMapper - expectError bool - errorCode api.ValidationErrorCode - }{ - { - name: "nil GVK passes validation", - req: &api.HTTPValidationRequest{GVK: nil}, - mapper: nil, - expectError: false, - }, - { - name: "nil RESTMapper passes validation", - req: &api.HTTPValidationRequest{ - GVK: &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}, - }, - mapper: nil, - expectError: false, - }, - { - name: "existing resource passes validation", - req: &api.HTTPValidationRequest{ - GVK: &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}, - }, - mapper: &mockRESTMapper{ - mappings: map[schema.GroupKind]*meta.RESTMapping{ - {Group: "", Kind: "Pod"}: { - Resource: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, - }, - }, - }, - expectError: false, - }, - { - name: "non-existent resource fails validation", - req: &api.HTTPValidationRequest{ - GVK: &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "FakeResource"}, - }, - mapper: &mockRESTMapper{ - mappings: map[schema.GroupKind]*meta.RESTMapping{}, - }, - expectError: true, - errorCode: api.ErrorCodeResourceNotFound, - }, - { - name: "apps group resource passes validation", - req: &api.HTTPValidationRequest{ - GVK: &schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - }, - mapper: &mockRESTMapper{ - mappings: map[schema.GroupKind]*meta.RESTMapping{ - {Group: "apps", Kind: "Deployment"}: { - Resource: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - }, - }, - }, - expectError: false, - }, - } - - for _, tc := range testCases { - s.Run(tc.name, func() { - v := NewResourceValidator(func() meta.RESTMapper { return tc.mapper }) - err := v.Validate(context.Background(), tc.req) - - if tc.expectError { - s.Error(err) - if ve, ok := err.(*api.ValidationError); ok { - s.Equal(tc.errorCode, ve.Code) - } - } else { - s.NoError(err) - } - }) - } -} - -func TestResourceValidator(t *testing.T) { - suite.Run(t, new(ResourceValidatorTestSuite)) -} diff --git a/pkg/kubernetes/schema_validator.go b/pkg/kubernetes/schema_validator.go index 04bbcb44d..fabd48676 100644 --- a/pkg/kubernetes/schema_validator.go +++ b/pkg/kubernetes/schema_validator.go @@ -39,8 +39,8 @@ func (v *SchemaValidator) Validate(ctx context.Context, req *api.HTTPValidationR return nil } - // Only validate for create/update operations - if req.Verb != "create" && req.Verb != "update" && req.Verb != "patch" { + // Only validate for create/update operations (exclude patch as partial bodies cause false positives) + if req.Verb != "create" && req.Verb != "update" { return nil } diff --git a/pkg/kubernetes/validator_registry.go b/pkg/kubernetes/validator_registry.go index e50b0b2f0..a6457c69e 100644 --- a/pkg/kubernetes/validator_registry.go +++ b/pkg/kubernetes/validator_registry.go @@ -2,14 +2,12 @@ package kubernetes import ( "github.com/containers/kubernetes-mcp-server/pkg/api" - "k8s.io/apimachinery/pkg/api/meta" "k8s.io/client-go/discovery" authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" ) // ValidatorProviders holds the providers needed to create validators. type ValidatorProviders struct { - RestMapper func() meta.RESTMapper Discovery func() discovery.DiscoveryInterface AuthClient func() authv1client.AuthorizationV1Interface } @@ -34,9 +32,6 @@ func CreateValidators(providers ValidatorProviders) []api.HTTPValidator { } func init() { - RegisterValidator(func(p ValidatorProviders) api.HTTPValidator { - return NewResourceValidator(p.RestMapper) - }) RegisterValidator(func(p ValidatorProviders) api.HTTPValidator { return NewSchemaValidator(p.Discovery) }) diff --git a/pkg/kubernetes/validator_registry_test.go b/pkg/kubernetes/validator_registry_test.go index c99596b85..8fab8b4f1 100644 --- a/pkg/kubernetes/validator_registry_test.go +++ b/pkg/kubernetes/validator_registry_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/stretchr/testify/suite" - "k8s.io/apimachinery/pkg/api/meta" "k8s.io/client-go/discovery" authv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" ) @@ -15,28 +14,25 @@ type ValidatorRegistryTestSuite struct { func (s *ValidatorRegistryTestSuite) TestCreateValidatorsReturnsRegisteredValidators() { providers := ValidatorProviders{ - RestMapper: func() meta.RESTMapper { return nil }, Discovery: func() discovery.DiscoveryInterface { return nil }, AuthClient: func() authv1client.AuthorizationV1Interface { return nil }, } validators := CreateValidators(providers) - s.GreaterOrEqual(len(validators), 3, "Expected at least 3 validators (resource, schema, rbac)") + s.GreaterOrEqual(len(validators), 2, "Expected at least 2 validators (schema, rbac)") names := make(map[string]bool) for _, v := range validators { names[v.Name()] = true } - s.True(names["resource"], "Expected resource validator to be registered") s.True(names["schema"], "Expected schema validator to be registered") s.True(names["rbac"], "Expected rbac validator to be registered") } func (s *ValidatorRegistryTestSuite) TestCreateValidatorsWithNilProviders() { providers := ValidatorProviders{ - RestMapper: nil, Discovery: nil, AuthClient: nil, } diff --git a/pkg/mcp/events_test.go b/pkg/mcp/events_test.go index e18ac6377..c034122ac 100644 --- a/pkg/mcp/events_test.go +++ b/pkg/mcp/events_test.go @@ -3,6 +3,7 @@ package mcp import ( "strings" "testing" + "time" "github.com/BurntSushi/toml" "github.com/mark3labs/mcp-go/mcp" @@ -150,12 +151,17 @@ func (s *EventsSuite) TestEventsListForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("events_list (forbidden)", func() { + capture := s.StartCapturingLogNotifications() toolResult, _ := s.CallTool("events_list", map[string]interface{}{}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - msg := toolResult.Content[0].(mcp.TextContent).Text - s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), - "error message should indicate forbidden or permission denied, got: %s", msg) + s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", + "error message should indicate forbidden") + }) + s.Run("sends log notification", func() { + logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) + s.Equal("error", logNotification.Level, "forbidden errors should log at error level") + s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") }) }) } diff --git a/pkg/mcp/namespaces_test.go b/pkg/mcp/namespaces_test.go index db0d979b8..de95aa8f1 100644 --- a/pkg/mcp/namespaces_test.go +++ b/pkg/mcp/namespaces_test.go @@ -3,8 +3,8 @@ package mcp import ( "regexp" "slices" - "strings" "testing" + "time" "github.com/BurntSushi/toml" "github.com/mark3labs/mcp-go/mcp" @@ -75,12 +75,17 @@ func (s *NamespacesSuite) TestNamespacesListForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("namespaces_list (forbidden)", func() { + capture := s.StartCapturingLogNotifications() toolResult, _ := s.CallTool("namespaces_list", map[string]interface{}{}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - msg := toolResult.Content[0].(mcp.TextContent).Text - s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), - "error message should indicate forbidden or permission denied, got: %s", msg) + s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", + "error message should indicate forbidden") + }) + s.Run("sends log notification", func() { + logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) + s.Equal("error", logNotification.Level, "forbidden errors should log at error level") + s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") }) }) } diff --git a/pkg/mcp/pods_test.go b/pkg/mcp/pods_test.go index 1b6f69222..918f9811a 100644 --- a/pkg/mcp/pods_test.go +++ b/pkg/mcp/pods_test.go @@ -194,12 +194,17 @@ func (s *PodsSuite) TestPodsListForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("pods_list (forbidden)", func() { + capture := s.StartCapturingLogNotifications() toolResult, _ := s.CallTool("pods_list", map[string]interface{}{}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - msg := toolResult.Content[0].(mcp.TextContent).Text - s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), - "error message should indicate forbidden or permission denied, got: %s", msg) + s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", + "error message should indicate forbidden") + }) + s.Run("sends log notification", func() { + logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) + s.Equal("error", logNotification.Level, "forbidden errors should log at error level") + s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") }) }) } diff --git a/pkg/mcp/resources_test.go b/pkg/mcp/resources_test.go index 5b02fe541..394690b0a 100644 --- a/pkg/mcp/resources_test.go +++ b/pkg/mcp/resources_test.go @@ -49,11 +49,8 @@ func (s *ResourcesSuite) TestResourcesList() { s.Run("resources_list with nonexistent apiVersion returns error", func() { toolResult, _ := s.CallTool("resources_list", map[string]interface{}{"apiVersion": "custom.non.existent.example.com/v1", "kind": "Custom"}) s.Truef(toolResult.IsError, "call tool should fail") - msg := toolResult.Content[0].(mcp.TextContent).Text - // Accept both validation error and API error formats - hasValidationError := strings.Contains(msg, "RESOURCE_NOT_FOUND") && strings.Contains(msg, "Custom") - hasAPIError := strings.Contains(msg, "no matches for kind") - s.Truef(hasValidationError || hasAPIError, "error should indicate resource not found, got: %s", msg) + s.Equalf(`failed to list resources: no matches for kind "Custom" in version "custom.non.existent.example.com/v1"`, + toolResult.Content[0].(mcp.TextContent).Text, "invalid error message, got %v", toolResult.Content[0].(mcp.TextContent).Text) }) s.Run("resources_list(apiVersion=v1, kind=Namespace) returns namespaces", func() { namespaces, err := s.CallTool("resources_list", map[string]interface{}{"apiVersion": "v1", "kind": "Namespace"}) @@ -210,12 +207,17 @@ func (s *ResourcesSuite) TestResourcesListForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("resources_list (forbidden)", func() { + capture := s.StartCapturingLogNotifications() toolResult, _ := s.CallTool("resources_list", map[string]interface{}{"apiVersion": "v1", "kind": "ConfigMap"}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - msg := toolResult.Content[0].(mcp.TextContent).Text - s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), - "error message should indicate forbidden or permission denied, got: %s", msg) + s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", + "error message should indicate forbidden") + }) + s.Run("sends log notification", func() { + logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) + s.Equal("error", logNotification.Level, "forbidden errors should log at error level") + s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") }) }) } @@ -322,11 +324,8 @@ func (s *ResourcesSuite) TestResourcesGet() { s.Run("resources_get with nonexistent apiVersion returns error", func() { toolResult, _ := s.CallTool("resources_get", map[string]interface{}{"apiVersion": "custom.non.existent.example.com/v1", "kind": "Custom", "name": "a-custom"}) s.Truef(toolResult.IsError, "call tool should fail") - msg := toolResult.Content[0].(mcp.TextContent).Text - // Accept both validation error and API error formats - hasValidationError := strings.Contains(msg, "RESOURCE_NOT_FOUND") && strings.Contains(msg, "Custom") - hasAPIError := strings.Contains(msg, "no matches for kind") - s.Truef(hasValidationError || hasAPIError, "error should indicate resource not found, got: %s", msg) + s.Equalf(`failed to get resource: no matches for kind "Custom" in version "custom.non.existent.example.com/v1"`, + toolResult.Content[0].(mcp.TextContent).Text, "invalid error message, got %v", toolResult.Content[0].(mcp.TextContent).Text) }) s.Run("resources_get with missing name returns error", func() { toolResult, _ := s.CallTool("resources_get", map[string]interface{}{"apiVersion": "v1", "kind": "Namespace"}) @@ -590,13 +589,18 @@ func (s *ResourcesSuite) TestResourcesCreateOrUpdateForbidden() { _ = client.RbacV1().ClusterRoles().Delete(s.T().Context(), "allow-all", metav1.DeleteOptions{}) s.Run("resources_create_or_update (forbidden)", func() { + capture := s.StartCapturingLogNotifications() configMapYaml := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a-forbidden-configmap\n namespace: default\n" toolResult, _ := s.CallTool("resources_create_or_update", map[string]interface{}{"resource": configMapYaml}) s.Run("returns error", func() { s.Truef(toolResult.IsError, "call tool should fail") - msg := toolResult.Content[0].(mcp.TextContent).Text - s.Truef(strings.Contains(msg, "forbidden") || strings.Contains(msg, "PERMISSION_DENIED"), - "error message should indicate forbidden or permission denied, got: %s", msg) + s.Contains(toolResult.Content[0].(mcp.TextContent).Text, "forbidden", + "error message should indicate forbidden") + }) + s.Run("sends log notification", func() { + logNotification := capture.RequireLogNotification(s.T(), 2*time.Second) + s.Equal("error", logNotification.Level, "forbidden errors should log at error level") + s.Contains(logNotification.Data, "Permission denied", "log message should indicate permission denied") }) }) } @@ -626,11 +630,8 @@ func (s *ResourcesSuite) TestResourcesDelete() { s.Run("resources_delete with nonexistent apiVersion returns error", func() { toolResult, _ := s.CallTool("resources_delete", map[string]interface{}{"apiVersion": "custom.non.existent.example.com/v1", "kind": "Custom", "name": "a-custom"}) s.Truef(toolResult.IsError, "call tool should fail") - msg := toolResult.Content[0].(mcp.TextContent).Text - // Accept both validation error and API error formats - hasValidationError := strings.Contains(msg, "RESOURCE_NOT_FOUND") && strings.Contains(msg, "Custom") - hasAPIError := strings.Contains(msg, "no matches for kind") - s.Truef(hasValidationError || hasAPIError, "error should indicate resource not found, got: %s", msg) + s.Equalf(`failed to delete resource: no matches for kind "Custom" in version "custom.non.existent.example.com/v1"`, + toolResult.Content[0].(mcp.TextContent).Text, "invalid error message, got %v", toolResult.Content[0].(mcp.TextContent).Text) }) s.Run("resources_delete with missing name returns error", func() { toolResult, _ := s.CallTool("resources_delete", map[string]interface{}{"apiVersion": "v1", "kind": "Namespace"})