TEP-0090: Include Matrix in Validation of Parameters in TaskRun…

… Reconciler

The `TaskRun` reconciler validates that the needed `Parameters` are provided,
no extra `Parameters` are provide and the types of `Parameters` are matching.

Prior to this change, the reconciler did the above validation considering the
`params` field in the `PipelineTask` only. In this change, we include `matrix`
field into the validation routine described above. This includes validating
that `Parameters` in the `Matrix` are substituting `Parameters` of type `String`
in the underlying `Task`.

Related:
- #4704
- #4841

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -482,7 +482,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
 
 	for _, rprt := range pipelineRunFacts.State {
 		if !rprt.IsCustomTask() {
-			err := taskrun.ValidateResolvedTaskResources(ctx, rprt.PipelineTask.Params, rprt.ResolvedTaskResources)
+			err := taskrun.ValidateResolvedTaskResources(ctx, rprt.PipelineTask.Params, rprt.PipelineTask.Matrix, rprt.ResolvedTaskResources)
 			if err != nil {
 				logger.Errorf("Failed to validate pipelinerun %q with error %v", pr.Name, err)
 				pr.Status.MarkFailed(ReasonFailedValidation, err.Error())

pkg/reconciler/taskrun/taskrun.go

@@ -332,7 +332,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1
 		return nil, nil, controller.NewPermanentError(err)
 	}
 
-	if err := ValidateResolvedTaskResources(ctx, tr.Spec.Params, rtr); err != nil {
+	if err := ValidateResolvedTaskResources(ctx, tr.Spec.Params, []v1beta1.Param{}, rtr); err != nil {
 		logger.Errorf("TaskRun %q resources are invalid: %v", tr.Name, err)
 		tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
 		return nil, nil, controller.NewPermanentError(err)

pkg/reconciler/taskrun/validate_resources.go

@@ -69,16 +69,16 @@ func validateResources(requiredResources []v1beta1.TaskResource, providedResourc
 	return nil
 }
 
-func validateParams(ctx context.Context, paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) error {
+func validateParams(ctx context.Context, paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param, matrix []v1beta1.Param) error {
 	neededParamsNames, neededParamsTypes := neededParamsNamesAndTypes(paramSpecs)
-	providedParamsNames := providedParamsNames(params)
+	providedParamsNames := providedParamsNames(append(params, matrix...))
 	if missingParamsNames := missingParamsNames(neededParamsNames, providedParamsNames, paramSpecs); len(missingParamsNames) != 0 {
 		return fmt.Errorf("missing values for these params which have no default values: %s", missingParamsNames)
 	}
 	if extraParamsNames := extraParamsNames(ctx, neededParamsNames, providedParamsNames); len(extraParamsNames) != 0 {
 		return fmt.Errorf("didn't need these params but they were provided anyway: %s", extraParamsNames)
 	}
-	if wrongTypeParamNames := wrongTypeParamsNames(params, neededParamsTypes); len(wrongTypeParamNames) != 0 {
+	if wrongTypeParamNames := wrongTypeParamsNames(params, matrix, neededParamsTypes); len(wrongTypeParamNames) != 0 {
 		return fmt.Errorf("param types don't match the user-specified type: %s", wrongTypeParamNames)
 	}
 
@@ -129,7 +129,7 @@ func extraParamsNames(ctx context.Context, neededParams []string, providedParams
 	return nil
 }
 
-func wrongTypeParamsNames(params []v1beta1.Param, neededParamsTypes map[string]v1beta1.ParamType) []string {
+func wrongTypeParamsNames(params []v1beta1.Param, matrix []v1beta1.Param, neededParamsTypes map[string]v1beta1.ParamType) []string {
 	var wrongTypeParamNames []string
 	for _, param := range params {
 		if _, ok := neededParamsTypes[param.Name]; !ok {
@@ -141,12 +141,22 @@ func wrongTypeParamsNames(params []v1beta1.Param, neededParamsTypes map[string]v
 			wrongTypeParamNames = append(wrongTypeParamNames, param.Name)
 		}
 	}
+	for _, param := range matrix {
+		if _, ok := neededParamsTypes[param.Name]; !ok {
+			// Ignore any missing params - this happens when extra params were
+			// passed to the task that aren't being used.
+			continue
+		}
+		if neededParamsTypes[param.Name] != v1beta1.ParamTypeString {
+			wrongTypeParamNames = append(wrongTypeParamNames, param.Name)
+		}
+	}
 	return wrongTypeParamNames
 }
 
 // ValidateResolvedTaskResources validates task inputs, params and output matches taskrun
-func ValidateResolvedTaskResources(ctx context.Context, params []v1beta1.Param, rtr *resources.ResolvedTaskResources) error {
-	if err := validateParams(ctx, rtr.TaskSpec.Params, params); err != nil {
+func ValidateResolvedTaskResources(ctx context.Context, params []v1beta1.Param, matrix []v1beta1.Param, rtr *resources.ResolvedTaskResources) error {
+	if err := validateParams(ctx, rtr.TaskSpec.Params, params, matrix); err != nil {
 		return fmt.Errorf("invalid input params for task %s: %w", rtr.TaskName, err)
 	}
 	inputs := []v1beta1.TaskResource{}

pkg/reconciler/taskrun/validate_resources_test.go

@@ -115,7 +115,7 @@ func TestValidateResolvedTaskResources_ValidResources(t *testing.T) {
 			},
 		},
 	}
-	if err := ValidateResolvedTaskResources(ctx, []v1beta1.Param{}, rtr); err != nil {
+	if err := ValidateResolvedTaskResources(ctx, []v1beta1.Param{}, []v1beta1.Param{}, rtr); err != nil {
 		t.Fatalf("Did not expect to see error when validating valid resolved TaskRun but saw %v", err)
 	}
 }
@@ -138,6 +138,10 @@ func TestValidateResolvedTaskResources_ValidParams(t *testing.T) {
 					Name: "bar",
 					Type: v1beta1.ParamTypeString,
 				},
+				{
+					Name: "zoo",
+					Type: v1beta1.ParamTypeString,
+				},
 			},
 		},
 	}
@@ -151,7 +155,11 @@ func TestValidateResolvedTaskResources_ValidParams(t *testing.T) {
 		Name:  "bar",
 		Value: *v1beta1.NewArrayOrString("somethinggood"),
 	}}
-	if err := ValidateResolvedTaskResources(ctx, p, rtr); err != nil {
+	m := []v1beta1.Param{{
+		Name:  "zoo",
+		Value: *v1beta1.NewArrayOrString("a", "b", "c"),
+	}}
+	if err := ValidateResolvedTaskResources(ctx, p, m, rtr); err != nil {
 		t.Fatalf("Did not expect to see error when validating TaskRun with correct params but saw %v", err)
 	}
 
@@ -161,7 +169,11 @@ func TestValidateResolvedTaskResources_ValidParams(t *testing.T) {
 			Name:  "extra",
 			Value: *v1beta1.NewArrayOrString("i am an extra param"),
 		}
-		if err := ValidateResolvedTaskResources(ctx, append(p, extra), rtr); err != nil {
+		extraarray := v1beta1.Param{
+			Name:  "extraarray",
+			Value: *v1beta1.NewArrayOrString("i", "am", "an", "extra", "array", "param"),
+		}
+		if err := ValidateResolvedTaskResources(ctx, append(p, extra), append(m, extraarray), rtr); err != nil {
 			t.Fatalf("Did not expect to see error when validating TaskRun with correct params but saw %v", err)
 		}
 	})
@@ -180,6 +192,9 @@ func TestValidateResolvedTaskResources_InvalidParams(t *testing.T) {
 				{
 					Name: "foo",
 					Type: v1beta1.ParamTypeString,
+				}, {
+					Name: "bar",
+					Type: v1beta1.ParamTypeArray,
 				},
 			},
 		},
@@ -188,6 +203,7 @@ func TestValidateResolvedTaskResources_InvalidParams(t *testing.T) {
 		name   string
 		rtr    *resources.ResolvedTaskResources
 		params []v1beta1.Param
+		matrix []v1beta1.Param
 	}{{
 		name: "missing-params",
 		rtr: &resources.ResolvedTaskResources{
@@ -197,10 +213,32 @@ func TestValidateResolvedTaskResources_InvalidParams(t *testing.T) {
 			Name:  "foobar",
 			Value: *v1beta1.NewArrayOrString("somethingfun"),
 		}},
+		matrix: []v1beta1.Param{{
+			Name:  "barfoo",
+			Value: *v1beta1.NewArrayOrString("bar", "foo"),
+		}},
+	}, {
+		name: "invalid-type-in-params",
+		rtr: &resources.ResolvedTaskResources{
+			TaskSpec: &task.Spec,
+		},
+		params: []v1beta1.Param{{
+			Name:  "foo",
+			Value: *v1beta1.NewArrayOrString("bar", "foo"),
+		}},
+	}, {
+		name: "invalid-type-in-matrix",
+		rtr: &resources.ResolvedTaskResources{
+			TaskSpec: &task.Spec,
+		},
+		matrix: []v1beta1.Param{{
+			Name:  "bar",
+			Value: *v1beta1.NewArrayOrString("bar", "foo"),
+		}},
 	}}
 	for _, tc := range tcs {
 		t.Run(tc.name, func(t *testing.T) {
-			if err := ValidateResolvedTaskResources(ctx, tc.params, tc.rtr); err == nil {
+			if err := ValidateResolvedTaskResources(ctx, tc.params, tc.matrix, tc.rtr); err == nil {
 				t.Errorf("Expected to see error when validating invalid resolved TaskRun with wrong params but saw none")
 			}
 		})
@@ -408,7 +446,7 @@ func TestValidateResolvedTaskResources_InvalidResources(t *testing.T) {
 
 	for _, tc := range tcs {
 		t.Run(tc.name, func(t *testing.T) {
-			if err := ValidateResolvedTaskResources(ctx, []v1beta1.Param{}, tc.rtr); err == nil {
+			if err := ValidateResolvedTaskResources(ctx, []v1beta1.Param{}, []v1beta1.Param{}, tc.rtr); err == nil {
 				t.Errorf("Expected to see error when validating invalid resolved TaskRun but saw none")
 			}
 		})