TEP-0075: Validate Pipeline object variables in value, matrix and when

Besides task steps, Parameter variables can be used as the value of a param, matrix or when expression. Those validations regarding object type are added: - The whole object value can only be used when providing values for other object param. example: ``` params: - name: arg value: $(params.myObject[*]) ``` - Individual object attributes should be used in all other cases (string/array val, matrix and when expression).
tektoncd · May 24, 2022 · 099f6ac · 099f6ac
1 parent 2cff700
commit 099f6ac
Show file tree

Hide file tree

Showing 4 changed files with 308 additions and 19 deletions.
diff --git a/pkg/apis/pipeline/v1beta1/param_types.go b/pkg/apis/pipeline/v1beta1/param_types.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"strings"
 
 	resource "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1"
@@ -210,23 +211,28 @@ func ArrayReference(a string) string {
 	return strings.TrimSuffix(strings.TrimPrefix(a, "$("+ParamsPrefix+"."), "[*])")
 }
 
-func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix string, paramNames sets.String, arrayParamNames sets.String) (errs *apis.FieldError) {
+func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
 	for _, param := range params {
-		if param.Value.Type == ParamTypeString {
-			errs = errs.Also(validateStringVariable(param.Value.StringVal, prefix, paramNames, arrayParamNames).ViaFieldKey("params", param.Name))
-		} else {
+		switch param.Value.Type {
+		case ParamTypeArray:
 			for idx, arrayElement := range param.Value.ArrayVal {
-				errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames).ViaFieldIndex("value", idx).ViaFieldKey("params", param.Name))
+				errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("params", param.Name))
 			}
+		case ParamTypeObject:
+			for key, val := range param.Value.ObjectVal {
+				errs = errs.Also(validateStringVariable(val, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldKey("properties", key).ViaFieldKey("params", param.Name))
+			}
+		default:
+			errs = errs.Also(validateStringVariableP(param, prefix, paramNames, arrayParamNames, objectParamNameKeys))
 		}
 	}
 	return errs
 }
 
-func validatePipelineParametersVariablesInMatrixParameters(matrix []Param, prefix string, paramNames sets.String, arrayParamNames sets.String) (errs *apis.FieldError) {
+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).ViaFieldIndex("value", idx).ViaFieldKey("matrix", param.Name))
+			errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix", param.Name))
 		}
 	}
 	return errs
@@ -258,12 +264,59 @@ func validateParameterInOneOfMatrixOrParams(matrix []Param, params []Param) (err
 	return errs
 }
 
