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: tektoncd#6592
JeromeJu · Aug 30, 2023 · 687b12a · 687b12a
1 parent 8c6bd54
commit 687b12a
Show file tree

Hide file tree

Showing 14 changed files with 757 additions and 27 deletions.
diff --git a/api_compatibility_policy.md b/api_compatibility_policy.md
@@ -90,26 +90,27 @@ 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. The current group api-driven feature flag `enable-api-fields` gates the overall stability levels of features in the CRDs and their default behaviors. These fields may be disabled by default and can be enabled by setting the `enable-api-fields` feature-flag value:
 
-* `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 |
+
+_Note that the alpha, beta features do not depend on apiVersions. i.e. beta api fields would need `enable-api-fields=beta` to be turned on in all CRD apiVersions._
 
 ### 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 +126,12 @@ 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 features are enabled by default
+- GA/Stable features 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.
 

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go
@@ -223,7 +223,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",
@@ -242,11 +242,13 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) {
  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{
@@ -261,8 +263,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
@@ -52,7 +52,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
@@ -87,6 +92,60 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
  return errs
 }
 
+// ValidateBetaFields returns an error if the Pipeline spec uses beta features 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(version.ValidateEnabledAPIFields(ctx, "object type parameter", config.BetaAPIFields).ViaFieldIndex("params", i))
+ }
+ }
+ // Indexing into array parameters
+ arrayParamIndexingRefs := ps.GetIndexingReferencesToArrayParams()
+ if len(arrayParamIndexingRefs) != 0 {
+ errs = errs.Also(version.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(version.ValidateEnabledAPIFields(ctx, "object results", config.BetaAPIFields).ViaFieldIndex("results", i))
+ case ResultsTypeArray:
+ errs = errs.Also(version.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("tasks", 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(version.ValidateEnabledAPIFields(ctx, "taskref.resolver", config.BetaAPIFields))
+ }
+ if len(pt.TaskRef.Params) > 0 {
+ errs = errs.Also(version.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
@@ -24,6 +24,7 @@ import (
  "github.com/google/go-cmp/cmp"
  "github.com/google/go-cmp/cmp/cmpopts"
  "github.com/tektoncd/pipeline/pkg/apis/config"
+ cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing"
  "github.com/tektoncd/pipeline/test/diff"
  corev1 "k8s.io/api/core/v1"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -3409,6 +3410,206 @@ func enableFeatures(t *testing.T, features []string) func(context.Context) conte
  }
 }
 
+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 without the feature flag set",
+ spec: PipelineSpec{
+ Tasks: []PipelineTask{{
+ Name: "uses-resolver",
+ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}},
+ }},
+ },
+ }, {
+ name: "pipeline tasks - use of resolver params without the feature flag set",
+ spec: PipelineSpec{
+ Tasks: []PipelineTask{{
+ Name: "uses-resolver-params",
+ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar", Params: Params{{}}}},
+ }},
+ },
+ }, {
+ name: "finally tasks - use of resolver without the feature flag set",
+ 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 without the feature flag set",
+ 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