From a95d40ae5bb124574aa2dc4cd1ff8fca6e8ba700 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Thu, 2 Mar 2023 22:14:29 +0000 Subject: [PATCH] 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. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- pkg/apis/pipeline/v1/pipeline_validation.go | 9 +-- .../pipeline/v1/pipeline_validation_test.go | 58 ++++++++++++++++--- pkg/apis/pipeline/v1/task_validation.go | 10 ++-- pkg/apis/pipeline/v1/task_validation_test.go | 48 ++++++++++++++- .../pipeline/v1beta1/pipeline_validation.go | 9 +-- .../v1beta1/pipeline_validation_test.go | 58 ++++++++++++++++--- pkg/apis/pipeline/v1beta1/task_validation.go | 10 ++-- .../pipeline/v1beta1/task_validation_test.go | 48 ++++++++++++++- 8 files changed, 214 insertions(+), 36 deletions(-) diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index db7dd5edd47..d15619af3d9 100644 --- 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 index d6f6efe4eeb..91236e130dd 100644 --- 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,9 +3435,29 @@ 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) @@ -3453,10 +3474,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 +3662,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 index fdf30e52dcb..8f98ac39501 100644 --- 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 index 5fa4d763f19..31d5c2f229d 100644 --- 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 index b1af8de0c26..b3cace2a722 100644 --- 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 index 1bc03563ec3..407e784bf07 100644 --- 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,9 +3895,29 @@ 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) @@ -3913,10 +3934,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 +4122,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 index 892aa1b35f3..5c74b2e622a 100644 --- 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) } diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index 0a42616e981..f7325fb23f9 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -1858,6 +1858,7 @@ func TestValidateParamArrayIndex(t *testing.T) { name string params []v1beta1.Param taskspec *v1beta1.TaskSpec + apiFields string expectedError error }{{ name: "steps reference invalid", @@ -2026,11 +2027,56 @@ func TestValidateParamArrayIndex(t *testing.T) { }, }, expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), + }, { + name: "stable gate not allowed", + params: []v1beta1.Param{{ + Name: "array-params", + Value: *v1beta1.NewStructuredValues("bar", "foo"), + }}, + taskspec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "array-params", + Default: v1beta1.NewStructuredValues("bar", "foo"), + }}, + Sidecars: []v1beta1.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: []v1beta1.Param{{ + Name: "array-params", + Value: *v1beta1.NewStructuredValues("bar", "foo"), + }}, + taskspec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "array-params", + Default: v1beta1.NewStructuredValues("bar", "foo"), + }}, + Sidecars: []v1beta1.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))