-func validateStringVariable(value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError {
+// validateStringVariableP validates param variable usage in string value
+// see https://github.com/tektoncd/pipeline/issues/4879#issuecomment-1133120522 for background
+// This will allow using the whole object value to provide value for object param. example:
+// params:
+// 	- name: arg
+// 		value: $(params.myObject[*])
+func validateStringVariableP(param Param, prefix string, stringVars sets.String, arrayVars sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
+	stringValue := param.Value.StringVal
+	singleVariableRegex := regexp.MustCompile(fmt.Sprintf("^\\$\\(%s\\.[_a-zA-Z][_a-zA-Z0-9.-]*\\[\\*\\]\\)$", prefix))
+
+	// if the provided param value is just $(params.myObject[*]) or $(params.myArray[*]), example:
+	// 	params:
+	// 	- name: arg
+	// 		value: $(params.myObject[*])
+	if singleVariableRegex.MatchString(stringValue) {
+		objectParamNames := sets.NewString()
+		for name := range objectParamNameKeys {
+			objectParamNames.Insert(name)
+		}
+		return errs.Also(validateIsolatedArrayOrObject(stringValue, prefix, arrayVars, objectParamNames).ViaFieldKey("params", param.Name))
+	}
+
+	// if the provided param value is string literal and/or contains multiple variables
+	// valid example: "$(params.myString) and another $(params.myObject.key1)"
+	// invalid example: "$(params.myString) and another $(params.myObject[*])"
+	return errs.Also(validateStringVariable(stringValue, prefix, stringVars, arrayVars, objectParamNameKeys).ViaFieldKey("params", param.Name))
+}
+
+func validateStringVariable(value, prefix string, stringVars sets.String, arrayVars sets.String, objectParamNameKeys map[string][]string) *apis.FieldError {
 	errs := substitution.ValidateVariableP(value, prefix, stringVars)
+	errs = errs.Also(validateObjectVariable(value, prefix, objectParamNameKeys))
 	return errs.Also(substitution.ValidateVariableProhibitedP(value, prefix, arrayVars))
 }
 
-func validateArrayVariable(value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError {
+func validateArrayVariable(value, prefix string, stringVars sets.String, arrayVars sets.String, objectParamNameKeys map[string][]string) *apis.FieldError {
 	errs := substitution.ValidateVariableP(value, prefix, stringVars)
+	errs = errs.Also(validateObjectVariable(value, prefix, objectParamNameKeys))
 	return errs.Also(substitution.ValidateVariableIsolatedP(value, prefix, arrayVars))
 }
+
+func validateObjectVariable(value, prefix string, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
+	objectNames := sets.NewString()
+	for objectParamName, keys := range objectParamNameKeys {
+		objectNames.Insert(objectParamName)
+		errs = errs.Also(substitution.ValidateVariableP(value, fmt.Sprintf("%s\\.%s", prefix, objectParamName), sets.NewString(keys...)))
+	}
+
+	// TODO (chuangw6): add this once #4861 is merged, and add tests
+	// return errs.Also(substitution.ValidateEntireVariableProhibitedP(value, prefix, objectNames))
+	return errs
+}
+
+func validateIsolatedArrayOrObject(value, prefix string, arrayVars sets.String, objectVars sets.String) (errs *apis.FieldError) {
+	errs = errs.Also(substitution.ValidateVariableIsolatedP(value, prefix, arrayVars))
+	return errs.Also(substitution.ValidateVariableIsolatedP(value, prefix, objectVars))
+}
diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go
@@ -128,6 +128,7 @@ func validatePipelineWorkspacesUsage(wss []PipelineWorkspaceDeclaration, pts []P
 func validatePipelineParameterVariables(tasks []PipelineTask, params []ParamSpec) (errs *apis.FieldError) {
 	parameterNames := sets.NewString()
 	arrayParameterNames := sets.NewString()
+	objectParameterNameKeys := map[string][]string{}
 
 	for _, p := range params {
 		// Verify that p is a valid type.
@@ -155,16 +156,21 @@ func validatePipelineParameterVariables(tasks []PipelineTask, params []ParamSpec
 		if p.Type == ParamTypeArray {
 			arrayParameterNames.Insert(p.Name)
 		}
-	}
 
-	return errs.Also(validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames))
+		if p.Type == ParamTypeObject {
+			for k := range p.Properties {
+				objectParameterNameKeys[p.Name] = append(objectParameterNameKeys[p.Name], k)
+			}
+		}
+	}
+	return errs.Also(validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames, objectParameterNameKeys))
 }
 
-func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, paramNames sets.String, arrayParamNames sets.String) (errs *apis.FieldError) {
+func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
 	for idx, task := range tasks {
-		errs = errs.Also(validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames).ViaIndex(idx))
-		errs = errs.Also(validatePipelineParametersVariablesInMatrixParameters(task.Matrix, prefix, paramNames, arrayParamNames).ViaIndex(idx))
-		errs = errs.Also(task.WhenExpressions.validatePipelineParametersVariables(prefix, paramNames, arrayParamNames).ViaIndex(idx))
+		errs = errs.Also(validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
+		errs = errs.Also(validatePipelineParametersVariablesInMatrixParameters(task.Matrix, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
+		errs = errs.Also(task.WhenExpressions.validatePipelineParametersVariables(prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
 	}
 	return errs
 }

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
@@ -1346,6 +1346,125 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) {
 				Name: "a-param", Value: ArrayOrString{Type: ParamTypeArray, ArrayVal: []string{"$(baz[*])", "and", "$(foo-is-baz[*])"}},
 			}},
 		}},
+	}, {
+		name: "array param - using the whole variable as a param's value that is intented to be array type",
+		params: []ParamSpec{{
+			Name: "myArray",
+			Type: ParamTypeArray,
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			Params: []Param{{
+				Name: "a-param-intented-to-be-array", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myArray[*])"},
+			}},
+		}},
+	}, {
+		name: "object param - using single individual variable in string param",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			Params: []Param{{
+				Name: "a-string-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myObject.key1)"},
+			}},
+		}},
+	}, {
+		name: "object param - using multiple individual variables in string param",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			Params: []Param{{
+				Name: "a-string-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myObject.key1) and $(params.myObject.key2)"},
+			}},
+		}},
+	}, {
+		name: "object param - using individual variables in array param",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			Params: []Param{{
+				Name: "an-array-param", Value: ArrayOrString{Type: ParamTypeArray, ArrayVal: []string{"$(params.myObject.key1)", "another one $(params.myObject.key2)"}},
+			}},
+		}},
+	}, {
+		name: "object param - using individual variables in matrix",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			Matrix: []Param{{
+				Name: "a-param", Value: ArrayOrString{Type: ParamTypeArray, ArrayVal: []string{"$(params.myObject.key1)", "and", "$(params.myObject.key2)"}},
+			}},
+		}},
+	}, {
+		name: "object param - using the whole variable as a param's value that is intented to be object type",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			Params: []Param{{
+				Name: "a-param-intented-to-be-object", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myObject[*])"},
+			}},
+		}},
+	}, {
+		name: "object param - using individual variable in input of when expression, and using both object individual variable and array reference in values of when expression",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}, {
+			Name: "foo", Type: ParamTypeArray, Default: &ArrayOrString{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			WhenExpressions: []WhenExpression{{
+				Input:    "$(params.myObject.key1)",
+				Operator: selection.In,
+				Values:   []string{"$(params.foo[*])", "$(params.myObject.key2)"},
+			}},
+		}},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -1671,6 +1790,117 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
 			Message: `non-existent variable in "$(params.does-not-exist)"`,
 			Paths:   []string{"[0].matrix[b-param].value[0]"},
 		},
