refactor param array indexing validation

This commit refactors param array indexing validation to extract all
references first, then validate if references are out of bound.
Previously we traverse all references and collect invalid references
there. The benefit of this refactoring is that it would be convenient
for other validation to reuse these references.

Signed-off-by: Yongxuan Zhang [email protected]

Author: Yongxuanzhang

pkg/reconciler/pipelinerun/resources/validate_params.go

@@ -24,8 +24,6 @@ import (
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/list"
 	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun"
-	"github.com/tektoncd/pipeline/pkg/substitution"
-	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 // ValidateParamTypesMatching validate that parameters in PipelineRun override corresponding parameters in Pipeline of the same type.
@@ -95,103 +93,49 @@ func ValidateParamArrayIndex(ctx context.Context, p *v1beta1.PipelineSpec, pr *v
 		return nil
 	}
 
-	arrayParams := extractParamIndexes(p.Params, pr.Spec.Params)
-
-	outofBoundParams := sets.String{}
+	arrayParams := taskrun.ExtractParamArrayLengths(p.Params, pr.Spec.Params)
 
+	arrayIndexingParams := []string{}
 	// collect all the references
 	for i := range p.Tasks {
-		findInvalidParamArrayReferences(p.Tasks[i].Params, arrayParams, &outofBoundParams)
+		extractArrayIndexingReferencesFromParams(p.Tasks[i].Params, arrayParams, &arrayIndexingParams)
 		if p.Tasks[i].IsMatrixed() {
-			findInvalidParamArrayReferences(p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams)
+			extractArrayIndexingReferencesFromParams(p.Tasks[i].Matrix.Params, arrayParams, &arrayIndexingParams)
 		}
 		for j := range p.Tasks[i].Workspaces {
-			findInvalidParamArrayReference(p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams)
+			taskrun.ExtractArrayIndexingParamReference(p.Tasks[i].Workspaces[j].SubPath, arrayParams, &arrayIndexingParams)
 		}
 		for _, wes := range p.Tasks[i].WhenExpressions {
-			findInvalidParamArrayReference(wes.Input, arrayParams, &outofBoundParams)
+			taskrun.ExtractArrayIndexingParamReference(wes.Input, arrayParams, &arrayIndexingParams)
 			for _, v := range wes.Values {
-				findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
+				taskrun.ExtractArrayIndexingParamReference(v, arrayParams, &arrayIndexingParams)
 			}
 		}
 	}
 
 	for i := range p.Finally {
-		findInvalidParamArrayReferences(p.Finally[i].Params, arrayParams, &outofBoundParams)
+		extractArrayIndexingReferencesFromParams(p.Finally[i].Params, arrayParams, &arrayIndexingParams)
 		if p.Finally[i].IsMatrixed() {
-			findInvalidParamArrayReferences(p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams)
+			extractArrayIndexingReferencesFromParams(p.Finally[i].Matrix.Params, arrayParams, &arrayIndexingParams)
 		}
 		for _, wes := range p.Finally[i].WhenExpressions {
 			for _, v := range wes.Values {
-				findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
+				taskrun.ExtractArrayIndexingParamReference(v, arrayParams, &arrayIndexingParams)
 			}
 		}
 	}
-
-	if outofBoundParams.Len() > 0 {
-		return fmt.Errorf("non-existent param references:%v", outofBoundParams.List())
-	}
-	return nil
+	return taskrun.ValidateOutofBoundArrayParams(arrayIndexingParams, arrayParams)
 }
 
