Add test to validate status is not lost when adding Finalizer

Signed-off-by: Christian Artin <cartin@genetec.com>

apis/common/v1/status.go

@@ -16,19 +16,11 @@ limitations under the License.
 
 package v1
 
-import "maps"
-
 // See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties
 
 // A Status reflects the observed status of a resource.
 type Status map[string]interface{}
 
-// GetStatus returns the status.
+type Statused struct {
-func (s *Status) GetStatus() Status {
+	Status Status `json:"status"`
-	return *s
-}
-
-// SetConditions sets the supplied status on the resource.
-func (s *Status) SetStatus(c Status) {
-	*s = maps.Clone(c)
 }

pkg/reconciler/managed/reconciler.go

@@ -1153,9 +1153,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 		return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
 	}
 
-	// Save the status before calling AddFinalizer since it may be overwritten
-	// by the kube client if the resource is updated.
-	status := managed.GetStatus()
 	if err := r.managed.AddFinalizer(ctx, managed); err != nil {
 		// If this is the first time we encounter this issue we'll be requeued
 		// implicitly when we update our status with the new error condition. If
@@ -1167,8 +1164,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 		managed.SetConditions(xpv1.ReconcileError(err))
 		return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
 	}
-	// Set the status back to its original value
+	// Set the status back to its original value before the AddFinalizer call.
-	managed.SetStatus(status)
+	// The AddFinalizer call may overwrite the managed object with what is
+	// currently in the kube API which would make us lose information that may
+	// have been populated by the Observe call.
+	managed.SetStatus(managedPreOp.GetStatus())
 
 	if !observation.ResourceExists && policy.ShouldCreate() {
 		// We write this annotation for two reasons. Firstly, it helps

pkg/reconciler/managed/reconciler_test.go

@@ -653,6 +653,52 @@ func TestReconciler(t *testing.T) {
 			},
 			want: want{result: reconcile.Result{Requeue: true}},
 		},
+		"AddFinalizerShouldNotClearStatus": {
+			reason: "Adding a finalizer should not clear the resource's status object.",
+			args: args{
+				m: &fake.Manager{
+					Client: &test.MockClient{
+						MockGet:    test.NewMockGetFn(nil),
+						MockUpdate: test.NewMockUpdateFn(nil),
+						MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
+							want := &fake.Managed{}
+							want.SetConditions(xpv1.ReconcileSuccess(), xpv1.Available())
+							if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
+								reason := "Adding a finalizer should not clear the resource's status object."
+								t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff)
+							}
+							return nil
+						}),
+					},
+					Scheme: fake.SchemeWith(&fake.Managed{}),
+				},
+				mg: resource.ManagedKind(fake.GVK(&fake.Managed{})),
+				o: []ReconcilerOption{
+					WithInitializers(),
+					WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
+					WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
+						c := &ExternalClientFns{
+							ObserveFn: func(_ context.Context, obj resource.Managed) (ExternalObservation, error) {
+								obj.SetConditions(xpv1.Available())
+								return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
+							},
+							DisconnectFn: func(_ context.Context) error {
+								return nil
+							},
+						}
+						return c, nil
+					})),
+					WithConnectionPublishers(),
+					WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, obj resource.Object) error {
+						// Imitate the behavior that the kube client would have when
+						// adding a finalizer to an object that does not have a status yet
+						obj.(*fake.Managed).Status = nil
+						return nil
+					}}),
+				},
+			},
+			want: want{result: reconcile.Result{RequeueAfter: defaultPollInterval}},
+		},
 		"AddFinalizerError": {
 			reason: "Errors adding a finalizer should trigger a requeue after a short wait.",
 			args: args{

pkg/resource/fake/mocks.go

@@ -21,6 +21,7 @@ package fake
 
 import (
 	"encoding/json"
+
 	"reflect"
 
 	"github.com/go-logr/logr"
@@ -49,6 +50,16 @@ func (m *Conditioned) GetCondition(ct xpv1.ConditionType) xpv1.Condition {
 	return xpv1.Condition{Type: ct, Status: corev1.ConditionUnknown}
 }
 
+// GetStatus gets the status sub object.
+func (m *Managed) GetStatus() xpv1.Status {
+	return m.Status
+}
+
+// SetStatus get the Condition with the given ConditionType.
+func (m *Managed) SetStatus(s xpv1.Status) {
+	m.Status = s
+}
+
 // ClaimReferencer is a mock that implements ClaimReferencer interface.
 type ClaimReferencer struct{ Ref *reference.Claim }
 
@@ -332,6 +343,7 @@ type Managed struct {
 	Manageable
 	Orphanable
 	xpv1.ConditionedStatus
+	xpv1.Statused
 }
 
 // GetObjectKind returns schema.ObjectKind.

pkg/resource/interfaces.go

@@ -38,7 +38,7 @@ type Conditioned interface {
 // A Statused may have status set or retrieved. Status is typically
 // a struct containing various runtime status properties.
 type Statused interface {
-	SetStatus(c ...xpv1.Status)
+	SetStatus(c xpv1.Status)
 	GetStatus() xpv1.Status
 }
 

pkg/resource/unstructured/composed/composed.go

@@ -89,6 +89,21 @@ func (cr *Unstructured) SetConditions(c ...xpv1.Condition) {
 	_ = fieldpath.Pave(cr.Object).SetValue("status.conditions", conditioned.Conditions)
 }
 
+// GetCondition of this Composed resource.
+func (cr *Unstructured) GetStatus() xpv1.Status {
+	status := xpv1.Statused{}
+	// The path is directly `status` because conditions are inline.
+	if err := fieldpath.Pave(cr.Object).GetValueInto("status", &status); err != nil {
+		return xpv1.Status{}
+	}
+	return status.Status
+}
+
+// SetConditions of this Composed resource.
+func (cr *Unstructured) SetStatus(s xpv1.Status) {
+	_ = fieldpath.Pave(cr.Object).SetValue("status", s)
+}
+
 // GetWriteConnectionSecretToReference of this Composed resource.
 func (cr *Unstructured) GetWriteConnectionSecretToReference() *xpv1.SecretReference {
 	out := &xpv1.SecretReference{}