Skip to content

Commit

Permalink
Remove finalizer when trigger is deleted (#4046)
Browse files Browse the repository at this point in the history 
* 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 <pierdipi@redhat.com>

* Unit tests

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

---------

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
  • Loading branch information
@pierDipi
pierDipi authored Aug 8, 2024
1 parent 241bdb5 commit c9e2320
Show file tree
Hide file tree
Showing 2 changed files with 182 additions and 0 deletions.
7 changes: 7 additions & 0 deletions control-plane/pkg/reconciler/trigger/v2/triggerv2.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
175 changes: 175 additions & 0 deletions control-plane/pkg/reconciler/trigger/v2/triggerv2_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -78,6 +80,12 @@ var (
}

exponential = eventingduck.BackoffPolicyExponential

finalizerUpdatedEvent = Eventf(
corev1.EventTypeNormal,
"FinalizerUpdate",
fmt.Sprintf(`Updated %q finalizers`, TriggerName),
)
)

func TestReconcileKind(t *testing.T) {
Expand Down Expand Up @@ -120,6 +128,12 @@ func TestReconcileKind(t *testing.T) {
)),
),
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{
{
Object: newTrigger(
Expand Down Expand Up @@ -189,6 +203,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Reconciled normal - Trigger with ordered delivery",
Expand Down Expand Up @@ -238,6 +258,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Reconciled normal - Trigger with unordered delivery",
Expand Down Expand Up @@ -272,6 +298,12 @@ func TestReconcileKind(t *testing.T) {
)),
),
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{
{
Object: newTrigger(
Expand Down Expand Up @@ -304,6 +336,7 @@ func TestReconcileKind(t *testing.T) {
Key: testKey,
WantErr: true,
WantEvents: []string{
finalizerUpdatedEvent,
Eventf(
corev1.EventTypeWarning,
"InternalError",
Expand All @@ -313,6 +346,9 @@ func TestReconcileKind(t *testing.T) {
),
),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{
{
Object: newTrigger(
Expand Down Expand Up @@ -384,6 +420,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Reconciled normal - existing cg with autoscaling annotations",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -506,6 +554,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Reconciled normal - with auth - SASL",
Expand Down Expand Up @@ -590,6 +644,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Reconciled normal - existing cg without update",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -687,6 +753,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Reconciled normal - existing cg but failed",
Expand Down Expand Up @@ -736,6 +808,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Reconciled normal with dead letter sink uri",
Expand Down Expand Up @@ -784,6 +862,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Broker not found",
Expand All @@ -793,6 +877,7 @@ func TestReconcileKind(t *testing.T) {
Key: testKey,
WantErr: true,
WantEvents: []string{
finalizerUpdatedEvent,
Eventf(
corev1.EventTypeWarning,
"InternalError",
Expand All @@ -810,6 +895,9 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Broker not ready",
Expand All @@ -829,6 +917,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Broker deleted",
Expand All @@ -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",
Expand All @@ -866,6 +966,12 @@ func TestReconcileKind(t *testing.T) {
),
},
},
WantEvents: []string{
finalizerUpdatedEvent,
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(),
},
},
{
Name: "Reconciled reusing old consumergroup naming",
Expand Down Expand Up @@ -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(),
},
},
}

Expand All @@ -951,6 +1107,7 @@ func TestReconcileKind(t *testing.T) {
listers.GetTriggerLister(),
controller.GetEventRecorder(ctx),
reconciler,
controller.Options{FinalizerName: FinalizerName},
)
}))
}
Expand Down Expand Up @@ -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
}

0 comments on commit c9e2320

Please sign in to comment.