Fail taskrun when results validation fails

This commit adds the ReasonFailedValidation into taskrun status and fail
the taskrun when results validation fails. Before this commit users can
only see error messages in logs but the taskrun are completed like no
errors happen.

add tests

pkg/reconciler/taskrun/taskrun.go

@@ -503,6 +503,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, rtr *re
 	}
 
 	if err := validateTaskRunResults(tr, rtr.TaskSpec); err != nil {
+		tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
 		return err
 	}
 

pkg/reconciler/taskrun/taskrun_test.go

@@ -132,6 +132,21 @@ var (
 			Steps: []v1beta1.Step{simpleStep},
 		},
 	}
+	resultsTask = &v1beta1.Task{
+		ObjectMeta: objectMeta("test-results-task", "foo"),
+		Spec: v1beta1.TaskSpec{
+			Steps: []v1beta1.Step{simpleStep},
+			Results: []v1beta1.TaskResult{{
+				Name: "aResult",
+				Type: v1beta1.ResultsTypeArray,
+			}, {
+				Name:       "objectResult",
+				Type:       v1beta1.ResultsTypeObject,
+				Properties: map[string]v1beta1.PropertySpec{"url": {Type: "string"}, "commit": {Type: "string"}},
+			},
+			},
+		},
+	}
 
 	clustertask = &v1beta1.ClusterTask{
 		ObjectMeta: metav1.ObjectMeta{Name: "test-cluster-task"},
@@ -3464,6 +3479,206 @@ spec:
 	}
 }
 
