Skip to content

Commit 214c202

Browse files
YongxuanzhangYongxuanzhang
committed
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]
1 parent f3e9fc1 commit 214c202

File tree

4 files changed

+143
-167
lines changed

4 files changed

+143
-167
lines changed

‎pkg/reconciler/pipelinerun/pipelinerun.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -507,7 +507,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
507507
}
508508

509509
// Ensure that the array reference is not out of bound
510-
if err := resources.ValidateParamArrayIndex(ctx, pipelineSpec, pr); err != nil {
510+
if err := resources.ValidateParamArrayIndex(ctx, pipelineSpec, pr.Spec.Params); err != nil {
511511
// This Run has failed, so we need to mark it as failed and stop reconciling it
512512
pr.Status.MarkFailed(ReasonObjectParameterMissKeys,
513513
"PipelineRun %s/%s parameters is missing object keys required by Pipeline %s/%s's parameters: %s",

‎pkg/reconciler/pipelinerun/resources/validate_params.go

+27-81
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,6 @@ import (
2424
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
2525
"github.com/tektoncd/pipeline/pkg/list"
2626
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun"
27-
"github.com/tektoncd/pipeline/pkg/substitution"
28-
"k8s.io/apimachinery/pkg/util/sets"
2927
)
3028

3129
// ValidateParamTypesMatching validate that parameters in PipelineRun override corresponding parameters in Pipeline of the same type.
@@ -90,108 +88,56 @@ func ValidateObjectParamRequiredKeys(pipelineParameters []v1beta1.ParamSpec, pip
9088
}
9189

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

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

100-
outofBoundParams := sets.String{}
101-
102-
// collect all the references
103-
for i := range p.Tasks {
104-
findInvalidParamArrayReferences(p.Tasks[i].Params, arrayParams, &outofBoundParams)
105-
if p.Tasks[i].IsMatrixed() {
106-
findInvalidParamArrayReferences(p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams)
100+
// extract all array indexing references, for example []{"$(params.array-params[1])"}
101+
arrayIndexParamRefs := []string{}
102+
for i := range pipelineSpec.Tasks {
103+
extractArrayIndexingRefsFromParams(pipelineSpec.Tasks[i].Params, &arrayIndexParamRefs)
104+
if pipelineSpec.Tasks[i].IsMatrixed() {
105+
extractArrayIndexingRefsFromParams(pipelineSpec.Tasks[i].Matrix.Params, &arrayIndexParamRefs)
107106
}
108-
for j := range p.Tasks[i].Workspaces {
109-
findInvalidParamArrayReference(p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams)
107+
for j := range pipelineSpec.Tasks[i].Workspaces {
108+
taskrun.ExtractArrayIndexingParamRefs(pipelineSpec.Tasks[i].Workspaces[j].SubPath, &arrayIndexParamRefs)
110109
}
111-
for _, wes := range p.Tasks[i].WhenExpressions {
112-
findInvalidParamArrayReference(wes.Input, arrayParams, &outofBoundParams)
110+
for _, wes := range pipelineSpec.Tasks[i].WhenExpressions {
111+
taskrun.ExtractArrayIndexingParamRefs(wes.Input, &arrayIndexParamRefs)
113112
for _, v := range wes.Values {
114-
findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
113+
taskrun.ExtractArrayIndexingParamRefs(v, &arrayIndexParamRefs)
115114
}
116115
}
117116
}
118117

119-
for i := range p.Finally {
120-
findInvalidParamArrayReferences(p.Finally[i].Params, arrayParams, &outofBoundParams)
121-
if p.Finally[i].IsMatrixed() {
122-
findInvalidParamArrayReferences(p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams)
118+
for i := range pipelineSpec.Finally {
119+
extractArrayIndexingRefsFromParams(pipelineSpec.Finally[i].Params, &arrayIndexParamRefs)
120+
if pipelineSpec.Finally[i].IsMatrixed() {
121+
extractArrayIndexingRefsFromParams(pipelineSpec.Finally[i].Matrix.Params, &arrayIndexParamRefs)
123122
}
124-
for _, wes := range p.Finally[i].WhenExpressions {
123+
for _, wes := range pipelineSpec.Finally[i].WhenExpressions {
125124
for _, v := range wes.Values {
126-
findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
125+
taskrun.ExtractArrayIndexingParamRefs(v, &arrayIndexParamRefs)
127126
}
128127
}
129128
}
130-
131-
if outofBoundParams.Len() > 0 {
132-
return fmt.Errorf("non-existent param references:%v", outofBoundParams.List())
133-
}
134-
return nil
129+
return taskrun.ValidateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths)
135130
}
136131

137-
func extractParamIndexes(defaults []v1beta1.ParamSpec, params []v1beta1.Param) map[string]int {
138-
// Collect all array params
139-
arrayParams := make(map[string]int)
140-
141-
patterns := []string{
142-
"$(params.%s)",
143-
"$(params[%q])",
144-
"$(params['%s'])",
145-
}
146-
147-
// Collect array params lengths from defaults
148-
for _, p := range defaults {
149-
if p.Default != nil {
150-
if p.Default.Type == v1beta1.ParamTypeArray {
151-
for _, pattern := range patterns {
152-
for i := 0; i < len(p.Default.ArrayVal); i++ {
153-
arrayParams[fmt.Sprintf(pattern, p.Name)] = len(p.Default.ArrayVal)
154-
}
155-
}
156-
}
157-
}
158-
}
159-
160-
// Collect array params lengths from pipelinerun or taskrun
161-
for _, p := range params {
162-
if p.Value.Type == v1beta1.ParamTypeArray {
163-
for _, pattern := range patterns {
164-
for i := 0; i < len(p.Value.ArrayVal); i++ {
165-
arrayParams[fmt.Sprintf(pattern, p.Name)] = len(p.Value.ArrayVal)
166-
}
167-
}
168-
}
169-
}
170-
return arrayParams
171-
}
172-
173-
func findInvalidParamArrayReferences(params []v1beta1.Param, arrayParams map[string]int, outofBoundParams *sets.String) {
132+
// extractArrayIndexingRefsFromParams get all array indexing references from params
133+
func extractArrayIndexingRefsFromParams(params []v1beta1.Param, arrayIndexingParams *[]string) {
174134
for i := range params {
175-
findInvalidParamArrayReference(params[i].Value.StringVal, arrayParams, outofBoundParams)
135+
taskrun.ExtractArrayIndexingParamRefs(params[i].Value.StringVal, arrayIndexingParams)
176136
for _, v := range params[i].Value.ArrayVal {
177-
findInvalidParamArrayReference(v, arrayParams, outofBoundParams)
137+
taskrun.ExtractArrayIndexingParamRefs(v, arrayIndexingParams)
178138
}
179139
for _, v := range params[i].Value.ObjectVal {
180-
findInvalidParamArrayReference(v, arrayParams, outofBoundParams)
181-
}
182-
}
183-
}
184-
185-
func findInvalidParamArrayReference(paramReference string, arrayParams map[string]int, outofBoundParams *sets.String) {
186-
list := substitution.ExtractParamsExpressions(paramReference)
187-
for _, val := range list {
188-
indexString := substitution.ExtractIndexString(paramReference)
189-
idx, _ := substitution.ExtractIndex(indexString)
190-
v := substitution.TrimArrayIndex(val)
191-
if paramLength, ok := arrayParams[v]; ok {
192-
if idx >= paramLength {
193-
outofBoundParams.Insert(val)
194-
}
140+
taskrun.ExtractArrayIndexingParamRefs(v, arrayIndexingParams)
195141
}
196142
}
197143
}

‎pkg/reconciler/pipelinerun/resources/validate_params_test.go

+26-13
Original file line numberDiff line numberDiff line change
@@ -492,12 +492,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) {
492492
tt := tt // capture range variable
493493
t.Run(tt.name, func(t *testing.T) {
494494
t.Parallel()
495-
run := &v1beta1.PipelineRun{
496-
Spec: v1beta1.PipelineRunSpec{
497-
Params: tt.params,
498-
},
499-
}
500-
err := ValidateParamArrayIndex(ctx, &tt.original, run)
495+
err := ValidateParamArrayIndex(ctx, &tt.original, tt.params)
501496
if err != nil {
502497
t.Errorf("ValidateParamArrayIndex() got err %s", err)
503498
}
@@ -647,10 +642,15 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) {
647642
Operator: selection.In,
648643
Values: []string{"$(params.second-param[2])"},
649644
}},
645+
Matrix: &v1beta1.Matrix{
646+
Params: []v1beta1.Param{
647+
{Name: "final-task-first-param", Value: *v1beta1.NewStructuredValues("$(params.first-param[4])")},
648+
{Name: "final-task-second-param", Value: *v1beta1.NewStructuredValues("$(params.second-param[4])")},
649+
}},
650650
}},
651651
},
652652
params: []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}},
653-
expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.first-param[3]) $(params.second-param[2]) $(params.second-param[3])]"),
653+
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])]"),
654654
}, {
655655
name: "parameter references with bracket notation and special characters reference out of bound",
656656
original: v1beta1.PipelineSpec{
@@ -698,17 +698,30 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) {
698698
},
699699
params: []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}},
700700
expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"),
701+
}, {
702+
name: "parameter in matrix reference out of bound",
703+
original: v1beta1.PipelineSpec{
704+
Params: []v1beta1.ParamSpec{
705+
{Name: "first-param", Type: v1beta1.ParamTypeArray, Default: v1beta1.NewStructuredValues("default-value", "default-value-again")},
706+
{Name: "second-param", Type: v1beta1.ParamTypeArray},
707+
},
708+
Tasks: []v1beta1.PipelineTask{{
709+
Matrix: &v1beta1.Matrix{
710+
Params: []v1beta1.Param{
711+
{Name: "first-task-first-param", Value: *v1beta1.NewStructuredValues("$(params.first-param[2])")},
712+
{Name: "first-task-second-param", Value: *v1beta1.NewStructuredValues("static value")},
713+
},
714+
},
715+
}},
716+
},
717+
params: []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}},
718+
expected: fmt.Errorf("non-existent param references:[$(params.first-param[2])]"),
701719
},
702720
} {
703721
tt := tt // capture range variable
704722
t.Run(tt.name, func(t *testing.T) {
705723
t.Parallel()
706-
run := &v1beta1.PipelineRun{
707-
Spec: v1beta1.PipelineRunSpec{
708-
Params: tt.params,
709-
},
710-
}
711-
err := ValidateParamArrayIndex(ctx, &tt.original, run)
724+
err := ValidateParamArrayIndex(ctx, &tt.original, tt.params)
712725
if d := cmp.Diff(tt.expected.Error(), err.Error()); d != "" {
713726
t.Errorf("ValidateParamArrayIndex() errors diff %s", diff.PrintWantGot(d))
714727
}

0 commit comments

Comments
 (0)