diff --git a/docs/matrix.md b/docs/matrix.md index 479f96d0c32..fcffb8e7440 100644 --- a/docs/matrix.md +++ b/docs/matrix.md @@ -213,6 +213,20 @@ tasks: - name: param-two value: $(params.bar) # array replacement from array param ``` + +`Matrix.Params` supports whole array replacements from array `Parameters`. + +```yaml +tasks: +... +- name: task-4 + taskRef: + name: task-4 + matrix: + params: + - name: param-one + value: $(params.bar[*]) # whole array replacement from array param +``` #### Parameters in Matrix.Include.Params `Matrix.Include.Params` takes string replacements from `Parameters` of type String, Array or Object. diff --git a/examples/v1beta1/pipelineruns/alpha/pipelinerun-with-matrix-array-references.yaml b/examples/v1beta1/pipelineruns/alpha/pipelinerun-with-matrix-array-references.yaml new file mode 100644 index 00000000000..7ef31481a7c --- /dev/null +++ b/examples/v1beta1/pipelineruns/alpha/pipelinerun-with-matrix-array-references.yaml @@ -0,0 +1,69 @@ +apiVersion: tekton.dev/v1beta1 +kind: Task +metadata: + name: platform-browsers + annotations: + description: | + A task that does something cool with platforms and browsers +spec: + params: + - name: platform + default: "" + - name: browser + default: "" + steps: + - name: echo + image: alpine + script: | + echo "$(params.platform) and $(params.browser)" +--- +apiVersion: tekton.dev/v1beta1 +kind: Pipeline +metadata: + name: matrixed-pipeline +spec: + params: + - name: platforms + type: array + - name: browsers + type: array + tasks: + - name: platforms-and-browsers + matrix: + params: + - name: platform + value: $(params.platforms[*]) + - name: browser + value: $(params.browsers[*]) + taskRef: + name: platform-browsers + finally: + - name: platforms-and-browsers-in-finally + matrix: + params: + - name: platform + value: $(params.platforms[*]) + - name: browser + value: $(params.browsers[*]) + taskRef: + name: platform-browsers +--- +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + generateName: matrixed-pr- +spec: + serviceAccountName: "default" + params: + - name: platforms + value: + - linux + - mac + - windows + - name: browsers + value: + - chrome + - safari + - firefox + pipelineRef: + name: matrixed-pipeline diff --git a/pkg/apis/pipeline/v1/matrix_types.go b/pkg/apis/pipeline/v1/matrix_types.go index 67fb8a6b811..03fba3f29aa 100644 --- a/pkg/apis/pipeline/v1/matrix_types.go +++ b/pkg/apis/pipeline/v1/matrix_types.go @@ -300,29 +300,17 @@ func (m *Matrix) validateCombinationsCount(ctx context.Context) (errs *apis.Fiel return errs } -// validateParams validates the type of Parameter for Matrix.Params and Matrix.Include.Params -// Matrix.Params must be of type array. Matrix.Include.Params must be of type string. -// validateParams also validates Matrix.Params for a unique list of params +// validateUniqueParams validates Matrix.Params for a unique list of params // and a unique list of params in each Matrix.Include.Params specification -func (m *Matrix) validateParams() (errs *apis.FieldError) { +func (m *Matrix) validateUniqueParams() (errs *apis.FieldError) { if m != nil { if m.HasInclude() { for i, include := range m.Include { errs = errs.Also(include.Params.validateDuplicateParameters().ViaField(fmt.Sprintf("matrix.include[%d].params", i))) - for _, param := range include.Params { - if param.Value.Type != ParamTypeString { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("parameters of type string only are allowed, but got param type %s", string(param.Value.Type)), "").ViaFieldKey("matrix.include.params", param.Name)) - } - } } } if m.HasParams() { errs = errs.Also(m.Params.validateDuplicateParameters().ViaField("matrix.params")) - for _, param := range m.Params { - if param.Value.Type != ParamTypeArray { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("parameters of type array only are allowed, but got param type %s", string(param.Value.Type)), "").ViaFieldKey("matrix.params", param.Name)) - } - } } } return errs @@ -360,3 +348,24 @@ func (m *Matrix) validateParameterInOneOfMatrixOrParams(params []Param) (errs *a } return errs } + +// validateNoWholeArrayResults() is used to ensure a matrix parameter does not contain result references +// to entire arrays. This is temporary until whole array replacements for matrix parameters are supported. +// See issue #6056 for more details +func (m *Matrix) validateNoWholeArrayResults() (errs *apis.FieldError) { + if m.HasParams() { + for i, param := range m.Params { + val := param.Value.StringVal + expressions, ok := GetVarSubstitutionExpressionsForParam(param) + if ok { + if LooksLikeContainsResultRefs(expressions) { + _, stringIdx := ParseResultName(val) + if stringIdx == "*" { + errs = errs.Also(apis.ErrGeneric("matrix parameters cannot contain whole array result references", "").ViaFieldIndex("matrix.params", i)) + } + } + } + } + } + return errs +} diff --git a/pkg/apis/pipeline/v1/pipeline_types_test.go b/pkg/apis/pipeline/v1/pipeline_types_test.go index 82e193ceccb..d0249b463ff 100644 --- a/pkg/apis/pipeline/v1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1/pipeline_types_test.go @@ -687,94 +687,42 @@ func TestPipelineTask_ValidateMatrix(t *testing.T) { Paths: []string{"matrix.include[0].params[1].name"}, }, }, { - name: "parameters in matrix are strings", + name: "parameters in matrix contain references to param arrays", pt: &PipelineTask{ Name: "task", + Params: Params{{ + Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }, { + Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, + }}, Matrix: &Matrix{ Params: Params{{ - Name: "foo", Value: ParamValue{Type: ParamTypeString, StringVal: "foo"}, + Name: "foo", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.foobar[*])"}, }, { - Name: "bar", Value: ParamValue{Type: ParamTypeString, StringVal: "bar"}, + Name: "bar", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.barfoo[*])"}, }}}, }, - wantErrs: &apis.FieldError{ - Message: "invalid value: parameters of type array only are allowed, but got param type string", - Paths: []string{"matrix.params[foo]", "matrix.params[bar]"}, - }, }, { - name: "parameters in matrix are arrays", + name: "parameters in matrix contain result references", pt: &PipelineTask{ Name: "task", Matrix: &Matrix{ Params: Params{{ - Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, - }, { - Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, - }}}, - }, - }, { - name: "parameters in include matrix are strings", - pt: &PipelineTask{ - Name: "task", - Matrix: &Matrix{ - Include: IncludeParamsList{{ - Name: "build-1", - Params: Params{{ - Name: "IMAGE", Value: ParamValue{Type: ParamTypeString, StringVal: "image-1"}, - }, { - Name: "DOCKERFILE", Value: ParamValue{Type: ParamTypeString, StringVal: "path/to/Dockerfile1"}, - }}}, - }}, - }, - }, { - name: "parameters in include matrix are objects", - pt: &PipelineTask{ - Name: "task", - Matrix: &Matrix{ - Include: IncludeParamsList{{ - Name: "build-1", - Params: Params{{ - Name: "barfoo", Value: ParamValue{Type: ParamTypeObject, ObjectVal: map[string]string{ - "url": "$(params.myObject.non-exist-key)", - "commit": "$(params.myString)", - }}, - }, { - Name: "foobar", Value: ParamValue{Type: ParamTypeObject, ObjectVal: map[string]string{ - "url": "$(params.myObject.non-exist-key)", - "commit": "$(params.myString)", - }}, - }}, + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.foo-task.results.a-result)"}}, }}}, }, - wantErrs: &apis.FieldError{ - Message: "invalid value: parameters of type string only are allowed, but got param type object", - Paths: []string{"matrix.include.params[barfoo]", "matrix.include.params[foobar]"}, - }, }, { - name: "parameters in include matrix are arrays", + name: "parameters in matrix contain whole array results references", pt: &PipelineTask{ Name: "task", Matrix: &Matrix{ - Include: IncludeParamsList{{ - Name: "build-1", - Params: Params{{ - Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, - }, { - Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}}}, + Params: Params{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(tasks.foo-task.results.arr-results[*])"}, }}}, }, wantErrs: &apis.FieldError{ - Message: "invalid value: parameters of type string only are allowed, but got param type array", - Paths: []string{"matrix.include.params[barfoo]", "matrix.include.params[foobar]"}, - }, - }, { - name: "parameters in matrix contain results references", - pt: &PipelineTask{ - Name: "task", - Matrix: &Matrix{ - Params: Params{{ - Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.foo-task.results.a-result)"}}, - }}}, + Message: "matrix parameters cannot contain whole array result references", + Paths: []string{"matrix.params[0]"}, }, }, { name: "count of combinations of parameters in the matrix exceeds the maximum", diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index f006989015d..32ff89656b1 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -155,9 +155,10 @@ func (pt *PipelineTask) validateMatrix(ctx context.Context) (errs *apis.FieldErr // when the enable-api-fields feature gate is anything but "alpha". errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "matrix", config.AlphaAPIFields)) errs = errs.Also(pt.Matrix.validateCombinationsCount(ctx)) + errs = errs.Also(pt.Matrix.validateNoWholeArrayResults()) + errs = errs.Also(pt.Matrix.validateUniqueParams()) } errs = errs.Also(pt.Matrix.validateParameterInOneOfMatrixOrParams(pt.Params)) - errs = errs.Also(pt.Matrix.validateParams()) return errs } diff --git a/pkg/apis/pipeline/v1/pipeline_validation_test.go b/pkg/apis/pipeline/v1/pipeline_validation_test.go index db1f677382b..03149b92244 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1/pipeline_validation_test.go @@ -3078,75 +3078,6 @@ func Test_validateMatrix(t *testing.T) { Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, }}, }}, - }, { - name: "parameters in matrix are strings", - tasks: PipelineTaskList{{ - Name: "a-task", - TaskRef: &TaskRef{Name: "a-task"}, - Matrix: &Matrix{ - Params: Params{{ - Name: "foo", Value: ParamValue{Type: ParamTypeString, StringVal: "foo"}, - }, { - Name: "bar", Value: ParamValue{Type: ParamTypeString, StringVal: "bar"}, - }}}, - }, { - Name: "b-task", - TaskRef: &TaskRef{Name: "b-task"}, - Matrix: &Matrix{ - Params: Params{{ - Name: "baz", Value: ParamValue{Type: ParamTypeString, StringVal: "baz"}, - }}}, - }}, - wantErrs: &apis.FieldError{ - Message: "invalid value: parameters of type array only are allowed, but got param type string", - Paths: []string{"[0].matrix.params[foo]", "[0].matrix.params[bar]", "[1].matrix.params[baz]"}, - }, - }, { - name: "parameters in matrix are arrays", - tasks: PipelineTaskList{{ - Name: "a-task", - TaskRef: &TaskRef{Name: "a-task"}, - Matrix: &Matrix{ - Params: Params{{ - Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, - }, { - 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: IncludeParamsList{{ - Name: "test", - Params: Params{{ - 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: IncludeParamsList{{ - Name: "test", - Params: Params{{ - 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{{ diff --git a/pkg/apis/pipeline/v1beta1/matrix_types.go b/pkg/apis/pipeline/v1beta1/matrix_types.go index f1c86d4e062..ac44ff6891f 100644 --- a/pkg/apis/pipeline/v1beta1/matrix_types.go +++ b/pkg/apis/pipeline/v1beta1/matrix_types.go @@ -17,6 +17,7 @@ import ( "context" "fmt" "sort" + "strings" "github.com/tektoncd/pipeline/pkg/apis/config" "golang.org/x/exp/maps" @@ -300,29 +301,17 @@ func (m *Matrix) validateCombinationsCount(ctx context.Context) (errs *apis.Fiel return errs } -// validateParams validates the type of Parameter for Matrix.Params and Matrix.Include.Params -// Matrix.Params must be of type array. Matrix.Include.Params must be of type string. -// validateParams also validates Matrix.Params for a unique list of params +// validateUniqueParams validates Matrix.Params for a unique list of params // and a unique list of params in each Matrix.Include.Params specification -func (m *Matrix) validateParams() (errs *apis.FieldError) { +func (m *Matrix) validateUniqueParams() (errs *apis.FieldError) { if m != nil { if m.HasInclude() { for i, include := range m.Include { errs = errs.Also(include.Params.validateDuplicateParameters().ViaField(fmt.Sprintf("matrix.include[%d].params", i))) - for _, param := range include.Params { - if param.Value.Type != ParamTypeString { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("parameters of type string only are allowed, but got param type %s", string(param.Value.Type)), "").ViaFieldKey("matrix.include.params", param.Name)) - } - } } } if m.HasParams() { errs = errs.Also(m.Params.validateDuplicateParameters().ViaField("matrix.params")) - for _, param := range m.Params { - if param.Value.Type != ParamTypeArray { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("parameters of type array only are allowed, but got param type %s", string(param.Value.Type)), "").ViaFieldKey("matrix.params", param.Name)) - } - } } } return errs @@ -360,3 +349,21 @@ func (m *Matrix) validateParameterInOneOfMatrixOrParams(params Params) (errs *ap } return errs } + +// validateNoWholeArrayResults() is used to ensure a matrix parameter does not contain result references +// to entire arrays. This is temporary until whole array replacements for matrix paraemeters are supported. +// See issue #6056 for more details +func (m *Matrix) validateNoWholeArrayResults() (errs *apis.FieldError) { + if m.HasParams() { + for i, param := range m.Params { + val := param.Value.StringVal + expressions, ok := GetVarSubstitutionExpressionsForParam(param) + if ok { + if LooksLikeContainsResultRefs(expressions) && strings.Contains(val, "[*]") { + errs = errs.Also(apis.ErrGeneric("matrix parameters cannot contain whole array result references", "").ViaFieldIndex("matrix.params", i)) + } + } + } + } + return errs +} diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go index 6f0d32416a2..f137ae152a6 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go @@ -641,32 +641,6 @@ func TestPipelineTask_validateMatrix(t *testing.T) { Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, }}, }, - }, { - name: "parameters in matrix are strings", - pt: &PipelineTask{ - Name: "task", - Matrix: &Matrix{ - Params: Params{{ - Name: "foo", Value: ParamValue{Type: ParamTypeString, StringVal: "foo"}, - }, { - Name: "bar", Value: ParamValue{Type: ParamTypeString, StringVal: "bar"}, - }}}, - }, - wantErrs: &apis.FieldError{ - Message: "invalid value: parameters of type array only are allowed, but got param type string", - Paths: []string{"matrix.params[foo]", "matrix.params[bar]"}, - }, - }, { - name: "parameters in matrix are arrays", - pt: &PipelineTask{ - Name: "task", - Matrix: &Matrix{ - Params: Params{{ - Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, - }, { - Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, - }}}, - }, }, { name: "duplicate parameters in matrix.include.params", pt: &PipelineTask{ @@ -686,69 +660,43 @@ func TestPipelineTask_validateMatrix(t *testing.T) { Paths: []string{"matrix.include[0].params[1].name"}, }, }, { - name: "parameters in include matrix are strings", - pt: &PipelineTask{ - Name: "task", - Matrix: &Matrix{ - Include: IncludeParamsList{{ - Name: "build-1", - Params: Params{{ - Name: "IMAGE", Value: ParamValue{Type: ParamTypeString, StringVal: "image-1"}, - }, { - Name: "DOCKERFILE", Value: ParamValue{Type: ParamTypeString, StringVal: "path/to/Dockerfile1"}, - }}}, - }}, - }, - }, { - name: "parameters in include matrix are objects", + name: "parameters in matrix contain references to param arrays", pt: &PipelineTask{ Name: "task", + Params: Params{{ + Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }, { + Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, + }}, Matrix: &Matrix{ - Include: IncludeParamsList{{ - Name: "build-1", - Params: Params{{ - Name: "barfoo", Value: ParamValue{Type: ParamTypeObject, ObjectVal: map[string]string{ - "url": "$(params.myObject.non-exist-key)", - "commit": "$(params.myString)", - }}, - }, { - Name: "foobar", Value: ParamValue{Type: ParamTypeObject, ObjectVal: map[string]string{ - "url": "$(params.myObject.non-exist-key)", - "commit": "$(params.myString)", - }}, - }}, + Params: Params{{ + Name: "foo", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.foobar[*])"}, + }, { + Name: "bar", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.barfoo[*])"}, }}}, }, - wantErrs: &apis.FieldError{ - Message: "invalid value: parameters of type string only are allowed, but got param type object", - Paths: []string{"matrix.include.params[barfoo]", "matrix.include.params[foobar]"}, - }, }, { - name: "parameters in include matrix are arrays", + name: "parameters in matrix contain result references", pt: &PipelineTask{ Name: "task", Matrix: &Matrix{ - Include: IncludeParamsList{{ - Name: "build-1", - Params: Params{{ - Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, - }, { - Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}}}, + Params: Params{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.foo-task.results.a-result)"}}, }}}, }, - wantErrs: &apis.FieldError{ - Message: "invalid value: parameters of type string only are allowed, but got param type array", - Paths: []string{"matrix.include.params[barfoo]", "matrix.include.params[foobar]"}, - }, }, { - name: "parameters in matrix contain results references", + name: "parameters in matrix contain whole array results references", pt: &PipelineTask{ Name: "task", Matrix: &Matrix{ Params: Params{{ - Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.foo-task.results.a-result)"}}, + Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(tasks.foo-task.results.arr-results[*])"}, }}}, }, + wantErrs: &apis.FieldError{ + Message: "matrix parameters cannot contain whole array result references", + Paths: []string{"matrix.params[0]"}, + }, }, { name: "count of combinations of parameters in the matrix exceeds the maximum", pt: &PipelineTask{ diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 80aeffc6ef8..80bb7c6d175 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -167,9 +167,10 @@ func (pt *PipelineTask) validateMatrix(ctx context.Context) (errs *apis.FieldErr // when the enable-api-fields feature gate is anything but "alpha". errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "matrix", config.AlphaAPIFields)) errs = errs.Also(pt.Matrix.validateCombinationsCount(ctx)) + errs = errs.Also(pt.Matrix.validateNoWholeArrayResults()) + errs = errs.Also(pt.Matrix.validateUniqueParams()) } errs = errs.Also(pt.Matrix.validateParameterInOneOfMatrixOrParams(pt.Params)) - errs = errs.Also(pt.Matrix.validateParams()) return errs } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 3d07c10b31d..04b366fa8f0 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -3122,75 +3122,6 @@ func Test_validateMatrix(t *testing.T) { Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, }}, }}, - }, { - name: "parameters in matrix are strings", - tasks: PipelineTaskList{{ - Name: "a-task", - TaskRef: &TaskRef{Name: "a-task"}, - Matrix: &Matrix{ - Params: Params{{ - Name: "foo", Value: ParamValue{Type: ParamTypeString, StringVal: "foo"}, - }, { - Name: "bar", Value: ParamValue{Type: ParamTypeString, StringVal: "bar"}, - }}}, - }, { - Name: "b-task", - TaskRef: &TaskRef{Name: "b-task"}, - Matrix: &Matrix{ - Params: Params{{ - Name: "baz", Value: ParamValue{Type: ParamTypeString, StringVal: "baz"}, - }}}, - }}, - wantErrs: &apis.FieldError{ - Message: "invalid value: parameters of type array only are allowed, but got param type string", - Paths: []string{"[0].matrix.params[foo]", "[0].matrix.params[bar]", "[1].matrix.params[baz]"}, - }, - }, { - name: "parameters in matrix are arrays", - tasks: PipelineTaskList{{ - Name: "a-task", - TaskRef: &TaskRef{Name: "a-task"}, - Matrix: &Matrix{ - Params: Params{{ - Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, - }, { - 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: IncludeParamsList{{ - Name: "test", - Params: Params{{ - 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: IncludeParamsList{{ - Name: "test", - Params: Params{{ - 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{{ diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 50023b58bc2..39f89587e43 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -113,6 +113,8 @@ const ( // ReasonCouldntTimeOut indicates that a PipelineRun was timed out but attempting to update // all of the running TaskRuns as timed out failed. ReasonCouldntTimeOut = "PipelineRunCouldntTimeOut" + // ReasonInvalidMatrixParameterTypes indicates a matrix contains invalid parameter types + ReasonInvalidMatrixParameterTypes = "ReasonInvalidMatrixParameterTypes" // ReasonInvalidTaskResultReference indicates a task result was declared // but was not initialized by that task ReasonInvalidTaskResultReference = "InvalidTaskResultReference" @@ -754,6 +756,15 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip continue } + // Validate parameter types in matrix after apply substitutions from Task Results + if rpt.PipelineTask.IsMatrixed() { + if err := resources.ValidateParameterTypesInMatrix(pipelineRunFacts.State); err != nil { + logger.Errorf("Failed to validate matrix %q with error %v", pr.Name, err) + pr.Status.MarkFailed(ReasonInvalidMatrixParameterTypes, err.Error()) + return controller.NewPermanentError(err) + } + } + defer func() { // If it is a permanent error, set pipelinerun to a failure state directly to avoid unnecessary retries. if err != nil && controller.IsPermanentError(err) { diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 8526fbec8de..4ad9719a842 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -6572,10 +6572,36 @@ spec: - name: optional-workspace optional: true serviceAccountName: test-sa +`), + parse.MustParseV1beta1PipelineRun(t, ` +metadata: + name: pipelinerun-matrix-param-invalid-type + namespace: foo +spec: + pipelineSpec: + tasks: + - name: mytask + params: + - name: platform + taskSpec: + steps: + - name: echo + image: alpine + script: | + echo "$(params.platform)" + - name: b-task + taskRef: + name: mytask + matrix: + params: + - name: platform + value: linux `), } + cms := []*corev1.ConfigMap{withEnabledAlphaAPIFields(newFeatureFlagsConfigMap())} d := test.Data{ + ConfigMaps: cms, PipelineRuns: prs, ServiceAccounts: []*corev1.ServiceAccount{{ ObjectMeta: metav1.ObjectMeta{Name: prs[0].Spec.ServiceAccountName, Namespace: "foo"}, @@ -6585,30 +6611,38 @@ spec: prt := newPipelineRunTest(t, d) defer prt.Cancel() - run1, _ := prt.reconcileRun("foo", "pipelinerun-param-invalid-result-variable", nil, true) - run2, _ := prt.reconcileRun("foo", "pipelinerun-pipeline-result-invalid-result-variable", nil, true) - run3, _ := prt.reconcileRun("foo", "pipelinerun-with-optional-workspace-validation", nil, true) - - cond1 := run1.Status.GetCondition(apis.ConditionSucceeded) - cond2 := run2.Status.GetCondition(apis.ConditionSucceeded) - cond3 := run3.Status.GetCondition(apis.ConditionSucceeded) - - for _, c := range []*apis.Condition{cond1, cond2, cond3} { - if c.Status != corev1.ConditionFalse { - t.Errorf("expected Succeeded/False condition but saw: %v", c) - } - } - - if cond1.Reason != ReasonInvalidTaskResultReference { - t.Errorf("expected invalid task reference condition but saw: %v", cond1) - } - - if cond2.Reason != ReasonInvalidTaskResultReference { - t.Errorf("expected invalid task reference condition but saw: %v", cond2) + testCases := []struct { + name string + reason string + }{ + { + name: "pipelinerun-param-invalid-result-variable", + reason: ReasonInvalidTaskResultReference, + }, { + name: "pipelinerun-pipeline-result-invalid-result-variable", + reason: ReasonInvalidTaskResultReference, + }, { + name: "pipelinerun-with-optional-workspace-validation", + reason: ReasonRequiredWorkspaceMarkedOptional, + }, { + name: "pipelinerun-matrix-param-invalid-type", + reason: ReasonInvalidMatrixParameterTypes, + }, } - - if cond3.Reason != ReasonRequiredWorkspaceMarkedOptional { - t.Errorf("expected optional workspace not supported condition but saw: %v", cond3) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + run, _ := prt.reconcileRun("foo", tc.name, nil, true) + if run == nil { + t.Fatalf("Could not reconcile run %s", tc.name) + } + c := run.Status.GetCondition(apis.ConditionSucceeded) + if c.Status != corev1.ConditionFalse { + t.Errorf("expected Succeeded/False condition but saw: %v", c) + } + if c.Reason != tc.reason { + t.Errorf("want reason %s but got %s", tc.reason, c.Reason) + } + }) } } @@ -7966,6 +8000,253 @@ spec: }) } } +func TestReconciler_PipelineTaskMatrixWithArrayReferences(t *testing.T) { + names.TestingSeed() + + task := parse.MustParseV1beta1Task(t, ` +metadata: + name: mytask + namespace: foo +spec: + params: + - name: platform + - name: browser + steps: + - name: echo + image: alpine + script: | + echo "$(params.platform) and $(params.browser)" +`) + + cms := []*corev1.ConfigMap{withEnabledAlphaAPIFields(newFeatureFlagsConfigMap())} + cms = append(cms, withMaxMatrixCombinationsCount(newDefaultsConfigMap(), 10)) + + tests := []struct { + name string + memberOf string + p *v1beta1.Pipeline + tr *v1beta1.TaskRun + expectedPipelineRun *v1beta1.PipelineRun + }{{ + name: "p-dag", + p: parse.MustParseV1beta1Pipeline(t, fmt.Sprintf(` +metadata: + name: %s + namespace: foo +spec: + params: + - name: platforms + type: array + - name: browsers + type: array + tasks: + - name: platforms-and-browsers + taskRef: + name: mytask + matrix: + params: + - name: platform + value: $(params.platforms[*]) + - name: browser + value: $(params.browsers[*]) + +`, "p-dag")), + expectedPipelineRun: parse.MustParseV1beta1PipelineRun(t, ` +metadata: + name: pr + namespace: foo + annotations: {} + labels: + tekton.dev/pipeline: p-dag +spec: + params: + - name: platforms + value: + - linux + - mac + - windows + - name: browsers + value: + - chrome + - safari + - firefox + serviceAccountName: test-sa + pipelineRef: + name: p-dag +status: + pipelineSpec: + params: + - name: platforms + type: array + - name: browsers + type: array + tasks: + - name: platforms-and-browsers + matrix: + params: + - name: platform + value: + - linux + - mac + - windows + - name: browser + value: + - chrome + - safari + - firefox + taskRef: + name: mytask + kind: Task + conditions: + - type: Succeeded + status: "Unknown" + reason: "Running" + message: "Tasks Completed: 0 (Failed: 0, Cancelled 0), Incomplete: 1, Skipped: 0" + childReferences: + - apiVersion: tekton.dev/v1beta1 + kind: TaskRun + name: pr-platforms-and-browsers-0 + pipelineTaskName: platforms-and-browsers + - apiVersion: tekton.dev/v1beta1 + kind: TaskRun + name: pr-platforms-and-browsers-1 + pipelineTaskName: platforms-and-browsers + - apiVersion: tekton.dev/v1beta1 + kind: TaskRun + name: pr-platforms-and-browsers-2 + pipelineTaskName: platforms-and-browsers + - apiVersion: tekton.dev/v1beta1 + kind: TaskRun + name: pr-platforms-and-browsers-3 + pipelineTaskName: platforms-and-browsers + - apiVersion: tekton.dev/v1beta1 + kind: TaskRun + name: pr-platforms-and-browsers-4 + pipelineTaskName: platforms-and-browsers + - apiVersion: tekton.dev/v1beta1 + kind: TaskRun + name: pr-platforms-and-browsers-5 + pipelineTaskName: platforms-and-browsers + - apiVersion: tekton.dev/v1beta1 + kind: TaskRun + name: pr-platforms-and-browsers-6 + pipelineTaskName: platforms-and-browsers + - apiVersion: tekton.dev/v1beta1 + kind: TaskRun + name: pr-platforms-and-browsers-7 + pipelineTaskName: platforms-and-browsers + - apiVersion: tekton.dev/v1beta1 + kind: TaskRun + name: pr-platforms-and-browsers-8 + pipelineTaskName: platforms-and-browsers +`), + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pr := parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(` +metadata: + name: pr + namespace: foo +spec: + serviceAccountName: test-sa + params: + - name: platforms + value: + - linux + - mac + - windows + - name: browsers + value: + - chrome + - safari + - firefox + pipelineRef: + name: %s +`, tt.name)) + d := test.Data{ + PipelineRuns: []*v1beta1.PipelineRun{pr}, + Pipelines: []*v1beta1.Pipeline{tt.p}, + Tasks: []*v1beta1.Task{task}, + ConfigMaps: cms, + } + if tt.tr != nil { + d.TaskRuns = []*v1beta1.TaskRun{tt.tr} + } + + expectedTaskRunsData := []struct { + browser string + platform string + }{{ + browser: "chrome", + platform: "linux", + }, { + browser: "chrome", + platform: "mac", + }, { + browser: "chrome", + platform: "windows", + }, { + browser: "safari", + platform: "linux", + }, { + browser: "safari", + platform: "mac", + }, { + browser: "safari", + platform: "windows", + }, { + browser: "firefox", + platform: "linux", + }, { + browser: "firefox", + platform: "mac", + }, { + browser: "firefox", + platform: "windows", + }} + + expectedTaskRuns := []*v1beta1.TaskRun{} + for i, trd := range expectedTaskRunsData { + trName := "pr-platforms-and-browsers-" + strconv.Itoa(i) + expectedTaskRuns = append(expectedTaskRuns, mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta(trName, "foo", "pr", "p-dag", "platforms-and-browsers", false), + fmt.Sprintf(` +spec: + params: + - name: browser + value: %s + - name: platform + value: %s + serviceAccountName: test-sa + taskRef: + name: mytask + kind: Task +labels: + tekton.dev/memberOf: tasks + tekton.dev/pipeline: p-dag +`, trd.browser, trd.platform))) + } + + prt := newPipelineRunTest(t, d) + defer prt.Cancel() + + pipelineRun, clients := prt.reconcileRun(pr.Namespace, pr.Name /*wantEvents*/, []string{} /*permanentError*/, false) + taskRuns := getTaskRunsForPipelineRun(prt.TestAssets.Ctx, t, clients, pr.Namespace, pr.Name) + validateTaskRunsCount(t, taskRuns, len(expectedTaskRuns)) + + for i, expectedTaskRun := range expectedTaskRuns { + trName := expectedTaskRun.Name + actual := getTaskRunByName(t, taskRuns, trName) + if d := cmp.Diff(expectedTaskRun, actual, ignoreResourceVersion, ignoreTypeMeta); d != "" { + t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRuns[i].Name, diff.PrintWantGot(d)) + } + } + if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, ignoreFinallyStartTime, cmpopts.EquateEmpty()); d != "" { + t.Errorf("found PipelineRun does not match expected PipelineRun. Diff %s", diff.PrintWantGot(d)) + } + }) + } +} func TestReconciler_PipelineTaskIncludeParams(t *testing.T) { names.TestingSeed() diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 01724843f84..bc70193cd56 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -477,7 +477,7 @@ func (t *ResolvedPipelineTask) skipBecausePipelineRunFinallyTimeoutReached(facts func (t *ResolvedPipelineTask) skipBecauseEmptyArrayInMatrixParams() bool { if t.PipelineTask.IsMatrixed() { for _, ps := range t.PipelineTask.Matrix.Params { - if len(ps.Value.ArrayVal) == 0 { + if ps.Value.Type == v1beta1.ParamTypeArray && len(ps.Value.ArrayVal) == 0 { return true } } diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go index b50b0cfe28d..7659ca3a400 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params.go @@ -84,3 +84,29 @@ func ValidateObjectParamRequiredKeys(pipelineParameters []v1beta1.ParamSpec, pip return nil } + +// ValidateParameterTypesInMatrix validates the type of Parameter for Matrix.Params +// and Matrix.Include.Params after any replacements are made from Task parameters or results +// Matrix.Params must be of type array. Matrix.Include.Params must be of type string. +func ValidateParameterTypesInMatrix(state PipelineRunState) error { + for _, rpt := range state { + m := rpt.PipelineTask.Matrix + if m.HasInclude() { + for _, include := range m.Include { + for _, param := range include.Params { + if param.Value.Type != v1beta1.ParamTypeString { + return fmt.Errorf("parameters of type string only are allowed, but param %s has type %s", param.Name, string(param.Value.Type)) + } + } + } + } + if m.HasParams() { + for _, param := range m.Params { + if param.Value.Type != v1beta1.ParamTypeArray { + return fmt.Errorf("parameters of type array only are allowed, but param %s has type %s", param.Name, string(param.Value.Type)) + } + } + } + } + return nil +} diff --git a/pkg/reconciler/pipelinerun/resources/validate_params_test.go b/pkg/reconciler/pipelinerun/resources/validate_params_test.go index 1057785a7b1..b66378205a9 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params_test.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params_test.go @@ -317,3 +317,104 @@ func TestValidateObjectParamRequiredKeys_Valid(t *testing.T) { }) } } + +// ValidateMatrixPipelineParameterTypes tests that a pipeline task with +// a matrix has the correct parameter types after any replacements are made +func TestValidatePipelineParameterTypes(t *testing.T) { + for _, tc := range []struct { + desc string + state resources.PipelineRunState + wantErrs string + }{{ + desc: "parameters in matrix are arrays", + state: resources.PipelineRunState{{ + PipelineTask: &v1beta1.PipelineTask{ + Name: "task", + Matrix: &v1beta1.Matrix{ + Params: v1beta1.Params{{ + Name: "foobar", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }, { + Name: "barfoo", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeArray, ArrayVal: []string{"bar", "foo"}}}}, + }, + }, + }}, + }, { + desc: "parameters in matrix are strings", + state: resources.PipelineRunState{{ + PipelineTask: &v1beta1.PipelineTask{ + Name: "task", + Matrix: &v1beta1.Matrix{ + Params: v1beta1.Params{{ + Name: "foo", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeString, StringVal: "foo"}, + }, { + Name: "bar", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeString, StringVal: "bar"}, + }}}, + }, + }}, + wantErrs: "parameters of type array only are allowed, but param foo has type string", + }, { + desc: "parameters in include matrix are strings", + state: resources.PipelineRunState{{ + PipelineTask: &v1beta1.PipelineTask{ + Name: "task", + Matrix: &v1beta1.Matrix{ + Include: v1beta1.IncludeParamsList{{ + Name: "build-1", + Params: v1beta1.Params{{ + Name: "foo", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeString, StringVal: "foo"}, + }, { + Name: "bar", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeString, StringVal: "bar"}}}, + }}}, + }, + }}, + }, { + desc: "parameters in include matrix are arrays", + state: resources.PipelineRunState{{ + PipelineTask: &v1beta1.PipelineTask{ + Name: "task", + Matrix: &v1beta1.Matrix{ + Include: v1beta1.IncludeParamsList{{ + Name: "build-1", + Params: v1beta1.Params{{ + Name: "foobar", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }, { + Name: "barfoo", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeArray, ArrayVal: []string{"bar", "foo"}}}}, + }}}, + }, + }}, + wantErrs: "parameters of type string only are allowed, but param foobar has type array", + }, { + desc: "parameters in include matrix are objects", + state: resources.PipelineRunState{{ + PipelineTask: &v1beta1.PipelineTask{ + Name: "task", + Matrix: &v1beta1.Matrix{ + Include: v1beta1.IncludeParamsList{{ + Name: "build-1", + Params: v1beta1.Params{{ + Name: "barfoo", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeObject, ObjectVal: map[string]string{ + "url": "$(params.myObject.non-exist-key)", + "commit": "$(params.myString)", + }}, + }, { + Name: "foobar", Value: v1beta1.ParamValue{Type: v1beta1.ParamTypeObject, ObjectVal: map[string]string{ + "url": "$(params.myObject.non-exist-key)", + "commit": "$(params.myString)", + }}, + }}, + }}}, + }, + }}, + wantErrs: "parameters of type string only are allowed, but param barfoo has type object", + }} { + t.Run(tc.desc, func(t *testing.T) { + err := resources.ValidateParameterTypesInMatrix(tc.state) + if (err != nil) && err.Error() != tc.wantErrs { + t.Errorf("expected err: %s, but got err %s", tc.wantErrs, err) + } + if tc.wantErrs == "" && err != nil { + t.Errorf("got unexpected error: %v", err) + } + }) + } +}