Fix bug causing status to be cleared after adding Finalizer

apis/common/v1/status.go

@@ -0,0 +1,29 @@
+/*
+Copyright 2019 The Crossplane Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1
+
+import "k8s.io/apimachinery/pkg/runtime"
+
+// 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]runtime.RawExtension
+
+// A Statused resource is one that has a status.
+type Statused struct {
+	Status Status `json:"status"`
+}

pkg/reconciler/managed/reconciler.go

@@ -1164,6 +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 before the AddFinalizer call.
+	// 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

@@ -49,6 +49,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 +342,7 @@ type Managed struct {
 	Manageable
 	Orphanable
 	xpv1.ConditionedStatus
+	xpv1.Statused
 }
 
 // GetObjectKind returns schema.ObjectKind.

pkg/resource/interfaces.go

@@ -28,13 +28,20 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/reference"
 )
 
-// A Conditioned may have conditions set or retrieved. Conditions are typically
+// A Conditioned may have conditions set or retrieved. Conditions typically
 // indicate the status of both a resource and its reconciliation process.
 type Conditioned interface {
 	SetConditions(c ...xpv1.Condition)
 	GetCondition(ct xpv1.ConditionType) xpv1.Condition
 }
 
+// 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)
+	GetStatus() xpv1.Status
+}
+
 // A ClaimReferencer may reference a resource claim.
 type ClaimReferencer interface {
 	SetClaimReference(r *reference.Claim)
@@ -200,6 +207,7 @@ type Managed interface { //nolint:interfacebloat // This interface has to be big
 	Orphanable
 
 	Conditioned
+	Statused
 }
 
 // A ManagedList is a list of managed resources.

pkg/resource/unstructured/composed/composed.go

@@ -89,6 +89,20 @@ func (cr *Unstructured) SetConditions(c ...xpv1.Condition) {
 	_ = fieldpath.Pave(cr.Object).SetValue("status.conditions", conditioned.Conditions)
 }
 
+// GetStatus of this Composed resource.
+func (cr *Unstructured) GetStatus() xpv1.Status {
+	status := xpv1.Statused{}
+	if err := fieldpath.Pave(cr.Object).GetValueInto("status", &status); err != nil {
+		return xpv1.Status{}
+	}
+	return status.Status
+}
+
+// SetStatus 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{}