Skip to content

Commit

Permalink
Update existent beta feature validation in v1beta1
Browse files Browse the repository at this point in the history 
This commit updates the existent beta feature validation in v1beta1
to decouple api and feature versioning. Prior to this change, beta
features are validated only in v1 api but not in v1beta1. This PR
sync the difference between these apiVersions. Now all beta features
would require `enable-api-fields` to be set as `beta` to be turned on
regardless of apiVersions.
  • Loading branch information
@JeromeJu
JeromeJu committed Jul 21, 2023
1 parent b3cd760 commit a32e4c8
Show file tree
Hide file tree
Showing 9 changed files with 372 additions and 8 deletions.
8 changes: 4 additions & 4 deletions pkg/apis/pipeline/v1beta1/pipeline_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,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",
Expand All @@ -240,11 +240,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{
Expand All @@ -259,9 +261,7 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) {
cfg := &config.Config{
FeatureFlags: &config.FeatureFlags{},
}
if tt.enableAPIFields {
cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields
}
cfg.FeatureFlags.EnableAPIFields = tt.enableAPIFields
if tt.enableBundles {
cfg.FeatureFlags.EnableTektonOCIBundles = true
}
Expand Down
55 changes: 55 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ 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"))
errs = errs.Also(p.Spec.ValidateBetaFields(ctx))
return errs.Also(p.Spec.validatePipelineWorkspacesUsage().ViaField("spec"))
}

Expand Down Expand Up @@ -87,6 +88,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 && !config.CheckAlphaOrBetaAPIFields(ctx) {
errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("cannot index into array parameters when 'enable-api-fields' is 'stable', but found indexing references: %s", arrayParamIndexingRefs)))
}
// 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 {
Expand Down
200 changes: 200 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3409,6 +3409,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 := config.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 = config.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
Expand Down
30 changes: 30 additions & 0 deletions pkg/apis/pipeline/v1beta1/task_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ 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.
errs = errs.Also(t.Spec.ValidateBetaFields(ctx))
return errs.Also(ValidateUsageOfDeclaredParameters(ctx, t.Spec.Steps, t.Spec.Params).ViaField("spec"))
}

Expand Down Expand Up @@ -99,6 +100,35 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
return errs
}

// ValidateBetaFields returns an error if the Task spec uses beta features 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(version.ValidateEnabledAPIFields(ctx, "object type parameter", config.BetaAPIFields).ViaFieldIndex("params", i))
}
}
// Indexing into array parameters
arrayIndexParamRefs := ts.GetIndexingReferencesToArrayParams()
if len(arrayIndexParamRefs) != 0 && !config.CheckAlphaOrBetaAPIFields(ctx) {
errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("cannot index into array parameters when 'enable-api-fields' is 'stable', but found indexing references: %s", arrayIndexParamRefs)))
}
// 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))
case ResultsTypeArray:
errs = errs.Also(version.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
Expand Down
Loading

0 comments on commit a32e4c8

Please sign in to comment.