Merge pull request #75 from negz/optimistic

Don't resolve references when being deleted

pkg/resource/managed_reconciler.go

@@ -455,20 +455,28 @@ func (r *ManagedReconciler) Reconcile(req reconcile.Request) (reconcile.Result,
 	// We resolve any references before observing our external resource because
 	// in some rare examples we need a spec field to make the observe call, and
 	// that spec field could be set by a reference.
-	if err := r.managed.ResolveReferences(ctx, managed); err != nil {
-		condition := v1alpha1.ReconcileError(err)
-		if IsReferencesAccessError(err) {
-			condition = v1alpha1.ReferenceResolutionBlocked(err)
-		}
+	//
+	// We do not resolve references when being deleted because it is likely that
+	// the resources we reference are also being deleted, and would thus block
+	// resolution due to being unready or non-existent. It is unlikely (but not
+	// impossible) that we need to resolve a reference in order to process a
+	// delete, and that reference is stale at delete time.
+	if !meta.WasDeleted(managed) {
+		if err := r.managed.ResolveReferences(ctx, managed); err != nil {
+			condition := v1alpha1.ReconcileError(err)
+			if IsReferencesAccessError(err) {
+				condition = v1alpha1.ReferenceResolutionBlocked(err)
+			}
 
-		// If any of our referenced resources are not yet ready (or if we
-		// encountered an error resolving them) we want to try again after a
-		// short wait. If this is the first time we encounter this situation
-		// we'll be requeued implicitly due to the status update.
-		managed.SetConditions(condition)
-		return reconcile.Result{RequeueAfter: r.shortWait}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
+			// If any of our referenced resources are not yet ready (or if we
+			// encountered an error resolving them) we want to try again after a
+			// short wait. If this is the first time we encounter this situation
+			// we'll be requeued implicitly due to the status update.
+			managed.SetConditions(condition)
+			return reconcile.Result{RequeueAfter: r.shortWait}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
+		}
+		managed.SetConditions(v1alpha1.ReferenceResolutionSuccess())
 	}
-	managed.SetConditions(v1alpha1.ReferenceResolutionSuccess())
 
 	observation, err := external.Observe(ctx, managed)
 	if err != nil {

pkg/resource/managed_reconciler_test.go

@@ -243,7 +243,6 @@ func TestManagedReconciler(t *testing.T) {
 							want := &MockManaged{}
 							want.SetDeletionTimestamp(&now)
 							want.SetReclaimPolicy(v1alpha1.ReclaimDelete)
-							want.SetConditions(v1alpha1.ReferenceResolutionSuccess())
 							want.SetConditions(v1alpha1.ReconcileError(errBoom))
 							if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
 								reason := "An error deleting an external resource should be reported as a conditioned status."
@@ -288,7 +287,6 @@ func TestManagedReconciler(t *testing.T) {
 							want := &MockManaged{}
 							want.SetDeletionTimestamp(&now)
 							want.SetReclaimPolicy(v1alpha1.ReclaimDelete)
-							want.SetConditions(v1alpha1.ReferenceResolutionSuccess())
 							want.SetConditions(v1alpha1.ReconcileSuccess())
 							if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
 								reason := "A deleted external resource should be reported as a conditioned status."
@@ -331,7 +329,6 @@ func TestManagedReconciler(t *testing.T) {
 						MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj runtime.Object, _ ...client.UpdateOption) error {
 							want := &MockManaged{}
 							want.SetDeletionTimestamp(&now)
-							want.SetConditions(v1alpha1.ReferenceResolutionSuccess())
 							want.SetConditions(v1alpha1.ReconcileError(errBoom))
 							if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
 								reason := "Errors unpublishing connection details should be reported as a conditioned status."
@@ -374,7 +371,6 @@ func TestManagedReconciler(t *testing.T) {
 						MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj runtime.Object, _ ...client.UpdateOption) error {
 							want := &MockManaged{}
 							want.SetDeletionTimestamp(&now)
-							want.SetConditions(v1alpha1.ReferenceResolutionSuccess())
 							want.SetConditions(v1alpha1.ReconcileError(errBoom))
 							if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
 								reason := "Errors removing the managed resource finalizer should be reported as a conditioned status."