From 018c4b84bf850d4cc1565863dab281ad07abc069 Mon Sep 17 00:00:00 2001 From: JeromeJu Date: Wed, 24 Jan 2024 18:48:14 +0000 Subject: [PATCH] Label user error for failed TaskRunStatus message This commit follows up #7475 and labels user error for failed TaskRun status messages. It marks off user errors in the taskrun reconciler and communicate to users via TaskRunStatus condition messages. /kind misc part of #7276 --- pkg/apis/pipeline/errors/errors.go | 16 +++++++++++-- pkg/apis/pipeline/errors/errors_test.go | 26 ++++++++++++++++++++++ pkg/apis/pipeline/v1/taskrun_types.go | 3 ++- pkg/reconciler/taskrun/taskrun.go | 3 ++- pkg/reconciler/taskrun/taskrun_test.go | 9 ++++---- pkg/reconciler/taskrun/validate_taskrun.go | 17 +++++++------- pkg/workspace/validate.go | 9 ++++---- 7 files changed, 63 insertions(+), 20 deletions(-) diff --git a/pkg/apis/pipeline/errors/errors.go b/pkg/apis/pipeline/errors/errors.go index a7dd38dad98..f81dd2e5f82 100644 --- a/pkg/apis/pipeline/errors/errors.go +++ b/pkg/apis/pipeline/errors/errors.go @@ -13,7 +13,9 @@ limitations under the License. package errors -const userErrorLabel = "[User error] " +import "errors" + +const UserErrorLabel = "[User error] " type UserError struct { Reason string @@ -44,7 +46,7 @@ func newUserError(reason string, err error) *UserError { // WrapUserError wraps the original error with the user error label func WrapUserError(err error) error { - return newUserError(userErrorLabel, err) + return newUserError(UserErrorLabel, err) } // LabelUserError labels the failure RunStatus message if any of its error messages has been @@ -59,3 +61,13 @@ func LabelUserError(messageFormat string, messageA []interface{}) string { } return messageFormat } + +// GetErrorMessage returns the error message with the user error label if it is of type user +// error +func GetErrorMessage(err error) string { + var ue *UserError + if errors.As(err, &ue) { + return ue.Reason + err.Error() + } + return err.Error() +} diff --git a/pkg/apis/pipeline/errors/errors_test.go b/pkg/apis/pipeline/errors/errors_test.go index 95faded9214..de7dd8fca46 100644 --- a/pkg/apis/pipeline/errors/errors_test.go +++ b/pkg/apis/pipeline/errors/errors_test.go @@ -96,3 +96,29 @@ func TestLabelsUserError(t *testing.T) { } } } + +func TestGetErrorMess(t *testing.T) { + original := errors.New("orignal error") + tcs := []struct { + description string + err error + expected string + }{{ + description: "error messages with user error", + err: pipelineErrors.WrapUserError(original), + expected: "[User error] " + original.Error(), + }, { + description: "error messages without user error", + err: original, + expected: original.Error(), + }} + for _, tc := range tcs { + { + msg := pipelineErrors.GetErrorMessage(tc.err) + + if msg != tc.expected { + t.Errorf("failure messageFormat expected: %s; but got %s", tc.expected, msg) + } + } + } +} diff --git a/pkg/apis/pipeline/v1/taskrun_types.go b/pkg/apis/pipeline/v1/taskrun_types.go index 7c3cf232ee5..ff94ed6c330 100644 --- a/pkg/apis/pipeline/v1/taskrun_types.go +++ b/pkg/apis/pipeline/v1/taskrun_types.go @@ -21,6 +21,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/config" apisconfig "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" + pipelineErrors "github.com/tektoncd/pipeline/pkg/apis/pipeline/errors" pod "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -242,7 +243,7 @@ func (trs *TaskRunStatus) MarkResourceFailed(reason TaskRunReason, err error) { Type: apis.ConditionSucceeded, Status: corev1.ConditionFalse, Reason: reason.String(), - Message: err.Error(), + Message: pipelineErrors.GetErrorMessage(err), }) succeeded := trs.GetCondition(apis.ConditionSucceeded) trs.CompletionTime = &succeeded.LastTransitionTime.Inner diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 09ca1b7a151..edbf36c1c37 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -28,6 +28,7 @@ import ( "github.com/tektoncd/pipeline/internal/sidecarlogresults" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" + pipelineErrors "github.com/tektoncd/pipeline/pkg/apis/pipeline/errors" "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" @@ -196,7 +197,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1.TaskRun) pkgrecon logger.Errorf("Reconcile: %v", err.Error()) if errors.Is(err, sidecarlogresults.ErrSizeExceeded) { cfg := config.FromContextOrDefaults(ctx) - message := fmt.Sprintf("TaskRun \"%q\" failed: results exceeded size limit %d bytes", tr.Name, cfg.FeatureFlags.MaxResultSize) + message := fmt.Sprintf("%s TaskRun \"%q\" failed: results exceeded size limit %d bytes", pipelineErrors.UserErrorLabel, tr.Name, cfg.FeatureFlags.MaxResultSize) err := c.failTaskRun(ctx, tr, v1.TaskRunReasonResultLargerThanAllowedLimit, message) return c.finishReconcileUpdateEmitEvents(ctx, tr, before, err) } diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index c681f8ae04b..684e6103621 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -40,6 +40,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/config" cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing" "github.com/tektoncd/pipeline/pkg/apis/pipeline" + pipelineErrors "github.com/tektoncd/pipeline/pkg/apis/pipeline/errors" "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" @@ -1666,7 +1667,7 @@ status: type: string value: aResultValue `) - failedOnReconcileFailureTaskRun = parse.MustParseV1TaskRun(t, ` + failedOnReconcileFailureTaskRun = parse.MustParseV1TaskRun(t, fmt.Sprintf(` metadata: name: test-taskrun-results-type-mismatched namespace: foo @@ -1679,14 +1680,14 @@ status: - reason: ToBeRetried status: Unknown type: Succeeded - message: "Provided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\"" + message: "%sProvided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\"" sideCars: retriesStatus: - conditions: - reason: TaskRunValidationFailed status: "False" type: "Succeeded" - message: "Provided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\"" + message: "%sProvided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\"" startTime: "2021-12-31T23:59:59Z" completionTime: "2022-01-01T00:00:00Z" podName: "test-taskrun-results-type-mismatched-pod" @@ -1714,7 +1715,7 @@ status: ResultExtractionMethod: "termination-message" MaxResultSize: 4096 Coschedule: "workspaces" -`) +`, pipelineErrors.UserErrorLabel, pipelineErrors.UserErrorLabel)) reconciliatonError = fmt.Errorf("1 error occurred:\n\t* Provided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\"") toBeRetriedTaskRun = parse.MustParseV1TaskRun(t, ` metadata: diff --git a/pkg/reconciler/taskrun/validate_taskrun.go b/pkg/reconciler/taskrun/validate_taskrun.go index e7bcb94812c..9407108a4a5 100644 --- a/pkg/reconciler/taskrun/validate_taskrun.go +++ b/pkg/reconciler/taskrun/validate_taskrun.go @@ -23,6 +23,7 @@ import ( "strings" "github.com/hashicorp/go-multierror" + pipelineErrors "github.com/tektoncd/pipeline/pkg/apis/pipeline/errors" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/pkg/list" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" @@ -170,7 +171,7 @@ func ValidateResolvedTask(ctx context.Context, params []v1.Param, matrix *v1.Mat paramSpecs = rtr.TaskSpec.Params } if err := validateParams(ctx, paramSpecs, params, matrix.GetAllParams()); err != nil { - return fmt.Errorf("invalid input params for task %s: %w", rtr.TaskName, err) + return pipelineErrors.WrapUserError(fmt.Errorf("invalid input params for task %s: %w", rtr.TaskName, err)) } return nil } @@ -198,7 +199,7 @@ func ValidateEnumParam(ctx context.Context, params []v1.Param, paramSpecs v1.Par } if !slices.Contains(paramSpecNameToEnum[p.Name], p.Value.StringVal) { - return fmt.Errorf("param `%s` value: %s is not in the enum list", p.Name, p.Value.StringVal) + return pipelineErrors.WrapUserError(fmt.Errorf("param `%s` value: %s is not in the enum list", p.Name, p.Value.StringVal)) } } return nil @@ -211,13 +212,13 @@ func validateTaskSpecRequestResources(taskSpec *v1.TaskSpec) error { // First validate the limit in step if limit, ok := step.ComputeResources.Limits[k]; ok { if (&limit).Cmp(request) == -1 { - return fmt.Errorf("invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String()) + return pipelineErrors.WrapUserError(fmt.Errorf("invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String())) } } else if taskSpec.StepTemplate != nil { // If step doesn't configure the limit, validate the limit in stepTemplate if limit, ok := taskSpec.StepTemplate.ComputeResources.Limits[k]; ok { if (&limit).Cmp(request) == -1 { - return fmt.Errorf("invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String()) + return pipelineErrors.WrapUserError(fmt.Errorf("invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String())) } } } @@ -243,7 +244,7 @@ func validateStepOverrides(ts *v1.TaskSpec, trs *v1.TaskRunSpec) error { } for _, stepOverride := range trs.StepSpecs { if !stepNames.Has(stepOverride.Name) { - err = multierror.Append(err, fmt.Errorf("invalid StepOverride: No Step named %s", stepOverride.Name)) + err = multierror.Append(err, pipelineErrors.WrapUserError(fmt.Errorf("invalid StepOverride: No Step named %s", stepOverride.Name))) } } return err @@ -257,7 +258,7 @@ func validateSidecarOverrides(ts *v1.TaskSpec, trs *v1.TaskRunSpec) error { } for _, sidecarOverride := range trs.SidecarSpecs { if !sidecarNames.Has(sidecarOverride.Name) { - err = multierror.Append(err, fmt.Errorf("invalid SidecarOverride: No Sidecar named %s", sidecarOverride.Name)) + err = multierror.Append(err, pipelineErrors.WrapUserError(fmt.Errorf("invalid SidecarOverride: No Sidecar named %s", sidecarOverride.Name))) } } return err @@ -281,12 +282,12 @@ func validateTaskRunResults(tr *v1.TaskRun, resolvedTaskSpec *v1.TaskSpec) error s = append(s, fmt.Sprintf(" \"%v\": %v", k, v)) } sort.Strings(s) - return fmt.Errorf("Provided results don't match declared results; may be invalid JSON or missing result declaration: %v", strings.Join(s, ",")) + return pipelineErrors.WrapUserError(fmt.Errorf("Provided results don't match declared results; may be invalid JSON or missing result declaration: %v", strings.Join(s, ","))) } // When get the results, for object value need to check if they have missing keys. if missingKeysObjectNames := missingKeysofObjectResults(tr, specResults); len(missingKeysObjectNames) != 0 { - return fmt.Errorf("missing keys for these results which are required in TaskResult's properties %v", missingKeysObjectNames) + return pipelineErrors.WrapUserError(fmt.Errorf("missing keys for these results which are required in TaskResult's properties %v", missingKeysObjectNames)) } return nil } diff --git a/pkg/workspace/validate.go b/pkg/workspace/validate.go index c048a5c0710..69a7d8b8ccf 100644 --- a/pkg/workspace/validate.go +++ b/pkg/workspace/validate.go @@ -20,6 +20,7 @@ import ( "context" "fmt" + pipelineErrors "github.com/tektoncd/pipeline/pkg/apis/pipeline/errors" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "k8s.io/apimachinery/pkg/util/sets" ) @@ -31,7 +32,7 @@ func ValidateBindings(ctx context.Context, decls []v1.WorkspaceDeclaration, bind // reason we'll invoke the same validation here. for _, b := range binds { if err := b.Validate(ctx); err != nil { - return fmt.Errorf("binding %q is invalid: %w", b.Name, err) + return pipelineErrors.WrapUserError(fmt.Errorf("binding %q is invalid: %w", b.Name, err)) } } @@ -49,12 +50,12 @@ func ValidateBindings(ctx context.Context, decls []v1.WorkspaceDeclaration, bind continue } if !bindNames.Has(decl.Name) { - return fmt.Errorf("declared workspace %q is required but has not been bound", decl.Name) + return pipelineErrors.WrapUserError(fmt.Errorf("declared workspace %q is required but has not been bound", decl.Name)) } } for _, bind := range binds { if !declNames.Has(bind.Name) { - return fmt.Errorf("workspace binding %q does not match any declared workspace", bind.Name) + return pipelineErrors.WrapUserError(fmt.Errorf("workspace binding %q does not match any declared workspace", bind.Name)) } } @@ -78,7 +79,7 @@ func ValidateOnlyOnePVCIsUsed(wb []v1.WorkspaceBinding) error { } if len(workspaceVolumes) > 1 { - return fmt.Errorf("more than one PersistentVolumeClaim is bound") + return pipelineErrors.WrapUserError(fmt.Errorf("more than one PersistentVolumeClaim is bound")) } return nil }