Skip to content

Commit c73c5ef

Browse files
jhrozekjhrozek
authored
Two minor fixes for vMCP retryDelay handling (#2795)
* 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 * 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. * 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.
1 parent 5818441 commit c73c5ef

File tree

7 files changed

+134
-31
lines changed

7 files changed

+134
-31
lines changed

‎cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,8 @@ func (r *VirtualMCPCompositeToolDefinition) Validate() error {
8484
// Validate failure mode
8585
if r.Spec.FailureMode != "" {
8686
validModes := map[string]bool{
87-
"abort": true,
88-
"continue": true,
87+
ErrorActionAbort: true,
88+
ErrorActionContinue: true,
8989
}
9090
if !validModes[r.Spec.FailureMode] {
9191
errors = append(errors, "spec.failureMode must be one of: abort, continue")
@@ -270,19 +270,19 @@ func (*VirtualMCPCompositeToolDefinition) validateStepDependencies(index int, st
270270
// validateErrorHandling validates error handling configuration
271271
func (*VirtualMCPCompositeToolDefinition) validateErrorHandling(stepIndex int, errorHandling *ErrorHandling) error {
272272
if errorHandling.Action == "" {
273-
return nil // Action is optional, defaults to "abort"
273+
return nil // Action is optional, defaults to ErrorActionAbort
274274
}
275275

276276
validActions := map[string]bool{
277-
"abort": true,
278-
"continue": true,
279-
"retry": true,
277+
ErrorActionAbort: true,
278+
ErrorActionContinue: true,
279+
ErrorActionRetry: true,
280280
}
281281
if !validActions[errorHandling.Action] {
282282
return fmt.Errorf("spec.steps[%d].onError.action must be one of: abort, continue, retry", stepIndex)
283283
}
284284

285-
if errorHandling.Action == "retry" {
285+
if errorHandling.Action == ErrorActionRetry {
286286
if errorHandling.MaxRetries < 1 {
287287
return fmt.Errorf("spec.steps[%d].onError.maxRetries must be at least 1 when action is retry", stepIndex)
288288
}

‎cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ func TestVirtualMCPCompositeToolDefinitionValidate(t *testing.T) {
363363
ID: "deploy",
364364
Tool: "kubectl.apply",
365365
OnError: &ErrorHandling{
366-
Action: "retry",
366+
Action: ErrorActionRetry,
367367
MaxRetries: 3,
368368
},
369369
},
@@ -383,7 +383,7 @@ func TestVirtualMCPCompositeToolDefinitionValidate(t *testing.T) {
383383
ID: "deploy",
384384
Tool: "kubectl.apply",
385385
OnError: &ErrorHandling{
386-
Action: "retry",
386+
Action: ErrorActionRetry,
387387
MaxRetries: 0,
388388
},
389389
},
@@ -455,7 +455,7 @@ func TestVirtualMCPCompositeToolDefinitionValidate(t *testing.T) {
455455
Spec: VirtualMCPCompositeToolDefinitionSpec{
456456
Name: "continue_deploy",
457457
Description: "Deploy with continue mode",
458-
FailureMode: "continue",
458+
FailureMode: ErrorActionContinue,
459459
Steps: []WorkflowStep{
460460
{
461461
ID: "deploy1",

‎cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -541,6 +541,18 @@ const (
541541
WorkflowStepTypeElicitation = "elicitation"
542542
)
543543

544+
// Error handling actions
545+
const (
546+
// ErrorActionAbort aborts the workflow on error
547+
ErrorActionAbort = "abort"
548+
549+
// ErrorActionContinue continues the workflow on error
550+
ErrorActionContinue = "continue"
551+
552+
// ErrorActionRetry retries the step on error
553+
ErrorActionRetry = "retry"
554+
)
555+
544556
//+kubebuilder:object:root=true
545557
//+kubebuilder:subresource:status
546558
//+kubebuilder:resource:shortName=vmcp;virtualmcp

‎cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -332,20 +332,27 @@ func validateStepErrorHandling(toolIndex, stepIndex int, step WorkflowStep) erro
332332
}
333333

334334
validActions := map[string]bool{
335-
"abort": true,
336-
"continue": true,
337-
"retry": true,
335+
ErrorActionAbort: true,
336+
ErrorActionContinue: true,
337+
ErrorActionRetry: true,
338338
}
339339
if !validActions[step.OnError.Action] {
340340
return fmt.Errorf("spec.compositeTools[%d].steps[%d].onError.action must be abort, continue, or retry",
341341
toolIndex, stepIndex)
342342
}
343343

344-
if step.OnError.Action == "retry" && step.OnError.MaxRetries < 1 {
344+
if step.OnError.Action == ErrorActionRetry && step.OnError.MaxRetries < 1 {
345345
return fmt.Errorf("spec.compositeTools[%d].steps[%d].onError.maxRetries must be at least 1 when action is retry",
346346
toolIndex, stepIndex)
347347
}
348348

349+
if step.OnError.Action == ErrorActionRetry && step.OnError.RetryDelay != "" {
350+
if err := validateDuration(step.OnError.RetryDelay); err != nil {
351+
return fmt.Errorf("spec.compositeTools[%d].steps[%d].onError.retryDelay: %w",
352+
toolIndex, stepIndex, err)
353+
}
354+
}
355+
349356
return nil
350357
}
351358

‎cmd/thv-operator/api/v1alpha1/virtualmcpserver_webhook_test.go‎

Lines changed: 81 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -508,7 +508,7 @@ func TestValidateCompositeToolsWithDependencies(t *testing.T) {
508508
ID: "step1",
509509
Tool: "backend.tool1",
510510
OnError: &ErrorHandling{
511-
Action: "retry",
511+
Action: ErrorActionRetry,
512512
MaxRetries: 3,
513513
},
514514
},
@@ -533,7 +533,7 @@ func TestValidateCompositeToolsWithDependencies(t *testing.T) {
533533
ID: "step1",
534534
Tool: "backend.tool1",
535535
OnError: &ErrorHandling{
536-
Action: "retry",
536+
Action: ErrorActionRetry,
537537
MaxRetries: 0,
538538
},
539539
},
@@ -545,6 +545,85 @@ func TestValidateCompositeToolsWithDependencies(t *testing.T) {
545545
wantErr: true,
546546
errMsg: "spec.compositeTools[0].steps[0].onError.maxRetries must be at least 1 when action is retry",
547547
},
548+
{
549+
name: "valid error handling with retryDelay",
550+
vmcp: &VirtualMCPServer{
551+
Spec: VirtualMCPServerSpec{
552+
GroupRef: GroupRef{Name: "test-group"},
553+
CompositeTools: []CompositeToolSpec{
554+
{
555+
Name: "test-tool",
556+
Description: "Test composite tool",
557+
Steps: []WorkflowStep{
558+
{
559+
ID: "step1",
560+
Tool: "backend.tool1",
561+
OnError: &ErrorHandling{
562+
Action: ErrorActionRetry,
563+
MaxRetries: 3,
564+
RetryDelay: "5s",
565+
},
566+
},
567+
},
568+
},
569+
},
570+
},
571+
},
572+
wantErr: false,
573+
},
574+
{
575+
name: "valid error handling with complex retryDelay",
576+
vmcp: &VirtualMCPServer{
577+
Spec: VirtualMCPServerSpec{
578+
GroupRef: GroupRef{Name: "test-group"},
579+
CompositeTools: []CompositeToolSpec{
580+
{
581+
Name: "test-tool",
582+
Description: "Test composite tool",
583+
Steps: []WorkflowStep{
584+
{
585+
ID: "step1",
586+
Tool: "backend.tool1",
587+
OnError: &ErrorHandling{
588+
Action: ErrorActionRetry,
589+
MaxRetries: 3,
590+
RetryDelay: "1m30s",
591+
},
592+
},
593+
},
594+
},
595+
},
596+
},
597+
},
598+
wantErr: false,
599+
},
600+
{
601+
name: "invalid error handling - invalid retryDelay format",
602+
vmcp: &VirtualMCPServer{
603+
Spec: VirtualMCPServerSpec{
604+
GroupRef: GroupRef{Name: "test-group"},
605+
CompositeTools: []CompositeToolSpec{
606+
{
607+
Name: "test-tool",
608+
Description: "Test composite tool",
609+
Steps: []WorkflowStep{
610+
{
611+
ID: "step1",
612+
Tool: "backend.tool1",
613+
OnError: &ErrorHandling{
614+
Action: ErrorActionRetry,
615+
MaxRetries: 3,
616+
RetryDelay: "invalid",
617+
},
618+
},
619+
},
620+
},
621+
},
622+
},
623+
},
624+
wantErr: true,
625+
errMsg: "spec.compositeTools[0].steps[0].onError.retryDelay: invalid duration format \"invalid\", expected format like '30s', '5m', '1h', '1h30m'",
626+
},
548627
}
549628

550629
for _, tt := range tests {

‎cmd/thv-operator/pkg/vmcpconfig/converter.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,12 @@ func (*Converter) convertCompositeTools(
309309
RetryCount: crdStep.OnError.MaxRetries,
310310
}
311311
if crdStep.OnError.RetryDelay != "" {
312-
if duration, err := time.ParseDuration(crdStep.OnError.RetryDelay); err == nil {
312+
if duration, err := time.ParseDuration(crdStep.OnError.RetryDelay); err != nil {
313+
// Log warning but continue - validation should have caught this at admission time
314+
ctxLogger := log.FromContext(ctx)
315+
ctxLogger.Error(err, "failed to parse retry delay",
316+
"step", crdStep.ID, "retryDelay", crdStep.OnError.RetryDelay)
317+
} else {
313318
stepError.RetryDelay = vmcpconfig.Duration(duration)
314319
}
315320
}

‎cmd/thv-operator/pkg/vmcpconfig/converter_test.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -253,72 +253,72 @@ func TestConverter_ConvertCompositeTools_ErrorHandling(t *testing.T) {
253253
{
254254
name: "with retry delay",
255255
errorHandling: &mcpv1alpha1.ErrorHandling{
256-
Action: "retry",
256+
Action: mcpv1alpha1.ErrorActionRetry,
257257
MaxRetries: 3,
258258
RetryDelay: "5s",
259259
},
260-
expectedAction: "retry",
260+
expectedAction: mcpv1alpha1.ErrorActionRetry,
261261
expectedRetry: 3,
262262
expectedDelay: vmcpconfig.Duration(5 * time.Second),
263263
},
264264
{
265265
name: "with millisecond retry delay",
266266
errorHandling: &mcpv1alpha1.ErrorHandling{
267-
Action: "retry",
267+
Action: mcpv1alpha1.ErrorActionRetry,
268268
MaxRetries: 5,
269269
RetryDelay: "500ms",
270270
},
271-
expectedAction: "retry",
271+
expectedAction: mcpv1alpha1.ErrorActionRetry,
272272
expectedRetry: 5,
273273
expectedDelay: vmcpconfig.Duration(500 * time.Millisecond),
274274
},
275275
{
276276
name: "with minute retry delay",
277277
errorHandling: &mcpv1alpha1.ErrorHandling{
278-
Action: "retry",
278+
Action: mcpv1alpha1.ErrorActionRetry,
279279
MaxRetries: 2,
280280
RetryDelay: "1m",
281281
},
282-
expectedAction: "retry",
282+
expectedAction: mcpv1alpha1.ErrorActionRetry,
283283
expectedRetry: 2,
284284
expectedDelay: vmcpconfig.Duration(1 * time.Minute),
285285
},
286286
{
287287
name: "without retry delay",
288288
errorHandling: &mcpv1alpha1.ErrorHandling{
289-
Action: "retry",
289+
Action: mcpv1alpha1.ErrorActionRetry,
290290
MaxRetries: 3,
291291
},
292-
expectedAction: "retry",
292+
expectedAction: mcpv1alpha1.ErrorActionRetry,
293293
expectedRetry: 3,
294294
expectedDelay: vmcpconfig.Duration(0),
295295
},
296296
{
297297
name: "abort action",
298298
errorHandling: &mcpv1alpha1.ErrorHandling{
299-
Action: "abort",
299+
Action: mcpv1alpha1.ErrorActionAbort,
300300
},
301-
expectedAction: "abort",
301+
expectedAction: mcpv1alpha1.ErrorActionAbort,
302302
expectedRetry: 0,
303303
expectedDelay: vmcpconfig.Duration(0),
304304
},
305305
{
306306
name: "continue action",
307307
errorHandling: &mcpv1alpha1.ErrorHandling{
308-
Action: "continue",
308+
Action: mcpv1alpha1.ErrorActionContinue,
309309
},
310-
expectedAction: "continue",
310+
expectedAction: mcpv1alpha1.ErrorActionContinue,
311311
expectedRetry: 0,
312312
expectedDelay: vmcpconfig.Duration(0),
313313
},
314314
{
315315
name: "invalid retry delay format is ignored",
316316
errorHandling: &mcpv1alpha1.ErrorHandling{
317-
Action: "retry",
317+
Action: mcpv1alpha1.ErrorActionRetry,
318318
MaxRetries: 3,
319319
RetryDelay: "invalid",
320320
},
321-
expectedAction: "retry",
321+
expectedAction: mcpv1alpha1.ErrorActionRetry,
322322
expectedRetry: 3,
323323
expectedDelay: vmcpconfig.Duration(0),
324324
},

0 commit comments

Comments
 (0)