From 7aed22b31750008717eb2ba8b8b15c6d964ae2a4 Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Tue, 21 Sep 2021 19:55:19 +0200 Subject: [PATCH] Default dead letter sink namespace to the object namespace Signed-off-by: Pierangelo Di Pilato --- pkg/apis/duck/v1/delivery_defaults.go | 28 ++++++ pkg/apis/duck/v1/delivery_defaults_test.go | 85 +++++++++++++++++ pkg/apis/eventing/v1/broker_defaults.go | 4 +- pkg/apis/eventing/v1/broker_defaults_test.go | 59 +++++++++++- pkg/apis/eventing/v1/trigger_defaults.go | 1 + pkg/apis/eventing/v1/trigger_defaults_test.go | 46 ++++++++++ pkg/apis/messaging/v1/channel_defaults.go | 4 +- .../v1/in_memory_channel_defaults.go | 5 +- .../v1/in_memory_channel_defaults_test.go | 38 ++++++++ .../messaging/v1/subscription_defaults.go | 13 ++- .../v1/subscription_defaults_test.go | 92 ++++++++++++++++++- 11 files changed, 369 insertions(+), 6 deletions(-) create mode 100644 pkg/apis/duck/v1/delivery_defaults.go create mode 100644 pkg/apis/duck/v1/delivery_defaults_test.go diff --git a/pkg/apis/duck/v1/delivery_defaults.go b/pkg/apis/duck/v1/delivery_defaults.go new file mode 100644 index 00000000000..b06c792ef89 --- /dev/null +++ b/pkg/apis/duck/v1/delivery_defaults.go @@ -0,0 +1,28 @@ +/* +Copyright 2021 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1 + +import "context" + +func (ds *DeliverySpec) SetDefaults(ctx context.Context) { + if ds == nil { + return + } + if ds.DeadLetterSink != nil { + ds.DeadLetterSink.SetDefaults(ctx) + } +} diff --git a/pkg/apis/duck/v1/delivery_defaults_test.go b/pkg/apis/duck/v1/delivery_defaults_test.go new file mode 100644 index 00000000000..7f77f8fc3cc --- /dev/null +++ b/pkg/apis/duck/v1/delivery_defaults_test.go @@ -0,0 +1,85 @@ +/* +Copyright 2021 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1 + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/apis" + duckv1 "knative.dev/pkg/apis/duck/v1" +) + +func TestDeliverySpecSetDefaults(t *testing.T) { + t.Parallel() + + tt := []struct { + name string + given *DeliverySpec + want *DeliverySpec + ctx context.Context + }{ + { + name: "nil", + ctx: context.Background(), + }, + { + name: "deadLetterSink nil", + ctx: context.Background(), + given: &DeliverySpec{}, + want: &DeliverySpec{}, + }, + { + name: "deadLetterSink.ref nil", + ctx: context.Background(), + given: &DeliverySpec{DeadLetterSink: &duckv1.Destination{}}, + want: &DeliverySpec{DeadLetterSink: &duckv1.Destination{}}, + }, + { + name: "deadLetterSink.ref.namespace empty string", + ctx: apis.WithinParent(context.Background(), metav1.ObjectMeta{Name: "b", Namespace: "custom"}), + given: &DeliverySpec{DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Kind: "Service", + Namespace: "", + Name: "svc", + APIVersion: "v1", + }, + }}, + want: &DeliverySpec{DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Kind: "Service", + Namespace: "custom", + Name: "svc", + APIVersion: "v1", + }, + }}, + }, + } + + for _, tc := range tt { + tc := tc + t.Run(tc.name, func(t *testing.T) { + tc.given.SetDefaults(tc.ctx) + if diff := cmp.Diff(tc.want, tc.given); diff != "" { + t.Error("(-want, +got)", diff) + } + }) + } +} diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index 468964bb64e..e101713e79a 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -21,9 +21,10 @@ import ( eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1" + "knative.dev/pkg/apis" + "knative.dev/eventing/pkg/apis/config" "knative.dev/eventing/pkg/apis/eventing" - "knative.dev/pkg/apis" ) func (b *Broker) SetDefaults(ctx context.Context) { @@ -53,4 +54,5 @@ func (bs *BrokerSpec) SetDefaults(ctx context.Context) { if bs.Config != nil { bs.Config.SetDefaults(ctx) } + bs.Delivery.SetDefaults(ctx) } diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index 7ecbd9a2ca3..4f341a68a44 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -21,14 +21,16 @@ import ( "testing" "k8s.io/utils/pointer" + eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1" "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + duckv1 "knative.dev/pkg/apis/duck/v1" + "knative.dev/eventing/pkg/apis/config" "knative.dev/eventing/pkg/apis/eventing" - duckv1 "knative.dev/pkg/apis/duck/v1" ) var ( @@ -404,6 +406,61 @@ func TestBrokerSetDefaults(t *testing.T) { }, }, }, + "missing deadLetterSink.ref.namespace, defaulted": { + initial: Broker{ + ObjectMeta: metav1.ObjectMeta{Name: "broker", Namespace: "custom"}, + Spec: BrokerSpec{ + Config: &duckv1.KReference{ + Kind: "ConfigMap", + Name: "natss-channel", + Namespace: "custom1", + APIVersion: "v1", + }, + Delivery: &eventingduckv1.DeliverySpec{ + DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Kind: "Service", + Name: "handle-error", + APIVersion: "serving.knative.dev/v1", + }, + }, + Retry: pointer.Int32Ptr(5), + BackoffPolicy: (*eventingduckv1.BackoffPolicyType)(pointer.StringPtr("linear")), + BackoffDelay: pointer.StringPtr("5s"), + }, + }, + }, + expected: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Name: "broker", + Namespace: "custom", + Annotations: map[string]string{ + eventing.BrokerClassKey: "MTChannelBasedBroker", + }, + }, + Spec: BrokerSpec{ + Config: &duckv1.KReference{ + Kind: "ConfigMap", + Namespace: "custom1", + Name: "natss-channel", + APIVersion: "v1", + }, + Delivery: &eventingduckv1.DeliverySpec{ + DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Kind: "Service", + Namespace: "custom", + Name: "handle-error", + APIVersion: "serving.knative.dev/v1", + }, + }, + Retry: pointer.Int32Ptr(5), + BackoffPolicy: (*eventingduckv1.BackoffPolicyType)(pointer.StringPtr("linear")), + BackoffDelay: pointer.StringPtr("5s"), + }, + }, + }, + }, } for n, tc := range testCases { t.Run(n, func(t *testing.T) { diff --git a/pkg/apis/eventing/v1/trigger_defaults.go b/pkg/apis/eventing/v1/trigger_defaults.go index 4c90811f355..1dde6bab9d3 100644 --- a/pkg/apis/eventing/v1/trigger_defaults.go +++ b/pkg/apis/eventing/v1/trigger_defaults.go @@ -39,6 +39,7 @@ func (ts *TriggerSpec) SetDefaults(ctx context.Context) { } // Default the Subscriber namespace ts.Subscriber.SetDefaults(ctx) + ts.Delivery.SetDefaults(ctx) } func setLabels(t *Trigger) { diff --git a/pkg/apis/eventing/v1/trigger_defaults_test.go b/pkg/apis/eventing/v1/trigger_defaults_test.go index 0d01bc10ea1..7ee2f51466a 100644 --- a/pkg/apis/eventing/v1/trigger_defaults_test.go +++ b/pkg/apis/eventing/v1/trigger_defaults_test.go @@ -25,6 +25,8 @@ import ( duckv1 "knative.dev/pkg/apis/duck/v1" "github.com/google/go-cmp/cmp" + + eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1" ) var ( @@ -80,6 +82,50 @@ func TestTriggerDefaults(t *testing.T) { }, }}, }, + "deadLetterSink, ns defaulted": { + initial: Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "custom", + }, + Spec: TriggerSpec{ + Delivery: &eventingduckv1.DeliverySpec{ + DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Name: "foo", + }, + }, + }, + Broker: otherBroker, + Subscriber: duckv1.Destination{ + Ref: &duckv1.KReference{ + Name: "foo", + Namespace: "custom1", + }, + }}}, + expected: Trigger{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "custom", + Labels: map[string]string{brokerLabel: otherBroker}, + }, + Spec: TriggerSpec{ + Broker: otherBroker, + Filter: emptyTriggerFilter, + Delivery: &eventingduckv1.DeliverySpec{ + DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Name: "foo", + Namespace: "custom", + }, + }, + }, + Subscriber: duckv1.Destination{ + Ref: &duckv1.KReference{ + Name: "foo", + Namespace: "custom1", + }, + }, + }}, + }, "nil broker and nil filter": { initial: Trigger{}, expected: defaultTrigger, diff --git a/pkg/apis/messaging/v1/channel_defaults.go b/pkg/apis/messaging/v1/channel_defaults.go index 82746cdd4fe..682e4d171f6 100644 --- a/pkg/apis/messaging/v1/channel_defaults.go +++ b/pkg/apis/messaging/v1/channel_defaults.go @@ -19,9 +19,10 @@ package v1 import ( "context" + "knative.dev/pkg/apis" + "knative.dev/eventing/pkg/apis/messaging" "knative.dev/eventing/pkg/apis/messaging/config" - "knative.dev/pkg/apis" ) func (c *Channel) SetDefaults(ctx context.Context) { @@ -48,6 +49,7 @@ func (cs *ChannelSpec) SetDefaults(ctx context.Context) { c.Spec, } } + cs.Delivery.SetDefaults(ctx) } // ChannelDefaulter sets the default Channel CRD and Arguments on Channels that do not diff --git a/pkg/apis/messaging/v1/in_memory_channel_defaults.go b/pkg/apis/messaging/v1/in_memory_channel_defaults.go index a081d6bb678..a3f3bedff54 100644 --- a/pkg/apis/messaging/v1/in_memory_channel_defaults.go +++ b/pkg/apis/messaging/v1/in_memory_channel_defaults.go @@ -19,6 +19,8 @@ package v1 import ( "context" + "knative.dev/pkg/apis" + "knative.dev/eventing/pkg/apis/messaging" ) @@ -35,9 +37,10 @@ func (imc *InMemoryChannel) SetDefaults(ctx context.Context) { imc.Annotations[messaging.SubscribableDuckVersionAnnotation] = "v1" } + ctx = apis.WithinParent(ctx, imc.ObjectMeta) imc.Spec.SetDefaults(ctx) } func (imcs *InMemoryChannelSpec) SetDefaults(ctx context.Context) { - // TODO: Nothing to default here... + imcs.Delivery.SetDefaults(ctx) } diff --git a/pkg/apis/messaging/v1/in_memory_channel_defaults_test.go b/pkg/apis/messaging/v1/in_memory_channel_defaults_test.go index 326c9b091df..e0c948469ad 100644 --- a/pkg/apis/messaging/v1/in_memory_channel_defaults_test.go +++ b/pkg/apis/messaging/v1/in_memory_channel_defaults_test.go @@ -21,6 +21,9 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + duckv1 "knative.dev/pkg/apis/duck/v1" + + eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1" "knative.dev/eventing/pkg/apis/messaging/config" "github.com/google/go-cmp/cmp" @@ -44,6 +47,41 @@ func TestInMemoryChannelSetDefaults(t *testing.T) { initial: InMemoryChannel{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"somethingelse": "yup"}}}, expected: InMemoryChannel{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1", "somethingelse": "yup"}}}, }, + "deadLetterSink.ref.namespace gets defaulted": { + initial: InMemoryChannel{ + ObjectMeta: metav1.ObjectMeta{ + Name: "imc", + Namespace: "custom", + Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1"}, + }, + Spec: InMemoryChannelSpec{eventingduckv1.ChannelableSpec{ + Delivery: &eventingduckv1.DeliverySpec{ + DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Name: "foo", + }, + }, + }, + }}, + }, + expected: InMemoryChannel{ + ObjectMeta: metav1.ObjectMeta{ + Name: "imc", + Namespace: "custom", + Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1"}, + }, + Spec: InMemoryChannelSpec{eventingduckv1.ChannelableSpec{ + Delivery: &eventingduckv1.DeliverySpec{ + DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Name: "foo", + Namespace: "custom", + }, + }, + }, + }}, + }, + }, } for n, tc := range testCases { t.Run(n, func(t *testing.T) { diff --git a/pkg/apis/messaging/v1/subscription_defaults.go b/pkg/apis/messaging/v1/subscription_defaults.go index 2beb73d00d1..0e894fc6379 100644 --- a/pkg/apis/messaging/v1/subscription_defaults.go +++ b/pkg/apis/messaging/v1/subscription_defaults.go @@ -18,10 +18,21 @@ package v1 import ( "context" + + "knative.dev/pkg/apis" ) func (s *Subscription) SetDefaults(ctx context.Context) { + if s == nil { + return + } + ctx = apis.WithinParent(ctx, s.ObjectMeta) s.Spec.SetDefaults(ctx) } -func (ss *SubscriptionSpec) SetDefaults(ctx context.Context) {} +func (ss *SubscriptionSpec) SetDefaults(ctx context.Context) { + if ss == nil { + return + } + ss.Delivery.SetDefaults(ctx) +} diff --git a/pkg/apis/messaging/v1/subscription_defaults_test.go b/pkg/apis/messaging/v1/subscription_defaults_test.go index 6e7d47d74ee..3dbe541fdfa 100644 --- a/pkg/apis/messaging/v1/subscription_defaults_test.go +++ b/pkg/apis/messaging/v1/subscription_defaults_test.go @@ -19,10 +19,100 @@ package v1 import ( "context" "testing" + + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + duckv1 "knative.dev/pkg/apis/duck/v1" + + eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1" ) -// No-op test because method does nothing. func TestSubscriptionDefaults(t *testing.T) { s := Subscription{} s.SetDefaults(context.TODO()) + + tt := []struct { + name string + given *Subscription + want *Subscription + }{ + { + name: "subscription empty", + }, + { + name: "subscription.spec nil", + given: &Subscription{}, + want: &Subscription{}, + }, + { + name: "subscription.spec empty", + given: &Subscription{ + Spec: SubscriptionSpec{}, + }, + want: &Subscription{ + Spec: SubscriptionSpec{}, + }, + }, + { + name: "subscription.spec.delivery empty", + given: &Subscription{ + Spec: SubscriptionSpec{ + Delivery: &eventingduckv1.DeliverySpec{}, + }, + }, + want: &Subscription{ + Spec: SubscriptionSpec{ + Delivery: &eventingduckv1.DeliverySpec{}, + }, + }, + }, + { + name: "subscription.spec.delivery.deadLetterSink.ref.namespace empty", + given: &Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "custom", + Name: "s", + }, + Spec: SubscriptionSpec{ + Delivery: &eventingduckv1.DeliverySpec{ + DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Kind: "Service", + Name: "svc", + APIVersion: "v1", + }, + }, + }, + }, + }, + want: &Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "custom", + Name: "s", + }, + Spec: SubscriptionSpec{ + Delivery: &eventingduckv1.DeliverySpec{ + DeadLetterSink: &duckv1.Destination{ + Ref: &duckv1.KReference{ + Kind: "Service", + Namespace: "custom", + Name: "svc", + APIVersion: "v1", + }, + }, + }, + }, + }, + }, + } + + for _, tc := range tt { + tc := tc + t.Run(tc.name, func(t *testing.T) { + tc.given.SetDefaults(context.Background()) + if diff := cmp.Diff(tc.want, tc.given); diff != "" { + t.Error("(-want, +got)", diff) + } + }) + } }