Merge pull request #64 from crossplane-contrib/backport-62-to-release…

…-0.4

[Backport release-0.4] Fix stuck reference finalizers

internal/controller/object/object.go

@@ -533,18 +533,8 @@ func (f *objFinalizer) RemoveFinalizer(ctx context.Context, res resource.Object)
 		return errors.New(errNotKubernetesObject)
 	}
 
-	if !meta.FinalizerExists(obj, objFinalizerName) {
-		return nil
-	}
-	meta.RemoveFinalizer(obj, objFinalizerName)
-
-	err := f.client.Update(ctx, obj)
-	if err != nil {
-		return errors.Wrap(err, errRemoveFinalizer)
-	}
-
 	// Remove finalizer from referenced resources if exists
-	err = f.handleRefFinalizer(ctx, obj, func(
+	err := f.handleRefFinalizer(ctx, obj, func(
 		ctx context.Context, res *unstructured.Unstructured, finalizer string) error {
 		if meta.FinalizerExists(res, finalizer) {
 			meta.RemoveFinalizer(res, finalizer)
@@ -554,5 +544,15 @@ func (f *objFinalizer) RemoveFinalizer(ctx context.Context, res resource.Object)
 		}
 		return nil
 	})
+	if err != nil {
+		return errors.Wrap(err, errRemoveFinalizer)
+	}
+
+	if !meta.FinalizerExists(obj, objFinalizerName) {
+		return nil
+	}
+	meta.RemoveFinalizer(obj, objFinalizerName)
+
+	err = f.client.Update(ctx, obj)
 	return errors.Wrap(err, errRemoveFinalizer)
 }

internal/controller/object/object_test.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
@@ -1274,7 +1275,8 @@ func Test_objFinalizer_RemoveFinalizer(t *testing.T) {
 		mg     resource.Managed
 	}
 	type want struct {
-		err error
+		err        error
+		finalizers []string
 	}
 	cases := map[string]struct {
 		args
@@ -1285,7 +1287,8 @@ func Test_objFinalizer_RemoveFinalizer(t *testing.T) {
 				mg: notKubernetesObject{},
 			},
 			want: want{
-				err: errors.New(errNotKubernetesObject),
+				err:        errors.New(errNotKubernetesObject),
+				finalizers: []string{},
 			},
 		},
 		"FailedToRemoveObjectFinalizer": {
@@ -1300,15 +1303,17 @@ func Test_objFinalizer_RemoveFinalizer(t *testing.T) {
 				},
 			},
 			want: want{
-				err: errors.Wrap(errBoom, errRemoveFinalizer),
+				err:        errors.Wrap(errBoom, errRemoveFinalizer),
+				finalizers: []string{},
 			},
 		},
 		"NoObjectFinalizerExists": {
 			args: args{
 				mg: kubernetesObject(),
 			},
 			want: want{
-				err: nil,
+				err:        nil,
+				finalizers: nil,
 			},
 		},
 		"NoReferenceFinalizerExists": {
@@ -1328,7 +1333,8 @@ func Test_objFinalizer_RemoveFinalizer(t *testing.T) {
 				},
 			},
 			want: want{
-				err: nil,
+				err:        nil,
+				finalizers: []string{},
 			},
 		},
 		"FailedToRemoveReferenceFinalizer": {
@@ -1358,6 +1364,7 @@ func Test_objFinalizer_RemoveFinalizer(t *testing.T) {
 				err: errors.Wrap(
 					errors.Wrap(errBoom,
 						errRemoveReferenceFinalizer), errRemoveFinalizer),
+				finalizers: []string{objFinalizerName},
 			},
 		},
 		"Success": {
@@ -1378,7 +1385,8 @@ func Test_objFinalizer_RemoveFinalizer(t *testing.T) {
 				},
 			},
 			want: want{
-				err: nil,
+				err:        nil,
+				finalizers: []string{},
 			},
 		},
 	}
@@ -1387,9 +1395,17 @@ func Test_objFinalizer_RemoveFinalizer(t *testing.T) {
 			f := &objFinalizer{
 				client: tc.args.client,
 			}
+
 			gotErr := f.RemoveFinalizer(context.Background(), tc.args.mg)
 			if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" {
-				t.Fatalf("f.RemoveFinalizer(...): -want error, +got error: %s", diff)
+				t.Errorf("f.RemoveFinalizer(...): -want error, +got error: %s", diff)
+			}
+
+			if _, ok := tc.args.mg.(*v1alpha1.Object); ok {
+				sort := cmpopts.SortSlices(func(a, b string) bool { return a < b })
+				if diff := cmp.Diff(tc.want.finalizers, tc.args.mg.GetFinalizers(), sort); diff != "" {
+					t.Errorf("managed resource finalizers: -want, +got: %s", diff)
+				}
 			}
 		})
 	}