From 30afbb6d19c6cae5d7c8c41aa89abea40d0f3732 Mon Sep 17 00:00:00 2001
From: Vijay Tripathi <vijay.trip40@gmail.com>
Date: Wed, 12 Jan 2022 05:29:31 -0800
Subject: [PATCH] attempt to patchStatus regardless of finalizer (#64)

Description of changes:

* ACK reconciler marks a resource as `Managed` before performing the `CREATE` call
* ResourceReference in ACK reconciler is resolved before `READ_ONE` call which precedes `CREATE` call
* When ResourceReference resolution fails, ACK reconciler needs to set `Status.Condition` indicating reference resolution failed
* Hence the status patching needs to be done for unmanaged resources too
* `NotFound` error while status patching of deleted resource is caught and not logged in controller logs

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
---
 pkg/runtime/reconciler.go | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go
index cc77b1f..169aff6 100644
--- a/pkg/runtime/reconciler.go
+++ b/pkg/runtime/reconciler.go
@@ -507,12 +507,15 @@ func (r *resourceReconciler) patchResourceStatus(
 		latest.RuntimeObject(),
 		client.MergeFrom(desired.DeepCopy().RuntimeObject()),
 	)
-	rlog.Exit("kc.Patch (status)", err)
-	if err != nil {
-		return err
+	if err == nil {
+		rlog.Debug("patched resource status")
+	} else if apierrors.IsNotFound(err) {
+		// reset the NotFound error so it is not printed in controller logs
+		// providing false positive error
+		err = nil
 	}
-	rlog.Debug("patched resource status", "latest", latest)
-	return nil
+	rlog.Exit("kc.Patch (status)", err)
+	return err
 }
 
 // deleteResource ensures that the supplied AWSResource's backing API resource
@@ -714,15 +717,17 @@ func (r *resourceReconciler) HandleReconcileError(
 	latest acktypes.AWSResource,
 	err error,
 ) (ctrlrt.Result, error) {
-	if ackcompare.IsNotNil(latest) && r.rd.IsManaged(latest) {
+	if ackcompare.IsNotNil(latest) {
 		// The reconciliation loop may have returned an error, but if latest is
 		// not nil, there may be some changes available in the CR's Status
 		// struct (example: Conditions), and we want to make sure we save those
 		// changes before proceeding
 		//
-		// Attempt to patch the status only if the resource is managed.
-		// Otherwise, the underlying K8s resource gets deleted and
-		// patchStatus call will return NotFound error
+		// PatchStatus even when resource is unmanaged. This helps in setting
+		// conditions when resolving resource-reference fails, which happens
+		// before resource is marked as managed.
+		// It is okay to patch status when resource is not present due to deletion
+		// because a NotFound error is thrown which will be ignored.
 		//
 		// TODO(jaypipes): We ignore error handling here but I don't know if
 		// there is a more robust way to handle failures in the patch operation