From 36fa11c31a6e692adff5e15157a5027f80136532 Mon Sep 17 00:00:00 2001 From: Ali Ok Date: Tue, 7 Jul 2020 00:30:48 +0300 Subject: [PATCH 1/3] Handle conversion errors of subtypes --- .../v1alpha1/subscribable_types_conversion.go | 41 ++++++++++--------- .../v1beta1/subscribable_types_conversion.go | 23 ++++++++--- 2 files changed, 39 insertions(+), 25 deletions(-) diff --git a/pkg/apis/duck/v1alpha1/subscribable_types_conversion.go b/pkg/apis/duck/v1alpha1/subscribable_types_conversion.go index f4b1cac6978..f0597a6c4ac 100644 --- a/pkg/apis/duck/v1alpha1/subscribable_types_conversion.go +++ b/pkg/apis/duck/v1alpha1/subscribable_types_conversion.go @@ -32,11 +32,16 @@ func (source *SubscribableType) ConvertTo(ctx context.Context, obj apis.Converti switch sink := obj.(type) { case *duckv1beta1.Subscribable: sink.ObjectMeta = source.ObjectMeta - source.Status.ConvertTo(ctx, &sink.Status) - return source.Spec.ConvertTo(ctx, &sink.Spec) + if err := source.Status.ConvertTo(ctx, &sink.Status); err != nil { + return err + } + if err := source.Spec.ConvertTo(ctx, &sink.Spec); err != nil { + return err + } default: return fmt.Errorf("unknown version, got: %T", sink) } + return nil } // ConvertTo implements apis.Convertible @@ -46,7 +51,9 @@ func (source *SubscribableTypeSpec) ConvertTo(ctx context.Context, obj apis.Conv if source.Subscribable != nil { sink.Subscribers = make([]duckv1beta1.SubscriberSpec, len(source.Subscribable.Subscribers)) for i, s := range source.Subscribable.Subscribers { - s.ConvertTo(ctx, &sink.Subscribers[i]) + if err := s.ConvertTo(ctx, &sink.Subscribers[i]); err != nil { + return err + } } } default: @@ -89,12 +96,7 @@ func (source *SubscribableTypeStatus) ConvertTo(ctx context.Context, obj apis.Co len(source.SubscribableStatus.Subscribers) > 0 { sink.Subscribers = make([]duckv1beta1.SubscriberStatus, len(source.SubscribableStatus.Subscribers)) for i, ss := range source.SubscribableStatus.Subscribers { - sink.Subscribers[i] = duckv1beta1.SubscriberStatus{ - UID: ss.UID, - ObservedGeneration: ss.ObservedGeneration, - Ready: ss.Ready, - Message: ss.Message, - } + sink.Subscribers[i] = *ss.DeepCopy() } } default: @@ -109,12 +111,16 @@ func (sink *SubscribableType) ConvertFrom(ctx context.Context, obj apis.Converti switch source := obj.(type) { case *duckv1beta1.Subscribable: sink.ObjectMeta = source.ObjectMeta - sink.Status.ConvertFrom(ctx, &source.Status) - sink.Spec.ConvertFrom(ctx, &source.Spec) - return nil + if err := sink.Status.ConvertFrom(ctx, &source.Status); err != nil { + return err + } + if err := sink.Spec.ConvertFrom(ctx, &source.Spec); err != nil { + return err + } default: return fmt.Errorf("unknown version, got: %T", source) } + return nil } // ConvertFrom implements apis.Convertible @@ -126,7 +132,9 @@ func (sink *SubscribableTypeSpec) ConvertFrom(ctx context.Context, obj apis.Conv Subscribers: make([]SubscriberSpec, len(source.Subscribers)), } for i, s := range source.Subscribers { - sink.Subscribable.Subscribers[i].ConvertFrom(ctx, &s) + if err := sink.Subscribable.Subscribers[i].ConvertFrom(ctx, &s); err != nil { + return err + } } } default: @@ -164,12 +172,7 @@ func (sink *SubscribableTypeStatus) ConvertFrom(ctx context.Context, obj apis.Co Subscribers: make([]duckv1beta1.SubscriberStatus, len(source.Subscribers)), } for i, ss := range source.Subscribers { - sink.SubscribableStatus.Subscribers[i] = duckv1beta1.SubscriberStatus{ - UID: ss.UID, - ObservedGeneration: ss.ObservedGeneration, - Ready: ss.Ready, - Message: ss.Message, - } + sink.SubscribableStatus.Subscribers[i] = *ss.DeepCopy() } } default: diff --git a/pkg/apis/duck/v1beta1/subscribable_types_conversion.go b/pkg/apis/duck/v1beta1/subscribable_types_conversion.go index b706cc3eccb..ac3ea43d0cf 100644 --- a/pkg/apis/duck/v1beta1/subscribable_types_conversion.go +++ b/pkg/apis/duck/v1beta1/subscribable_types_conversion.go @@ -30,12 +30,16 @@ func (source *Subscribable) ConvertTo(ctx context.Context, to apis.Convertible) switch sink := to.(type) { case *eventingduckv1.Subscribable: sink.ObjectMeta = source.ObjectMeta - source.Status.ConvertTo(ctx, &sink.Status) - source.Spec.ConvertTo(ctx, &sink.Spec) - return nil + if err := source.Status.ConvertTo(ctx, &sink.Status); err != nil { + return err + } + if err := source.Spec.ConvertTo(ctx, &sink.Spec); err != nil { + return err + } default: return fmt.Errorf("unknown version, got: %T", sink) } + return nil } // ConvertTo helps implement apis.Convertible @@ -45,7 +49,9 @@ func (source *SubscribableSpec) ConvertTo(ctx context.Context, obj apis.Converti if len(source.Subscribers) > 0 { sink.Subscribers = make([]eventingduckv1.SubscriberSpec, len(source.Subscribers)) for i, s := range source.Subscribers { - s.ConvertTo(ctx, &sink.Subscribers[i]) + if err := s.ConvertTo(ctx, &sink.Subscribers[i]); err != nil { + return err + } } } default: @@ -112,11 +118,16 @@ func (sink *Subscribable) ConvertFrom(ctx context.Context, from apis.Convertible switch source := from.(type) { case *eventingduckv1.Subscribable: sink.ObjectMeta = source.ObjectMeta - sink.Status.ConvertFrom(ctx, &source.Status) - return sink.Spec.ConvertFrom(ctx, &source.Spec) + if err := sink.Status.ConvertFrom(ctx, &source.Status); err != nil { + return err + } + if err := sink.Spec.ConvertFrom(ctx, &source.Spec); err != nil { + return err + } default: return fmt.Errorf("unknown version, got: %T", source) } + return nil } // ConvertFrom helps implement apis.Convertible From 4ebb4cdfa287ca19d716cf1aa990c66bf47b983c Mon Sep 17 00:00:00 2001 From: Ali Ok Date: Tue, 7 Jul 2020 08:45:38 +0300 Subject: [PATCH 2/3] No deepcopy --- .../duck/v1alpha1/subscribable_types_conversion.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/apis/duck/v1alpha1/subscribable_types_conversion.go b/pkg/apis/duck/v1alpha1/subscribable_types_conversion.go index f0597a6c4ac..c174306d312 100644 --- a/pkg/apis/duck/v1alpha1/subscribable_types_conversion.go +++ b/pkg/apis/duck/v1alpha1/subscribable_types_conversion.go @@ -96,7 +96,12 @@ func (source *SubscribableTypeStatus) ConvertTo(ctx context.Context, obj apis.Co len(source.SubscribableStatus.Subscribers) > 0 { sink.Subscribers = make([]duckv1beta1.SubscriberStatus, len(source.SubscribableStatus.Subscribers)) for i, ss := range source.SubscribableStatus.Subscribers { - sink.Subscribers[i] = *ss.DeepCopy() + sink.Subscribers[i] = duckv1beta1.SubscriberStatus{ + UID: ss.UID, + ObservedGeneration: ss.ObservedGeneration, + Ready: ss.Ready, + Message: ss.Message, + } } } default: @@ -172,7 +177,12 @@ func (sink *SubscribableTypeStatus) ConvertFrom(ctx context.Context, obj apis.Co Subscribers: make([]duckv1beta1.SubscriberStatus, len(source.Subscribers)), } for i, ss := range source.Subscribers { - sink.SubscribableStatus.Subscribers[i] = *ss.DeepCopy() + sink.SubscribableStatus.Subscribers[i] = duckv1beta1.SubscriberStatus{ + UID: ss.UID, + ObservedGeneration: ss.ObservedGeneration, + Ready: ss.Ready, + Message: ss.Message, + } } } default: From 2b2acf80b4745ff738e4904c6649fb09a1bff74b Mon Sep 17 00:00:00 2001 From: Ali Ok Date: Tue, 7 Jul 2020 09:47:57 +0300 Subject: [PATCH 3/3] Add a test case for lossy Delivery conversion --- .../subscribable_types_conversion_test.go | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/pkg/apis/duck/v1alpha1/subscribable_types_conversion_test.go b/pkg/apis/duck/v1alpha1/subscribable_types_conversion_test.go index e51d6a6520b..cdc4e4dbc7d 100644 --- a/pkg/apis/duck/v1alpha1/subscribable_types_conversion_test.go +++ b/pkg/apis/duck/v1alpha1/subscribable_types_conversion_test.go @@ -95,6 +95,14 @@ func TestSubscribableTypeConversion(t *testing.T) { BackoffDelay: pointer.StringPtr("5s"), }, }, + { + UID: "uid-2", + Generation: 8, + SubscriberURI: apis.HTTP("subscriber2.example.com"), + ReplyURI: apis.HTTP("reply2.example.com"), + DeadLetterSinkURI: apis.HTTP("subscriber2.dls.example.com"), + Delivery: nil, + }, }, }, }, @@ -123,7 +131,9 @@ func TestSubscribableTypeConversion(t *testing.T) { if err := got.ConvertFrom(context.Background(), ver); err != nil { t.Errorf("ConvertFrom() = %v", err) } - if diff := cmp.Diff(test.in, got); diff != "" { + + fixed := fixSubscribableTypeDeprecated(test.in) + if diff := cmp.Diff(fixed, got); diff != "" { t.Errorf("roundtrip (-want, +got) = %v", diff) } }) @@ -250,3 +260,20 @@ func TestSubscriberSpecConversionBadType(t *testing.T) { t.Errorf("ConvertFrom() = %#v, wanted error", good) } } + +// Since v1beta1 to v1alpha1 is lossy. +func fixSubscribableTypeDeprecated(in *SubscribableType) *SubscribableType { + if in.Spec.Subscribable != nil && len(in.Spec.Subscribable.Subscribers) > 0 { + for i := range in.Spec.Subscribable.Subscribers { + if in.Spec.Subscribable.Subscribers[i].Delivery == nil { + in.Spec.Subscribable.Subscribers[i].Delivery = &v1beta1.DeliverySpec{ + DeadLetterSink: &pkgduckv1.Destination{ + URI: in.Spec.Subscribable.Subscribers[i].DeadLetterSinkURI, + }, + } + } + } + } + + return in +}