Terminal condition sets ResourceSynced=False (#94)

This patch updates the `resourceReconciler.ensureConditions()` method to
set the `ACK.ResourceSynced` Condition value to `False` when a Terminal
error has been returned from the resource manager. Previously, the
`ACK.ResourceSynced` Condition value was erroneously being set to `True`
due to a misunderstanding of what `ACK.ResourceSynced` means.

`ACK.ResourceSynced` is a Condition that informs the Kubernetes user
*whether the desired state of a resource matches the latest observed
state of the resource*. In the case of a Terminal error, the desired
state of a resource will *never* match the latest observed state of the
resource because there is something invalid about the desired resource
state.

Further, this patch changes the default value for `ACK.ResourceSynced`
Condition to be `Unknown` instead of `False` for any non-Terminal
reconciler error. The reasoning behind this change is that for
non-Terminal errors, we simply do not know whether the desired resource
state matches the latest observed state or not.

Signed-off-by: Jay Pipes <[email protected]>

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

Author: jaypipes

pkg/condition/condition.go

@@ -29,8 +29,9 @@ 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 creating a services.k8s.aws/AdoptedResource"
-	NotSyncedMessage = "Resource not synced"
-	SyncedMessage    = "Resource synced successfully"
+	UnknownSyncedMessage = "Unable to determine if desired resource state matches latest observed state"
+	NotSyncedMessage     = "Resource not synced"
+	SyncedMessage        = "Resource synced successfully"
 )
 
 // Synced returns the Condition in the resource's Conditions collection that is

pkg/runtime/reconciler.go

@@ -326,15 +326,19 @@ func (r *resourceReconciler) ensureConditions(
 		if reconcileErr != nil {
 			condReason = reconcileErr.Error()
 			if reconcileErr == ackerr.Terminal {
-				// A terminal condition by its very nature indicates a stable state
-				// for a resource being synced. The resource is considered synced
-				// because its state will not change.
-				condStatus = corev1.ConditionTrue
-				condMessage = ackcondition.SyncedMessage
-			} else {
-				// For any other reconciler error, set synced condition to false
+				// A terminal condition is a stable state for a resource.
+				// Terminal conditions indicate that without changes to the
+				// desired state of a resource, the resource's desired state
+				// will never match the latest observed state. Thus,
+				// ACK.ResourceSynced must be False.
 				condStatus = corev1.ConditionFalse
 				condMessage = ackcondition.NotSyncedMessage
+			} else {
+				// For any other reconciler error, set synced condition to
+				// unknown, since we don't know whether the resource's desired
+				// state matches the resource's latest observed state.
+				condStatus = corev1.ConditionUnknown
+				condMessage = ackcondition.UnknownSyncedMessage
 			}
 		}
 		ackcondition.SetSynced(res, condStatus, &condMessage, &condReason)

pkg/runtime/reconciler_test.go

@@ -994,9 +994,10 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) {
 			hasSynced = true
 			// Even though mocked IsSynced method returns (true, nil),
 			// the reconciler error from late initialization correctly causes
-			// the ResourceSynced condition to be False
-			assert.Equal(corev1.ConditionFalse, condition.Status)
-			assert.Equal(ackcondition.NotSyncedMessage, *condition.Message)
+			// the ResourceSynced condition to be Unknown since the reconciler
+			// error is not a Terminal error.
+			assert.Equal(corev1.ConditionUnknown, condition.Status)
+			assert.Equal(ackcondition.UnknownSyncedMessage, *condition.Message)
 			assert.Equal(requeueError.Error(), *condition.Reason)
 		}
 		assert.True(hasSynced)
@@ -1112,9 +1113,9 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
 			}
 			hasSynced = true
 			// The terminal error from reconciler correctly causes
-			// the ResourceSynced condition to be True
-			assert.Equal(corev1.ConditionTrue, condition.Status)
-			assert.Equal(ackcondition.SyncedMessage, *condition.Message)
+			// the ResourceSynced condition to be False
+			assert.Equal(corev1.ConditionFalse, condition.Status)
+			assert.Equal(ackcondition.NotSyncedMessage, *condition.Message)
 		}
 		assert.True(hasSynced)
 	})