Skip to content

Commit 9286433

Browse files
YongxuanzhangYongxuanzhang
committed
add index validation
1 parent f41d7ea commit 9286433

File tree

8 files changed

+179
-15
lines changed

8 files changed

+179
-15
lines changed

‎pkg/apis/pipeline/v1beta1/openapi_generated.go‎

Lines changed: 8 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/apis/pipeline/v1beta1/resultref.go‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,15 @@ package v1beta1
1919
import (
2020
"fmt"
2121
"regexp"
22+
"strconv"
2223
"strings"
2324
)
2425

2526
// ResultRef is a type that represents a reference to a task run result
2627
type ResultRef struct {
2728
PipelineTask string `json:"pipelineTask"`
2829
Result string `json:"result"`
30+
ResultsIndex int `json:"resultsIndex"`
2931
}
3032

3133
const (
@@ -37,30 +39,31 @@ const (
3739
// TODO(#2462) use one regex across all substitutions
3840
// variableSubstitutionFormat matches format like $result.resultname, $result.resultname[int] and $result.resultname[*]
3941
variableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9])*\*?\])?\)`
40-
// excludeArrayIndexing will replace all `[int]` and `[*]` for parseExpression to extract result name
41-
excludeArrayIndexing = `\[([0-9])*\*?\]`
42+
// arrayIndexing will match all `[int]` and `[*]` for parseExpression
43+
arrayIndexing = `\[([0-9])*\*?\]`
4244
// ResultNameFormat Constant used to define the the regex Result.Name should follow
4345
ResultNameFormat = `^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$`
4446
)
4547

4648
var variableSubstitutionRegex = regexp.MustCompile(variableSubstitutionFormat)
4749
var resultNameFormatRegex = regexp.MustCompile(ResultNameFormat)
48-
var excludeArrayIndexingRegex = regexp.MustCompile(excludeArrayIndexing)
50+
var arrayIndexingRegex = regexp.MustCompile(arrayIndexing)
4951

5052
// NewResultRefs extracts all ResultReferences from a param or a pipeline result.
5153
// If the ResultReference can be extracted, they are returned. Expressions which are not
5254
// results are ignored.
5355
func NewResultRefs(expressions []string) []*ResultRef {
5456
var resultRefs []*ResultRef
5557
for _, expression := range expressions {
56-
pipelineTask, result, err := parseExpression(expression)
58+
pipelineTask, result, index, err := parseExpression(expression)
5759
// If the expression isn't a result but is some other expression,
5860
// parseExpression will return an error, in which case we just skip that expression,
5961
// since although it's not a result ref, it might be some other kind of reference
6062
if err == nil {
6163
resultRefs = append(resultRefs, &ResultRef{
6264
PipelineTask: pipelineTask,
6365
Result: result,
66+
ResultsIndex: index,
6467
})
6568
}
6669
}
@@ -126,13 +129,19 @@ func stripVarSubExpression(expression string) string {
126129
return strings.TrimSuffix(strings.TrimPrefix(expression, "$("), ")")
127130
}
128131

129-
func parseExpression(substitutionExpression string) (string, string, error) {
132+
func parseExpression(substitutionExpression string) (string, string, int, error) {
130133
subExpressions := strings.Split(substitutionExpression, ".")
131134
if len(subExpressions) != 4 || subExpressions[0] != ResultTaskPart || subExpressions[2] != ResultResultPart {
132-
return "", "", fmt.Errorf("Must be of the form %q", resultExpressionFormat)
135+
return "", "", 0, fmt.Errorf("Must be of the form %q", resultExpressionFormat)
133136
}
134-
subExpressions[3] = excludeArrayIndexingRegex.ReplaceAllString(subExpressions[3], "")
135-
return subExpressions[1], subExpressions[3], nil
137+
138+
stringIdx := strings.TrimSuffix(strings.TrimPrefix(arrayIndexingRegex.FindString(subExpressions[3]), "["), "]")
139+
subExpressions[3] = arrayIndexingRegex.ReplaceAllString(subExpressions[3], "")
140+
if stringIdx != "" {
141+
intIdx, _ := strconv.Atoi(stringIdx)
142+
return subExpressions[1], subExpressions[3], intIdx, nil
143+
}
144+
return subExpressions[1], subExpressions[3], 0, nil
136145
}
137146

138147
// PipelineTaskResultRefs walks all the places a result reference can be used

‎pkg/apis/pipeline/v1beta1/resultref_test.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,27 @@ func TestNewResultReference(t *testing.T) {
4141
PipelineTask: "sumTask",
4242
Result: "sumResult",
4343
}},
44+
},{
45+
name: "refer whole array result",
46+
param: v1beta1.Param{
47+
Name: "param",
48+
Value: *v1beta1.NewArrayOrString("$(tasks.sumTask.results.sumResult[*])"),
49+
},
50+
want: []*v1beta1.ResultRef{{
51+
PipelineTask: "sumTask",
52+
Result: "sumResult",
53+
}},
54+
},{
55+
name: "refer array indexing result",
56+
param: v1beta1.Param{
57+
Name: "param",
58+
Value: *v1beta1.NewArrayOrString("$(tasks.sumTask.results.sumResult[1])"),
59+
},
60+
want: []*v1beta1.ResultRef{{
61+
PipelineTask: "sumTask",
62+
Result: "sumResult",
63+
ResultsIndex: 1,
64+
}},
4465
}, {
4566
name: "substitution within string",
4667
param: v1beta1.Param{

‎pkg/apis/pipeline/v1beta1/swagger.json‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1711,7 +1711,8 @@
17111711
"type": "object",
17121712
"required": [
17131713
"pipelineTask",
1714-
"result"
1714+
"result",
1715+
"resultsIndex"
17151716
],
17161717
"properties": {
17171718
"pipelineTask": {
@@ -1721,6 +1722,11 @@
17211722
"result": {
17221723
"type": "string",
17231724
"default": ""
1725+
},
1726+
"resultsIndex": {
1727+
"type": "integer",
1728+
"format": "int32",
1729+
"default": 0
17241730
}
17251731
}
17261732
},

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

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,38 @@ func TestApplyTaskResults_MinimalExpression(t *testing.T) {
431431
}},
432432
},
433433
}},
434-
}, {
434+
}, {
435+
name: "Test array indexing result substitution out of bound - params",
436+
resolvedResultRefs: ResolvedResultRefs{{
437+
Value: *v1beta1.NewArrayOrString("arrayResultValueOne", "arrayResultValueTwo"),
438+
ResultReference: v1beta1.ResultRef{
439+
PipelineTask: "aTask",
440+
Result: "a.Result",
441+
},
442+
FromTaskRun: "aTaskRun",
443+
}},
444+
targets: PipelineRunState{{
445+
PipelineTask: &v1beta1.PipelineTask{
446+
Name: "bTask",
447+
TaskRef: &v1beta1.TaskRef{Name: "bTask"},
448+
Params: []v1beta1.Param{{
449+
Name: "bParam",
450+
Value: *v1beta1.NewArrayOrString(`$(tasks.aTask.results["a.Result"][3])`),
451+
}},
452+
},
453+
}},
454+
want: PipelineRunState{{
455+
PipelineTask: &v1beta1.PipelineTask{
456+
Name: "bTask",
457+
TaskRef: &v1beta1.TaskRef{Name: "bTask"},
458+
Params: []v1beta1.Param{{
459+
Name: "bParam",
460+
// index validation is done in ResolveResultRefs() before ApplyTaskResults()
461+
Value: *v1beta1.NewArrayOrString(`$(tasks.aTask.results["a.Result"][3])`),
462+
}},
463+
},
464+
}},
465+
},{
435466
name: "Test result substitution on minimal variable substitution expression - when expressions",
436467
resolvedResultRefs: ResolvedResultRefs{{
437468
Value: *v1beta1.NewArrayOrString("aResultValue"),

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -306,8 +306,10 @@ func (t *ResolvedPipelineRunTask) skipBecauseResultReferencesAreMissing(facts *P
306306
if t.checkParentsDone(facts) && t.hasResultReferences() {
307307
resolvedResultRefs, pt, err := ResolveResultRefs(facts.State, PipelineRunState{t})
308308
rprt := facts.State.ToMap()[pt]
309-
if err != nil && (t.IsFinalTask(facts) || rprt.Skip(facts).SkippingReason == v1beta1.WhenExpressionsSkip) {
310-
return true
309+
if rprt != nil {
310+
if err != nil && (t.IsFinalTask(facts) || rprt.Skip(facts).SkippingReason == v1beta1.WhenExpressionsSkip) {
311+
return true
312+
}
311313
}
312314
ApplyTaskResults(PipelineRunState{t}, resolvedResultRefs)
313315
facts.ResetSkippedCache()

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

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ func ResolveResultRef(pipelineRunState PipelineRunState, target *ResolvedPipelin
4343
if err != nil {
4444
return nil, pt, err
4545
}
46-
return removeDup(resolvedResultRefs), "", nil
46+
return validateArrayResultsIndex(removeDup(resolvedResultRefs))
4747
}
4848

4949
// ResolveResultRefs resolves any ResultReference that are found in the target ResolvedPipelineRunTask
@@ -56,7 +56,19 @@ func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunSta
5656
}
5757
allResolvedResultRefs = append(allResolvedResultRefs, resolvedResultRefs...)
5858
}
59-
return removeDup(allResolvedResultRefs), "", nil
59+
return validateArrayResultsIndex(removeDup(allResolvedResultRefs))
60+
}
61+
62+
// validateArrayResultsIndex checks if the result array indexing reference is out of bound of the array size
63+
func validateArrayResultsIndex(allResolvedResultRefs ResolvedResultRefs) (ResolvedResultRefs, string, error) {
64+
for _, r := range allResolvedResultRefs {
65+
if r.Value.Type == v1beta1.ParamTypeArray {
66+
if r.ResultReference.ResultsIndex >= len(r.Value.ArrayVal) {
67+
return nil, "", fmt.Errorf("Array Result Index %d for Task %s Result %s is out of bound of size %d", r.ResultReference.ResultsIndex, r.ResultReference.PipelineTask, r.ResultReference.Result, len(r.Value.ArrayVal))
68+
}
69+
}
70+
}
71+
return allResolvedResultRefs, "", nil
6072
}
6173

6274
// extractResultRefs resolves any ResultReference that are found in param or pipeline result

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

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,58 @@ var pipelineRunState = PipelineRunState{{
120120
Value: *v1beta1.NewArrayOrString("$(tasks.aCustomPipelineTask.results.aResult)"),
121121
}},
122122
},
123+
}, {
124+
TaskRunName: "cTaskRun",
125+
TaskRun: &v1beta1.TaskRun{
126+
ObjectMeta: metav1.ObjectMeta{
127+
Name: "cTaskRun",
128+
},
129+
Status: v1beta1.TaskRunStatus{
130+
Status: duckv1beta1.Status{
131+
Conditions: duckv1beta1.Conditions{successCondition},
132+
},
133+
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
134+
TaskRunResults: []v1beta1.TaskRunResult{{
135+
Name: "cResult",
136+
Value: *v1beta1.NewArrayOrString("arrayResultOne", "arrayResultTwo"),
137+
}},
138+
},
139+
},
140+
},
141+
PipelineTask: &v1beta1.PipelineTask{
142+
Name: "cTask",
143+
TaskRef: &v1beta1.TaskRef{Name: "cTask"},
144+
Params: []v1beta1.Param{{
145+
Name: "cParam",
146+
Value: *v1beta1.NewArrayOrString("$(tasks.cTask.results.cResult[1])"),
147+
}},
148+
},
149+
}, {
150+
TaskRunName: "dTaskRun",
151+
TaskRun: &v1beta1.TaskRun{
152+
ObjectMeta: metav1.ObjectMeta{
153+
Name: "dTaskRun",
154+
},
155+
Status: v1beta1.TaskRunStatus{
156+
Status: duckv1beta1.Status{
157+
Conditions: duckv1beta1.Conditions{successCondition},
158+
},
159+
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
160+
TaskRunResults: []v1beta1.TaskRunResult{{
161+
Name: "dResult",
162+
Value: *v1beta1.NewArrayOrString("arrayResultOne", "arrayResultTwo"),
163+
}},
164+
},
165+
},
166+
},
167+
PipelineTask: &v1beta1.PipelineTask{
168+
Name: "dTask",
169+
TaskRef: &v1beta1.TaskRef{Name: "dTask"},
170+
Params: []v1beta1.Param{{
171+
Name: "dParam",
172+
Value: *v1beta1.NewArrayOrString("$(tasks.dTask.results.dResult[3])"),
173+
}},
174+
},
123175
}}
124176

125177
func TestTaskParamResolver_ResolveResultRefs(t *testing.T) {
@@ -479,6 +531,30 @@ func TestResolveResultRefs(t *testing.T) {
479531
FromTaskRun: "aTaskRun",
480532
}},
481533
wantErr: false,
534+
}, {
535+
name: "Test successful array result references resolution - params",
536+
pipelineRunState: pipelineRunState,
537+
targets: PipelineRunState{
538+
pipelineRunState[7],
539+
},
540+
want: ResolvedResultRefs{{
541+
Value: *v1beta1.NewArrayOrString("arrayResultOne", "arrayResultTwo"),
542+
ResultReference: v1beta1.ResultRef{
543+
PipelineTask: "cTask",
544+
Result: "cResult",
545+
ResultsIndex: 1,
546+
},
547+
FromTaskRun: "cTaskRun",
548+
}},
549+
wantErr: false,
550+
}, {
551+
name: "Test unsuccessful result references resolution - params",
552+
pipelineRunState: pipelineRunState,
553+
targets: PipelineRunState{
554+
pipelineRunState[8],
555+
},
556+
want: nil,
557+
wantErr: true,
482558
}, {
483559
name: "Test successful result references resolution - when expressions",
484560
pipelineRunState: pipelineRunState,

0 commit comments

Comments
 (0)