Merge pull request #288 from crossplane/backport-283-to-release-0.15

[Backport release-0.15] Account for two different kinds of consistency issues

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,32 @@ 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 (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
+	// 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 +269,90 @@ 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) time.Time {
+	a := o.GetAnnotations()[AnnotationKeyExternalCreatePending]
+	t, err := time.Parse(time.RFC3339, a)
+	if err != nil {
+		return time.Time{}
+	}
+	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 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) time.Time {
+	a := o.GetAnnotations()[AnnotationKeyExternalCreateSucceeded]
+	t, err := time.Parse(time.RFC3339, a)
+	if err != nil {
+		return time.Time{}
+	}
+	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 time.Time) {
+	AddAnnotations(o, map[string]string{AnnotationKeyExternalCreateSucceeded: t.Format(time.RFC3339)})
+}
+
+// GetExternalCreateFailed returns the time at which the external resource
+// recently failed to create.
+func GetExternalCreateFailed(o metav1.Object) time.Time {
+	a := o.GetAnnotations()[AnnotationKeyExternalCreateFailed]
+	t, err := time.Parse(time.RFC3339, a)
+	if err != nil {
+		return time.Time{}
+	}
+	return t
+}
+
+// SetExternalCreateFailed sets the time at which the external resource most
+// recently failed to create.
+func SetExternalCreateFailed(o metav1.Object, t time.Time) {
+	AddAnnotations(o, map[string]string{AnnotationKeyExternalCreateFailed: t.Format(time.RFC3339)})
+}
+
+// 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
+// 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.IsZero() {
+		return false
+	}
+	return time.Since(t) < d
+}
+
 // AllowPropagation from one object to another by adding consenting annotations
 // to both.
 // Deprecated: This functionality will be removed soon.

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,284 @@ func TestSetExternalName(t *testing.T) {
 	}
 }
 
+func TestGetExternalCreatePending(t *testing.T) {
+	now := time.Now().Round(time.Second)
+
+	cases := map[string]struct {
+		o    metav1.Object
+		want time.Time
+	}{
+		"ExternalCreatePendingExists": {
+			o:    &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreatePending: now.Format(time.RFC3339)}}},
+			want: now,
+		},
+		"NoExternalCreatePending": {
+			o:    &corev1.Pod{},
+			want: time.Time{},
+		},
+	}
+
+	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 := time.Now()
+
+	cases := map[string]struct {
+		o    metav1.Object
+		t    time.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 := time.Now().Round(time.Second)
+
+	cases := map[string]struct {
+		o    metav1.Object
+		want time.Time
+	}{
+		"ExternalCreateTimeExists": {
+			o:    &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateSucceeded: now.Format(time.RFC3339)}}},
+			want: now,
+		},
+		"NoExternalCreateTime": {
+			o:    &corev1.Pod{},
+			want: time.Time{},
+		},
+	}
+
+	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 := time.Now()
+
+	cases := map[string]struct {
+		o    metav1.Object
+		t    time.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)}}},
+		},
+	}
+
+	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 := time.Now().Round(time.Second)
+
+	cases := map[string]struct {
+		o    metav1.Object
+		want time.Time
+	}{
+		"ExternalCreateFailedExists": {
+			o:    &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyExternalCreateFailed: now.Format(time.RFC3339)}}},
+			want: now,
+		},
+		"NoExternalCreateFailed": {
+			o:    &corev1.Pod{},
+			want: time.Time{},
+		},
+	}
+
+	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 := time.Now()
+
+	cases := map[string]struct {
+		o    metav1.Object
+		t    time.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)}}},
+		},
+	}
+
+	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, 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, 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 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"

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
@@ -152,3 +155,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)
+}

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)
 			}
 		})
 	}

pkg/reconciler/managed/reconciler.go

