[TEP-0076]Validate Results in reconciler

This is part of work in TEP-0076.
This commit provides the validation of results in reconciler after we
get the emitted results. It covers the validation of mismatched types
between the results emitted and specified, the results object
properties.
Previous this commit these validations are missing and errors are
ignored.

pkg/apis/pipeline/v1beta1/result_types.go

@@ -23,6 +23,10 @@ type TaskResult struct {
 	// +optional
 	Type ResultsType `json:"type,omitempty"`
 
+	// Properties is the JSON Schema properties to support key-value pairs results.
+	// +optional
+	Properties map[string]PropertySpec `json:"properties,omitempty"`
+
 	// Description is a human-readable description of the result
 	// +optional
 	Description string `json:"description"`

pkg/apis/pipeline/v1beta1/swagger.json

@@ -2452,6 +2452,14 @@
           "type": "string",
           "default": ""
         },
+        "properties": {
+          "description": "Properties is the JSON Schema properties to support key-value pairs results.",
+          "type": "object",
+          "additionalProperties": {
+            "default": {},
+            "$ref": "#/definitions/v1beta1.PropertySpec"
+          }
+        },
         "type": {
           "description": "Type is the user-specified type of the result. The possible type is currently \"string\" and will support \"array\" in following work.",
           "type": "string"

pkg/pod/status.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/hashicorp/go-multierror"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+	"github.com/tektoncd/pipeline/pkg/list"
 	"github.com/tektoncd/pipeline/pkg/termination"
 	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
@@ -100,7 +101,7 @@ func SidecarsReady(podStatus corev1.PodStatus) bool {
 }
 
 // MakeTaskRunStatus returns a TaskRunStatus based on the Pod's status.
-func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev1.Pod) (v1beta1.TaskRunStatus, error) {
+func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev1.Pod, resolvedTaskSpec *v1beta1.TaskSpec) (v1beta1.TaskRunStatus, error) {
 	trs := &tr.Status
 	if trs.GetCondition(apis.ConditionSucceeded) == nil || trs.GetCondition(apis.ConditionSucceeded).Status == corev1.ConditionUnknown {
 		// If the taskRunStatus doesn't exist yet, it's because we just started running
@@ -140,9 +141,95 @@ func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev
 
 	trs.TaskRunResults = removeDuplicateResults(trs.TaskRunResults)
 
+	if resolvedTaskSpec != nil {
+		// When get the results, check if the type of result is expected
+		if missmatchedTypes := missmatchedTypesResults(tr, trs, resolvedTaskSpec); len(missmatchedTypes) != 0 {
+			err := fmt.Errorf("missmatched Types for these results, array and object are alpha feature %v", missmatchedTypes)
+			merr = multierror.Append(merr, err)
+		}
+
+		// When get the results, for object value need to check if they have missing keys.
+		if missingKeysObjectNames := missingKeyObjectResults(tr, trs, resolvedTaskSpec); len(missingKeysObjectNames) != 0 {
+			err := fmt.Errorf("missing keys for these results which are required in TaskResult's properties %v", missingKeysObjectNames)
+			merr = multierror.Append(merr, err)
+		}
+	}
+
 	return *trs, merr.ErrorOrNil()
 }
 
+func missmatchedTypesResults(tr v1beta1.TaskRun, trs *v1beta1.TaskRunStatus, resolvedTaskSpec *v1beta1.TaskSpec) []string {
+	neededTypes := make(map[string][]string)
+	providedTypes := make(map[string][]string)
+	// collect needed keys for object results
+	if tr.Spec.TaskSpec != nil {
+		for _, tr := range tr.Spec.TaskSpec.Results {
+			neededTypes[tr.Name] = append(neededTypes[tr.Name], string(tr.Type))
+		}
+	}
+
+	for _, rtr := range resolvedTaskSpec.Results {
+		neededTypes[rtr.Name] = append(neededTypes[rtr.Name], string(rtr.Type))
+	}
+
+	// collect provided keys for object results
+	for _, trr := range trs.TaskRunResults {
+		providedTypes[trr.Name] = append(providedTypes[trr.Name], string(trr.Value.Type))
+	}
+	return validateObjectKeys(neededTypes, providedTypes)
+}
+
+func missingKeyObjectResults(tr v1beta1.TaskRun, trs *v1beta1.TaskRunStatus, resolvedTaskSpec *v1beta1.TaskSpec) []string {
+	neededKeys := make(map[string][]string)
+	providedKeys := make(map[string][]string)
+	// collect needed keys for object results
+	if tr.Spec.TaskSpec != nil {
+		for _, tr := range tr.Spec.TaskSpec.Results {
+			if string(tr.Type) == string(v1beta1.ParamTypeObject) {
+				for key := range tr.Properties {
+					neededKeys[tr.Name] = append(neededKeys[tr.Name], key)
+				}
+			}
+		}
+	}
+
+	for _, rtr := range resolvedTaskSpec.Results {
+		if string(rtr.Type) == string(v1beta1.ParamTypeObject) {
+			for key := range rtr.Properties {
+				neededKeys[rtr.Name] = append(neededKeys[rtr.Name], key)
+			}
+		}
+	}
+
+	// collect provided keys for object results
+	for _, trr := range trs.TaskRunResults {
+		if trr.Value.Type == v1beta1.ParamTypeObject {
+			for key := range trr.Value.ObjectVal {
+				providedKeys[trr.Name] = append(providedKeys[trr.Name], key)
+			}
+		}
+	}
+	return validateObjectKeys(neededKeys, providedKeys)
+}
+
+// validateObjectKeys checks if objects have missing keys in its provider (either taskrun value or result value)
+func validateObjectKeys(neededObjectKeys, providedObjectKeys map[string][]string) []string {
+	missings := []string{}
+	for p, keys := range providedObjectKeys {
+		if _, ok := neededObjectKeys[p]; !ok {
+			// Ignore any missing objects - this happens when extra objects were
+			// passed that aren't being used.
+			continue
+		}
+		missedKeys := list.DiffLeft(neededObjectKeys[p], keys)
+		if len(missedKeys) != 0 {
+			missings = append(missings, p)
+		}
+	}
+
+	return missings
+}
+
 func setTaskRunStatusBasedOnStepStatus(logger *zap.SugaredLogger, stepStatuses []corev1.ContainerStatus, tr *v1beta1.TaskRun) *multierror.Error {
 	trs := &tr.Status
 	var merr *multierror.Error

pkg/pod/status_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package pod
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 	"time"
@@ -1061,7 +1062,7 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				},
 			}
 			logger, _ := logging.NewLogger("", "status")
