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 8, 2023 · f9737b2 · f9737b2
1 parent 7c040de
commit f9737b2
Show file tree

Hide file tree

Showing 13 changed files with 137 additions and 57 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"`
 }
 
@@ -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.<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
@@ -43,14 +44,15 @@ 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[*])"),
 		},
 		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
@@ -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"`
 }
 
@@ -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.<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
@@ -42,14 +43,15 @@ 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[*])"),
 		},
 		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
@@ -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))
 			}
 		}
 	}