Skip to content

Commit

Permalink
refactor param array indexing validation
Browse files Browse the repository at this point in the history 
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]
  • Loading branch information
@Yongxuanzhang
Yongxuanzhang committed Feb 16, 2023
1 parent f3e9fc1 commit d03fc5e
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 167 deletions.
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
}

// Ensure that the array reference is not out of bound
if err := resources.ValidateParamArrayIndex(ctx, pipelineSpec, pr); err != nil {
if err := resources.ValidateParamArrayIndex(ctx, pipelineSpec, pr.Spec.Params); err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.MarkFailed(ReasonObjectParameterMissKeys,
"PipelineRun %s/%s parameters is missing object keys required by Pipeline %s/%s's parameters: %s",
Expand Down
108 changes: 27 additions & 81 deletions pkg/reconciler/pipelinerun/resources/validate_params.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -90,108 +88,56 @@ func ValidateObjectParamRequiredKeys(pipelineParameters []v1beta1.ParamSpec, pip
}

// ValidateParamArrayIndex validate if the array indexing param reference target is existent
func ValidateParamArrayIndex(ctx context.Context, p *v1beta1.PipelineSpec, pr *v1beta1.PipelineRun) error {
func ValidateParamArrayIndex(ctx context.Context, pipelineSpec *v1beta1.PipelineSpec, prParams []v1beta1.Param) error {
if !config.CheckAlphaOrBetaAPIFields(ctx) {
return nil
}

arrayParams := extractParamIndexes(p.Params, pr.Spec.Params)
// Collect all array params lengths
// Example: {"$(params.array-params)": 2, "$(params[\"array-params\"])": 2, "$(params['array-params'])": 2}
arrayParamsLengths := taskrun.ExtractParamArrayLengths(pipelineSpec.Params, prParams)

outofBoundParams := sets.String{}

// collect all the references
for i := range p.Tasks {
findInvalidParamArrayReferences(p.Tasks[i].Params, arrayParams, &outofBoundParams)
if p.Tasks[i].IsMatrixed() {
findInvalidParamArrayReferences(p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams)
// extract all array indexing references, for example []{"$(params.array-params[1])"}
arrayIndexParamRefs := []string{}
for i := range pipelineSpec.Tasks {
extractArrayIndexingRefsFromParams(pipelineSpec.Tasks[i].Params, arrayParamsLengths, &arrayIndexParamRefs)
if pipelineSpec.Tasks[i].IsMatrixed() {
extractArrayIndexingRefsFromParams(pipelineSpec.Tasks[i].Matrix.Params, arrayParamsLengths, &arrayIndexParamRefs)
}
for j := range p.Tasks[i].Workspaces {
findInvalidParamArrayReference(p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams)
for j := range pipelineSpec.Tasks[i].Workspaces {
taskrun.ExtractArrayIndexingParamRefs(pipelineSpec.Tasks[i].Workspaces[j].SubPath, arrayParamsLengths, &arrayIndexParamRefs)
}
for _, wes := range p.Tasks[i].WhenExpressions {
findInvalidParamArrayReference(wes.Input, arrayParams, &outofBoundParams)
for _, wes := range pipelineSpec.Tasks[i].WhenExpressions {
taskrun.ExtractArrayIndexingParamRefs(wes.Input, arrayParamsLengths, &arrayIndexParamRefs)
for _, v := range wes.Values {
findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
taskrun.ExtractArrayIndexingParamRefs(v, arrayParamsLengths, &arrayIndexParamRefs)
}
}
}

for i := range p.Finally {
findInvalidParamArrayReferences(p.Finally[i].Params, arrayParams, &outofBoundParams)
if p.Finally[i].IsMatrixed() {
findInvalidParamArrayReferences(p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams)
for i := range pipelineSpec.Finally {
extractArrayIndexingRefsFromParams(pipelineSpec.Finally[i].Params, arrayParamsLengths, &arrayIndexParamRefs)
if pipelineSpec.Finally[i].IsMatrixed() {
extractArrayIndexingRefsFromParams(pipelineSpec.Finally[i].Matrix.Params, arrayParamsLengths, &arrayIndexParamRefs)
}
for _, wes := range p.Finally[i].WhenExpressions {
for _, wes := range pipelineSpec.Finally[i].WhenExpressions {
for _, v := range wes.Values {
findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
taskrun.ExtractArrayIndexingParamRefs(v, arrayParamsLengths, &arrayIndexParamRefs)
}
}
}

if outofBoundParams.Len() > 0 {
return fmt.Errorf("non-existent param references:%v", outofBoundParams.List())
}
return nil
return taskrun.ValidateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths)
}

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) {
// extractArrayIndexingRefsFromParams get all array indexing references from params
func extractArrayIndexingRefsFromParams(params []v1beta1.Param, arrayParams map[string]int, arrayIndexingParams *[]string) {
for i := range params {
findInvalidParamArrayReference(params[i].Value.StringVal, arrayParams, outofBoundParams)
taskrun.ExtractArrayIndexingParamRefs(params[i].Value.StringVal, arrayParams, arrayIndexingParams)
for _, v := range params[i].Value.ArrayVal {
findInvalidParamArrayReference(v, arrayParams, outofBoundParams)
taskrun.ExtractArrayIndexingParamRefs(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.ExtractArrayIndexingParamRefs(v, arrayParams, arrayIndexingParams)
}
}
}
39 changes: 26 additions & 13 deletions pkg/reconciler/pipelinerun/resources/validate_params_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -492,12 +492,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) {
tt := tt // capture range variable
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
run := &v1beta1.PipelineRun{
Spec: v1beta1.PipelineRunSpec{
Params: tt.params,
},
}
err := ValidateParamArrayIndex(ctx, &tt.original, run)
err := ValidateParamArrayIndex(ctx, &tt.original, tt.params)
if err != nil {
t.Errorf("ValidateParamArrayIndex() got err %s", err)
}
Expand Down Expand Up @@ -647,10 +642,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{
Expand Down Expand Up @@ -698,17 +698,30 @@ 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
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
run := &v1beta1.PipelineRun{
Spec: v1beta1.PipelineRunSpec{
Params: tt.params,
},
}
err := ValidateParamArrayIndex(ctx, &tt.original, run)
err := ValidateParamArrayIndex(ctx, &tt.original, tt.params)
if d := cmp.Diff(tt.expected.Error(), err.Error()); d != "" {
t.Errorf("ValidateParamArrayIndex() errors diff %s", diff.PrintWantGot(d))
}
Expand Down
Loading

0 comments on commit d03fc5e

Please sign in to comment.