From 9af875d1f44d9b456a5fa020983033a22e2cfd01 Mon Sep 17 00:00:00 2001 From: JeromeJu Date: Wed, 30 Aug 2023 03:25:32 +0000 Subject: [PATCH] Decouple beta feature validation in v1beta1 This commit decouples the existing beta feature validation in v1beta1. Prior to this change, beta features are regarded as stable in v1beta1 apiVersion and they only require enable-api-fields to be set to beta in v1 apiVersion but not in v1beta1. This PR removes the gap between the validations of the stable features in v1 and v1beta1 by syncing up the validations in v1beta1. Fixes: #6592 --- api_compatibility_policy.md | 30 ++- docs/additional-configs.md | 23 +- .../pipeline/v1beta1/pipeline_types_test.go | 13 +- .../pipeline/v1beta1/pipeline_validation.go | 61 +++++- .../v1beta1/pipeline_validation_test.go | 203 +++++++++++++++++ .../v1beta1/pipelineref_validation.go | 2 + .../v1beta1/pipelineref_validation_test.go | 56 ++++- .../v1beta1/pipelinerun_validation.go | 5 + .../v1beta1/pipelinerun_validation_test.go | 205 ++++++++++++++++++ pkg/apis/pipeline/v1beta1/task_validation.go | 36 ++- .../pipeline/v1beta1/task_validation_test.go | 70 ++++++ .../pipeline/v1beta1/taskref_validation.go | 3 + .../v1beta1/taskref_validation_test.go | 28 ++- .../pipeline/v1beta1/taskrun_validation.go | 9 + .../v1beta1/taskrun_validation_test.go | 72 ++++++ 15 files changed, 773 insertions(+), 43 deletions(-) diff --git a/api_compatibility_policy.md b/api_compatibility_policy.md index 36324e59a41..5d2cc231003 100644 --- a/api_compatibility_policy.md +++ b/api_compatibility_policy.md @@ -90,26 +90,25 @@ For more information on support windows, see the [deprecations table](./docs/dep ## Feature Gates -CRD API versions gate the overall stability of the CRD and its default behaviors. Within a particular CRD version, certain opt-in features may be at a lower stability level as described in [TEP-33](https://github.com/tektoncd/community/blob/main/teps/0033-tekton-feature-gates.md). These fields may be disabled by default and can be enabled by setting the right `enable-api-fields` feature-flag as described in TEP-33: +Stability levels of feature gates are independent from CRD apiVersions. Features enabled by API fields at different levels of stability can be enabled using the flag `enable-api-fields`: -* `stable` - This value indicates that only fields of the highest stability level are enabled; For `beta` CRDs, this means only beta stability fields are enabled, i.e. `alpha` fields are not enabled. For `GA` CRDs, this means only `GA` fields are enabled, i.e. `beta` and `alpha` fields would not be enabled. TODO(#6592): Decouple feature stability from API stability. -* `beta` (default) - This value indicates that only fields which are of `beta` (or greater) stability are enabled, i.e. `alpha` fields are not enabled. -* `alpha` - This value indicates that fields of all stability levels are enabled, specifically `alpha`, `beta` and `GA`. +* `stable` - This value indicates that only fields of the highest stability level are enabled; i.e. `alpha` and `beta` fields are not enabled. +* `beta` (default) - This value indicates that only fields which are of `beta` (or greater) stability are enabled, i.e. `alpha` fields are not enabled. -| Feature Versions -> | v1 | beta | alpha | -|---------------------|----|------|-------| -| stable | x | | | -| beta | x | x | | -| alpha | x | x | x | - +* `alpha` - This value indicates that fields of all stability levels are enabled, specifically `alpha`, `beta` and `GA`(`stable`). See the current list of [alpha features](https://github.com/tektoncd/pipeline/blob/main/docs/additional-configs.md#alpha-features) and [beta features](https://github.com/tektoncd/pipeline/blob/main/docs/additional-configs.md#beta-features). +| `enable-api-fields` value | stable features enabled | beta features enabled | alpha features enabled | +|----------------------------|-------------------------|-----------------------|------------------------| +| stable | x | | | +| beta | x | x | | +| alpha | x | x | x | ### Alpha features -- Alpha feature in beta or GA CRDs are disabled by default and must be enabled by [setting `enable-api-fields` to `alpha`](https://github.com/tektoncd/pipeline/blob/main/docs/additional-configs.md#alpha-features) +- Alpha features are disabled by default and must be enabled by [setting `enable-api-fields` to `alpha`](https://github.com/tektoncd/pipeline/blob/main/docs/additional-configs.md#alpha-features) - These features may be dropped or backwards incompatible changes made at any time, though one release worth of warning will be provided. @@ -125,14 +124,13 @@ See the current list of [alpha features](https://github.com/tektoncd/pipeline/bl i.e. by providing a 9 month support period. - Beta features are reviewed for promotion to GA/Stable on a regular basis. However, there is no guarantee that they will be promoted to GA/stable. - -- For beta API versions, beta is the highest level of stability possible for any feature. ### GA/Stable features -- GA/Stable features are present in a [GA CRD](#ga-crds) only. +- GA/Stable features are enabled by default. + - GA/Stable API-driven features are no longer controlled by the `enable-api-fields` flag because they cannot be disabled. -- GA/Stable features are enabled by default +- GA/Stable features are features that have been promoted from beta to the highest level of stability. They cannot be disabled in any CRD version. - GA/Stable features will not be removed or changed in a backwards incompatible manner without incrementing the API Version. @@ -145,7 +143,7 @@ Features are first released as experimental in alpha, refined in beta, and final #### Promoting a feature to `beta` - After feedback of the usage of the alpha features, once the needs and motivations are validated, a feature could be promoted to `beta`. This stage is where features are further tested and refined. -- The dedicated feature flag for this feature will change the stability level for validation to `beta`. It will continue to be disabled by default. +- The dedicated feature flag for this feature will change its stability level to `beta`. It will continue to be disabled by default. #### Graduating a feature to `stable` - This is the final stage of feature graduation process, where features are considered to be complete and ready to be released for the public. diff --git a/docs/additional-configs.md b/docs/additional-configs.md index a5a5a796d94..11b95952590 100644 --- a/docs/additional-configs.md +++ b/docs/additional-configs.md @@ -325,22 +325,17 @@ To enable these features, set the `enable-api-fields` feature flag to `"beta"` i the `feature-flags` ConfigMap alongside your Tekton Pipelines deployment via `kubectl patch cm feature-flags -n tekton-pipelines -p '{"data":{"enable-api-fields":"beta"}}'`. - -For beta versions of Tekton CRDs, setting `enable-api-fields` to "beta" is the same as setting it to "stable", -except where otherwise noted. - Features currently in "beta" are: -| Feature | Proposal | Alpha Release | Beta Release | Individual Flag | `enable-api-fields=beta` required for `v1beta1` | -|:-------------------------------------------------------------------|:------------------------------------------------------------------------------------------------|:---------------------------------------------------------------------|:---------------------------------------------------------------------|:----------------|:---| -| [Array Results and Array Indexing](pipelineruns.md#specifying-parameters) | [TEP-0076](https://github.com/tektoncd/community/blob/main/teps/0076-array-result-types.md) | [v0.38.0](https://github.com/tektoncd/pipeline/releases/tag/v0.38.0) | [v0.45.0](https://github.com/tektoncd/pipeline/releases/tag/v0.45.0) | | No | -| [Object Parameters and Results](pipelineruns.md#specifying-parameters) | [TEP-0075](https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md) | | [v0.46.0](https://github.com/tektoncd/pipeline/releases/tag/v0.46.0) | | No | -| [Remote Tasks](./taskruns.md#remote-tasks) and [Remote Pipelines](./pipelineruns.md#remote-pipelines) | [TEP-0060](https://github.com/tektoncd/community/blob/main/teps/0060-remote-resolution.md) | | [v0.41.0](https://github.com/tektoncd/pipeline/releases/tag/v0.41.0) | | No | -| [`Provenance` field in Status](pipeline-api.md#provenance)| [issue#5550](https://github.com/tektoncd/pipeline/issues/5550)| [v0.41.0](https://github.com/tektoncd/pipeline/releases/tag/v0.41.0)| [v0.48.0](https://github.com/tektoncd/pipeline/releases/tag/v0.48.0) | `enable-provenance-in-status`| No | -| [Isolated `Step` & `Sidecar` `Workspaces`](./workspaces.md#isolated-workspaces) | [TEP-0029](https://github.com/tektoncd/community/blob/main/teps/0029-step-workspaces.md) | [v0.24.0](https://github.com/tektoncd/pipeline/releases/tag/v0.24.0) | [v0.50.0](https://github.com/tektoncd/pipeline/releases/tag/v0.50.0) | | Yes | -| [Matrix](./matrix.md) | [TEP-0090](https://github.com/tektoncd/community/blob/main/teps/0090-matrix.md) | [v0.38.0](https://github.com/tektoncd/pipeline/releases/tag/v0.38.0) | [v0.53.0](https://github.com/tektoncd/pipeline/releases/tag/v0.53.0) | | No | -| [Task-level Resource Requirements](compute-resources.md#task-level-compute-resources-configuration) | [TEP-0104](https://github.com/tektoncd/community/blob/main/teps/0104-tasklevel-resource-requirements.md) | [v0.39.0](https://github.com/tektoncd/pipeline/releases/tag/v0.39.0) | | - +| Feature | Proposal | Alpha Release | Beta Release | Individual Flag | +|:-------------------------------------------------------------------|:------------------------------------------------------------------------------------------------|:---------------------------------------------------------------------|:---------------------------------------------------------------------|:----------------| +| [Array Results and Array Indexing](pipelineruns.md#specifying-parameters) | [TEP-0076](https://github.com/tektoncd/community/blob/main/teps/0076-array-result-types.md) | [v0.38.0](https://github.com/tektoncd/pipeline/releases/tag/v0.38.0) | [v0.45.0](https://github.com/tektoncd/pipeline/releases/tag/v0.45.0) | | +| [Object Parameters and Results](pipelineruns.md#specifying-parameters) | [TEP-0075](https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md) | | [v0.46.0](https://github.com/tektoncd/pipeline/releases/tag/v0.46.0) | | +| [Remote Tasks](./taskruns.md#remote-tasks) and [Remote Pipelines](./pipelineruns.md#remote-pipelines) | [TEP-0060](https://github.com/tektoncd/community/blob/main/teps/0060-remote-resolution.md) | | [v0.41.0](https://github.com/tektoncd/pipeline/releases/tag/v0.41.0) | | +| [`Provenance` field in Status](pipeline-api.md#provenance)| [issue#5550](https://github.com/tektoncd/pipeline/issues/5550)| [v0.41.0](https://github.com/tektoncd/pipeline/releases/tag/v0.41.0)| [v0.48.0](https://github.com/tektoncd/pipeline/releases/tag/v0.48.0) | `enable-provenance-in-status`| +| [Isolated `Step` & `Sidecar` `Workspaces`](./workspaces.md#isolated-workspaces) | [TEP-0029](https://github.com/tektoncd/community/blob/main/teps/0029-step-workspaces.md) | [v0.24.0](https://github.com/tektoncd/pipeline/releases/tag/v0.24.0) | [v0.50.0](https://github.com/tektoncd/pipeline/releases/tag/v0.50.0) | | +| [Matrix](./matrix.md) | [TEP-0090](https://github.com/tektoncd/community/blob/main/teps/0090-matrix.md) | [v0.38.0](https://github.com/tektoncd/pipeline/releases/tag/v0.38.0) | [v0.53.0](https://github.com/tektoncd/pipeline/releases/tag/v0.53.0) | | +| [Task-level Resource Requirements](compute-resources.md#task-level-compute-resources-configuration) | [TEP-0104](https://github.com/tektoncd/community/blob/main/teps/0104-tasklevel-resource-requirements.md) | [v0.39.0](https://github.com/tektoncd/pipeline/releases/tag/v0.39.0) | [v0.53.0](https://github.com/tektoncd/pipeline/releases/tag/v0.53.0) | | ## Enabling larger results using sidecar logs diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go index 680b6b2cafe..7b58610f61e 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go @@ -543,7 +543,7 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { tests := []struct { name string tasks PipelineTask - enableAPIFields bool + enableAPIFields string enableBundles bool }{{ name: "pipeline task - valid taskRef name", @@ -551,29 +551,34 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { Name: "foo", TaskRef: &TaskRef{Name: "example.com/my-foo-task"}, }, + enableAPIFields: "stable", }, { name: "pipeline task - valid taskSpec", tasks: PipelineTask{ Name: "foo", TaskSpec: &EmbeddedTask{TaskSpec: getTaskSpec()}, }, + enableAPIFields: "stable", }, { name: "pipeline task - use of resolver", tasks: PipelineTask{ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}}, }, + enableAPIFields: "beta", }, { name: "pipeline task - use of params", tasks: PipelineTask{ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar", Params: Params{}}}, }, + enableAPIFields: "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, + enableBundles: true, + enableAPIFields: "stable", }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -581,8 +586,8 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { cfg := &config.Config{ FeatureFlags: &config.FeatureFlags{}, } - if tt.enableAPIFields { - cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + if tt.enableAPIFields != "" { + cfg.FeatureFlags.EnableAPIFields = tt.enableAPIFields } if tt.enableBundles { cfg.FeatureFlags.EnableTektonOCIBundles = true diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 118fff45a15..38f6b1b2a2b 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -58,7 +58,12 @@ func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError { // we do not support propagated parameters and workspaces. // Validate that all params and workspaces it uses are declared. errs = errs.Also(p.Spec.validatePipelineParameterUsage(ctx).ViaField("spec")) - return errs.Also(p.Spec.validatePipelineWorkspacesUsage().ViaField("spec")) + errs = errs.Also(p.Spec.validatePipelineWorkspacesUsage().ViaField("spec")) + // Validate beta fields when a Pipeline is defined, but not as part of validating a Pipeline spec. + // This prevents validation from failing when a Pipeline is converted to a different API version. + // See https://github.com/tektoncd/pipeline/issues/6616 for more information. + errs = errs.Also(p.Spec.ValidateBetaFields(ctx)) + return errs } // Validate checks that taskNames in the Pipeline are valid and that the graph @@ -92,6 +97,60 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { return errs } +// ValidateBetaFields returns an error if the PipelineSpec uses beta specifications governed by +// `enable-api-fields` but does not have "enable-api-fields" set to "alpha" or "beta". +func (ps *PipelineSpec) ValidateBetaFields(ctx context.Context) *apis.FieldError { + var errs *apis.FieldError + // Object parameters + for i, p := range ps.Params { + if p.Type == ParamTypeObject { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "object type parameter", config.BetaAPIFields).ViaFieldIndex("params", i)) + } + } + // Indexing into array parameters + arrayParamIndexingRefs := ps.GetIndexingReferencesToArrayParams() + if len(arrayParamIndexingRefs) != 0 { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "indexing into array parameters", config.BetaAPIFields)) + } + // array and object results + for i, result := range ps.Results { + switch result.Type { + case ResultsTypeObject: + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "object results", config.BetaAPIFields).ViaFieldIndex("results", i)) + case ResultsTypeArray: + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "array results", config.BetaAPIFields).ViaFieldIndex("results", i)) + case ResultsTypeString: + default: + } + } + for i, pt := range ps.Tasks { + errs = errs.Also(pt.validateBetaFields(ctx).ViaFieldIndex("tasks", i)) + } + for i, pt := range ps.Finally { + errs = errs.Also(pt.validateBetaFields(ctx).ViaFieldIndex("finally", i)) + } + + return errs +} + +// validateBetaFields returns an error if the PipelineTask uses beta features but does not +// have "enable-api-fields" set to "alpha" or "beta". +func (pt *PipelineTask) validateBetaFields(ctx context.Context) *apis.FieldError { + var errs *apis.FieldError + if pt.TaskRef != nil { + // Resolvers + if pt.TaskRef.Resolver != "" { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "taskref.resolver", config.BetaAPIFields)) + } + if len(pt.TaskRef.Params) > 0 { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "taskref.params", config.BetaAPIFields)) + } + } else if pt.TaskSpec != nil { + errs = errs.Also(pt.TaskSpec.ValidateBetaFields(ctx)) + } + return errs +} + // ValidatePipelineTasks ensures that pipeline tasks has unique label, pipeline tasks has specified one of // taskRef or taskSpec, and in case of a pipeline task with taskRef, it has a reference to a valid task (task name) func ValidatePipelineTasks(ctx context.Context, tasks []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 13d862d61a9..00ceb53d210 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -4119,6 +4119,209 @@ func enableFeatures(t *testing.T, features []string) func(context.Context) conte } } +// TestPipelineWithBetaFields tests the beta API-driven features of +// PipelineSpec are correctly governed `enable-api-fields`, which must +// be set to "alpha" or "beta". +func TestPipelineWithBetaFields(t *testing.T) { + tts := []struct { + name string + spec PipelineSpec + }{{ + name: "array indexing in Tasks", + spec: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + }, + Tasks: []PipelineTask{{ + Name: "foo", + Params: Params{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[0])")}, + }, + TaskRef: &TaskRef{Name: "foo"}, + }}, + }, + }, { + name: "array indexing in Finally", + spec: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + }, + Tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo"}, + }}, + Finally: []PipelineTask{{ + Name: "bar", + Params: Params{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[0])")}, + }, + TaskRef: &TaskRef{Name: "bar"}, + }}, + }, + }, { + name: "pipeline tasks - use of resolver", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "uses-resolver", + TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}}, + }}, + }, + }, { + name: "pipeline tasks - use of resolver params", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "uses-resolver-params", + TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar", Params: Params{{}}}}, + }}, + }, + }, { + name: "finally tasks - use of resolver", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Finally: []PipelineTask{{ + Name: "uses-resolver", + TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}}, + }}, + }, + }, { + name: "finally tasks - use of resolver params", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Finally: []PipelineTask{{ + Name: "uses-resolver-params", + TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar", Params: Params{{}}}}, + }}, + }, + }, { + name: "object params", + spec: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeObject, Properties: map[string]PropertySpec{}}, + }, + Tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo"}, + }}, + }, + }, { + name: "object params in Tasks", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Params: []ParamSpec{{Name: "my-object-param", Type: ParamTypeObject, Properties: map[string]PropertySpec{}}}, + }}, + }}, + }, + }, { + name: "object params in Finally", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo"}, + }}, + Finally: []PipelineTask{{ + Name: "valid-finally-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Params: []ParamSpec{{Name: "my-object-param", Type: ParamTypeObject, Properties: map[string]PropertySpec{}}}, + }}, + }}, + }, + }, { + name: "array results", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Results: []PipelineResult{{Name: "my-array-result", Type: ResultsTypeArray, Value: *NewStructuredValues("$(tasks.valid-pipeline-task.results.foo[*])")}}, + }, + }, { + name: "array results in Tasks", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Results: []TaskResult{{Name: "my-array-result", Type: ResultsTypeArray}}, + }}, + }}, + }, + }, { + name: "array results in Finally", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Finally: []PipelineTask{{ + Name: "valid-finally-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Results: []TaskResult{{Name: "my-array-result", Type: ResultsTypeArray}}, + }}, + }}, + }, + }, { + name: "object results", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Results: []PipelineResult{{Name: "my-object-result", Type: ResultsTypeObject, Value: *NewStructuredValues("$(tasks.valid-pipeline-task.results.foo[*])")}}, + }, + }, { + name: "object results in Tasks", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Results: []TaskResult{{Name: "my-object-result", Type: ResultsTypeObject, Properties: map[string]PropertySpec{}}}, + }}, + }}, + }, + }, { + name: "object results in Finally", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Finally: []PipelineTask{{ + Name: "valid-finally-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Results: []TaskResult{{Name: "my-object-result", Type: ResultsTypeObject, Properties: map[string]PropertySpec{}}}, + }}, + }}, + }, + }} + for _, tt := range tts { + t.Run(tt.name, func(t *testing.T) { + pipeline := Pipeline{ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: tt.spec} + ctx := cfgtesting.EnableStableAPIFields(context.Background()) + if err := pipeline.Validate(ctx); err == nil { + t.Errorf("no error when using beta field when `enable-api-fields` is stable") + } + + ctx = cfgtesting.EnableBetaAPIFields(context.Background()) + if err := pipeline.Validate(ctx); err != nil { + t.Errorf("unexpected error when using beta field: %s", err) + } + }) + } +} + func TestGetIndexingReferencesToArrayParams(t *testing.T) { for _, tt := range []struct { name string diff --git a/pkg/apis/pipeline/v1beta1/pipelineref_validation.go b/pkg/apis/pipeline/v1beta1/pipelineref_validation.go index 6186c177a28..7131f6c62f5 100644 --- a/pkg/apis/pipeline/v1beta1/pipelineref_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelineref_validation.go @@ -34,6 +34,7 @@ func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) { if ref.Resolver != "" || ref.Params != nil { if ref.Resolver != "" { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) if ref.Name != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) } @@ -42,6 +43,7 @@ func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) { } } if ref.Params != nil { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) if ref.Name != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "params")) } diff --git a/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go index ea1ae14a1b2..075dafcfe5e 100644 --- a/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go @@ -22,6 +22,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/tektoncd/pipeline/pkg/apis/config" + cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/test/diff" corev1 "k8s.io/api/core/v1" @@ -62,6 +63,24 @@ func TestPipelineRef_Invalid(t *testing.T) { name: "pipelineRef without Pipeline Name", ref: &v1beta1.PipelineRef{}, wantErr: apis.ErrMissingField("name"), + }, { + name: "pipelineref resolver disallowed without beta feature gate", + ref: &v1beta1.PipelineRef{ + ResolverRef: v1beta1.ResolverRef{ + Resolver: "foo", + }, + }, + withContext: cfgtesting.EnableStableAPIFields, + wantErr: apis.ErrGeneric("resolver requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\""), + }, { + name: "pipelineref params disallowed without beta feature gate", + ref: &v1beta1.PipelineRef{ + ResolverRef: v1beta1.ResolverRef{ + Params: v1beta1.Params{}, + }, + }, + withContext: cfgtesting.EnableStableAPIFields, + wantErr: apis.ErrMissingField("resolver").Also(apis.ErrGeneric("resolver params requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"")), }, { name: "pipelineref params disallowed without resolver", ref: &v1beta1.PipelineRef{ @@ -69,7 +88,8 @@ func TestPipelineRef_Invalid(t *testing.T) { Params: v1beta1.Params{}, }, }, - wantErr: apis.ErrMissingField("resolver"), + wantErr: apis.ErrMissingField("resolver"), + withContext: cfgtesting.EnableBetaAPIFields, }, { name: "pipelineref resolver disallowed in conjunction with pipelineref name", ref: &v1beta1.PipelineRef{ @@ -78,7 +98,8 @@ func TestPipelineRef_Invalid(t *testing.T) { Resolver: "bar", }, }, - wantErr: apis.ErrMultipleOneOf("name", "resolver"), + wantErr: apis.ErrMultipleOneOf("name", "resolver"), + withContext: cfgtesting.EnableBetaAPIFields, }, { name: "pipelineref resolver disallowed in conjunction with pipelineref bundle", ref: &v1beta1.PipelineRef{ @@ -103,7 +124,8 @@ func TestPipelineRef_Invalid(t *testing.T) { }}, }, }, - wantErr: apis.ErrMultipleOneOf("name", "params").Also(apis.ErrMissingField("resolver")), + wantErr: apis.ErrMultipleOneOf("name", "params").Also(apis.ErrMissingField("resolver")), + withContext: cfgtesting.EnableBetaAPIFields, }, { name: "pipelineref params disallowed in conjunction with pipelineref bundle", ref: &v1beta1.PipelineRef{ @@ -120,6 +142,24 @@ func TestPipelineRef_Invalid(t *testing.T) { }, wantErr: apis.ErrMultipleOneOf("bundle", "params").Also(apis.ErrMissingField("resolver")), withContext: enableTektonOCIBundles(t), + }, { + name: "pipelineref param object not allowed in stable", + ref: &v1beta1.PipelineRef{ + ResolverRef: v1beta1.ResolverRef{ + Resolver: "some-resolver", + Params: v1beta1.Params{{ + Name: "foo", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeObject, + ObjectVal: map[string]string{"bar": "baz"}, + }, + }}, + }, + }, + wantErr: apis.ErrGeneric("resolver requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"").Also( + apis.ErrGeneric("resolver params requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"")).Also( + apis.ErrGeneric("object type parameter requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"")), + withContext: cfgtesting.EnableStableAPIFields, }} for _, tc := range tests { @@ -145,10 +185,15 @@ func TestPipelineRef_Valid(t *testing.T) { name: "no pipelineRef", ref: nil, }, { - name: "valid resolver", + name: "beta feature: valid resolver", + ref: &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, + wc: cfgtesting.EnableAlphaAPIFields, + }, { + name: "beta feature: valid resolver with alpha flag", ref: &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, + wc: cfgtesting.EnableAlphaAPIFields, }, { - name: "valid resolver with params", + name: "beta feature: valid resolver with params with alpha flag", ref: &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git", Params: v1beta1.Params{{ Name: "repo", Value: v1beta1.ParamValue{ @@ -162,6 +207,7 @@ func TestPipelineRef_Valid(t *testing.T) { StringVal: "baz", }, }}}}, + wc: cfgtesting.EnableAlphaAPIFields, }} for _, ts := range tests { diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go index 56d44138805..4c641f6d804 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go @@ -71,6 +71,11 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) // Validate PipelineSpec if it's present if ps.PipelineSpec != nil { errs = errs.Also(ps.PipelineSpec.Validate(ctx).ViaField("pipelineSpec")) + // Validate beta fields separately for inline Pipeline definitions. + // This prevents validation from failing in the reconciler when a Pipeline is converted to a different API version. + // See https://github.com/tektoncd/pipeline/issues/6616 for more information. + // TODO(#6592): Decouple API versioning from feature versioning + errs = errs.Also(ps.PipelineSpec.ValidateBetaFields(ctx).ViaField("pipelineSpec")) } // Validate PipelineRun parameters diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go index b21439ef949..d0666d019a4 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go @@ -1557,3 +1557,208 @@ func TestPipelineRunWithTimeout_Validate(t *testing.T) { }) } } + +// TestPipelineRunSpecBetaFeatures tests the beta API-driven features +// of PipelineRunSpec are correctly governed `enable-api-fields`, which +// must be set to "alpha" or "beta". +func TestPipelineRunSpecBetaFeatures(t *testing.T) { + tts := []struct { + name string + spec v1beta1.PipelineSpec + }{{ + name: "array indexing in Tasks", + spec: v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{ + {Name: "first-param", Type: v1beta1.ParamTypeArray, Default: v1beta1.NewStructuredValues("default-value", "default-value-again")}, + }, + Tasks: []v1beta1.PipelineTask{{ + Name: "foo", + Params: v1beta1.Params{ + {Name: "first-task-first-param", Value: *v1beta1.NewStructuredValues("$(params.first-param[0])")}, + }, + TaskRef: &v1beta1.TaskRef{Name: "foo"}, + }}, + }, + }, { + name: "array indexing in Finally", + spec: v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{ + {Name: "first-param", Type: v1beta1.ParamTypeArray, Default: v1beta1.NewStructuredValues("default-value", "default-value-again")}, + }, + Tasks: []v1beta1.PipelineTask{{ + Name: "foo", + TaskRef: &v1beta1.TaskRef{Name: "foo"}, + }}, + Finally: []v1beta1.PipelineTask{{ + Name: "bar", + Params: v1beta1.Params{ + {Name: "first-task-first-param", Value: *v1beta1.NewStructuredValues("$(params.first-param[0])")}, + }, + TaskRef: &v1beta1.TaskRef{Name: "bar"}, + }}, + }, + }, { + name: "pipeline tasks - use of resolver", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "uses-resolver", + TaskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "bar"}}, + }}, + }, + }, { + name: "pipeline tasks - use of resolver params", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "uses-resolver-params", + TaskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "bar", Params: v1beta1.Params{{}}}}, + }}, + }, + }, { + name: "finally tasks - use of resolver", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &v1beta1.TaskRef{Name: "foo-task"}, + }}, + Finally: []v1beta1.PipelineTask{{ + Name: "uses-resolver", + TaskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "bar"}}, + }}, + }, + }, { + name: "finally tasks - use of resolver params", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &v1beta1.TaskRef{Name: "foo-task"}, + }}, + Finally: []v1beta1.PipelineTask{{ + Name: "uses-resolver-params", + TaskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "bar", Params: v1beta1.Params{{}}}}, + }}, + }, + }, { + name: "object params", + spec: v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{ + {Name: "first-param", Type: v1beta1.ParamTypeObject, Properties: map[string]v1beta1.PropertySpec{}}, + }, + Tasks: []v1beta1.PipelineTask{{ + Name: "foo", + TaskRef: &v1beta1.TaskRef{Name: "foo"}, + }}, + }, + }, { + name: "object params in Tasks", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "valid-pipeline-task", + TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Image: "busybox", Script: "echo hello"}}, + Params: []v1beta1.ParamSpec{{Name: "my-object-param", Type: v1beta1.ParamTypeObject, Properties: map[string]v1beta1.PropertySpec{}}}, + }}, + }}, + }, + }, { + name: "object params in Finally", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "foo", + TaskRef: &v1beta1.TaskRef{Name: "foo"}, + }}, + Finally: []v1beta1.PipelineTask{{ + Name: "valid-finally-task", + TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Image: "busybox", Script: "echo hello"}}, + Params: []v1beta1.ParamSpec{{Name: "my-object-param", Type: v1beta1.ParamTypeObject, Properties: map[string]v1beta1.PropertySpec{}}}, + }}, + }}, + }, + }, { + name: "array results", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &v1beta1.TaskRef{Name: "foo-task"}, + }}, + Results: []v1beta1.PipelineResult{{Name: "my-array-result", Type: v1beta1.ResultsTypeArray, Value: *v1beta1.NewStructuredValues("$(tasks.valid-pipeline-task.results.foo[*])")}}, + }, + }, { + name: "array results in Tasks", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "valid-pipeline-task", + TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Image: "busybox", Script: "echo hello"}}, + Results: []v1beta1.TaskResult{{Name: "my-array-result", Type: v1beta1.ResultsTypeArray}}, + }}, + }}, + }, + }, { + name: "array results in Finally", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &v1beta1.TaskRef{Name: "foo-task"}, + }}, + Finally: []v1beta1.PipelineTask{{ + Name: "valid-finally-task", + TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Image: "busybox", Script: "echo hello"}}, + Results: []v1beta1.TaskResult{{Name: "my-array-result", Type: v1beta1.ResultsTypeArray}}, + }}, + }}, + }, + }, { + name: "object results", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &v1beta1.TaskRef{Name: "foo-task"}, + }}, + Results: []v1beta1.PipelineResult{{Name: "my-object-result", Type: v1beta1.ResultsTypeObject, Value: *v1beta1.NewStructuredValues("$(tasks.valid-pipeline-task.results.foo[*])")}}, + }, + }, { + name: "object results in Tasks", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "valid-pipeline-task", + TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Image: "busybox", Script: "echo hello"}}, + Results: []v1beta1.TaskResult{{Name: "my-object-result", Type: v1beta1.ResultsTypeObject, Properties: map[string]v1beta1.PropertySpec{}}}, + }}, + }}, + }, + }, { + name: "object results in Finally", + spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &v1beta1.TaskRef{Name: "foo-task"}, + }}, + Finally: []v1beta1.PipelineTask{{ + Name: "valid-finally-task", + TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Image: "busybox", Script: "echo hello"}}, + Results: []v1beta1.TaskResult{{Name: "my-object-result", Type: v1beta1.ResultsTypeObject, Properties: map[string]v1beta1.PropertySpec{}}}, + }}, + }}, + }, + }} + for _, tt := range tts { + t.Run(tt.name, func(t *testing.T) { + pr := v1beta1.PipelineRun{ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: v1beta1.PipelineRunSpec{ + PipelineSpec: &tt.spec, + }} + ctx := cfgtesting.EnableStableAPIFields(context.Background()) + if err := pr.Validate(ctx); err == nil { + t.Errorf("no error when using beta field when `enable-api-fields` is stable") + } + + ctx = cfgtesting.EnableBetaAPIFields(context.Background()) + if err := pr.Validate(ctx); err != nil { + t.Errorf("unexpected error when using beta field: %s", err) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index cfdd84433c6..74f78d05afc 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -64,7 +64,12 @@ func (t *Task) Validate(ctx context.Context) *apis.FieldError { errs = errs.Also(t.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec")) // When a Task is created directly, instead of declared inline in a TaskRun or PipelineRun, // we do not support propagated parameters. Validate that all params it uses are declared. - return errs.Also(ValidateUsageOfDeclaredParameters(ctx, t.Spec.Steps, t.Spec.Params).ViaField("spec")) + errs = errs.Also(ValidateUsageOfDeclaredParameters(ctx, t.Spec.Steps, t.Spec.Params).ViaField("spec")) + // Validate beta fields when a Pipeline is defined, but not as part of validating a Pipeline spec. + // This prevents validation from failing when a Pipeline is converted to a different API version. + // See https://github.com/tektoncd/pipeline/issues/6616 for more information. + errs = errs.Also(t.Spec.ValidateBetaFields(ctx)) + return errs } // Validate implements apis.Validatable @@ -98,6 +103,35 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) { return errs } +// ValidateBetaFields returns an error if the TaskSpec uses beta specifications governed by +// `enable-api-fields` but does not have "enable-api-fields" set to "alpha" or "beta". +func (ts *TaskSpec) ValidateBetaFields(ctx context.Context) *apis.FieldError { + var errs *apis.FieldError + // Object parameters + for i, p := range ts.Params { + if p.Type == ParamTypeObject { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "object type parameter", config.BetaAPIFields).ViaFieldIndex("params", i)) + } + } + // Indexing into array parameters + arrayIndexParamRefs := ts.GetIndexingReferencesToArrayParams() + if len(arrayIndexParamRefs) != 0 { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "indexing into array parameters", config.BetaAPIFields)) + } + // Array and object results + for i, result := range ts.Results { + switch result.Type { + case ResultsTypeObject: + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "object results", config.BetaAPIFields).ViaFieldIndex("results", i)) + case ResultsTypeArray: + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "array results", config.BetaAPIFields).ViaFieldIndex("results", i)) + case ResultsTypeString: + default: + } + } + return errs +} + // ValidateUsageOfDeclaredParameters validates that all parameters referenced in the Task are declared by the Task. func ValidateUsageOfDeclaredParameters(ctx context.Context, steps []Step, params ParamSpecs) *apis.FieldError { var errs *apis.FieldError diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index c89bc74a8ae..484c90cb748 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -1707,6 +1707,76 @@ func TestIncompatibleAPIVersions(t *testing.T) { } } +func TestTaskBetaFields(t *testing.T) { + tests := []struct { + name string + spec v1beta1.TaskSpec + }{{ + name: "array param indexing", + spec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{Name: "foo", Type: v1beta1.ParamTypeArray}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo $(params.foo[1])`, + }}, + }, + }, { + name: "object params", + spec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{Name: "foo", Type: v1beta1.ParamTypeObject, Properties: map[string]v1beta1.PropertySpec{"bar": {Type: v1beta1.ParamTypeString}}}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo $(params.foo.bar)`, + }}, + }, + }, { + name: "array results", + spec: v1beta1.TaskSpec{ + Results: []v1beta1.TaskResult{{Name: "array-result", Type: v1beta1.ResultsTypeArray}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo -n "[\"hello\",\"world\"]" | tee $(results.array-result.path)`, + }}, + }, + }, { + name: "object results", + spec: v1beta1.TaskSpec{ + Results: []v1beta1.TaskResult{{Name: "object-result", Type: v1beta1.ResultsTypeObject, + Properties: map[string]v1beta1.PropertySpec{}}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo -n "{\"hello\":\"world\"}" | tee $(results.object-result.path)`, + }}, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := cfgtesting.EnableStableAPIFields(context.Background()) + task := v1beta1.Task{ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: tt.spec} + if err := task.Validate(ctx); err == nil { + t.Errorf("no error when using beta field when `enable-api-fields` is stable") + } + + ctx = cfgtesting.EnableBetaAPIFields(context.Background()) + if err := task.Validate(ctx); err != nil { + t.Errorf("unexpected error when using beta field: %s", err) + } + }) + } +} + func TestGetArrayIndexParamRefs(t *testing.T) { tcs := []struct { name string diff --git a/pkg/apis/pipeline/v1beta1/taskref_validation.go b/pkg/apis/pipeline/v1beta1/taskref_validation.go index 4e42b7aa2e2..a8bbe480ae8 100644 --- a/pkg/apis/pipeline/v1beta1/taskref_validation.go +++ b/pkg/apis/pipeline/v1beta1/taskref_validation.go @@ -21,6 +21,7 @@ import ( "strings" "github.com/google/go-containerregistry/pkg/name" + "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" ) @@ -35,6 +36,7 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { switch { case ref.Resolver != "" || ref.Params != nil: if ref.Resolver != "" { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) if ref.Name != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) } @@ -43,6 +45,7 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { } } if ref.Params != nil { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) if ref.Name != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "params")) } diff --git a/pkg/apis/pipeline/v1beta1/taskref_validation_test.go b/pkg/apis/pipeline/v1beta1/taskref_validation_test.go index 424f40c3dd3..8ec39f75fa9 100644 --- a/pkg/apis/pipeline/v1beta1/taskref_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/taskref_validation_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/test/diff" "knative.dev/pkg/apis" @@ -37,10 +38,15 @@ func TestTaskRef_Valid(t *testing.T) { name: "simple taskref", taskRef: &v1beta1.TaskRef{Name: "taskrefname"}, }, { - name: "valid resolver", + name: "beta feature: valid resolver", taskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, + wc: cfgtesting.EnableBetaAPIFields, }, { - name: "valid resolver with params", + name: "beta feature: valid resolver with alpha flag", + taskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, + wc: cfgtesting.EnableAlphaAPIFields, + }, { + name: "beta feature: valid resolver with params", taskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git", Params: v1beta1.Params{{ Name: "repo", Value: v1beta1.ParamValue{ @@ -171,6 +177,24 @@ func TestTaskRef_Invalid(t *testing.T) { Message: `invalid value: name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`, Paths: []string{"name"}, }, + }, { + name: "taskref param object requires beta", + taskRef: &v1beta1.TaskRef{ + ResolverRef: v1beta1.ResolverRef{ + Resolver: "some-resolver", + Params: v1beta1.Params{{ + Name: "foo", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeObject, + ObjectVal: map[string]string{"bar": "baz"}, + }, + }}, + }, + }, + wc: cfgtesting.EnableStableAPIFields, + wantErr: apis.ErrGeneric("resolver requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"").Also( + apis.ErrGeneric("resolver params requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"")).Also( + apis.ErrGeneric("object type parameter requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"")), }} for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation.go b/pkg/apis/pipeline/v1beta1/taskrun_validation.go index e5108a6464c..31382e964d3 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation.go @@ -62,6 +62,10 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) { // Validate TaskSpec if it's present. if ts.TaskSpec != nil { errs = errs.Also(ts.TaskSpec.Validate(ctx).ViaField("taskSpec")) + // Validate beta fields separately for inline Task definitions. + // This prevents validation from failing in the reconciler when a Task is converted to a different API version. + // See https://github.com/tektoncd/pipeline/issues/6616 for more information. + errs = errs.Also(ts.TaskSpec.ValidateBetaFields(ctx).ViaField("taskSpec")) } errs = errs.Also(ValidateParameters(ctx, ts.Params).ViaField("params")) @@ -239,6 +243,11 @@ func ValidateWorkspaceBindings(ctx context.Context, wb []WorkspaceBinding) (errs func ValidateParameters(ctx context.Context, params Params) (errs *apis.FieldError) { var names []string for _, p := range params { + if p.Value.Type == ParamTypeObject { + // Object type parameter is a beta feature and will fail validation if it's used in a taskrun spec + // when the enable-api-fields feature gate is not "alpha" or "beta". + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "object type parameter", config.BetaAPIFields)) + } names = append(names, p.Name) } return errs.Also(validateNoDuplicateNames(names, false)) diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go index 6e604fb6e36..3207331b0fd 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go @@ -904,3 +904,75 @@ func TestTaskRunSpec_Validate(t *testing.T) { }) } } + +func TestTaskRunBetaFields(t *testing.T) { + tests := []struct { + name string + spec v1beta1.TaskSpec + }{{ + name: "array param indexing", + spec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{Name: "foo", Type: v1beta1.ParamTypeArray}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo $(params.foo[1])`, + }}, + }, + }, { + name: "object params", + spec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{Name: "foo", Type: v1beta1.ParamTypeObject, Properties: map[string]v1beta1.PropertySpec{"bar": {Type: v1beta1.ParamTypeString}}}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo $(params.foo.bar)`, + }}, + }, + }, { + name: "array results", + spec: v1beta1.TaskSpec{ + Results: []v1beta1.TaskResult{{Name: "array-result", Type: v1beta1.ResultsTypeArray}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo -n "[\"hello\",\"world\"]" | tee $(results.array-result.path)`, + }}, + }, + }, { + name: "object results", + spec: v1beta1.TaskSpec{ + Results: []v1beta1.TaskResult{{Name: "object-result", Type: v1beta1.ResultsTypeObject, + Properties: map[string]v1beta1.PropertySpec{}}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo -n "{\"hello\":\"world\"}" | tee $(results.object-result.path)`, + }}, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := cfgtesting.EnableStableAPIFields(context.Background()) + tr := v1beta1.TaskRun{ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: v1beta1.TaskRunSpec{ + TaskSpec: &tt.spec, + }} + if err := tr.Validate(ctx); err == nil { + t.Errorf("no error when using beta field when `enable-api-fields` is stable") + } + + ctx = cfgtesting.EnableBetaAPIFields(context.Background()) + if err := tr.Validate(ctx); err != nil { + t.Errorf("unexpected error when using beta field: %s", err) + } + }) + } +}