Skip to content

Commit

Permalink
Refactor version.ValidateEnabledAPIFields to config pkg
Browse files Browse the repository at this point in the history
This commit refactors the ValidateEnabledAPIFields function to the
config package. The validation previously lived in v1beta1 and it was
moved to the version pkg which functions as API version conversion
utils rather than for feature versions. This commit moves it to the
config pkg since it is actually more tied with config validation of
the  feature flag. version.ValidateEnabledAPIFields has been
deprecated and aliased to keep backwards compatibility.

/kind misc
  • Loading branch information
JeromeJu authored and tekton-robot committed Oct 13, 2023
1 parent d26a45c commit 5d403c6
Show file tree
Hide file tree
Showing 14 changed files with 194 additions and 65 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,40 +14,39 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package version
package config

import (
"context"
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/config"
"knative.dev/pkg/apis"
)

// ValidateEnabledAPIFields checks that the enable-api-fields feature gate is set
// to a version at most as stable as wantVersion, if not, returns an error stating which feature
// is dependent on the version and what the current version actually is.
func ValidateEnabledAPIFields(ctx context.Context, featureName string, wantVersion string) *apis.FieldError {
currentVersion := config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields
currentVersion := FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields
var errs *apis.FieldError
message := `%s requires "enable-api-fields" feature gate to be %s but it is %q`
switch wantVersion {
case config.StableAPIFields:
case StableAPIFields:
// If the feature is stable, it doesn't matter what the current version is
case config.BetaAPIFields:
case BetaAPIFields:
// If the feature requires "beta" fields to be enabled, the current version may be "beta" or "alpha"
if currentVersion != config.BetaAPIFields && currentVersion != config.AlphaAPIFields {
message = fmt.Sprintf(message, featureName, fmt.Sprintf("%q or %q", config.AlphaAPIFields, config.BetaAPIFields), currentVersion)
if currentVersion != BetaAPIFields && currentVersion != AlphaAPIFields {
message = fmt.Sprintf(message, featureName, fmt.Sprintf("%q or %q", AlphaAPIFields, BetaAPIFields), currentVersion)
errs = apis.ErrGeneric(message)
}
case config.AlphaAPIFields:
case AlphaAPIFields:
// If the feature requires "alpha" fields to be enabled, the current version must be "alpha"
if currentVersion != wantVersion {
message = fmt.Sprintf(message, featureName, fmt.Sprintf("%q", config.AlphaAPIFields), currentVersion)
message = fmt.Sprintf(message, featureName, fmt.Sprintf("%q", AlphaAPIFields), currentVersion)
errs = apis.ErrGeneric(message)
}
default:
errs = apis.ErrGeneric("invalid wantVersion %s, must be one of (%s, %s, %s)", wantVersion, config.AlphaAPIFields, config.BetaAPIFields, config.StableAPIFields)
errs = apis.ErrGeneric("invalid wantVersion %s, must be one of (%s, %s, %s)", wantVersion, AlphaAPIFields, BetaAPIFields, StableAPIFields)
}
return errs
}
116 changes: 116 additions & 0 deletions pkg/apis/config/featureflags_validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
Copyright 2021 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
http://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.
*/

package config_test

import (
"context"
"testing"

"github.com/tektoncd/pipeline/pkg/apis/config"
)

func TestValidateEnabledAPIFields(t *testing.T) {
tcs := []struct {
name string
wantVersion string
currentVersion string
}{{
name: "alpha fields w/ alpha",
wantVersion: "alpha",
currentVersion: "alpha",
}, {
name: "beta fields w/ alpha",
wantVersion: "beta",
currentVersion: "alpha",
}, {
name: "beta fields w/ beta",
wantVersion: "beta",
currentVersion: "beta",
}, {
name: "stable fields w/ alpha",
wantVersion: "stable",
currentVersion: "alpha",
}, {
name: "stable fields w/ beta",
wantVersion: "stable",
currentVersion: "beta",
}, {
name: "stable fields w/ stable",
wantVersion: "stable",
currentVersion: "stable",
}}
for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
flags, err := config.NewFeatureFlagsFromMap(map[string]string{
"enable-api-fields": tc.currentVersion,
})
if err != nil {
t.Fatalf("error creating feature flags from map: %v", err)
}
cfg := &config.Config{
FeatureFlags: flags,
}
ctx := config.ToContext(context.Background(), cfg)
if err := config.ValidateEnabledAPIFields(ctx, "test feature", tc.wantVersion); err != nil {
t.Errorf("unexpected error for compatible feature gates: %q", err)
}
})
}
}

