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.

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

pkg/apis/pipeline/v1alpha1/pipeline_conversion.go

@@ -19,13 +19,15 @@ package v1alpha1
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"knative.dev/pkg/apis"
 )
 
 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
 }
 

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))
 		}
 	})
 }

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

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")
 			}