Account for two different kinds of consistency issues

This commit is intended to address two issues that we diagnosed while
investigating crossplane-contrib/provider-aws#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 <negz@rk0n.org>

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

pkg/reconciler/managed/reconciler.go

@@ -22,8 +22,6 @@ import (
 	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/util/retry"
 
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -49,7 +47,8 @@ const (
 // Error strings.
 const (
 	errGetManaged               = "cannot get managed resource"
-	errUpdateManagedAnnotations = "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"
@@ -82,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
@@ -284,15 +298,6 @@ type ExternalObservation struct {
 	// determine whether it needs to create or delete the external resource.
 	ResourceExists bool
 
-	// ResourcePending must be true if the corresponding external resource
-	// is suspected to exist, but cannot yet be confirmed to exist.
-	// Typically this is due to an eventually consistent external API with
-	// some amount of delay between when a resource is created and when it
-	// is reported to exist. Returning ResourcePending will cause Crossplane
-	// to wait a while before calling Observe again to determine whether the
-	// resource exists. Supercedes ResourceExists when set.
-	ResourcePending bool
-
 	// ResourceUpToDate should be true if the corresponding external resource
 	// appears to be up-to-date - i.e. updating the external resource to match
 	// the desired state of the managed resource would be a no-op. Keep in mind
@@ -367,8 +372,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
@@ -382,6 +388,7 @@ type Reconciler struct {
 }
 
 type mrManaged struct {
+	CriticalAnnotationUpdater
 	ConnectionPublisher
 	resource.Finalizer
 	Initializer
@@ -390,10 +397,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()),
 	}
 }
 
@@ -431,6 +439,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 {
@@ -439,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 {
@@ -600,6 +628,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.
@@ -650,7 +690,12 @@ 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 observation.ResourcePending {
+	// 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
@@ -736,6 +781,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 {
@@ -746,28 +807,46 @@ 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)
 		}
 
-		meta.SetExternalCreateTime(managed, metav1.Now())
-
-		// We try to persist annotation several times because if we fail
-		// to persist the external name annotation (if necessary) we'll
-		// leak our newly created external resource. Any other changes
-		// made during Create will be reverted; at the time of writing
+		// 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.
-		a := managed.GetAnnotations()
-		if 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.AddAnnotations(managed, a)
-			return r.client.Update(ctx, managed)
-		}); err != nil {
-			log.Debug("Cannot update managed resource", "error", err)
+		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)