From 7c0b4770144e0fd3d026f1b4884242abaa9bd282 Mon Sep 17 00:00:00 2001 From: Lu Dang Date: Tue, 13 Apr 2021 08:18:00 -0400 Subject: [PATCH] fix(PingSource): remove annotation based conversion logic (#5234) - v1beta1 will not guaranteed to be roundtrip-able (in the case that jsonData cannot be marshalled into json, e.g. jsonData="hello"), but the cloudevent on the wire stays the same. - duckv1.SourceSpec, TimeZone, Schedule will be populated when called by v1beta1 clients - object with v1beta2-specific features (dataBase64, contentType beyond `application/json`) will not populate jsonData field when converted to v1beta1. --- pkg/apis/sources/v1beta1/ping_conversion.go | 131 +------ .../sources/v1beta1/ping_conversion_test.go | 346 ++++++++++++------ 2 files changed, 252 insertions(+), 225 deletions(-) diff --git a/pkg/apis/sources/v1beta1/ping_conversion.go b/pkg/apis/sources/v1beta1/ping_conversion.go index 60beedb66c0..44bd379e920 100644 --- a/pkg/apis/sources/v1beta1/ping_conversion.go +++ b/pkg/apis/sources/v1beta1/ping_conversion.go @@ -20,7 +20,6 @@ import ( "context" "encoding/json" "fmt" - "reflect" cloudevents "github.com/cloudevents/sdk-go/v2" @@ -28,16 +27,6 @@ import ( "knative.dev/pkg/apis" ) -const ( - // V1B1SpecAnnotationKey is used to indicate that a v1beta2 object is converted from v1beta1 - // also it can be used to downgrade such object to v1beta1 - V1B1SpecAnnotationKey = "pingsources.sources.knative.dev/v1beta1-spec" - - // V1B2SpecAnnotationKey is used to indicate that a v1beta1 object is converted from v1beta2 - // also it can be used to convert the v1beta1 object back to v1beta2, considering that v1beta2 introduces more features. - V1B2SpecAnnotationKey = "pingsources.sources.knative.dev/v1beta2-spec" -) - type message struct { Body string `json:"body"` } @@ -61,35 +50,21 @@ func (source *PingSource) ConvertTo(ctx context.Context, obj apis.Convertible) e sink.Status = v1beta2.PingSourceStatus{ SourceStatus: source.Status.SourceStatus, } - - // deep copy annotations to avoid mutation on source.ObjectMeta.Annotations - annotations := make(map[string]string) - for key, value := range source.GetAnnotations() { - annotations[key] = value + sink.Spec = v1beta2.PingSourceSpec{ + SourceSpec: source.Spec.SourceSpec, + Schedule: source.Spec.Schedule, + Timezone: source.Spec.Timezone, } - if isCreatedViaV1Beta2API(source) { - // try to unmarshal v1beta2.PingSource.Spec from V1B2SpecAnnotationKey - // key existence and json marshal error already checked in isCreatedViaV1Beta2API - v1beta2Spec := annotations[V1B2SpecAnnotationKey] - _ = json.Unmarshal([]byte(v1beta2Spec), &sink.Spec) - } else { - var err error - if sink.Spec, err = toV1Beta2Spec(&source.Spec); err != nil { - return err - } - // marshal and store v1beta1.PingSource.Spec into V1B1SpecAnnotationKey - // this is to help if we need to convert back to v1beta1.PingSource - v1beta1Spec, err := json.Marshal(source.Spec) + if source.Spec.JsonData != "" { + msg, err := makeMessage(source.Spec.JsonData) if err != nil { - return fmt.Errorf("error marshalling source.Spec: %v, err: %v", source.Spec, err) + return fmt.Errorf("error converting jsonData to a higher version: %v", err) } - annotations[V1B1SpecAnnotationKey] = string(v1beta1Spec) + sink.Spec.ContentType = cloudevents.ApplicationJSON + sink.Spec.Data = string(msg) } - // we don't need this annotation in a v1beta2.PingSource object - delete(annotations, V1B2SpecAnnotationKey) - sink.SetAnnotations(annotations) return nil default: return apis.ConvertToViaProxy(ctx, source, &v1beta2.PingSource{}, sink) @@ -106,94 +81,18 @@ func (sink *PingSource) ConvertFrom(ctx context.Context, obj apis.Convertible) e SourceStatus: source.Status.SourceStatus, } - // deep copy annotations to avoid mutation on source.ObjectMeta.Annotations - annotations := make(map[string]string) - for key, value := range source.GetAnnotations() { - annotations[key] = value - } - - if isV1Beta1AnnotationConsistentWithV1Beta2Spec(source) { - // errors already handled in isV1Beta1AnnotationConsistentWithV1Beta2Spec - v1beta1Spec := annotations[V1B1SpecAnnotationKey] - _ = json.Unmarshal([]byte(v1beta1Spec), &sink.Spec) + sink.Spec = PingSourceSpec{ + SourceSpec: source.Spec.SourceSpec, + Schedule: source.Spec.Schedule, + Timezone: source.Spec.Timezone, } - // marshal and store v1beta2.PingSource.Spec into V1B2SpecAnnotationKey - // this is to help if we need to convert back to v1beta2.PingSource - v1beta2Configuration, err := json.Marshal(source.Spec) - if err != nil { - return fmt.Errorf("error marshalling source.Spec: %v, err: %v", source.Spec, err) + if source.Spec.ContentType == cloudevents.ApplicationJSON { + sink.Spec.JsonData = source.Spec.Data } - annotations[V1B2SpecAnnotationKey] = string(v1beta2Configuration) - // we don't need this annotation in a v1beta1.PingSource object - delete(annotations, V1B1SpecAnnotationKey) - sink.SetAnnotations(annotations) return nil default: return apis.ConvertFromViaProxy(ctx, source, &v1beta2.PingSource{}, sink) } } - -func toV1Beta2Spec(srcSpec *PingSourceSpec) (v1beta2.PingSourceSpec, error) { - targetSpec := v1beta2.PingSourceSpec{ - SourceSpec: srcSpec.SourceSpec, - Schedule: srcSpec.Schedule, - Timezone: srcSpec.Timezone, - } - - if srcSpec.JsonData != "" { - msg, err := makeMessage(srcSpec.JsonData) - if err != nil { - return targetSpec, fmt.Errorf("error converting jsonData to a higher version: %v", err) - } - targetSpec.ContentType = cloudevents.ApplicationJSON - targetSpec.Data = string(msg) - } - - return targetSpec, nil -} - -// checks if a v1beta1.PingSource is originally created in v1beta2, it must meet both of the following criteria: -// -// 1. V1B2SpecAnnotationKey annotation must exist and can be unmarshalled to v1beta2.PingSourceSpec, it indicates that it's converted from v1beta2 -> v1beta1. -// 2. Spec.Sink must be {Ref: nil, URI: nil}, as we don't set these values during conversion from v1beta2 -> v1beta1, see PingSource.ConvertFrom; -func isCreatedViaV1Beta2API(source *PingSource) bool { - v1beta2Annotation, ok := source.GetAnnotations()[V1B2SpecAnnotationKey] - if !ok { - return false - } - - v1beta2Spec := &v1beta2.PingSourceSpec{} - if err := json.Unmarshal([]byte(v1beta2Annotation), v1beta2Spec); err != nil { - return false - } - - return source.Spec.Sink.Ref == nil && source.Spec.Sink.URI == nil -} - -// for a v1beta2.PingSource, checks if its V1B1SpecAnnotationKey is consistent with its spec. -// returns false if one of the following satisfies: -// -// 1. V1B1SpecAnnotationKey does not exist. -// 2. V1B1SpecAnnotationKey exists, but we cannot unmarshal it to v1beta1.PingSourceSpec. -// 3. V1B1SpecAnnotationKey exists, but if we unmarshal it to v1beta1.PingSourceSpec and convert it to v1beta2, -// the converted v1beta2.PingSourceSpec is not the same as source.Spec. -func isV1Beta1AnnotationConsistentWithV1Beta2Spec(source *v1beta2.PingSource) bool { - v1beta1Annotation, ok := source.GetAnnotations()[V1B1SpecAnnotationKey] - if !ok { - return false - } - - v1beta1Spec := &PingSourceSpec{} - if err := json.Unmarshal([]byte(v1beta1Annotation), v1beta1Spec); err != nil { - return false - } - - v1beta2Spec, err := toV1Beta2Spec(v1beta1Spec) - if err != nil { - return false - } - - return reflect.DeepEqual(v1beta2Spec, source.Spec) -} diff --git a/pkg/apis/sources/v1beta1/ping_conversion_test.go b/pkg/apis/sources/v1beta1/ping_conversion_test.go index 6e070c6ff58..999ea8db313 100644 --- a/pkg/apis/sources/v1beta1/ping_conversion_test.go +++ b/pkg/apis/sources/v1beta1/ping_conversion_test.go @@ -18,7 +18,6 @@ package v1beta1 import ( "context" - "encoding/json" "testing" cloudevents "github.com/cloudevents/sdk-go/v2" @@ -123,16 +122,97 @@ func TestPingSourceConversionRoundTripUp(t *testing.T) { }, }, }, - }, { - "full with jsonData that cannot be unmarshalled", - &PingSource{ - ObjectMeta: meta, + }} + + for _, test := range tests { + for _, version := range versions { + t.Run(test.name, func(t *testing.T) { + ver := version + if err := test.in.ConvertTo(context.Background(), ver); err != nil { + t.Error("ConvertTo() =", err) + } + + got := &PingSource{} + + if err := got.ConvertFrom(context.Background(), ver); err != nil { + t.Error("ConvertFrom() =", err) + } + + if diff := cmp.Diff(test.in, got); diff != "" { + t.Error("roundtrip (-want, +got) =", diff) + } + }) + } + } +} + +// one way conversion: v1beta1 -> higher version +func TestPingSourceConversionOneWayUp(t *testing.T) { + path := apis.HTTP("") + path.Path = "/path" + + sinkUri := apis.HTTP("example.com") + sinkUri.Path = "path" + sink := duckv1.Destination{ + Ref: &duckv1.KReference{ + Kind: "Foo", + Namespace: "Bar", + Name: "Baz", + APIVersion: "Baf", + }, + URI: path, + } + + ceOverrides := duckv1.CloudEventOverrides{ + Extensions: map[string]string{ + "foo": "bar", + "baz": "baf", + }, + } + + ceAttributes := []duckv1.CloudEventAttributes{{ + Type: PingSourceEventType, + Source: PingSourceSource("ping-ns", "ping-name"), + }} + + tests := []struct { + name string + in *PingSource + out *v1beta2.PingSource + }{{name: "empty", + in: &PingSource{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ping-name", + Namespace: "ping-ns", + Generation: 17, + }, + Spec: PingSourceSpec{}, + Status: PingSourceStatus{}, + }, + out: &v1beta2.PingSource{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ping-name", + Namespace: "ping-ns", + Generation: 17, + }, + Spec: v1beta2.PingSourceSpec{}, + Status: v1beta2.PingSourceStatus{}, + }, + }, {name: "full configuration: marshalable jsonData", + in: &PingSource{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ping-name", + Namespace: "ping-ns", + Generation: 17, + }, Spec: PingSourceSpec{ SourceSpec: duckv1.SourceSpec{ - Sink: sink, + Sink: sink, + CloudEventOverrides: &ceOverrides, }, - Schedule: "* * * * *", - JsonData: "hello", + Schedule: "1 2 3 4 5", + Timezone: "Knative/Land", + JsonData: `{"foo":"bar"}`, }, Status: PingSourceStatus{ SourceStatus: duckv1.SourceStatus{ @@ -143,26 +223,55 @@ func TestPingSourceConversionRoundTripUp(t *testing.T) { Status: "True", }}, }, - SinkURI: sinkUri, + SinkURI: sinkUri, + CloudEventAttributes: ceAttributes, }, }, }, - }, { - "full with invalid v1beta2 spec annotation", - &PingSource{ + out: &v1beta2.PingSource{ ObjectMeta: metav1.ObjectMeta{ Name: "ping-name", Namespace: "ping-ns", Generation: 17, - Annotations: map[string]string{ - V1B2SpecAnnotationKey: "$$ invalid json $$", + }, + Spec: v1beta2.PingSourceSpec{ + SourceSpec: duckv1.SourceSpec{ + Sink: sink, + CloudEventOverrides: &ceOverrides, + }, + Schedule: "1 2 3 4 5", + Timezone: "Knative/Land", + ContentType: cloudevents.ApplicationJSON, + Data: `{"foo":"bar"}`, + }, + Status: v1beta2.PingSourceStatus{ + SourceStatus: duckv1.SourceStatus{ + Status: duckv1.Status{ + ObservedGeneration: 1, + Conditions: duckv1.Conditions{{ + Type: "Ready", + Status: "True", + }}, + }, + SinkURI: sinkUri, + CloudEventAttributes: ceAttributes, }, }, + }, + }, {name: "full configuration: unmarshalable jsonData", + in: &PingSource{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ping-name", + Namespace: "ping-ns", + Generation: 17, + }, Spec: PingSourceSpec{ SourceSpec: duckv1.SourceSpec{ - Sink: sink, + Sink: sink, + CloudEventOverrides: &ceOverrides, }, - Schedule: "* * * * *", + Schedule: "1 2 3 4 5", + Timezone: "Knative/Land", JsonData: "hello", }, Status: PingSourceStatus{ @@ -174,36 +283,59 @@ func TestPingSourceConversionRoundTripUp(t *testing.T) { Status: "True", }}, }, - SinkURI: sinkUri, + SinkURI: sinkUri, + CloudEventAttributes: ceAttributes, + }, + }, + }, + out: &v1beta2.PingSource{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ping-name", + Namespace: "ping-ns", + Generation: 17, + }, + Spec: v1beta2.PingSourceSpec{ + SourceSpec: duckv1.SourceSpec{ + Sink: sink, + CloudEventOverrides: &ceOverrides, + }, + Schedule: "1 2 3 4 5", + Timezone: "Knative/Land", + ContentType: cloudevents.ApplicationJSON, + Data: `{"body":"hello"}`, + }, + Status: v1beta2.PingSourceStatus{ + SourceStatus: duckv1.SourceStatus{ + Status: duckv1.Status{ + ObservedGeneration: 1, + Conditions: duckv1.Conditions{{ + Type: "Ready", + Status: "True", + }}, + }, + SinkURI: sinkUri, + CloudEventAttributes: ceAttributes, }, }, }, }} for _, test := range tests { - for _, version := range versions { - t.Run(test.name, func(t *testing.T) { - ver := version - if err := test.in.ConvertTo(context.Background(), ver); err != nil { - t.Error("ConvertTo() =", err) - } - - got := &PingSource{} - - if err := got.ConvertFrom(context.Background(), ver); err != nil { - t.Error("ConvertFrom() =", err) - } + t.Run(test.name, func(t *testing.T) { + result := &v1beta2.PingSource{} + if err := test.in.ConvertTo(context.Background(), result); err != nil { + t.Error("ConvertFrom() =", err) + } - if diff := diffIgnoringAnnotations(test.in, got); diff != "" { - t.Error("roundtrip (-want, +got) =", diff) - } - }) - } + if diff := cmp.Diff(test.out, result); diff != "" { + t.Error("one-way up conversion (-want, +got) =", diff) + } + }) } } -// This tests round tripping from a higher version -> v1beta1 and back to the higher version. -func TestPingSourceConversionRoundTripDown(t *testing.T) { +// one way conversion: higher version -> v1beta1 +func TestPingSourceConversionOneWayDown(t *testing.T) { path := apis.HTTP("") path.Path = "/path" @@ -234,6 +366,7 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { tests := []struct { name string in *v1beta2.PingSource + out *PingSource }{{name: "empty", in: &v1beta2.PingSource{ ObjectMeta: metav1.ObjectMeta{ @@ -244,7 +377,16 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { Spec: v1beta2.PingSourceSpec{}, Status: v1beta2.PingSourceStatus{}, }, - }, {name: "simple configuration", + out: &PingSource{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ping-name", + Namespace: "ping-ns", + Generation: 17, + }, + Spec: PingSourceSpec{}, + Status: PingSourceStatus{}, + }, + }, {name: "full configuration: json", in: &v1beta2.PingSource{ ObjectMeta: metav1.ObjectMeta{ Name: "ping-name", @@ -253,8 +395,13 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { }, Spec: v1beta2.PingSourceSpec{ SourceSpec: duckv1.SourceSpec{ - Sink: sink, + Sink: sink, + CloudEventOverrides: &ceOverrides, }, + Schedule: "1 2 3 4 5", + Timezone: "Knative/Land", + ContentType: cloudevents.ApplicationJSON, + Data: `{"foo":"bar"}`, }, Status: v1beta2.PingSourceStatus{ SourceStatus: duckv1.SourceStatus{ @@ -265,28 +412,27 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { Status: "True", }}, }, - SinkURI: sinkUri, + SinkURI: sinkUri, + CloudEventAttributes: ceAttributes, }, }, }, - }, {name: "full: v1beta1 annotation does not exist", - in: &v1beta2.PingSource{ + out: &PingSource{ ObjectMeta: metav1.ObjectMeta{ Name: "ping-name", Namespace: "ping-ns", Generation: 17, }, - Spec: v1beta2.PingSourceSpec{ + Spec: PingSourceSpec{ SourceSpec: duckv1.SourceSpec{ Sink: sink, CloudEventOverrides: &ceOverrides, }, - Schedule: "1 2 3 4 5", - Timezone: "Knative/Land", - ContentType: cloudevents.ApplicationJSON, - Data: `{"foo":"bar"}`, + Schedule: "1 2 3 4 5", + Timezone: "Knative/Land", + JsonData: `{"foo":"bar"}`, }, - Status: v1beta2.PingSourceStatus{ + Status: PingSourceStatus{ SourceStatus: duckv1.SourceStatus{ Status: duckv1.Status{ ObservedGeneration: 1, @@ -300,23 +446,12 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { }, }, }, - }, {name: "full: v1beta1 spec annotation is inconsistent with v1beta2 spec", + }, {name: "full configuration: xml payload", in: &v1beta2.PingSource{ ObjectMeta: metav1.ObjectMeta{ Name: "ping-name", Namespace: "ping-ns", Generation: 17, - Annotations: map[string]string{ - V1B1SpecAnnotationKey: toJSON(PingSourceSpec{ - SourceSpec: duckv1.SourceSpec{ - Sink: sink, - CloudEventOverrides: &ceOverrides, - }, - Schedule: "1 2 3 4 5", - Timezone: "Knative/GoLand", - JsonData: `{"foo":"bar"}`, - }), - }, }, Spec: v1beta2.PingSourceSpec{ SourceSpec: duckv1.SourceSpec{ @@ -325,8 +460,8 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { }, Schedule: "1 2 3 4 5", Timezone: "Knative/Land", - ContentType: cloudevents.ApplicationJSON, - Data: `{"foo":"bar"}`, + ContentType: cloudevents.ApplicationXML, + Data: "hello world", }, Status: v1beta2.PingSourceStatus{ SourceStatus: duckv1.SourceStatus{ @@ -342,35 +477,21 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { }, }, }, - }, {name: "full: v1beta1 spec annotation is consistent with v1beta2 spec", - in: &v1beta2.PingSource{ + out: &PingSource{ ObjectMeta: metav1.ObjectMeta{ Name: "ping-name", Namespace: "ping-ns", Generation: 17, - Annotations: map[string]string{ - V1B1SpecAnnotationKey: toJSON(PingSourceSpec{ - SourceSpec: duckv1.SourceSpec{ - Sink: sink, - CloudEventOverrides: &ceOverrides, - }, - Schedule: "1 2 3 4 5", - Timezone: "Knative/Land", - JsonData: `{"foo":"bar"}`, - }), - }, }, - Spec: v1beta2.PingSourceSpec{ + Spec: PingSourceSpec{ SourceSpec: duckv1.SourceSpec{ Sink: sink, CloudEventOverrides: &ceOverrides, }, - Schedule: "1 2 3 4 5", - Timezone: "Knative/Land", - ContentType: cloudevents.ApplicationJSON, - Data: `{"foo":"bar"}`, + Schedule: "1 2 3 4 5", + Timezone: "Knative/Land", }, - Status: v1beta2.PingSourceStatus{ + Status: PingSourceStatus{ SourceStatus: duckv1.SourceStatus{ Status: duckv1.Status{ ObservedGeneration: 1, @@ -384,15 +505,12 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { }, }, }, - }, {name: "full: v1beta1 spec annotation is invalid", + }, {name: "full configuration: binary payload", in: &v1beta2.PingSource{ ObjectMeta: metav1.ObjectMeta{ Name: "ping-name", Namespace: "ping-ns", Generation: 17, - Annotations: map[string]string{ - V1B1SpecAnnotationKey: "$$ invalid json $$", - }, }, Spec: v1beta2.PingSourceSpec{ SourceSpec: duckv1.SourceSpec{ @@ -401,8 +519,8 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { }, Schedule: "1 2 3 4 5", Timezone: "Knative/Land", - ContentType: cloudevents.ApplicationJSON, - Data: `{"body":"hello"}`, + ContentType: cloudevents.TextPlain, + Data: "ZGF0YQ==", }, Status: v1beta2.PingSourceStatus{ SourceStatus: duckv1.SourceStatus{ @@ -418,36 +536,46 @@ func TestPingSourceConversionRoundTripDown(t *testing.T) { }, }, }, + out: &PingSource{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ping-name", + Namespace: "ping-ns", + Generation: 17, + }, + Spec: PingSourceSpec{ + SourceSpec: duckv1.SourceSpec{ + Sink: sink, + CloudEventOverrides: &ceOverrides, + }, + Schedule: "1 2 3 4 5", + Timezone: "Knative/Land", + }, + Status: PingSourceStatus{ + SourceStatus: duckv1.SourceStatus{ + Status: duckv1.Status{ + ObservedGeneration: 1, + Conditions: duckv1.Conditions{{ + Type: "Ready", + Status: "True", + }}, + }, + SinkURI: sinkUri, + CloudEventAttributes: ceAttributes, + }, + }, + }, }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { - down := &PingSource{} - if err := down.ConvertFrom(context.Background(), test.in); err != nil { - t.Error("ConvertTo() =", err) - } - - got := &v1beta2.PingSource{} - - if err := down.ConvertTo(context.Background(), got); err != nil { + result := &PingSource{} + if err := result.ConvertFrom(context.Background(), test.in); err != nil { t.Error("ConvertFrom() =", err) } - if diff := diffIgnoringAnnotations(test.in, got); diff != "" { - t.Error("roundtrip (-want, +got) =", diff) + + if diff := cmp.Diff(test.out, result); diff != "" { + t.Error("one-way down conversion (-want, +got) =", diff) } }) } } - -// returns the diff of want and got, but ignoring the difference of annotations. -func diffIgnoringAnnotations(want metav1.Object, got metav1.Object) string { - want.SetAnnotations(nil) - got.SetAnnotations(nil) - return cmp.Diff(want, got) -} - -// marshal an interface to JSON string, ignoring error -func toJSON(obj interface{}) string { - marshalled, _ := json.Marshal(obj) - return string(marshalled) -}