From 6ad369368b5f622dd49e6d8635db7b0b0f5f401d Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Tue, 5 Dec 2023 21:54:21 +0000 Subject: [PATCH] change ResultRef.ResultsIndex from int to *int This commit closes 7392. When we introduced array results, we added validation funciton to check if the result reference is out of the array bound, in the cases of refercing a whole array and that array is empty, the resolved array index is 0, so the validation will error. Since the resolved index is only used when array indexing references exist, it is an optional field for ResultRef so we should change it to a pointer. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- examples/v1/pipelineruns/7392-regression.yaml | 51 +++++++++++++++++++ pkg/apis/pipeline/v1/openapi_generated.go | 5 +- pkg/apis/pipeline/v1/resultref.go | 23 +++++---- pkg/apis/pipeline/v1/resultref_test.go | 3 +- pkg/apis/pipeline/v1/swagger.json | 3 +- .../pipeline/v1beta1/openapi_generated.go | 5 +- pkg/apis/pipeline/v1beta1/resultref.go | 24 +++++---- pkg/apis/pipeline/v1beta1/resultref_test.go | 3 +- pkg/apis/pipeline/v1beta1/swagger.json | 3 +- .../resources/resultrefresolution.go | 4 +- .../resources/resultrefresolution_test.go | 38 +++++++++----- 11 files changed, 115 insertions(+), 47 deletions(-) create mode 100644 examples/v1/pipelineruns/7392-regression.yaml diff --git a/examples/v1/pipelineruns/7392-regression.yaml b/examples/v1/pipelineruns/7392-regression.yaml new file mode 100644 index 00000000000..c15369ffdc0 --- /dev/null +++ b/examples/v1/pipelineruns/7392-regression.yaml @@ -0,0 +1,51 @@ +apiVersion: tekton.dev/v1 +kind: Task +metadata: + name: task-with-array-result +spec: + params: + - name: arrayVal1 + description: "description3" + type: array + default: [] + steps: + - name: step1 + image: bash:latest + args: + - --images + - $(params.arrayVal1[*]) + script: | + #!/usr/bin/env bash + for arg in "$@"; do + echo $arg + done + - name: step2 + image: bash:latest + script: | + #!/usr/bin/env bash + echo -n '[]' | tee $(results.resultArray.path) + results: + - name: resultArray + description: "description4" + type: array + +--- +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + name: pipelinerun-array-result +spec: + pipelineSpec: + tasks: + - name: task-1 + taskRef: + name: task-with-array-result + params: + - name: arrayVal1 + value: [] + - name: task-2 + taskRef: + name: task-with-array-result + params: + - name: arrayVal1 + value: $(tasks.task-1.results.resultArray) diff --git a/pkg/apis/pipeline/v1/openapi_generated.go b/pkg/apis/pipeline/v1/openapi_generated.go index fe5d925166f..f3de88d9659 100644 --- a/pkg/apis/pipeline/v1/openapi_generated.go +++ b/pkg/apis/pipeline/v1/openapi_generated.go @@ -2328,9 +2328,8 @@ func schema_pkg_apis_pipeline_v1_ResultRef(ref common.ReferenceCallback) common. }, "resultsIndex": { SchemaProps: spec.SchemaProps{ - Default: 0, - Type: []string{"integer"}, - Format: "int32", + Type: []string{"integer"}, + Format: "int32", }, }, "property": { diff --git a/pkg/apis/pipeline/v1/resultref.go b/pkg/apis/pipeline/v1/resultref.go index 2de6bb80804..a2db1ab02a2 100644 --- a/pkg/apis/pipeline/v1/resultref.go +++ b/pkg/apis/pipeline/v1/resultref.go @@ -27,7 +27,7 @@ import ( type ResultRef struct { PipelineTask string `json:"pipelineTask"` Result string `json:"result"` - ResultsIndex int `json:"resultsIndex"` + ResultsIndex *int `json:"resultsIndex"` Property string `json:"property"` } @@ -124,35 +124,38 @@ func stripVarSubExpression(expression string) string { // parseExpression parses "task name", "result name", "array index" (iff it's an array result) and "object key name" (iff it's an object result) // Valid Example 1: // - Input: tasks.myTask.results.aStringResult -// - Output: "myTask", "aStringResult", -1, "", nil +// - Output: "myTask", "aStringResult", nil, "", nil // Valid Example 2: // - Input: tasks.myTask.results.anObjectResult.key1 -// - Output: "myTask", "anObjectResult", 0, "key1", nil +// - Output: "myTask", "anObjectResult", nil, "key1", nil // Valid Example 3: // - Input: tasks.myTask.results.anArrayResult[1] // - Output: "myTask", "anArrayResult", 1, "", nil +// Valid Example 4: +// - Input: tasks.myTask.results.Result[*] +// - Output: "myTask", "Result", nil, "", nil // Invalid Example 1: // - Input: tasks.myTask.results.resultName.foo.bar -// - Output: "", "", 0, "", error +// - Output: "", "", nil, "", error // TODO: may use regex for each type to handle possible reference formats -func parseExpression(substitutionExpression string) (string, string, int, string, error) { +func parseExpression(substitutionExpression string) (string, string, *int, string, error) { if looksLikeResultRef(substitutionExpression) { subExpressions := strings.Split(substitutionExpression, ".") // For string result: tasks..results. // For array result: tasks..results.[index] if len(subExpressions) == 4 { resultName, stringIdx := ParseResultName(subExpressions[3]) - if stringIdx != "" { + if stringIdx != "" && stringIdx != "*" { intIdx, _ := strconv.Atoi(stringIdx) - return subExpressions[1], resultName, intIdx, "", nil + return subExpressions[1], resultName, &intIdx, "", nil } - return subExpressions[1], resultName, 0, "", nil + return subExpressions[1], resultName, nil, "", nil } else if len(subExpressions) == 5 { // For object type result: tasks..results.. - return subExpressions[1], subExpressions[3], 0, subExpressions[4], nil + return subExpressions[1], subExpressions[3], nil, subExpressions[4], nil } } - return "", "", 0, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat) + return "", "", nil, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat) } // ParseResultName parse the input string to extract resultName and result index. diff --git a/pkg/apis/pipeline/v1/resultref_test.go b/pkg/apis/pipeline/v1/resultref_test.go index 0ec0c37a79c..51f914a7170 100644 --- a/pkg/apis/pipeline/v1/resultref_test.go +++ b/pkg/apis/pipeline/v1/resultref_test.go @@ -28,6 +28,7 @@ import ( ) func TestNewResultReference(t *testing.T) { + idx := 1 for _, tt := range []struct { name string param v1.Param @@ -72,7 +73,7 @@ func TestNewResultReference(t *testing.T) { want: []*v1.ResultRef{{ PipelineTask: "sumTask", Result: "sumResult", - ResultsIndex: 1, + ResultsIndex: &idx, }}, }, { name: "Test valid expression with multiple object result properties", diff --git a/pkg/apis/pipeline/v1/swagger.json b/pkg/apis/pipeline/v1/swagger.json index 8093c7e8389..1264eb28913 100644 --- a/pkg/apis/pipeline/v1/swagger.json +++ b/pkg/apis/pipeline/v1/swagger.json @@ -1176,8 +1176,7 @@ }, "resultsIndex": { "type": "integer", - "format": "int32", - "default": 0 + "format": "int32" } } }, diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index d9eaff0ecc4..1646119cb51 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -3197,9 +3197,8 @@ func schema_pkg_apis_pipeline_v1beta1_ResultRef(ref common.ReferenceCallback) co }, "resultsIndex": { SchemaProps: spec.SchemaProps{ - Default: 0, - Type: []string{"integer"}, - Format: "int32", + Type: []string{"integer"}, + Format: "int32", }, }, "property": { diff --git a/pkg/apis/pipeline/v1beta1/resultref.go b/pkg/apis/pipeline/v1beta1/resultref.go index 43ad32036f1..ac4a06abbc8 100644 --- a/pkg/apis/pipeline/v1beta1/resultref.go +++ b/pkg/apis/pipeline/v1beta1/resultref.go @@ -27,7 +27,7 @@ import ( type ResultRef struct { PipelineTask string `json:"pipelineTask"` Result string `json:"result"` - ResultsIndex int `json:"resultsIndex"` + ResultsIndex *int `json:"resultsIndex"` Property string `json:"property"` } @@ -159,36 +159,38 @@ func stripVarSubExpression(expression string) string { // parseExpression parses "task name", "result name", "array index" (iff it's an array result) and "object key name" (iff it's an object result) // Valid Example 1: // - Input: tasks.myTask.results.aStringResult -// - Output: "myTask", "aStringResult", -1, "", nil +// - Output: "myTask", "aStringResult", nil, "", nil // Valid Example 2: // - Input: tasks.myTask.results.anObjectResult.key1 -// - Output: "myTask", "anObjectResult", 0, "key1", nil +// - Output: "myTask", "anObjectResult", nil, "key1", nil // Valid Example 3: // - Input: tasks.myTask.results.anArrayResult[1] // - Output: "myTask", "anArrayResult", 1, "", nil +// Valid Example 4: +// - Input: tasks.myTask.results.Result[*] +// - Output: "myTask", "Result", nil, "", nil // Invalid Example 1: // - Input: tasks.myTask.results.resultName.foo.bar -// - Output: "", "", 0, "", error +// - Output: "", "", nil, "", error // TODO: may use regex for each type to handle possible reference formats -func parseExpression(substitutionExpression string) (string, string, int, string, error) { +func parseExpression(substitutionExpression string) (string, string, *int, string, error) { if looksLikeResultRef(substitutionExpression) { subExpressions := strings.Split(substitutionExpression, ".") // For string result: tasks..results. // For array result: tasks..results.[index] if len(subExpressions) == 4 { resultName, stringIdx := ParseResultName(subExpressions[3]) - if stringIdx != "" { + if stringIdx != "" && stringIdx != "*" { intIdx, _ := strconv.Atoi(stringIdx) - return subExpressions[1], resultName, intIdx, "", nil + return subExpressions[1], resultName, &intIdx, "", nil } - return subExpressions[1], resultName, 0, "", nil + return subExpressions[1], resultName, nil, "", nil } else if len(subExpressions) == 5 { // For object type result: tasks..results.. - return subExpressions[1], subExpressions[3], 0, subExpressions[4], nil + return subExpressions[1], subExpressions[3], nil, subExpressions[4], nil } } - - return "", "", 0, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat) + return "", "", nil, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat) } // ParseResultName parse the input string to extract resultName and result index. diff --git a/pkg/apis/pipeline/v1beta1/resultref_test.go b/pkg/apis/pipeline/v1beta1/resultref_test.go index 42e881ed5b2..f13a9227397 100644 --- a/pkg/apis/pipeline/v1beta1/resultref_test.go +++ b/pkg/apis/pipeline/v1beta1/resultref_test.go @@ -27,6 +27,7 @@ import ( ) func TestNewResultReference(t *testing.T) { + idx1 := 1 for _, tt := range []struct { name string param v1beta1.Param @@ -71,7 +72,7 @@ func TestNewResultReference(t *testing.T) { want: []*v1beta1.ResultRef{{ PipelineTask: "sumTask", Result: "sumResult", - ResultsIndex: 1, + ResultsIndex: &idx1, }}, }, { name: "Test valid expression with multiple object result properties", diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index 63c4af3d15b..62be890e599 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -1780,8 +1780,7 @@ }, "resultsIndex": { "type": "integer", - "format": "int32", - "default": 0 + "format": "int32" } } }, diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go index 103d587101c..2172a8f8eae 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go @@ -70,8 +70,8 @@ func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunSta func validateArrayResultsIndex(allResolvedResultRefs ResolvedResultRefs) error { for _, r := range allResolvedResultRefs { if r.Value.Type == v1.ParamTypeArray { - if r.ResultReference.ResultsIndex >= len(r.Value.ArrayVal) { - return 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)) + if r.ResultReference.ResultsIndex != nil && *r.ResultReference.ResultsIndex >= len(r.Value.ArrayVal) { + return 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)) } } } diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go index 657805c7e23..ef18e506b13 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go @@ -41,6 +41,8 @@ var ( Type: apis.ConditionSucceeded, Status: corev1.ConditionFalse, } + idx1 = 1 + idx2 = 2 ) var pipelineRunState = PipelineRunState{{ @@ -375,7 +377,7 @@ func TestResolveResultRefs(t *testing.T) { ResultReference: v1.ResultRef{ PipelineTask: "cTask", Result: "cResult", - ResultsIndex: 1, + ResultsIndex: &idx1, }, FromTaskRun: "cTaskRun", }}, @@ -391,7 +393,6 @@ func TestResolveResultRefs(t *testing.T) { ResultReference: v1.ResultRef{ PipelineTask: "cTask", Result: "cResult", - ResultsIndex: 0, }, FromTaskRun: "cTaskRun", }, { @@ -399,7 +400,6 @@ func TestResolveResultRefs(t *testing.T) { ResultReference: v1.ResultRef{ PipelineTask: "dTask", Result: "dResult", - ResultsIndex: 0, }, FromTaskRun: "dTaskRun", }}, @@ -753,7 +753,21 @@ func TestValidateArrayResultsIndex(t *testing.T) { ResultReference: v1.ResultRef{ PipelineTask: "aTask", Result: "aResult", - ResultsIndex: 1, + ResultsIndex: nil, + }, + FromTaskRun: "aTaskRun", + }}, + }, { + name: "Reference an Empty Array", + refs: ResolvedResultRefs{{ + Value: v1.ResultValue{ + Type: "array", + ArrayVal: []string{}, + }, + ResultReference: v1.ResultRef{ + PipelineTask: "aTask", + Result: "aResult", + ResultsIndex: &idx1, }, FromTaskRun: "aTaskRun", }}, @@ -768,7 +782,7 @@ func TestValidateArrayResultsIndex(t *testing.T) { ResultReference: v1.ResultRef{ PipelineTask: "aTask", Result: "aResult", - ResultsIndex: 1, + ResultsIndex: &idx1, }, FromTaskRun: "aTaskRun", }}, @@ -778,16 +792,16 @@ func TestValidateArrayResultsIndex(t *testing.T) { refs: ResolvedResultRefs{{ Value: v1.ResultValue{ Type: "array", - ArrayVal: []string{"a", "b", "c"}, + ArrayVal: []string{"a", "b"}, }, ResultReference: v1.ResultRef{ PipelineTask: "aTask", Result: "aResult", - ResultsIndex: 3, + ResultsIndex: &idx2, }, FromTaskRun: "aTaskRun", }}, - wantErr: "array Result Index 3 for Task aTask Result aResult is out of bound of size 3", + wantErr: "array Result Index 2 for Task aTask Result aResult is out of bound of size 2", }, { name: "In Bounds and Out of Bounds Array", refs: ResolvedResultRefs{{ @@ -798,22 +812,22 @@ func TestValidateArrayResultsIndex(t *testing.T) { ResultReference: v1.ResultRef{ PipelineTask: "aTask", Result: "aResult", - ResultsIndex: 1, + ResultsIndex: &idx1, }, FromTaskRun: "aTaskRun", }, { Value: v1.ResultValue{ Type: "array", - ArrayVal: []string{"a", "b", "c"}, + ArrayVal: []string{"a", "b"}, }, ResultReference: v1.ResultRef{ PipelineTask: "aTask", Result: "aResult", - ResultsIndex: 3, + ResultsIndex: &idx2, }, FromTaskRun: "aTaskRun", }}, - wantErr: "array Result Index 3 for Task aTask Result aResult is out of bound of size 3", + wantErr: "array Result Index 2 for Task aTask Result aResult is out of bound of size 2", }} { t.Run(tt.name, func(t *testing.T) { err := validateArrayResultsIndex(tt.refs)