+func TestReconcileValidateTaskRunResults(t *testing.T) {
+	for _, tt := range []struct {
+		desc             string
+		d                test.Data
+		wantFailedReason string
+		wantEvents       []string
+	}{{
+		desc: "wrong result type",
+		d: test.Data{
+			ConfigMaps: []*corev1.ConfigMap{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: system.Namespace(), Name: config.GetFeatureFlagsConfigName()},
+				Data: map[string]string{
+					"enable-api-fields": config.AlphaAPIFields,
+				},
+			}},
+			Tasks: []*v1beta1.Task{parse.MustParseTask(t, `
+metadata:
+  name: test-task-missing-resource
+  namespace: foo
+spec:
+  results:
+   - name: string-results
+     type: string
+  steps:
+  - name: foo
+    image: bash:latest
+    script: |
+     #!/usr/bin/env bash
+     echo -n "hello" | tee $(results.string-results.path)
+    resources:
+      limits:
+        cpu: "8"
+        memory: 8Gi
+      requests:
+        cpu: "8"
+        memory: 4Gi
+
+`)},
+			TaskRuns: []*v1beta1.TaskRun{parse.MustParseTaskRun(t, `
+metadata:
+  name: test-taskrun-missing-resource
+  namespace: foo
+spec:
+  taskRef:
+    apiVersion: a1
+    name: test-task-missing-resource
+`)},
+			ClusterTasks:      nil,
+			PipelineResources: nil,
+		},
+		wantFailedReason: podconvert.ReasonFailedValidation,
+		wantEvents: []string{
+			"Normal Started ",
+			"Warning Failed",        // Event about the TaskRun state changed
+			"Warning InternalError", // Event about the error (generated by the genreconciler)
+		},
+	}, {
+		desc: "Fail ValidateResolvedTaskResources",
+		d: test.Data{
+			Tasks: []*v1beta1.Task{parse.MustParseTask(t, `
+metadata:
+  name: test-task-missing-resource
+  namespace: foo
+spec:
+  resources:
+    inputs:
+    - name: workspace
+      type: git
+`)},
+			TaskRuns: []*v1beta1.TaskRun{parse.MustParseTaskRun(t, `
+metadata:
+  name: test-taskrun-missing-resource
+  namespace: foo
+spec:
+  taskRef:
+    apiVersion: a1
+    name: test-task-missing-resource
+`)},
+			ClusterTasks:      nil,
+			PipelineResources: nil,
+		},
+		wantFailedReason: podconvert.ReasonFailedValidation,
+		wantEvents: []string{
+			"Normal Started ",
+			"Warning Failed",        // Event about the TaskRun state changed
+			"Warning InternalError", // Event about the error (generated by the genreconciler)
+		},
+	}, {
+		desc: "Fail ValidateTaskSpecRequestResources",
+		d: test.Data{
+			Tasks: []*v1beta1.Task{parse.MustParseTask(t, `
+metadata:
+  name: test-task-invalid-taskspec-resource
+  namespace: foo
+spec:
+  steps:
+  - command:
+    - cmd
+    image: image
+    resources:
+      limits:
+        cpu: "8"
+        memory: 4Gi
+      requests:
+        cpu: "8"
+        memory: 8Gi
+  workspaces:
+  - description: a test task workspace
+    name: ws1
+    readOnly: true
+`)},
+			TaskRuns: []*v1beta1.TaskRun{parse.MustParseTaskRun(t, `
+metadata:
+  name: test-taskrun-invalid-taskspec-resource
+  namespace: foo
+spec:
+  taskRef:
+    apiVersion: a1
+    name: test-task-invalid-taskspec-resource
+`)},
+			ClusterTasks:      nil,
+			PipelineResources: nil,
+		},
+		wantFailedReason: podconvert.ReasonFailedValidation,
+		wantEvents: []string{
+			"Normal Started ",
+			"Warning Failed",        // Event about the TaskRun state changed
+			"Warning InternalError", // Event about the error (generated by the genreconciler)
+		},
+	}} {
+		t.Run(tt.desc, func(t *testing.T) {
+			testAssets, cancel := getTaskRunController(t, tt.d)
+			defer cancel()
+			clients := testAssets.Clients
+			c := testAssets.Controller
+
+			tt.d.TaskRuns[0].Status = v1beta1.TaskRunStatus{
+				TaskRunStatusFields: v1beta1.TaskRunStatusFields{
+					TaskRunResults: []v1beta1.TaskRunResult{{
+						Name:  "taskresult1",
+						Type:  v1beta1.ResultsTypeArray,
+						Value: *v1beta1.NewArrayOrString("foo", "bar"),
+					},
+					},
+				},
+			}
+			tr1, err := clients.Pipeline.TektonV1beta1().TaskRuns(tt.d.TaskRuns[0].Namespace).Update(testAssets.Ctx, tt.d.TaskRuns[0], metav1.UpdateOptions{FieldValidation: "Ignore"})
+			if err != nil {
+				t.Fatalf("wrong err %v", err)
+			}
+			fmt.Println(tr1)
+
+			reconcileErr := c.Reconciler.Reconcile(testAssets.Ctx, getRunName(tt.d.TaskRuns[0]))
+
+			// When a TaskRun is invalid and can't run, we return a permanent error because
+			// a regular error will tell the Reconciler to keep trying to reconcile; instead we want to stop
+			// and forget about the Run.
+			if reconcileErr == nil {
+				t.Fatalf("Expected to see error when reconciling invalid TaskRun but none")
+			}
+			if !controller.IsPermanentError(reconcileErr) {
+				t.Fatalf("Expected to see a permanent error when reconciling invalid TaskRun, got %s instead", reconcileErr)
+			}
+			/*
+				tt.d.TaskRuns[0].Status = v1beta1.TaskRunStatus{
+					TaskRunStatusFields: v1beta1.TaskRunStatusFields{
+						TaskRunResults: []v1beta1.TaskRunResult{{
+							Name: "taskresult1",
+							Type: v1beta1.ResultsTypeArray,
+							Value: *v1beta1.NewArrayOrString("foo", "bar"),
+						},
+						},
+					},
+				}
+				tr1, err := clients.Pipeline.TektonV1beta1().TaskRuns(tt.d.TaskRuns[0].Namespace).Update(testAssets.Ctx, tt.d.TaskRuns[0],metav1.UpdateOptions{FieldValidation: "Ignore"})
+				if err!=nil{
+					t.Fatalf("wrong err %v", err)
+				}
+				fmt.Println(tr1)
+			*/
+
+			tr, err := clients.Pipeline.TektonV1beta1().TaskRuns(tt.d.TaskRuns[0].Namespace).Get(testAssets.Ctx, tt.d.TaskRuns[0].Name, metav1.GetOptions{})
+			if err != nil {
+				t.Fatalf("Expected TaskRun %s to exist but instead got error when getting it: %v", tt.d.TaskRuns[0].Name, err)
+			}
+
+			for _, c := range tr.Status.Conditions {
+				if c.Type != apis.ConditionSucceeded || c.Status != corev1.ConditionFalse || c.Reason != tt.wantFailedReason {
+					t.Errorf("Expected TaskRun to \"%s\" but it did not. Final conditions were:\n%#v", tt.wantFailedReason, tr.Status.Conditions)
+				}
+			}
+
+			err = eventstest.CheckEventsOrdered(t, testAssets.Recorder.Events, tt.desc, tt.wantEvents)
+			if !(err == nil) {
+				t.Errorf(err.Error())
+			}
+		})
+	}
+}
+
 // TestReconcileWithWorkspacesIncompatibleWithAffinityAssistant tests that a TaskRun used with an associated
 // Affinity Assistant is validated and that the validation fails for a TaskRun that is incompatible with
 // Affinity Assistant; e.g. using more than one PVC-backed workspace.
