Merge pull request #855 from nokia/honor_finalizers

Delay external delete call when other finalizers exist

Author: bobh66

pkg/reconciler/managed/reconciler.go

@@ -1163,6 +1163,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 	if meta.WasDeleted(managed) {
 		log = log.WithValues("deletion-timestamp", managed.GetDeletionTimestamp())
 
+		if len(managed.GetFinalizers()) > 1 {
+			// There are other controllers monitoring this resource so preserve the external instance
+			// until all other finalizers have been removed
+			log.Debug("Delay external deletion until all finalizers have been removed")
+			return reconcile.Result{Requeue: true}, nil
+		}
+
 		if observation.ResourceExists && policy.ShouldDelete() {
 			deletion, err := external.Delete(externalCtx, managed)
 			if err != nil {

pkg/reconciler/managed/reconciler_legacy_test.go

@@ -202,6 +202,41 @@ func TestReconciler(t *testing.T) {
 			},
 			want: want{result: reconcile.Result{Requeue: true}},
 		},
+		"ExtraFinalizersDelayDelete": {
+			reason: "The existence of multiple finalizers should trigger a requeue after a short wait.",
+			args: args{
+				m: &fake.Manager{
+					Client: &test.MockClient{
+						MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
+							mg := asLegacyManaged(obj, 42)
+							mg.SetDeletionTimestamp(&now)
+							mg.SetDeletionPolicy(xpv1.DeletionDelete)
+							mg.SetFinalizers([]string{FinalizerName, "finalizer2"})
+							return nil
+						}),
+						MockUpdate: test.NewMockUpdateFn(nil),
+					},
+					Scheme: fake.SchemeWith(&fake.LegacyManaged{}),
+				},
+				mg: resource.ManagedKind(fake.GVK(&fake.LegacyManaged{})),
+				o: []ReconcilerOption{
+					WithInitializers(),
+					WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
+					WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
+						c := &ExternalClientFns{
+							ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
+								return ExternalObservation{ResourceExists: true}, nil
+							},
+							DisconnectFn: func(_ context.Context) error {
+								return nil
+							},
+						}
+						return c, nil
+					})),
+				},
+			},
+			want: want{result: reconcile.Result{Requeue: true}},
+		},
 		"ExternalCreatePending": {
 			reason: "We should return early if the managed resource appears to be pending creation. We might have leaked a resource and don't want to create another.",
 			args: args{

pkg/reconciler/managed/reconciler_modern_test.go

@@ -207,6 +207,40 @@ func TestModernReconciler(t *testing.T) {
 			},
 			want: want{result: reconcile.Result{Requeue: true}},
 		},
+		"ExtraFinalizersDelayDelete": {
+			reason: "The existence of multiple finalizers should trigger a requeue after a short wait.",
+			args: args{
+				m: &fake.Manager{
+					Client: &test.MockClient{
+						MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
+							mg := asModernManaged(obj, 42)
+							mg.SetDeletionTimestamp(&now)
+							mg.SetFinalizers([]string{FinalizerName, "finalizer2"})
+							return nil
+						}),
+						MockUpdate: test.NewMockUpdateFn(nil),
+					},
+					Scheme: fake.SchemeWith(&fake.ModernManaged{}),
+				},
+				mg: resource.ManagedKind(fake.GVK(&fake.ModernManaged{})),
+				o: []ReconcilerOption{
+					WithInitializers(),
+					WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
+					WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
+						c := &ExternalClientFns{
+							ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
+								return ExternalObservation{ResourceExists: true}, nil
+							},
+							DisconnectFn: func(_ context.Context) error {
+								return nil
+							},
+						}
+						return c, nil
+					})),
+				},
+			},
+			want: want{result: reconcile.Result{Requeue: true}},
+		},
 		"ExternalCreatePending": {
 			reason: "We should return early if the managed resource appears to be pending creation. We might have leaked a resource and don't want to create another.",
 			args: args{