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/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.

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"