TEP-0075: Validate object keys, PipelineRunSpec -> PipelineSpec

Add validation that checks if object param value from PipelineRunSpec
misses some keys required for that object param declared in PipelineSpec.

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -81,6 +81,9 @@ const (
 	// parameter(s) declared in the PipelineRun do not have the some declared type as the
 	// parameters(s) declared in the Pipeline that they are supposed to override.
 	ReasonParameterTypeMismatch = "ParameterTypeMismatch"
+	// ReasonObjectParameterMissKeys indicates that the object param value provided from PipelineRun spec
+	// misses some keys required for the object param declared in Pipeline spec.
+	ReasonObjectParameterMissKeys = "ObjectParameterMissKeys"
 	// ReasonCouldntGetTask indicates that the reason for the failure status is that the
 	// associated Pipeline's Tasks couldn't all be retrieved
 	ReasonCouldntGetTask = "CouldntGetTask"
@@ -429,15 +432,23 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
 
 	// Ensure that the parameters from the PipelineRun are overriding Pipeline parameters with the same type.
 	// Weird substitution issues can occur if this is not validated (ApplyParameters() does not verify type).
-	err = resources.ValidateParamTypesMatching(pipelineSpec, pr)
-	if err != nil {
+	if err = resources.ValidateParamTypesMatching(pipelineSpec, pr); err != nil {
 		// This Run has failed, so we need to mark it as failed and stop reconciling it
 		pr.Status.MarkFailed(ReasonParameterTypeMismatch,
 			"PipelineRun %s/%s parameters have mismatching types with Pipeline %s/%s's parameters: %s",
 			pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err)
 		return controller.NewPermanentError(err)
 	}
 
+	// Ensure that the keys of an object param declared in PipelineSpec are not missed in the PipelineRunSpec
+	if err = resources.ValidateObjectParamRequiredKeys(pipelineSpec.Params, pr.Spec.Params); err != nil {
+		// This Run has failed, so we need to mark it as failed and stop reconciling it
+		pr.Status.MarkFailed(ReasonObjectParameterMissKeys,
+			"PipelineRun %s/%s parameters is missing object keys required by Pipeline %s/%s's parameters: %s",
+			pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err)
+		return controller.NewPermanentError(err)
+	}
+
 	// Ensure that the workspaces expected by the Pipeline are provided by the PipelineRun.
 	if err := resources.ValidateWorkspaceBindings(pipelineSpec, pr); err != nil {
 		pr.Status.MarkFailed(ReasonInvalidWorkspaceBinding,

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -914,6 +914,18 @@ spec:
       type: %s
 `, v1beta1.ParamTypeArray)),
 		parse.MustParseTask(t, fmt.Sprintf(`
+metadata:
+  name: a-task-that-needs-object-params
+  namespace: foo
+spec:
+  params:
+    - name: some-param
+      type: %s
+      properties:
+        key1: {}
+        key2: {}
+`, v1beta1.ParamTypeObject)),
+		parse.MustParseTask(t, fmt.Sprintf(`
 metadata:
   name: a-task-that-needs-a-resource
   namespace: foo
@@ -989,6 +1001,22 @@ spec:
       taskRef:
         name: a-task-that-needs-array-params
 `, v1beta1.ParamTypeArray)),
+		parse.MustParsePipeline(t, fmt.Sprintf(`
+metadata:
+  name: a-pipeline-with-object-params
+  namespace: foo
+spec:
+  params:
+    - name: some-param
+      type: %s
+      properties:
+        key1: {type: string}
+        key2: {type: string}
+  tasks:
+    - name: some-task
+      taskRef:
+        name: a-task-that-needs-object-params
+`, v1beta1.ParamTypeObject)),
 	}
 
 	for _, tc := range []struct {
@@ -1118,6 +1146,26 @@ spec:
 			"Normal Started",
 			"Warning Failed PipelineRun foo/pipeline-mismatching-param-type parameters have mismatching types",
 		},
+	}, {
+		name: "invalid-pipeline-missing-object-keys",
+		pipelineRun: parse.MustParsePipelineRun(t, `
+metadata:
+  name: pipeline-missing-object-param-keys
+  namespace: foo
+spec:
+  pipelineRef:
+    name: a-pipeline-with-object-params
+  params:
+    - name: some-param
+      value:
+        key1: "a"
+`),
+		reason:         ReasonObjectParameterMissKeys,
+		permanentError: true,
+		wantEvents: []string{
+			"Normal Started",
+			fmt.Sprintf("Warning Failed PipelineRun foo/pipeline-missing-object-param-keys parameters is missing object keys required by Pipeline foo/a-pipeline-with-object-params's parameters: PipelineRun missing object keys for parameters: %v", map[string][]string{"some-param": {"key2"}}),
+		},
 	}, {
 		name: "invalid-embedded-pipeline-resources-bot-bound-shd-stop-reconciling",
 		pipelineRun: parse.MustParsePipelineRun(t, fmt.Sprintf(`
@@ -1254,10 +1302,13 @@ spec:
 		},
 	}} {
 		t.Run(tc.name, func(t *testing.T) {
+			cms := []*corev1.ConfigMap{withEnabledAlphaAPIFields(newFeatureFlagsConfigMap())}
+
 			d := test.Data{
 				PipelineRuns: []*v1beta1.PipelineRun{tc.pipelineRun},
 				Pipelines:    ps,
 				Tasks:        ts,
+				ConfigMaps:   cms,
 			}
 			prt := newPipelineRunTest(d, t)
 			defer prt.Cancel()
@@ -7326,7 +7377,7 @@ spec:
   - pipelineTaskName: hello-world-1
     metadata:
       labels:
-        PipelineTaskRunSpecLabel: PipelineTaskRunSpecValue 	
+        PipelineTaskRunSpecLabel: PipelineTaskRunSpecValue
       annotations:
         PipelineTaskRunSpecAnnotation: PipelineTaskRunSpecValue
     taskServiceAccountName: custom-sa
@@ -7377,7 +7428,7 @@ spec:
             image: foo-image
         metadata:
           labels:
-            TestPrecedenceLabel: PipelineTaskSpecValue 
+            TestPrecedenceLabel: PipelineTaskSpecValue
           annotations:
             TestPrecedenceAnnotation: PipelineTaskSpecValue
 `)}
@@ -7387,17 +7438,17 @@ metadata:
   namespace: foo
   metadata:
     labels:
-      TestPrecedenceLabel: PipelineRunValue 
+      TestPrecedenceLabel: PipelineRunValue
     annotations:
-      TestPrecedenceAnnotation: PipelineRunValue 
+      TestPrecedenceAnnotation: PipelineRunValue
 spec:
   pipelineRef:
     name: test-pipeline
   taskRunSpecs:
   - pipelineTaskName: hello-world-1
     metadata:
       labels:
-        TestPrecedenceLabel: PipelineTaskRunSpecValue 
+        TestPrecedenceLabel: PipelineTaskRunSpecValue
       annotations:
         TestPrecedenceAnnotation: PipelineTaskRunSpecValue
     taskServiceAccountName: custom-sa

pkg/reconciler/pipelinerun/resources/validate_params.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/list"
+	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun"
 )
 
 // ValidateParamTypesMatching validate that parameters in PipelineRun override corresponding parameters in Pipeline of the same type.
@@ -73,3 +74,13 @@ func ValidateRequiredParametersProvided(pipelineParameters *[]v1beta1.ParamSpec,
 	}
 	return nil
 }
+
+// ValidateObjectParamRequiredKeys validates that the required keys of all the object parameters expected by the Pipeline are provided by the PipelineRun.
+func ValidateObjectParamRequiredKeys(pipelineParameters []v1beta1.ParamSpec, pipelineRunParameters []v1beta1.Param) error {
+	missings := taskrun.MissingKeysObjectParamNames(pipelineParameters, pipelineRunParameters)
+	if len(missings) != 0 {
+		return fmt.Errorf("PipelineRun missing object keys for parameters: %v", missings)
+	}
+
+	return nil
+}

pkg/reconciler/pipelinerun/resources/validate_params_test.go

@@ -188,3 +188,110 @@ func TestValidateRequiredParametersProvided_Invalid(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateObjectParamRequiredKeys_Invalid(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		pp   []v1beta1.ParamSpec
+		prp  []v1beta1.Param
+	}{{
+		name: "miss all required keys",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name: "an-object-param",
+				Type: v1beta1.ParamTypeObject,
+				Properties: map[string]v1beta1.PropertySpec{
+					"key1": {Type: "string"},
+					"key2": {Type: "string"},
+				},
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name: "an-object-param",
+				Value: *v1beta1.NewObject(map[string]string{
+					"foo": "val1",
+				})},
+		},
+	}, {
+		name: "miss one of the required keys",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name: "an-object-param",
+				Type: v1beta1.ParamTypeObject,
+				Properties: map[string]v1beta1.PropertySpec{
+					"key1": {Type: "string"},
+					"key2": {Type: "string"},
+				},
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name: "an-object-param",
+				Value: *v1beta1.NewObject(map[string]string{
+					"key1": "foo",
+				})},
+		},
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := ValidateObjectParamRequiredKeys(tc.pp, tc.prp); err == nil {
+				t.Errorf("Expected to see error when validating invalid object parameter keys but saw none")
+			}
+		})
+	}
+}
+
+func TestValidateObjectParamRequiredKeys_Valid(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		pp   []v1beta1.ParamSpec
+		prp  []v1beta1.Param
+	}{{
+		name: "all keys are provided with a value",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name: "an-object-param",
+				Type: v1beta1.ParamTypeObject,
+				Properties: map[string]v1beta1.PropertySpec{
+					"key1": {Type: "string"},
+					"key2": {Type: "string"},
+				},
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name: "an-object-param",
+				Value: *v1beta1.NewObject(map[string]string{
+					"key1": "val1",
+					"key2": "val2",
+				})},
+		},
+	}, {
+		name: "extra keys are provided",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name: "an-object-param",
+				Type: v1beta1.ParamTypeObject,
+				Properties: map[string]v1beta1.PropertySpec{
+					"key1": {Type: "string"},
+					"key2": {Type: "string"},
+				},
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name: "an-object-param",
+				Value: *v1beta1.NewObject(map[string]string{
+					"key1": "val1",
+					"key2": "val2",
+					"key3": "val3",
+				})},
+		},
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := ValidateObjectParamRequiredKeys(tc.pp, tc.prp); err != nil {
+				t.Errorf("Didn't expect to see error when validating invalid object parameter keys but got: %v", err)
+			}
+		})
+	}
+}

pkg/reconciler/taskrun/validate_resources.go

@@ -81,7 +81,7 @@ func validateParams(ctx context.Context, paramSpecs []v1beta1.ParamSpec, params
 	if wrongTypeParamNames := wrongTypeParamsNames(params, matrix, neededParamsTypes); len(wrongTypeParamNames) != 0 {
 		return fmt.Errorf("param types don't match the user-specified type: %s", wrongTypeParamNames)
 	}
-	if missingKeysObjectParamNames := missingKeysObjectParamNames(paramSpecs, params); len(missingKeysObjectParamNames) != 0 {
+	if missingKeysObjectParamNames := MissingKeysObjectParamNames(paramSpecs, params); len(missingKeysObjectParamNames) != 0 {
 		return fmt.Errorf("missing keys for these params which are required in ParamSpec's properties %v", missingKeysObjectParamNames)
 	}
 
@@ -157,9 +157,8 @@ func wrongTypeParamsNames(params []v1beta1.Param, matrix []v1beta1.Param, needed
 	return wrongTypeParamNames
 }
 
-// missingKeysObjectParamNames checks if all required keys of object type params are provided in taskrun params.
-// TODO (@chuangw6): This might be refactored out to support single pair validation.
-func missingKeysObjectParamNames(paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) []string {
+// MissingKeysObjectParamNames checks if all required keys of object type params are provided in taskrun params.
+func MissingKeysObjectParamNames(paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) map[string][]string {
 	neededKeys := make(map[string][]string)
 	providedKeys := make(map[string][]string)
 
@@ -185,17 +184,16 @@ func missingKeysObjectParamNames(paramSpecs []v1beta1.ParamSpec, params []v1beta
 }
 
 // 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{}
+func validateObjectKeys(neededObjectKeys, providedObjectKeys map[string][]string) map[string][]string {
+	missings := map[string][]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.
+			// Ignore any missing objects - this happens when object param is provided with default
 			continue
 		}
 		missedKeys := list.DiffLeft(neededObjectKeys[p], keys)
 		if len(missedKeys) != 0 {
-			missings = append(missings, p)
+			missings[p] = missedKeys
 		}
 	}