From 3ea59814fefa0650dede90a09e9a43af214480d6 Mon Sep 17 00:00:00 2001 From: Scott Date: Thu, 11 Feb 2021 17:27:35 +0000 Subject: [PATCH] Losslessly roundtrip Pipelines with Finally from beta to alpha and back Prior to this commit pipelines returned an error when converting from a v1beta1 pipeline to a v1alpha1 pipeline if that pipeline had a Finally section in it. As a result of this error the kubeapi would repeatedly request v1alpha1 pipelines every second, filling up our webhook logs with these errors. This commit allows Finally to be serialized into alpha Pipelines, keeping the Finally Tasks as an annotation on the resource. If the alpha Pipeline is then re-applied to the cluster the Finally annotation is rehydrated into the Finally section of the stored v1beta1 resource. --- .../pipeline/v1alpha1/conversion_error.go | 3 -- .../pipeline/v1alpha1/pipeline_conversion.go | 37 ++++++++++++++++--- .../v1alpha1/pipeline_conversion_test.go | 25 +++++++------ .../pipeline/v1beta1/pipeline_validation.go | 4 +- .../v1beta1/pipeline_validation_test.go | 4 +- 5 files changed, 49 insertions(+), 24 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/conversion_error.go b/pkg/apis/pipeline/v1alpha1/conversion_error.go index 9df97adc2d0..fae79e6595d 100644 --- a/pkg/apis/pipeline/v1alpha1/conversion_error.go +++ b/pkg/apis/pipeline/v1alpha1/conversion_error.go @@ -34,6 +34,3 @@ const ( type CannotConvertError = v1beta1.CannotConvertError var _ error = (*CannotConvertError)(nil) - -// convertErrorf creates a CannotConvertError from the field name and format string. -var convertErrorf = v1beta1.ConvertErrorf diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go b/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go index 4ce6ec730c8..a7a30a378e7 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go @@ -19,6 +19,7 @@ package v1alpha1 import ( "context" + "encoding/json" "fmt" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" @@ -26,6 +27,7 @@ import ( ) const FinallyFieldName = "finally" +const finallyAnnotationKey = "tekton.dev/v1beta1Finally" var _ apis.Convertible = (*Pipeline)(nil) @@ -34,10 +36,29 @@ func (source *Pipeline) ConvertTo(ctx context.Context, obj apis.Convertible) err switch sink := obj.(type) { case *v1beta1.Pipeline: sink.ObjectMeta = source.ObjectMeta - return source.Spec.ConvertTo(ctx, &sink.Spec) + if err := source.Spec.ConvertTo(ctx, &sink.Spec); err != nil { + return err + } + if source.Annotations != nil { + if _, ok := source.Annotations[finallyAnnotationKey]; ok { + finally := []v1beta1.PipelineTask{} + if err := json.Unmarshal([]byte(source.Annotations[finallyAnnotationKey]), &finally); err != nil { + return fmt.Errorf("error converting finally annotation into beta field: %w", err) + } + if err := v1beta1.ValidatePipelineTasks(ctx, sink.Spec.Tasks, finally); err != nil { + return fmt.Errorf("error converting finally annotation into beta field: %w", err) + } + delete(sink.ObjectMeta.Annotations, finallyAnnotationKey) + if len(sink.ObjectMeta.Annotations) == 0 { + sink.ObjectMeta.Annotations = nil + } + sink.Spec.Finally = finally + } + } default: return fmt.Errorf("unknown version, got: %T", sink) } + return nil } func (source *PipelineSpec) ConvertTo(ctx context.Context, sink *v1beta1.PipelineSpec) error { @@ -81,6 +102,16 @@ func (sink *Pipeline) ConvertFrom(ctx context.Context, obj apis.Convertible) err switch source := obj.(type) { case *v1beta1.Pipeline: sink.ObjectMeta = source.ObjectMeta + if len(source.Spec.Finally) != 0 { + b, err := json.Marshal(source.Spec.Finally) + if err != nil { + return err + } + if sink.ObjectMeta.Annotations == nil { + sink.ObjectMeta.Annotations = make(map[string]string) + } + sink.ObjectMeta.Annotations[finallyAnnotationKey] = string(b) + } return sink.Spec.ConvertFrom(ctx, source.Spec) default: return fmt.Errorf("unknown version, got: %T", sink) @@ -100,10 +131,6 @@ func (sink *PipelineSpec) ConvertFrom(ctx context.Context, source v1beta1.Pipeli } } } - // finally clause was introduced in v1beta1 and not available in v1alpha1 - if len(source.Finally) > 0 { - return convertErrorf(FinallyFieldName, ConversionErrorFieldNotAvailableMsg) - } return nil } diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go index 6585cf87309..7bab36f7130 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go @@ -215,7 +215,6 @@ func TestPipelineConversionFromWithFinally(t *testing.T) { if err := p.ConvertTo(context.Background(), ver); err != nil { t.Errorf("ConvertTo() = %v", err) } - // modify ver to introduce new field which causes failure to convert v1beta1 to v1alpha1 source := ver source.(*v1beta1.Pipeline).Spec.Finally = []v1beta1.PipelineTask{{Name: "finaltask", TaskRef: &TaskRef{Name: "task"}}} got := &Pipeline{} @@ -231,7 +230,7 @@ func TestPipelineConversionFromWithFinally(t *testing.T) { } } -func TestPipelineConversionFromBetaToAlphaWithFinally_Failure(t *testing.T) { +func TestPipelineConversionFromBetaToAlphaWithFinally(t *testing.T) { p := &v1beta1.Pipeline{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", @@ -240,18 +239,20 @@ func TestPipelineConversionFromBetaToAlphaWithFinally_Failure(t *testing.T) { }, Spec: v1beta1.PipelineSpec{ Tasks: []v1beta1.PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}}, - Finally: []v1beta1.PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}}, + Finally: []v1beta1.PipelineTask{{Name: "myfinallytask", TaskRef: &TaskRef{Name: "task"}}}, }, } - t.Run("finally not available in v1alpha1", func(t *testing.T) { - got := &Pipeline{} - if err := got.ConvertFrom(context.Background(), p); err != nil { - cce, ok := err.(*CannotConvertError) - // conversion error (cce) contains the field name which resulted in the failure and should be equal to "finally" here - if ok && cce.Field == FinallyFieldName { - return - } - t.Errorf("ConvertFrom() should have failed") + t.Run("finally stored by v1alpha1 and rehydrated for v1beta1", func(t *testing.T) { + downgrade := &Pipeline{} + if err := downgrade.ConvertFrom(context.Background(), p); err != nil { + t.Errorf("error converting from v1beta1 with finally field to v1alpha1 with finally annotation: %v", err) + } + upgrade := &v1beta1.Pipeline{} + if err := downgrade.ConvertTo(context.Background(), upgrade); err != nil { + t.Errorf("error converting from v1alpha1 with finally annotation to v1beta1 with finally field: %v", err) + } + if d := cmp.Diff(p, upgrade); d != "" { + t.Errorf("unexpected difference between v1beta1 with finally field and round-tripped v1beta1 with finally field: %s", diff.PrintWantGot(d)) } }) } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 0db801cff12..67be1b00a0f 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -49,7 +49,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(apis.ErrGeneric("expected at least one, got none", "description", "params", "resources", "tasks", "workspaces")) } // PipelineTask must have a valid unique label and at least one of taskRef or taskSpec should be specified - errs = errs.Also(validatePipelineTasks(ctx, ps.Tasks, ps.Finally)) + errs = errs.Also(ValidatePipelineTasks(ctx, ps.Tasks, ps.Finally)) // All declared resources should be used, and the Pipeline shouldn't try to use any resources // that aren't declared errs = errs.Also(validateDeclaredResources(ps.Resources, ps.Tasks, ps.Finally)) @@ -76,7 +76,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { // 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 { +func ValidatePipelineTasks(ctx context.Context, tasks []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { // Names cannot be duplicated taskNames := sets.NewString() var errs *apis.FieldError diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 3d2a529b314..b66cf0722e5 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -767,7 +767,7 @@ func TestValidatePipelineTasks_Success(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validatePipelineTasks(context.Background(), tt.tasks, []PipelineTask{}) + err := ValidatePipelineTasks(context.Background(), tt.tasks, []PipelineTask{}) if err != nil { t.Errorf("Pipeline.validatePipelineTasks() returned error for valid pipeline tasks: %v", err) } @@ -927,7 +927,7 @@ func TestValidatePipelineTasks_Failure(t *testing.T) { if tt.wc != nil { ctx = tt.wc(ctx) } - err := validatePipelineTasks(ctx, tt.tasks, []PipelineTask{}) + err := ValidatePipelineTasks(ctx, tt.tasks, []PipelineTask{}) if err == nil { t.Error("Pipeline.validatePipelineTasks() did not return error for invalid pipeline tasks") }