-			got, err := MakeTaskRunStatus(logger, tr, &c.pod)
+			got, err := MakeTaskRunStatus(logger, tr, &c.pod, nil)
 			if err != nil {
 				t.Errorf("MakeTaskRunResult: %s", err)
 			}
@@ -1088,10 +1089,12 @@ func TestMakeTaskRunStatus(t *testing.T) {
 
 func TestMakeTaskRunStatusAlpha(t *testing.T) {
 	for _, c := range []struct {
-		desc      string
-		podStatus corev1.PodStatus
-		pod       corev1.Pod
-		want      v1beta1.TaskRunStatus
+		desc             string
+		podStatus        corev1.PodStatus
+		pod              corev1.Pod
+		resolvedTaskSpec *v1beta1.TaskSpec
+		want             v1beta1.TaskRunStatus
+		expectedError    error
 	}{{
 		desc: "test empty result",
 		podStatus: corev1.PodStatus{
@@ -1248,6 +1251,51 @@ func TestMakeTaskRunStatusAlpha(t *testing.T) {
 				CompletionTime: &metav1.Time{Time: time.Now()},
 			},
 		},
+	}, {
+		desc: "test result types mismatch",
+		podStatus: corev1.PodStatus{
+			Phase: corev1.PodSucceeded,
+			ContainerStatuses: []corev1.ContainerStatus{{
+				Name: "step-bar",
+				State: corev1.ContainerState{
+					Terminated: &corev1.ContainerStateTerminated{
+						Message: `[{"key":"resultName","value":"{\"hello\":\"world\"}", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`,
+					},
+				},
+			}},
+		},
+		resolvedTaskSpec: &v1beta1.TaskSpec{
+			Results: []v1beta1.TaskResult{
+				{Name: "resultName",
+					Type: v1beta1.ResultsTypeString,
+				},
+			},
+		},
+		expectedError: fmt.Errorf("1 error occurred:\n\t* missmatched Types for these results, array and object are alpha feature %v\n\n", []string{"resultName"}),
+	}, {
+		desc: "test object keys invalid",
+		podStatus: corev1.PodStatus{
+			Phase: corev1.PodSucceeded,
+			ContainerStatuses: []corev1.ContainerStatus{{
+				Name: "step-bar",
+				State: corev1.ContainerState{
+					Terminated: &corev1.ContainerStateTerminated{
+						Message: `[{"key":"resultName","value":"{\"hello\":\"world\"}", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`,
+					},
+				},
+			}},
+		},
+		resolvedTaskSpec: &v1beta1.TaskSpec{
+			Results: []v1beta1.TaskResult{
+				{Name: "resultName",
+					Type: v1beta1.ResultsTypeObject,
+					Properties: map[string]v1beta1.PropertySpec{
+						"foo": {Type: v1beta1.ParamTypeString},
+					},
+				},
+			},
+		},
+		expectedError: fmt.Errorf("1 error occurred:\n\t* missing keys for these results which are required in TaskResult's properties %v\n\n", []string{"resultName"}),
 	}} {
 		t.Run(c.desc, func(t *testing.T) {
 			now := metav1.Now()
@@ -1268,14 +1316,34 @@ func TestMakeTaskRunStatusAlpha(t *testing.T) {
 					Name:      "task-run",
 					Namespace: "foo",
 				},
+				Spec: v1beta1.TaskRunSpec{
+					TaskSpec: &v1beta1.TaskSpec{
+						Results: []v1beta1.TaskResult{
+							{
+								Name: "result",
+								Type: v1beta1.ResultsTypeString,
+								Properties: map[string]v1beta1.PropertySpec{
+									"foo": {Type: v1beta1.ParamTypeString},
+								},
+							},
+						},
+					},
+				},
 				Status: v1beta1.TaskRunStatus{
 					TaskRunStatusFields: v1beta1.TaskRunStatusFields{
 						StartTime: &metav1.Time{Time: startTime},
 					},
 				},
 			}
 			logger, _ := logging.NewLogger("", "status")
-			got, err := MakeTaskRunStatus(logger, tr, &c.pod)
+			got, err := MakeTaskRunStatus(logger, tr, &c.pod, c.resolvedTaskSpec)
+			if c.expectedError != nil {
+				if err.Error() != c.expectedError.Error() {
+					t.Errorf("Expected Error: %s but got %s", c.expectedError.Error(), err.Error())
+				}
+				return
+			}
+
 			if err != nil {
 				t.Errorf("MakeTaskRunResult: %s", err)
 			}
@@ -1396,7 +1464,7 @@ func TestMakeRunStatusJSONError(t *testing.T) {
 	}
 
 	logger, _ := logging.NewLogger("", "status")
-	gotTr, err := MakeTaskRunStatus(logger, tr, pod)
+	gotTr, err := MakeTaskRunStatus(logger, tr, pod, nil)
 	if err == nil {
 		t.Error("Expected error, got nil")
 	}

pkg/reconciler/taskrun/taskrun.go

@@ -463,7 +463,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, rtr *re
 	}
 
 	// Convert the Pod's status to the equivalent TaskRun Status.
-	tr.Status, err = podconvert.MakeTaskRunStatus(logger, *tr, pod)
+	tr.Status, err = podconvert.MakeTaskRunStatus(logger, *tr, pod, rtr.TaskSpec)
 	if err != nil {
 		return err
 	}