From 19d01852140946f30451392b4fdff4aad162a021 Mon Sep 17 00:00:00 2001 From: Lee Bernick Date: Mon, 27 Jun 2022 13:19:26 -0400 Subject: [PATCH] Clean up validation for task and pipeline refs This commit creates separate files for testing validation of Task and Pipeline references, and moves test cases related to this validation into those files. It also updates the error message for when a bundle is included in a reference without the "enable-tekton-oci-bundles" flag set to "true" to instruct the user to set that flag. In addition, it updates the error message for when resolvers are set without the "enable-api-fields" flag set to "alpha" to instruct the user to set that flag. --- .../pipeline/v1beta1/openapi_generated.go | 4 +- .../pipeline/v1beta1/pipelineref_types.go | 35 +++ .../v1beta1/pipelineref_validation.go | 77 +++---- .../v1beta1/pipelineref_validation_test.go | 200 ++++++++++++++++++ .../pipeline/v1beta1/pipelinerun_types.go | 19 -- .../v1beta1/pipelinerun_validation_test.go | 148 ------------- pkg/apis/pipeline/v1beta1/swagger.json | 4 +- pkg/apis/pipeline/v1beta1/task_types.go | 33 --- pkg/apis/pipeline/v1beta1/taskref_types.go | 49 +++++ .../pipeline/v1beta1/taskref_validation.go | 76 ++----- .../v1beta1/taskref_validation_test.go | 189 +++++++++++++++++ .../v1beta1/taskrun_validation_test.go | 161 -------------- 12 files changed, 528 insertions(+), 467 deletions(-) create mode 100644 pkg/apis/pipeline/v1beta1/pipelineref_types.go create mode 100644 pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go create mode 100644 pkg/apis/pipeline/v1beta1/taskref_types.go create mode 100644 pkg/apis/pipeline/v1beta1/taskref_validation_test.go diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index eead110f31e..113bfc69e98 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -1109,7 +1109,7 @@ func schema_pkg_apis_pipeline_v1beta1_PipelineRef(ref common.ReferenceCallback) return common.OpenAPIDefinition{ Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ - Description: "PipelineRef can be used to refer to a specific instance of a Pipeline. Copied from CrossVersionObjectReference: https://github.com/kubernetes/kubernetes/blob/169df7434155cbbc22f1532cba8e0a9588e29ad8/pkg/apis/autoscaling/types.go#L64", + Description: "PipelineRef can be used to refer to a specific instance of a Pipeline.", Type: []string{"object"}, Properties: map[string]spec.Schema{ "name": { @@ -3763,7 +3763,7 @@ func schema_pkg_apis_pipeline_v1beta1_TaskRef(ref common.ReferenceCallback) comm return common.OpenAPIDefinition{ Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ - Description: "TaskRef can be used to refer to a specific instance of a task. Copied from CrossVersionObjectReference: https://github.com/kubernetes/kubernetes/blob/169df7434155cbbc22f1532cba8e0a9588e29ad8/pkg/apis/autoscaling/types.go#L64", + Description: "TaskRef can be used to refer to a specific instance of a task.", Type: []string{"object"}, Properties: map[string]spec.Schema{ "name": { diff --git a/pkg/apis/pipeline/v1beta1/pipelineref_types.go b/pkg/apis/pipeline/v1beta1/pipelineref_types.go new file mode 100644 index 00000000000..17bd57428cc --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/pipelineref_types.go @@ -0,0 +1,35 @@ +/* +Copyright 2022 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 v1beta1 + +// PipelineRef can be used to refer to a specific instance of a Pipeline. +type PipelineRef struct { + // Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names + Name string `json:"name,omitempty"` + // API version of the referent + // +optional + APIVersion string `json:"apiVersion,omitempty"` + // Bundle url reference to a Tekton Bundle. + // +optional + Bundle string `json:"bundle,omitempty"` + + // ResolverRef allows referencing a Pipeline in a remote location + // like a git repo. This field is only supported when the alpha + // feature gate is enabled. + // +optional + ResolverRef `json:",omitempty"` +} diff --git a/pkg/apis/pipeline/v1beta1/pipelineref_validation.go b/pkg/apis/pipeline/v1beta1/pipelineref_validation.go index 1a61b7b219b..45300a54801 100644 --- a/pkg/apis/pipeline/v1beta1/pipelineref_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelineref_validation.go @@ -18,71 +18,58 @@ package v1beta1 import ( "context" + "fmt" "github.com/google/go-containerregistry/pkg/name" "github.com/tektoncd/pipeline/pkg/apis/config" + "github.com/tektoncd/pipeline/pkg/apis/version" "knative.dev/pkg/apis" ) // Validate ensures that a supplied PipelineRef field is populated // correctly. No errors are returned for a nil PipelineRef. func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) { - cfg := config.FromContextOrDefaults(ctx) if ref == nil { return } - if cfg.FeatureFlags.EnableAPIFields == config.AlphaAPIFields { - errs = errs.Also(ref.validateAlphaRef(ctx)) - } else { - errs = errs.Also(ref.validateInTreeRef(ctx)) - } - return -} - -// validateInTreeRef returns errors if the given pipelineRef is not -// valid for Pipelines' built-in resolution machinery. -func (ref *PipelineRef) validateInTreeRef(ctx context.Context) (errs *apis.FieldError) { - cfg := config.FromContextOrDefaults(ctx) - if ref.Resolver != "" { - errs = errs.Also(apis.ErrDisallowedFields("resolver")) - } - if ref.Resource != nil { - errs = errs.Also(apis.ErrDisallowedFields("resource")) - } - if ref.Name == "" { - errs = errs.Also(apis.ErrMissingField("name")) - } - if cfg.FeatureFlags.EnableTektonOCIBundles { - if ref.Bundle != "" && ref.Name == "" { - errs = errs.Also(apis.ErrMissingField("name")) - } - if ref.Bundle != "" { - if _, err := name.ParseReference(ref.Bundle); err != nil { - errs = errs.Also(apis.ErrInvalidValue("invalid bundle reference", "bundle", err.Error())) - } - } - } else if ref.Bundle != "" { - errs = errs.Also(apis.ErrDisallowedFields("bundle")) - } - return -} -// validateAlphaRef ensures that the user has passed either a -// valid remote resource reference or a valid in-tree resource reference, -// but not both. -func (ref *PipelineRef) validateAlphaRef(ctx context.Context) (errs *apis.FieldError) { switch { - case ref.Resolver == "" && ref.Resource != nil: - errs = errs.Also(apis.ErrMissingField("resolver")) - case ref.Resolver == "": - errs = errs.Also(ref.validateInTreeRef(ctx)) - default: + case ref.Resolver != "": + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver", config.AlphaAPIFields).ViaField("resolver")) if ref.Name != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) } if ref.Bundle != "" { errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resolver")) } + case ref.Resource != nil: + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resource", config.AlphaAPIFields).ViaField("resource")) + if ref.Name != "" { + errs = errs.Also(apis.ErrMultipleOneOf("name", "resource")) + } + if ref.Bundle != "" { + errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resource")) + } + if ref.Resolver == "" { + errs = errs.Also(apis.ErrMissingField("resolver")) + } + case ref.Name == "": + errs = errs.Also(apis.ErrMissingField("name")) + case ref.Bundle != "": + errs = errs.Also(validateBundleFeatureFlag(ctx, "bundle", true).ViaField("bundle")) + if _, err := name.ParseReference(ref.Bundle); err != nil { + errs = errs.Also(apis.ErrInvalidValue("invalid bundle reference", "bundle", err.Error())) + } } return } + +func validateBundleFeatureFlag(ctx context.Context, featureName string, wantValue bool) *apis.FieldError { + flagValue := config.FromContextOrDefaults(ctx).FeatureFlags.EnableTektonOCIBundles + if flagValue != wantValue { + var errs *apis.FieldError + message := fmt.Sprintf(`%s requires "enable-tekton-oci-bundles" feature gate to be %t but it is %t`, featureName, wantValue, flagValue) + return errs.Also(apis.ErrGeneric(message)) + } + return nil +} diff --git a/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go new file mode 100644 index 00000000000..24101b4d0e2 --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go @@ -0,0 +1,200 @@ +/* +Copyright 2020 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 v1beta1_test + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/pipeline/test/diff" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/apis" + logtesting "knative.dev/pkg/logging/testing" +) + +func TestPipelineRef_Invalid(t *testing.T) { + tests := []struct { + name string + ref *v1beta1.PipelineRef + wantErr *apis.FieldError + withContext func(context.Context) context.Context + }{{ + name: "use of bundle without the feature flag set", + ref: &v1beta1.PipelineRef{ + Name: "my-pipeline", + Bundle: "docker.io/foo", + }, + wantErr: apis.ErrGeneric("bundle requires \"enable-tekton-oci-bundles\" feature gate to be true but it is false"), + }, { + name: "bundle missing name", + ref: &v1beta1.PipelineRef{ + Bundle: "docker.io/foo", + }, + wantErr: apis.ErrMissingField("name"), + withContext: enableTektonOCIBundles(t), + }, { + name: "invalid bundle reference", + ref: &v1beta1.PipelineRef{ + Name: "my-pipeline", + Bundle: "not a valid reference", + }, + wantErr: apis.ErrInvalidValue("invalid bundle reference", "bundle", "could not parse reference: not a valid reference"), + withContext: enableTektonOCIBundles(t), + }, { + name: "pipelineRef without Pipeline Name", + ref: &v1beta1.PipelineRef{}, + wantErr: apis.ErrMissingField("name"), + }, { + name: "pipelineref resolver disallowed without alpha feature gate", + ref: &v1beta1.PipelineRef{ + ResolverRef: v1beta1.ResolverRef{ + Resolver: "foo", + }, + }, + wantErr: apis.ErrGeneric("resolver requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\""), + }, { + name: "pipelineref resource disallowed without alpha feature gate", + ref: &v1beta1.PipelineRef{ + ResolverRef: v1beta1.ResolverRef{ + Resource: []v1beta1.ResolverParam{}, + }, + }, + wantErr: apis.ErrMissingField("resolver").Also(apis.ErrGeneric("resource requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"")), + }, { + name: "pipelineref resource disallowed without resolver", + ref: &v1beta1.PipelineRef{ + ResolverRef: v1beta1.ResolverRef{ + Resource: []v1beta1.ResolverParam{}, + }, + }, + wantErr: apis.ErrMissingField("resolver"), + withContext: enableAlphaAPIFields, + }, { + name: "pipelineref resolver disallowed in conjunction with pipelineref name", + ref: &v1beta1.PipelineRef{ + Name: "foo", + ResolverRef: v1beta1.ResolverRef{ + Resolver: "bar", + }, + }, + wantErr: apis.ErrMultipleOneOf("name", "resolver"), + withContext: enableAlphaAPIFields, + }, { + name: "pipelineref resolver disallowed in conjunction with pipelineref bundle", + ref: &v1beta1.PipelineRef{ + Bundle: "foo", + ResolverRef: v1beta1.ResolverRef{ + Resolver: "baz", + }, + }, + wantErr: apis.ErrMultipleOneOf("bundle", "resolver"), + withContext: enableAlphaAPIFields, + }, { + name: "pipelineref resource disallowed in conjunction with pipelineref name", + ref: &v1beta1.PipelineRef{ + Name: "bar", + ResolverRef: v1beta1.ResolverRef{ + Resource: []v1beta1.ResolverParam{{ + Name: "foo", + Value: "bar", + }}, + }, + }, + wantErr: apis.ErrMultipleOneOf("name", "resource").Also(apis.ErrMissingField("resolver")), + withContext: enableAlphaAPIFields, + }, { + name: "pipelineref resource disallowed in conjunction with pipelineref bundle", + ref: &v1beta1.PipelineRef{ + Bundle: "bar", + ResolverRef: v1beta1.ResolverRef{ + Resource: []v1beta1.ResolverParam{{ + Name: "foo", + Value: "bar", + }}, + }, + }, + wantErr: apis.ErrMultipleOneOf("bundle", "resource").Also(apis.ErrMissingField("resolver")), + withContext: enableAlphaAPIFields, + }} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + if tc.withContext != nil { + ctx = tc.withContext(ctx) + } + err := tc.ref.Validate(ctx) + if d := cmp.Diff(tc.wantErr.Error(), err.Error()); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } +} + +func TestPipelineRef_Valid(t *testing.T) { + tests := []struct { + name string + ref *v1beta1.PipelineRef + wc func(context.Context) context.Context + }{{ + name: "no pipelineRef", + ref: nil, + }, { + name: "alpha feature: valid resolver", + ref: &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, + wc: enableAlphaAPIFields, + }, { + name: "alpha feature: valid resolver with resource parameters", + ref: &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git", Resource: []v1beta1.ResolverParam{{ + Name: "repo", + Value: "https://github.com/tektoncd/pipeline.git", + }, { + Name: "branch", + Value: "baz", + }}}}, + wc: enableAlphaAPIFields, + }} + + for _, ts := range tests { + t.Run(ts.name, func(t *testing.T) { + ctx := context.Background() + if ts.wc != nil { + ctx = ts.wc(ctx) + } + if err := ts.ref.Validate(ctx); err != nil { + t.Error(err) + } + }) + } +} + +func enableTektonOCIBundles(t *testing.T) func(context.Context) context.Context { + return func(ctx context.Context) context.Context { + s := config.NewStore(logtesting.TestLogger(t)) + s.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName()}, + Data: map[string]string{ + "enable-tekton-oci-bundles": "true", + }, + }) + return s.ToContext(ctx) + } +} diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go index a9e75f33b6d..e8f5b047fff 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go @@ -265,25 +265,6 @@ const ( PipelineRunSpecStatusPending = "PipelineRunPending" ) -// PipelineRef can be used to refer to a specific instance of a Pipeline. -// Copied from CrossVersionObjectReference: https://github.com/kubernetes/kubernetes/blob/169df7434155cbbc22f1532cba8e0a9588e29ad8/pkg/apis/autoscaling/types.go#L64 -type PipelineRef struct { - // Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names - Name string `json:"name,omitempty"` - // API version of the referent - // +optional - APIVersion string `json:"apiVersion,omitempty"` - // Bundle url reference to a Tekton Bundle. - // +optional - Bundle string `json:"bundle,omitempty"` - - // ResolverRef allows referencing a Pipeline in a remote location - // like a git repo. This field is only supported when the alpha - // feature gate is enabled. - // +optional - ResolverRef `json:",omitempty"` -} - // PipelineRunStatus defines the observed state of PipelineRun type PipelineRunStatus struct { duckv1beta1.Status `json:",inline"` diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go index ce82e89697a..dbf45fc5d85 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go @@ -29,7 +29,6 @@ import ( corev1resources "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/apis" - logtesting "knative.dev/pkg/logging/testing" ) func TestPipelineRun_Invalid(t *testing.T) { @@ -93,49 +92,6 @@ func TestPipelineRun_Invalid(t *testing.T) { }, }, want: apis.ErrInvalidValue("PipelineRunCancell should be Cancelled, CancelledRunFinally, StoppedRunFinally or PipelineRunPending", "spec.status"), - }, { - name: "use of bundle without the feature flag set", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelinename", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "my-pipeline", - Bundle: "docker.io/foo", - }, - }, - }, - want: apis.ErrDisallowedFields("spec.pipelineRef.bundle"), - }, { - name: "bundle missing name", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelinename", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Bundle: "docker.io/foo", - }, - }, - }, - want: apis.ErrMissingField("spec.pipelineRef.name"), - wc: enableTektonOCIBundles(t), - }, { - name: "invalid bundle reference", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelinename", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "my-pipeline", - Bundle: "not a valid reference", - }, - }, - }, - want: apis.ErrInvalidValue("invalid bundle reference", "spec.pipelineRef.bundle", "could not parse reference: not a valid reference"), - wc: enableTektonOCIBundles(t), }, { name: "pipelinerun pending while running", pr: v1beta1.PipelineRun{ @@ -319,34 +275,6 @@ func TestPipelineRun_Validate(t *testing.T) { }, }, }, - }, { - name: "alpha feature: valid resolver", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pr", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, - }, - }, - wc: enableAlphaAPIFields, - }, { - name: "alpha feature: valid resolver with resource parameters", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pr", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git", Resource: []v1beta1.ResolverParam{{ - Name: "repo", - Value: "https://github.com/tektoncd/pipeline.git", - }, { - Name: "branch", - Value: "baz", - }}}}, - }, - }, - wc: enableAlphaAPIFields, }, { name: "alpha feature: sidecar and step overrides", pr: v1beta1.PipelineRun{ @@ -397,12 +325,6 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { wantErr *apis.FieldError withContext func(context.Context) context.Context }{{ - name: "pipelineRef without Pipeline Name", - spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{}, - }, - wantErr: apis.ErrMissingField("pipelineRef.name"), - }, { name: "pipelineRef and pipelineSpec together", spec: v1beta1.PipelineRunSpec{ PipelineRef: &v1beta1.PipelineRef{ @@ -455,63 +377,6 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { "workspaces[0].volumeclaimtemplate", }, }, - }, { - name: "pipelineref resolver disallowed without alpha feature gate", - spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "foo", - ResolverRef: v1beta1.ResolverRef{ - Resolver: "foo", - }, - }, - }, - wantErr: apis.ErrDisallowedFields("resolver").ViaField("pipelineRef"), - }, { - name: "pipelineref resource disallowed without alpha feature gate", - spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "foo", - ResolverRef: v1beta1.ResolverRef{ - Resource: []v1beta1.ResolverParam{}, - }, - }, - }, - wantErr: apis.ErrDisallowedFields("resource").ViaField("pipelineRef"), - }, { - name: "pipelineref resource disallowed without resolver", - spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - ResolverRef: v1beta1.ResolverRef{ - Resource: []v1beta1.ResolverParam{}, - }, - }, - }, - wantErr: apis.ErrMissingField("resolver").ViaField("pipelineRef"), - withContext: enableAlphaAPIFields, - }, { - name: "pipelineref resolver disallowed in conjunction with pipelineref name", - spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "foo", - ResolverRef: v1beta1.ResolverRef{ - Resolver: "bar", - }, - }, - }, - wantErr: apis.ErrMultipleOneOf("name", "resolver").ViaField("pipelineRef"), - withContext: enableAlphaAPIFields, - }, { - name: "pipelineref resolver disallowed in conjunction with pipelineref bundle", - spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Bundle: "foo", - ResolverRef: v1beta1.ResolverRef{ - Resolver: "baz", - }, - }, - }, - wantErr: apis.ErrMultipleOneOf("bundle", "resolver").ViaField("pipelineRef"), - withContext: enableAlphaAPIFields, }, { name: "duplicate stepOverride names", spec: v1beta1.PipelineRunSpec{ @@ -975,19 +840,6 @@ func TestPipelineRunWithTimeout_Validate(t *testing.T) { } } -func enableTektonOCIBundles(t *testing.T) func(context.Context) context.Context { - return func(ctx context.Context) context.Context { - s := config.NewStore(logtesting.TestLogger(t)) - s.OnConfigChanged(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName()}, - Data: map[string]string{ - "enable-tekton-oci-bundles": "true", - }, - }) - return s.ToContext(ctx) - } -} - func enableAlphaAPIFields(ctx context.Context) context.Context { featureFlags, _ := config.NewFeatureFlagsFromMap(map[string]string{ "enable-api-fields": "alpha", diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index 0fbcefe17f8..033808a2d7c 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -707,7 +707,7 @@ } }, "v1beta1.PipelineRef": { - "description": "PipelineRef can be used to refer to a specific instance of a Pipeline. Copied from CrossVersionObjectReference: https://github.com/kubernetes/kubernetes/blob/169df7434155cbbc22f1532cba8e0a9588e29ad8/pkg/apis/autoscaling/types.go#L64", + "description": "PipelineRef can be used to refer to a specific instance of a Pipeline.", "type": "object", "properties": { "apiVersion": { @@ -2134,7 +2134,7 @@ } }, "v1beta1.TaskRef": { - "description": "TaskRef can be used to refer to a specific instance of a task. Copied from CrossVersionObjectReference: https://github.com/kubernetes/kubernetes/blob/169df7434155cbbc22f1532cba8e0a9588e29ad8/pkg/apis/autoscaling/types.go#L64", + "description": "TaskRef can be used to refer to a specific instance of a task.", "type": "object", "properties": { "apiVersion": { diff --git a/pkg/apis/pipeline/v1beta1/task_types.go b/pkg/apis/pipeline/v1beta1/task_types.go index c0d168b0a77..957b0aef697 100644 --- a/pkg/apis/pipeline/v1beta1/task_types.go +++ b/pkg/apis/pipeline/v1beta1/task_types.go @@ -134,36 +134,3 @@ type TaskList struct { metav1.ListMeta `json:"metadata,omitempty"` Items []Task `json:"items"` } - -// TaskRef can be used to refer to a specific instance of a task. -// Copied from CrossVersionObjectReference: https://github.com/kubernetes/kubernetes/blob/169df7434155cbbc22f1532cba8e0a9588e29ad8/pkg/apis/autoscaling/types.go#L64 -type TaskRef struct { - // Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names - Name string `json:"name,omitempty"` - // TaskKind indicates the kind of the task, namespaced or cluster scoped. - Kind TaskKind `json:"kind,omitempty"` - // API version of the referent - // +optional - APIVersion string `json:"apiVersion,omitempty"` - // Bundle url reference to a Tekton Bundle. - // +optional - Bundle string `json:"bundle,omitempty"` - - // ResolverRef allows referencing a Task in a remote location - // like a git repo. This field is only supported when the alpha - // feature gate is enabled. - // +optional - ResolverRef `json:",omitempty"` -} - -// Check that Pipeline may be validated and defaulted. - -// TaskKind defines the type of Task used by the pipeline. -type TaskKind string - -const ( - // NamespacedTaskKind indicates that the task type has a namespaced scope. - NamespacedTaskKind TaskKind = "Task" - // ClusterTaskKind indicates that task type has a cluster scope. - ClusterTaskKind TaskKind = "ClusterTask" -) diff --git a/pkg/apis/pipeline/v1beta1/taskref_types.go b/pkg/apis/pipeline/v1beta1/taskref_types.go new file mode 100644 index 00000000000..07aeb436d79 --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/taskref_types.go @@ -0,0 +1,49 @@ +/* +Copyright 2022 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 v1beta1 + +// TaskRef can be used to refer to a specific instance of a task. +type TaskRef struct { + // Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names + Name string `json:"name,omitempty"` + // TaskKind indicates the kind of the task, namespaced or cluster scoped. + Kind TaskKind `json:"kind,omitempty"` + // API version of the referent + // +optional + APIVersion string `json:"apiVersion,omitempty"` + // Bundle url reference to a Tekton Bundle. + // +optional + Bundle string `json:"bundle,omitempty"` + + // ResolverRef allows referencing a Task in a remote location + // like a git repo. This field is only supported when the alpha + // feature gate is enabled. + // +optional + ResolverRef `json:",omitempty"` +} + +// Check that Pipeline may be validated and defaulted. + +// TaskKind defines the type of Task used by the pipeline. +type TaskKind string + +const ( + // NamespacedTaskKind indicates that the task type has a namespaced scope. + NamespacedTaskKind TaskKind = "Task" + // ClusterTaskKind indicates that task type has a cluster scope. + ClusterTaskKind TaskKind = "ClusterTask" +) diff --git a/pkg/apis/pipeline/v1beta1/taskref_validation.go b/pkg/apis/pipeline/v1beta1/taskref_validation.go index dc5e97611f0..652eed6cff8 100644 --- a/pkg/apis/pipeline/v1beta1/taskref_validation.go +++ b/pkg/apis/pipeline/v1beta1/taskref_validation.go @@ -21,81 +21,43 @@ import ( "github.com/google/go-containerregistry/pkg/name" "github.com/tektoncd/pipeline/pkg/apis/config" + "github.com/tektoncd/pipeline/pkg/apis/version" "knative.dev/pkg/apis" ) // Validate ensures that a supplied TaskRef field is populated // correctly. No errors are returned for a nil TaskRef. func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { - cfg := config.FromContextOrDefaults(ctx) if ref == nil { return } - if cfg.FeatureFlags.EnableAPIFields == config.AlphaAPIFields { - errs = errs.Also(ref.validateAlphaRef(ctx)) - } else { - errs = errs.Also(ref.validateInTreeRef(ctx)) - } - return -} -// validateInTreeRef returns errors if the given taskRef is not valid for -// Pipelines' built-in resolution machinery. -func (ref *TaskRef) validateInTreeRef(ctx context.Context) (errs *apis.FieldError) { - cfg := config.FromContextOrDefaults(ctx) - if ref.Resolver != "" { - errs = errs.Also(apis.ErrDisallowedFields("resolver")) - } - if ref.Resource != nil { - errs = errs.Also(apis.ErrDisallowedFields("resource")) - } - if ref.Name == "" { - errs = errs.Also(apis.ErrMissingField("name")) - } - if cfg.FeatureFlags.EnableTektonOCIBundles { - if ref.Bundle != "" && ref.Name == "" { - errs = errs.Also(apis.ErrMissingField("name")) + switch { + case ref.Resolver != "": + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver", config.AlphaAPIFields).ViaField("resolver")) + if ref.Name != "" { + errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) } if ref.Bundle != "" { - if _, err := name.ParseReference(ref.Bundle); err != nil { - errs = errs.Also(apis.ErrInvalidValue("invalid bundle reference", "bundle", err.Error())) - } - } - } else if ref.Bundle != "" { - errs = errs.Also(apis.ErrDisallowedFields("bundle")) - } - return -} - -// validateAlphaRef ensures that the user has passed either a -// valid remote resource reference or a valid in-tree resource reference, -// but not both. -func (ref *TaskRef) validateAlphaRef(ctx context.Context) (errs *apis.FieldError) { - hasResolver := ref.Resolver != "" - hasResource := ref.Resource != nil - hasName := ref.Name != "" - hasBundle := ref.Bundle != "" - if hasName { - if hasResolver { - errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) + errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resolver")) } - if hasResource { + case ref.Resource != nil: + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resource", config.AlphaAPIFields).ViaField("resource")) + if ref.Name != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "resource")) } - } - if hasBundle { - if hasResolver { - errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resolver")) - } - if hasResource { + if ref.Bundle != "" { errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resource")) } - } - if !hasResolver { - if hasResource { + if ref.Resolver == "" { errs = errs.Also(apis.ErrMissingField("resolver")) - } else { - errs = errs.Also(ref.validateInTreeRef(ctx)) + } + case ref.Name == "": + errs = errs.Also(apis.ErrMissingField("name")) + case ref.Bundle != "": + errs = errs.Also(validateBundleFeatureFlag(ctx, "bundle", true).ViaField("bundle")) + if _, err := name.ParseReference(ref.Bundle); err != nil { + errs = errs.Also(apis.ErrInvalidValue("invalid bundle reference", "bundle", err.Error())) } } return diff --git a/pkg/apis/pipeline/v1beta1/taskref_validation_test.go b/pkg/apis/pipeline/v1beta1/taskref_validation_test.go new file mode 100644 index 00000000000..9e0be193f2d --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/taskref_validation_test.go @@ -0,0 +1,189 @@ +/* +Copyright 2019 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 v1beta1_test + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/pipeline/test/diff" + "knative.dev/pkg/apis" +) + +func TestTaskRef_Valid(t *testing.T) { + tests := []struct { + name string + taskRef *v1beta1.TaskRef + wc func(context.Context) context.Context + }{{ + name: "nil taskref", + }, { + name: "simple taskref", + taskRef: &v1beta1.TaskRef{Name: "taskrefname"}, + }, { + name: "alpha feature: valid resolver", + taskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, + wc: enableAlphaAPIFields, + }, { + name: "alpha feature: valid resolver with resource parameters", + taskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git", Resource: []v1beta1.ResolverParam{{ + Name: "repo", + Value: "https://github.com/tektoncd/pipeline.git", + }, { + Name: "branch", + Value: "baz", + }}}}, + wc: enableAlphaAPIFields, + }, { + name: "valid bundle", + taskRef: &v1beta1.TaskRef{ + Name: "bundled-task", + Bundle: "gcr.io/my-bundle"}, + wc: enableAlphaAPIFields, + }} + for _, ts := range tests { + t.Run(ts.name, func(t *testing.T) { + ctx := context.Background() + if ts.wc != nil { + ctx = ts.wc(ctx) + } + if err := ts.taskRef.Validate(ctx); err != nil { + t.Errorf("TaskRef.Validate() error = %v", err) + } + }) + } +} + +func TestTaskRef_Invalid(t *testing.T) { + tests := []struct { + name string + taskRef *v1beta1.TaskRef + wantErr *apis.FieldError + wc func(context.Context) context.Context + }{{ + name: "missing taskref name", + taskRef: &v1beta1.TaskRef{}, + wantErr: apis.ErrMissingField("name"), + }, { + name: "use of bundle without the feature flag set", + taskRef: &v1beta1.TaskRef{ + Name: "my-task", + Bundle: "docker.io/foo", + }, + wantErr: apis.ErrGeneric("bundle requires \"enable-tekton-oci-bundles\" feature gate to be true but it is false"), + }, { + name: "bundle missing name", + taskRef: &v1beta1.TaskRef{ + Bundle: "docker.io/foo", + }, + wantErr: apis.ErrMissingField("name"), + wc: enableTektonOCIBundles(t), + }, { + name: "invalid bundle reference", + taskRef: &v1beta1.TaskRef{ + Name: "my-task", + Bundle: "invalid reference", + }, + wantErr: apis.ErrInvalidValue("invalid bundle reference", "bundle", "could not parse reference: invalid reference"), + wc: enableTektonOCIBundles(t), + }, { + name: "taskref resolver disallowed without alpha feature gate", + taskRef: &v1beta1.TaskRef{ + ResolverRef: v1beta1.ResolverRef{ + Resolver: "git", + }, + }, + wantErr: apis.ErrGeneric("resolver requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\""), + }, { + name: "taskref resource disallowed without alpha feature gate", + taskRef: &v1beta1.TaskRef{ + ResolverRef: v1beta1.ResolverRef{ + Resource: []v1beta1.ResolverParam{}, + }, + }, + wantErr: apis.ErrMissingField("resolver").Also(apis.ErrGeneric("resource requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"")), + }, { + name: "taskref resource disallowed without resolver", + taskRef: &v1beta1.TaskRef{ + ResolverRef: v1beta1.ResolverRef{ + Resource: []v1beta1.ResolverParam{}, + }, + }, + wantErr: apis.ErrMissingField("resolver"), + wc: enableAlphaAPIFields, + }, { + name: "taskref resolver disallowed in conjunction with taskref name", + taskRef: &v1beta1.TaskRef{ + Name: "foo", + ResolverRef: v1beta1.ResolverRef{ + Resolver: "git", + }, + }, + wantErr: apis.ErrMultipleOneOf("name", "resolver"), + wc: enableAlphaAPIFields, + }, { + name: "taskref resolver disallowed in conjunction with taskref bundle", + taskRef: &v1beta1.TaskRef{ + Bundle: "bar", + ResolverRef: v1beta1.ResolverRef{ + Resolver: "git", + }, + }, + wantErr: apis.ErrMultipleOneOf("bundle", "resolver"), + wc: enableAlphaAPIFields, + }, { + name: "taskref resource disallowed in conjunction with taskref name", + taskRef: &v1beta1.TaskRef{ + Name: "bar", + ResolverRef: v1beta1.ResolverRef{ + Resource: []v1beta1.ResolverParam{{ + Name: "foo", + Value: "bar", + }}, + }, + }, + wantErr: apis.ErrMultipleOneOf("name", "resource").Also(apis.ErrMissingField("resolver")), + wc: enableAlphaAPIFields, + }, { + name: "taskref resource disallowed in conjunction with taskref bundle", + taskRef: &v1beta1.TaskRef{ + Bundle: "bar", + ResolverRef: v1beta1.ResolverRef{ + Resource: []v1beta1.ResolverParam{{ + Name: "foo", + Value: "bar", + }}, + }, + }, + wantErr: apis.ErrMultipleOneOf("bundle", "resource").Also(apis.ErrMissingField("resolver")), + wc: enableAlphaAPIFields, + }} + for _, ts := range tests { + t.Run(ts.name, func(t *testing.T) { + ctx := context.Background() + if ts.wc != nil { + ctx = ts.wc(ctx) + } + err := ts.taskRef.Validate(ctx) + if d := cmp.Diff(ts.wantErr.Error(), err.Error()); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go index cbd30e966bf..b2e7d28ec30 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go @@ -58,49 +58,11 @@ func TestTaskRun_Validate(t *testing.T) { taskRun *v1beta1.TaskRun wc func(context.Context) context.Context }{{ - name: "simple taskref", - taskRun: &v1beta1.TaskRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "taskrname", - }, - Spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{Name: "taskrefname"}, - }, - }, - }, { name: "do not validate spec on delete", taskRun: &v1beta1.TaskRun{ ObjectMeta: metav1.ObjectMeta{Name: "taskrname"}, }, wc: apis.WithinDelete, - }, { - name: "alpha feature: valid resolver", - taskRun: &v1beta1.TaskRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "tr", - }, - Spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, - }, - }, - wc: enableAlphaAPIFields, - }, { - name: "alpha feature: valid resolver with resource parameters", - taskRun: &v1beta1.TaskRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "tr", - }, - Spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git", Resource: []v1beta1.ResolverParam{{ - Name: "repo", - Value: "https://github.com/tektoncd/pipeline.git", - }, { - Name: "branch", - Value: "baz", - }}}}, - }, - }, - wc: enableAlphaAPIFields, }, { name: "alpha feature: valid step and sidecar overrides", taskRun: &v1beta1.TaskRun{ @@ -194,12 +156,6 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { name: "invalid taskspec", spec: v1beta1.TaskRunSpec{}, wantErr: apis.ErrMissingOneOf("taskRef", "taskSpec"), - }, { - name: "missing taskref name", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{}, - }, - wantErr: apis.ErrMissingField("taskRef.name"), }, { name: "invalid taskref and taskspec together", spec: v1beta1.TaskRunSpec{ @@ -287,34 +243,6 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }, wantErr: apis.ErrMultipleOneOf("params[myobjectparam].name"), wc: enableAlphaAPIFields, - }, { - name: "use of bundle without the feature flag set", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - Name: "my-task", - Bundle: "docker.io/foo", - }, - }, - wantErr: apis.ErrDisallowedFields("taskRef.bundle"), - }, { - name: "bundle missing name", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - Bundle: "docker.io/foo", - }, - }, - wantErr: apis.ErrMissingField("taskRef.name"), - wc: enableTektonOCIBundles(t), - }, { - name: "invalid bundle reference", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - Name: "my-task", - Bundle: "invalid reference", - }, - }, - wantErr: apis.ErrInvalidValue("invalid bundle reference", "taskRef.bundle", "could not parse reference: invalid reference"), - wc: enableTektonOCIBundles(t), }, { name: "using debug when apifields stable", spec: v1beta1.TaskRunSpec{ @@ -338,95 +266,6 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }, wantErr: apis.ErrInvalidValue("breakito is not a valid breakpoint. Available valid breakpoints include [onFailure]", "debug.breakpoint"), wc: enableAlphaAPIFields, - }, { - name: "taskref resolver disallowed without alpha feature gate", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - Name: "foo", - ResolverRef: v1beta1.ResolverRef{ - Resolver: "git", - }, - }, - }, - wantErr: apis.ErrDisallowedFields("resolver").ViaField("taskRef"), - }, { - name: "taskref resource disallowed without alpha feature gate", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - Name: "foo", - ResolverRef: v1beta1.ResolverRef{ - Resource: []v1beta1.ResolverParam{}, - }, - }, - }, - wantErr: apis.ErrDisallowedFields("resource").ViaField("taskRef"), - }, { - name: "taskref resource disallowed without resolver", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - ResolverRef: v1beta1.ResolverRef{ - Resource: []v1beta1.ResolverParam{}, - }, - }, - }, - wantErr: apis.ErrMissingField("resolver").ViaField("taskRef"), - wc: enableAlphaAPIFields, - }, { - name: "taskref resolver disallowed in conjunction with taskref name", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - Name: "foo", - ResolverRef: v1beta1.ResolverRef{ - Resolver: "git", - }, - }, - }, - wantErr: apis.ErrMultipleOneOf("name", "resolver").ViaField("taskRef"), - wc: enableAlphaAPIFields, - }, { - name: "taskref resolver disallowed in conjunction with taskref bundle", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - Bundle: "bar", - ResolverRef: v1beta1.ResolverRef{ - Resolver: "git", - }, - }, - }, - wantErr: apis.ErrMultipleOneOf("bundle", "resolver").ViaField("taskRef"), - wc: enableAlphaAPIFields, - }, { - name: "taskref resource disallowed in conjunction with taskref name", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - Name: "bar", - ResolverRef: v1beta1.ResolverRef{ - Resource: []v1beta1.ResolverParam{{ - Name: "foo", - Value: "bar", - }}, - }, - }, - }, - wantErr: apis.ErrMultipleOneOf("name", "resource").ViaField("taskRef").Also( - apis.ErrMissingField("resolver").ViaField("taskRef")), - wc: enableAlphaAPIFields, - }, { - name: "taskref resource disallowed in conjunction with taskref bundle", - spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ - Bundle: "bar", - ResolverRef: v1beta1.ResolverRef{ - Resource: []v1beta1.ResolverParam{{ - Name: "foo", - Value: "bar", - }}, - }, - }, - }, - wantErr: apis.ErrMultipleOneOf("bundle", "resource").ViaField("taskRef").Also( - apis.ErrMissingField("resolver").ViaField("taskRef")), - wc: enableAlphaAPIFields, }, { name: "stepOverride disallowed without alpha feature gate", spec: v1beta1.TaskRunSpec{