add feature gate check for param array indexing

Before this commit, if alpha feature gate is not enabled, the array indexing params will not be added to string replacements, thus will lead to non-existent variable error. This is confusing to users and doesn't provide correct guidance. This commit adds the check to the array indexing validation. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
tektoncd · Feb 13, 2023 · c4b0824 · c4b0824
1 parent 1e45bed
commit c4b0824
Show file tree

Hide file tree

Showing 6 changed files with 276 additions and 100 deletions.
diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go
@@ -94,6 +94,9 @@ const (
 	// ReasonObjectParameterMissKeys indicates that the object param value provided from PipelineRun spec
 	// misses some keys required for the object param declared in Pipeline spec.
 	ReasonObjectParameterMissKeys = "ObjectParameterMissKeys"
+	// ReasonParamArrayIndexingInvalid indicates that the use of param array indexing is not under correct api fields feature gate
+	// or the array is out of bound.
+	ReasonParamArrayIndexingInvalid = "ParamArrayIndexingInvalid"
 	// ReasonCouldntGetTask indicates that the reason for the failure status is that the
 	// associated Pipeline's Tasks couldn't all be retrieved
 	ReasonCouldntGetTask = "CouldntGetTask"
@@ -509,8 +512,8 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
 	// Ensure that the array reference is not out of bound
 	if err := resources.ValidateParamArrayIndex(ctx, pipelineSpec, pr); err != nil {
 		// This Run has failed, so we need to mark it as failed and stop reconciling it
-		pr.Status.MarkFailed(ReasonObjectParameterMissKeys,
-			"PipelineRun %s/%s parameters is missing object keys required by Pipeline %s/%s's parameters: %s",
+		pr.Status.MarkFailed(ReasonParamArrayIndexingInvalid,
+			"PipelineRun %s/%s array indexing params fail validation by Pipeline %s/%s's parameters: %s",
 			pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err)
 		return controller.NewPermanentError(err)
 	}

diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go
@@ -1061,6 +1061,22 @@ spec:
         name: a-task-that-needs-array-params
 `, v1beta1.ParamTypeArray)),
 		parse.MustParseV1beta1Pipeline(t, fmt.Sprintf(`
+metadata:
+  name: a-pipeline-with-array-indexing-params
+  namespace: foo
+spec:
+  params:
+    - name: some-param
+      type: %s
+  tasks:
+    - name: some-task
+      taskRef:
+        name: a-task-that-needs-array-params
+      params:
+        - name: param
+          value: "$(params.some-param[2])"
+`, v1beta1.ParamTypeArray)),
+		parse.MustParseV1beta1Pipeline(t, fmt.Sprintf(`
 metadata:
   name: a-pipeline-with-object-params
   namespace: foo
@@ -1225,6 +1241,27 @@ spec:
 			"Normal Started",
 			"Warning Failed PipelineRun foo/pipeline-missing-object-param-keys parameters is missing object keys required by Pipeline foo/a-pipeline-with-object-params's parameters: PipelineRun missing object keys for parameters",
 		},
+	}, {
+		name: "invalid-pipeline-array-index-out-of-bound",
+		pipelineRun: parse.MustParseV1beta1PipelineRun(t, `
+metadata:
+  name: pipeline-param-array-out-of-bound
+  namespace: foo
+spec:
+  pipelineRef:
+    name: a-pipeline-with-array-indexing-params
+  params:
+    - name: some-param
+      value:
+        - "a"
+        - "b"
+`),
+		reason:         ReasonParamArrayIndexingInvalid,
+		permanentError: true,
+		wantEvents: []string{
+			"Normal Started",
+			"Warning Failed PipelineRun foo/pipeline-param-array-out-of-bound array indexing params fail validation by Pipeline foo/a-pipeline-with-array-indexing-params's parameters: non-existent param references:[$(params.some-param[2]",
+		},
 	}, {
 		name: "invalid-embedded-pipeline-resources-bot-bound-shd-stop-reconciling",
 		pipelineRun: parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(`

diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go
@@ -20,11 +20,9 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/tektoncd/pipeline/pkg/apis/config"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/list"
 	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun"
-	"github.com/tektoncd/pipeline/pkg/substitution"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
@@ -91,40 +89,51 @@ func ValidateObjectParamRequiredKeys(pipelineParameters []v1beta1.ParamSpec, pip
 
 // ValidateParamArrayIndex validate if the array indexing param reference  target is existent
 func ValidateParamArrayIndex(ctx context.Context, p *v1beta1.PipelineSpec, pr *v1beta1.PipelineRun) error {
-	cfg := config.FromContextOrDefaults(ctx)
-	if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields {
-		return nil
-	}
-
 	arrayParams := extractParamIndexes(p.Params, pr.Spec.Params)
 
 	outofBoundParams := sets.String{}
 
 	// collect all the references
 	for i := range p.Tasks {
-		findInvalidParamArrayReferences(p.Tasks[i].Params, arrayParams, &outofBoundParams)
+		if err := findInvalidParamArrayReferences(ctx, p.Tasks[i].Params, arrayParams, &outofBoundParams); err != nil {
+			return err
+		}
 		if p.Tasks[i].IsMatrixed() {
-			findInvalidParamArrayReferences(p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams)
+			if err := findInvalidParamArrayReferences(ctx, p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams); err != nil {
+				return err
+			}
 		}
 		for j := range p.Tasks[i].Workspaces {
-			findInvalidParamArrayReference(p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams)
+			if err := taskrun.FindInvalidParamArrayReference(ctx, p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams); err != nil {
+				return err
+			}
 		}
 		for _, wes := range p.Tasks[i].WhenExpressions {
-			findInvalidParamArrayReference(wes.Input, arrayParams, &outofBoundParams)
+			if err := taskrun.FindInvalidParamArrayReference(ctx, wes.Input, arrayParams, &outofBoundParams); err != nil {
+				return err
+			}
 			for _, v := range wes.Values {
-				findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
+				if err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, &outofBoundParams); err != nil {
+					return err
+				}
 			}
 		}
 	}
 
 	for i := range p.Finally {
-		findInvalidParamArrayReferences(p.Finally[i].Params, arrayParams, &outofBoundParams)
+		if err := findInvalidParamArrayReferences(ctx, p.Finally[i].Params, arrayParams, &outofBoundParams); err != nil {
+			return err
+		}
 		if p.Finally[i].IsMatrixed() {
-			findInvalidParamArrayReferences(p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams)
+			if err := findInvalidParamArrayReferences(ctx, p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams); err != nil {
+				return err
+			}
 		}
 		for _, wes := range p.Finally[i].WhenExpressions {
 			for _, v := range wes.Values {
-				findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
+				if err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, &outofBoundParams); err != nil {
+					return err
+				}
 			}
 		}
 	}
@@ -171,28 +180,22 @@ func extractParamIndexes(defaults []v1beta1.ParamSpec, params []v1beta1.Param) m
 	return arrayParams
 }
 
-func findInvalidParamArrayReferences(params []v1beta1.Param, arrayParams map[string]int, outofBoundParams *sets.String) {
+// findInvalidParamArrayReferences validates all the params' array indexing references and check if they are valid
+func findInvalidParamArrayReferences(ctx context.Context, params []v1beta1.Param, arrayParams map[string]int, outofBoundParams *sets.String) error {
 	for i := range params {
-		findInvalidParamArrayReference(params[i].Value.StringVal, arrayParams, outofBoundParams)
+		if err := taskrun.FindInvalidParamArrayReference(ctx, params[i].Value.StringVal, arrayParams, outofBoundParams); err != nil {
+			return err
+		}
 		for _, v := range params[i].Value.ArrayVal {
-			findInvalidParamArrayReference(v, arrayParams, outofBoundParams)
+			if err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, outofBoundParams); err != nil {
+				return err
+			}
 		}
 		for _, v := range params[i].Value.ObjectVal {
-			findInvalidParamArrayReference(v, arrayParams, outofBoundParams)
-		}
-	}
-}
-
-func findInvalidParamArrayReference(paramReference string, arrayParams map[string]int, outofBoundParams *sets.String) {
-	list := substitution.ExtractParamsExpressions(paramReference)
-	for _, val := range list {
-		indexString := substitution.ExtractIndexString(paramReference)
-		idx, _ := substitution.ExtractIndex(indexString)
-		v := substitution.TrimArrayIndex(val)
-		if paramLength, ok := arrayParams[v]; ok {
-			if idx >= paramLength {
-				outofBoundParams.Insert(val)
+			if err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, outofBoundParams); err != nil {
+				return err
 			}
 		}
 	}
+	return nil
 }
diff --git a/pkg/reconciler/pipelinerun/resources/validate_params_test.go b/pkg/reconciler/pipelinerun/resources/validate_params_test.go
@@ -506,15 +506,12 @@ func TestValidateParamArrayIndex_valid(t *testing.T) {
 }
 
 func TestValidateParamArrayIndex_invalid(t *testing.T) {
-	ctx := context.Background()
-	cfg := config.FromContextOrDefaults(ctx)
-	cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields
-	ctx = config.ToContext(ctx, cfg)
 	for _, tt := range []struct {
-		name     string
-		original v1beta1.PipelineSpec
-		params   []v1beta1.Param
-		expected error
+		name      string
+		original  v1beta1.PipelineSpec
+		params    []v1beta1.Param
+		apifields string
+		expected  error
 	}{{
 		name: "single parameter reference out of bound",
 		original: v1beta1.PipelineSpec{
@@ -698,11 +695,38 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) {
 		},
 		params:   []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}},
 		expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"),
+	}, {
+		name: "alpha gate not enabled",
+		original: v1beta1.PipelineSpec{
+			Params: []v1beta1.ParamSpec{
+				{Name: "first-param", Type: v1beta1.ParamTypeArray, Default: v1beta1.NewStructuredValues("default-value", "default-value-again")},
+				{Name: "second-param", Type: v1beta1.ParamTypeArray},
+			},
+			Tasks: []v1beta1.PipelineTask{{
+				Params: []v1beta1.Param{
+					{Name: "first-task-first-param", Value: *v1beta1.NewStructuredValues("$(params.first-param[2])")},
+					{Name: "first-task-second-param", Value: *v1beta1.NewStructuredValues("static value")},
+				},
+				Workspaces: []v1beta1.WorkspacePipelineTaskBinding{
+					{
+						Name:      "first-workspace",
+						Workspace: "first-workspace",
+						SubPath:   "$(params.second-param[3])",
+					},
+				},
+			}},
+		},
+		params:    []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}},
+		apifields: "stable",
+		expected:  fmt.Errorf(`indexing into array param %s requires "enable-api-fields" feature gate to be "alpha"`, "$(params.first-param[2])"),
 	},
 	} {
 		tt := tt // capture range variable
 		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
+			ctx := context.Background()
+			if tt.apifields != "stable" {
+				ctx = config.ToContext(ctx, &config.Config{FeatureFlags: &config.FeatureFlags{EnableAPIFields: "alpha"}})
+			}
 			run := &v1beta1.PipelineRun{
 				Spec: v1beta1.PipelineRunSpec{
 					Params: tt.params,