Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TEP-0118: Add validation for matrix include pipeline parameter variables #6235

Merged
merged 1 commit into from
Mar 7, 2023
Merged

TEP-0118: Add validation for matrix include pipeline parameter variables #6235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions pkg/apis/pipeline/v1/matrix_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/config"
"k8s.io/apimachinery/pkg/util/sets"
"knative.dev/pkg/apis"
)

Expand Down Expand Up @@ -147,3 +148,41 @@ 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
// Matrix Include Params must be of type string
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 {
// Matrix Params must be of type array
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
}
26 changes: 0 additions & 26 deletions pkg/apis/pipeline/v1/param_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1/pipeline_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1/pipeline_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
80 changes: 74 additions & 6 deletions pkg/apis/pipeline/v1/pipeline_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
EmmaMunley marked this conversation as resolved.
Show resolved Hide resolved
params: []ParamSpec{{
Name: "baz", Type: ParamTypeString,
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Matrix: &Matrix{
Include: []MatrixInclude{{
Name: "build-1",
Params: []Param{{
EmmaMunley marked this conversation as resolved.
Show resolved Hide resolved
Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.baz)"}},
}}}},
}},
}, {
name: "object param - using single individual variable in string param",
params: []ParamSpec{{
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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)"},
}},
EmmaMunley marked this conversation as resolved.
Show resolved Hide resolved
}}},
}},
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",
Expand Down Expand Up @@ -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",
}, {
Expand Down Expand Up @@ -2409,7 +2444,6 @@ func TestValidateFinalTasks_Failure(t *testing.T) {
})
}
}

func TestContextValid(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -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{{
Expand Down
39 changes: 39 additions & 0 deletions pkg/apis/pipeline/v1beta1/matrix_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/config"
"k8s.io/apimachinery/pkg/util/sets"
"knative.dev/pkg/apis"
)

Expand Down Expand Up @@ -147,3 +148,41 @@ 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
// Matrix Include Params must be of type string
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 {
// Matrix Params must be of type array
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
}
26 changes: 0 additions & 26 deletions pkg/apis/pipeline/v1beta1/param_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/pipeline_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/pipeline_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
Loading