@@ -21,9 +21,6 @@ import (
 	"strings"
 	"time"
 
-	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/util/retry"
-
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -43,12 +40,14 @@ const (
 	reconcileTimeout     = 1 * time.Minute
 
 	defaultpollInterval = 1 * time.Minute
+	defaultGracePeriod  = 30 * time.Second
 )
 
 // Error strings.
 const (
 	errGetManaged               = "cannot get managed resource"
-	errUpdateManagedAfterCreate = "cannot update managed resource. this may have resulted in a leaked external resource"
+	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"
@@ -72,6 +71,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 +80,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 +200,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
@@ -316,10 +335,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
@@ -348,8 +371,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
@@ -363,6 +387,7 @@ type Reconciler struct {
 }
 
 type mrManaged struct {
+	CriticalAnnotationUpdater
 	ConnectionPublisher
 	resource.Finalizer
 	Initializer
@@ -371,10 +396,11 @@ type mrManaged struct {
 
 func defaultMRManaged(m manager.Manager) mrManaged {
 	return mrManaged{
-		ConnectionPublisher: NewAPISecretPublisher(m.GetClient(), m.GetScheme()),
-		Finalizer:           resource.NewAPIFinalizer(m.GetClient(), managedFinalizerName),
-		Initializer:         NewNameAsExternalName(m.GetClient()),
-		ReferenceResolver:   NewAPISimpleReferenceResolver(m.GetClient()),
+		CriticalAnnotationUpdater: NewRetryingCriticalAnnotationUpdater(m.GetClient()),
+		ConnectionPublisher:       NewAPISecretPublisher(m.GetClient(), m.GetScheme()),
+		Finalizer:                 resource.NewAPIFinalizer(m.GetClient(), managedFinalizerName),
+		Initializer:               NewNameAsExternalName(m.GetClient()),
+		ReferenceResolver:         NewAPISimpleReferenceResolver(m.GetClient()),
 	}
 }
 
@@ -412,6 +438,17 @@ 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. All
+// resources have a 30 second grace period by default.
+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 {
@@ -420,6 +457,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 {
@@ -483,14 +530,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 {
@@ -581,6 +629,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 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
 	// in some rare examples we need a spec field to make the observe call, and
 	// that spec field could be set by a reference.
@@ -631,6 +690,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())
 		managed.SetConditions(xpv1.Deleting())
@@ -711,6 +781,21 @@ 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, 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)))
+			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 {
@@ -721,30 +806,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 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)))
+
+				// 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
+		// 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, 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)))
+			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 {

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"
@@ -199,6 +201,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.Time)
+							return nil
+						}),
+						MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+							want := &fake.Managed{}
+							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)
+							}
+							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: false}},
+		},
 		"ResolveReferencesError": {
 			reason: "Errors during reference resolution references should trigger a requeue after a short wait.",
 			args: args{
@@ -288,6 +319,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, time.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{
@@ -558,17 +617,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, 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."
 								t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff)
 							}
@@ -598,17 +658,109 @@ 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.",
+			args: args{
+				m: &fake.Manager{
+					Client: &test.MockClient{
+						MockGet:    test.NewMockGetFn(nil),
+						MockUpdate: test.NewMockUpdateFn(nil),
+						MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+							want := &fake.Managed{}
+							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 != "" {
+								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
+						}),
+					},
+					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, mg resource.Managed) (ExternalClient, error) {
+						c := &ExternalClientFns{
+							ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
+								return ExternalObservation{ResourceExists: false}, nil
+							},
+							CreateFn: func(_ context.Context, _ resource.Managed) (ExternalCreation, error) {
+								return ExternalCreation{}, errBoom
+							},
+						}
+						return c, nil
+					})),
+					// 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}},
+		},
+		"UpdateCriticalAnnotationsError": {
+			reason: "Errors updating critical annotations after creation should trigger a requeue after a short wait.",
+			args: args{
+				m: &fake.Manager{
+					Client: &test.MockClient{
+						MockGet:    test.NewMockGetFn(nil),
+						MockUpdate: test.NewMockUpdateFn(nil),
+						MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+							want := &fake.Managed{}
+							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 != "" {
+								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
+						}),
+					},
+					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, mg resource.Managed) (ExternalClient, error) {
+						c := &ExternalClientFns{
+							ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
+								return ExternalObservation{ResourceExists: false}, nil
+							},
+							CreateFn: func(_ context.Context, _ resource.Managed) (ExternalCreation, error) {
+								return ExternalCreation{}, nil
+							},
+						}
+						return c, nil
+					})),
+					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}},
+		},
 		"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: 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{}
+							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()); diff != "" {
+							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)
 							}
@@ -633,6 +785,7 @@ func TestReconciler(t *testing.T) {
 						}
 						return c, nil
 					})),
+					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
@@ -654,12 +807,15 @@ func TestReconciler(t *testing.T) {
 			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{}
+							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()); 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)
 							}
@@ -673,139 +829,7 @@ func TestReconciler(t *testing.T) {
 					WithInitializers(),
 					WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
 					WithExternalConnecter(&NopConnecter{}),
-					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.",
-			args: args{
-				m: &fake.Manager{
-					Client: &test.MockClient{
-						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.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(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
-					})),
-					WithConnectionPublishers(),
-					WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}),
-				},
-			},
-			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.",
-			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
-						},
-						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)))
-							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(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
-					})),
-					WithConnectionPublishers(),
-					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.",
-			args: args{
-				m: &fake.Manager{
-					Client: &test.MockClient{
-						MockGet:    test.NewMockGetFn(nil),
-						MockUpdate: test.NewMockUpdateFn(errBoom),
-						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)))
-							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(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
-					})),
+					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 }}),
 				},