diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 305200b7fe9..bcbfcdc4c1d 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -315,17 +315,17 @@ func NewFeatureFlagsFromConfigMap(config *corev1.ConfigMap) (*FeatureFlags, erro // EnableAlphaAPIFields enables alpha features in an existing context (for use in testing) func EnableAlphaAPIFields(ctx context.Context) context.Context { - return setEnableAPIFields(ctx, AlphaAPIFields) + return SetEnableAPIFields(ctx, AlphaAPIFields) } // EnableBetaAPIFields enables beta features in an existing context (for use in testing) func EnableBetaAPIFields(ctx context.Context) context.Context { - return setEnableAPIFields(ctx, BetaAPIFields) + return SetEnableAPIFields(ctx, BetaAPIFields) } // EnableStableAPIFields enables stable features in an existing context (for use in testing) func EnableStableAPIFields(ctx context.Context) context.Context { - return setEnableAPIFields(ctx, StableAPIFields) + return SetEnableAPIFields(ctx, StableAPIFields) } // CheckEnforceResourceVerificationMode returns true if the ResourceVerificationMode is EnforceResourceVerificationMode @@ -348,7 +348,8 @@ func CheckAlphaOrBetaAPIFields(ctx context.Context) bool { return cfg.FeatureFlags.EnableAPIFields == AlphaAPIFields || cfg.FeatureFlags.EnableAPIFields == BetaAPIFields } -func setEnableAPIFields(ctx context.Context, want string) context.Context { +// SetEnableAPIFields sets the config "enable-api-fields" to the "want" value +func SetEnableAPIFields(ctx context.Context, want string) context.Context { featureFlags, _ := NewFeatureFlagsFromMap(map[string]string{ "enable-api-fields": want, }) diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index db7dd5edd47..7e54cd5cd94 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(`invalid parameter expression %s: indexing into array params requires "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..1ccf1213a0d 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1/pipeline_validation_test.go @@ -3271,14 +3271,11 @@ func getTaskSpec() TaskSpec { } func TestValidateParamArrayIndex_valid(t *testing.T) { - ctx := context.Background() - cfg := config.FromContextOrDefaults(ctx) - 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{ @@ -3294,7 +3291,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "single parameter with when expression", original: PipelineSpec{ @@ -3310,7 +3308,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "pipeline parameter nested inside task parameter", original: PipelineSpec{ @@ -3325,7 +3324,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: nil, // no parameter values. + params: nil, // no parameter values. + apiFields: config.BetaAPIFields, }, { name: "array parameter", original: PipelineSpec{ @@ -3343,6 +3343,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, + apiFields: config.BetaAPIFields, }, { name: "parameter evaluation with final tasks", original: PipelineSpec{ @@ -3362,7 +3363,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "parameter evaluation with both tasks and final tasks", original: PipelineSpec{ @@ -3388,7 +3390,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "parameter references with bracket notation and special characters", original: PipelineSpec{ @@ -3412,6 +3415,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { {Name: "second/param", Value: *NewStructuredValues("second-value", "second-value-again")}, {Name: "fourth/param", Value: *NewStructuredValues("fourth-value", "fourth-value-again")}, }, + apiFields: config.BetaAPIFields, }, { name: "single parameter in workspace subpath", original: PipelineSpec{ @@ -3433,10 +3437,29 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + }, { + 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 + ctx := config.SetEnableAPIFields(context.Background(), tt.apiFields) t.Run(tt.name, func(t *testing.T) { t.Parallel() err := tt.original.ValidateParamArrayIndex(ctx, tt.params) @@ -3448,15 +3471,12 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { } func TestValidateParamArrayIndex_invalid(t *testing.T) { - ctx := context.Background() - cfg := config.FromContextOrDefaults(ctx) - 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{ @@ -3472,8 +3492,9 @@ 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[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "single parameter reference with when expression out of bound", original: PipelineSpec{ @@ -3489,8 +3510,9 @@ 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[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "pipeline parameter reference nested inside task parameter out of bound", original: PipelineSpec{ @@ -3505,8 +3527,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: nil, // no parameter values. - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: nil, // no parameter values. + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "array parameter reference out of bound", original: PipelineSpec{ @@ -3524,7 +3547,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[3]) $(params.second-param[4])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[3]) $(params.second-param[4])]"), }, { name: "object parameter reference out of bound", original: PipelineSpec{ @@ -3544,7 +3568,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[4]) $(params.second-param[4])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[4]) $(params.second-param[4])]"), }, { name: "parameter evaluation with final tasks reference out of bound", original: PipelineSpec{ @@ -3564,8 +3589,9 @@ 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[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "parameter evaluation with both tasks and final tasks reference out of bound", original: PipelineSpec{ @@ -3591,8 +3617,9 @@ 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.first-param[3]) $(params.second-param[2]) $(params.second-param[3])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.first-param[3]) $(params.second-param[2]) $(params.second-param[3])]"), }, { name: "parameter references with bracket notation and special characters reference out of bound", original: PipelineSpec{ @@ -3616,7 +3643,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { {Name: "second/param", Value: *NewStructuredValues("second-value", "second-value-again")}, {Name: "fourth/param", Value: *NewStructuredValues("fourth-value", "fourth-value-again")}, }, - expected: fmt.Errorf("non-existent param references:[$(params[\"first.param\"][2]) $(params[\"second/param\"][2]) $(params['fourth/param'][2]) $(params['third.param'][2])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params[\"first.param\"][2]) $(params[\"second/param\"][2]) $(params['fourth/param'][2]) $(params['third.param'][2])]"), }, { name: "single parameter in workspace subpath reference out of bound", original: PipelineSpec{ @@ -3638,13 +3666,56 @@ 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])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"), + }, { + name: "validation alpha feature gate", + original: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + {Name: "second-param", Type: ParamTypeArray}, + }, + Tasks: []PipelineTask{{ + Params: []Param{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[2])")}, + {Name: "first-task-second-param", Value: *NewStructuredValues("static value")}, + }, + Workspaces: []WorkspacePipelineTaskBinding{ + { + Name: "first-workspace", + Workspace: "first-workspace", + SubPath: "$(params.second-param[3])", + }, + }, + }}, + }, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.AlphaAPIFields, + 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(`invalid parameter expression %s: indexing into array params requires "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() + ctx := config.SetEnableAPIFields(context.Background(), tt.apiFields) 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..ba7e0a34288 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -3731,14 +3731,11 @@ func enableFeatures(t *testing.T, features []string) func(context.Context) conte } func TestValidateParamArrayIndex_valid(t *testing.T) { - ctx := context.Background() - cfg := config.FromContextOrDefaults(ctx) - 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{ @@ -3754,7 +3751,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "single parameter with when expression", original: PipelineSpec{ @@ -3770,7 +3768,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "pipeline parameter nested inside task parameter", original: PipelineSpec{ @@ -3785,7 +3784,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: nil, // no parameter values. + params: nil, // no parameter values. + apiFields: config.BetaAPIFields, }, { name: "array parameter", original: PipelineSpec{ @@ -3803,6 +3803,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, + apiFields: config.BetaAPIFields, }, { name: "parameter evaluation with final tasks", original: PipelineSpec{ @@ -3822,7 +3823,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "parameter evaluation with both tasks and final tasks", original: PipelineSpec{ @@ -3848,7 +3850,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "parameter references with bracket notation and special characters", original: PipelineSpec{ @@ -3872,6 +3875,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { {Name: "second/param", Value: *NewStructuredValues("second-value", "second-value-again")}, {Name: "fourth/param", Value: *NewStructuredValues("fourth-value", "fourth-value-again")}, }, + apiFields: config.BetaAPIFields, }, { name: "single parameter in workspace subpath", original: PipelineSpec{ @@ -3893,10 +3897,29 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + }, { + 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 + ctx := config.SetEnableAPIFields(context.Background(), tt.apiFields) t.Run(tt.name, func(t *testing.T) { t.Parallel() err := tt.original.ValidateParamArrayIndex(ctx, tt.params) @@ -3908,15 +3931,12 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { } func TestValidateParamArrayIndex_invalid(t *testing.T) { - ctx := context.Background() - cfg := config.FromContextOrDefaults(ctx) - 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{ @@ -3932,8 +3952,9 @@ 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[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "single parameter reference with when expression out of bound", original: PipelineSpec{ @@ -3949,8 +3970,9 @@ 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[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "pipeline parameter reference nested inside task parameter out of bound", original: PipelineSpec{ @@ -3965,8 +3987,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: nil, // no parameter values. - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: nil, // no parameter values. + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "array parameter reference out of bound", original: PipelineSpec{ @@ -3984,7 +4007,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[3]) $(params.second-param[4])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[3]) $(params.second-param[4])]"), }, { name: "object parameter reference out of bound", original: PipelineSpec{ @@ -4004,7 +4028,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[4]) $(params.second-param[4])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[4]) $(params.second-param[4])]"), }, { name: "parameter evaluation with final tasks reference out of bound", original: PipelineSpec{ @@ -4024,8 +4049,9 @@ 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[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "parameter evaluation with both tasks and final tasks reference out of bound", original: PipelineSpec{ @@ -4051,8 +4077,9 @@ 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.first-param[3]) $(params.second-param[2]) $(params.second-param[3])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.first-param[3]) $(params.second-param[2]) $(params.second-param[3])]"), }, { name: "parameter references with bracket notation and special characters reference out of bound", original: PipelineSpec{ @@ -4076,7 +4103,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { {Name: "second/param", Value: *NewStructuredValues("second-value", "second-value-again")}, {Name: "fourth/param", Value: *NewStructuredValues("fourth-value", "fourth-value-again")}, }, - expected: fmt.Errorf("non-existent param references:[$(params[\"first.param\"][2]) $(params[\"second/param\"][2]) $(params['fourth/param'][2]) $(params['third.param'][2])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params[\"first.param\"][2]) $(params[\"second/param\"][2]) $(params['fourth/param'][2]) $(params['third.param'][2])]"), }, { name: "single parameter in workspace subpath reference out of bound", original: PipelineSpec{ @@ -4098,13 +4126,56 @@ 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])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"), + }, { + name: "validation alpha feature gate", + original: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + {Name: "second-param", Type: ParamTypeArray}, + }, + Tasks: []PipelineTask{{ + Params: []Param{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[2])")}, + {Name: "first-task-second-param", Value: *NewStructuredValues("static value")}, + }, + Workspaces: []WorkspacePipelineTaskBinding{ + { + Name: "first-workspace", + Workspace: "first-workspace", + SubPath: "$(params.second-param[3])", + }, + }, + }}, + }, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.AlphaAPIFields, + 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() + ctx := config.SetEnableAPIFields(context.Background(), tt.apiFields) 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..84668df6ffa 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(`invalid parameter expression %s: indexing into array params requires "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..46bc6d4b626 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(`invalid parameter expression %s: indexing into array params requires "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))