-func extractParamIndexes(defaults []v1beta1.ParamSpec, params []v1beta1.Param) map[string]int {
-	// Collect all array params
-	arrayParams := make(map[string]int)
-
-	patterns := []string{
-		"$(params.%s)",
-		"$(params[%q])",
-		"$(params['%s'])",
-	}
-
-	// Collect array params lengths from defaults
-	for _, p := range defaults {
-		if p.Default != nil {
-			if p.Default.Type == v1beta1.ParamTypeArray {
-				for _, pattern := range patterns {
-					for i := 0; i < len(p.Default.ArrayVal); i++ {
-						arrayParams[fmt.Sprintf(pattern, p.Name)] = len(p.Default.ArrayVal)
-					}
-				}
-			}
-		}
-	}
-
-	// Collect array params lengths from pipelinerun or taskrun
-	for _, p := range params {
-		if p.Value.Type == v1beta1.ParamTypeArray {
-			for _, pattern := range patterns {
-				for i := 0; i < len(p.Value.ArrayVal); i++ {
-					arrayParams[fmt.Sprintf(pattern, p.Name)] = len(p.Value.ArrayVal)
-				}
-			}
-		}
-	}
-	return arrayParams
-}
-
-func findInvalidParamArrayReferences(params []v1beta1.Param, arrayParams map[string]int, outofBoundParams *sets.String) {
+// extractArrayIndexingReferencesFromParams get all array indexing references from params
+func extractArrayIndexingReferencesFromParams(params []v1beta1.Param, arrayParams map[string]int, arrayIndexingParams *[]string) {
 	for i := range params {
-		findInvalidParamArrayReference(params[i].Value.StringVal, arrayParams, outofBoundParams)
+		taskrun.ExtractArrayIndexingParamReference(params[i].Value.StringVal, arrayParams, arrayIndexingParams)
 		for _, v := range params[i].Value.ArrayVal {
-			findInvalidParamArrayReference(v, arrayParams, outofBoundParams)
+			taskrun.ExtractArrayIndexingParamReference(v, arrayParams, arrayIndexingParams)
 		}
 		for _, v := range params[i].Value.ObjectVal {
-			findInvalidParamArrayReference(v, arrayParams, outofBoundParams)
-		}
-	}
-}
-
-func findInvalidParamArrayReference(paramReference string, arrayParams map[string]int, outofBoundParams *sets.String) {
-	list := substitution.ExtractParamsExpressions(paramReference)
-	for _, val := range list {
-		indexString := substitution.ExtractIndexString(paramReference)
-		idx, _ := substitution.ExtractIndex(indexString)
-		v := substitution.TrimArrayIndex(val)
-		if paramLength, ok := arrayParams[v]; ok {
-			if idx >= paramLength {
-				outofBoundParams.Insert(val)
-			}
+			taskrun.ExtractArrayIndexingParamReference(v, arrayParams, arrayIndexingParams)
 		}
 	}
 }

pkg/reconciler/pipelinerun/resources/validate_params_test.go

@@ -647,10 +647,15 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) {
 					Operator: selection.In,
 					Values:   []string{"$(params.second-param[2])"},
 				}},
+				Matrix: &v1beta1.Matrix{
+					Params: []v1beta1.Param{
+						{Name: "final-task-first-param", Value: *v1beta1.NewStructuredValues("$(params.first-param[4])")},
+						{Name: "final-task-second-param", Value: *v1beta1.NewStructuredValues("$(params.second-param[4])")},
+					}},
 			}},
 		},
 		params:   []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}},
-		expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.first-param[3]) $(params.second-param[2]) $(params.second-param[3])]"),
+		expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.first-param[3]) $(params.first-param[4]) $(params.second-param[2]) $(params.second-param[3]) $(params.second-param[4])]"),
 	}, {
 		name: "parameter references with bracket notation and special characters reference out of bound",
 		original: v1beta1.PipelineSpec{
@@ -698,6 +703,24 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) {
 		},
 		params:   []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}},
 		expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"),
+	}, {
+		name: "parameter in matrix reference out of bound",
+		original: v1beta1.PipelineSpec{
+			Params: []v1beta1.ParamSpec{
+				{Name: "first-param", Type: v1beta1.ParamTypeArray, Default: v1beta1.NewStructuredValues("default-value", "default-value-again")},
+				{Name: "second-param", Type: v1beta1.ParamTypeArray},
+			},
+			Tasks: []v1beta1.PipelineTask{{
+				Matrix: &v1beta1.Matrix{
+					Params: []v1beta1.Param{
+						{Name: "first-task-first-param", Value: *v1beta1.NewStructuredValues("$(params.first-param[2])")},
+						{Name: "first-task-second-param", Value: *v1beta1.NewStructuredValues("static value")},
+					},
+				},
+			}},
+		},
+		params:   []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}},
+		expected: fmt.Errorf("non-existent param references:[$(params.first-param[2])]"),
 	},
 	} {
 		tt := tt // capture range variable