From 6c3c47e7cd6ed47a43aad7192aa5839920268dda Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Wed, 6 Jul 2022 15:32:45 +0000 Subject: [PATCH] [TEP-0075]Validate Pipeline results array index This is part of work in TEP-0075. Previous to this commit, we have added support for pipeline array results indexing. This commit adds the validation to check if it is out of bound. --- docs/pipelines.md | 8 ++++ .../alpha/pipeline-emitting-results.yaml | 4 ++ pkg/apis/pipeline/v1beta1/resultref.go | 3 ++ pkg/apis/pipeline/v1beta1/resultref_test.go | 33 ++++++++------ pkg/reconciler/pipelinerun/resources/apply.go | 3 ++ .../pipelinerun/resources/apply_test.go | 43 +++++++++++++++++++ 6 files changed, 81 insertions(+), 13 deletions(-) diff --git a/docs/pipelines.md b/docs/pipelines.md index d315d5aff3a..26679c29d71 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -1005,6 +1005,14 @@ Array and object results is supported as alpha feature, see [`Array Results` in ```yaml results: + - name: array-results + type: array + description: whole array + value: $(tasks.task1.results.array-results[*]) + - name: array-indexing-results + type: string + description: array element + value: $(tasks.task1.results.array-results[1]) - name: object-results type: object description: whole object diff --git a/examples/v1beta1/pipelineruns/alpha/pipeline-emitting-results.yaml b/examples/v1beta1/pipelineruns/alpha/pipeline-emitting-results.yaml index 10baa456f38..4bb2b8d9002 100644 --- a/examples/v1beta1/pipelineruns/alpha/pipeline-emitting-results.yaml +++ b/examples/v1beta1/pipelineruns/alpha/pipeline-emitting-results.yaml @@ -41,6 +41,10 @@ spec: type: array description: whole array value: $(tasks.task1.results.array-results[*]) + - name: array-results-from-array-indexing-and-object-elements + type: array + description: whole array + value: ["$(tasks.task1.results.array-results[0])", "$(tasks.task2.results.object-results.foo)"] - name: array-indexing-results type: string description: array element diff --git a/pkg/apis/pipeline/v1beta1/resultref.go b/pkg/apis/pipeline/v1beta1/resultref.go index 6add8177d71..4eb8fa322a1 100644 --- a/pkg/apis/pipeline/v1beta1/resultref.go +++ b/pkg/apis/pipeline/v1beta1/resultref.go @@ -128,6 +128,9 @@ func GetVarSubstitutionExpressionsForParam(param Param) ([]string, bool) { // GetVarSubstitutionExpressionsForPipelineResult extracts all the value between "$(" and ")"" for a pipeline result func GetVarSubstitutionExpressionsForPipelineResult(result PipelineResult) ([]string, bool) { allExpressions := validateString(result.Value.StringVal) + for _, v := range result.Value.ArrayVal { + allExpressions = append(allExpressions, validateString(v)...) + } for _, v := range result.Value.ObjectVal { allExpressions = append(allExpressions, validateString(v)...) } diff --git a/pkg/apis/pipeline/v1beta1/resultref_test.go b/pkg/apis/pipeline/v1beta1/resultref_test.go index 10b7cd73aa0..fd2aa2959e5 100644 --- a/pkg/apis/pipeline/v1beta1/resultref_test.go +++ b/pkg/apis/pipeline/v1beta1/resultref_test.go @@ -732,20 +732,27 @@ func TestGetVarSubstitutionExpressionsForPipelineResult(t *testing.T) { Value: *v1beta1.NewArrayOrString("$(tasks.task1.results.result1) and $(tasks.task2.results.result2)"), }, want: []string{"tasks.task1.results.result1", "tasks.task2.results.result2"}, - }, - { - name: "get object result expressions", - result: v1beta1.PipelineResult{ - Name: "object result", - Type: v1beta1.ResultsTypeString, - Value: *v1beta1.NewObject(map[string]string{ - "key1": "$(tasks.task1.results.result1)", - "key2": "$(tasks.task2.results.result2) and another one $(tasks.task3.results.result3)", - "key3": "no ref here", - }), - }, - want: []string{"tasks.task1.results.result1", "tasks.task2.results.result2", "tasks.task3.results.result3"}, + }, { + name: "get array result expressions", + result: v1beta1.PipelineResult{ + Name: "array result", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewArrayOrString("$(tasks.task1.results.result1)", "$(tasks.task2.results.result2)"), + }, + want: []string{"tasks.task1.results.result1", "tasks.task2.results.result2"}, + }, { + name: "get object result expressions", + result: v1beta1.PipelineResult{ + Name: "object result", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewObject(map[string]string{ + "key1": "$(tasks.task1.results.result1)", + "key2": "$(tasks.task2.results.result2) and another one $(tasks.task3.results.result3)", + "key3": "no ref here", + }), }, + want: []string{"tasks.task1.results.result1", "tasks.task2.results.result2", "tasks.task3.results.result3"}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index b04e04f266a..4ef695a16ca 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -303,6 +303,9 @@ func ApplyTaskResultsToPipelineResults( intIdx, _ := strconv.Atoi(stringIdx) if intIdx < len(resultValue.ArrayVal) { stringReplacements[variable] = resultValue.ArrayVal[intIdx] + } else { + invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name) + validPipelineResult = false } } else { arrayReplacements[substitution.StripStarVarSubExpression(variable)] = resultValue.ArrayVal diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index 8946f3ba993..e7ae384ea83 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -2079,6 +2079,22 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { }, }, expectedResults: nil, + }, { + description: "array-index-out-of-bound", + results: []v1beta1.PipelineResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewArrayOrString("$(tasks.pt1.results.foo[4])"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{ + "pt1": { + { + Name: "foo", + Value: *v1beta1.NewArrayOrString("do", "rae", "mi"), + }, + }, + }, + expectedResults: nil, + expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), }, { description: "object-reference-not-exist", results: []v1beta1.PipelineResult{{ @@ -2191,6 +2207,33 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { "pkey2": "rae", }), }}, + }, { + description: "array-results-from-array-indexing-and-object-element", + results: []v1beta1.PipelineResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewArrayOrString("$(tasks.pt1.results.foo.key1)", "$(tasks.pt2.results.bar[1])"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{ + "pt1": { + { + Name: "foo", + Value: *v1beta1.NewObject(map[string]string{ + "key1": "val1", + "key2": "val2", + }), + }, + }, + "pt2": { + { + Name: "bar", + Value: *v1beta1.NewArrayOrString("do", "rae", "mi"), + }, + }, + }, + expectedResults: []v1beta1.PipelineRunResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewArrayOrString("val1", "rae"), + }}, }, { description: "apply-object-element", results: []v1beta1.PipelineResult{{