From 3e6b9e35008c8311bea87d02df884aeca35906cf Mon Sep 17 00:00:00 2001 From: Quan Zhang Date: Tue, 24 Oct 2023 11:26:42 -0400 Subject: [PATCH] [TEP-0144] Add enum API field Part of [#7270][#7270]. In [TEP-0144][tep-0144] we proposed a new `enum` field to support built-in param input validation. This commit adds the `Enum` api field, validation and conversion logic. /kind feature [#7270]: #7270 [tep-0144]: https://github.com/tektoncd/community/blob/main/teps/0144-param-enum.md --- config/config-feature-flags.yaml | 3 + docs/pipeline-api.md | 26 ++ pkg/apis/config/feature_flags.go | 8 + pkg/apis/config/feature_flags_test.go | 4 + .../testdata/feature-flags-all-flags-set.yaml | 1 + ...ature-flags-invalid-enable-param-enum.yaml | 21 + pkg/apis/pipeline/v1/openapi_generated.go | 15 + pkg/apis/pipeline/v1/param_types.go | 48 ++- pkg/apis/pipeline/v1/pipeline_types_test.go | 47 ++- pkg/apis/pipeline/v1/pipeline_validation.go | 3 +- .../pipeline/v1/pipeline_validation_test.go | 83 +++- pkg/apis/pipeline/v1/swagger.json | 8 + pkg/apis/pipeline/v1/task_validation.go | 1 + pkg/apis/pipeline/v1/task_validation_test.go | 83 ++++ pkg/apis/pipeline/v1/zz_generated.deepcopy.go | 5 + .../pipeline/v1beta1/openapi_generated.go | 15 + pkg/apis/pipeline/v1beta1/param_conversion.go | 2 + pkg/apis/pipeline/v1beta1/param_types.go | 48 ++- .../v1beta1/pipeline_conversion_test.go | 1 + .../pipeline/v1beta1/pipeline_types_test.go | 53 ++- .../pipeline/v1beta1/pipeline_validation.go | 3 +- .../v1beta1/pipeline_validation_test.go | 397 +++++++++++------- pkg/apis/pipeline/v1beta1/swagger.json | 8 + .../pipeline/v1beta1/task_conversion_test.go | 1 + pkg/apis/pipeline/v1beta1/task_validation.go | 1 + .../pipeline/v1beta1/task_validation_test.go | 83 ++++ .../pipeline/v1beta1/zz_generated.deepcopy.go | 5 + 27 files changed, 746 insertions(+), 227 deletions(-) create mode 100644 pkg/apis/config/testdata/feature-flags-invalid-enable-param-enum.yaml diff --git a/config/config-feature-flags.yaml b/config/config-feature-flags.yaml index a49d26ccb78..6c5639bae7f 100644 --- a/config/config-feature-flags.yaml +++ b/config/config-feature-flags.yaml @@ -127,3 +127,6 @@ data: # Setting this flag to "true" will enable the use of StepActions in Steps # This feature is in preview mode and not implemented yet. Please check #7259 for updates. enable-step-actions: "false" + # Setting this flag to "true" will enable the built-in param input validation via param enum. + # NOTE (#7270): this feature is still under development and not yet functional. + enable-param-enum: "false" diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index 39d002be95c..90d680da780 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -1691,6 +1691,19 @@ default is set, a Task may be executed without a supplied value for the parameter.

+ + +enum
+ +[]string + + + +(Optional) +

Enum declares a set of allowed param input values for tasks/pipelines that can be validated. +If Enum is not set, no input validation is performed for the param.

+ +

ParamSpecs @@ -9917,6 +9930,19 @@ default is set, a Task may be executed without a supplied value for the parameter.

+ + +enum
+ +[]string + + + +(Optional) +

Enum declares a set of allowed param input values for tasks/pipelines that can be validated. +If Enum is not set, no input validation is performed for the param.

+ +

ParamSpecs diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 656de7c8a93..d0655c3dcf1 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -100,6 +100,10 @@ const ( EnableStepActions = "enable-step-actions" // DefaultEnableStepActions is the default value for EnableStepActions DefaultEnableStepActions = false + // EnableParamEnum is the flag to enabled enum in params + EnableParamEnum = "enable-param-enum" + // DefaultEnableParamEnum is the default value for EnableParamEnum + DefaultEnableParamEnum = false disableAffinityAssistantKey = "disable-affinity-assistant" disableCredsInitKey = "disable-creds-init" @@ -150,6 +154,7 @@ type FeatureFlags struct { Coschedule string EnableCELInWhenExpression bool EnableStepActions bool + EnableParamEnum bool } // GetFeatureFlagsConfigName returns the name of the configmap containing all @@ -228,6 +233,9 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { if err := setFeature(EnableStepActions, DefaultEnableStepActions, &tc.EnableStepActions); err != nil { return nil, err } + if err := setFeature(EnableParamEnum, DefaultEnableParamEnum, &tc.EnableParamEnum); err != nil { + return nil, err + } // Given that they are alpha features, Tekton Bundles and Custom Tasks should be switched on if // enable-api-fields is "alpha". If enable-api-fields is not "alpha" then fall back to the value of // each feature's individual flag. diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go index afa954c8d4d..cb0f145ad1e 100644 --- a/pkg/apis/config/feature_flags_test.go +++ b/pkg/apis/config/feature_flags_test.go @@ -75,6 +75,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { Coschedule: config.CoscheduleDisabled, EnableCELInWhenExpression: true, EnableStepActions: true, + EnableParamEnum: true, }, fileName: "feature-flags-all-flags-set", }, @@ -277,6 +278,9 @@ func TestNewFeatureFlagsConfigMapErrors(t *testing.T) { }, { fileName: "feature-flags-invalid-enable-step-actions", want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax`, + }, { + fileName: "feature-flags-invalid-enable-param-enum", + want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax`, }} { t.Run(tc.fileName, func(t *testing.T) { cm := test.ConfigMapFromTestFile(t, tc.fileName) diff --git a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml index bf087e8a427..349527d4125 100644 --- a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml +++ b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml @@ -34,3 +34,4 @@ data: keep-pod-on-cancel: "true" enable-cel-in-whenexpression: "true" enable-step-actions: "true" + enable-param-enum: "true" diff --git a/pkg/apis/config/testdata/feature-flags-invalid-enable-param-enum.yaml b/pkg/apis/config/testdata/feature-flags-invalid-enable-param-enum.yaml new file mode 100644 index 00000000000..b101896d0b4 --- /dev/null +++ b/pkg/apis/config/testdata/feature-flags-invalid-enable-param-enum.yaml @@ -0,0 +1,21 @@ +# Copyright 2023 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: feature-flags + namespace: tekton-pipelines +data: + enable-param-enum: "invalid" diff --git a/pkg/apis/pipeline/v1/openapi_generated.go b/pkg/apis/pipeline/v1/openapi_generated.go index 6425ec1de3b..5ddf62b303b 100644 --- a/pkg/apis/pipeline/v1/openapi_generated.go +++ b/pkg/apis/pipeline/v1/openapi_generated.go @@ -788,6 +788,21 @@ func schema_pkg_apis_pipeline_v1_ParamSpec(ref common.ReferenceCallback) common. Ref: ref("github.com/tektoncd/pipeline/pkg/apis/pipeline/v1.ParamValue"), }, }, + "enum": { + SchemaProps: spec.SchemaProps{ + Description: "Enum declares a set of allowed param input values for tasks/pipelines that can be validated. If Enum is not set, no input validation is performed for the param.", + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Default: "", + Type: []string{"string"}, + Format: "", + }, + }, + }, + }, + }, }, Required: []string{"name"}, }, diff --git a/pkg/apis/pipeline/v1/param_types.go b/pkg/apis/pipeline/v1/param_types.go index 167a1084a20..072b3726202 100644 --- a/pkg/apis/pipeline/v1/param_types.go +++ b/pkg/apis/pipeline/v1/param_types.go @@ -22,6 +22,7 @@ import ( "fmt" "strings" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/substitution" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -53,6 +54,10 @@ type ParamSpec struct { // parameter. // +optional Default *ParamValue `json:"default,omitempty"` + // Enum declares a set of allowed param input values for tasks/pipelines that can be validated. + // If Enum is not set, no input validation is performed for the param. + // +optional + Enum []string `json:"enum,omitempty"` } // ParamSpecs is a list of ParamSpec @@ -132,22 +137,47 @@ func (ps ParamSpecs) sortByType() (ParamSpecs, ParamSpecs, ParamSpecs) { // validateNoDuplicateNames returns an error if any of the params have the same name func (ps ParamSpecs) validateNoDuplicateNames() *apis.FieldError { + var errs *apis.FieldError names := ps.getNames() - seen := sets.String{} - dups := sets.String{} + for dup := range findDups(names) { + errs = errs.Also(apis.ErrGeneric("parameter appears more than once", "").ViaFieldKey("params", dup)) + } + return errs +} + +// validateParamEnum validates feature flag, duplication and allowed types for Param Enum +func (ps ParamSpecs) validateParamEnums(ctx context.Context) *apis.FieldError { var errs *apis.FieldError - for _, n := range names { - if seen.Has(n) { - dups.Insert(n) + for _, p := range ps { + if len(p.Enum) == 0 { + continue + } + if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { + errs = errs.Also(errs, apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use Enum", config.EnableParamEnum), "").ViaFieldKey("params", p.Name)) + } + if p.Type != ParamTypeString { + errs = errs.Also(apis.ErrGeneric("enum can only be set with string type param", "").ViaFieldKey("params", p.Name)) + } + for dup := range findDups(p.Enum) { + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("parameter enum value %v appears more than once", dup), "").ViaFieldKey("params", p.Name)) } - seen.Insert(n) - } - for n := range dups { - errs = errs.Also(apis.ErrGeneric("parameter appears more than once", "").ViaFieldKey("params", n)) } return errs } +// findDups returns the duplicate element in the given slice +func findDups(vals []string) sets.String { + seen := sets.String{} + dups := sets.String{} + for _, val := range vals { + if seen.Has(val) { + dups.Insert(val) + } + seen.Insert(val) + } + return dups +} + // Param declares an ParamValues to use for the parameter called name. type Param struct { Name string `json:"name"` diff --git a/pkg/apis/pipeline/v1/pipeline_types_test.go b/pkg/apis/pipeline/v1/pipeline_types_test.go index 75d8657962a..61771b90f2a 100644 --- a/pkg/apis/pipeline/v1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1/pipeline_types_test.go @@ -508,10 +508,9 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) { func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { tests := []struct { - name string - tasks PipelineTask - enableAlphaAPIFields bool - enableBetaAPIFields bool + name string + tasks PipelineTask + configMap map[string]string }{{ name: "pipeline task - valid taskRef name", tasks: PipelineTask{ @@ -524,37 +523,51 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { Name: "foo", TaskSpec: &EmbeddedTask{TaskSpec: getTaskSpec()}, }, + }, { + name: "pipeline task - valid taskSpec with param enum", + tasks: PipelineTask{ + Name: "foo", + TaskSpec: &EmbeddedTask{ + TaskSpec: TaskSpec{ + Steps: []Step{ + { + Name: "foo", + Image: "bar", + }, + }, + Params: []ParamSpec{ + { + Name: "param1", + Type: ParamTypeString, + Enum: []string{"v1", "v2"}, + }, + }, + }, + }, + }, + configMap: map[string]string{"enable-param-enum": "true"}, }, { name: "pipeline task - use of resolver with the feature flag set", tasks: PipelineTask{ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}}, }, - enableBetaAPIFields: true, + configMap: map[string]string{"enable-api-field": "beta"}, }, { name: "pipeline task - use of resolver with the feature flag set to alpha", tasks: PipelineTask{ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}}, }, - enableAlphaAPIFields: true, + configMap: map[string]string{"enable-api-field": "alpha"}, }, { name: "pipeline task - use of resolver params with the feature flag set", tasks: PipelineTask{ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar", Params: Params{{}}}}, }, - enableBetaAPIFields: true, + configMap: map[string]string{"enable-api-field": "beta"}, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := context.Background() - cfg := &config.Config{ - FeatureFlags: &config.FeatureFlags{}, - } - if tt.enableAlphaAPIFields { - cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields - } else if tt.enableBetaAPIFields { - cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields - } - ctx = config.ToContext(ctx, cfg) + ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tt.configMap) err := tt.tasks.validateTask(ctx) if err != nil { t.Errorf("PipelineTask.validateTask() returned error for valid pipeline task: %v", err) diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index 2552692d87a..06e85050851 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -433,11 +433,12 @@ func validatePipelineTasksWorkspacesUsage(wss []PipelineWorkspaceDeclaration, pt // ValidatePipelineParameterVariables validates parameters with those specified by each pipeline task, // (1) it validates the type of parameter is either string or array (2) parameter default value matches -// with the type of that param +// with the type of that param (3) no duplicateion, feature flag and allowed param type when using param enum func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { // validates all the types within a slice of ParamSpecs errs = errs.Also(ValidateParameterTypes(ctx, params).ViaField("params")) errs = errs.Also(params.validateNoDuplicateNames()) + errs = errs.Also(params.validateParamEnums(ctx)) for i, task := range tasks { errs = errs.Also(task.Params.validateDuplicateParameters().ViaField("params").ViaIndex(i)) } diff --git a/pkg/apis/pipeline/v1/pipeline_validation_test.go b/pkg/apis/pipeline/v1/pipeline_validation_test.go index 820e7958e6d..c76537ba633 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1/pipeline_validation_test.go @@ -1295,13 +1295,14 @@ func TestFinallyTaskResultsToPipelineResults_Failure(t *testing.T) { func TestValidatePipelineParameterVariables_Success(t *testing.T) { tests := []struct { - name string - params []ParamSpec - tasks []PipelineTask + name string + params []ParamSpec + tasks []PipelineTask + configMap map[string]string }{{ - name: "valid string parameter variables", + name: "valid string parameter variables with enum", params: []ParamSpec{{ - Name: "baz", Type: ParamTypeString, + Name: "baz", Type: ParamTypeString, Enum: []string{"v1", "v2"}, }, { Name: "foo-is-baz", Type: ParamTypeString, }}, @@ -1312,6 +1313,7 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) { Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.baz) and $(params.foo-is-baz)"}, }}, }}, + configMap: map[string]string{"enable-param-enum": "true"}, }, { name: "valid string parameter variables in when expression", params: []ParamSpec{{ @@ -1580,6 +1582,9 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := cfgtesting.EnableAlphaAPIFields(context.Background()) + if tt.configMap != nil { + ctx = cfgtesting.SetFeatureFlags(ctx, t, tt.configMap) + } err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params) if err != nil { t.Errorf("Pipeline.ValidatePipelineParameterVariables() returned error for valid pipeline parameters: %v", err) @@ -1921,7 +1926,7 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := validatePipelineTaskParameterUsage(tt.tasks, tt.params) if err == nil { - t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters") + t.Errorf("Pipeline.validatePipelineTaskParameterUsage() did not return error for invalid pipeline parameters") } if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) @@ -1936,7 +1941,69 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { params []ParamSpec tasks []PipelineTask expectedError apis.FieldError + configMap map[string]string }{{ + name: "param enum with array type - failure", + params: []ParamSpec{{ + Name: "param2", + Type: ParamTypeArray, + Enum: []string{"v1", "v2"}, + }}, + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedError: apis.FieldError{ + Message: `enum can only be set with string type param`, + Paths: []string{"params[param2]"}, + }, + }, { + name: "param enum with object type - failure", + params: []ParamSpec{{ + Name: "param2", + Type: ParamTypeObject, + Enum: []string{"v1", "v2"}, + }}, + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedError: apis.FieldError{ + Message: `enum can only be set with string type param`, + Paths: []string{"params[param2]"}, + }, + }, { + name: "param enum with duplicate values - failure", + params: []ParamSpec{{ + Name: "param1", + Type: ParamTypeString, + Enum: []string{"v1", "v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedError: apis.FieldError{ + Message: `parameter enum value v1 appears more than once`, + Paths: []string{"params[param1]"}, + }, + }, { + name: "param enum with feature flag disabled - failure", + params: []ParamSpec{{ + Name: "param1", + Type: ParamTypeString, + Enum: []string{"v1", "v2"}, + }}, + expectedError: apis.FieldError{ + Message: `feature flag enable-param-enum should be set to true to use Enum`, + Paths: []string{"params[param1]"}, + }, + }, { name: "invalid parameter type", params: []ParamSpec{{ Name: "foo", Type: "invalidtype", @@ -2105,6 +2172,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() + if tt.configMap != nil { + ctx = cfgtesting.SetFeatureFlags(ctx, t, tt.configMap) + } + err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params) if err == nil { t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters") diff --git a/pkg/apis/pipeline/v1/swagger.json b/pkg/apis/pipeline/v1/swagger.json index 76d325bfff0..20a30df097d 100644 --- a/pkg/apis/pipeline/v1/swagger.json +++ b/pkg/apis/pipeline/v1/swagger.json @@ -343,6 +343,14 @@ "description": "Description is a user-facing description of the parameter that may be used to populate a UI.", "type": "string" }, + "enum": { + "description": "Enum declares a set of allowed param input values for tasks/pipelines that can be validated. If Enum is not set, no input validation is performed for the param.", + "type": "array", + "items": { + "type": "string", + "default": "" + } + }, "name": { "description": "Name declares the name by which a parameter is referenced.", "type": "string", diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index e85179d60d1..cdf430b35cf 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -399,6 +399,7 @@ func (p ParamSpec) ValidateObjectType(ctx context.Context) *apis.FieldError { func ValidateParameterVariables(ctx context.Context, steps []Step, params ParamSpecs) *apis.FieldError { var errs *apis.FieldError errs = errs.Also(params.validateNoDuplicateNames()) + errs = errs.Also(params.validateParamEnums(ctx)) stringParams, arrayParams, objectParams := params.sortByType() stringParameterNames := sets.NewString(stringParams.getNames()...) arrayParameterNames := sets.NewString(arrayParams.getNames()...) diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index b04d43f190e..14c744b12c2 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -2111,3 +2111,86 @@ func TestGetArrayIndexParamRefs(t *testing.T) { }) } } + +func TestParamEnum(t *testing.T) { + tcs := []struct { + name string + params v1.ParamSpecs + configMap map[string]string + expectedErr error + }{{ + name: "valid param enum - success", + params: []v1.ParamSpec{{ + Name: "param1", + Type: v1.ParamTypeString, + Enum: []string{"v1", "v2"}, + }, { + Name: "param2", + Type: v1.ParamTypeString, + Enum: []string{"v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + }, { + name: "param enum with array type - failure", + params: []v1.ParamSpec{{ + Name: "param2", + Type: v1.ParamTypeArray, + Enum: []string{"v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedErr: fmt.Errorf("enum can only be set with string type param: params[param2]"), + }, { + name: "param enum with object type - failure", + params: []v1.ParamSpec{{ + Name: "param2", + Type: v1.ParamTypeObject, + Enum: []string{"v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedErr: fmt.Errorf("enum can only be set with string type param: params[param2]"), + }, { + name: "param enum with duplicate values - failure", + params: []v1.ParamSpec{{ + Name: "param1", + Type: v1.ParamTypeString, + Enum: []string{"v1", "v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedErr: fmt.Errorf("parameter enum value v1 appears more than once: params[param1]"), + }, { + name: "param enum with feature flag disabled - failure", + params: []v1.ParamSpec{{ + Name: "param1", + Type: v1.ParamTypeString, + Enum: []string{"v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "false", + }, + expectedErr: fmt.Errorf("feature flag enable-param-enum should be set to true to use Enum: params[param1]"), + }} + + for _, tc := range tcs { + ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tc.configMap) + + err := v1.ValidateParameterVariables(ctx, []v1.Step{{Image: "foo"}}, tc.params) + + if tc.expectedErr == nil && err != nil { + t.Errorf("No error expected from TaskSpec.Validate() but got = %v", err) + } else if tc.expectedErr != nil { + if err == nil { + t.Errorf("Expected error from TaskSpec.Validate() = %v, but got none", tc.expectedErr) + } else if d := cmp.Diff(tc.expectedErr.Error(), err.Error()); d != "" { + t.Errorf("returned error from TaskSpec.Validate() does not match with the expected error: %s", diff.PrintWantGot(d)) + } + } + } +} diff --git a/pkg/apis/pipeline/v1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1/zz_generated.deepcopy.go index d47e242753c..8e1b025c82d 100644 --- a/pkg/apis/pipeline/v1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1/zz_generated.deepcopy.go @@ -231,6 +231,11 @@ func (in *ParamSpec) DeepCopyInto(out *ParamSpec) { *out = new(ParamValue) (*in).DeepCopyInto(*out) } + if in.Enum != nil { + in, out := &in.Enum, &out.Enum + *out = make([]string, len(*in)) + copy(*out, *in) + } return } diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index eba5a872419..fa53e5b064b 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -1329,6 +1329,21 @@ func schema_pkg_apis_pipeline_v1beta1_ParamSpec(ref common.ReferenceCallback) co Ref: ref("github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.ParamValue"), }, }, + "enum": { + SchemaProps: spec.SchemaProps{ + Description: "Enum declares a set of allowed param input values for tasks/pipelines that can be validated. If Enum is not set, no input validation is performed for the param.", + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Default: "", + Type: []string{"string"}, + Format: "", + }, + }, + }, + }, + }, }, Required: []string{"name"}, }, diff --git a/pkg/apis/pipeline/v1beta1/param_conversion.go b/pkg/apis/pipeline/v1beta1/param_conversion.go index 5ac4f50a863..a47206b76e1 100644 --- a/pkg/apis/pipeline/v1beta1/param_conversion.go +++ b/pkg/apis/pipeline/v1beta1/param_conversion.go @@ -30,6 +30,7 @@ func (p ParamSpec) convertTo(ctx context.Context, sink *v1.ParamSpec) { sink.Type = v1.ParamType(ParamTypeString) } sink.Description = p.Description + sink.Enum = p.Enum var properties map[string]v1.PropertySpec if p.Properties != nil { properties = make(map[string]v1.PropertySpec) @@ -54,6 +55,7 @@ func (p *ParamSpec) convertFrom(ctx context.Context, source v1.ParamSpec) { p.Type = ParamTypeString } p.Description = source.Description + p.Enum = source.Enum var properties map[string]PropertySpec if source.Properties != nil { properties = make(map[string]PropertySpec) diff --git a/pkg/apis/pipeline/v1beta1/param_types.go b/pkg/apis/pipeline/v1beta1/param_types.go index c703fcd5a4d..46735ee08c0 100644 --- a/pkg/apis/pipeline/v1beta1/param_types.go +++ b/pkg/apis/pipeline/v1beta1/param_types.go @@ -22,6 +22,7 @@ import ( "fmt" "strings" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/substitution" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -53,6 +54,10 @@ type ParamSpec struct { // parameter. // +optional Default *ParamValue `json:"default,omitempty"` + // Enum declares a set of allowed param input values for tasks/pipelines that can be validated. + // If Enum is not set, no input validation is performed for the param. + // +optional + Enum []string `json:"enum,omitempty"` } // ParamSpecs is a list of ParamSpec @@ -123,22 +128,47 @@ func (ps ParamSpecs) sortByType() (ParamSpecs, ParamSpecs, ParamSpecs) { // validateNoDuplicateNames returns an error if any of the params have the same name func (ps ParamSpecs) validateNoDuplicateNames() *apis.FieldError { + var errs *apis.FieldError names := ps.getNames() - seen := sets.String{} - dups := sets.String{} + for dup := range findDups(names) { + errs = errs.Also(apis.ErrGeneric("parameter appears more than once", "").ViaFieldKey("params", dup)) + } + return errs +} + +// validateParamEnum validates feature flag, duplication and allowed types for Param Enum +func (ps ParamSpecs) validateParamEnums(ctx context.Context) *apis.FieldError { var errs *apis.FieldError - for _, n := range names { - if seen.Has(n) { - dups.Insert(n) + for _, p := range ps { + if len(p.Enum) == 0 { + continue + } + if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { + errs = errs.Also(errs, apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use Enum", config.EnableParamEnum), "").ViaFieldKey("params", p.Name)) + } + if p.Type != ParamTypeString { + errs = errs.Also(apis.ErrGeneric("enum can only be set with string type param", "").ViaFieldKey("params", p.Name)) + } + for dup := range findDups(p.Enum) { + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("parameter enum value %v appears more than once", dup), "").ViaFieldKey("params", p.Name)) } - seen.Insert(n) - } - for n := range dups { - errs = errs.Also(apis.ErrGeneric("parameter appears more than once", "").ViaFieldKey("params", n)) } return errs } +// findDups returns the duplicate element in the given slice +func findDups(vals []string) sets.String { + seen := sets.String{} + dups := sets.String{} + for _, val := range vals { + if seen.Has(val) { + dups.Insert(val) + } + seen.Insert(val) + } + return dups +} + // setDefaultsForProperties sets default type for PropertySpec (string) if it's not specified func (pp *ParamSpec) setDefaultsForProperties() { for key, propertySpec := range pp.Properties { diff --git a/pkg/apis/pipeline/v1beta1/pipeline_conversion_test.go b/pkg/apis/pipeline/v1beta1/pipeline_conversion_test.go index 3b5939ccce3..23b66eabb1d 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_conversion_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_conversion_test.go @@ -69,6 +69,7 @@ func TestPipelineConversion(t *testing.T) { Params: []v1beta1.ParamSpec{{ Name: "param-1", Type: v1beta1.ParamTypeString, + Enum: []string{"v1", "v2"}, Description: "My first param", }}, }, diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go index 7b58610f61e..d0dc19b4cdf 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go @@ -541,58 +541,69 @@ func TestPipelineTask_ValidateBundle_Failure(t *testing.T) { func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { tests := []struct { - name string - tasks PipelineTask - enableAPIFields string - enableBundles bool + name string + tasks PipelineTask + configMap map[string]string }{{ name: "pipeline task - valid taskRef name", tasks: PipelineTask{ Name: "foo", TaskRef: &TaskRef{Name: "example.com/my-foo-task"}, }, - enableAPIFields: "stable", + configMap: map[string]string{"enable-api-fields": "stable"}, }, { name: "pipeline task - valid taskSpec", tasks: PipelineTask{ Name: "foo", TaskSpec: &EmbeddedTask{TaskSpec: getTaskSpec()}, }, - enableAPIFields: "stable", + configMap: map[string]string{"enable-api-fields": "stable"}, + }, { + name: "pipeline task - valid taskSpec with param enum", + tasks: PipelineTask{ + Name: "foo", + TaskSpec: &EmbeddedTask{ + TaskSpec: TaskSpec{ + Steps: []Step{ + { + Name: "foo", + Image: "bar", + }, + }, + Params: []ParamSpec{ + { + Name: "param1", + Type: ParamTypeString, + Enum: []string{"v1", "v2"}, + }, + }, + }, + }, + }, + configMap: map[string]string{"enable-param-enum": "true"}, }, { name: "pipeline task - use of resolver", tasks: PipelineTask{ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}}, }, - enableAPIFields: "beta", + configMap: map[string]string{"enable-api-fields": "beta"}, }, { name: "pipeline task - use of params", tasks: PipelineTask{ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar", Params: Params{}}}, }, - enableAPIFields: "beta", + configMap: map[string]string{"enable-api-fields": "beta"}, }, { name: "pipeline task - use of bundle with the feature flag set", tasks: PipelineTask{ Name: "foo", TaskRef: &TaskRef{Name: "bar", Bundle: "docker.io/foo"}, }, - enableBundles: true, - enableAPIFields: "stable", + configMap: map[string]string{"enable-tekton-oci-bundles": "true"}, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := context.Background() - cfg := &config.Config{ - FeatureFlags: &config.FeatureFlags{}, - } - if tt.enableAPIFields != "" { - cfg.FeatureFlags.EnableAPIFields = tt.enableAPIFields - } - if tt.enableBundles { - cfg.FeatureFlags.EnableTektonOCIBundles = true - } - ctx = config.ToContext(ctx, cfg) + ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tt.configMap) err := tt.tasks.validateTask(ctx) if err != nil { t.Errorf("PipelineTask.validateTask() returned error for valid pipeline task: %v", err) diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index bf384b1c815..526e60c094a 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -452,11 +452,12 @@ func validatePipelineTasksWorkspacesUsage(wss []PipelineWorkspaceDeclaration, pt // ValidatePipelineParameterVariables validates parameters with those specified by each pipeline task, // (1) it validates the type of parameter is either string or array (2) parameter default value matches -// with the type of that param +// with the type of that param (3) no duplicateion, feature flag and allowed param type when using param enum func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { // validates all the types within a slice of ParamSpecs errs = errs.Also(ValidateParameterTypes(ctx, params).ViaField("params")) errs = errs.Also(params.validateNoDuplicateNames()) + errs = errs.Also(params.validateParamEnums(ctx)) for i, task := range tasks { errs = errs.Also(task.Params.validateDuplicateParameters().ViaField("params").ViaIndex(i)) } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 00ceb53d210..f47562adb89 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -1338,13 +1338,14 @@ func TestFinallyTaskResultsToPipelineResults_Failure(t *testing.T) { func TestValidatePipelineParameterVariables_Success(t *testing.T) { tests := []struct { - name string - params []ParamSpec - tasks []PipelineTask + name string + params []ParamSpec + tasks []PipelineTask + configMap map[string]string }{{ - name: "valid string parameter variables", + name: "valid string parameter variables with enum", params: []ParamSpec{{ - Name: "baz", Type: ParamTypeString, + Name: "baz", Type: ParamTypeString, Enum: []string{"v1", "v2"}, }, { Name: "foo-is-baz", Type: ParamTypeString, }}, @@ -1355,6 +1356,7 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) { Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.baz) and $(params.foo-is-baz)"}, }}, }}, + configMap: map[string]string{"enable-param-enum": "true"}, }, { name: "valid string parameter variables in when expression", params: []ParamSpec{{ @@ -1622,7 +1624,11 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := ValidatePipelineParameterVariables(context.Background(), tt.tasks, tt.params) + ctx := context.Background() + if tt.configMap != nil { + ctx = cfgtesting.SetFeatureFlags(ctx, t, tt.configMap) + } + err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params) if err != nil { t.Errorf("Pipeline.ValidatePipelineParameterVariables() returned error for valid pipeline parameters: %v", err) } @@ -1963,7 +1969,7 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := validatePipelineTaskParameterUsage(tt.tasks, tt.params) if err == nil { - t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters") + t.Errorf("Pipeline.validatePipelineTaskParameterUsage() did not return error for invalid pipeline parameters") } if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) @@ -1978,175 +1984,240 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { params []ParamSpec tasks []PipelineTask expectedError apis.FieldError - }{{ - name: "invalid parameter type", - params: []ParamSpec{{ - Name: "foo", Type: "invalidtype", - }}, - tasks: []PipelineTask{{ - Name: "foo", - TaskRef: &TaskRef{Name: "foo-task"}, - }}, - expectedError: apis.FieldError{ - Message: `invalid value: invalidtype`, - Paths: []string{"params.foo.type"}, - }, - }, { - name: "array parameter mismatching default type", - params: []ParamSpec{{ - Name: "foo", Type: ParamTypeArray, Default: &ParamValue{Type: ParamTypeString, StringVal: "astring"}, - }}, - tasks: []PipelineTask{{ - Name: "foo", - TaskRef: &TaskRef{Name: "foo-task"}, - }}, - expectedError: apis.FieldError{ - Message: `"array" type does not match default value's type: "string"`, - Paths: []string{"params.foo.default.type", "params.foo.type"}, - }, - }, { - name: "string parameter mismatching default type", - params: []ParamSpec{{ - Name: "foo", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, - }}, - tasks: []PipelineTask{{ - Name: "foo", - TaskRef: &TaskRef{Name: "foo-task"}, - }}, - expectedError: apis.FieldError{ - Message: `"string" type does not match default value's type: "array"`, - Paths: []string{"params.foo.default.type", "params.foo.type"}, - }, - }, { - name: "array parameter used as string", - params: []ParamSpec{{ - Name: "baz", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, - }}, - tasks: []PipelineTask{{ - Name: "bar", - TaskRef: &TaskRef{Name: "bar-task"}, - Params: Params{{ - Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.baz)"}, + configMap map[string]string + }{ + {name: "param enum with array type - failure", + params: []ParamSpec{{ + Name: "param2", + Type: ParamTypeArray, + Enum: []string{"v1", "v2"}, + }}, + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, }}, - }}, - expectedError: apis.FieldError{ - Message: `"string" type does not match default value's type: "array"`, - Paths: []string{"params.baz.default.type", "params.baz.type"}, - }, - }, { - name: "star array parameter used as string", - params: []ParamSpec{{ - Name: "baz", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, - }}, - tasks: []PipelineTask{{ - Name: "bar", - TaskRef: &TaskRef{Name: "bar-task"}, - Params: Params{{ - Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.baz[*])"}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedError: apis.FieldError{ + Message: `enum can only be set with string type param`, + Paths: []string{"params[param2]"}, + }, + }, { + name: "param enum with object type - failure", + params: []ParamSpec{{ + Name: "param2", + Type: ParamTypeObject, + Enum: []string{"v1", "v2"}, }}, - }}, - expectedError: apis.FieldError{ - Message: `"string" type does not match default value's type: "array"`, - Paths: []string{"params.baz.default.type", "params.baz.type"}, - }, - }, { - name: "array parameter string template not isolated", - params: []ParamSpec{{ - Name: "baz", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, - }}, - tasks: []PipelineTask{{ - Name: "bar", - TaskRef: &TaskRef{Name: "bar-task"}, - Params: Params{{ - Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"value: $(params.baz)", "last"}}, + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, }}, - }}, - expectedError: apis.FieldError{ - Message: `"string" type does not match default value's type: "array"`, - Paths: []string{"params.baz.default.type", "params.baz.type"}, - }, - }, { - name: "star array parameter string template not isolated", - params: []ParamSpec{{ - Name: "baz", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, - }}, - tasks: []PipelineTask{{ - Name: "bar", - TaskRef: &TaskRef{Name: "bar-task"}, - Params: Params{{ - Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"value: $(params.baz[*])", "last"}}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedError: apis.FieldError{ + Message: `enum can only be set with string type param`, + Paths: []string{"params[param2]"}, + }, + }, { + name: "param enum with duplicate values - failure", + params: []ParamSpec{{ + Name: "param1", + Type: ParamTypeString, + Enum: []string{"v1", "v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedError: apis.FieldError{ + Message: `parameter enum value v1 appears more than once`, + Paths: []string{"params[param1]"}, + }, + }, { + name: "param enum with feature flag disabled - failure", + params: []ParamSpec{{ + Name: "param1", + Type: ParamTypeString, + Enum: []string{"v1", "v2"}, + }}, + expectedError: apis.FieldError{ + Message: `feature flag enable-param-enum should be set to true to use Enum`, + Paths: []string{"params[param1]"}, + }, + }, { + name: "invalid parameter type", + params: []ParamSpec{{ + Name: "foo", Type: "invalidtype", }}, - }}, - expectedError: apis.FieldError{ - Message: `"string" type does not match default value's type: "array"`, - Paths: []string{"params.baz.default.type", "params.baz.type"}, - }, - }, { - name: "multiple string parameters with the same name", - params: []ParamSpec{{ - Name: "baz", Type: ParamTypeString, + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + expectedError: apis.FieldError{ + Message: `invalid value: invalidtype`, + Paths: []string{"params.foo.type"}, + }, }, { - Name: "baz", Type: ParamTypeString, - }}, - tasks: []PipelineTask{{ - Name: "foo", - TaskRef: &TaskRef{Name: "foo-task"}, - }}, - expectedError: apis.FieldError{ - Message: `parameter appears more than once`, - Paths: []string{"params[baz]"}, - }, - }, { - name: "multiple array parameters with the same name", - params: []ParamSpec{{ - Name: "baz", Type: ParamTypeArray, + name: "array parameter mismatching default type", + params: []ParamSpec{{ + Name: "foo", Type: ParamTypeArray, Default: &ParamValue{Type: ParamTypeString, StringVal: "astring"}, + }}, + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + expectedError: apis.FieldError{ + Message: `"array" type does not match default value's type: "string"`, + Paths: []string{"params.foo.default.type", "params.foo.type"}, + }, }, { - Name: "baz", Type: ParamTypeArray, - }}, - tasks: []PipelineTask{{ - Name: "foo", - TaskRef: &TaskRef{Name: "foo-task"}, - }}, - expectedError: apis.FieldError{ - Message: `parameter appears more than once`, - Paths: []string{"params[baz]"}, - }, - }, { - name: "multiple different type parameters with the same name", - params: []ParamSpec{{ - Name: "baz", Type: ParamTypeArray, + name: "string parameter mismatching default type", + params: []ParamSpec{{ + Name: "foo", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, + }}, + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params.foo.default.type", "params.foo.type"}, + }, }, { - Name: "baz", Type: ParamTypeString, - }}, - tasks: []PipelineTask{{ - Name: "foo", - TaskRef: &TaskRef{Name: "foo-task"}, - }}, - expectedError: apis.FieldError{ - Message: `parameter appears more than once`, - Paths: []string{"params[baz]"}, - }, - }, { - name: "invalid task use duplicate parameters", - tasks: []PipelineTask{{ - Name: "foo-task", - TaskRef: &TaskRef{Name: "foo-task"}, - Params: Params{{ - Name: "duplicate-param", Value: ParamValue{Type: ParamTypeString, StringVal: "val1"}, + name: "array parameter used as string", + params: []ParamSpec{{ + Name: "baz", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, + }}, + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: Params{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.baz)"}, + }}, + }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params.baz.default.type", "params.baz.type"}, + }, + }, { + name: "star array parameter used as string", + params: []ParamSpec{{ + Name: "baz", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, + }}, + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: Params{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.baz[*])"}, + }}, + }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params.baz.default.type", "params.baz.type"}, + }, + }, { + name: "array parameter string template not isolated", + params: []ParamSpec{{ + Name: "baz", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, + }}, + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: Params{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"value: $(params.baz)", "last"}}, + }}, + }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params.baz.default.type", "params.baz.type"}, + }, + }, { + name: "star array parameter string template not isolated", + params: []ParamSpec{{ + Name: "baz", Type: ParamTypeString, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"anarray", "elements"}}, + }}, + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: Params{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"value: $(params.baz[*])", "last"}}, + }}, + }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params.baz.default.type", "params.baz.type"}, + }, + }, { + name: "multiple string parameters with the same name", + params: []ParamSpec{{ + Name: "baz", Type: ParamTypeString, + }, { + Name: "baz", Type: ParamTypeString, + }}, + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + expectedError: apis.FieldError{ + Message: `parameter appears more than once`, + Paths: []string{"params[baz]"}, + }, + }, { + name: "multiple array parameters with the same name", + params: []ParamSpec{{ + Name: "baz", Type: ParamTypeArray, }, { - Name: "duplicate-param", Value: ParamValue{Type: ParamTypeString, StringVal: "val2"}, + Name: "baz", Type: ParamTypeArray, + }}, + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + expectedError: apis.FieldError{ + Message: `parameter appears more than once`, + Paths: []string{"params[baz]"}, + }, + }, { + name: "multiple different type parameters with the same name", + params: []ParamSpec{{ + Name: "baz", Type: ParamTypeArray, }, { - Name: "duplicate-param", Value: ParamValue{Type: ParamTypeString, StringVal: "val3"}, + Name: "baz", Type: ParamTypeString, }}, - }}, - expectedError: apis.FieldError{ - Message: `parameter names must be unique, the parameter "duplicate-param" is also defined at`, - Paths: []string{"[0].params[1].name, [0].params[2].name"}, - }, - }} + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + expectedError: apis.FieldError{ + Message: `parameter appears more than once`, + Paths: []string{"params[baz]"}, + }, + }, { + name: "invalid task use duplicate parameters", + tasks: []PipelineTask{{ + Name: "foo-task", + TaskRef: &TaskRef{Name: "foo-task"}, + Params: Params{{ + Name: "duplicate-param", Value: ParamValue{Type: ParamTypeString, StringVal: "val1"}, + }, { + Name: "duplicate-param", Value: ParamValue{Type: ParamTypeString, StringVal: "val2"}, + }, { + Name: "duplicate-param", Value: ParamValue{Type: ParamTypeString, StringVal: "val3"}, + }}, + }}, + expectedError: apis.FieldError{ + Message: `parameter names must be unique, the parameter "duplicate-param" is also defined at`, + Paths: []string{"[0].params[1].name, [0].params[2].name"}, + }, + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() + if tt.configMap != nil { + ctx = cfgtesting.SetFeatureFlags(ctx, t, tt.configMap) + } err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params) if err == nil { t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters") diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index 4ff7d3d43ab..3f65e38d9eb 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -624,6 +624,14 @@ "description": "Description is a user-facing description of the parameter that may be used to populate a UI.", "type": "string" }, + "enum": { + "description": "Enum declares a set of allowed param input values for tasks/pipelines that can be validated. If Enum is not set, no input validation is performed for the param.", + "type": "array", + "items": { + "type": "string", + "default": "" + } + }, "name": { "description": "Name declares the name by which a parameter is referenced.", "type": "string", diff --git a/pkg/apis/pipeline/v1beta1/task_conversion_test.go b/pkg/apis/pipeline/v1beta1/task_conversion_test.go index 629ed816c7a..6eb79ea63e9 100644 --- a/pkg/apis/pipeline/v1beta1/task_conversion_test.go +++ b/pkg/apis/pipeline/v1beta1/task_conversion_test.go @@ -56,6 +56,7 @@ spec: params: - name: param-1 type: string + enum: ["v1", "v2"] description: my first param results: - name: result-1 diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 36646a4d1d2..328b03bd6db 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -405,6 +405,7 @@ func (p ParamSpec) ValidateObjectType(ctx context.Context) *apis.FieldError { func ValidateParameterVariables(ctx context.Context, steps []Step, params ParamSpecs) *apis.FieldError { var errs *apis.FieldError errs = errs.Also(params.validateNoDuplicateNames()) + errs = errs.Also(params.validateParamEnums(ctx)) stringParams, arrayParams, objectParams := params.sortByType() stringParameterNames := sets.NewString(stringParams.getNames()...) arrayParameterNames := sets.NewString(arrayParams.getNames()...) diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index 484c90cb748..8b08a6fb39a 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -1947,3 +1947,86 @@ func TestGetArrayIndexParamRefs(t *testing.T) { }) } } + +func TestParamEnum(t *testing.T) { + tcs := []struct { + name string + params v1beta1.ParamSpecs + configMap map[string]string + expectedErr error + }{{ + name: "valid param enum - success", + params: []v1beta1.ParamSpec{{ + Name: "param1", + Type: v1beta1.ParamTypeString, + Enum: []string{"v1", "v2"}, + }, { + Name: "param2", + Type: v1beta1.ParamTypeString, + Enum: []string{"v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + }, { + name: "param enum with array type - failure", + params: []v1beta1.ParamSpec{{ + Name: "param2", + Type: v1beta1.ParamTypeArray, + Enum: []string{"v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedErr: fmt.Errorf("enum can only be set with string type param: params[param2]"), + }, { + name: "param enum with object type - failure", + params: []v1beta1.ParamSpec{{ + Name: "param2", + Type: v1beta1.ParamTypeObject, + Enum: []string{"v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedErr: fmt.Errorf("enum can only be set with string type param: params[param2]"), + }, { + name: "param enum with duplicate values - failure", + params: []v1beta1.ParamSpec{{ + Name: "param1", + Type: v1beta1.ParamTypeString, + Enum: []string{"v1", "v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "true", + }, + expectedErr: fmt.Errorf("parameter enum value v1 appears more than once: params[param1]"), + }, { + name: "param enum with feature flag disabled - failure", + params: []v1beta1.ParamSpec{{ + Name: "param1", + Type: v1beta1.ParamTypeString, + Enum: []string{"v1", "v2"}, + }}, + configMap: map[string]string{ + "enable-param-enum": "false", + }, + expectedErr: fmt.Errorf("feature flag enable-param-enum should be set to true to use Enum: params[param1]"), + }} + + for _, tc := range tcs { + ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tc.configMap) + + err := v1beta1.ValidateParameterVariables(ctx, []v1beta1.Step{{Image: "foo"}}, tc.params) + + if tc.expectedErr == nil && err != nil { + t.Errorf("No error expected from TaskSpec.Validate() but got = %v", err) + } else if tc.expectedErr != nil { + if err == nil { + t.Errorf("Expected error from TaskSpec.Validate() = %v, but got none", tc.expectedErr) + } else if d := cmp.Diff(tc.expectedErr.Error(), err.Error()); d != "" { + t.Errorf("returned error from TaskSpec.Validate() does not match with the expected error: %s", diff.PrintWantGot(d)) + } + } + } +} diff --git a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index aa44da6184e..99288ead150 100644 --- a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go @@ -515,6 +515,11 @@ func (in *ParamSpec) DeepCopyInto(out *ParamSpec) { *out = new(ParamValue) (*in).DeepCopyInto(*out) } + if in.Enum != nil { + in, out := &in.Enum, &out.Enum + *out = make([]string, len(*in)) + copy(*out, *in) + } return }