From f1f31fee804f8233aa28b4c4b18fdd028b9020c1 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Mon, 16 Aug 2021 05:01:33 +0000 Subject: [PATCH 1/2] Account for two different kinds of consistency issues This commit is intended to address two issues that we diagnosed while investigating https://github.com/crossplane/provider-aws/issues/802. The first issue is that controller-runtime does not guarantee reads from cache will return the freshest version of a resource. It's possible we could create an external resource in one reconcile, then shortly after trigger another in which it appears that the managed resource was never created because we didn't record its external-name. This only affects the subset of managed resources with non-deterministic external-names that are assigned during creation. The second issue is that some external APIs are eventually consistent. A newly created external resource may take some time before our ExternalClient's observe call can confirm it exists. AWS EC2 is an example of one such API. This commit attempts to address the first issue by making an Update to a managed resource immediately before Create it called. This Update call will be rejected by the API server if the managed resource we read from cache was not the latest version. It attempts to address the second issue by allowing managed resource controller authors to configure an optional grace period that begins when an external resource is successfully created. During this grace period we'll requeue and keep waiting if Observe determines that the external resource doesn't exist, rather than (re)creating it. Signed-off-by: Nic Cope --- pkg/meta/meta.go | 95 ++++++++- pkg/meta/meta_test.go | 220 +++++++++++++++++++++ pkg/reconciler/managed/api.go | 41 +++- pkg/reconciler/managed/api_test.go | 62 +++++- pkg/reconciler/managed/reconciler.go | 177 +++++++++++++---- pkg/reconciler/managed/reconciler_test.go | 224 ++++++++++++---------- 6 files changed, 673 insertions(+), 146 deletions(-) diff --git a/pkg/meta/meta.go b/pkg/meta/meta.go index 71493990d..0f5ce6359 100644 --- a/pkg/meta/meta.go +++ b/pkg/meta/meta.go @@ -21,6 +21,7 @@ import ( "fmt" "hash/fnv" "strings" + "time" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -31,9 +32,33 @@ import ( xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" ) -// AnnotationKeyExternalName is the key in the annotations map of a resource for -// the name of the resource as it appears on provider's systems. -const AnnotationKeyExternalName = "crossplane.io/external-name" +const ( + // AnnotationKeyExternalName is the key in the annotations map of a + // resource for the name of the resource as it appears on provider's + // systems. + AnnotationKeyExternalName = "crossplane.io/external-name" + + // AnnotationKeyExternalCreatePending is the key in the annotations map + // of a resource that indicates the last time creation of the external + // resource was pending. Its value must be an RFC3999 timestamp. This + // annotation is removed once we record the result of external resource + // creation. + AnnotationKeyExternalCreatePending = "crossplane.io/external-create-pending" + + // AnnotationKeyExternalCreateSucceeded is the key in the annotations + // map of a resource that represents the last time the external resource + // was created successfully. Its value must be an RFC3339 timestamp, + // which can be used to determine how long ago a resource was created. + // This is useful for eventually consistent APIs that may take some time + // before the API called by Observe will report that a recently created + // external resource exists. + AnnotationKeyExternalCreateSucceeded = "crossplane.io/external-create-succeeded" + + // AnnotationKeyExternalCreateFailed is the key in the annotations map + // of a resource that indicates the last time creation of the external + // resource failed. Its value must be an RFC3999 timestamp. + AnnotationKeyExternalCreateFailed = "crossplane.io/external-create-failed" +) // Supported resources with all of these annotations will be fully or partially // propagated to the named resource of the same kind, assuming it exists and @@ -245,6 +270,70 @@ func SetExternalName(o metav1.Object, name string) { AddAnnotations(o, map[string]string{AnnotationKeyExternalName: name}) } +// GetExternalCreatePending returns the time at which the external resource +// was most recently pending creation. +func GetExternalCreatePending(o metav1.Object) *metav1.Time { + a := o.GetAnnotations()[AnnotationKeyExternalCreatePending] + t, err := time.Parse(time.RFC3339, a) + if err != nil { + return nil + } + return &metav1.Time{Time: t} +} + +// SetExternalCreatePending sets the time at which the external resource was +// most recently pending creation to the supplied time. +func SetExternalCreatePending(o metav1.Object, t metav1.Time) { + AddAnnotations(o, map[string]string{AnnotationKeyExternalCreatePending: t.Format(time.RFC3339)}) +} + +// GetExternalCreateSucceeded returns the time at which the external resource +// was most recently created. +func GetExternalCreateSucceeded(o metav1.Object) *metav1.Time { + a := o.GetAnnotations()[AnnotationKeyExternalCreateSucceeded] + t, err := time.Parse(time.RFC3339, a) + if err != nil { + return nil + } + return &metav1.Time{Time: t} +} + +// SetExternalCreateSucceeded sets the time at which the external resource was +// most recently created to the supplied time. +func SetExternalCreateSucceeded(o metav1.Object, t metav1.Time) { + AddAnnotations(o, map[string]string{AnnotationKeyExternalCreateSucceeded: t.Format(time.RFC3339)}) + RemoveAnnotations(o, AnnotationKeyExternalCreatePending) +} + +// GetExternalCreateFailed returns the time at which the external resource +// recently failed to create. +func GetExternalCreateFailed(o metav1.Object) *metav1.Time { + a := o.GetAnnotations()[AnnotationKeyExternalCreateFailed] + t, err := time.Parse(time.RFC3339, a) + if err != nil { + return nil + } + return &metav1.Time{Time: t} +} + +// SetExternalCreateFailed sets the time at which the external resource most +// recently failed to create. +func SetExternalCreateFailed(o metav1.Object, t metav1.Time) { + AddAnnotations(o, map[string]string{AnnotationKeyExternalCreateFailed: t.Format(time.RFC3339)}) + RemoveAnnotations(o, AnnotationKeyExternalCreatePending) +} + +// ExternalCreateSucceededDuring returns true if creation of the external +// resource that corresponds to the supplied managed resource succeeded within +// the supplied duration. +func ExternalCreateSucceededDuring(o metav1.Object, d time.Duration) bool { + t := GetExternalCreateSucceeded(o) + if t == nil { + return false + } + return time.Since(t.Time) < d +} + // AllowPropagation from one object to another by adding consenting annotations // to both. // Deprecated: This functionality will be removed soon. diff --git a/pkg/meta/meta_test.go b/pkg/meta/meta_test.go index a71fb798d..62daa6cfb 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -20,6 +20,7 @@ import ( "fmt" "hash/fnv" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/pkg/errors" @@ -901,6 +902,225 @@ func TestSetExternalName(t *testing.T) { } } +func TestGetExternalCreatePending(t *testing.T) { + now := &metav1.Time{Time: time.Now().Round(time.Second)} + + cases := map[string]struct { + o metav1.Object + want *metav1.Time + }{ + "ExternalCreatePendingExists": { + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreatePending: now.Format(time.RFC3339)}}}, + want: now, + }, + "NoExternalCreatePending": { + o: &corev1.Pod{}, + want: nil, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := GetExternalCreatePending(tc.o) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("GetExternalCreatePending(...): -want, +got:\n%s", diff) + } + }) + } +} + +func TestSetExternalCreatePending(t *testing.T) { + now := metav1.Now() + + cases := map[string]struct { + o metav1.Object + t metav1.Time + want metav1.Object + }{ + "SetsTheCorrectKey": { + o: &corev1.Pod{}, + t: now, + want: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreatePending: now.Format(time.RFC3339)}}}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + SetExternalCreatePending(tc.o, tc.t) + if diff := cmp.Diff(tc.want, tc.o); diff != "" { + t.Errorf("SetExternalCreatePending(...): -want, +got:\n%s", diff) + } + }) + } +} + +func TestGetExternalCreateSucceeded(t *testing.T) { + now := &metav1.Time{Time: time.Now().Round(time.Second)} + + cases := map[string]struct { + o metav1.Object + want *metav1.Time + }{ + "ExternalCreateTimeExists": { + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateSucceeded: now.Format(time.RFC3339)}}}, + want: now, + }, + "NoExternalCreateTime": { + o: &corev1.Pod{}, + want: nil, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := GetExternalCreateSucceeded(tc.o) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("GetExternalCreateSucceeded(...): -want, +got:\n%s", diff) + } + }) + } +} + +func TestSetExternalCreateSucceeded(t *testing.T) { + now := metav1.Now() + + cases := map[string]struct { + o metav1.Object + t metav1.Time + want metav1.Object + }{ + "SetsTheCorrectKey": { + o: &corev1.Pod{}, + t: now, + want: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateSucceeded: now.Format(time.RFC3339)}}}, + }, + "RemovesCreatePendingKey": { + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreatePending: now.Format(time.RFC3339)}}}, + t: now, + want: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateSucceeded: now.Format(time.RFC3339)}}}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + SetExternalCreateSucceeded(tc.o, tc.t) + if diff := cmp.Diff(tc.want, tc.o); diff != "" { + t.Errorf("SetExternalCreateSucceeded(...): -want, +got:\n%s", diff) + } + }) + } +} + +func TestGetExternalCreateFailed(t *testing.T) { + now := &metav1.Time{Time: time.Now().Round(time.Second)} + + cases := map[string]struct { + o metav1.Object + want *metav1.Time + }{ + "ExternalCreateFailedExists": { + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateFailed: now.Format(time.RFC3339)}}}, + want: now, + }, + "NoExternalCreateFailed": { + o: &corev1.Pod{}, + want: nil, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := GetExternalCreateFailed(tc.o) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("GetExternalCreateFailed(...): -want, +got:\n%s", diff) + } + }) + } +} + +func TestSetExternalCreateFailed(t *testing.T) { + now := metav1.Now() + + cases := map[string]struct { + o metav1.Object + t metav1.Time + want metav1.Object + }{ + "SetsTheCorrectKey": { + o: &corev1.Pod{}, + t: now, + want: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateFailed: now.Format(time.RFC3339)}}}, + }, + "RemovesCreatePendingKey": { + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreatePending: now.Format(time.RFC3339)}}}, + t: now, + want: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateFailed: now.Format(time.RFC3339)}}}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + SetExternalCreateFailed(tc.o, tc.t) + if diff := cmp.Diff(tc.want, tc.o); diff != "" { + t.Errorf("SetExternalCreateFailed(...): -want, +got:\n%s", diff) + } + }) + } +} + +func TestExternalCreateSucceededDuring(t *testing.T) { + type args struct { + o metav1.Object + d time.Duration + } + + cases := map[string]struct { + args args + want bool + }{ + "NotYetSuccessfullyCreated": { + args: args{ + o: &corev1.Pod{}, + d: 1 * time.Minute, + }, + want: false, + }, + "SuccessfullyCreatedTooLongAgo": { + args: args{ + o: func() metav1.Object { + o := &corev1.Pod{} + t := time.Now().Add(-2 * time.Minute) + SetExternalCreateSucceeded(o, metav1.NewTime(t)) + return o + }(), + d: 1 * time.Minute, + }, + want: false, + }, + "SuccessfullyCreatedWithinDuration": { + args: args{ + o: func() metav1.Object { + o := &corev1.Pod{} + t := time.Now().Add(-30 * time.Second) + SetExternalCreateSucceeded(o, metav1.NewTime(t)) + return o + }(), + d: 1 * time.Minute, + }, + want: true, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := ExternalCreateSucceededDuring(tc.args.o, tc.args.d) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("ExternalCreateSucceededDuring(...): -want, +got:\n%s", diff) + } + }) + } +} + func TestAllowPropagation(t *testing.T) { fromns := "from-namespace" from := "from-name" diff --git a/pkg/reconciler/managed/api.go b/pkg/reconciler/managed/api.go index 6aff26943..3eb1d00a2 100644 --- a/pkg/reconciler/managed/api.go +++ b/pkg/reconciler/managed/api.go @@ -22,6 +22,8 @@ import ( "github.com/google/go-cmp/cmp" "github.com/pkg/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" @@ -31,10 +33,11 @@ import ( // Error strings. const ( - errCreateOrUpdateSecret = "cannot create or update connection secret" - errUpdateManaged = "cannot update managed resource" - errUpdateManagedStatus = "cannot update managed resource status" - errResolveReferences = "cannot resolve references" + errCreateOrUpdateSecret = "cannot create or update connection secret" + errUpdateManaged = "cannot update managed resource" + errUpdateManagedStatus = "cannot update managed resource status" + errResolveReferences = "cannot resolve references" + errUpdateCriticalAnnotations = "cannot update critical annotations" ) // NameAsExternalName writes the name of the managed resource to @@ -146,3 +149,33 @@ func (a *APISimpleReferenceResolver) ResolveReferences(ctx context.Context, mg r return errors.Wrap(a.client.Update(ctx, mg), errUpdateManaged) } + +// A RetryingCriticalAnnotationUpdater is a CriticalAnnotationUpdater that +// retries annotation updates in the face of API server errors. +type RetryingCriticalAnnotationUpdater struct { + client client.Client +} + +// NewRetryingCriticalAnnotationUpdater returns a CriticalAnnotationUpdater that +// retries annotation updates in the face of API server errors. +func NewRetryingCriticalAnnotationUpdater(c client.Client) *RetryingCriticalAnnotationUpdater { + return &RetryingCriticalAnnotationUpdater{client: c} +} + +// UpdateCriticalAnnotations updates (i.e. persists) the annotations of the +// supplied Object. It retries in the face of any API server error several times +// in order to ensure annotations that contain critical state are persisted. Any +// pending changes to the supplied Object's spec, status, or other metadata are +// reset to their current state according to the API server. +func (u *RetryingCriticalAnnotationUpdater) UpdateCriticalAnnotations(ctx context.Context, o client.Object) error { + a := o.GetAnnotations() + err := retry.OnError(retry.DefaultRetry, resource.IsAPIError, func() error { + nn := types.NamespacedName{Name: o.GetName()} + if err := u.client.Get(ctx, nn, o); err != nil { + return err + } + meta.AddAnnotations(o, a) + return u.client.Update(ctx, o) + }) + return errors.Wrap(err, errUpdateCriticalAnnotations) +} diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index f89da6cb6..650273759 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -377,7 +377,67 @@ func TestResolveReferences(t *testing.T) { r := NewAPISimpleReferenceResolver(tc.c) got := r.ResolveReferences(tc.args.ctx, tc.args.mg) if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { - t.Errorf("\nReason: %s\r.ResolveReferences(...): -want, +got:\n%s", tc.reason, diff) + t.Errorf("\n%s\nr.ResolveReferences(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestRetryingCriticalAnnotationUpdater(t *testing.T) { + + errBoom := errors.New("boom") + + type args struct { + ctx context.Context + o client.Object + } + + cases := map[string]struct { + reason string + c client.Client + args args + want error + }{ + "GetError": { + reason: "We should return any error we encounter getting the supplied object", + c: &test.MockClient{ + MockGet: test.NewMockGetFn(errBoom), + }, + args: args{ + o: &fake.Managed{}, + }, + want: errors.Wrap(errBoom, errUpdateCriticalAnnotations), + }, + "UpdateError": { + reason: "We should return any error we encounter updating the supplied object", + c: &test.MockClient{ + MockGet: test.NewMockGetFn(nil), + MockUpdate: test.NewMockUpdateFn(errBoom), + }, + args: args{ + o: &fake.Managed{}, + }, + want: errors.Wrap(errBoom, errUpdateCriticalAnnotations), + }, + "Success": { + reason: "We should return without error if we successfully update our annotations", + c: &test.MockClient{ + MockGet: test.NewMockGetFn(nil), + MockUpdate: test.NewMockUpdateFn(errBoom), + }, + args: args{ + o: &fake.Managed{}, + }, + want: errors.Wrap(errBoom, errUpdateCriticalAnnotations), + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + u := NewRetryingCriticalAnnotationUpdater(tc.c) + got := u.UpdateCriticalAnnotations(tc.args.ctx, tc.args.o) + if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...): -want, +got:\n%s", tc.reason, diff) } }) } diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 3e155ca53..717a5d9c4 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -21,8 +21,7 @@ import ( "strings" "time" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/util/retry" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/pkg/errors" "k8s.io/apimachinery/pkg/runtime/schema" @@ -48,7 +47,8 @@ const ( // Error strings. const ( errGetManaged = "cannot get managed resource" - errUpdateManagedAfterCreate = "cannot update managed resource. this may have resulted in a leaked external resource" + errCreatePending = "refusing to reconcile managed resource with " + meta.AnnotationKeyExternalCreatePending + " annotation - remove if it is safe to proceed" + errUpdateManagedAnnotations = "cannot update managed resource annotations" errReconcileConnect = "connect failed" errReconcileObserve = "observe failed" errReconcileCreate = "create failed" @@ -72,6 +72,7 @@ const ( reasonDeleted event.Reason = "DeletedExternalResource" reasonCreated event.Reason = "CreatedExternalResource" reasonUpdated event.Reason = "UpdatedExternalResource" + reasonPending event.Reason = "PendingExternalResource" ) // ControllerName returns the recommended name for controllers that use this @@ -80,6 +81,21 @@ func ControllerName(kind string) string { return "managed/" + strings.ToLower(kind) } +// A CriticalAnnotationUpdater is used when it is critical that annotations must +// be updated before returning from the Reconcile loop. +type CriticalAnnotationUpdater interface { + UpdateCriticalAnnotations(ctx context.Context, o client.Object) error +} + +// A CriticalAnnotationUpdateFn may be used when it is critical that annotations +// must be updated before returning from the Reconcile loop. +type CriticalAnnotationUpdateFn func(ctx context.Context, o client.Object) error + +// UpdateCriticalAnnotations of the supplied object. +func (fn CriticalAnnotationUpdateFn) UpdateCriticalAnnotations(ctx context.Context, o client.Object) error { + return fn(ctx, o) +} + // ConnectionDetails created or updated during an operation on an external // resource, for example usernames, passwords, endpoints, ports, etc. type ConnectionDetails map[string][]byte @@ -185,15 +201,19 @@ func (ec ExternalConnectorFn) Connect(ctx context.Context, mg resource.Managed) // if it's called again with the same parameters or Delete call should not // return error if there is an ongoing deletion or resource does not exist. type ExternalClient interface { - // Observe the external resource the supplied Managed resource represents, - // if any. Observe implementations must not modify the external resource, - // but may update the supplied Managed resource to reflect the state of the - // external resource. + // Observe the external resource the supplied Managed resource + // represents, if any. Observe implementations must not modify the + // external resource, but may update the supplied Managed resource to + // reflect the state of the external resource. Status modifications are + // automatically persisted unless ResourceLateInitialized is true - see + // ResourceLateInitialized for more detail. Observe(ctx context.Context, mg resource.Managed) (ExternalObservation, error) // Create an external resource per the specifications of the supplied // Managed resource. Called when Observe reports that the associated - // external resource does not exist. + // external resource does not exist. Create implementations may update + // managed resource annotations, and those updates will be persisted. + // All other updates will be discarded. Create(ctx context.Context, mg resource.Managed) (ExternalCreation, error) // Update the external resource represented by the supplied Managed @@ -310,10 +330,14 @@ type ExternalObservation struct { // An ExternalCreation is the result of the creation of an external resource. type ExternalCreation struct { - // ExternalNameAssigned is true if the Create operation resulted in a change - // in the external name annotation. If that's the case, we need to issue a - // spec update and make sure it goes through so that we don't lose the identifier - // of the resource we just created. + // ExternalNameAssigned should be true if the Create operation resulted + // in a change in the resource's external name. This is typically only + // needed for external resource's with unpredictable external names that + // are returned from the API at create time. + // + // Deprecated: The managed.Reconciler no longer needs to be informed + // when an external name is assigned by the Create operation. It will + // automatically detect and handle external name assignment. ExternalNameAssigned bool // ConnectionDetails required to connect to this resource. These details @@ -342,8 +366,9 @@ type Reconciler struct { client client.Client newManaged func() resource.Managed - pollInterval time.Duration - timeout time.Duration + pollInterval time.Duration + timeout time.Duration + creationGracePeriod time.Duration // The below structs embed the set of interfaces used to implement the // managed resource reconciler. We do this primarily for readability, so @@ -357,6 +382,7 @@ type Reconciler struct { } type mrManaged struct { + CriticalAnnotationUpdater ConnectionPublisher resource.Finalizer Initializer @@ -365,8 +391,9 @@ type mrManaged struct { func defaultMRManaged(m manager.Manager) mrManaged { return mrManaged{ - ConnectionPublisher: NewAPISecretPublisher(m.GetClient(), m.GetScheme()), - Finalizer: resource.NewAPIFinalizer(m.GetClient(), managedFinalizerName), + CriticalAnnotationUpdater: NewRetryingCriticalAnnotationUpdater(m.GetClient()), + ConnectionPublisher: NewAPISecretPublisher(m.GetClient(), m.GetScheme()), + Finalizer: resource.NewAPIFinalizer(m.GetClient(), managedFinalizerName), Initializer: InitializerChain{ NewDefaultProviderConfig(m.GetClient()), NewNameAsExternalName(m.GetClient()), @@ -409,6 +436,16 @@ func WithPollInterval(after time.Duration) ReconcilerOption { } } +// WithCreationGracePeriod configures an optional period during which we will +// wait for the external API to report that a newly created external resource +// exists. This allows us to tolerate eventually consistent APIs that do not +// immediately report that newly created resources exist when queried. +func WithCreationGracePeriod(d time.Duration) ReconcilerOption { + return func(r *Reconciler) { + r.creationGracePeriod = d + } +} + // WithExternalConnecter specifies how the Reconciler should connect to the API // used to sync and delete external resources. func WithExternalConnecter(c ExternalConnecter) ReconcilerOption { @@ -417,6 +454,16 @@ func WithExternalConnecter(c ExternalConnecter) ReconcilerOption { } } +// WithCriticalAnnotationUpdater specifies how the Reconciler should update a +// managed resource's critical annotations. Implementations typically contain +// some kind of retry logic to increase the likelihood that critical annotations +// (like non-deterministic external names) will be persisted. +func WithCriticalAnnotationUpdater(u CriticalAnnotationUpdater) ReconcilerOption { + return func(r *Reconciler) { + r.managed.CriticalAnnotationUpdater = u + } +} + // WithConnectionPublishers specifies how the Reconciler should publish // its connection details such as credentials and endpoints. func WithConnectionPublishers(p ...ConnectionPublisher) ReconcilerOption { @@ -538,6 +585,18 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + // A resource would only be pending creation at this point if we failed + // to persist our annotations after the ExternalClient's Create method + // was called. If that is the case we may have lost a critical update to + // the external name and leaked a resource. The safest thing to do is to + // refuse to proceed. + if meta.GetExternalCreatePending(managed) != nil { + log.Debug(errCreatePending) + record.Event(managed, event.Warning(reasonCannotInitialize, errors.New(errCreatePending))) + managed.SetConditions(xpv1.ReconcileError(errors.New(errCreatePending))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + } + // We resolve any references before observing our external resource because // in some rare examples we need a spec field to make the observe call, and // that spec field could be set by a reference. @@ -588,6 +647,17 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + // If this resource has a non-zero creation grace period we want to wait + // for that period to expire before we trust that the resource really + // doesn't exist. This is because some external APIs are eventually + // consistent and may report that a recently created resource does not + // exist. + if !observation.ResourceExists && meta.ExternalCreateSucceededDuring(managed, r.creationGracePeriod) { + log.Debug("Waiting for external resource existence to be confirmed") + record.Event(managed, event.Normal(reasonPending, "Waiting for external resource existence to be confirmed")) + return reconcile.Result{Requeue: true}, nil + } + if meta.WasDeleted(managed) { log = log.WithValues("deletion-timestamp", managed.GetDeletionTimestamp()) @@ -666,6 +736,22 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc } if !observation.ResourceExists { + + // We write this annotation for two reasons. Firstly, it helps + // us to detect the case in which we fail to persist critical + // information (like the external name) that may be set by the + // subsequent external.Create call. Secondly, it guarantees that + // we're operating on the latest version of our resource. We + // don't use the CriticalAnnotationUpdater because we _want_ the + // update to fail if we get a 409 due to a stale version. + meta.SetExternalCreatePending(managed, metav1.Now()) + if err := r.client.Update(ctx, managed); err != nil { + log.Debug(errUpdateManaged, "error", err) + record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManaged))) + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errUpdateManaged))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + } + managed.SetConditions(xpv1.Creating()) creation, err := external.Create(externalCtx, managed) if err != nil { @@ -676,30 +762,49 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc // the new error condition. If not, we requeue explicitly, which will trigger backoff. log.Debug("Cannot create external resource", "error", err) record.Event(managed, event.Warning(reasonCannotCreate, err)) + + // We handle annotations specially here because it's + // critical that they are persisted to the API server. + // If we don't remove the external-create-pending + // annotation the reconciler will refuse to proceed, + // because it won't know whether or not it created an + // external resource. + meta.SetExternalCreateFailed(managed, metav1.Now()) + if err := r.managed.UpdateCriticalAnnotations(ctx, managed); err != nil { + log.Debug(errUpdateManagedAnnotations, "error", err) + record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAnnotations))) + + // We only log and emit an event here rather + // than setting a status condition and returning + // early because presumably it's more useful to + // set our status condition to the reason the + // create failed. + } + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileCreate))) return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } - if creation.ExternalNameAssigned { - en := meta.GetExternalName(managed) - // We will retry in all cases where the error comes from the api-server. - // At one point, context deadline will be exceeded and we'll get out - // of the loop. In that case, we warn the user that the external resource - // might be leaked. - err := retry.OnError(retry.DefaultRetry, resource.IsAPIError, func() error { - nn := types.NamespacedName{Name: managed.GetName()} - if err := r.client.Get(ctx, nn, managed); err != nil { - return err - } - meta.SetExternalName(managed, en) - return r.client.Update(ctx, managed) - }) - if err != nil { - log.Debug("Cannot update managed resource", "error", err) - record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAfterCreate))) - managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errUpdateManagedAfterCreate))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) - } + // In some cases our external-name may be set by Create above. + log = log.WithValues("external-name", meta.GetExternalName(managed)) + record = r.record.WithAnnotations("external-name", meta.GetExternalName(managed)) + + // We handle annotations specially here because it's critical + // that they are persisted to the API server. If we don't remove + // the external-create-pending annotation the reconciler will + // refuse to proceed, because it won't know whether or not it + // created an external resource. This is important in cases + // where we must record an external-name annotation set by the + // Create call. Any other changes made during Create will be + // reverted when annotations are updated; at the time of writing + // Create implementations are advised not to alter status, but + // we may revisit this in future. + meta.SetExternalCreateSucceeded(managed, metav1.Now()) + if err := r.managed.UpdateCriticalAnnotations(ctx, managed); err != nil { + log.Debug(errUpdateManagedAnnotations, "error", err) + record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAnnotations))) + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errUpdateManagedAnnotations))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } if err := r.managed.PublishConnection(ctx, managed, creation.ConnectionDetails); err != nil { diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index 250c26531..93a64bbc5 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -19,12 +19,14 @@ package managed import ( "context" "testing" + "time" kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -108,6 +110,35 @@ func TestReconciler(t *testing.T) { }, want: want{result: reconcile.Result{Requeue: true}}, }, + "ExternalCreatePending": { + reason: "We should return early if the managed resource appears to be pending creation. We might have leaked a resource and don't want to create another.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + meta.SetExternalCreatePending(obj, now) + return nil + }), + MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { + want := &fake.Managed{} + meta.SetExternalCreatePending(want, now) + want.SetConditions(xpv1.ReconcileError(errors.New(errCreatePending))) + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := "We should update our status when we're asked to reconcile a managed resource that is pending creation." + 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(InitializerFn(func(_ context.Context, mg resource.Managed) error { return nil })), + }, + }, + want: want{result: reconcile.Result{Requeue: true}}, + }, "ResolveReferencesError": { reason: "Errors during reference resolution references should trigger a requeue after a short wait.", args: args{ @@ -197,6 +228,34 @@ func TestReconciler(t *testing.T) { }, want: want{result: reconcile.Result{Requeue: true}}, }, + "CreationGracePeriod": { + reason: "If our resource appears not to exist during the creation grace period we should return early.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + meta.SetExternalCreateSucceeded(obj, metav1.Now()) + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithCreationGracePeriod(1 * time.Minute), + WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{ResourceExists: false}, nil + }, + } + return c, nil + })), + }, + }, + want: want{result: reconcile.Result{Requeue: true}}, + }, "ExternalDeleteError": { reason: "Errors deleting the external resource should trigger a requeue after a short wait.", args: args{ @@ -460,17 +519,18 @@ func TestReconciler(t *testing.T) { }, want: want{result: reconcile.Result{Requeue: true}}, }, - "CreateExternalError": { - reason: "Errors while creating an external resource should trigger a requeue after a short wait.", + "UpdateCreatePendingError": { + reason: "Errors while updating our external-create-pending annotation should trigger a requeue after a short wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: test.NewMockGetFn(nil), + MockGet: test.NewMockGetFn(nil), + MockUpdate: test.NewMockUpdateFn(errBoom), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileCreate))) - want.SetConditions(xpv1.Creating()) - if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + meta.SetExternalCreatePending(want, metav1.Now()) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errUpdateManaged))) + if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { reason := "Errors while creating an external resource should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) } @@ -500,18 +560,20 @@ func TestReconciler(t *testing.T) { }, want: want{result: reconcile.Result{Requeue: true}}, }, - "PublishCreationConnectionDetailsError": { - reason: "Errors publishing connection details after creation should trigger a requeue after a short wait.", + "CreateExternalError": { + reason: "Errors while creating an external resource should trigger a requeue after a short wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: test.NewMockGetFn(nil), + MockGet: test.NewMockGetFn(nil), + MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - want.SetConditions(xpv1.ReconcileError(errBoom)) + meta.SetExternalCreateFailed(want, metav1.Now()) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileCreate))) want.SetConditions(xpv1.Creating()) - if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { - reason := "Errors publishing connection details after creation should be reported as a conditioned status." + if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { + reason := "Errors while creating an external resource should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) } return nil @@ -529,60 +591,22 @@ func TestReconciler(t *testing.T) { return ExternalObservation{ResourceExists: false}, nil }, CreateFn: func(_ context.Context, _ resource.Managed) (ExternalCreation, error) { - cd := ConnectionDetails{"create": []byte{}} - return ExternalCreation{ConnectionDetails: cd}, nil + return ExternalCreation{}, errBoom }, } return c, nil })), - WithConnectionPublishers(ConnectionPublisherFns{ - PublishConnectionFn: func(_ context.Context, _ resource.Managed, cd ConnectionDetails) error { - // We're called after observe, create, and update - // but we only want to fail when publishing details - // after a creation. - if _, ok := cd["create"]; ok { - return errBoom - } - return nil - }, - }), - WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), - }, - }, - want: want{result: reconcile.Result{Requeue: true}}, - }, - "CreateSuccessful": { - reason: "Successful managed resource creation should trigger a requeue after a short wait.", - args: args{ - m: &fake.Manager{ - Client: &test.MockClient{ - MockGet: test.NewMockGetFn(nil), - MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { - want := &fake.Managed{} - want.SetConditions(xpv1.ReconcileSuccess()) - want.SetConditions(xpv1.Creating()) - if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { - reason := "Successful managed resource creation should be reported as a conditioned status." - 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(&NopConnecter{}), + // We simulate our critical annotation update failing too here. + // This is mostly just to exercise the code, which just creates a log and an event. + WithCriticalAnnotationUpdater(CriticalAnnotationUpdateFn(func(ctx context.Context, o client.Object) error { return errBoom })), WithConnectionPublishers(), WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), }, }, want: want{result: reconcile.Result{Requeue: true}}, }, - "CreateWithExternalNameAssignmentSuccessful": { - reason: "Successful managed resource creation with external name assignment should trigger an update.", + "UpdateCriticalAnnotationsError": { + reason: "Errors updating critical annotations after creation should trigger a requeue after a short wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ @@ -590,11 +614,11 @@ func TestReconciler(t *testing.T) { MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - meta.SetExternalName(want, "test") - want.SetConditions(xpv1.ReconcileSuccess()) + meta.SetExternalCreateSucceeded(want, metav1.Now()) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errUpdateManagedAnnotations))) want.SetConditions(xpv1.Creating()) - if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { - reason := "Successful managed resource creation should be reported as a conditioned status." + if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { + reason := "Errors updating critical annotations after creation should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) } return nil @@ -608,40 +632,35 @@ func TestReconciler(t *testing.T) { WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { c := &ExternalClientFns{ - CreateFn: func(_ context.Context, mg resource.Managed) (ExternalCreation, error) { - meta.SetExternalName(mg, "test") - return ExternalCreation{ExternalNameAssigned: true}, nil - }, ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { - return ExternalObservation{}, nil + return ExternalObservation{ResourceExists: false}, nil + }, + CreateFn: func(_ context.Context, _ resource.Managed) (ExternalCreation, error) { + return ExternalCreation{}, nil }, } return c, nil })), - WithConnectionPublishers(), WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + WithCriticalAnnotationUpdater(CriticalAnnotationUpdateFn(func(ctx context.Context, o client.Object) error { return errBoom })), }, }, want: want{result: reconcile.Result{Requeue: true}}, }, - "CreateWithExternalNameAssignmentGetError": { - reason: "If the Get call during the update after Create does not go through, we need to inform the user and requeue shortly.", + "PublishCreationConnectionDetailsError": { + reason: "Errors publishing connection details after creation should trigger a requeue after a short wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ - MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error { - if meta.GetExternalName(obj.(metav1.Object)) == "test" { - return errBoom - } - return nil - }, + MockGet: test.NewMockGetFn(nil), + MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - meta.SetExternalName(want, "test") - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errUpdateManagedAfterCreate))) + meta.SetExternalCreateSucceeded(want, metav1.Now()) + want.SetConditions(xpv1.ReconcileError(errBoom)) want.SetConditions(xpv1.Creating()) - if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { - reason := "Successful managed resource creation should be reported as a conditioned status." + if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { + reason := "Errors publishing connection details after creation should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) } return nil @@ -655,35 +674,46 @@ func TestReconciler(t *testing.T) { WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { c := &ExternalClientFns{ - CreateFn: func(_ context.Context, mg resource.Managed) (ExternalCreation, error) { - meta.SetExternalName(mg, "test") - return ExternalCreation{ExternalNameAssigned: true}, nil - }, ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { - return ExternalObservation{}, nil + return ExternalObservation{ResourceExists: false}, nil + }, + CreateFn: func(_ context.Context, _ resource.Managed) (ExternalCreation, error) { + cd := ConnectionDetails{"create": []byte{}} + return ExternalCreation{ConnectionDetails: cd}, nil }, } return c, nil })), - WithConnectionPublishers(), + WithCriticalAnnotationUpdater(CriticalAnnotationUpdateFn(func(ctx context.Context, o client.Object) error { return nil })), + WithConnectionPublishers(ConnectionPublisherFns{ + PublishConnectionFn: func(_ context.Context, _ resource.Managed, cd ConnectionDetails) error { + // We're called after observe, create, and update + // but we only want to fail when publishing details + // after a creation. + if _, ok := cd["create"]; ok { + return errBoom + } + return nil + }, + }), WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), }, }, want: want{result: reconcile.Result{Requeue: true}}, }, - "CreateWithExternalNameAssignmentUpdateError": { - reason: "If the update after Create does not go through, we need to inform the user and requeue shortly.", + "CreateSuccessful": { + reason: "Successful managed resource creation should trigger a requeue after a short wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ MockGet: test.NewMockGetFn(nil), - MockUpdate: test.NewMockUpdateFn(errBoom), + MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - meta.SetExternalName(want, "test") - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errUpdateManagedAfterCreate))) + meta.SetExternalCreateSucceeded(want, metav1.Now()) + want.SetConditions(xpv1.ReconcileSuccess()) want.SetConditions(xpv1.Creating()) - if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { reason := "Successful managed resource creation should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) } @@ -696,18 +726,8 @@ func TestReconciler(t *testing.T) { o: []ReconcilerOption{ WithInitializers(), WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), - WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { - c := &ExternalClientFns{ - CreateFn: func(_ context.Context, mg resource.Managed) (ExternalCreation, error) { - meta.SetExternalName(mg, "test") - return ExternalCreation{ExternalNameAssigned: true}, nil - }, - ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { - return ExternalObservation{}, nil - }, - } - return c, nil - })), + WithExternalConnecter(&NopConnecter{}), + WithCriticalAnnotationUpdater(CriticalAnnotationUpdateFn(func(ctx context.Context, o client.Object) error { return nil })), WithConnectionPublishers(), WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), }, From 88ef9dcbb9cdf50a7fdc07d23b8201453864ceac Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Wed, 1 Sep 2021 09:41:12 +0000 Subject: [PATCH 2/2] Don't rely on removal of the external-create-pending annotation The retry logic we use to persist critical annotations makes it difficult to delete an annotation without potentially also deleting annotations added by another controller (e.g. the composition logic). This commit therefore changes the way we detect whether we might have created an external resource but not recorded the result. Previously we relied on the presence of the 'pending' annotation to detect this state. Now we check whether the 'pending' annotation is newer than any 'succeeded' or 'failed' annotation. Signed-off-by: Nic Cope --- pkg/meta/meta.go | 57 +++++++---- pkg/meta/meta_test.go | 113 ++++++++++++++++------ pkg/reconciler/managed/reconciler.go | 69 +++++++------ pkg/reconciler/managed/reconciler_test.go | 24 +++-- 4 files changed, 172 insertions(+), 91 deletions(-) diff --git a/pkg/meta/meta.go b/pkg/meta/meta.go index 0f5ce6359..cdc96e50d 100644 --- a/pkg/meta/meta.go +++ b/pkg/meta/meta.go @@ -40,9 +40,8 @@ const ( // AnnotationKeyExternalCreatePending is the key in the annotations map // of a resource that indicates the last time creation of the external - // resource was pending. Its value must be an RFC3999 timestamp. This - // annotation is removed once we record the result of external resource - // creation. + // resource was pending (i.e. about to happen). Its value must be an + // RFC3999 timestamp. AnnotationKeyExternalCreatePending = "crossplane.io/external-create-pending" // AnnotationKeyExternalCreateSucceeded is the key in the annotations @@ -272,55 +271,75 @@ func SetExternalName(o metav1.Object, name string) { // GetExternalCreatePending returns the time at which the external resource // was most recently pending creation. -func GetExternalCreatePending(o metav1.Object) *metav1.Time { +func GetExternalCreatePending(o metav1.Object) time.Time { a := o.GetAnnotations()[AnnotationKeyExternalCreatePending] t, err := time.Parse(time.RFC3339, a) if err != nil { - return nil + return time.Time{} } - return &metav1.Time{Time: t} + return t } // SetExternalCreatePending sets the time at which the external resource was // most recently pending creation to the supplied time. -func SetExternalCreatePending(o metav1.Object, t metav1.Time) { +func SetExternalCreatePending(o metav1.Object, t time.Time) { AddAnnotations(o, map[string]string{AnnotationKeyExternalCreatePending: t.Format(time.RFC3339)}) } // GetExternalCreateSucceeded returns the time at which the external resource // was most recently created. -func GetExternalCreateSucceeded(o metav1.Object) *metav1.Time { +func GetExternalCreateSucceeded(o metav1.Object) time.Time { a := o.GetAnnotations()[AnnotationKeyExternalCreateSucceeded] t, err := time.Parse(time.RFC3339, a) if err != nil { - return nil + return time.Time{} } - return &metav1.Time{Time: t} + return t } // SetExternalCreateSucceeded sets the time at which the external resource was // most recently created to the supplied time. -func SetExternalCreateSucceeded(o metav1.Object, t metav1.Time) { +func SetExternalCreateSucceeded(o metav1.Object, t time.Time) { AddAnnotations(o, map[string]string{AnnotationKeyExternalCreateSucceeded: t.Format(time.RFC3339)}) - RemoveAnnotations(o, AnnotationKeyExternalCreatePending) } // GetExternalCreateFailed returns the time at which the external resource // recently failed to create. -func GetExternalCreateFailed(o metav1.Object) *metav1.Time { +func GetExternalCreateFailed(o metav1.Object) time.Time { a := o.GetAnnotations()[AnnotationKeyExternalCreateFailed] t, err := time.Parse(time.RFC3339, a) if err != nil { - return nil + return time.Time{} } - return &metav1.Time{Time: t} + return t } // SetExternalCreateFailed sets the time at which the external resource most // recently failed to create. -func SetExternalCreateFailed(o metav1.Object, t metav1.Time) { +func SetExternalCreateFailed(o metav1.Object, t time.Time) { AddAnnotations(o, map[string]string{AnnotationKeyExternalCreateFailed: t.Format(time.RFC3339)}) - RemoveAnnotations(o, AnnotationKeyExternalCreatePending) +} + +// ExternalCreateIncomplete returns true if creation of the external resource +// appears to be incomplete. We deem creation to be incomplete if the 'external +// create pending' annotation is the newest of all tracking annotations that are +// set (i.e. pending, succeeded, and failed). +func ExternalCreateIncomplete(o metav1.Object) bool { + pending := GetExternalCreatePending(o) + succeeded := GetExternalCreateSucceeded(o) + failed := GetExternalCreateFailed(o) + + // If creation never started it can't be incomplete. + if pending.IsZero() { + return false + } + + latest := succeeded + if failed.After(succeeded) { + latest = failed + } + + return pending.After(latest) } // ExternalCreateSucceededDuring returns true if creation of the external @@ -328,10 +347,10 @@ func SetExternalCreateFailed(o metav1.Object, t metav1.Time) { // the supplied duration. func ExternalCreateSucceededDuring(o metav1.Object, d time.Duration) bool { t := GetExternalCreateSucceeded(o) - if t == nil { + if t.IsZero() { return false } - return time.Since(t.Time) < d + return time.Since(t) < d } // AllowPropagation from one object to another by adding consenting annotations diff --git a/pkg/meta/meta_test.go b/pkg/meta/meta_test.go index 62daa6cfb..8f0ffadd8 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -903,11 +903,11 @@ func TestSetExternalName(t *testing.T) { } func TestGetExternalCreatePending(t *testing.T) { - now := &metav1.Time{Time: time.Now().Round(time.Second)} + now := time.Now().Round(time.Second) cases := map[string]struct { o metav1.Object - want *metav1.Time + want time.Time }{ "ExternalCreatePendingExists": { o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreatePending: now.Format(time.RFC3339)}}}, @@ -915,7 +915,7 @@ func TestGetExternalCreatePending(t *testing.T) { }, "NoExternalCreatePending": { o: &corev1.Pod{}, - want: nil, + want: time.Time{}, }, } @@ -930,11 +930,11 @@ func TestGetExternalCreatePending(t *testing.T) { } func TestSetExternalCreatePending(t *testing.T) { - now := metav1.Now() + now := time.Now() cases := map[string]struct { o metav1.Object - t metav1.Time + t time.Time want metav1.Object }{ "SetsTheCorrectKey": { @@ -955,11 +955,11 @@ func TestSetExternalCreatePending(t *testing.T) { } func TestGetExternalCreateSucceeded(t *testing.T) { - now := &metav1.Time{Time: time.Now().Round(time.Second)} + now := time.Now().Round(time.Second) cases := map[string]struct { o metav1.Object - want *metav1.Time + want time.Time }{ "ExternalCreateTimeExists": { o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateSucceeded: now.Format(time.RFC3339)}}}, @@ -967,7 +967,7 @@ func TestGetExternalCreateSucceeded(t *testing.T) { }, "NoExternalCreateTime": { o: &corev1.Pod{}, - want: nil, + want: time.Time{}, }, } @@ -982,11 +982,11 @@ func TestGetExternalCreateSucceeded(t *testing.T) { } func TestSetExternalCreateSucceeded(t *testing.T) { - now := metav1.Now() + now := time.Now() cases := map[string]struct { o metav1.Object - t metav1.Time + t time.Time want metav1.Object }{ "SetsTheCorrectKey": { @@ -994,11 +994,6 @@ func TestSetExternalCreateSucceeded(t *testing.T) { t: now, want: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateSucceeded: now.Format(time.RFC3339)}}}, }, - "RemovesCreatePendingKey": { - o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreatePending: now.Format(time.RFC3339)}}}, - t: now, - want: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateSucceeded: now.Format(time.RFC3339)}}}, - }, } for name, tc := range cases { @@ -1012,11 +1007,11 @@ func TestSetExternalCreateSucceeded(t *testing.T) { } func TestGetExternalCreateFailed(t *testing.T) { - now := &metav1.Time{Time: time.Now().Round(time.Second)} + now := time.Now().Round(time.Second) cases := map[string]struct { o metav1.Object - want *metav1.Time + want time.Time }{ "ExternalCreateFailedExists": { o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateFailed: now.Format(time.RFC3339)}}}, @@ -1024,7 +1019,7 @@ func TestGetExternalCreateFailed(t *testing.T) { }, "NoExternalCreateFailed": { o: &corev1.Pod{}, - want: nil, + want: time.Time{}, }, } @@ -1039,11 +1034,11 @@ func TestGetExternalCreateFailed(t *testing.T) { } func TestSetExternalCreateFailed(t *testing.T) { - now := metav1.Now() + now := time.Now() cases := map[string]struct { o metav1.Object - t metav1.Time + t time.Time want metav1.Object }{ "SetsTheCorrectKey": { @@ -1051,11 +1046,6 @@ func TestSetExternalCreateFailed(t *testing.T) { t: now, want: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateFailed: now.Format(time.RFC3339)}}}, }, - "RemovesCreatePendingKey": { - o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreatePending: now.Format(time.RFC3339)}}}, - t: now, - want: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateFailed: now.Format(time.RFC3339)}}}, - }, } for name, tc := range cases { @@ -1090,7 +1080,7 @@ func TestExternalCreateSucceededDuring(t *testing.T) { o: func() metav1.Object { o := &corev1.Pod{} t := time.Now().Add(-2 * time.Minute) - SetExternalCreateSucceeded(o, metav1.NewTime(t)) + SetExternalCreateSucceeded(o, t) return o }(), d: 1 * time.Minute, @@ -1102,7 +1092,7 @@ func TestExternalCreateSucceededDuring(t *testing.T) { o: func() metav1.Object { o := &corev1.Pod{} t := time.Now().Add(-30 * time.Second) - SetExternalCreateSucceeded(o, metav1.NewTime(t)) + SetExternalCreateSucceeded(o, t) return o }(), d: 1 * time.Minute, @@ -1121,6 +1111,75 @@ func TestExternalCreateSucceededDuring(t *testing.T) { } } +func TestExternalCreateIncomplete(t *testing.T) { + + now := time.Now().Format(time.RFC3339) + earlier := time.Now().Add(-1 * time.Second).Format(time.RFC3339) + evenEarlier := time.Now().Add(-1 * time.Minute).Format(time.RFC3339) + + cases := map[string]struct { + reason string + o metav1.Object + want bool + }{ + "CreateNeverPending": { + reason: "If we've never called Create it can't be incomplete.", + o: &corev1.Pod{}, + want: false, + }, + "CreateSucceeded": { + reason: "If Create succeeded since it was pending, it's complete.", + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + AnnotationKeyExternalCreateFailed: evenEarlier, + AnnotationKeyExternalCreatePending: earlier, + AnnotationKeyExternalCreateSucceeded: now, + }}}, + want: false, + }, + "CreateFailed": { + reason: "If Create failed since it was pending, it's complete.", + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + AnnotationKeyExternalCreateSucceeded: evenEarlier, + AnnotationKeyExternalCreatePending: earlier, + AnnotationKeyExternalCreateFailed: now, + }}}, + want: false, + }, + "CreateNeverCompleted": { + reason: "If Create was pending but never succeeded or failed, it's incomplete.", + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + AnnotationKeyExternalCreatePending: earlier, + }}}, + want: true, + }, + "RecreateNeverCompleted": { + reason: "If Create is pending and there's an older success we're probably trying to recreate a deleted external resource, and it's incomplete.", + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + AnnotationKeyExternalCreateSucceeded: earlier, + AnnotationKeyExternalCreatePending: now, + }}}, + want: true, + }, + "RetryNeverCompleted": { + reason: "If Create is pending and there's an older failure we're probably trying to recreate a deleted external resource, and it's incomplete.", + o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + AnnotationKeyExternalCreateFailed: earlier, + AnnotationKeyExternalCreatePending: now, + }}}, + want: true, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := ExternalCreateIncomplete(tc.o) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("ExternalCreateIncomplete(...): -want, +got:\n%s", diff) + } + }) + } +} + func TestAllowPropagation(t *testing.T) { fromns := "from-namespace" from := "from-name" diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 717a5d9c4..96b9be650 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -21,8 +21,6 @@ import ( "strings" "time" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/pkg/errors" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" @@ -42,13 +40,14 @@ const ( reconcileTimeout = 1 * time.Minute defaultpollInterval = 1 * time.Minute + defaultGracePeriod = 30 * time.Second ) // Error strings. const ( errGetManaged = "cannot get managed resource" - errCreatePending = "refusing to reconcile managed resource with " + meta.AnnotationKeyExternalCreatePending + " annotation - remove if it is safe to proceed" errUpdateManagedAnnotations = "cannot update managed resource annotations" + errCreateIncomplete = "cannot determine creation result - remove the " + meta.AnnotationKeyExternalCreatePending + " annotation if it is safe to proceed" errReconcileConnect = "connect failed" errReconcileObserve = "observe failed" errReconcileCreate = "create failed" @@ -439,7 +438,8 @@ func WithPollInterval(after time.Duration) ReconcilerOption { // WithCreationGracePeriod configures an optional period during which we will // wait for the external API to report that a newly created external resource // exists. This allows us to tolerate eventually consistent APIs that do not -// immediately report that newly created resources exist when queried. +// immediately report that newly created resources exist when queried. All +// resources have a 30 second grace period by default. func WithCreationGracePeriod(d time.Duration) ReconcilerOption { return func(r *Reconciler) { r.creationGracePeriod = d @@ -527,14 +527,15 @@ func NewReconciler(m manager.Manager, of resource.ManagedKind, o ...ReconcilerOp _ = nm() r := &Reconciler{ - client: m.GetClient(), - newManaged: nm, - pollInterval: defaultpollInterval, - timeout: reconcileTimeout, - managed: defaultMRManaged(m), - external: defaultMRExternal(), - log: logging.NewNopLogger(), - record: event.NewNopRecorder(), + client: m.GetClient(), + newManaged: nm, + pollInterval: defaultpollInterval, + creationGracePeriod: defaultGracePeriod, + timeout: reconcileTimeout, + managed: defaultMRManaged(m), + external: defaultMRExternal(), + log: logging.NewNopLogger(), + record: event.NewNopRecorder(), } for _, ro := range o { @@ -585,16 +586,15 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } - // A resource would only be pending creation at this point if we failed - // to persist our annotations after the ExternalClient's Create method - // was called. If that is the case we may have lost a critical update to - // the external name and leaked a resource. The safest thing to do is to - // refuse to proceed. - if meta.GetExternalCreatePending(managed) != nil { - log.Debug(errCreatePending) - record.Event(managed, event.Warning(reasonCannotInitialize, errors.New(errCreatePending))) - managed.SetConditions(xpv1.ReconcileError(errors.New(errCreatePending))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + // If we started but never completed creation of an external resource we + // may have lost critical information. For example if we didn't persist + // an updated external name we've leaked a resource. The safest thing to + // do is to refuse to proceed. + if meta.ExternalCreateIncomplete(managed) { + log.Debug(errCreateIncomplete) + record.Event(managed, event.Warning(reasonCannotInitialize, errors.New(errCreateIncomplete))) + managed.SetConditions(xpv1.ReconcileError(errors.New(errCreateIncomplete))) + return reconcile.Result{Requeue: false}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } // We resolve any references before observing our external resource because @@ -736,7 +736,6 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc } if !observation.ResourceExists { - // We write this annotation for two reasons. Firstly, it helps // us to detect the case in which we fail to persist critical // information (like the external name) that may be set by the @@ -744,7 +743,7 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc // we're operating on the latest version of our resource. We // don't use the CriticalAnnotationUpdater because we _want_ the // update to fail if we get a 409 due to a stale version. - meta.SetExternalCreatePending(managed, metav1.Now()) + meta.SetExternalCreatePending(managed, time.Now()) if err := r.client.Update(ctx, managed); err != nil { log.Debug(errUpdateManaged, "error", err) record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManaged))) @@ -765,11 +764,11 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc // We handle annotations specially here because it's // critical that they are persisted to the API server. - // If we don't remove the external-create-pending - // annotation the reconciler will refuse to proceed, - // because it won't know whether or not it created an - // external resource. - meta.SetExternalCreateFailed(managed, metav1.Now()) + // If we don't add the external-create-failed annotation + // the reconciler will refuse to proceed, because it + // won't know whether or not it created an external + // resource. + meta.SetExternalCreateFailed(managed, time.Now()) if err := r.managed.UpdateCriticalAnnotations(ctx, managed); err != nil { log.Debug(errUpdateManagedAnnotations, "error", err) record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAnnotations))) @@ -791,15 +790,15 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc // We handle annotations specially here because it's critical // that they are persisted to the API server. If we don't remove - // the external-create-pending annotation the reconciler will - // refuse to proceed, because it won't know whether or not it - // created an external resource. This is important in cases - // where we must record an external-name annotation set by the - // Create call. Any other changes made during Create will be + // add the external-create-succeeded annotation the reconciler + // will refuse to proceed, because it won't know whether or not + // it created an external resource. This is also important in + // cases where we must record an external-name annotation set by + // the Create call. Any other changes made during Create will be // reverted when annotations are updated; at the time of writing // Create implementations are advised not to alter status, but // we may revisit this in future. - meta.SetExternalCreateSucceeded(managed, metav1.Now()) + meta.SetExternalCreateSucceeded(managed, time.Now()) if err := r.managed.UpdateCriticalAnnotations(ctx, managed); err != nil { log.Debug(errUpdateManagedAnnotations, "error", err) record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAnnotations))) diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index 93a64bbc5..869d90a08 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -116,13 +116,13 @@ func TestReconciler(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { - meta.SetExternalCreatePending(obj, now) + meta.SetExternalCreatePending(obj, now.Time) return nil }), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - meta.SetExternalCreatePending(want, now) - want.SetConditions(xpv1.ReconcileError(errors.New(errCreatePending))) + meta.SetExternalCreatePending(want, now.Time) + want.SetConditions(xpv1.ReconcileError(errors.New(errCreateIncomplete))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "We should update our status when we're asked to reconcile a managed resource that is pending creation." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -137,7 +137,7 @@ func TestReconciler(t *testing.T) { WithInitializers(InitializerFn(func(_ context.Context, mg resource.Managed) error { return nil })), }, }, - want: want{result: reconcile.Result{Requeue: true}}, + want: want{result: reconcile.Result{Requeue: false}}, }, "ResolveReferencesError": { reason: "Errors during reference resolution references should trigger a requeue after a short wait.", @@ -234,7 +234,7 @@ func TestReconciler(t *testing.T) { m: &fake.Manager{ Client: &test.MockClient{ MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { - meta.SetExternalCreateSucceeded(obj, metav1.Now()) + meta.SetExternalCreateSucceeded(obj, time.Now()) return nil }), }, @@ -528,7 +528,7 @@ func TestReconciler(t *testing.T) { MockUpdate: test.NewMockUpdateFn(errBoom), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - meta.SetExternalCreatePending(want, metav1.Now()) + meta.SetExternalCreatePending(want, time.Now()) want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errUpdateManaged))) if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { reason := "Errors while creating an external resource should be reported as a conditioned status." @@ -569,7 +569,8 @@ func TestReconciler(t *testing.T) { MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - meta.SetExternalCreateFailed(want, metav1.Now()) + meta.SetExternalCreatePending(want, time.Now()) + meta.SetExternalCreateFailed(want, time.Now()) want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileCreate))) want.SetConditions(xpv1.Creating()) if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { @@ -614,7 +615,8 @@ func TestReconciler(t *testing.T) { MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - meta.SetExternalCreateSucceeded(want, metav1.Now()) + meta.SetExternalCreatePending(want, time.Now()) + meta.SetExternalCreateSucceeded(want, time.Now()) want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errUpdateManagedAnnotations))) want.SetConditions(xpv1.Creating()) if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { @@ -656,7 +658,8 @@ func TestReconciler(t *testing.T) { MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - meta.SetExternalCreateSucceeded(want, metav1.Now()) + meta.SetExternalCreatePending(want, time.Now()) + meta.SetExternalCreateSucceeded(want, time.Now()) want.SetConditions(xpv1.ReconcileError(errBoom)) want.SetConditions(xpv1.Creating()) if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { @@ -710,7 +713,8 @@ func TestReconciler(t *testing.T) { MockUpdate: test.NewMockUpdateFn(nil), MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { want := &fake.Managed{} - meta.SetExternalCreateSucceeded(want, metav1.Now()) + meta.SetExternalCreatePending(want, time.Now()) + meta.SetExternalCreateSucceeded(want, time.Now()) want.SetConditions(xpv1.ReconcileSuccess()) want.SetConditions(xpv1.Creating()) if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" {