@@ -4657,3 +4872,96 @@ func objectMeta(name, ns string) metav1.ObjectMeta {
 		Annotations: map[string]string{},
 	}
 }
+
+func TestReconcilevalidateTaskRunResults(t *testing.T) {
+	taskRunResultsTypeMismatched := parse.MustParseTaskRun(t, `
+metadata:
+  name: test-taskrun-results-type-mismatched
+  namespace: foo
+spec:
+  taskRef:
+    name: test-results-task
+status:
+  taskResults:
+    - name: aResult
+      type: string
+      value: aResultValue
+`)
+
+	taskRunResultsObjectMissKey := parse.MustParseTaskRun(t, `
+metadata:
+  name: test-taskrun-results-object-miss-key
+  namespace: foo
+spec:
+  taskRef:
+    name: test-results-task
+status:
+  taskResults:
+    - name: aResult
+      type: array
+      value:
+       - 1
+       - 2
+    - name: objectResult
+      type: object
+      value:
+        url: abc
+`)
+
+	taskruns := []*v1beta1.TaskRun{
+		taskRunResultsTypeMismatched, taskRunResultsObjectMissKey,
+	}
+
+	d := test.Data{
+		TaskRuns: taskruns,
+		Tasks:    []*v1beta1.Task{resultsTask},
+		ConfigMaps: []*corev1.ConfigMap{{
+			ObjectMeta: metav1.ObjectMeta{Namespace: system.Namespace(), Name: config.GetFeatureFlagsConfigName()},
+			Data: map[string]string{
+				"enable-api-fields": config.AlphaAPIFields,
+			},
+		}},
+	}
+	for _, tc := range []struct {
+		name             string
+		taskRun          *v1beta1.TaskRun
+		wantFailedReason string
+		expectedError    error
+	}{{
+		name:             "taskrun results type mismatched",
+		taskRun:          taskRunResultsTypeMismatched,
+		wantFailedReason: podconvert.ReasonFailedValidation,
+		expectedError:    fmt.Errorf("1 error occurred:\n\t* missmatched Types for these results, map[aResult:[array]]\n\n"),
+	}, {
+		name:             "taskrun results object miss key",
+		taskRun:          taskRunResultsObjectMissKey,
+		wantFailedReason: podconvert.ReasonFailedValidation,
+		expectedError:    fmt.Errorf("1 error occurred:\n\t* missing keys for these results which are required in TaskResult's properties map[objectResult:[commit]]\n\n"),
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			testAssets, cancel := getTaskRunController(t, d)
+			defer cancel()
+			c := testAssets.Controller
+			clients := testAssets.Clients
+			saName := tc.taskRun.Spec.ServiceAccountName
+			if saName == "" {
+				saName = "default"
+			}
+			createServiceAccount(t, testAssets, saName, tc.taskRun.Namespace)
+
+			err := c.Reconciler.Reconcile(testAssets.Ctx, getRunName(tc.taskRun))
+			if d := cmp.Diff(err.Error(), tc.expectedError.Error()); d != "" {
+				t.Errorf("Expected: %v, but Got: %v", tc.expectedError, err)
+			}
+			tr, err := clients.Pipeline.TektonV1beta1().TaskRuns(tc.taskRun.Namespace).Get(testAssets.Ctx, tc.taskRun.Name, metav1.GetOptions{})
+			if err != nil {
+				t.Fatalf("getting updated taskrun: %v", err)
+			}
+			condition := tr.Status.GetCondition(apis.ConditionSucceeded)
+			if condition.Type != apis.ConditionSucceeded || condition.Status != corev1.ConditionFalse || condition.Reason != tc.wantFailedReason {
+				t.Errorf("Expected TaskRun to \"%s\" but it did not. Final conditions were:\n%#v", tc.wantFailedReason, tr.Status.Conditions)
+			}
+
+		})
+	}
+}

pkg/reconciler/taskrun/validate_resources.go

@@ -303,7 +303,6 @@ func validateTaskRunResults(tr *v1beta1.TaskRun, resolvedTaskSpec *v1beta1.TaskS
 	if resolvedTaskSpec != nil {
 		specResults = append(specResults, resolvedTaskSpec.Results...)
 	}
-
 	// When get the results, check if the type of result is the expected one
 	if missmatchedTypes := mismatchedTypesResults(tr, specResults); len(missmatchedTypes) != 0 {
 		return fmt.Errorf("missmatched Types for these results, %v", missmatchedTypes)