func TestValidateEnabledAPIFieldsError(t *testing.T) {
tcs := []struct {
name string
wantVersion string
currentVersion string
}{{
name: "alpha fields w/ stable",
wantVersion: "alpha",
currentVersion: "stable",
}, {
name: "alpha fields w/ beta",
wantVersion: "alpha",
currentVersion: "beta",
}, {
name: "beta fields w/ stable",
wantVersion: "beta",
currentVersion: "stable",
}, {
name: "invalid wantVersion",
wantVersion: "foo",
currentVersion: "stable",
}}
for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
flags, err := config.NewFeatureFlagsFromMap(map[string]string{
"enable-api-fields": tc.currentVersion,
})
if err != nil {
t.Fatalf("error creating feature flags from map: %v", err)
}
cfg := &config.Config{
FeatureFlags: flags,
}
ctx := config.ToContext(context.Background(), cfg)
fieldErr := config.ValidateEnabledAPIFields(ctx, "test feature", tc.wantVersion)

if fieldErr == nil {
t.Errorf("error expected for incompatible feature gates")
}
})
}
}
19 changes: 9 additions & 10 deletions pkg/apis/pipeline/v1/pipeline_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/validate"
"github.com/tektoncd/pipeline/pkg/apis/version"
"github.com/tektoncd/pipeline/pkg/reconciler/pipeline/dag"
"github.com/tektoncd/pipeline/pkg/substitution"
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
Expand Down Expand Up @@ -103,21 +102,21 @@ func (ps *PipelineSpec) ValidateBetaFields(ctx context.Context) *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))
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(version.ValidateEnabledAPIFields(ctx, "indexing into array parameters", config.BetaAPIFields))
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(version.ValidateEnabledAPIFields(ctx, "object results", config.BetaAPIFields).ViaFieldIndex("results", i))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "object results", config.BetaAPIFields).ViaFieldIndex("results", i))
case ResultsTypeArray:
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "array results", config.BetaAPIFields).ViaFieldIndex("results", i))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "array results", config.BetaAPIFields).ViaFieldIndex("results", i))
case ResultsTypeString:
default:
}
Expand All @@ -139,10 +138,10 @@ func (pt *PipelineTask) validateBetaFields(ctx context.Context) *apis.FieldError
if pt.TaskRef != nil {
// Resolvers
if pt.TaskRef.Resolver != "" {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "taskref.resolver", config.BetaAPIFields))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "taskref.resolver", config.BetaAPIFields))
}
if len(pt.TaskRef.Params) > 0 {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "taskref.params", config.BetaAPIFields))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "taskref.params", config.BetaAPIFields))
}
} else if pt.TaskSpec != nil {
errs = errs.Also(pt.TaskSpec.ValidateBetaFields(ctx))
Expand Down Expand Up @@ -232,7 +231,7 @@ func (pt *PipelineTask) validateMatrix(ctx context.Context) (errs *apis.FieldErr
if pt.IsMatrixed() {
// This is an alpha feature and will fail validation if it's used in a pipeline spec
// when the enable-api-fields feature gate is anything but "alpha".
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "matrix", config.AlphaAPIFields))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "matrix", config.AlphaAPIFields))
errs = errs.Also(pt.Matrix.validateCombinationsCount(ctx))
errs = errs.Also(pt.Matrix.validateUniqueParams())
}
Expand Down Expand Up @@ -307,11 +306,11 @@ func (pt PipelineTask) validateRefOrSpec(ctx context.Context) (errs *apis.FieldE
nonNilFields = append(nonNilFields, taskSpec)
}
if pt.PipelineRef != nil {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, pipelineRef, config.AlphaAPIFields))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, pipelineRef, config.AlphaAPIFields))
nonNilFields = append(nonNilFields, pipelineRef)
}
if pt.PipelineSpec != nil {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, pipelineSpec, config.AlphaAPIFields))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, pipelineSpec, config.AlphaAPIFields))
nonNilFields = append(nonNilFields, pipelineSpec)
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/apis/pipeline/v1/pipelineref_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/version"
"knative.dev/pkg/apis"
)

