TEP-0118: Add validation for matrix pipeline parameter variables incl…

…ude Matrix.Include.Params

[TEP-0090: Matrix] introduced `Matrix` to the `PipelineTask` specification such that the `PipelineTask` executes a list of `TaskRuns` or `Runs` in parallel with the specified list of inputs for a `Parameter` or with different combinations of the inputs for a set of `Parameters`.

To build on this, Tep-0018 introduced Matrix.Include, which allows passing in a specific combinations of `Parameters` into the `Matrix`.

This commit validates that matrix pipeline parameter variables  including include params that may contain the reference(s) to other params are used appropriately.

Note: This is still in preview mode. Other forms of validation and implementation logic will be added in subsequent commits.

pkg/apis/pipeline/v1/matrix_types.go

@@ -18,6 +18,7 @@ import (
 	"fmt"
 
 	"github.com/tektoncd/pipeline/pkg/apis/config"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"knative.dev/pkg/apis"
 )
 
@@ -147,3 +148,39 @@ func (m *Matrix) validateParamTypes() (errs *apis.FieldError) {
 	}
 	return errs
 }
+
+// validatePipelineParametersVariablesInMatrixParameters validates all pipeline paramater variables including Matrix.Params and Matrix.Include.Params
+// that may contain the reference(s) to other params to make sure those references are used appropriately.
+func (m *Matrix) validatePipelineParametersVariablesInMatrixParameters(prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
+	if m.hasInclude() {
+		for _, include := range m.Include {
+			for idx, param := range include.Params {
+				stringElement := param.Value.StringVal
+				errs = errs.Also(validateStringVariable(stringElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("", idx).ViaField("matrix.include.params", ""))
+			}
+		}
+	}
+	if m.hasParams() {
+		for _, param := range m.Params {
+			for idx, arrayElement := range param.Value.ArrayVal {
+				errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix.params", param.Name))
+			}
+		}
+	}
+	return errs
+}
+
+func (m *Matrix) validateParameterInOneOfMatrixOrParams(params []Param) (errs *apis.FieldError) {
+	matrixParameterNames := sets.NewString()
+	if m != nil {
+		for _, param := range m.Params {
+			matrixParameterNames.Insert(param.Name)
+		}
+	}
+	for _, param := range params {
+		if matrixParameterNames.Has(param.Name) {
+			errs = errs.Also(apis.ErrMultipleOneOf("matrix["+param.Name+"]", "params["+param.Name+"]"))
+		}
+	}
+	return errs
+}

pkg/apis/pipeline/v1/param_types.go

@@ -528,32 +528,6 @@ func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix
 	return errs
 }
 
-// validatePipelineParametersVariablesInMatrixParameters validates matrix param value
-// that may contain the reference(s) to other params to make sure those references are used appropriately.
-func validatePipelineParametersVariablesInMatrixParameters(matrix []Param, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
-	for _, param := range matrix {
-		for idx, arrayElement := range param.Value.ArrayVal {
-			errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix", param.Name))
-		}
-	}
-	return errs
-}
-
-func validateParameterInOneOfMatrixOrParams(matrix *Matrix, params []Param) (errs *apis.FieldError) {
-	matrixParameterNames := sets.NewString()
-	if matrix != nil {
-		for _, param := range matrix.Params {
-			matrixParameterNames.Insert(param.Name)
-		}
-	}
-	for _, param := range params {
-		if matrixParameterNames.Has(param.Name) {
-			errs = errs.Also(apis.ErrMultipleOneOf("matrix["+param.Name+"]", "params["+param.Name+"]"))
-		}
-	}
-	return errs
-}
-
 // validateParamStringValue validates the param value field of string type
 // that may contain references to other isolated array/object params other than string param.
 func validateParamStringValue(param Param, prefix string, paramNames sets.String, arrayVars sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {

pkg/apis/pipeline/v1/pipeline_types.go

@@ -274,7 +274,7 @@ func (pt *PipelineTask) validateMatrix(ctx context.Context) (errs *apis.FieldErr
 		errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "matrix", config.AlphaAPIFields))
 		errs = errs.Also(pt.Matrix.validateCombinationsCount(ctx))
 	}
-	errs = errs.Also(validateParameterInOneOfMatrixOrParams(pt.Matrix, pt.Params))
+	errs = errs.Also(pt.Matrix.validateParameterInOneOfMatrixOrParams(pt.Params))
 	errs = errs.Also(pt.Matrix.validateParamTypes())
 	return errs
 }

pkg/apis/pipeline/v1/pipeline_validation.go

