From c9e23202d9af3748fe650886569e135b13c7c6d4 Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Thu, 8 Aug 2024 15:39:26 +0200 Subject: [PATCH] Remove finalizer when trigger is deleted (#4046) * Remove finalizer when trigger is deleted Add a no-op `FinalizeKind` to configure knative/pkg base reconciler to remove the finalizer when a trigger is deleted. Signed-off-by: Pierangelo Di Pilato * Unit tests Signed-off-by: Pierangelo Di Pilato --------- Signed-off-by: Pierangelo Di Pilato --- .../pkg/reconciler/trigger/v2/triggerv2.go | 7 + .../reconciler/trigger/v2/triggerv2_test.go | 175 ++++++++++++++++++ 2 files changed, 182 insertions(+) diff --git a/control-plane/pkg/reconciler/trigger/v2/triggerv2.go b/control-plane/pkg/reconciler/trigger/v2/triggerv2.go index b2aee545e7..d933b17d64 100644 --- a/control-plane/pkg/reconciler/trigger/v2/triggerv2.go +++ b/control-plane/pkg/reconciler/trigger/v2/triggerv2.go @@ -134,6 +134,13 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, trigger *eventing.Trigge return nil } +func (r *Reconciler) FinalizeKind(context.Context, *eventing.Trigger) reconciler.Event { + // No-op, left here for backward compatibility. + // This configures the knative/pkg base reconciler to remove the finalizer, + // for more details, see https://github.com/knative-extensions/eventing-kafka-broker/issues/4034 + return nil +} + func (r *Reconciler) reconcileConsumerGroup(ctx context.Context, broker *eventing.Broker, trigger *eventing.Trigger) (*internalscg.ConsumerGroup, error) { var deliveryOrdering = sources.Unordered diff --git a/control-plane/pkg/reconciler/trigger/v2/triggerv2_test.go b/control-plane/pkg/reconciler/trigger/v2/triggerv2_test.go index 3883fac8d8..1f87fd63c1 100644 --- a/control-plane/pkg/reconciler/trigger/v2/triggerv2_test.go +++ b/control-plane/pkg/reconciler/trigger/v2/triggerv2_test.go @@ -20,7 +20,9 @@ import ( "context" "fmt" "testing" + "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" eventingduck "knative.dev/eventing/pkg/apis/duck/v1" @@ -78,6 +80,12 @@ var ( } exponential = eventingduck.BackoffPolicyExponential + + finalizerUpdatedEvent = Eventf( + corev1.EventTypeNormal, + "FinalizerUpdate", + fmt.Sprintf(`Updated %q finalizers`, TriggerName), + ) ) func TestReconcileKind(t *testing.T) { @@ -120,6 +128,12 @@ func TestReconcileKind(t *testing.T) { )), ), }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{ { Object: newTrigger( @@ -189,6 +203,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled normal - Trigger with ordered delivery", @@ -238,6 +258,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled normal - Trigger with unordered delivery", @@ -272,6 +298,12 @@ func TestReconcileKind(t *testing.T) { )), ), }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{ { Object: newTrigger( @@ -304,6 +336,7 @@ func TestReconcileKind(t *testing.T) { Key: testKey, WantErr: true, WantEvents: []string{ + finalizerUpdatedEvent, Eventf( corev1.EventTypeWarning, "InternalError", @@ -313,6 +346,9 @@ func TestReconcileKind(t *testing.T) { ), ), }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{ { Object: newTrigger( @@ -384,6 +420,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled normal - existing cg with autoscaling annotations", @@ -448,6 +490,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled normal - existing cg with update but not ready", @@ -506,6 +554,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled normal - with auth - SASL", @@ -590,6 +644,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled normal - existing cg without update", @@ -639,6 +699,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled normal - existing cg without update but not ready", @@ -687,6 +753,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled normal - existing cg but failed", @@ -736,6 +808,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled normal with dead letter sink uri", @@ -784,6 +862,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Broker not found", @@ -793,6 +877,7 @@ func TestReconcileKind(t *testing.T) { Key: testKey, WantErr: true, WantEvents: []string{ + finalizerUpdatedEvent, Eventf( corev1.EventTypeWarning, "InternalError", @@ -810,6 +895,9 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Broker not ready", @@ -829,6 +917,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Broker deleted", @@ -846,6 +940,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Don't reconcile trigger associated with a broker with a different broker class", @@ -866,6 +966,12 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, }, { Name: "Reconciled reusing old consumergroup naming", @@ -926,6 +1032,56 @@ func TestReconcileKind(t *testing.T) { ), }, }, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(), + }, + }, + { + Name: "Finalized normal", + Objects: []runtime.Object{ + NewBroker( + BrokerReady, + WithTopicStatusAnnotation(BrokerTopic()), + WithBootstrapServerStatusAnnotation(bootstrapServers), + ), + DataPlaneConfigMap(env.DataPlaneConfigMapNamespace, env.DataPlaneConfigConfigMapName, brokerreconciler.ConsumerConfigKey, + DataPlaneConfigInitialOffset(brokerreconciler.ConsumerConfigKey, sources.OffsetLatest), + ), + newTrigger(func(trigger *eventing.Trigger) { + trigger.DeletionTimestamp = &metav1.Time{Time: time.Time{}.AddDate(1999, 1, 3)} + trigger.Finalizers = []string{FinalizerName} + }), + NewConsumerGroup( + WithConsumerGroupName(consumerGroupId), + WithConsumerGroupNamespace(triggerNamespace), + WithConsumerGroupOwnerRef(kmeta.NewControllerRef(newTrigger())), + WithConsumerGroupMetaLabels(OwnerAsTriggerLabel), + WithConsumerGroupLabels(ConsumerTriggerLabel), + WithConsumerGroupAnnotations(ConsumerGroupAnnotations), + ConsumerGroupConsumerSpec(NewConsumerSpec( + ConsumerTopics(BrokerTopics[0]), + ConsumerConfigs( + ConsumerGroupIdConfig(consumerGroupId), + ConsumerBootstrapServersConfig(bootstrapServers), + ), + ConsumerDelivery(NewConsumerSpecDelivery(sources.Unordered, ConsumerInitialOffset(sources.OffsetLatest))), + ConsumerFilters(NewConsumerSpecFilters()), + ConsumerReply(ConsumerTopicReply()), + )), + ConsumerGroupReady, + ConsumerGroupReplicas(1), + ), + }, + Key: testKey, + WantEvents: []string{ + finalizerUpdatedEvent, + }, + WantPatches: []clientgotesting.PatchActionImpl{ + removeFinalizers(), + }, }, } @@ -951,6 +1107,7 @@ func TestReconcileKind(t *testing.T) { listers.GetTriggerLister(), controller.GetEventRecorder(ctx), reconciler, + controller.Options{FinalizerName: FinalizerName}, ) })) } @@ -997,3 +1154,21 @@ func withTriggerStatusGroupIdAnnotation(groupId string) func(*eventing.Trigger) t.Status.Annotations[kafka.GroupIdAnnotation] = groupId } } + +func patchFinalizers() clientgotesting.PatchActionImpl { + action := clientgotesting.PatchActionImpl{} + action.Name = TriggerName + action.Namespace = TriggerNamespace + patch := `{"metadata":{"finalizers":["` + FinalizerName + `"],"resourceVersion":""}}` + action.Patch = []byte(patch) + return action +} + +func removeFinalizers() clientgotesting.PatchActionImpl { + action := clientgotesting.PatchActionImpl{} + action.Name = TriggerName + action.Namespace = TriggerNamespace + patch := `{"metadata":{"finalizers":[],"resourceVersion":""}}` + action.Patch = []byte(patch) + return action +}