Enhance v1beta1 validation code for taskrun 🍸

This make a better use of knative.dev/pkg apis errors package in order
to make the code simpler, *and* the report better.

- This allows reporting multiple validation error at once — so, one
  failure message (webhook error) would be more useful to the user.
- This reduce `if err != nil` path, simplifying the code.

This commit tackles `TaskRun` 🙃

Signed-off-by: Vincent Demeester <[email protected]>

Author: vdemeester

pkg/apis/pipeline/v1beta1/taskrun_validation.go

@@ -22,7 +22,6 @@ import (
 	"strings"
 
 	"github.com/tektoncd/pipeline/pkg/apis/validate"
-	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"knative.dev/pkg/apis"
 )
@@ -31,89 +30,69 @@ var _ apis.Validatable = (*TaskRun)(nil)
 
 // Validate taskrun
 func (tr *TaskRun) Validate(ctx context.Context) *apis.FieldError {
-	if err := validate.ObjectMetadata(tr.GetObjectMeta()).ViaField("metadata"); err != nil {
-		return err
-	}
-	return tr.Spec.Validate(ctx)
+	errs := validate.ObjectMetadata(tr.GetObjectMeta()).ViaField("metadata")
+	return errs.Also(tr.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
 }
 
 // Validate taskrun spec
-func (ts *TaskRunSpec) Validate(ctx context.Context) *apis.FieldError {
-	if equality.Semantic.DeepEqual(ts, &TaskRunSpec{}) {
-		return apis.ErrMissingField("spec")
-	}
-
+func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 	// can't have both taskRef and taskSpec at the same time
 	if (ts.TaskRef != nil && ts.TaskRef.Name != "") && ts.TaskSpec != nil {
-		return apis.ErrDisallowedFields("spec.taskref", "spec.taskspec")
+		errs = errs.Also(apis.ErrDisallowedFields("taskref", "taskspec"))
 	}
 
 	// Check that one of TaskRef and TaskSpec is present
 	if (ts.TaskRef == nil || (ts.TaskRef != nil && ts.TaskRef.Name == "")) && ts.TaskSpec == nil {
-		return apis.ErrMissingField("spec.taskref.name", "spec.taskspec")
+		errs = errs.Also(apis.ErrMissingField("taskref.name", "taskspec"))
 	}
 
 	// Validate TaskSpec if it's present
 	if ts.TaskSpec != nil {
-		if err := ts.TaskSpec.Validate(ctx).ViaField("taskspec"); err != nil {
-			return err
-		}
+		errs = errs.Also(ts.TaskSpec.Validate(ctx).ViaField("taskspec"))
 	}
 
-	if err := validateParameters(ts.Params); err != nil {
-		return err
-	}
-
-	if err := validateWorkspaceBindings(ctx, ts.Workspaces); err != nil {
-		return err
-	}
-
-	// Validate Resources declaration
-	if err := ts.Resources.Validate(ctx); err != nil {
-		return err
-	}
+	errs = errs.Also(validateParameters(ts.Params).ViaField("params"))
+	errs = errs.Also(validateWorkspaceBindings(ctx, ts.Workspaces).ViaField("workspaces"))
+	errs = errs.Also(ts.Resources.Validate(ctx).ViaField("resources"))
 
 	if ts.Status != "" {
 		if ts.Status != TaskRunSpecStatusCancelled {
-			return apis.ErrInvalidValue(fmt.Sprintf("%s should be %s", ts.Status, TaskRunSpecStatusCancelled), "spec.status")
+			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s should be %s", ts.Status, TaskRunSpecStatusCancelled), "status"))
 		}
 	}
-
 	if ts.Timeout != nil {
 		// timeout should be a valid duration of at least 0.
 		if ts.Timeout.Duration < 0 {
-			return apis.ErrInvalidValue(fmt.Sprintf("%s should be >= 0", ts.Timeout.Duration.String()), "spec.timeout")
+			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s should be >= 0", ts.Timeout.Duration.String()), "timeout"))
 		}
 	}
 
-	return nil
+	return errs
 }
 
 // validateWorkspaceBindings makes sure the volumes provided for the Task's declared workspaces make sense.
-func validateWorkspaceBindings(ctx context.Context, wb []WorkspaceBinding) *apis.FieldError {
+func validateWorkspaceBindings(ctx context.Context, wb []WorkspaceBinding) (errs *apis.FieldError) {
 	seen := sets.NewString()
-	for _, w := range wb {
+	for idx, w := range wb {
 		if seen.Has(w.Name) {
-			return apis.ErrMultipleOneOf("spec.workspaces.name")
+			errs = errs.Also(apis.ErrMultipleOneOf("name").ViaIndex(idx))
 		}
 		seen.Insert(w.Name)
 
-		if err := w.Validate(ctx).ViaField("workspace"); err != nil {
-			return err
-		}
+		errs = errs.Also(w.Validate(ctx).ViaIndex(idx))
 	}
 
-	return nil
+	return errs
 }
 
-func validateParameters(params []Param) *apis.FieldError {
+func validateParameters(params []Param) (errs *apis.FieldError) {
 	// Template must not duplicate parameter names.
 	seen := sets.NewString()
 	for _, p := range params {
 		if seen.Has(strings.ToLower(p.Name)) {
-			return apis.ErrMultipleOneOf("spec.params.name")
+			errs = errs.Also(apis.ErrMultipleOneOf("name").ViaKey(p.Name))
 		}
 		seen.Insert(p.Name)
 	}
-	return nil
+	return errs
 }

pkg/apis/pipeline/v1beta1/taskrun_validation_test.go

@@ -38,13 +38,16 @@ func TestTaskRun_Invalidate(t *testing.T) {
 	}{{
 		name: "invalid taskspec",
 		task: &v1beta1.TaskRun{},
-		want: apis.ErrMissingField("spec"),
+		want: apis.ErrMissingField("spec.taskref.name", "spec.taskspec"),
 	}, {
 		name: "invalid taskrun metadata",
 		task: &v1beta1.TaskRun{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: "task.name",
 			},
+			Spec: v1beta1.TaskRunSpec{
+				TaskRef: &v1beta1.TaskRef{Name: "task"},
+			},
 		},
 		want: &apis.FieldError{
 			Message: "Invalid resource name: special character . must not be present",
@@ -92,7 +95,7 @@ func TestTaskRun_Workspaces_Invalid(t *testing.T) {
 				}},
 			},
 		},
-		wantErr: apis.ErrMissingField("workspace.persistentvolumeclaim.claimname"),
+		wantErr: apis.ErrMissingField("spec.workspaces[0].persistentvolumeclaim.claimname"),
 	}, {
 		name: "bind same workspace twice",
 		tr: &v1beta1.TaskRun{
@@ -108,7 +111,7 @@ func TestTaskRun_Workspaces_Invalid(t *testing.T) {
 				}},
 			},
 		},
-		wantErr: apis.ErrMultipleOneOf("spec.workspaces.name"),
+		wantErr: apis.ErrMultipleOneOf("spec.workspaces[1].name"),
 	}}
 	for _, ts := range tests {
 		t.Run(ts.name, func(t *testing.T) {
@@ -131,13 +134,13 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
 	}{{
 		name:    "invalid taskspec",
 		spec:    v1beta1.TaskRunSpec{},
-		wantErr: apis.ErrMissingField("spec"),
+		wantErr: apis.ErrMissingField("taskref.name", "taskspec"),
 	}, {
 		name: "invalid taskref name",
 		spec: v1beta1.TaskRunSpec{
 			TaskRef: &v1beta1.TaskRef{},
 		},
-		wantErr: apis.ErrMissingField("spec.taskref.name, spec.taskspec"),
+		wantErr: apis.ErrMissingField("taskref.name, taskspec"),
 	}, {
 		name: "invalid taskref and taskspec together",
 		spec: v1beta1.TaskRunSpec{
@@ -151,7 +154,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
 				}}},
 			},
 		},
-		wantErr: apis.ErrDisallowedFields("spec.taskspec", "spec.taskref"),
+		wantErr: apis.ErrDisallowedFields("taskspec", "taskref"),
 	}, {
 		name: "negative pipeline timeout",
 		spec: v1beta1.TaskRunSpec{
@@ -160,7 +163,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
 			},
 			Timeout: &metav1.Duration{Duration: -48 * time.Hour},
 		},
-		wantErr: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeout"),
+		wantErr: apis.ErrInvalidValue("-48h0m0s should be >= 0", "timeout"),
 	}, {
 		name: "wrong taskrun cancel",
 		spec: v1beta1.TaskRunSpec{
@@ -169,7 +172,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
 			},
 			Status: "TaskRunCancell",
 		},
-		wantErr: apis.ErrInvalidValue("TaskRunCancell should be TaskRunCancelled", "spec.status"),
+		wantErr: apis.ErrInvalidValue("TaskRunCancell should be TaskRunCancelled", "status"),
 	}, {
 		name: "invalid taskspec",
 		spec: v1beta1.TaskRunSpec{
@@ -189,15 +192,15 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
 		name: "invalid params",
 		spec: v1beta1.TaskRunSpec{
 			Params: []v1beta1.Param{{
-				Name:  "name",
+				Name:  "myname",
 				Value: *v1beta1.NewArrayOrString("value"),
 			}, {
-				Name:  "name",
+				Name:  "myname",
 				Value: *v1beta1.NewArrayOrString("value"),
 			}},
 			TaskRef: &v1beta1.TaskRef{Name: "mytask"},
 		},
-		wantErr: apis.ErrMultipleOneOf("spec.params.name"),
+		wantErr: apis.ErrMultipleOneOf("params[myname].name"),
 	}}
 	for _, ts := range tests {
 		t.Run(ts.name, func(t *testing.T) {