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 3fe5d3d1a..0786b92bf 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go @@ -332,20 +332,27 @@ 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 == 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) + } + } + 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..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, }, }, @@ -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: ErrorActionRetry, + 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: ErrorActionRetry, + 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: ErrorActionRetry, + 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 { 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) } } 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), },