feat: Add Support for Ready Condition (#210)

Description of changes:
   Adding support for a Ready condition that is set after every
   reconciliation standardizes ACK's conditions.
   This condition will serve as a high level status check for users that
   want to use ACK alogside with other controllers (kro).
    
   The existing conditions will remain unchanged, and will still be set on
   the resource as they have been in the past. With this change, you'll be
   seeing a new condition that brings all our conditions together.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: michaelhtm

apis/core/v1alpha1/conditions.go

@@ -23,6 +23,10 @@ import (
 type ConditionType string
 
 const (
+	// ConditionTypeReady indicates the resource is ready to be consumed by other resources.
+	// It is equivalent to the ResourceSynced condition type. Using Ready will standardize
+	// ACK conditions.
+	ConditionTypeReady ConditionType = "Ready"
 	// ConditionTypeAdopted indicates that the adopted resource custom resource
 	// has been successfully reconciled and the target has been created
 	ConditionTypeAdopted ConditionType = "ACK.Adopted"

pkg/condition/condition.go

@@ -29,15 +29,23 @@ var (
 	NotManagedReason  = "This resource already exists but is not managed by ACK. " +
 		"To bring the resource under ACK management, you should explicitly adopt " +
 		"the resource by enabling the ResourceAdoption feature gate and populating " +
-		"the `services.k8s.aws/adoption-policy` and `services.k8s.aws/adoption-fields` " + 
+		"the `services.k8s.aws/adoption-policy` and `services.k8s.aws/adoption-fields` " +
 		"annotations."
 	UnknownSyncedMessage             = "Unable to determine if desired resource state matches latest observed state"
 	NotSyncedMessage                 = "Resource not synced"
+	TerminalMessage                 = "Resource is "
 	SyncedMessage                    = "Resource synced successfully"
 	FailedReferenceResolutionMessage = "Reference resolution failed"
 	UnavailableIAMRoleMessage        = "IAM Role is not available"
 )
 
+// Ready returns the Condition in the resource's Conditions collection that is
+// of type ConditionTypeResourceReady. If no such condition is found, returns
+// nil.
+func Ready(subject acktypes.ConditionManager) *ackv1alpha1.Condition {
+	return FirstOfType(subject, ackv1alpha1.ConditionTypeReady)
+}
+
 // Synced returns the Condition in the resource's Conditions collection that is
 // of type ConditionTypeResourceSynced. If no such condition is found, returns
 // nil.
@@ -126,6 +134,28 @@ func SetSynced(
 	subject.ReplaceConditions(allConds)
 }
 
+func SetReady(
+	subject acktypes.ConditionManager,
+	status corev1.ConditionStatus,
+	message *string,
+	reason *string,
+) {
+	allConds := subject.Conditions()
+	var c *ackv1alpha1.Condition
+	if c = Ready(subject); c == nil {
+		c = &ackv1alpha1.Condition{
+			Type: ackv1alpha1.ConditionTypeReady,
+		}
+		allConds = append(allConds, c)
+	}
+	now := metav1.Now()
+	c.LastTransitionTime = &now
+	c.Status = status
+	c.Message = message
+	c.Reason = reason
+	subject.ReplaceConditions(allConds)
+}
+
 // SetTerminal sets the resource's Condition of type ConditionTypeTerminal to
 // the supplied status, optional message and reason.
 func SetTerminal(

pkg/condition/condition_test.go

@@ -46,6 +46,9 @@ func TestConditionGetters(t *testing.T) {
 	got = ackcond.ReferencesResolved(r)
 	assert.Nil(got)
 
+	got = ackcond.Ready(r)
+	assert.Nil(got)
+
 	conds = append(conds, &ackv1alpha1.Condition{
 		Type:   ackv1alpha1.ConditionTypeResourceSynced,
 		Status: corev1.ConditionFalse,
@@ -60,6 +63,9 @@ func TestConditionGetters(t *testing.T) {
 	got = ackcond.Terminal(r)
 	assert.Nil(got)
 
+	got = ackcond.Ready(r)
+	assert.Nil(got)
+
 	conds = append(conds, &ackv1alpha1.Condition{
 		Type:   ackv1alpha1.ConditionTypeTerminal,
 		Status: corev1.ConditionFalse,
@@ -74,6 +80,9 @@ func TestConditionGetters(t *testing.T) {
 	got = ackcond.Terminal(r)
 	assert.NotNil(got)
 
+	got = ackcond.Ready(r)
+	assert.Nil(got)
+
 	gotAll := ackcond.AllOfType(r, ackv1alpha1.ConditionTypeAdvisory)
 	assert.Empty(gotAll)
 
@@ -106,6 +115,15 @@ func TestConditionGetters(t *testing.T) {
 	r.On("Conditions").Return(conds)
 	got = ackcond.ReferencesResolved(r)
 	assert.NotNil(got)
+
+	conds = append(conds, &ackv1alpha1.Condition{
+		Type:   ackv1alpha1.ConditionTypeReady,
+		Status: corev1.ConditionTrue,
+	})
+	r = &ackmocks.AWSResource{}
+	r.On("Conditions").Return(conds)
+	got = ackcond.Ready(r)
+	assert.NotNil(got)
 }
 
 func TestConditionSetters(t *testing.T) {
@@ -295,4 +313,20 @@ func TestConditionSetters(t *testing.T) {
 		}),
 	)
 	ackcond.WithReferencesResolvedCondition(r, terminalError)
+
+	// Ready condition
+	// SetReady
+	r = &ackmocks.AWSResource{}
+	r.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	r.On(
+		"ReplaceConditions",
+		mock.MatchedBy(func(subject []*ackv1alpha1.Condition) bool {
+			if len(subject) != 1 {
+				return false
+			}
+			return (subject[0].Type == ackv1alpha1.ConditionTypeReady &&
+				subject[0].Status == corev1.ConditionTrue)
+		}),
+	)
+	ackcond.SetReady(r, corev1.ConditionTrue, nil, nil)
 }

pkg/runtime/reconciler.go

@@ -28,6 +28,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/ptr"
 	ctrlrt "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	ctrlrtcontroller "sigs.k8s.io/controller-runtime/pkg/controller"
@@ -582,6 +583,20 @@ func (r *resourceReconciler) ensureConditions(
 		}
 		ackcondition.SetSynced(res, condStatus, &condMessage, &condReason)
 	}
+
+	terminal := ackcondition.Terminal(res)
+	recoverable := ackcondition.Recoverable(res)
+	synced := ackcondition.Synced(res)
+
+	if terminal != nil && terminal.Status == corev1.ConditionTrue {
+		ackcondition.SetReady(res, corev1.ConditionFalse, terminal.Message, ptr.To(string(ackv1alpha1.ConditionTypeTerminal)))
+	} else if recoverable != nil && recoverable.Status == corev1.ConditionTrue {
+		ackcondition.SetReady(res, corev1.ConditionFalse, recoverable.Message, ptr.To(string(ackv1alpha1.ConditionTypeRecoverable)))
+	} else if synced != nil {
+		ackcondition.SetReady(res, synced.Status, synced.Message, synced.Reason)
+	} else {
+		ackcondition.SetReady(res, corev1.ConditionUnknown, &ackcondition.UnknownSyncedMessage, nil)
+	}
 }
 
 // createResource marks the CR as managed by ACK, calls one or more AWS APIs to

pkg/runtime/reconciler_test.go

@@ -505,7 +505,7 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) {
 	latest, latestRTObj, latestMetaObj := resourceMocks()
 	latest.On("Identifiers").Return(ids)
 	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
-		latest.On(
+	latest.On(
 		"ReplaceConditions",
 		mock.AnythingOfType("[]*v1alpha1.Condition"),
 	).Return().Run(func(args mock.Arguments) {
@@ -521,7 +521,12 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) {
 			assert.Equal(ackcondition.SyncedMessage, *condition.Message)
 		}
 		assert.True(hasSynced)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
+
 	latestMetaObj.SetAnnotations(map[string]string{
 		ackv1alpha1.AnnotationAdoptionPolicy: "adopt-or-create",
 		ackv1alpha1.AnnotationAdoptionFields: adoptionFieldsString,
@@ -617,7 +622,11 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveOnce(t *testin
 		cond := conditions[0]
 		assert.Equal(t, ackv1alpha1.ConditionTypeResourceSynced, cond.Type)
 		assert.Equal(t, corev1.ConditionTrue, cond.Status)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ResolveReferences", ctx, nil, desired).Return(
@@ -700,7 +709,11 @@ func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing.
 		cond := conditions[0]
 		assert.Equal(t, ackv1alpha1.ConditionTypeResourceSynced, cond.Type)
 		assert.Equal(t, corev1.ConditionTrue, cond.Status)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ResolveReferences", ctx, nil, desired).Return(
@@ -783,7 +796,11 @@ func TestReconcilerUpdate(t *testing.T) {
 		cond := conditions[0]
 		assert.Equal(t, ackv1alpha1.ConditionTypeResourceSynced, cond.Type)
 		assert.Equal(t, corev1.ConditionTrue, cond.Status)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ResolveReferences", ctx, nil, desired).Return(
@@ -870,7 +887,11 @@ func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) {
 		// False
 		assert.Equal(t, corev1.ConditionFalse, cond.Status)
 		assert.Equal(t, ackcondition.NotSyncedMessage, *cond.Message)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ResolveReferences", ctx, nil, desired).Return(
@@ -954,7 +975,11 @@ func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) {
 		// False
 		assert.Equal(t, corev1.ConditionFalse, cond.Status)
 		assert.Equal(t, ackcondition.NotSyncedMessage, *cond.Message)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ResolveReferences", ctx, nil, desired).Return(
@@ -1033,7 +1058,11 @@ func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) {
 		// True
 		assert.Equal(t, corev1.ConditionTrue, cond.Status)
 		assert.Equal(t, ackcondition.SyncedMessage, *cond.Message)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ResolveReferences", ctx, nil, desired).Return(
@@ -1116,7 +1145,11 @@ func TestReconcilerUpdate_IsSyncedError(t *testing.T) {
 		assert.Equal(t, corev1.ConditionFalse, cond.Status)
 		assert.Equal(t, ackcondition.NotSyncedMessage, *cond.Message)
 		assert.Equal(t, syncedError.Error(), *cond.Reason)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ResolveReferences", ctx, nil, desired).Return(
@@ -1275,7 +1308,11 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) {
 			assert.Equal(ackcondition.SyncedMessage, *condition.Message)
 		}
 		assert.True(hasSynced)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 	// Note no change to metadata...
 
 	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
@@ -1425,7 +1462,11 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) {
 			assert.Equal(requeueError.Error(), *condition.Reason)
 		}
 		assert.True(hasSynced)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ResolveReferences", ctx, nil, desired).Return(
@@ -1545,7 +1586,11 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
 			assert.Equal(ackcondition.NotSyncedMessage, *condition.Message)
 		}
 		assert.True(hasSynced)
-	})
+	}).Once()
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rmf, rd := managerFactoryMocks(desired, latest, false)