From f9737b259f54484428a52003bd8794ddae7a3d9a 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 --- .../v1/pipelineruns/beta/7392-regression.yaml | 51 +++++++++++++++++++ pkg/apis/pipeline/v1/openapi_generated.go | 5 +- pkg/apis/pipeline/v1/resultref.go | 31 ++++++----- pkg/apis/pipeline/v1/resultref_test.go | 6 ++- pkg/apis/pipeline/v1/swagger.json | 3 +- pkg/apis/pipeline/v1/zz_generated.deepcopy.go | 5 ++ .../pipeline/v1beta1/openapi_generated.go | 5 +- pkg/apis/pipeline/v1beta1/resultref.go | 32 ++++++------ pkg/apis/pipeline/v1beta1/resultref_test.go | 6 ++- pkg/apis/pipeline/v1beta1/swagger.json | 3 +- .../pipeline/v1beta1/zz_generated.deepcopy.go | 5 ++ .../resources/resultrefresolution.go | 4 +- .../resources/resultrefresolution_test.go | 38 +++++++++----- 13 files changed, 137 insertions(+), 57 deletions(-) create mode 100644 examples/v1/pipelineruns/beta/7392-regression.yaml diff --git a/examples/v1/pipelineruns/beta/7392-regression.yaml b/examples/v1/pipelineruns/beta/7392-regression.yaml new file mode 100644 index 00000000000..c15369ffdc0 --- /dev/null +++ b/examples/v1/pipelineruns/beta/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 06f71333bd5..c6a73d7afc3 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..3948581f6d6 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"` } @@ -122,37 +122,40 @@ 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: +// 1. Reference string result // - Input: tasks.myTask.results.aStringResult -// - Output: "myTask", "aStringResult", -1, "", nil -// Valid Example 2: +// - Output: "myTask", "aStringResult", nil, "", nil +// 2. Reference Object value with key: // - Input: tasks.myTask.results.anObjectResult.key1 -// - Output: "myTask", "anObjectResult", 0, "key1", nil -// Valid Example 3: +// - Output: "myTask", "anObjectResult", nil, "key1", nil +// 3. Reference array elements with array indexing : // - Input: tasks.myTask.results.anArrayResult[1] // - Output: "myTask", "anArrayResult", 1, "", nil -// Invalid Example 1: +// 4. Referencing whole array or object result: +// - Input: tasks.myTask.results.Result[*] +// - Output: "myTask", "Result", nil, "", nil +// Invalid Case: // - 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..22f0dba5f3d 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 @@ -43,7 +44,7 @@ func TestNewResultReference(t *testing.T) { Result: "sumResult", }}, }, { - name: "refer whole array result", + name: "reference whole array result", param: v1.Param{ Name: "param", Value: *v1.NewStructuredValues("$(tasks.sumTask.results.sumResult[*])"), @@ -51,6 +52,7 @@ func TestNewResultReference(t *testing.T) { want: []*v1.ResultRef{{ PipelineTask: "sumTask", Result: "sumResult", + ResultsIndex: nil, }}, }, { name: "Test valid expression with single object result property", @@ -72,7 +74,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 cfae4b0e679..1424609e3b6 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/v1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1/zz_generated.deepcopy.go index 40fe4ba804d..db26c7d4c5e 100644 --- a/pkg/apis/pipeline/v1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1/zz_generated.deepcopy.go @@ -1079,6 +1079,11 @@ func (in *ResolverRef) DeepCopy() *ResolverRef { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ResultRef) DeepCopyInto(out *ResultRef) { *out = *in + if in.ResultsIndex != nil { + in, out := &in.ResultsIndex, &out.ResultsIndex + *out = new(int) + **out = **in + } return } diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index 3bd5ad6f1bf..c55315ac140 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..7c99dc0c034 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"` } @@ -157,38 +157,40 @@ 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: +// 1. Reference string result // - Input: tasks.myTask.results.aStringResult -// - Output: "myTask", "aStringResult", -1, "", nil -// Valid Example 2: +// - Output: "myTask", "aStringResult", nil, "", nil +// 2. Reference Object value with key: // - Input: tasks.myTask.results.anObjectResult.key1 -// - Output: "myTask", "anObjectResult", 0, "key1", nil -// Valid Example 3: +// - Output: "myTask", "anObjectResult", nil, "key1", nil +// 3. Reference array elements with array indexing : // - Input: tasks.myTask.results.anArrayResult[1] // - Output: "myTask", "anArrayResult", 1, "", nil -// Invalid Example 1: +// 4. Referencing whole array or object result: +// - Input: tasks.myTask.results.Result[*] +// - Output: "myTask", "Result", nil, "", nil +// Invalid Case: // - 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..6e775582733 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 @@ -42,7 +43,7 @@ func TestNewResultReference(t *testing.T) { Result: "sumResult", }}, }, { - name: "refer whole array result", + name: "reference whole array/object result", param: v1beta1.Param{ Name: "param", Value: *v1beta1.NewStructuredValues("$(tasks.sumTask.results.sumResult[*])"), @@ -50,6 +51,7 @@ func TestNewResultReference(t *testing.T) { want: []*v1beta1.ResultRef{{ PipelineTask: "sumTask", Result: "sumResult", + ResultsIndex: nil, }}, }, { name: "Test valid expression with single object result property", @@ -71,7 +73,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 671e6eba73d..b8624cf62ce 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/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index 2dd4fd8edb2..68cce2d2643 100644 --- a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go @@ -1527,6 +1527,11 @@ func (in *ResolverRef) DeepCopy() *ResolverRef { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ResultRef) DeepCopyInto(out *ResultRef) { *out = *in + if in.ResultsIndex != nil { + in, out := &in.ResultsIndex, &out.ResultsIndex + *out = new(int) + **out = **in + } return } 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)