Skip to content

Commit

Permalink
Label user error for failed TaskRunStatus message
Browse files Browse the repository at this point in the history
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
  • Loading branch information
JeromeJu authored and tekton-robot committed Jan 30, 2024
1 parent 607eb3d commit 018c4b8
Show file tree
Hide file tree
Showing 7 changed files with 63 additions and 20 deletions.
16 changes: 14 additions & 2 deletions pkg/apis/pipeline/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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()
}
26 changes: 26 additions & 0 deletions pkg/apis/pipeline/errors/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
}
3 changes: 2 additions & 1 deletion pkg/apis/pipeline/v1/taskrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion pkg/reconciler/taskrun/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
Expand Down
9 changes: 5 additions & 4 deletions pkg/reconciler/taskrun/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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"
Expand Down Expand Up @@ -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:
Expand Down
17 changes: 9 additions & 8 deletions pkg/reconciler/taskrun/validate_taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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()))
}
}
}
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
}
Expand Down
9 changes: 5 additions & 4 deletions pkg/workspace/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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))
}
}

Expand All @@ -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))
}
}

Expand All @@ -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
}

0 comments on commit 018c4b8

Please sign in to comment.