@@ -162,7 +162,7 @@ func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, pa
 	for idx, task := range tasks {
 		errs = errs.Also(validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
 		if task.IsMatrixed() {
-			errs = errs.Also(validatePipelineParametersVariablesInMatrixParameters(task.Matrix.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
+			errs = errs.Also(task.Matrix.validatePipelineParametersVariablesInMatrixParameters(prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
 		}
 		errs = errs.Also(task.When.validatePipelineParametersVariables(prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
 	}

pkg/apis/pipeline/v1/pipeline_validation_test.go

@@ -1127,6 +1127,21 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
 				Name: "a-param-intended-to-be-array", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.myArray[*])"},
 			}},
 		}},
+	}, {
+		name: "valid string parameter variables in matrix include",
+		params: []ParamSpec{{
+			Name: "baz", Type: ParamTypeString,
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			Matrix: &Matrix{
+				Include: []MatrixInclude{{
+					Name: "build-1",
+					Params: []Param{{
+						Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.baz)"}},
+					}}}},
+		}},
 	}, {
 		name: "object param - using single individual variable in string param",
 		params: []ParamSpec{{
@@ -1550,7 +1565,7 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
 		}},
 		expectedError: apis.FieldError{
 			Message: `non-existent variable in "$(params.does-not-exist)"`,
-			Paths:   []string{"[0].matrix[a-param].value[0]"},
+			Paths:   []string{"[0].matrix.params[a-param].value[0]"},
 		},
 	}, {
 		name: "invalid pipeline task with a matrix parameter combined with missing param from the param declarations",
@@ -1567,7 +1582,7 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
 		}},
 		expectedError: apis.FieldError{
 			Message: `non-existent variable in "$(params.does-not-exist)"`,
-			Paths:   []string{"[0].matrix[a-param].value[2]"},
+			Paths:   []string{"[0].matrix.params[a-param].value[2]"},
 		},
 	}, {
 		name: "invalid pipeline task with two matrix parameters and one of them missing from the param declarations",
@@ -1581,12 +1596,32 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
 				Params: []Param{{
 					Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.foo)"}},
 				}, {
-					Name: "b-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.does-not-exist)"}},
+					Name: "b-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.does-not-exist)"}}}}},
+		}},
+		expectedError: apis.FieldError{
+			Message: `non-existent variable in "$(params.does-not-exist)"`,
+			Paths:   []string{"[0].matrix.params[b-param].value[0]"},
+		},
+	}, {
+		name: "invalid pipeline task with two matrix include parameters and one of them missing from the param declarations",
+		params: []ParamSpec{{
+			Name: "foo", Type: ParamTypeString,
+		}},
+		tasks: []PipelineTask{{
+			Name:    "foo-task",
+			TaskRef: &TaskRef{Name: "foo-task"},
+			Matrix: &Matrix{
+				Include: []MatrixInclude{{
+					Params: []Param{{
+						Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.foo)"},
+					}, {
+						Name: "b-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.does-not-exist)"},
+					}},
 				}}},
 		}},
 		expectedError: apis.FieldError{
 			Message: `non-existent variable in "$(params.does-not-exist)"`,
-			Paths:   []string{"[0].matrix[b-param].value[0]"},
+			Paths:   []string{"[0].matrix.include.params[1]"},
 		},
 	}, {
 		name: "invalid object key in the input of the when expression",
@@ -1730,7 +1765,7 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
 		}},
 		expectedError: apis.FieldError{
 			Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
-			Paths:   []string{"[0].matrix[b-param].value[0]"},
+			Paths:   []string{"[0].matrix.params[b-param].value[0]"},
 		},
 		api: "alpha",
 	}, {
@@ -2409,7 +2444,6 @@ func TestValidateFinalTasks_Failure(t *testing.T) {
 		})
 	}
 }
-
 func TestContextValid(t *testing.T) {
 	tests := []struct {
 		name  string
@@ -3063,6 +3097,40 @@ func Test_validateMatrix(t *testing.T) {
 					Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}},
 				}}},
 		}},
