From 1eeb86632be7eb5f55435074243f575e6edb734f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 11:43:20 +0000 Subject: [PATCH 1/3] Add webhook validation for RetryDelay field Add admission-time validation for the RetryDelay field in VirtualMCPServer composite tool error handling. This ensures invalid duration formats are rejected with clear error messages instead of silently failing during conversion. Changes: - Add RetryDelay format validation in validateStepErrorHandling() - Reuse existing validateDuration() from the same package - Add tests for valid durations (5s, 1m30s) and invalid formats --- .../api/v1alpha1/virtualmcpserver_webhook.go | 7 ++ .../v1alpha1/virtualmcpserver_webhook_test.go | 79 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go index 3fe5d3d1a..e36a73054 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go @@ -346,6 +346,13 @@ func validateStepErrorHandling(toolIndex, stepIndex int, step WorkflowStep) erro toolIndex, stepIndex) } + if step.OnError.Action == "retry" && step.OnError.RetryDelay != "" { + if err := validateDuration(step.OnError.RetryDelay); err != nil { + return fmt.Errorf("spec.compositeTools[%d].steps[%d].onError.retryDelay: %w", + toolIndex, stepIndex, err) + } + } + return nil } diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook_test.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook_test.go index c1163fb08..68e46c660 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook_test.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook_test.go @@ -545,6 +545,85 @@ func TestValidateCompositeToolsWithDependencies(t *testing.T) { wantErr: true, errMsg: "spec.compositeTools[0].steps[0].onError.maxRetries must be at least 1 when action is retry", }, + { + name: "valid error handling with retryDelay", + vmcp: &VirtualMCPServer{ + Spec: VirtualMCPServerSpec{ + GroupRef: GroupRef{Name: "test-group"}, + CompositeTools: []CompositeToolSpec{ + { + Name: "test-tool", + Description: "Test composite tool", + Steps: []WorkflowStep{ + { + ID: "step1", + Tool: "backend.tool1", + OnError: &ErrorHandling{ + Action: "retry", + MaxRetries: 3, + RetryDelay: "5s", + }, + }, + }, + }, + }, + }, + }, + wantErr: false, + }, + { + name: "valid error handling with complex retryDelay", + vmcp: &VirtualMCPServer{ + Spec: VirtualMCPServerSpec{ + GroupRef: GroupRef{Name: "test-group"}, + CompositeTools: []CompositeToolSpec{ + { + Name: "test-tool", + Description: "Test composite tool", + Steps: []WorkflowStep{ + { + ID: "step1", + Tool: "backend.tool1", + OnError: &ErrorHandling{ + Action: "retry", + MaxRetries: 3, + RetryDelay: "1m30s", + }, + }, + }, + }, + }, + }, + }, + wantErr: false, + }, + { + name: "invalid error handling - invalid retryDelay format", + vmcp: &VirtualMCPServer{ + Spec: VirtualMCPServerSpec{ + GroupRef: GroupRef{Name: "test-group"}, + CompositeTools: []CompositeToolSpec{ + { + Name: "test-tool", + Description: "Test composite tool", + Steps: []WorkflowStep{ + { + ID: "step1", + Tool: "backend.tool1", + OnError: &ErrorHandling{ + Action: "retry", + MaxRetries: 3, + RetryDelay: "invalid", + }, + }, + }, + }, + }, + }, + }, + wantErr: true, + errMsg: "spec.compositeTools[0].steps[0].onError.retryDelay: invalid duration format \"invalid\", expected format like '30s', '5m', '1h', '1h30m'", + }, } for _, tt := range tests { From 11b887ab2d4fa1be77dfeef9ce88880ddb0cc26a Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 11:48:31 +0000 Subject: [PATCH 2/3] Add error logging for RetryDelay parsing failures Log a warning when RetryDelay duration parsing fails in the converter. This improves debugging by making it clear when an invalid duration is silently defaulting to zero, even though webhook validation should catch invalid formats at admission time. --- cmd/thv-operator/pkg/vmcpconfig/converter.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cmd/thv-operator/pkg/vmcpconfig/converter.go b/cmd/thv-operator/pkg/vmcpconfig/converter.go index e4f3ef52b..14b76a1a8 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter.go @@ -309,7 +309,12 @@ func (*Converter) convertCompositeTools( RetryCount: crdStep.OnError.MaxRetries, } if crdStep.OnError.RetryDelay != "" { - if duration, err := time.ParseDuration(crdStep.OnError.RetryDelay); err == nil { + if duration, err := time.ParseDuration(crdStep.OnError.RetryDelay); err != nil { + // Log warning but continue - validation should have caught this at admission time + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "failed to parse retry delay", + "step", crdStep.ID, "retryDelay", crdStep.OnError.RetryDelay) + } else { stepError.RetryDelay = vmcpconfig.Duration(duration) } } From ea0923eae38e572f0802a67e5c7dc6661ebb1e7c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 13:32:40 +0000 Subject: [PATCH 3/3] Add ErrorAction constants for workflow error handling Define ErrorActionAbort, ErrorActionContinue, and ErrorActionRetry constants to replace string literals in webhook validation code. This addresses a goconst linter warning about repeated string usage and improves code maintainability. --- ...rtualmcpcompositetooldefinition_webhook.go | 14 +++++----- ...mcpcompositetooldefinition_webhook_test.go | 6 ++-- .../api/v1alpha1/virtualmcpserver_types.go | 12 ++++++++ .../api/v1alpha1/virtualmcpserver_webhook.go | 10 +++---- .../v1alpha1/virtualmcpserver_webhook_test.go | 10 +++---- .../pkg/vmcpconfig/converter_test.go | 28 +++++++++---------- 6 files changed, 46 insertions(+), 34 deletions(-) diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook.go b/cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook.go index 2800bbea3..a07447025 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook.go @@ -84,8 +84,8 @@ func (r *VirtualMCPCompositeToolDefinition) Validate() error { // Validate failure mode if r.Spec.FailureMode != "" { validModes := map[string]bool{ - "abort": true, - "continue": true, + ErrorActionAbort: true, + ErrorActionContinue: true, } if !validModes[r.Spec.FailureMode] { errors = append(errors, "spec.failureMode must be one of: abort, continue") @@ -270,19 +270,19 @@ func (*VirtualMCPCompositeToolDefinition) validateStepDependencies(index int, st // validateErrorHandling validates error handling configuration func (*VirtualMCPCompositeToolDefinition) validateErrorHandling(stepIndex int, errorHandling *ErrorHandling) error { if errorHandling.Action == "" { - return nil // Action is optional, defaults to "abort" + return nil // Action is optional, defaults to ErrorActionAbort } validActions := map[string]bool{ - "abort": true, - "continue": true, - "retry": true, + ErrorActionAbort: true, + ErrorActionContinue: true, + ErrorActionRetry: true, } if !validActions[errorHandling.Action] { return fmt.Errorf("spec.steps[%d].onError.action must be one of: abort, continue, retry", stepIndex) } - if errorHandling.Action == "retry" { + if errorHandling.Action == ErrorActionRetry { if errorHandling.MaxRetries < 1 { return fmt.Errorf("spec.steps[%d].onError.maxRetries must be at least 1 when action is retry", stepIndex) } diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook_test.go b/cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook_test.go index 23cac81b7..680515fd8 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook_test.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook_test.go @@ -363,7 +363,7 @@ func TestVirtualMCPCompositeToolDefinitionValidate(t *testing.T) { ID: "deploy", Tool: "kubectl.apply", OnError: &ErrorHandling{ - Action: "retry", + Action: ErrorActionRetry, MaxRetries: 3, }, }, @@ -383,7 +383,7 @@ func TestVirtualMCPCompositeToolDefinitionValidate(t *testing.T) { ID: "deploy", Tool: "kubectl.apply", OnError: &ErrorHandling{ - Action: "retry", + Action: ErrorActionRetry, MaxRetries: 0, }, }, @@ -455,7 +455,7 @@ func TestVirtualMCPCompositeToolDefinitionValidate(t *testing.T) { Spec: VirtualMCPCompositeToolDefinitionSpec{ Name: "continue_deploy", Description: "Deploy with continue mode", - FailureMode: "continue", + FailureMode: ErrorActionContinue, Steps: []WorkflowStep{ { ID: "deploy1", diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go index 3355bd848..d1e13fc5f 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go @@ -541,6 +541,18 @@ const ( WorkflowStepTypeElicitation = "elicitation" ) +// Error handling actions +const ( + // ErrorActionAbort aborts the workflow on error + ErrorActionAbort = "abort" + + // ErrorActionContinue continues the workflow on error + ErrorActionContinue = "continue" + + // ErrorActionRetry retries the step on error + ErrorActionRetry = "retry" +) + //+kubebuilder:object:root=true //+kubebuilder:subresource:status //+kubebuilder:resource:shortName=vmcp;virtualmcp diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go index e36a73054..0786b92bf 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go @@ -332,21 +332,21 @@ func validateStepErrorHandling(toolIndex, stepIndex int, step WorkflowStep) erro } validActions := map[string]bool{ - "abort": true, - "continue": true, - "retry": true, + ErrorActionAbort: true, + ErrorActionContinue: true, + ErrorActionRetry: true, } if !validActions[step.OnError.Action] { return fmt.Errorf("spec.compositeTools[%d].steps[%d].onError.action must be abort, continue, or retry", toolIndex, stepIndex) } - if step.OnError.Action == "retry" && step.OnError.MaxRetries < 1 { + if step.OnError.Action == ErrorActionRetry && step.OnError.MaxRetries < 1 { return fmt.Errorf("spec.compositeTools[%d].steps[%d].onError.maxRetries must be at least 1 when action is retry", toolIndex, stepIndex) } - if step.OnError.Action == "retry" && step.OnError.RetryDelay != "" { + if step.OnError.Action == ErrorActionRetry && step.OnError.RetryDelay != "" { if err := validateDuration(step.OnError.RetryDelay); err != nil { return fmt.Errorf("spec.compositeTools[%d].steps[%d].onError.retryDelay: %w", toolIndex, stepIndex, err) diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook_test.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook_test.go index 68e46c660..149c63580 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook_test.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook_test.go @@ -508,7 +508,7 @@ func TestValidateCompositeToolsWithDependencies(t *testing.T) { ID: "step1", Tool: "backend.tool1", OnError: &ErrorHandling{ - Action: "retry", + Action: ErrorActionRetry, MaxRetries: 3, }, }, @@ -533,7 +533,7 @@ func TestValidateCompositeToolsWithDependencies(t *testing.T) { ID: "step1", Tool: "backend.tool1", OnError: &ErrorHandling{ - Action: "retry", + Action: ErrorActionRetry, MaxRetries: 0, }, }, @@ -559,7 +559,7 @@ func TestValidateCompositeToolsWithDependencies(t *testing.T) { ID: "step1", Tool: "backend.tool1", OnError: &ErrorHandling{ - Action: "retry", + Action: ErrorActionRetry, MaxRetries: 3, RetryDelay: "5s", }, @@ -585,7 +585,7 @@ func TestValidateCompositeToolsWithDependencies(t *testing.T) { ID: "step1", Tool: "backend.tool1", OnError: &ErrorHandling{ - Action: "retry", + Action: ErrorActionRetry, MaxRetries: 3, RetryDelay: "1m30s", }, @@ -611,7 +611,7 @@ func TestValidateCompositeToolsWithDependencies(t *testing.T) { ID: "step1", Tool: "backend.tool1", OnError: &ErrorHandling{ - Action: "retry", + Action: ErrorActionRetry, MaxRetries: 3, RetryDelay: "invalid", }, diff --git a/cmd/thv-operator/pkg/vmcpconfig/converter_test.go b/cmd/thv-operator/pkg/vmcpconfig/converter_test.go index 7d6aaa865..55351813a 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter_test.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter_test.go @@ -253,72 +253,72 @@ func TestConverter_ConvertCompositeTools_ErrorHandling(t *testing.T) { { name: "with retry delay", errorHandling: &mcpv1alpha1.ErrorHandling{ - Action: "retry", + Action: mcpv1alpha1.ErrorActionRetry, MaxRetries: 3, RetryDelay: "5s", }, - expectedAction: "retry", + expectedAction: mcpv1alpha1.ErrorActionRetry, expectedRetry: 3, expectedDelay: vmcpconfig.Duration(5 * time.Second), }, { name: "with millisecond retry delay", errorHandling: &mcpv1alpha1.ErrorHandling{ - Action: "retry", + Action: mcpv1alpha1.ErrorActionRetry, MaxRetries: 5, RetryDelay: "500ms", }, - expectedAction: "retry", + expectedAction: mcpv1alpha1.ErrorActionRetry, expectedRetry: 5, expectedDelay: vmcpconfig.Duration(500 * time.Millisecond), }, { name: "with minute retry delay", errorHandling: &mcpv1alpha1.ErrorHandling{ - Action: "retry", + Action: mcpv1alpha1.ErrorActionRetry, MaxRetries: 2, RetryDelay: "1m", }, - expectedAction: "retry", + expectedAction: mcpv1alpha1.ErrorActionRetry, expectedRetry: 2, expectedDelay: vmcpconfig.Duration(1 * time.Minute), }, { name: "without retry delay", errorHandling: &mcpv1alpha1.ErrorHandling{ - Action: "retry", + Action: mcpv1alpha1.ErrorActionRetry, MaxRetries: 3, }, - expectedAction: "retry", + expectedAction: mcpv1alpha1.ErrorActionRetry, expectedRetry: 3, expectedDelay: vmcpconfig.Duration(0), }, { name: "abort action", errorHandling: &mcpv1alpha1.ErrorHandling{ - Action: "abort", + Action: mcpv1alpha1.ErrorActionAbort, }, - expectedAction: "abort", + expectedAction: mcpv1alpha1.ErrorActionAbort, expectedRetry: 0, expectedDelay: vmcpconfig.Duration(0), }, { name: "continue action", errorHandling: &mcpv1alpha1.ErrorHandling{ - Action: "continue", + Action: mcpv1alpha1.ErrorActionContinue, }, - expectedAction: "continue", + expectedAction: mcpv1alpha1.ErrorActionContinue, expectedRetry: 0, expectedDelay: vmcpconfig.Duration(0), }, { name: "invalid retry delay format is ignored", errorHandling: &mcpv1alpha1.ErrorHandling{ - Action: "retry", + Action: mcpv1alpha1.ErrorActionRetry, MaxRetries: 3, RetryDelay: "invalid", }, - expectedAction: "retry", + expectedAction: mcpv1alpha1.ErrorActionRetry, expectedRetry: 3, expectedDelay: vmcpconfig.Duration(0), },