Expand All @@ -33,13 +32,13 @@ func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) {

if ref.Resolver != "" || ref.Params != nil {
if ref.Resolver != "" {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver"))
if ref.Name != "" {
errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver"))
}
}
if ref.Params != nil {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params"))
if ref.Name != "" {
errs = errs.Also(apis.ErrMultipleOneOf("name", "params"))
}
Expand Down
7 changes: 3 additions & 4 deletions pkg/apis/pipeline/v1/pipelinerun_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/validate"
"github.com/tektoncd/pipeline/pkg/apis/version"
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
Expand Down Expand Up @@ -283,15 +282,15 @@ func (ps *PipelineRunSpec) validatePipelineTimeout(timeout time.Duration, errorM

func validateTaskRunSpec(ctx context.Context, trs PipelineTaskRunSpec) (errs *apis.FieldError) {
if trs.StepSpecs != nil {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "stepSpecs", config.AlphaAPIFields).ViaField("stepSpecs"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "stepSpecs", config.AlphaAPIFields).ViaField("stepSpecs"))
errs = errs.Also(validateStepSpecs(trs.StepSpecs).ViaField("stepSpecs"))
}
if trs.SidecarSpecs != nil {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "sidecarSpecs", config.AlphaAPIFields).ViaField("sidecarSpecs"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "sidecarSpecs", config.AlphaAPIFields).ViaField("sidecarSpecs"))
errs = errs.Also(validateSidecarSpecs(trs.SidecarSpecs).ViaField("sidecarSpecs"))
}
if trs.ComputeResources != nil {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "computeResources", config.AlphaAPIFields).ViaField("computeResources"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "computeResources", config.AlphaAPIFields).ViaField("computeResources"))
errs = errs.Also(validateTaskRunComputeResources(trs.ComputeResources, trs.StepSpecs))
}
if trs.PodTemplate != nil {
Expand Down
19 changes: 9 additions & 10 deletions pkg/apis/pipeline/v1/task_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/validate"
"github.com/tektoncd/pipeline/pkg/apis/version"
"github.com/tektoncd/pipeline/pkg/substitution"
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -109,21 +108,21 @@ func (ts *TaskSpec) ValidateBetaFields(ctx context.Context) *apis.FieldError {
// Object parameters
for i, p := range ts.Params {
if p.Type == ParamTypeObject {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "object type parameter", config.BetaAPIFields).ViaFieldIndex("params", i))
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(version.ValidateEnabledAPIFields(ctx, "indexing into array parameters", config.BetaAPIFields))
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(version.ValidateEnabledAPIFields(ctx, "object results", config.BetaAPIFields).ViaFieldIndex("results", i))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "object results", config.BetaAPIFields).ViaFieldIndex("results", i))
case ResultsTypeArray:
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "array results", config.BetaAPIFields).ViaFieldIndex("results", i))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "array results", config.BetaAPIFields).ViaFieldIndex("results", i))
case ResultsTypeString:
default:
}
Expand Down Expand Up @@ -222,7 +221,7 @@ func validateWorkspaceUsages(ctx context.Context, ts *TaskSpec) (errs *apis.Fiel

for stepIdx, step := range steps {
if len(step.Workspaces) != 0 {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "step workspaces", config.BetaAPIFields).ViaIndex(stepIdx).ViaField("steps"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "step workspaces", config.BetaAPIFields).ViaIndex(stepIdx).ViaField("steps"))
}
for workspaceIdx, w := range step.Workspaces {
if !wsNames.Has(w.Name) {
Expand All @@ -233,7 +232,7 @@ func validateWorkspaceUsages(ctx context.Context, ts *TaskSpec) (errs *apis.Fiel

for sidecarIdx, sidecar := range sidecars {
if len(sidecar.Workspaces) != 0 {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "sidecar workspaces", config.BetaAPIFields).ViaIndex(sidecarIdx).ViaField("sidecars"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "sidecar workspaces", config.BetaAPIFields).ViaIndex(sidecarIdx).ViaField("sidecars"))
}
for workspaceIdx, w := range sidecar.Workspaces {
if !wsNames.Has(w.Name) {
Expand Down Expand Up @@ -325,19 +324,19 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi
if s.Script != "" {
cleaned := strings.TrimSpace(s.Script)
if strings.HasPrefix(cleaned, "#!win") {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "windows script support", config.AlphaAPIFields).ViaField("script"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "windows script support", config.AlphaAPIFields).ViaField("script"))
}
}

// StdoutConfig is an alpha feature and will fail validation if it's used in a task spec
// when the enable-api-fields feature gate is not "alpha".
if s.StdoutConfig != nil {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "step stdout stream support", config.AlphaAPIFields).ViaField("stdoutconfig"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "step stdout stream support", config.AlphaAPIFields).ViaField("stdoutconfig"))
}
// StderrConfig is an alpha feature and will fail validation if it's used in a task spec
// when the enable-api-fields feature gate is not "alpha".
if s.StderrConfig != nil {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "step stderr stream support", config.AlphaAPIFields).ViaField("stderrconfig"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "step stderr stream support", config.AlphaAPIFields).ViaField("stderrconfig"))
}
return errs
}
Expand Down
5 changes: 2 additions & 3 deletions pkg/apis/pipeline/v1/taskref_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"strings"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/version"
"k8s.io/apimachinery/pkg/util/validation"
"knative.dev/pkg/apis"
)
Expand All @@ -36,13 +35,13 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) {
switch {
case ref.Resolver != "" || ref.Params != nil:
if ref.Resolver != "" {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver"))
if ref.Name != "" {
errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver"))
}
}
if ref.Params != nil {
errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params"))
errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params"))
if ref.Name != "" {
errs = errs.Also(apis.ErrMultipleOneOf("name", "params"))
}
Expand Down
Loading

0 comments on commit 5d403c6

Please sign in to comment.