Default dead letter sink namespace to the object namespace

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
knative · Sep 21, 2021 · 1b448d3 · 1b448d3
1 parent f031ba2
commit 1b448d3
Show file tree

Hide file tree

Showing 9 changed files with 266 additions and 4 deletions.
diff --git a/pkg/apis/duck/v1/delivery_defaults.go 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {