add feature gate validation for param array indexing

Before this commit, if alpha or beta 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. closes #6102 Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
tektoncd · Mar 2, 2023 · 4c941dd · 4c941dd
1 parent 67abc4a
commit 4c941dd
Show file tree

Hide file tree

Showing 8 changed files with 214 additions and 38 deletions.
diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go
@@ -429,10 +429,6 @@ func validateResultsFromMatrixedPipelineTasksNotConsumed(tasks []PipelineTask, f
 // error is returned when the array indexing reference is out of bound of the array param
 // e.g. if a param reference of $(params.array-param[2]) and the array param is of length 2.
 func (ps *PipelineSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error {
-	if !config.CheckAlphaOrBetaAPIFields(ctx) {
-		return nil
-	}
-
 	// Collect all array params lengths
 	arrayParamsLengths := ps.Params.extractParamArrayLengths()
 	for k, v := range params.extractParamArrayLengths() {
@@ -470,5 +466,10 @@ func (ps *PipelineSpec) ValidateParamArrayIndex(ctx context.Context, params Para
 		arrayIndexParamRefs = append(arrayIndexParamRefs, extractArrayIndexingParamRefs(p)...)
 	}
 
+	// if there are array indexing param references, the api feature gate needs to be set to `alpha` or `beta`
+	if len(arrayIndexParamRefs) > 0 && !config.CheckAlphaOrBetaAPIFields(ctx) {
+		return fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)
+	}
+
 	return validateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths)
 }
diff --git a/pkg/apis/pipeline/v1/pipeline_validation_test.go b/pkg/apis/pipeline/v1/pipeline_validation_test.go
@@ -3276,9 +3276,10 @@ func TestValidateParamArrayIndex_valid(t *testing.T) {
 	cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields
 	ctx = config.ToContext(ctx, cfg)
 	for _, tt := range []struct {
-		name     string
-		original PipelineSpec
-		params   []Param
+		name      string
+		original  PipelineSpec
+		params    []Param
+		apiFields string
 	}{{
 		name: "single parameter",
 		original: PipelineSpec{
@@ -3434,11 +3435,30 @@ func TestValidateParamArrayIndex_valid(t *testing.T) {
 			}},
 		},
 		params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}},
+	}, {
+		name: "validation with alpha gate",
+		original: PipelineSpec{
+			Params: []ParamSpec{
+				{Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")},
+				{Name: "second-param", Type: ParamTypeString},
+			},
+			Tasks: []PipelineTask{{
+				Params: []Param{
+					{Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[1])")},
+					{Name: "first-task-second-param", Value: *NewStructuredValues("$(params.second-param[0])")},
+					{Name: "first-task-third-param", Value: *NewStructuredValues("static value")},
+				},
+			}},
+		},
+		params:    []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}},
+		apiFields: config.AlphaAPIFields,
 	},
 	} {
 		tt := tt // capture range variable
+		if tt.apiFields == config.AlphaAPIFields {
+			ctx = config.EnableAlphaAPIFields(ctx)
+		}
 		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
 			err := tt.original.ValidateParamArrayIndex(ctx, tt.params)
 			if err != nil {
 				t.Errorf("ValidateParamArrayIndex() got err %s", err)
@@ -3453,10 +3473,11 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) {
 	cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields
 	ctx = config.ToContext(ctx, cfg)
 	for _, tt := range []struct {
-		name     string
-		original PipelineSpec
-		params   []Param
-		expected error
+		name      string
+		original  PipelineSpec
+		params    []Param
+		apiFields string
+		expected  error
 	}{{
 		name: "single parameter reference out of bound",
 		original: PipelineSpec{
@@ -3640,11 +3661,31 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) {
 		},
 		params:   []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}},
 		expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"),
+	}, {
+		name: "stable gate not allowed",
+		original: PipelineSpec{
+			Params: []ParamSpec{
+				{Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")},
+				{Name: "second-param", Type: ParamTypeString},
+			},
+			Tasks: []PipelineTask{{
+				Params: []Param{
+					{Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[2])")},
+					{Name: "first-task-second-param", Value: *NewStructuredValues("$(params.second-param[2])")},
+					{Name: "first-task-third-param", Value: *NewStructuredValues("static value")},
+				},
+			}},
+		},
+		params:    []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}},
+		apiFields: config.StableAPIFields,
+		expected:  fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, []string{"$(params.first-param[2])", "$(params.second-param[2])"}),
 	},
 	} {
 		tt := tt // capture range variable
 		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
+			if tt.apiFields == config.StableAPIFields {
+				ctx = config.EnableStableAPIFields(ctx)
+			}
 			err := tt.original.ValidateParamArrayIndex(ctx, tt.params)
 			if d := cmp.Diff(tt.expected.Error(), err.Error()); d != "" {
 				t.Errorf("ValidateParamArrayIndex() errors diff %s", diff.PrintWantGot(d))

diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go
@@ -607,11 +607,6 @@ func isParamRefs(s string) bool {
 // - `trParams` are params from taskrun.
 // - `taskSpec` contains params declarations.
 func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error {
-	cfg := config.FromContextOrDefaults(ctx)
-	if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields {
-		return nil
-	}
-
 	// Collect all array params lengths
 	arrayParamsLengths := ts.Params.extractParamArrayLengths()
 	for k, v := range params.extractParamArrayLengths() {
@@ -634,5 +629,10 @@ func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params)
 		arrayIndexParamRefs = append(arrayIndexParamRefs, extractArrayIndexingParamRefs(p)...)
 	}
 
+	// if there are array indexing param references, the api feature gate needs to be set to `alpha` or `beta`
+	if len(arrayIndexParamRefs) > 0 && !config.CheckAlphaOrBetaAPIFields(ctx) {
+		return fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)
+	}
+
 	return validateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths)
 }
diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go
@@ -1745,6 +1745,7 @@ func TestValidateParamArrayIndex(t *testing.T) {
 		name          string
 		params        []v1.Param
 		taskspec      *v1.TaskSpec
+		apiFields     string
 		expectedError error
 	}{{
 		name: "steps reference invalid",
@@ -1913,11 +1914,56 @@ func TestValidateParamArrayIndex(t *testing.T) {
 			},
 		},
 		expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"),
+	}, {
+		name: "stable gate not allowed",
+		params: []v1.Param{{
+			Name:  "array-params",
+			Value: *v1.NewStructuredValues("bar", "foo"),
+		}},
+		taskspec: &v1.TaskSpec{
+			Params: []v1.ParamSpec{{
+				Name:    "array-params",
+				Default: v1.NewStructuredValues("bar", "foo"),
+			}},
+			Sidecars: []v1.Sidecar{{
+				Script: "$(params.array-params[3])",
+			},
+			},
+		},
+		apiFields:     config.StableAPIFields,
+		expectedError: fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, []string{"$(params.array-params[3])"}),
+	}, {
+		name: "alpha gate allowed",
+		params: []v1.Param{{
+			Name:  "array-params",
+			Value: *v1.NewStructuredValues("bar", "foo"),
+		}},
+		taskspec: &v1.TaskSpec{
+			Params: []v1.ParamSpec{{
+				Name:    "array-params",
+				Default: v1.NewStructuredValues("bar", "foo"),
+			}},
+			Sidecars: []v1.Sidecar{{
+				Script: "$(params.array-params[3])",
+			},
+			},
+		},
+		apiFields:     config.AlphaAPIFields,
+		expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"),
 	},
 	}
 	for _, tc := range tcs {
 		t.Run(tc.name, func(t *testing.T) {
-			ctx := config.ToContext(context.Background(), &config.Config{FeatureFlags: &config.FeatureFlags{EnableAPIFields: "alpha"}})
+			// set default API fields to "beta" if not specified
+			ctx := context.Background()
+			switch tc.apiFields {
+			case config.AlphaAPIFields:
+				ctx = config.EnableAlphaAPIFields(ctx)
+			case config.StableAPIFields:
+				ctx = config.EnableStableAPIFields(ctx)
+			default:
+				ctx = config.EnableBetaAPIFields(ctx)
+			}
 			err := tc.taskspec.ValidateParamArrayIndex(ctx, tc.params)
 			if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" {
 				t.Errorf("validateParamArrayIndex() errors diff %s", diff.PrintWantGot(d))

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go
@@ -540,10 +540,6 @@ func validateResultsFromMatrixedPipelineTasksNotConsumed(tasks []PipelineTask, f
 // error is returned when the array indexing reference is out of bound of the array param
 // e.g. if a param reference of $(params.array-param[2]) and the array param is of length 2.
 func (ps *PipelineSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error {
-	if !config.CheckAlphaOrBetaAPIFields(ctx) {
-		return nil
-	}
-
 	// Collect all array params lengths
 	arrayParamsLengths := ps.Params.extractParamArrayLengths()
 	for k, v := range params.extractParamArrayLengths() {
@@ -581,5 +577,10 @@ func (ps *PipelineSpec) ValidateParamArrayIndex(ctx context.Context, params Para
 		arrayIndexParamRefs = append(arrayIndexParamRefs, extractArrayIndexingParamRefs(p)...)
 	}
 
+	// if there are array indexing param references, the api feature gate needs to be set to `alpha` or `beta`
+	if len(arrayIndexParamRefs) > 0 && !config.CheckAlphaOrBetaAPIFields(ctx) {
+		return fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)
+	}
+
 	return validateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths)
 }
diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
@@ -3736,9 +3736,10 @@ func TestValidateParamArrayIndex_valid(t *testing.T) {
 	cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields
 	ctx = config.ToContext(ctx, cfg)
 	for _, tt := range []struct {
-		name     string
-		original PipelineSpec
-		params   []Param
+		name      string
+		original  PipelineSpec
+		params    []Param
+		apiFields string
 	}{{
 		name: "single parameter",
 		original: PipelineSpec{
@@ -3894,11 +3895,30 @@ func TestValidateParamArrayIndex_valid(t *testing.T) {
 			}},
 		},
 		params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}},
+	}, {
+		name: "validation with alpha gate",
+		original: PipelineSpec{
+			Params: []ParamSpec{
+				{Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")},
+				{Name: "second-param", Type: ParamTypeString},
+			},
+			Tasks: []PipelineTask{{
+				Params: []Param{
+					{Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[1])")},
+					{Name: "first-task-second-param", Value: *NewStructuredValues("$(params.second-param[0])")},
+					{Name: "first-task-third-param", Value: *NewStructuredValues("static value")},
+				},
+			}},
+		},
+		params:    []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}},
+		apiFields: config.AlphaAPIFields,
 	},
 	} {
 		tt := tt // capture range variable
+		if tt.apiFields == config.AlphaAPIFields {
+			ctx = config.EnableAlphaAPIFields(ctx)
+		}
 		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
 			err := tt.original.ValidateParamArrayIndex(ctx, tt.params)
 			if err != nil {
 				t.Errorf("ValidateParamArrayIndex() got err %s", err)
@@ -3913,10 +3933,11 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) {
 	cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields
 	ctx = config.ToContext(ctx, cfg)
 	for _, tt := range []struct {
-		name     string
-		original PipelineSpec
-		params   []Param
-		expected error
+		name      string
+		original  PipelineSpec
+		params    []Param
+		apiFields string
+		expected  error
 	}{{
 		name: "single parameter reference out of bound",
 		original: PipelineSpec{
@@ -4100,11 +4121,31 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) {
 		},
 		params:   []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}},
 		expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"),
+	}, {
+		name: "stable gate not allowed",
+		original: PipelineSpec{
+			Params: []ParamSpec{
+				{Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")},
+				{Name: "second-param", Type: ParamTypeString},
+			},
+			Tasks: []PipelineTask{{
+				Params: []Param{
+					{Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[2])")},
+					{Name: "first-task-second-param", Value: *NewStructuredValues("$(params.second-param[2])")},
+					{Name: "first-task-third-param", Value: *NewStructuredValues("static value")},
+				},
+			}},
+		},
+		params:    []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}},
+		apiFields: config.StableAPIFields,
+		expected:  fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, []string{"$(params.first-param[2])", "$(params.second-param[2])"}),
 	},
 	} {
 		tt := tt // capture range variable
 		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
+			if tt.apiFields == config.StableAPIFields {
+				ctx = config.EnableStableAPIFields(ctx)
+			}
 			err := tt.original.ValidateParamArrayIndex(ctx, tt.params)
 			if d := cmp.Diff(tt.expected.Error(), err.Error()); d != "" {
 				t.Errorf("ValidateParamArrayIndex() errors diff %s", diff.PrintWantGot(d))

diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go
@@ -628,11 +628,6 @@ func isParamRefs(s string) bool {
 // - `trParams` are params from taskrun.
 // - `taskSpec` contains params declarations.
 func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error {
-	cfg := config.FromContextOrDefaults(ctx)
-	if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields {
-		return nil
-	}
-
 	// Collect all array params lengths
 	arrayParamsLengths := ts.Params.extractParamArrayLengths()
 	for k, v := range params.extractParamArrayLengths() {
@@ -655,5 +650,10 @@ func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params)
 		arrayIndexParamRefs = append(arrayIndexParamRefs, extractArrayIndexingParamRefs(p)...)
 	}
 
+	// if there are array indexing param references, the api feature gate needs to be set to `alpha` or `beta`
+	if len(arrayIndexParamRefs) > 0 && !config.CheckAlphaOrBetaAPIFields(ctx) {
+		return fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)
+	}
+
 	return validateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths)
 }