From 6e57e7a3d20c6825495d03bb90e5cc7631c781ef Mon Sep 17 00:00:00 2001 From: nachocano Date: Wed, 22 May 2019 17:00:54 -0700 Subject: [PATCH 01/10] removing channelInformer from subscription controller as it is not needed. --- cmd/controller/main.go | 1 - pkg/reconciler/subscription/subscription.go | 9 --------- pkg/reconciler/subscription/subscription_test.go | 3 +-- 3 files changed, 1 insertion(+), 12 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 2444b02d79e..59387fc6484 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -106,7 +106,6 @@ func main() { subscription.NewController( opt, subscriptionInformer, - channelInformer, addressableInformer, ), namespace.NewController( diff --git a/pkg/reconciler/subscription/subscription.go b/pkg/reconciler/subscription/subscription.go index 60124e19b06..1bd8bbf9171 100644 --- a/pkg/reconciler/subscription/subscription.go +++ b/pkg/reconciler/subscription/subscription.go @@ -82,7 +82,6 @@ var _ controller.Reconciler = (*Reconciler)(nil) func NewController( opt reconciler.Options, subscriptionInformer eventinginformers.SubscriptionInformer, - channelInformer eventinginformers.ChannelInformer, addressableInformer eventingduck.AddressableInformer, ) *controller.Impl { @@ -99,14 +98,6 @@ func NewController( // Tracker is used to notify us when the resources Subscription depends on change, so that the // Subscription needs to reconcile again. r.tracker = tracker.New(impl.EnqueueKey, opt.GetTrackerLease()) - channelInformer.Informer().AddEventHandler(reconciler.Handler( - // Call the tracker's OnChanged method, but we've seen the objects coming through this path - // missing TypeMeta, so ensure it is properly populated. - controller.EnsureTypeMeta( - r.tracker.OnChanged, - v1alpha1.SchemeGroupVersion.WithKind("Channel"), - ), - )) return impl } diff --git a/pkg/reconciler/subscription/subscription_test.go b/pkg/reconciler/subscription/subscription_test.go index 39f1b3d401e..e7794af4e67 100644 --- a/pkg/reconciler/subscription/subscription_test.go +++ b/pkg/reconciler/subscription/subscription_test.go @@ -627,13 +627,12 @@ func TestNew(t *testing.T) { eventingInformer := informers.NewSharedInformerFactory(eventingClient, 0) subscriptionInformer := eventingInformer.Eventing().V1alpha1().Subscriptions() - channelInformer := eventingInformer.Eventing().V1alpha1().Channels() addressableInformer := &fakeAddressableInformer{} c := NewController(reconciler.Options{ KubeClientSet: kubeClient, EventingClientSet: eventingClient, Logger: logtesting.TestLogger(t), - }, subscriptionInformer, channelInformer, addressableInformer) + }, subscriptionInformer, addressableInformer) if c == nil { t.Fatal("Expected NewController to return a non-nil value") From cf934f9ddd966c2d625354cad96c6a6d7312e433 Mon Sep 17 00:00:00 2001 From: nachocano Date: Wed, 22 May 2019 17:12:27 -0700 Subject: [PATCH 02/10] properly mark subscription as not ready --- .../eventing/v1alpha1/subscription_lifecycle.go | 14 ++++++++++++++ pkg/apis/eventing/v1alpha1/subscription_types.go | 4 ---- pkg/reconciler/subscription/subscription.go | 4 ++++ 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/subscription_lifecycle.go b/pkg/apis/eventing/v1alpha1/subscription_lifecycle.go index 193673d0adc..5c996fd4df8 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_lifecycle.go +++ b/pkg/apis/eventing/v1alpha1/subscription_lifecycle.go @@ -18,6 +18,10 @@ package v1alpha1 import duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" +// subCondSet is a condition set with Ready as the happy condition and +// ReferencesResolved and ChannelReady as the dependent conditions. +var subCondSet = duckv1alpha1.NewLivingConditionSet(SubscriptionConditionReferencesResolved, SubscriptionConditionChannelReady) + const ( // SubscriptionConditionReady has status True when all subconditions below have been set to True. SubscriptionConditionReady = duckv1alpha1.ConditionReady @@ -54,3 +58,13 @@ func (ss *SubscriptionStatus) MarkReferencesResolved() { func (ss *SubscriptionStatus) MarkChannelReady() { subCondSet.Manage(ss).MarkTrue(SubscriptionConditionChannelReady) } + +// MarkReferencesNotResolved sets the ReferencesResolved condition to False state. +func (ss *SubscriptionStatus) MarkReferencesNotResolved(reason, messageFormat string, messageA ...interface{}) { + subCondSet.Manage(ss).MarkFalse(SubscriptionConditionReferencesResolved, reason, messageFormat, messageA...) +} + +// MarkChannelNotReady sets the ChannelReady condition to False state. +func (ss *SubscriptionStatus) MarkChannelNotReady(reason, messageFormat string, messageA ...interface{}) { + subCondSet.Manage(ss).MarkFalse(SubscriptionConditionChannelReady, reason, messageFormat, messageA) +} diff --git a/pkg/apis/eventing/v1alpha1/subscription_types.go b/pkg/apis/eventing/v1alpha1/subscription_types.go index 013e2fab0c6..fd2166bfa23 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types.go @@ -165,10 +165,6 @@ type ReplyStrategy struct { Channel *corev1.ObjectReference `json:"channel,omitempty"` } -// subCondSet is a condition set with Ready as the happy condition and -// ReferencesResolved and ChannelReady as the dependent conditions. -var subCondSet = duckv1alpha1.NewLivingConditionSet(SubscriptionConditionReferencesResolved, SubscriptionConditionChannelReady) - // SubscriptionStatus (computed) for a subscription type SubscriptionStatus struct { // inherits duck/v1alpha1 Status, which currently provides: diff --git a/pkg/reconciler/subscription/subscription.go b/pkg/reconciler/subscription/subscription.go index 1bd8bbf9171..42707a2e195 100644 --- a/pkg/reconciler/subscription/subscription.go +++ b/pkg/reconciler/subscription/subscription.go @@ -176,6 +176,7 @@ func (r *Reconciler) reconcile(ctx context.Context, subscription *v1alpha1.Subsc zap.Error(err), zap.Any("channel", subscription.Spec.Channel)) r.Recorder.Eventf(subscription, corev1.EventTypeWarning, channelReferenceFetchFailed, "Failed to validate spec.channel exists: %v", err) + subscription.Status.MarkReferencesNotResolved(channelReferenceFetchFailed, "Failed to validate spec.channel exists: %v", err) return err } @@ -191,6 +192,7 @@ func (r *Reconciler) reconcile(ctx context.Context, subscription *v1alpha1.Subsc zap.Error(err), zap.Any("subscriber", subscription.Spec.Subscriber)) r.Recorder.Eventf(subscription, corev1.EventTypeWarning, subscriberResolveFailed, "Failed to resolve spec.subscriber: %v", err) + subscription.Status.MarkReferencesNotResolved(subscriberResolveFailed, "Failed to resolve spec.subscriber: %v", err) return err } @@ -203,6 +205,7 @@ func (r *Reconciler) reconcile(ctx context.Context, subscription *v1alpha1.Subsc zap.Error(err), zap.Any("reply", subscription.Spec.Reply)) r.Recorder.Eventf(subscription, corev1.EventTypeWarning, resultResolveFailed, "Failed to resolve spec.reply: %v", err) + subscription.Status.MarkReferencesNotResolved(resultResolveFailed, "Failed to resolve spec.reply: %v", err) return err } @@ -221,6 +224,7 @@ func (r *Reconciler) reconcile(ctx context.Context, subscription *v1alpha1.Subsc if err := r.syncPhysicalChannel(ctx, subscription, false); err != nil { logging.FromContext(ctx).Warn("Failed to sync physical Channel", zap.Error(err)) r.Recorder.Eventf(subscription, corev1.EventTypeWarning, physicalChannelSyncFailed, "Failed to sync physical Channel: %v", err) + subscription.Status.MarkChannelNotReady(physicalChannelSyncFailed, "Failed to sync physical Channel: %v", err) return err } // Everything went well, set the fact that subscriptions have been modified From 744b4bd7240be1a18ce5814ddd320edff9a82f70 Mon Sep 17 00:00:00 2001 From: nachocano Date: Wed, 22 May 2019 17:47:50 -0700 Subject: [PATCH 03/10] marking subscription as not ready... --- .../subscription/subscription_test.go | 29 +++++++++++++++++++ pkg/reconciler/testing/subscription.go | 6 ++++ 2 files changed, 35 insertions(+) diff --git a/pkg/reconciler/subscription/subscription_test.go b/pkg/reconciler/subscription/subscription_test.go index e7794af4e67..bb912ed8d10 100644 --- a/pkg/reconciler/subscription/subscription_test.go +++ b/pkg/reconciler/subscription/subscription_test.go @@ -113,6 +113,31 @@ func TestAllCases(t *testing.T) { // WantEvents: []string{ // Eventf(corev1.EventTypeWarning, "ChannelReferenceFetchFailed", "Failed to validate spec.channel exists: s \"\" not found"), // }, + }, { + Name: "subscription, but channel does not exist", + Objects: []runtime.Object{ + NewSubscription(subscriptionName, testNS, + WithSubscriptionUID(subscriptionUID), + WithSubscriptionChannel(channelGVK, channelName), + WithSubscriptionSubscriberRef(subscriberGVK, subscriberName), + ), + NewUnstructured(subscriberGVK, subscriberName, testNS), + }, + Key: testNS + "/" + subscriptionName, + WantErr: true, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "ChannelReferenceFetchFailed", "Failed to validate spec.channel exists: channels.eventing.knative.dev %q not found", channelName), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: NewSubscription(subscriptionName, testNS, + WithSubscriptionUID(subscriptionUID), + WithSubscriptionChannel(channelGVK, channelName), + WithSubscriptionSubscriberRef(subscriberGVK, subscriberName), + // The first reconciliation will initialize the status conditions. + WithInitSubscriptionConditions, + WithSubscriptionReferencesNotResolved(channelReferenceFetchFailed, fmt.Sprintf("Failed to validate spec.channel exists: channels.eventing.knative.dev %q not found", channelName)), + ), + }}, }, { Name: "subscription, but subscriber is not addressable", Objects: []runtime.Object{ @@ -139,6 +164,7 @@ func TestAllCases(t *testing.T) { WithSubscriptionSubscriberRef(subscriberGVK, subscriberName), // The first reconciliation will initialize the status conditions. WithInitSubscriptionConditions, + WithSubscriptionReferencesNotResolved(subscriberResolveFailed, "Failed to resolve spec.subscriber: status does not contain address"), ), }}, }, { @@ -166,6 +192,7 @@ func TestAllCases(t *testing.T) { WithSubscriptionSubscriberRef(subscriberGVK, subscriberName), // The first reconciliation will initialize the status conditions. WithInitSubscriptionConditions, + WithSubscriptionReferencesNotResolved(subscriberResolveFailed, fmt.Sprintf("Failed to resolve spec.subscriber: subscribers.eventing.knative.dev %q not found", subscriberName)), ), }}, }, { @@ -198,6 +225,7 @@ func TestAllCases(t *testing.T) { // The first reconciliation will initialize the status conditions. WithInitSubscriptionConditions, WithSubscriptionPhysicalSubscriptionSubscriber(subscriberURI), + WithSubscriptionReferencesNotResolved(resultResolveFailed, fmt.Sprintf("Failed to resolve spec.reply: channels.eventing.knative.dev %q not found", replyName)), ), }}, }, { @@ -472,6 +500,7 @@ func TestAllCases(t *testing.T) { WithSubscriptionSubscriberRef(serviceGVK, serviceName), // The first reconciliation will initialize the status conditions. WithInitSubscriptionConditions, + WithSubscriptionReferencesNotResolved(subscriberResolveFailed, fmt.Sprintf("Failed to resolve spec.subscriber: services %q not found", serviceName)), ), }}, }, { diff --git a/pkg/reconciler/testing/subscription.go b/pkg/reconciler/testing/subscription.go index 91d3eb9d687..4857e8dfc44 100644 --- a/pkg/reconciler/testing/subscription.go +++ b/pkg/reconciler/testing/subscription.go @@ -143,6 +143,12 @@ func MarkSubscriptionReady(s *v1alpha1.Subscription) { s.Status.MarkReferencesResolved() } +func WithSubscriptionReferencesNotResolved(reason, msg string) SubscriptionOption { + return func(s *v1alpha1.Subscription) { + s.Status.MarkReferencesNotResolved(reason, msg) + } +} + func WithSubscriptionReply(gvk metav1.GroupVersionKind, name string) SubscriptionOption { return func(s *v1alpha1.Subscription) { s.Spec.Reply = &v1alpha1.ReplyStrategy{ From 036e7143439f64c625cbb2ec2f661b678ccc3b6d Mon Sep 17 00:00:00 2001 From: nachocano Date: Wed, 22 May 2019 18:15:47 -0700 Subject: [PATCH 04/10] rolling back the change of removing the channelInformer --- cmd/controller/main.go | 1 + pkg/reconciler/subscription/subscription.go | 9 +++++++++ pkg/reconciler/subscription/subscription_test.go | 3 ++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 59387fc6484..2444b02d79e 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -106,6 +106,7 @@ func main() { subscription.NewController( opt, subscriptionInformer, + channelInformer, addressableInformer, ), namespace.NewController( diff --git a/pkg/reconciler/subscription/subscription.go b/pkg/reconciler/subscription/subscription.go index 42707a2e195..5c2a0cd6278 100644 --- a/pkg/reconciler/subscription/subscription.go +++ b/pkg/reconciler/subscription/subscription.go @@ -82,6 +82,7 @@ var _ controller.Reconciler = (*Reconciler)(nil) func NewController( opt reconciler.Options, subscriptionInformer eventinginformers.SubscriptionInformer, + channelInformer eventinginformers.ChannelInformer, addressableInformer eventingduck.AddressableInformer, ) *controller.Impl { @@ -98,6 +99,14 @@ func NewController( // Tracker is used to notify us when the resources Subscription depends on change, so that the // Subscription needs to reconcile again. r.tracker = tracker.New(impl.EnqueueKey, opt.GetTrackerLease()) + channelInformer.Informer().AddEventHandler(reconciler.Handler( + // Call the tracker's OnChanged method, but we've seen the objects coming through this path + // missing TypeMeta, so ensure it is properly populated. + controller.EnsureTypeMeta( + r.tracker.OnChanged, + v1alpha1.SchemeGroupVersion.WithKind("Channel"), + ), + )) return impl } diff --git a/pkg/reconciler/subscription/subscription_test.go b/pkg/reconciler/subscription/subscription_test.go index bb912ed8d10..ceec5d908b7 100644 --- a/pkg/reconciler/subscription/subscription_test.go +++ b/pkg/reconciler/subscription/subscription_test.go @@ -656,12 +656,13 @@ func TestNew(t *testing.T) { eventingInformer := informers.NewSharedInformerFactory(eventingClient, 0) subscriptionInformer := eventingInformer.Eventing().V1alpha1().Subscriptions() + channelInformer := eventingInformer.Eventing().V1alpha1().Channels() addressableInformer := &fakeAddressableInformer{} c := NewController(reconciler.Options{ KubeClientSet: kubeClient, EventingClientSet: eventingClient, Logger: logtesting.TestLogger(t), - }, subscriptionInformer, addressableInformer) + }, subscriptionInformer, channelInformer, addressableInformer) if c == nil { t.Fatal("Expected NewController to return a non-nil value") From 61ebc8c9f1d097017eacdcb8485142325f95456c Mon Sep 17 00:00:00 2001 From: nachocano Date: Wed, 22 May 2019 18:17:37 -0700 Subject: [PATCH 05/10] add TODO --- pkg/reconciler/subscription/subscription.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/reconciler/subscription/subscription.go b/pkg/reconciler/subscription/subscription.go index 5c2a0cd6278..b9edd369f81 100644 --- a/pkg/reconciler/subscription/subscription.go +++ b/pkg/reconciler/subscription/subscription.go @@ -99,6 +99,7 @@ func NewController( // Tracker is used to notify us when the resources Subscription depends on change, so that the // Subscription needs to reconcile again. r.tracker = tracker.New(impl.EnqueueKey, opt.GetTrackerLease()) + // TODO further analyze if this informer can be removed. channelInformer.Informer().AddEventHandler(reconciler.Handler( // Call the tracker's OnChanged method, but we've seen the objects coming through this path // missing TypeMeta, so ensure it is properly populated. From 13d24200bd5f48b3f99bbf082b62e904a66d518a Mon Sep 17 00:00:00 2001 From: nachocano Date: Thu, 23 May 2019 10:12:05 -0700 Subject: [PATCH 06/10] removing channelInformer from subscription controller --- cmd/controller/main.go | 1 - pkg/reconciler/subscription/subscription.go | 26 +++++++------------ .../subscription/subscription_test.go | 3 +-- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 2444b02d79e..59387fc6484 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -106,7 +106,6 @@ func main() { subscription.NewController( opt, subscriptionInformer, - channelInformer, addressableInformer, ), namespace.NewController( diff --git a/pkg/reconciler/subscription/subscription.go b/pkg/reconciler/subscription/subscription.go index b9edd369f81..8b2aba55961 100644 --- a/pkg/reconciler/subscription/subscription.go +++ b/pkg/reconciler/subscription/subscription.go @@ -82,7 +82,6 @@ var _ controller.Reconciler = (*Reconciler)(nil) func NewController( opt reconciler.Options, subscriptionInformer eventinginformers.SubscriptionInformer, - channelInformer eventinginformers.ChannelInformer, addressableInformer eventingduck.AddressableInformer, ) *controller.Impl { @@ -99,15 +98,6 @@ func NewController( // Tracker is used to notify us when the resources Subscription depends on change, so that the // Subscription needs to reconcile again. r.tracker = tracker.New(impl.EnqueueKey, opt.GetTrackerLease()) - // TODO further analyze if this informer can be removed. - channelInformer.Informer().AddEventHandler(reconciler.Handler( - // Call the tracker's OnChanged method, but we've seen the objects coming through this path - // missing TypeMeta, so ensure it is properly populated. - controller.EnsureTypeMeta( - r.tracker.OnChanged, - v1alpha1.SchemeGroupVersion.WithKind("Channel"), - ), - )) return impl } @@ -180,6 +170,16 @@ func (r *Reconciler) reconcile(ctx context.Context, subscription *v1alpha1.Subsc return err } + // Track the channel using the addressableInformer. + // We don't need the explicitly set a channelInformer, as this will dynamically generate one for us. + // This code needs to be called before checking the existence of the `channel`, in order to make sure the + // subscription controller will reconcile upon a `channel` change. + track := r.addressableInformer.TrackInNamespace(r.tracker, subscription) + if err := track(subscription.Spec.Channel); err != nil { + logging.FromContext(ctx).Error("Unable to track changes to spec.channel", zap.Error(err)) + return err + } + // Verify that `channel` exists. if _, err := eventingduck.ObjectReference(ctx, r.DynamicClientSet, subscription.Namespace, &subscription.Spec.Channel); err != nil { logging.FromContext(ctx).Warn("Failed to validate Channel exists", @@ -190,12 +190,6 @@ func (r *Reconciler) reconcile(ctx context.Context, subscription *v1alpha1.Subsc return err } - track := r.addressableInformer.TrackInNamespace(r.tracker, subscription) - if err := track(subscription.Spec.Channel); err != nil { - logging.FromContext(ctx).Error("Unable to track changes to spec.channel", zap.Error(err)) - return err - } - subscriberURI, err := eventingduck.SubscriberSpec(ctx, r.DynamicClientSet, subscription.Namespace, subscription.Spec.Subscriber, track) if err != nil { logging.FromContext(ctx).Warn("Failed to resolve Subscriber", diff --git a/pkg/reconciler/subscription/subscription_test.go b/pkg/reconciler/subscription/subscription_test.go index ceec5d908b7..bb912ed8d10 100644 --- a/pkg/reconciler/subscription/subscription_test.go +++ b/pkg/reconciler/subscription/subscription_test.go @@ -656,13 +656,12 @@ func TestNew(t *testing.T) { eventingInformer := informers.NewSharedInformerFactory(eventingClient, 0) subscriptionInformer := eventingInformer.Eventing().V1alpha1().Subscriptions() - channelInformer := eventingInformer.Eventing().V1alpha1().Channels() addressableInformer := &fakeAddressableInformer{} c := NewController(reconciler.Options{ KubeClientSet: kubeClient, EventingClientSet: eventingClient, Logger: logtesting.TestLogger(t), - }, subscriptionInformer, channelInformer, addressableInformer) + }, subscriptionInformer, addressableInformer) if c == nil { t.Fatal("Expected NewController to return a non-nil value") From 0f633978002683c9dda82ccaca96c70eece95b98 Mon Sep 17 00:00:00 2001 From: nachocano Date: Thu, 23 May 2019 13:47:22 -0700 Subject: [PATCH 07/10] Allow creating subscriptions to CRD channels. --- config/300-subscription.yaml | 3 ++- .../subscribable_channelable_validation.go | 13 +++++++------ .../subscribable_channelable_validation_test.go | 15 ++++++++++++--- pkg/apis/eventing/v1alpha1/subscription_types.go | 4 ++-- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/config/300-subscription.yaml b/config/300-subscription.yaml index 19f5d7f7d98..a29fe164f81 100644 --- a/config/300-subscription.yaml +++ b/config/300-subscription.yaml @@ -62,7 +62,8 @@ spec: minLength: 1 kind: type: string - pattern: "^Channel$" + # Ending with Channel. E.g., Channel, InMemoryChannel, etc. + pattern: "Channel$" name: type: string minLength: 1 diff --git a/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation.go b/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation.go index b415b68a183..8585e22009c 100644 --- a/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation.go +++ b/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation.go @@ -18,6 +18,7 @@ package v1alpha1 import ( "reflect" + "strings" "github.com/google/go-cmp/cmp" "github.com/knative/pkg/apis" @@ -30,21 +31,21 @@ func isChannelEmpty(f corev1.ObjectReference) bool { } // Valid from only contains the following fields: -// - Kind == 'Channel' -// - APIVersion == 'eventing.knative.dev/v1alpha1' +// - Kind ends in 'Channel', e.g., 'Channel', 'InMemoryChannel', etc. +// - APIVersion == 'eventing.knative.dev/v1alpha1' || 'messaging.knative.dev/v1alpha1' // - Name == not empty func isValidChannel(f corev1.ObjectReference) *apis.FieldError { errs := isValidObjectReference(f) - if f.Kind != "Channel" { + if !strings.HasSuffix(f.Kind, "Channel") { fe := apis.ErrInvalidValue(f.Kind, "kind") fe.Paths = []string{"kind"} - fe.Details = "only 'Channel' kind is allowed" + fe.Details = "only 'Channel$' kind is allowed" errs = errs.Also(fe) } - if f.APIVersion != "eventing.knative.dev/v1alpha1" { + if f.APIVersion != "eventing.knative.dev/v1alpha1" && f.APIVersion != "messaging.knative.dev/v1alpha1" { fe := apis.ErrInvalidValue(f.APIVersion, "apiVersion") - fe.Details = "only eventing.knative.dev/v1alpha1 is allowed for apiVersion" + fe.Details = "only eventing.knative.dev/v1alpha1 or messaging.knative.dev/v1alpha1 are allowed for apiVersion" errs = errs.Also(fe) } return errs diff --git a/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation_test.go b/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation_test.go index 1607bcd5035..e05f682133b 100644 --- a/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation_test.go +++ b/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation_test.go @@ -39,7 +39,7 @@ var validationTests = []struct { want: &apis.FieldError{ Message: "invalid value: Strait", Paths: []string{"kind"}, - Details: "only 'Channel' kind is allowed", + Details: "only 'Channel$' kind is allowed", }, }, { @@ -52,8 +52,8 @@ var validationTests = []struct { want: &apis.FieldError{ Message: `invalid value: eventing.knative.dev/v1alpha2`, Paths: []string{"apiVersion"}, - Details: "only eventing.knative.dev/v1alpha1 " + - "is allowed for apiVersion", + Details: "only eventing.knative.dev/v1alpha1 or messaging.knative.dev/v1alpha1 " + + "are allowed for apiVersion", }, }, { @@ -65,6 +65,15 @@ var validationTests = []struct { }, want: nil, }, + { + name: "valid channel messaging", + ref: corev1.ObjectReference{ + Name: "boaty-mcboatface", + APIVersion: "messaging.knative.dev/v1alpha1", + Kind: "InMemoryChannel", + }, + want: nil, + }, } func TestIsChannelEmpty(t *testing.T) { diff --git a/pkg/apis/eventing/v1alpha1/subscription_types.go b/pkg/apis/eventing/v1alpha1/subscription_types.go index fd2166bfa23..472d989e01b 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types.go @@ -76,8 +76,8 @@ type SubscriptionSpec struct { // - Kind // - APIVersion // - Name - // Kind must be "Channel" and APIVersion must be - // "eventing.knative.dev/v1alpha1" + // Kind must end in "Channel". E.g., "Channel", "InMemoryChannel", etc. + // APIVersion must be "eventing.knative.dev/v1alpha1" or ""messaging.knative.dev/v1alpha1". // // This field is immutable. We have no good answer on what happens to // the events that are currently in the channel being consumed from From 62480f32a4f0c47b3938e0a23307334910ae27ca Mon Sep 17 00:00:00 2001 From: nachocano Date: Thu, 23 May 2019 14:07:51 -0700 Subject: [PATCH 08/10] updating UTs --- .../eventing/v1alpha1/subscription_validation_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/subscription_validation_test.go b/pkg/apis/eventing/v1alpha1/subscription_validation_test.go index d1dcc7ae35c..a4e3ae3b606 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_validation_test.go +++ b/pkg/apis/eventing/v1alpha1/subscription_validation_test.go @@ -421,7 +421,7 @@ func TestValidChannel(t *testing.T) { }, want: func() *apis.FieldError { fe := apis.ErrInvalidValue("", "apiVersion") - fe.Details = "only eventing.knative.dev/v1alpha1 is allowed for apiVersion" + fe.Details = "only eventing.knative.dev/v1alpha1 or messaging.knative.dev/v1alpha1 are allowed for apiVersion" return apis.ErrMissingField("apiVersion").Also(fe) }(), }, { @@ -432,7 +432,7 @@ func TestValidChannel(t *testing.T) { }, want: func() *apis.FieldError { fe := apis.ErrInvalidValue("", "kind") - fe.Details = "only 'Channel' kind is allowed" + fe.Details = "only 'Channel$' kind is allowed" return apis.ErrMissingField("kind").Also(fe) }(), }, { @@ -444,7 +444,7 @@ func TestValidChannel(t *testing.T) { }, want: func() *apis.FieldError { fe := apis.ErrInvalidValue("subscription", "kind") - fe.Details = "only 'Channel' kind is allowed" + fe.Details = "only 'Channel$' kind is allowed" return fe }(), }, { @@ -456,7 +456,7 @@ func TestValidChannel(t *testing.T) { }, want: func() *apis.FieldError { fe := apis.ErrInvalidValue("wrongapiversion", "apiVersion") - fe.Details = "only eventing.knative.dev/v1alpha1 is allowed for apiVersion" + fe.Details = "only eventing.knative.dev/v1alpha1 or messaging.knative.dev/v1alpha1 are allowed for apiVersion" return fe }(), }, { From 3085dfb23457fc710bef51d7f8a218697a218835 Mon Sep 17 00:00:00 2001 From: nachocano Date: Thu, 23 May 2019 14:10:12 -0700 Subject: [PATCH 09/10] cosmetic --- pkg/apis/eventing/v1alpha1/subscription_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/eventing/v1alpha1/subscription_types.go b/pkg/apis/eventing/v1alpha1/subscription_types.go index 472d989e01b..445a24eefa5 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types.go @@ -77,7 +77,7 @@ type SubscriptionSpec struct { // - APIVersion // - Name // Kind must end in "Channel". E.g., "Channel", "InMemoryChannel", etc. - // APIVersion must be "eventing.knative.dev/v1alpha1" or ""messaging.knative.dev/v1alpha1". + // APIVersion must be "eventing.knative.dev/v1alpha1" or "messaging.knative.dev/v1alpha1". // // This field is immutable. We have no good answer on what happens to // the events that are currently in the channel being consumed from From 583c6e0ce76fe4f0141feab60c35e926d9b34f49 Mon Sep 17 00:00:00 2001 From: nachocano Date: Thu, 23 May 2019 15:18:24 -0700 Subject: [PATCH 10/10] adding TODOs --- config/300-subscription.yaml | 1 + .../eventing/v1alpha1/subscribable_channelable_validation.go | 1 + 2 files changed, 2 insertions(+) diff --git a/config/300-subscription.yaml b/config/300-subscription.yaml index a29fe164f81..7e23623eba2 100644 --- a/config/300-subscription.yaml +++ b/config/300-subscription.yaml @@ -62,6 +62,7 @@ spec: minLength: 1 kind: type: string + # TODO remove this once we check for subscribable type. # Ending with Channel. E.g., Channel, InMemoryChannel, etc. pattern: "Channel$" name: diff --git a/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation.go b/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation.go index 8585e22009c..75a7993d80f 100644 --- a/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation.go +++ b/pkg/apis/eventing/v1alpha1/subscribable_channelable_validation.go @@ -37,6 +37,7 @@ func isChannelEmpty(f corev1.ObjectReference) bool { func isValidChannel(f corev1.ObjectReference) *apis.FieldError { errs := isValidObjectReference(f) + // TODO check whether is subscribable instead of doing this suffix match. if !strings.HasSuffix(f.Kind, "Channel") { fe := apis.ErrInvalidValue(f.Kind, "kind") fe.Paths = []string{"kind"}