diff --git a/apis/common/v1/status.go b/apis/common/v1/status.go new file mode 100644 index 0000000..6737539 --- /dev/null +++ b/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"` +} diff --git a/apis/common/v1/zz_generated.deepcopy.go b/apis/common/v1/zz_generated.deepcopy.go index 85dc2d3..74a7be0 100644 --- a/apis/common/v1/zz_generated.deepcopy.go +++ b/apis/common/v1/zz_generated.deepcopy.go @@ -541,6 +541,49 @@ func (in *Selector) DeepCopy() *Selector { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in Status) DeepCopyInto(out *Status) { + { + in := &in + *out = make(Status, len(*in)) + for key, val := range *in { + (*out)[key] = *val.DeepCopy() + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Status. +func (in Status) DeepCopy() Status { + if in == nil { + return nil + } + out := new(Status) + in.DeepCopyInto(out) + return *out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Statused) DeepCopyInto(out *Statused) { + *out = *in + if in.Status != nil { + in, out := &in.Status, &out.Status + *out = make(Status, len(*in)) + for key, val := range *in { + (*out)[key] = *val.DeepCopy() + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Statused. +func (in *Statused) DeepCopy() *Statused { + if in == nil { + return nil + } + out := new(Statused) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TargetSpec) DeepCopyInto(out *TargetSpec) { *out = *in diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 4d51888..93b7744 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -1207,6 +1207,11 @@ 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) } + // 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 3fc6df4..6f2f9f0 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -656,6 +656,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 e80db16..044b468 100644 --- a/pkg/resource/fake/mocks.go +++ b/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 } @@ -335,6 +345,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 1e383ea..1fd63d0 100644 --- a/pkg/resource/interfaces.go +++ b/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. diff --git a/pkg/resource/unstructured/composed/composed.go b/pkg/resource/unstructured/composed/composed.go index daf37ca..c2cd25a 100644 --- a/pkg/resource/unstructured/composed/composed.go +++ b/pkg/resource/unstructured/composed/composed.go @@ -91,6 +91,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{}