+	}, {
+		name: "parameters in include matrix are strings",
+		tasks: PipelineTaskList{{
+			Name:    "a-task",
+			TaskRef: &TaskRef{Name: "a-task"},
+			Matrix: &Matrix{
+				Include: []MatrixInclude{{
+					Name: "test",
+					Params: []Param{{
+						Name: "foobar", Value: ParamValue{Type: ParamTypeString, StringVal: "foo"},
+					}, {
+						Name: "barfoo", Value: ParamValue{Type: ParamTypeString, StringVal: "bar"}},
+					}}},
+			},
+		}},
+	}, {
+		name: "parameters in include matrix are arrays",
+		tasks: PipelineTaskList{{
+			Name:    "a-task",
+			TaskRef: &TaskRef{Name: "a-task"},
+			Matrix: &Matrix{
+				Include: []MatrixInclude{{
+					Name: "test",
+					Params: []Param{{
+						Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo"}},
+					}, {
+						Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar"}}},
+					}}},
+			},
+		}},
+		wantErrs: &apis.FieldError{
+			Message: "invalid value: parameters of type string only are allowed, but got param type array",
+			Paths:   []string{"[0].matrix.include.params[barfoo], [0].matrix.include.params[foobar]"},
+		},
 	}, {
 		name: "parameters in matrix contain results references",
 		tasks: PipelineTaskList{{

pkg/apis/pipeline/v1beta1/matrix_types.go

@@ -18,6 +18,7 @@ import (
 	"fmt"
 
 	"github.com/tektoncd/pipeline/pkg/apis/config"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"knative.dev/pkg/apis"
 )
 
@@ -147,3 +148,39 @@ func (m *Matrix) validateParamTypes() (errs *apis.FieldError) {
 	}
 	return errs
 }
+
+// validatePipelineParametersVariablesInMatrixParameters validates all pipeline paramater variables including Matrix.Params and Matrix.Include.Params
+// that may contain the reference(s) to other params to make sure those references are used appropriately.
+func (m *Matrix) validatePipelineParametersVariablesInMatrixParameters(prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
+	if m.hasInclude() {
+		for _, include := range m.Include {
+			for idx, param := range include.Params {
+				stringElement := param.Value.StringVal
+				errs = errs.Also(validateStringVariable(stringElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("", idx).ViaField("matrix.include.params", ""))
+			}
+		}
+	}
+	if m.hasParams() {
+		for _, param := range m.Params {
+			for idx, arrayElement := range param.Value.ArrayVal {
+				errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix.params", param.Name))
+			}
+		}
+	}
+	return errs
+}
+
+func (m *Matrix) validateParameterInOneOfMatrixOrParams(params []Param) (errs *apis.FieldError) {
+	matrixParameterNames := sets.NewString()
+	if m != nil {
+		for _, param := range m.Params {
+			matrixParameterNames.Insert(param.Name)
+		}
+	}
+	for _, param := range params {
+		if matrixParameterNames.Has(param.Name) {
+			errs = errs.Also(apis.ErrMultipleOneOf("matrix["+param.Name+"]", "params["+param.Name+"]"))
+		}
+	}
+	return errs
+}

pkg/apis/pipeline/v1beta1/param_types.go

@@ -524,32 +524,6 @@ func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix
 	return errs
 }
 
-// validatePipelineParametersVariablesInMatrixParameters validates matrix param value
-// that may contain the reference(s) to other params to make sure those references are used appropriately.
-func validatePipelineParametersVariablesInMatrixParameters(matrix []Param, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
-	for _, param := range matrix {
-		for idx, arrayElement := range param.Value.ArrayVal {
-			errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix", param.Name))
-		}
-	}
-	return errs
-}
-
-func validateParameterInOneOfMatrixOrParams(matrix *Matrix, params []Param) (errs *apis.FieldError) {
-	matrixParameterNames := sets.NewString()
-	if matrix != nil {
-		for _, param := range matrix.Params {
-			matrixParameterNames.Insert(param.Name)
-		}
-	}
-	for _, param := range params {
-		if matrixParameterNames.Has(param.Name) {
-			errs = errs.Also(apis.ErrMultipleOneOf("matrix["+param.Name+"]", "params["+param.Name+"]"))
-		}
-	}
-	return errs
-}
-
 // validateParamStringValue validates the param value field of string type
 // that may contain references to other isolated array/object params other than string param.
 func validateParamStringValue(param Param, prefix string, paramNames sets.String, arrayVars sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {

pkg/apis/pipeline/v1beta1/pipeline_types.go

@@ -304,7 +304,7 @@ func (pt *PipelineTask) validateMatrix(ctx context.Context) (errs *apis.FieldErr
 		errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "matrix", config.AlphaAPIFields))
 		errs = errs.Also(pt.Matrix.validateCombinationsCount(ctx))
 	}
-	errs = errs.Also(validateParameterInOneOfMatrixOrParams(pt.Matrix, pt.Params))
+	errs = errs.Also(pt.Matrix.validateParameterInOneOfMatrixOrParams(pt.Params))
 	errs = errs.Also(pt.Matrix.validateParamTypes())
 	return errs
 }

pkg/apis/pipeline/v1beta1/pipeline_validation.go

@@ -168,7 +168,7 @@ func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, pa
 	for idx, task := range tasks {
 		errs = errs.Also(validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
 		if task.IsMatrixed() {
-			errs = errs.Also(validatePipelineParametersVariablesInMatrixParameters(task.Matrix.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
+			errs = errs.Also(task.Matrix.validatePipelineParametersVariablesInMatrixParameters(prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
 		}
 		errs = errs.Also(task.WhenExpressions.validatePipelineParametersVariables(prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
 	}