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 [email protected]
tektoncd · Dec 6, 2023 · 42f0847 · 42f0847
1 parent a179226
commit 42f0847
Show file tree

Hide file tree

Showing 13 changed files with 125 additions and 47 deletions.
diff --git a/examples/v1/pipelineruns/beta/7392-regression.yaml 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
diff --git 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.<taskName>.results.<stringResultName>
 		// For array result: tasks.<taskName>.results.<arrayResultName>[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.<taskName>.results.<objectResultName>.<individualAttribute>
-			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
@@ -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
@@ -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
diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go
diff --git 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.<taskName>.results.<stringResultName>
 		// For array result: tasks.<taskName>.results.<arrayResultName>[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.<taskName>.results.<objectResultName>.<individualAttribute>
-			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
@@ -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
@@ -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
diff --git 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
@@ -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,15 +393,13 @@ func TestResolveResultRefs(t *testing.T) {
 			ResultReference: v1.ResultRef{
 				PipelineTask: "cTask",
 				Result:       "cResult",
-				ResultsIndex: 0,
 			},
 			FromTaskRun: "cTaskRun",
 		}, {
 			Value: *v1.NewStructuredValues("arrayResultOne", "arrayResultTwo"),
 			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)