+	}, {
+		name: "invalid object key in the input of the when expression",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			WhenExpressions: []WhenExpression{{
+				Input:    "$(params.myObject.non-exist-key)",
+				Operator: selection.In,
+				Values:   []string{"foo"},
+			}},
+		}},
+		expectedError: apis.FieldError{
+			Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
+			Paths:   []string{"[0].when[0].input"},
+		},
+	}, {
+		name: "invalid object key in the Values of the when expression",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			WhenExpressions: []WhenExpression{{
+				Input:    "bax",
+				Operator: selection.In,
+				Values:   []string{"$(params.myObject.non-exist-key)"},
+			}},
+		}},
+		expectedError: apis.FieldError{
+			Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
+			Paths:   []string{"[0].when[0].values"},
+		},
+	}, {
+		name: "invalid object key is used to provide values for array params",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			Params: []Param{{
+				Name: "a-param", Value: ArrayOrString{Type: ParamTypeArray, ArrayVal: []string{"$(params.myObject.non-exist-key)", "last"}},
+			}},
+		}},
+		expectedError: apis.FieldError{
+			Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
+			Paths:   []string{"[0].params[a-param].value[0]"},
+		},
+	}, {
+		name: "invalid object key is used to provide values for string params",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "bar",
+			TaskRef: &TaskRef{Name: "bar-task"},
+			Params: []Param{{
+				Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.myObject.non-exist-key)"},
+			}},
+		}},
+		expectedError: apis.FieldError{
+			Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
+			Paths:   []string{"[0].params[a-param]"},
+		},
+	}, {
+		name: "invalid object key is used to provide values for string params",
+		params: []ParamSpec{{
+			Name: "myObject",
+			Type: ParamTypeObject,
+			Properties: map[string]PropertySpec{
+				"key1": {Type: "string"},
+				"key2": {Type: "string"},
+			},
+		}},
+		tasks: []PipelineTask{{
+			Name:    "foo-task",
+			TaskRef: &TaskRef{Name: "foo-task"},
+			Matrix: []Param{{
+				Name: "a-param", Value: ArrayOrString{Type: ParamTypeArray, ArrayVal: []string{"$(params.myObject.key1)"}},
+			}, {
+				Name: "b-param", Value: ArrayOrString{Type: ParamTypeArray, ArrayVal: []string{"$(params.myObject.non-exist-key)"}},
+			}},
+		}},
+		expectedError: apis.FieldError{
+			Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
+			Paths:   []string{"[0].matrix[b-param].value[0]"},
+		},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {