diff --git a/apis/common/v1/status.go b/apis/common/v1/status.go index eb99314..bade4a7 100644 --- a/apis/common/v1/status.go +++ b/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. -func (s *Status) GetStatus() Status { - return *s -} - -// SetConditions sets the supplied status on the resource. -func (s *Status) SetStatus(c Status) { - *s = maps.Clone(c) +type Statused struct { + Status Status `json:"status"` } diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index e8aaedb..4d10b4a 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/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 - managed.SetStatus(status) + // 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 diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index 00e6f08..ce28f77 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/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{ diff --git a/pkg/resource/fake/mocks.go b/pkg/resource/fake/mocks.go index ae8f80e..e6f0186 100644 --- a/pkg/resource/fake/mocks.go +++ b/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. diff --git a/pkg/resource/interfaces.go b/pkg/resource/interfaces.go index ff017fe..fb7c97a 100644 --- a/pkg/resource/interfaces.go +++ b/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 } diff --git a/pkg/resource/unstructured/composed/composed.go b/pkg/resource/unstructured/composed/composed.go index 4a29048..ddc7b50 100644 --- a/pkg/resource/unstructured/composed/composed.go +++ b/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{}